chore(trie): add e2e for reorg by yongkangc · Pull Request #18293 · paradigmxyz/reth

yongkangc · 2025-09-05T11:26:37Z

Summary

add trie_updates_test.rs, an end-to-end regression test that reproduces the trie-update loss fixed in refactor(engine): persistence logic #18318
ensure canonical blocks retain their trie updates after a short reorg by exercising the e2e TestBuilder flow
guard against a recurrence of the state-root mismatch that stalled mainnet nodes in early September 2024

Scenario Under Test (trie_updates_test.rs)

Spin up a single-node mainnet chain spec with a pre-funded storage contract whose slots hash to shared prefixes, creating branch nodes identical to the production incident.
Produce block_1, which writes to those slots, capture the block, and assert its trie updates are persisted.
Produce block_2, which rewrites the same slots (mirroring the canonical block after the 23003311 fork), capture it, and assert trie updates are still present.
The assertions use AssertMissingTrieUpdates so that any regression that drops trie updates for canonical descendants stops the test.

mattsse

looking at the new test I get the idea what this does

unsure if we need all of this additional setup, looks like we do, wdyt @fgimenez

crates/e2e-test-utils/src/testsuite/actions/transactions.rs

yongkangc · 2025-09-08T07:43:14Z

looking at the new test I get the idea what this does

unsure if we need all of this additional setup, looks like we do, wdyt @fgimenez

@mattsse realising that my approach might have to change to a simpler one due to issues mocking of genesis state and recreate the other blocks, as it results in blockhash mismatch.

instead of the current approach in the test, we would have to do something like
     1. Genesis: Deploy storage contract with initial storage
     2. Block 1 (canonical): Transaction that sets storage slot 0x0f to value A
     3. Block 1' (fork): Transaction that sets storage slot 0x0f to value B (creates trie node)
     4. Reorg: Make Block 1' canonical
     5. Block 2: Transaction that clears storage slot 0x0f (should delete trie node)

fgimenez · 2025-09-09T08:28:41Z

instead of the current approach in the test, we would have to do something like
1. Genesis: Deploy storage contract with initial storage
2. Block 1 (canonical): Transaction that sets storage slot 0x0f to value A
3. Block 1' (fork): Transaction that sets storage slot 0x0f to value B (creates trie node)
4. Reorg: Make Block 1' canonical
5. Block 2: Transaction that clears storage slot 0x0f (should delete trie node)

sgtm, this should be doable with the current actions plus maybe the new action that allows adding custom txs?

crates/e2e-test-utils/src/testsuite/actions/transactions.rs

fgimenez · 2025-09-09T08:29:23Z

crates/e2e-test-utils/src/testsuite/actions/transactions.rs

+/// This action sends transactions to the node's transaction pool and then
+/// triggers block production to include them.
+#[derive(Debug)]
+pub struct ProduceBlockWithTransactions {


yongkangc · 2025-09-10T08:08:41Z

crates/chain-state/src/in_memory.rs

+                // For single-block chains, preserve trie updates for testing
+                // This maintains backwards compatibility while allowing tests to access trie
+                // updates
+                let new = if new.len() == 1 {


@shekhirin @mediocregopher @Rjected would appreciate a look over here as its what is required to replicate the test.

However as with anything with the trie state would need a double look

my understanding of single block chains - the chain contains exactly one executed block

Afaict this is ok, the only behavior change is that CanonStateNotification::Commit contains trie updates for single block chains now, which the documentation states is the case anyway.

- Added transaction utility functions for e2e testing - Implemented new action module for transaction operations - Enhanced engine API and block production actions - Created comprehensive trie corruption test suite - Added test JSON payloads and testdata for reorg scenarios - Updated test paths to use testdata directory structure

- Introduced SendPayloadFromJson action to facilitate sending payloads from embedded JSON data. - Updated engine API to support new action and modified existing payload handling logic. - Enhanced test suite to utilize SendPayloadFromJson for improved test scenarios. - Added reth-trie-common as a dependency in Cargo.toml and Cargo.lock.

- Reformatted comments for clarity and consistency. - Consolidated import statements for better organization. - Enhanced logging messages for improved debugging. - Streamlined function definitions and removed redundant code. - Updated test structure to ensure better maintainability.

- Removed unused module `transaction_utils` from the testsuite. - Updated comments for clarity and consistency across action implementations. - Changed method signatures to `const fn` for `with_node_idx` and `with_expected_status` for better performance. - Streamlined error handling and logging messages in transaction-related actions. - Consolidated import statements for better organization in the trie corruption test.

- Reformatted logging statements for better readability and consistency in `ProduceBlockWithTransactionsViaEngineAPI`. - Enhanced clarity in the code structure by adjusting the formatting of variable assignments and comments, improving overall maintainability. - Updated comments to provide clearer context for synthetic trie updates during testing scenarios.

- Fix clippy warnings by handling empty VERGEN_CARGO_FEATURES macro - Add missing HeaderProvider trait import in node.rs - Fix Engine API version compatibility issues: - Implement dynamic version detection based on payload attributes - Add v2 to v3 payload conversion for internal storage compatibility - Handle parent_beacon_block_root field properly for different forks - Fix documentation warning by escaping Mutex in doc comment - Update test chain specs to use Cancun fork for v3 API support

- Fix clippy warnings: remove unused imports, fix documentation backticks, remove unnecessary conversions - Fix Engine API compatibility: use v2 API instead of v3 for better compatibility - Update test configurations to use Shanghai instead of Cancun - Remove parent_beacon_block_root from payload attributes to match v2 API - Convert ExecutionPayloadV3 to ExecutionPayloadInputV2 for API calls

yongkangc · 2025-09-15T14:58:37Z

crates/e2e-test-utils/src/node.rs

+                                );
+                            }
+                        } else {
+                            println!("Node {}: No trie updates in committed chain", node_idx);


to remove after debugging

- Modified multiple test cases to activate the Cancun fork, replacing previous Shanghai activations. - Ensured compatibility with the latest test configurations for improved testing accuracy.

- Replaced the obsolete `trie_fork_test` with `trie_updates_test` to focus on validating trie updates in canonical blocks. - Adjusted the `ProduceBlockWithTransactionsViaEngineAPI` to ensure proper handling of trie updates. - Updated the `parent_beacon_block_root` in transaction payload attributes to align with the latest block structure. - Enhanced test configurations to ensure compatibility with the Cancun fork and improve testing accuracy.

- Renamed and restructured storage configurations to improve clarity and organization in trie update tests. - Introduced `BlockStorageValues` to encapsulate storage values for individual blocks, enhancing the readability of test setups. - Updated comments to reflect changes in storage slot configurations and their purpose in testing scenarios.

- Updated comments and naming conventions in the `trie_updates_test` to enhance clarity regarding storage slot configurations. - Adjusted descriptions to better reflect the purpose of the storage slots in relation to Merkle Patricia Trie behavior during updates.

- Removed unused imports from the `trie` module in the test suite actions, retaining only `AssertMissingTrieUpdates` for clarity and maintainability.

- Removed unnecessary state root fallback and always compare trie updates settings in the `trie_updates_test` setup for improved clarity and simplicity. - Retained the legacy state root configuration to maintain compatibility with trie update optimizations.

engine payloads by relying on the shared payload- attributes trait instead of hard Into conversions.

- Remove unused tracing::error import - Replace error! macro with eprintln! to fix compilation - Remove all emojis from output messages per project standards - Maintain test functionality while fixing CI issues

Required for compilation after rebase. The op-alloy-rpc-types-engine dependency needs the serde feature enabled to match the configuration of alloy-rpc-types-engine.

mediocregopher

Left a bunch of comments, mostly nitpicks.

One thing I'd note is that another change that should be possible as part of #18460 is that ExExNotifications will have trie updates on them as well. I'm not sure if that would be easier to work into the tests than the canon notifications, maybe it's the same, but just something that came to mind reading this.

mediocregopher · 2025-09-23T08:29:32Z

crates/chain-state/src/in_memory.rs

+                // For single-block chains, preserve trie updates for testing
+                // This maintains backwards compatibility while allowing tests to access trie
+                // updates
+                let new = if new.len() == 1 {


Afaict this is ok, the only behavior change is that CanonStateNotification::Commit contains trie updates for single block chains now, which the documentation states is the case anyway.

mediocregopher · 2025-09-23T08:34:51Z

crates/e2e-test-utils/src/testsuite/actions/engine_api.rs

+                // Try multiple possible locations
+                let candidates = vec![
+                    std::path::Path::new(file_path).to_path_buf(),
+                    std::path::Path::new("../../..").join(file_path),


What are these extra candidates trying to search? If file_path is the crates dir won't these be looking outside the reth project?

mediocregopher · 2025-09-23T08:45:22Z

crates/e2e-test-utils/src/testsuite/actions/trie.rs

+
+/// Action that asserts whether a block has missing trie updates
+#[derive(Debug)]
+pub struct AssertMissingTrieUpdates {


nit: really we're asserting that the updates are present, so AssertTrieUpdatesPresent would be a better name

mediocregopher · 2025-09-23T08:46:23Z

crates/e2e-test-utils/src/testsuite/actions/trie.rs

+                .get(&self.block_tag)
+                .ok_or_else(|| eyre::eyre!("Unknown block tag {}", self.block_tag))?;
+
+            eprintln!("\n=== CHECKING TRIE UPDATES STATUS FOR BLOCK {} ===", self.block_tag);


Are these left in intentionally? They seem like they'd be noisy

mediocregopher · 2025-09-23T08:47:59Z

crates/e2e-test-utils/src/testsuite/actions/trie.rs

+where
+    Engine: EngineTypes,
+{
+    fn execute<'a>(


I think this method will get simplified quite a bit once #18460 is done, but overall definitely still valuable to be able to assert that any trie updates were generated 👍

mediocregopher · 2025-09-23T08:53:46Z

crates/e2e-test-utils/src/testsuite/setup.rs

            node_clients.push(crate::testsuite::NodeClient::new(rpc, auth, url));
        }

+        // We'll handle node context setting in a different way since nodes don't implement Clone


Is this comment still relevant?

mediocregopher · 2025-09-23T08:54:39Z

crates/e2e-test-utils/src/node.rs

+        );
+
+        tokio::spawn(async move {
+            println!("Starting trie update forwarder for node {}", node_idx);


leftover println

mediocregopher · 2025-09-23T08:56:51Z

crates/e2e-test-utils/tests/e2e_testsuite/main.rs

@@ -37,7 +39,7 @@ async fn test_apply_with_import() -> Result<()> {
                .unwrap(),
            )
            .london_activated()
-            .shanghai_activated()
+            .cancun_activated()


was this deliberate? cancun is already activated here

mediocregopher · 2025-09-23T08:59:06Z

crates/e2e-test-utils/tests/e2e_testsuite/trie_updates_test.rs

+        .with_tree_config(
+            TreeConfig::default()
+                .with_persistence_threshold(0) // Persist blocks immediately so trie updates are kept
+                .with_legacy_state_root(true), /* Use legacy state root to bypass trie update


I'm not sure this with_legacy_state_root should be necessary, would be better to leave it out if possible

mediocregopher · 2025-09-23T09:00:38Z

crates/node/core/src/version.rs

        vergen_build_timestamp: Cow::Owned(env!("VERGEN_BUILD_TIMESTAMP").to_string()),
        vergen_cargo_target_triple: Cow::Owned(env!("VERGEN_CARGO_TARGET_TRIPLE").to_string()),
-        vergen_cargo_features: Cow::Owned(env!("VERGEN_CARGO_FEATURES").to_string()),
+        vergen_cargo_features: Cow::Owned(String::new()),


probably not deliberate?

The test directory was named 'e2e-testsuite' but Cargo.toml expected 'e2e_testsuite' (with underscore). This mismatch was causing CI failures. - Renamed tests/e2e-testsuite/ to tests/e2e_testsuite/ - Updated Cargo.toml path to match renamed directory

- Remove unnecessary abstractions and helper structs - Focus test on the actual reorg scenario from the incident - Test now properly creates fork → canonical → next block sequence - Use actual contract address from the incident - Remove unused genesis.json file - Make test clearer and more maintainable

github-project-automation bot added this to Reth Tracker Sep 5, 2025

github-project-automation bot moved this to Backlog in Reth Tracker Sep 5, 2025

yongkangc force-pushed the yk/e2e_reorg_trie branch 2 times, most recently from e3dca93 to 07c7a42 Compare September 5, 2025 12:53

yongkangc marked this pull request as ready for review September 5, 2025 13:00

yongkangc requested review from Rjected, gakonst and mattsse as code owners September 5, 2025 13:00

yongkangc requested review from klkvr, mediocregopher and shekhirin September 5, 2025 13:01

yongkangc marked this pull request as draft September 8, 2025 04:00

mattsse reviewed Sep 8, 2025

View reviewed changes

crates/e2e-test-utils/src/testsuite/actions/transactions.rs Outdated Show resolved Hide resolved

yongkangc self-assigned this Sep 8, 2025

yongkangc moved this from Backlog to In Progress in Reth Tracker Sep 8, 2025

fgimenez reviewed Sep 9, 2025

View reviewed changes

yongkangc marked this pull request as ready for review September 10, 2025 06:48

yongkangc marked this pull request as draft September 10, 2025 07:54

yongkangc marked this pull request as ready for review September 10, 2025 08:07

yongkangc requested a review from rkrasiuk as a code owner September 10, 2025 08:07

yongkangc commented Sep 10, 2025

View reviewed changes

yongkangc added 7 commits September 15, 2025 08:23

removed unused files

609b8af

combine files

d6b89e4

feat: introduce unified payload handling for engine API actions

1137015

yongkangc added 4 commits September 15, 2025 08:25

remove parent beacon root

e0abdd9

yongkangc force-pushed the yk/e2e_reorg_trie branch from eed5f29 to e0abdd9 Compare September 15, 2025 08:28

yongkangc commented Sep 15, 2025

View reviewed changes

yongkangc added 8 commits September 16, 2025 04:10

update e2e tests to use Cancun fork instead of Shanghai

eb6b484

- Modified multiple test cases to activate the Cancun fork, replacing previous Shanghai activations. - Ensured compatibility with the latest test configurations for improved testing accuracy.

refactor: simplify trie update imports in test suite

99f5427

- Removed unused imports from the `trie` module in the test suite actions, retaining only `AssertMissingTrieUpdates` for clarity and maintainability.

fmt

6b4d5b2

Made the AssertMineBlock action work with Optimism

ed0ae11

engine payloads by relying on the shared payload- attributes trait instead of hard Into conversions.

yongkangc mentioned this pull request Sep 18, 2025

feat(trie): Tool for controlled trie structure generation for e2e reorg testing #18545

Closed

5 tasks

yongkangc added 2 commits September 19, 2025 05:54

fix(e2e-test): remove emojis and fix compilation errors in trie actions

82a8dfe

- Remove unused tracing::error import - Replace error! macro with eprintln! to fix compilation - Remove all emojis from output messages per project standards - Maintain test functionality while fixing CI issues

fix(deps): add serde feature to op-alloy-rpc-types-engine

f85422d

Required for compilation after rebase. The op-alloy-rpc-types-engine dependency needs the serde feature enabled to match the configuration of alloy-rpc-types-engine.

mediocregopher requested changes Sep 23, 2025

View reviewed changes

yongkangc added 7 commits September 25, 2025 05:01

ci: trigger CI checks after fixing test directory naming

bc925b2

chore: merge main and resolve conflicts

310e60a

fix: resolve remaining merge conflicts and format code

cd2f9b2

fix: resolve compilation error in test genesis setup

905c5dc

Merge remote-tracking branch 'origin/main' into yk/e2e_reorg_trie

b145018

yongkangc moved this from In Progress to Backlog in Reth Tracker Oct 6, 2025

yongkangc closed this Oct 15, 2025

github-project-automation bot moved this from Backlog to Done in Reth Tracker Oct 15, 2025

jenpaff moved this from Done to Out of Scope in Reth Tracker Oct 15, 2025

mediocregopher deleted the yk/e2e_reorg_trie branch February 25, 2026 17:29

Conversation

yongkangc commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yongkangc commented Sep 8, 2025

Uh oh!

fgimenez commented Sep 9, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mediocregopher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mediocregopher Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yongkangc commented Sep 5, 2025 •

edited

Loading

mediocregopher Sep 23, 2025 •

edited

Loading