refactor: stabilise bench-tx.json keys and consolidate cost-table tests - #3726
Open
mmagician wants to merge 10 commits into
Open
refactor: stabilise bench-tx.json keys and consolidate cost-table tests#3726mmagician wants to merge 10 commits into
mmagician wants to merge 10 commits into
Conversation
`checked_in_cost_matches_benched_cycles` listed its notes as 22 hand-written rstest cases, mirroring the 23 entries of `PricedNote::all`. The CONSTANT_FEE_POLICY_CONFIG case was missing, leaving `CONSTANT_FEE_POLICY_CONFIG_CONSUMPTION_CYCLES` with no drift guard: the other tests over `PricedNote::all` cover the note, but neither checks the constant's value. Replace the case list with a loop over `PricedNote::all`, so a note added to the tables cannot go unchecked - the generator renders the tables from that same list. Drift is collected across all the notes before it is asserted on, so one run still reports every stale constant. `rstest` was this crate's only use of the dev-dependency and is dropped with it. Wall clock is unchanged: `created_notes_cover_executed_output_notes` already executes every scenario serially in a single test and dominates the module's runtime; the two now run concurrently at ~47s each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LXcU9HrrmWTCTZTJZnuLSU
Co-authored-by: Marti <marcin.gorny.94@protonmail.com>
mmagician
commented
Aug 25, 2026
mmagician
marked this pull request as ready for review
August 25, 2026 12:02
A note ID commits to the note's recipient, so any edit to an inlined note script renames it. That makes note IDs unusable as keys in a checked-in artifact. Add `NoteLabels`, which names each input note by its kind - resolved from its script root - suffixed with an occurrence index when a scenario consumes several notes of that kind. Not wired into the snapshot writer yet; the next commit switches `bench-tx.json` over to these labels. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LXcU9HrrmWTCTZTJZnuLSU
`note_execution` was keyed by the hex of a note commitment, so an inlined
MASM change renamed every entry and re-sorted the map - a diff that reads
as large cycle swings when the underlying counts barely moved. Emit an
array of `{ note, cycles }` in consumption order instead, labelled via
`NoteLabels`. A measured note that does not join against the transaction's
input notes aborts the run rather than being emitted unlabelled.
The join is on the note's details commitment, because that is what a
measurement entry carries despite being typed `NoteId`; see the TODO on
`measured_note_key` and #3724, which fixes the host to report the real ID.
Also record each scenario's `total_cycles`, the figure the checked-in cost
tables are derived from, so the snapshot carries it alongside the stages.
Regenerating `bench-tx.json` follows in the next commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LXcU9HrrmWTCTZTJZnuLSU
One-time rewrite of the whole artifact: every scenario gains `total_cycles` and its `note_execution` becomes an ordered array keyed by note label. The `trace` section - the part miden-vm's synthetic bench consumes - is unchanged in shape. The cost tables are deliberately left alone. Regenerating them here would move every constant, but every move is sub-threshold (worst is BURN at -3.1% against the 5% drift tolerance), so the guard never asked for it and this refactor has no business shifting fee-affecting values. Also extend `committed_bench_tx_matches_trace_contract` to cover the two new sections, so a stale or hand-edited artifact fails instead of passing on the `trace` section alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LXcU9HrrmWTCTZTJZnuLSU
`checked_in_cost_matches_benched_cycles` and `created_notes_cover_executed_output_notes` each walked `PricedNote::all` and executed all ~31 priced scenarios independently, so a test run paid for ~62 transaction executions to assert two properties of the same `NoteCost`. Merge them: execute each scenario once and assert the cycle count and the `created_notes` declaration off that result. Failures still accumulate across all notes before being asserted on, and the two kinds are reported separately - a stale cost table and a wrong `created_notes` list need different fixes. The shared execution pass also validates the label join `bench-tx.json`'s generator depends on, which nothing else exercises against a real transaction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LXcU9HrrmWTCTZTJZnuLSU
Collapses them in GitHub's PR diff and drops them from the repo's language stats. Limited to files something automated is already checking, since collapsing a diff takes a reviewer's eyes off it: `bench-tx.json` is parsed by `committed_bench_tx_matches_producer_contract`, and `Cargo.lock` is gated by `cargo deny`. The note cost tables stay uncollapsed - their constants reach `NetworkNotePricer` - as do the agglayer Solidity test vectors, which are the expected-value side of the bridge conformance tests and have no CI job regenerating them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LXcU9HrrmWTCTZTJZnuLSU
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LXcU9HrrmWTCTZTJZnuLSU
Co-authored-by: Marti <marcin.gorny.94@protonmail.com>
`cargo doc` denies `rustdoc::private_intra_doc_links`, so the module doc and two public methods failed to document while pointing at `measured_note_key` and `UNKNOWN_NOTE_LABEL`. Both stay crate-private, so drop the links and name them in plain code spans. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LXcU9HrrmWTCTZTJZnuLSU
mmagician
force-pushed
the
mmagician-claude/stabilise-bench-tx-keys
branch
from
August 25, 2026 13:57
d5e3327 to
632b757
Compare
Base automatically changed from
mmagician-claude/note-cost-drift-guard-from-priced-notes
to
next
August 26, 2026 14:14
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #3714. Review the top four commits.
PRs touching MASM or kernel code carry large generated diffs, and
bench-tx.json's share of them is mostly noise. Itsnote_executionmap was keyed by a note commitment hex and stored in aBTreeMap, so any change to an inlined note script renamed every key and re-sorted the section.In #3699 both P2ID notes in the two-note scenario moved by exactly +22 cycles (2109→2131, 2324→2346), but because the new hexes sorted the other way the diff read as
2109 → 2346(+11%) and2324 → 2131(−8%). A reviewer scanning line by line drew the opposite conclusion from the truth.What changes
note_executionbecomes an ordered array keyed by note kind:Labels come from the note's script root, so they move only when the note's kind, its multiplicity in the transaction, or its position among same-kind notes changes. Array order is the order the kernel measured the notes, so it never re-sorts.
Each scenario also gains
total_cycles- the figure the cost tables are derived from - so the snapshot carries it alongside the per-stage counts.Cost tables are deliberately untouched
Regenerating would move every constant, but every move is within the 5% drift tolerance (worst:
BURNat −3.06%), so the guard never asked for it. A refactor has no business shifting fee-affecting values. One consequence worth knowing:make update-note-costsis therefore not a no-op on this branch — it rewrites the tables with that accumulated sub-threshold drift. That is pre-existing onnextand is the churn the follow-up PR's policy addresses.A bug this surfaced
The join needed real note IDs and did not get them:
TransactionMeasurements::note_executionis typedVec<(NoteId, usize)>but carries each note's details commitment, because the host reads the input note segment's base word instead ofINPUT_NOTE_ID_OFFSET. Fixed separately in #3724, based onnext.This PR does not depend on that fix. It joins on the details commitment, isolated in
measured_note_keywith a TODO stating the premise, linking #3724, and warning that the migration is all-or-nothing — every lookup misses at once, so whoever lands #3724 must update this in the same PR. A details commitment is weaker than a note ID (it excludes the metadata the nullifier binds), so duplicates are rejected rather than silently misattributed.Test consolidation
checked_in_cost_matches_benched_cyclesandcreated_notes_cover_executed_output_noteseach walkedPricedNote::alland executed ~31 scenarios independently. Merged intochecked_in_note_costs_match_executed_scenarios, which executes each once and asserts both properties: ~62 transaction executions per test run become ~31. The two failure kinds are still reported separately. That pass also validates the label join, which nothing else exercises against a real transaction..gitattributesbench-tx.jsonandCargo.lockare markedlinguist-generated=true. Limited to files something automated already checks, since collapsing a diff takes a reviewer's eyes off it. The cost tables and the agglayer Solidity test vectors are deliberately excluded — the latter are bridge conformance oracles regenerated only by a manualmake generate-solidity-test-vectors, with no CI job watching them.Verification
make test-release: 1886 passedmake lintcleanrange_rowson 7 scenarios, the run-to-run variance the README documents. Labels, structure and all cycle counts identical.🤖 Generated with Claude Code
https://claude.ai/code/session_01LXcU9HrrmWTCTZTJZnuLSU