fix(noir-contracts): deliver notes via onchain_unconstrained in app/deployment contracts#24333
Closed
AztecBot wants to merge 2 commits into
Closed
fix(noir-contracts): deliver notes via onchain_unconstrained in app/deployment contracts#24333AztecBot wants to merge 2 commits into
AztecBot wants to merge 2 commits into
Conversation
…eployment contracts These account, token and benchmarking contracts delivered their notes with MessageDelivery::onchain_constrained(). That mode was effectively a no-op on v5-next, but #23866 (wire constrained message delivery) made it issue a real validate_handshake private call on the first send (index 0) of each (sender, recipient, secret) sequence. That extra app circuit broke the full (merge-queue-only) client_flows step-count benches once v5-next merged into merge-train/fairies-v5, dequeuing #24223: - account_deployments: 10 -> 11 execution steps - amm add_liquidity: 14 -> 17 execution steps and it advanced the per-secret tagging index until bench_build_block exceeded UNFINALIZED_TAGGING_INDEXES_WINDOW_LEN (20), failing every tx commit until the 600s timeout. These flows test app/deployment logic and don't need constrained delivery (constrained delivery has dedicated coverage in constrained_delivery_test_contract), so deliver via onchain_unconstrained instead -- the behavior these benches were written against. No benchmark expectations change: they already encode the unconstrained (no-bootstrap) step counts. The warm-send benches (transfers, bridging) were unaffected either way, and the dedicated constrained_delivery_test_contract is intentionally left as-is.
ba4c36f to
78d11d1
Compare
Reflects the onchain_constrained -> onchain_unconstrained delivery change in token_contract. Only token_contract is in EXPAND_CASES among the changed contracts; the call text is verbatim in nargo expand output, so the snapshot diff is exactly the six delivery-mode lines.
Contributor
|
Superceded by #24336, will do a test token contract split separately |
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.
What
Switch the note delivery in the account (
ecdsa_r/ecdsa_k/schnorr),tokenandbenchmarkingcontracts fromMessageDelivery::onchain_constrained()toMessageDelivery::onchain_unconstrained(). No benchmark expectations are changed.(Supersedes this PR's earlier approach of bumping the
expectedExecutionSteps(...)inputs — per the fairies team the extra step isn't wanted here.)Why
#24223 (
merge-train/fairies-v5→v5-next) kept getting dequeued. After the compile fix (#24323), the full merge-queue lane failed on three benches, all traced to one cause:client_flows/account_deployments—Expected 10 execution steps, got 11client_flows/amm(add_liquidity) —Expected 14 execution steps, got 17bench_build_block— repeatedFailed to commit transaction: … Tagging window length 20 is configured too low→ 600s timeoutThese contracts delivered notes with
onchain_constrained(), which was effectively a no-op on v5-next. #23866 — wire constrained message delivery (on the train) made that mode issue a realvalidate_handshakeprivate call on the first send (index 0) of each(sender, recipient, secret)sequence. Once v5-next merged into the train:bench_build_block(which emits many notes to one recipient before finalization) blew pastUNFINALIZED_TAGGING_INDEXES_WINDOW_LEN = 20(yarn-project/pxe/src/tagging/constants.ts:10), failing every tx commit until timeout.These flows test app/deployment logic and don't need constrained delivery — that has dedicated coverage in
constrained_delivery_test_contract. So deliver viaonchain_unconstrained(), the behavior these benches were written against.Why no benchmark-expectation changes
The
onchain_constrained()usage and the bench expectations both came from v5-next, where the mode was a no-op — so the existingexpectedExecutionSteps(...)inputs already encode the unconstrained (no-bootstrap) counts. Reverting the contracts to unconstrained makes the actual counts match them again (account_deployments → 10, amm → 14). The warm-send benches (transfers,bridging) never hit a cold bootstrap, so they're unaffected either way.Scope / verification
ecdsa_kfor account-contract consistency). The dedicatedconstrained_delivery_test_contractis intentionally left untouched.client_flowsbenches run only in the full/merge-queue lane, not in this PR's fast CI, so the authoritative check is re-queuing feat: merge-train/fairies-v5 #24223 (or aCI_FULLrun). A local build/run wasn't possible in this container (no docker/cache; needs the noir submodule).nft,escrow,counter,simple_token, …) still useonchain_constrained(). If you want the full "unconstrained everywhere" sweep (which also touches e2e discovery tests), say the word and I'll extend it — kept scoped here to what unblocks feat: merge-train/fairies-v5 #24223.cc @vezenovm @nicolas-venturo — this implements the
onchain_unconstrainedswitch you described.