feat(l2-withdrawals): Withdrawals storage root verification in execution #14124

emhane · 2025-01-31T13:35:47Z

Ref ethereum-optimism#19, https://github.com/ethereum-optimism/op-reth/actions/runs/13064293936

emhane · 2025-02-04T00:17:07Z

looked into possibility of making this reth-optimism-consensus and reth-optimism-evm wasm compatible, but it is way out of scope of this pr. isthmus requires HashedStorage type from reth-trie as well as StateRootProvider, StateRootProviderFactory and StorageRootProvider. cc @Rjected @shekhirin

codspeed-hq · 2025-02-04T13:16:34Z

CodSpeed Performance Report

Merging #14124 will not alter performance

_{Comparing emhane/exec-verif-storage-root (2acf3f6) with main (29e6e5c)}

Summary

✅ 77 untouched benchmarks

mattsse

I'd like to get all of the consensus related functions in separately.

I'm no longer sure about the StateProvider vs Database changes, which will clash with the new revm wip.
I do like that these now operate just on StateProvider which gives the user more flexibility, but want to hear what @klkvr thinks

mattsse · 2025-02-04T16:25:23Z

crates/optimism/consensus/src/validation/canyon.rs

could we submit all of these separately?

hm, I don't see the need for this hassle, since the feature is already tested in kurtosis, and we're starting to run out of time to upstream

isthmus code cut is Monday, meaning we have 3 days left

deadline has been extended to wait in op-reth. will open a separate pr for consensus functions and change the function args to be as generic as possible since it isn't clear if the verification should be done in execution or post-execution at this point

#14308 @mattsse

klkvr · 2025-02-04T20:24:15Z

crates/optimism/evm/src/execute.rs

+        // isthmus verification
+        if self.chain_spec.is_isthmus_active_at_timestamp(block.timestamp) {
+            let state_updates = mem::take(&mut evm.db_mut().bundle_state);
+            drop(evm);
+            isthmus::verify_withdrawals_storage_root(
+                &state_updates,
+                &*self.state.database,
+                block.header(),
+            )
+            .map_err(OpBlockExecutionError::Consensus)?;
+        }
+


we can't do this in executor because storage root verification requires StorageHashingStage which is happening only after execution

right, forgot about this, this won't work in backfill sync because storage hashing comes after.

this is similar to state root which we also don't check during execution.
the only way we can do this is with a new stage that checks this or we only do this in live sync because we only trigger backfill towards a finalized range

first of all I was implementing it to happen post execution, then @mattsse told me to do it in execution because otherwise we won't be able to verify the storage root in pipeline sync, same as for state root, based on that convo I opened this issue #13537 and closed #13214

crates/optimism/consensus/src/validation/isthmus.rs

emhane · 2025-02-05T14:01:51Z

crates/optimism/consensus/src/validation/isthmus.rs

+            let hashed_storage = HashedStorage::from_plain_storage(
+                account.status,
+                account.storage.iter().map(|(slot, value)| (slot, &value.present_value)),
+            );
+            storage_root_prehashed(hashed_storage.storage)


storage hashing is done here @klkvr re: https://github.com/paradigmxyz/reth/pull/14124/files#r1941840549

what I mean is that we need values from tables::HashedStorages here. this table will be empty until StorageHashingStage runs

reth/crates/trie/trie/src/trie.rs

Lines 400 to 405 in 08285c9

let mut hashed_storage_cursor =

self.hashed_cursor_factory.hashed_storage_cursor(self.hashed_address)?;

// short circuit on empty storage

if hashed_storage_cursor.is_storage_empty()? {

return Ok((EMPTY_ROOT_HASH, 0, StorageTrieUpdates::deleted()))

ah you mean in pipeline sync specifically it's a problem

does this mean we are ok with adding a field with to the type ExecuteOutput for the state updates in order to verify l2 withdrawals root after execution and only for live sync? this is what I was working on initially, so just want to make sure the change to accommodate for OP in shared reth code is tolerated

reth/crates/evm/src/execute.rs

Lines 167 to 174 in b955551

/// Helper type for the output of executing a block.

#[derive(Debug, Clone)]

pub struct ExecuteOutput<R = Receipt> {

/// Receipts obtained after executing a block.

pub receipts: Vec<R>,

/// Cumulative gas used in the block execution.

pub gas_used: u64,

}

alt we can just hash the storage for that account in pipeline sync and pass a custom db wrapper type to execution which stores the storage hashed only for account at 0x42..16 in a separate field

emhane · 2025-02-15T22:16:18Z

closing since established that storage root verif cannot be done for pipeline sync anyhow this way because storage hashing happens after execution stage in pipeline. new approach, verify storage root post execution, pre-req: #14464

emhane added A-execution Related to the Execution and EVM A-consensus Related to the consensus engine A-op-reth Related to Optimism and op-reth labels Jan 31, 2025

emhane requested review from onbjerg, gakonst, DaniPopes, rakita, rkrasiuk, shekhirin, mattsse, Rjected, joshieDo, klkvr and fgimenez as code owners January 31, 2025 13:35

emhane marked this pull request as draft January 31, 2025 13:35

Verify withdrawals storage root in execution

3c715de

emhane force-pushed the emhane/exec-verif-storage-root branch from 2485b74 to 3c715de Compare January 31, 2025 17:27

emhane added 8 commits January 31, 2025 18:29

Fix merge conflicts

da72782

Fix merge conflicts

a2d539f

Merge branch 'main' into emhane/exec-verif-storage-root

edc5534

Fix feature propagation

9ba8944

Fix docs

41b12dc

Fix dev-deps

d4d4f3c

Fix features

6ce54ad

Fix features

730ac29

emhane requested a review from clabby February 1, 2025 00:12

emhane added 2 commits February 1, 2025 01:14

Fix features

8d4645d

Revert changes to Makefile

72cc9df

emhane marked this pull request as ready for review February 1, 2025 00:26

emhane mentioned this pull request Feb 3, 2025

Abstraction over ExecutionPayload and ExecutionPayloadSidecar #14165

Closed

Merge branch 'main' into emhane/exec-verif-storage-root

c44271f

meyer9 approved these changes Feb 3, 2025

View reviewed changes

Exclude reth-optimism-evm and reth-optimism-consensus from wasm check

91cd66c

Merge branch 'main' into emhane/exec-verif-storage-root

e3878eb

emhane mentioned this pull request Feb 4, 2025

feat(no_std): Add no_std support for reth-storage-api #14187

Merged

emhane added 2 commits February 4, 2025 14:01

Merge branch 'main' into emhane/exec-verif-storage-root

ac7ed1a

Fix merge conflicts

661dbc8

emhane added 6 commits February 4, 2025 14:17

Disable default-features for tracing

63a9bbc

Merge branch 'main' into emhane/exec-verif-storage-root

55a70f8

Fix feature propagation

7a2cbfe

Fix deps

ed03660

Format manifest

8b7f6cb

Fix manifest

e0f22d7

mattsse requested changes Feb 4, 2025

View reviewed changes

emhane requested a review from mattsse February 4, 2025 17:56

klkvr reviewed Feb 4, 2025

View reviewed changes

crates/optimism/consensus/src/validation/isthmus.rs Outdated Show resolved Hide resolved

emhane commented Feb 5, 2025

View reviewed changes

emhane added 4 commits February 5, 2025 15:27

Fix bug, storage root always computed from storage since genesis

4c05794

Merge branch 'main' into emhane/exec-verif-storage-root

2d60805

Fix deps

b24f462

Add test for execution of l2 withdrawal transaction

2acf3f6

emhane mentioned this pull request Feb 7, 2025

feat: add error traits and ATs alloy-rs/evm#11

Merged

3 tasks

emhane requested a review from klkvr February 7, 2025 16:02

emhane mentioned this pull request Feb 7, 2025

feat(l2-withdrawals): consensus rules #14308

Merged

emhane closed this Feb 15, 2025

emhane deleted the emhane/exec-verif-storage-root branch February 15, 2025 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(l2-withdrawals): Withdrawals storage root verification in execution #14124

feat(l2-withdrawals): Withdrawals storage root verification in execution #14124

emhane commented Jan 31, 2025 •

edited

Loading

emhane commented Feb 4, 2025

codspeed-hq bot commented Feb 4, 2025 •

edited

Loading

mattsse left a comment

mattsse Feb 4, 2025

emhane Feb 4, 2025

emhane Feb 4, 2025

emhane Feb 7, 2025

emhane Feb 7, 2025

emhane Feb 7, 2025

klkvr Feb 4, 2025

mattsse Feb 4, 2025

emhane Feb 5, 2025

emhane Feb 5, 2025

klkvr Feb 5, 2025 •

edited

Loading

emhane Feb 6, 2025

emhane Feb 7, 2025

emhane Feb 7, 2025

emhane commented Feb 15, 2025

	let mut hashed_storage_cursor =
	self.hashed_cursor_factory.hashed_storage_cursor(self.hashed_address)?;

	// short circuit on empty storage
	if hashed_storage_cursor.is_storage_empty()? {
	return Ok((EMPTY_ROOT_HASH, 0, StorageTrieUpdates::deleted()))

	/// Helper type for the output of executing a block.
	#[derive(Debug, Clone)]
	pub struct ExecuteOutput<R = Receipt> {
	/// Receipts obtained after executing a block.
	pub receipts: Vec<R>,
	/// Cumulative gas used in the block execution.
	pub gas_used: u64,
	}

feat(l2-withdrawals): Withdrawals storage root verification in execution #14124

feat(l2-withdrawals): Withdrawals storage root verification in execution #14124

Conversation

emhane commented Jan 31, 2025 • edited Loading

emhane commented Feb 4, 2025

codspeed-hq bot commented Feb 4, 2025 • edited Loading

Merging #14124 will not alter performance

Summary

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klkvr Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emhane commented Feb 15, 2025

emhane commented Jan 31, 2025 •

edited

Loading

codspeed-hq bot commented Feb 4, 2025 •

edited

Loading

klkvr Feb 5, 2025 •

edited

Loading