feat(agglayer): add CLAIM note cycle count benchmark#2618
feat(agglayer): add CLAIM note cycle count benchmark#2618partylikeits1983 wants to merge 14 commits intoagglayerfrom
Conversation
Nice! So, we should definitely set the max for network note execution above 32K cycles. Maybe 64K cycles? cc @Mirko-von-Leipzig (not sure what it is set at currently). Also, how difficult is it to add an actual benchmark? (i.e., similar to what we have for P2ID notes) |
|
We don't explicitly configure the executor, so we get the default values for expected and max cycles. afaict this is set to pub const MAX_CYCLES: u32 = 1 << 29; |
I think it would be easy, will update to be more of a true benchmark. |
Ah - let's create an issue in the node to add such a cap (unless we have such an issue already). |
mmagician
left a comment
There was a problem hiding this comment.
While on it, could we further add benchmarks for B2AGG note?
mmagician
left a comment
There was a problem hiding this comment.
LGTM with the disclaimer that I haven't looked at our benchmarks before, so would be good to get another set of eyes on this
| // TX0: EXECUTE CONFIG_AGG_BRIDGE NOTE TO REGISTER FAUCET IN BRIDGE | ||
| let config_executed = mock_chain | ||
| .build_tx_context(bridge_account.id(), &[config_note.id()], &[])? | ||
| .build()? | ||
| .execute() | ||
| .await?; | ||
| mock_chain.add_pending_executed_transaction(&config_executed)?; | ||
| mock_chain.prove_next_block()?; |
There was a problem hiding this comment.
This is not strictly necessary here as a transaction: we could get the exact same effect by deconstructing the bridge account storage and directly writing to it, if I'm not mistaken.
There was a problem hiding this comment.
You're right that we could directly write to the bridge account storage to achieve the same state. However, I kept the CONFIG_AGG_BRIDGE transaction execution here intentionally for two reasons:
- It mirrors the real-world flow more closely, making the benchmark setup more representative.
- The
CONFIG_AGG_BRIDGEexecution happens during setup only, it's not included in the benchmarked cycle/time measurements. Only the returned transaction context (CLAIMorB2AGG) is actually benchmarked.
| // TX0: EXECUTE CONFIG_AGG_BRIDGE NOTE TO REGISTER FAUCET IN BRIDGE | ||
| let config_executed = mock_chain | ||
| .build_tx_context(bridge_account.id(), &[config_note.id()], &[])? | ||
| .build()? | ||
| .execute() | ||
| .await?; | ||
| mock_chain.add_pending_executed_transaction(&config_executed)?; | ||
| mock_chain.prove_next_block()?; | ||
|
|
||
| // TX1: BUILD B2AGG NOTE TRANSACTION CONTEXT (ready to execute) | ||
| let burn_note_script = StandardNote::BURN.script(); | ||
| let foreign_account_inputs = mock_chain.get_foreign_account_inputs(faucet.id())?; | ||
| let b2agg_tx_context = mock_chain | ||
| .build_tx_context(bridge_account.id(), &[b2agg_note.id()], &[])? | ||
| .add_note_script(burn_note_script) | ||
| .foreign_accounts(vec![foreign_account_inputs]) | ||
| .disable_debug_mode() | ||
| .build()?; | ||
|
|
||
| Ok(b2agg_tx_context) |
There was a problem hiding this comment.
does the benchmark count cycles in all executed txs?
There was a problem hiding this comment.
No, the benchmark only counts cycles for the final transaction (the one returned by the setup function). The prerequisite transactions (CONFIG_AGG_BRIDGE, UPDATE_GER) are executed during setup to prepare the bridge account state, but their cycles are not measured.
| "epilogue": { | ||
| "total": 13756, | ||
| "auth_procedure": 880, | ||
| "after_tx_cycles_obtained": 612 |
There was a problem hiding this comment.
Summarizing these, we get:
CLAIM(L1 → Miden): 35.4K cycles.CLAIM(L2 → Miden): 48.9K cycles.B2AGG(Miden → L1/L2): 163.2K cycles.
Currently, the default maximum for network transaction cycles is 65K - so, processing B2AGG notes would not work. We can of course bump up the limited to 256K, but I'm wondering, why is B2AGG note processing so expensive? Is there something we can do to simplify it?
There was a problem hiding this comment.
Looking (verify superficially), at what bridge_out procedure does, it seems like maybe updating the MMR frontier may be the most heavy operation there. Do we have any benchmarks on how many cycles it takes to perform this? cc @Fumuran.
There was a problem hiding this comment.
We'll investigate, I'll create a separate issue for that though
There was a problem hiding this comment.
it seems like maybe updating the MMR frontier may be the most heavy operation there
It for sure takes quite a lot of cycles (only the load_zeros_to_memory takes 1473 cycles itself), but potentially it could be not the only reason. I'll create a benchmark to investigate how many cycles each stage takes.
4c3fad9 to
6d4d247
Compare
Fumuran
left a comment
There was a problem hiding this comment.
Looks good, thank you! I left just one nit inline.
| /// | ||
| /// Amounts are serialized as uint256 values (JSON numbers). | ||
| #[derive(Debug, Deserialize)] | ||
| pub struct MmrFrontierVectorsFile { |
There was a problem hiding this comment.
nit: I believe that the renaming of the MMR Frontier is already in the agglayer branch, so we should use the new name here (it seems like it was rolled back to the old name during the merge, because the diff has the new name in the test_utils.rs).
This PR adds an assertion to the
CLAIMnote tests which asserts that the cycle count for consuming a CLAIM note should be less than 40k cycles.Currently the CLAIM note takes ~25662 cycles for L1 to Miden bridging, and ~38547 cycles for L2 to Miden bridging.
Resolves #2590