[scratch] e2e: aztec-benchmark block-duration-ms (do not merge) - #45
Closed
alejoamiras wants to merge 3 commits into
Closed
alejoamiras wants to merge 3 commits into
alejoamiras wants to merge 3 commits into
Conversation
The conversions compute `a * b / denominator`. Done natively in u128 the intermediate product overflows for large-but-legitimate inputs; Noir range-checks u128, so the transaction reverts rather than wrapping. That is an availability bug (audit F-004): a vault whose totals reach the range can no longer be deposited to or withdrawn from, permanently locking every participant's funds. Both sites carried TODOs. New `conversion.nr` module with a `mul_div` primitive that widens both operands to noir-bignum's U256 (the same library the escrow's key derivation already uses), multiplies and divides there, then narrows the quotient back to u128, asserting it fits. U256 arithmetic is modulo 2^256 and the largest possible product, (2^128-1)^2, is 2^129-1 short of that modulus, so the product is always exact — no modular wraparound. Rounding is unchanged: both old and new return floor(p/d) + (round_up && p%d != 0). For every input the old code accepted the results are identical; the widening only extends the domain that succeeds. This matters because the vault's economic safety depends on rounding always favouring the vault — a shift in either direction would leak value between the vault and its depositors. `mul_div` also rejects a zero denominator explicitly: noir-bignum's constrained udiv_mod fails on it, but its unconstrained path assumes non-zero and would return a meaningless witness. Unreachable from the vault (denominators are total_assets+1 and total_supply+vault_offset with vault_offset >= 1) but the helper is now safe in isolation. Extracting mul_div into its own module is what makes the overflow boundary testable at all: a real vault cannot be driven to a 2^128 supply in a test, but the primitive can be called directly at its edges. Also adds `ensureVaultContractClassPublished` to the JS test utils. Publishing just the class is what the vault tests actually need and is substantially cheaper in DA gas than deploying a throwaway Vault to get the class published as a side effect. It is idempotent — publication emits a nullifier keyed on the class id, so a second publish of the same class is rejected with "Existing nullifier"; the helper checks registration state first, the same way DeployMethod does. Validated: vault_contract 195 Noir tests (188 pre-existing all still green — the strongest evidence rounding did not shift — plus 7 new covering limb round-trips, rounding direction both ways, products that previously overflowed, max operands, and the two revert guards). aztec compile OK. Codex adversarial review: correct, rounding invariance proven algebraically, no value-leak path; its zero-denominator hardening is applied. Note: the vault README still describes the overflow as a known issue. That warning block is rewritten in PR #24 (unmerged); leaving it there avoids a three-way conflict. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`aztec start --local-network` defaults to 3s blocks. With 72s slots that packs 21 blocks into a checkpoint, and the per-tx DA admission limit is `ceil(daBudget / blocks * 1.5)` = 55,882 DA gas. Mainnet runs 6s blocks (10 blocks per checkpoint) and admits 117,668. The protocol picks that 1.5 multiplier deliberately, and says so in aztec stdlib `gas/tx_gas_limits.ts`: it is set "so the largest tx we want to support — a maximal contract class registration (~97k DA gas) — fits a single block under v5 mainnet geometry (72s slots, 6s blocks -> 10 blocks per checkpoint)". The default local geometry therefore advertises a limit well below the ~97k the protocol guarantees for exactly this kind of transaction, and rejects contracts that are valid on the network we ship to. Publishing the Vault contract class costs ~64k DA gas. That is fine on mainnet and inside the protocol's stated envelope, but over the local 55,882 cap. Note this is not specific to any one change: main's Vault already sits at ~54k, i.e. 97% of the local ceiling, so essentially any growth in that contract trips it. Reusable workflows do not inherit the caller's `env`, and aztec-ci-actions' run-tests.yml exposes no knob for this, so the JS job is inlined here (`run-js-tests: false` on the reusable call) purely to own the environment. It still calls the same pinned `setup-aztec` and `js-tests` composite actions, so behaviour is otherwise unchanged. NOTE FOR REVIEWERS: this renames the check from "checks / JS Tests" to "JS Tests". Any branch protection rule naming the old check needs updating, or it will block merges waiting on a check that no longer runs. The better long-term fix is upstream — either the local network should default to mainnet geometry, or run-tests.yml should expose the knob. Worth raising with the Aztec CI folks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… merge) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Benchmark Comparison
Contract: escrow
Contract: logic
Contract: multitoken
Contract: nft
Contract: token
Contract: vault
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Collaborator
Author
|
E2E done: J1 (6000) passed with the Vault class published, J2 (default) failed at the 55,836 DA cap, J3 ("6s") was rejected in Configure without echoing the value, and J4 (update-baseline) passed and uploaded a branch-keyed artifact. The branch stays until aztec-benchmark releases, in case a re-run is needed. |
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
1 participant
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.
Scratch PR, do not merge. It will be closed once the checks report.
This is a live e2e of the new
block-duration-msinput in AztecProtocol/aztec-benchmark atd623993(branchworktree-aztec-v6-update). It runs on top of #38, whose Vault class publish needs 63,776 DA gas; the local network's default 3s blocks cap a transaction at 55,836."6000"): expected to pass, with the Vault class published."6s"): expected to fail in "Configure block duration".update-baseline.ymlpinned to the same commit with"6000", dispatched on this branch. Its artifact is keyed by this branch, so main's baseline is untouched.🤖 Generated with Claude Code