Skip to content

Compute claimed global index chain hash#2516

Merged
Fumuran merged 68 commits intoagglayerfrom
andrew-compute-cgi-chain-hash
Mar 17, 2026
Merged

Compute claimed global index chain hash#2516
Fumuran merged 68 commits intoagglayerfrom
andrew-compute-cgi-chain-hash

Conversation

@Fumuran
Copy link
Copy Markdown
Contributor

@Fumuran Fumuran commented Feb 25, 2026

This PR:

  • Modifies the bridge_in.masm file to add the CGI chain hash computation in the verify_leaf_bridge procedure. For now it is more like in a "template" state, since for now it is impossible to compute this value: to do so we need to read the previous CGI hash value, but currently it is not store anywhere (yet).
  • Adds a Solidity contract to generate the data required for the CGI chain hash computation: leaf value, global index, previous (old) CGI chain hash and a new (expected) CGI chain hash. This contract was generated with AI help, but in my layman's opinion this code works correctly, though I still have some questions about it. I beg for the masters of the Solidity @partylikeits1983 and @mmagician to check its correctness. It should be fine, since the test is passing, but nevertheless.
  • Adds a new test to check that the CGI chain hash value computed in the verify_leaf_bridge procedure (or, to be precise, the value, computed using the same algorithm: I can't reuse the compute_cgi_hash_chain procedure itself since it is private and it uses some memory values which are not available during the test) is correct.

We still need to store the computed value in memory, but this approach is blocked until we change the way SWAP note it consumed. Storing the CGI hash will be done in a separate PR.

Part of #2386

@Fumuran Fumuran added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Feb 25, 2026
@Fumuran Fumuran marked this pull request as ready for review February 25, 2026 17:50
@mmagician mmagician added agglayer PRs or issues related to AggLayer bridging integration pr-from-maintainers PRs that come from internal contributors or integration partners. They should be given priority labels Mar 1, 2026
Copy link
Copy Markdown
Member

@partylikeits1983 partylikeits1983 left a comment

Choose a reason for hiding this comment

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

Looks great!

I just think we should reuse the pure function test helpers: test_utils::execute_program_with_default_host. I think using execute_program_with_default_host will clean up the test, and also make it possible to not have to have a "copy" of the compute_cgi_hash_chain procedure in the test.

Also lets update references to "chain hash" to "hash chain". Otherwise everything looks great!

Comment on lines +54 to +69
proc compute_cgi_hash_chain_copy
# load the global index onto the stack
push.GLOBAL_INDEX_PTR exec.utils::mem_load_double_word
# => [GLOBAL_INDEX[8], LEAF_VALUE[8]]

exec.keccak256::merge
# => [Keccak256(GLOBAL_INDEX, LEAF_VALUE), pad(8)]

# load the old CGI chain hash
push.OLD_CGI_CHAIN_HASH_PTR exec.utils::mem_load_double_word
# => [OLD_CGI_CHAIN_HASH[8], Keccak256(GLOBAL_INDEX, LEAF_VALUE), pad(8)]

# compute the new CGI chain hash
exec.keccak256::merge
# => [NEW_CGI_CHAIN_HASH[8], pad(8)]
end
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd actually call the real procedure, not a copy of it. I think the asset conversion tests are a good example which show how to do this so that you don't have to run a full transaction.

"#
);

let tx_script = CodeBuilder::new()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would be cleaner if we had the assert_eq! in Rust as opposed to running the test logic in masm. I think it reads easier if you call the masm procedure, get the returned values off the stack in Rust, then check the two Rust variable values are equal. As noted above, we have this testing functions available in the agglayer crate, check out the asset_conversion.rs file to see how they are used

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I recall that we had a discussion about whether we should use asserts in the Rust code vs MASM code, but IIRC we agreed to follow @PhilippGackstatter 's proposal to use asserts in the MASM code, but honestly I don't remember why. Probably to make the code self-contained and, which is more important, to check the error messages correctness in case the testing code uses some of the internal kernel/protocol procedures.

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.

I think we discussed it at some point, but don't recall the outcome. I think there are pros and cons. Multiple asserts in MASM read relatively nicely and can be done sequentially, as done in this test.

On the other hand, asserts in Rust generally result in a better error when they fail. The downside is if you need to assert the correctness of multiple values, you sometimes need to write extra MASM to arrange the output stack appropriately and then also extract multiple stack values in the right order from the ExecutionOutput, which is messy and incurs unnecessary mental load to review its correctness.

So, I'd use whichever approach results in a more readable and easily maintainble test and one that provides good errors when it fails.

partylikeits1983 and others added 14 commits March 2, 2026 17:39
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
partylikeits1983 and others added 15 commits March 12, 2026 16:48
* chore: pass faucet ID on stack to verify_claim_amount

* chore: pass faucet ID on stack to build_mint_output_note

* chore: completely remove CLAIM_FAUCET_ID_PREFIX_MEM_ADDR

* chore: drop "y" in verify_u128_to_native_amount_conversion

* chore: avoid uplicating y only to drop it later

* chore: adjust comments; no longer reading faucetID from mem

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* fix: rename to output & fix copilot suggestion

---------

Co-authored-by: Alexander John Lee <77119221+partylikeits1983@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: riemann <aleqvids@gmail.com>
Copy link
Copy Markdown
Member

@partylikeits1983 partylikeits1983 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@mmagician is the plan to merge #2528 first, and then this PR?

Base automatically changed from ajl-reorient-claim-note-flow to agglayer March 17, 2026 08:46
Copy link
Copy Markdown
Collaborator

@mmagician mmagician left a comment

Choose a reason for hiding this comment

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

Overall looks good

@Fumuran Fumuran merged commit 7712230 into agglayer Mar 17, 2026
17 checks passed
@Fumuran Fumuran deleted the andrew-compute-cgi-chain-hash branch March 17, 2026 15:53
mmagician pushed a commit that referenced this pull request Mar 17, 2026
Merge 3 agglayer commits: claim flow reorientation (#2528), rollup
claim processing (#2573), and global index chain hash (#2516).

Conflict resolution:
- bridge_config.masm: take agglayer (token registry additions)
- faucet/mod.masm: take agglayer (claim removal, get_scale refactor),
  keep our get_metadata_hash procedure and storage slots
- components/faucet.masm: export both get_metadata_hash and get_scale
- faucet.rs: take agglayer ownable pattern (owner_config_slot),
  keep metadata hash slots and accessors

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agglayer PRs or issues related to AggLayer bridging integration no changelog This PR does not require an entry in the `CHANGELOG.md` file pr-from-maintainers PRs that come from internal contributors or integration partners. They should be given priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants