feat(agglayer): process CLAIM notes from a rollup#2573
Conversation
Support rollup deposits (mainnet_flag=0) in bridge-in verification. Previously only mainnet deposits were supported and rollup deposits would panic. Rollup deposits use two-level Merkle proof verification: first computing the local exit root from the leaf, then verifying it against the rollup exit root. Closes #2394 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
90d0479 to
54d7d66
Compare
…pers Address review feedback: - Remove leading zeros assertion and mainnet flag validation from process_global_index_mainnet and process_global_index_rollup helpers. These are now done once in verify_leaf before branching. - The helpers now take [rollup_index_le, leaf_index_le] instead of the full 8-element global index. - In process_global_index_rollup, removed unnecessary byte-swap before asserting zero (zero is byte-order-independent). This is now moot since the mainnet flag is no longer checked in the helper. - Updated MASM unit tests to match the new helper signatures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
54d7d66 to
cb4627e
Compare
There was a problem hiding this comment.
Pull request overview
Adds support for processing CLAIM notes originating from rollups (mainnet_flag=0) by introducing rollup-aware global index handling and two-level Merkle proof verification, plus corresponding test vectors and integration coverage.
Changes:
- Added rollup deposit test vectors and enabled bridge-in tests to run against them.
- Introduced rollup global index processing in MASM and added Rust
GlobalIndex::validate_rollup()/validate()helpers. - Added Solidity generator contract to produce rollup two-level Merkle proof vectors.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/miden-testing/tests/agglayer/test_utils.rs | Adds rollup claim vector JSON + lazy parsed vector and exposes it via ClaimDataSource. |
| crates/miden-testing/tests/agglayer/global_index.rs | Extends MASM global index tests to cover both mainnet and rollup procedures. |
| crates/miden-testing/tests/agglayer/bridge_in.rs | Runs bridge-in integration test for rollup data source and creates destination account for rollup case. |
| crates/miden-agglayer/src/eth_types/global_index.rs | Adds rollup validation + shared leading-bits check + dispatching validate(), plus unit tests. |
| crates/miden-agglayer/solidity-compat/test/ClaimAssetTestVectorsRollupTx.t.sol | Adds Foundry test that generates rollup two-level Merkle proof vectors. |
| crates/miden-agglayer/solidity-compat/test-vectors/claim_asset_vectors_rollup_tx.json | Adds generated rollup test vector JSON consumed by Rust tests. |
| crates/miden-agglayer/asm/agglayer/bridge/bridge_in.masm | Adds process_global_index_rollup and rollup branch to verify_leaf using two-level verification. |
Comments suppressed due to low confidence (1)
crates/miden-agglayer/asm/agglayer/bridge/bridge_in.masm:1
movdn.noperates on the stack top, but at this point the stack top isleaf_index, notrollup_index. As written,movdn.9will moveleaf_indexdown (and the latermovdn.9 movdn.9will reshuffle again), which makes the comments about the resulting stack state inconsistent and can easily lead to callingcalculate_rootwith mis-ordered arguments. This is especially risky because the current rollup test vector usesrollup_index=0andleaf_index=0, so an ordering bug here can be masked. Rework these stack moves so thatrollup_index(depth 1) is the value being moved/preserved (e.g., bring it to the top first with aswap/movupand only then move it down past the leaf value), and update the stack-shape comments to match the actual effects of the instructions.
use miden::agglayer::bridge::bridge_config
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/miden-agglayer/solidity-compat/test/ClaimAssetTestVectorsRollupTx.t.sol
Outdated
Show resolved
Hide resolved
crates/miden-agglayer/solidity-compat/test/ClaimAssetTestVectorsRollupTx.t.sol
Show resolved
Hide resolved
crates/miden-agglayer/solidity-compat/test-vectors/claim_asset_vectors_rollup_tx.json
Outdated
Show resolved
Hide resolved
Include the auto-generated ERR_MAINNET_FLAG_INVALID constant in the committed errors file to fix the generated files CI check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use non-zero leafIndex (2) and indexRollup (5) in the rollup test vectors to exercise byte-ordering and stack manipulation paths. This exposed a bug in verify_leaf's rollup branch: the stack rearrangement after process_global_index_rollup had leaf_index and rollup_index in the wrong positions, which was masked when both were zero. Also restructure the rollup exit tree test helper to use an SMT-style approach (setLocalExitRootAt) matching the real PolygonRollupManager.getRollupExitRoot() construction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Split ERR_BRIDGE_NOT_MAINNET into two errors: - ERR_BRIDGE_NOT_MAINNET: "mainnet flag must be 1 for a mainnet deposit" - ERR_BRIDGE_NOT_ROLLUP: "mainnet flag must be 0 for a rollup deposit" The previous error text "bridge not mainnet" read backwards when used in the rollup helper to reject a mainnet flag. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix stale comment in verify_leaf that incorrectly referenced "stack position 2" for the mainnet flag (it's at position 5) - Add explicit MASM test for the mainnet flag boolean validation (ERR_MAINNET_FLAG_INVALID) which rejects flag values >= 2 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix comment referencing non-existent MAINNET_FLAG_STACK_POS constant - Remove test_mainnet_flag_rejects_invalid_value: it inlined the same logic as verify_leaf rather than testing verify_leaf itself, so it only proved u32lt.2 works on the value 3, not that verify_leaf actually performs the check Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Make validate_mainnet() and validate_rollup() private, exposing only validate() which checks the mainnet flag is a valid boolean (0 or 1) before dispatching. This fixes a bug where flag values >= 2 were incorrectly accepted by validate_rollup() since is_mainnet() returned false for any non-1 value. Add mainnet_flag() accessor and a test for invalid flag rejection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Express SMT_PROOF_LOCAL_EXIT_ROOT_PTR and SMT_PROOF_ROLLUP_EXIT_ROOT_PTR relative to PROOF_DATA_PTR and each other, rather than hard-coding absolute values. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reorganize constants into clear sections: proof data memory layout (with address map comment), leaf data, calculate_root locals, and data sizes. Define GLOBAL_INDEX_PTR relative to SMT_PROOF_ROLLUP_EXIT_ROOT_PTR for a fully chained layout. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
| exec.verify_merkle_proof | ||
| # => [verification_flag] | ||
|
|
||
| assert.err=ERR_SMT_ROOT_VERIFICATION_FAILED |
There was a problem hiding this comment.
Should we have different error messages for if the proof verification failed for rollup vs mainnet deposit?
Say, ERR_SMT_ROOT_VERIFICATION_FAILED_ROLLUP & ERR_SMT_ROOT_VERIFICATION_FAILED_MAINNET?
There was a problem hiding this comment.
We could, but we only ever verify one of them per CLAIM note, and I think it should be clear for the submitted of the CLAIM note which path they're taking (by definition, they must know where this data came from, mainnet or rollup). So I think not strictly necessary, lmk if you disagree though (I'll merge as-is, this can be done in a follow up if needed)
| @@ -189,17 +200,16 @@ end | |||
| #! | |||
| #! Invocation: exec | |||
| pub proc process_global_index_mainnet | |||
There was a problem hiding this comment.
I think we should have a helper procedure which takes GLOBAL_INDEX[8] and returns leaf_index & sourceBridgeNetwork. Not for this PR, but for the nullifier tracking PR.
Co-authored-by: Alexander John Lee <77119221+partylikeits1983@users.noreply.github.com>
Summary
calculate_root, then verifying it against therollupExitRootusingsmtProofRollupExitRootprocess_global_index_rollupMASM procedure andvalidate_rollup()/validate()Rust methods onGlobalIndexCloses #2394
Test plan
case_3_rollupbridge-in integration test)GlobalIndexunit tests pass (1 existing + 3 new rollup)make lintpassesforge test🤖 Generated with Claude Code