Skip to content

fix: use cumulative prefix sets for incremental trie state root#445

Open
avalonche wants to merge 6 commits intomainfrom
fix/incremental-trie-cumulative-prefix-sets
Open

fix: use cumulative prefix sets for incremental trie state root#445
avalonche wants to merge 6 commits intomainfrom
fix/incremental-trie-cumulative-prefix-sets

Conversation

@avalonche
Copy link
Copy Markdown
Collaborator

Fix: track cumulative TriePrefixSetsMut across all flashblocks. Before building TrieInput, extend the current prefix sets with all prior flashblocks' prefix sets. This forces the walker to re-visit every path modified in earlier flashblocks.

The fix is also ~30% faster than the unfixed incremental path because descending into cached in-memory nodes is faster than DB cursor seeks for skipped branches.

Benchmarks (10 flashblocks, 50k accounts):

  • Without cache: ~2,200ms (baseline)
  • Incremental (no fix): ~844ms (2.6x faster, incorrect on reverts)
  • Incremental (with fix): ~650ms (3.4x faster, correct)

@avalonche avalonche force-pushed the fix/incremental-trie-cumulative-prefix-sets branch 5 times, most recently from b74a166 to baa5064 Compare March 15, 2026 20:27
Comment thread docs/TRIE_CACHE_BENCHMARK_REPORT.md Outdated
Copy link
Copy Markdown
Contributor

@akundaz akundaz left a comment

Choose a reason for hiding this comment

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

The benchmark and tests run against replicated state root calculations instead of the actual production code path. Can you extract the code to run this calculation and and update the benchmark and tests to call that directly? Otherwise we can't really draw conclusions about real world performance from these results. And if the code drifts we should be able to catch with our tests and the benchmark.

Comment thread docs/TRIE_CACHE_BENCHMARK_REPORT.md Outdated
@avalonche avalonche force-pushed the fix/incremental-trie-cumulative-prefix-sets branch from c866328 to 56da125 Compare March 17, 2026 22:10
@akundaz
Copy link
Copy Markdown
Contributor

akundaz commented Mar 25, 2026

The test_storage_revert_to_original_with_populated_trie test asserts both that the old incremental computation is buggy and that the new computation is correct. But we don't need to check the old computation and prove that the bug existed in an older version of the code, just that the fix works. And then the test should be able to catch the regression if the fix is removed by failing.

I would also like to see better encapsulation of the state root computation. Right now it's spread across state_root.rs and payload.rs but we should try to get it all in state_root.rs (the unit test should go there as well). I would suggest that instead of FlashblocksState containing fields enable_incremental_state_root, prev_trie_updates, and cumulative_prefix_sets, you define a new type for the state root computation.

@avalonche
Copy link
Copy Markdown
Collaborator Author

@akundaz

The test_storage_revert_to_original_with_populated_trie test asserts both that the old incremental computation is buggy and that the new computation is correct. But we don't need to check the old computation and prove that the bug existed in an older version of the code, just that the fix works. And then the test should be able to catch the regression if the fix is removed by failing.

This change was just illustrative to prove that the issue is reproducible and that the new code fixes the issue. I can remove it if you're satisfied this fix works with reverts.

@akundaz
Copy link
Copy Markdown
Contributor

akundaz commented Mar 26, 2026

This change was just illustrative to prove that the issue is reproducible and that the new code fixes the issue. I can remove it if you're satisfied this fix works with reverts.

That's fine! Just make sure to have a test with reverts since it triggers the bug

Copy link
Copy Markdown
Contributor

@akundaz akundaz left a comment

Choose a reason for hiding this comment

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

Just adding a few comments. I think once they are resolved and CI passes this will probably be good

Comment thread crates/op-rbuilder/src/builder/mod.rs Outdated
Comment thread crates/op-rbuilder/src/builder/state_root.rs Outdated
Comment thread crates/op-rbuilder/src/builder/state_root.rs Outdated
Comment thread crates/op-rbuilder/src/builder/state_root.rs Outdated
avalonche and others added 6 commits April 10, 2026 16:09
The incremental trie cache produces wrong state roots when a storage slot
modified in flashblock N reverts in flashblock N+1. The reverted slot
disappears from the cumulative HashedPostState, so its nibble path is
missing from the prefix set. The trie walker skips the subtree and uses
the stale cached hash from the previous flashblock's branch node.

Fix: track cumulative TriePrefixSetsMut across all flashblocks. Before
building TrieInput, extend the current prefix sets with all prior
flashblocks' prefix sets. This forces the walker to re-visit every path
modified in earlier flashblocks.

The fix is also ~30% faster than the unfixed incremental path because
descending into cached in-memory nodes is faster than DB cursor seeks
for skipped branches.

Benchmarks (10 flashblocks, 50k accounts):
- Without cache:           ~2,200ms (baseline)
- Incremental (no fix):    ~844ms  (2.6x faster, incorrect on reverts)
- Incremental (with fix):  ~650ms  (3.4x faster, correct)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract the incremental state root calculation logic from
build_flashblock_payload_inner into builder::state_root::compute_state_root.

Tests and benchmarks now call the same function as production code,
ensuring they exercise the actual code path rather than replicated logic
that could drift.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace free function and manual state threading with a StateRootCalculator
struct that owns cached trie updates and cumulative prefix sets internally.
FlashblocksState always holds a calculator; incremental vs full mode is
configured at construction. Move tests into state_root module and add e2e test.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reorder pub use statements in builder/mod.rs to satisfy nightly rustfmt
(unblocks the failed lint job).

Update test_incremental_state_root expected count from 15 to 18 — the
flashblocks listener captures the base/fallback flashblock (index 0) in
addition to the 5 incremental flashblocks built per block, so 3 blocks
yield 18 messages.

Tighten the StateRootCalculator usage in payload.rs to bind the default
calculator to a let so the &mut borrow lives long enough, drop the
unused is_incremental helper, and consolidate the two
simulate_flashblocks test helpers behind a populate_trie flag. Switch
the bench to import StateRootCalculator from the public re-export.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ompute

Hoist the `!self.incremental` case out of compute() so the rest of the
function can assume incremental mode. This drops both `if self.incremental`
checks (one inside the else arm that gated seeding the cumulative prefix
sets, one after the match that gated writing back to self) and removes the
three-tuple return from the match. The two paths now read as: non-incremental
returns immediately; incremental builds cumulative prefix sets, runs the
cached or full computation, and writes the cache unconditionally.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@avalonche avalonche force-pushed the fix/incremental-trie-cumulative-prefix-sets branch from 8f7d3e9 to 735e2cb Compare April 11, 2026 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants