Skip to content

Improve Benchmarking#2798

Open
JohnReedV wants to merge 62 commits into
mainfrom
apply-benchmark-to-only-bad-benches
Open

Improve Benchmarking#2798
JohnReedV wants to merge 62 commits into
mainfrom
apply-benchmark-to-only-bad-benches

Conversation

@JohnReedV

@JohnReedV JohnReedV commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Description

This PR

  • Modifies the benchmark flow so that only extrinsics that have drifted passed the threshold or have incorrect read/writes are updated rather than replacing the entire file and creating conflicts everywhere.

  • Adds a lint to check for extrinsics with missing benchmarks. This can be hidden with
    #[allow(unknown_lints, benchmarked_weight_not_plugged)]

  • Adds 48 missing benchmarks

  • Fixes a bug where benchmarks could fail on noisy internal component drift even when the actual benchmark-level weight was still within the configured threshold.

  • Adds block_step & reveal_timelocked_commitments to the benchmark flow so the on_initialize weight is generated instead of hardcoded. The block_step benchmark simulates the current worst case with 2 epochs in a block.

  • Updates the drift check to include proof size and to compare the actual benchmark-level weight instead of failing on noisy internal component drift.

@JohnReedV JohnReedV added the skip-cargo-audit This PR fails cargo audit but needs to be merged anyway label Jun 25, 2026
@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

🛡️ AI Review — Skeptic (security review)

VERDICT: VULNERABLE

Baseline scrutiny: established @opentensor-associated contributor with substantial history; cached repository permission is read, no Gittensor association found; apply-benchmark-to-only-bad-benches -> main.

No malicious intent was established. Static analysis found one unresolved, merge-blocking resource-accounting vulnerability.

Findings

Sev File Finding
HIGH pallets/commitments/src/lib.rs:346 Fixed hook weight undercharges multiple timelock reveals inline

Prior-comment reconciliation

  • 31216722: not addressed — The generated reveal CPU weight remains fixed and is added once, while the hook can perform cryptographic work for multiple indexed commitments; its dynamic weight covers only database operations.

Conclusion

The hook can perform repeated cryptographic reveals while charging the benchmarked CPU cost only once, allowing actual block execution to exceed its declared weight.


📜 Previous run (superseded)
Sev File Finding Status
HIGH pallets/commitments/src/lib.rs:346 Fixed hook weight undercharges multiple timelock reveals ➡️ Carried forward to current findings
The generated reveal CPU weight remains fixed and is added once, while the hook can perform cryptographic work for multiple indexed commitments; its dynamic weight covers only database operations.

🔍 AI Review — Auditor (domain review)

VERDICT: 👍

Gittensor UNKNOWN; author is an established @opentensor/subtensor contributor with substantial recent PR history, cached repo permission is read.

PR body is substantive and matches the benchmark/tooling scope, so I would leave it unchanged.

The prior register_limit concern remains addressed: the benchmark still uses setup_full_subnet_registration_benchmark, fills the subnet, funds the caller, and dispatches against the replacement path rather than the cheap append path.

Duplicate-work check found overlapping PRs only through broad shared runtime, benchmark, and generated-weight files; none appear to be alternate implementations of this benchmark-flow change.

Validation performed: bash -n scripts/benchmark_action.sh, bash -n scripts/benchmark_all.sh, bash -n scripts/discover_pallets.sh, python3 -m py_compile scripts/benchmark_action.py, scripts/discover_pallets.sh, and git diff --check origin/devnet-ready...HEAD passed. Targeted Rust tests could not start because rustup attempted to create files under the read-only /home/runner/.rustup; live devnet spec_version comparison also could not complete because DNS resolution for dev.chain.opentensor.ai failed. No auto-fix changes were made.

Findings

No findings.

Prior-comment reconciliation

  • 2caec2f2: addressedregister_limit still benchmarks through setup_full_subnet_registration_benchmark, filling the subnet before dispatch so the measured path reaches replacement rather than append.

Conclusion

I did not find a remaining domain issue that should block merge. CI should still perform the normal live spec-version and Rust test checks that this sandbox could not complete.

@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@JohnReedV JohnReedV changed the title Apply Benchmark patch only where its needed Improve Benchmarking Jun 26, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

5 similar comments
@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment thread support/weight-tools/src/weight_compare.rs
@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👎

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment thread pallets/subtensor/src/weights.rs
@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👎

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment thread pallets/subtensor/src/weights.rs
Comment thread pallets/admin-utils/src/weights.rs
Comment thread pallets/swap/src/weights.rs
@UnArbosSix
UnArbosSix force-pushed the apply-benchmark-to-only-bad-benches branch from 51345f0 to a1f4ff1 Compare July 17, 2026 15:58
@github-actions

Copy link
Copy Markdown
Contributor

eco-tests changed — indexer review required

This PR modifies files under eco-tests/. and may affect downstream indexing.
cc @evgeny-s — please review manually

Changed files
  • eco-tests/Cargo.toml
  • eco-tests/src/lib.rs
  • eco-tests/src/mock.rs
  • eco-tests/src/tests_mentat_indexer.rs
  • eco-tests/src/tests_taocom_indexer.rs

@github-actions
github-actions Bot requested a review from evgeny-s July 17, 2026 15:58
@UnArbosSix
UnArbosSix changed the base branch from devnet-ready to main July 17, 2026 15:59
@UnArbosSix UnArbosSix added the run-benchmarks run benchmarks label Jul 17, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment thread pallets/commitments/src/lib.rs
@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: VULNERABLE

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment thread pallets/commitments/src/lib.rs
@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: VULNERABLE

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment thread pallets/commitments/src/lib.rs
@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: VULNERABLE

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment thread pallets/commitments/src/lib.rs
@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: VULNERABLE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-benchmarks run benchmarks skip-cargo-audit This PR fails cargo audit but needs to be merged anyway

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants