Introduce `StateRootProviderExt` and Integrate with `StateCommitment` #12896

frisitano · 2024-11-27T04:28:45Z

Overview

This PR introduces the StateRootProviderExt which is used for performing state root operations on the latest state stored in the database. We integrate both StateRootProvider and StateRootProviderExt with the StateRoot type from StateCommitment. This PR should be reviewed after #12895.

…-commitment

…tate-provider

…ge-provider

…rovider) with StateCommitment

…ate-provider

…ge-provider

…ider

emhane · 2024-11-27T09:01:23Z

crates/storage/storage-api/src/trie.rs

+    /// Returns the state root of the current state.
+    fn state_root(&self) -> ProviderResult<B256>;
+
+    /// Returns the state root of the current state and trie updates.


Suggested change

/// Returns the state root of the current state and trie updates.

/// Tries to compute the state root of the current state and [`TrieUpdates`].

we appreciate well linked docs.

does this load state root saved to db "Tries to load the state root of the current state and [`TrieUpdates`]" or does it compute it "Tries to calculate the state root of the current state and [`TrieUpdates`]". this distinction tells us smthg about the cost of this method and is helpful.

emhane · 2024-11-27T09:01:38Z

crates/storage/storage-api/src/trie.rs

+    /// Returns the state root of the current state and trie updates.
+    fn state_root_with_updates(&self) -> ProviderResult<(B256, TrieUpdates)>;
+
+    /// Returns the state root with trie updates associated with the given block range.


Suggested change

/// Returns the state root with trie updates associated with the given block range.

/// Tries to compute the state root with [`TrieUpdates`] associated with the given block range.

emhane · 2024-11-27T09:01:56Z

crates/storage/storage-api/src/trie.rs

+        range: std::ops::RangeInclusive<BlockNumber>,
+    ) -> ProviderResult<(B256, TrieUpdates)>;
+
+    /// Returns the state root progress.


Suggested change

/// Returns the state root progress.

/// Tries to compute the [`StateRootProgress`].

with this link, you spare yourself explaining here what StateRootProgress type is, since we're relying on the docs for StateRootProgress to explain this, which is accessible with a click when its linked.

emhane · 2024-11-27T09:03:10Z

crates/storage/storage-api/src/trie.rs

+        state: Option<IntermediateStateRootState>,
+    ) -> ProviderResult<StateRootProgress>;
+
+    /// Returns the state root of the current state with the provided prefix sets updated.


Suggested change

/// Returns the state root of the current state with the provided prefix sets updated.

/// Tries to compute the state root of the current state with the provided prefix sets updated ([`TrieUpdates`]).

emhane · 2024-11-27T09:04:43Z

crates/trie/db/src/state.rs

@@ -116,13 +118,34 @@ pub trait DatabaseStateRoot<'a, TX>: Sized {
        tx: &'a TX,
        input: TrieInput,
    ) -> Result<(B256, TrieUpdates), StateRootError>;
+
+    /// Calculates the state root for the current state stored in the database.


Suggested change

/// Calculates the state root for the current state stored in the database.

/// Tries to calculate the state root for the current state stored in the database. Returns the state root hash or [`StateRootError`].

emhane · 2024-11-27T09:06:02Z

crates/trie/db/src/state.rs

+    /// Calculates the state root for the current state stored in the database and returns
+    /// trie updates.


Suggested change

/// Calculates the state root for the current state stored in the database and returns

/// trie updates.

/// Tries to calculate the state root for the current state stored in the database. Returns tuple of state root and corresponding [`TrieUpdates`], or [`StateRootError`].

emhane · 2024-11-27T09:06:36Z

crates/trie/db/src/state.rs

+    /// Calculates the state root for the current state stored in the database updating the paths
+    /// associated with the provided prefix sets and returns the trie updates.


Suggested change

/// Calculates the state root for the current state stored in the database updating the paths

/// associated with the provided prefix sets and returns the trie updates.

/// Tries to calculate the state root for the current state stored in the database, updating the paths

/// associated with the provided prefix sets. Returns tuple of state root and corresponding [`TrieUpdates`], or [`StateRootError`].

unclear from docs how this method differs from DatabaseStateRoot::root_with_updates - "updating the paths associated with the provided prefix sets" where? it writes updates to database and DatabaseStateRoot::root_with_updates does not? does DatabaseStateRoot::root_with_updates calculate the prefix sets in its scope instead? could you pls make this clear in the docs. you could even have sentence "Unlike [`root_with_updates`](DatabaseStateRoot::root_with_updates), this method ..."

emhane · 2024-11-27T09:09:18Z

crates/trie/db/src/state.rs

+    /// Calculates the state root for the current state stored in the database and returns the
+    /// intermediate progress of the computation.


Suggested change

/// Calculates the state root for the current state stored in the database and returns the

/// intermediate progress of the computation.

/// Tries to calculate the state root for the current state stored in the database. Returns the

/// intermediate progress ([`StateRootProgress`]) of the computation, or [`StateRootError`].

emhane · 2024-11-27T09:33:33Z

crates/storage/storage-api/src/trie.rs

@@ -41,6 +42,33 @@ pub trait StateRootProvider: Send + Sync {
    ) -> ProviderResult<(B256, TrieUpdates)>;
 }

+/// A trait that is used to compute the state root of the latest state stored in the database.


wondering if ProviderError needs a new variant ProviderError::Custom(String) for this to be fully extensible?

Yes, this would be useful. Shall I go ahead and include it in this PR?

up to core responsible for review here, scope is already big @Rjected @mattsse @shekhirin

Scope should be reduced as preceding PR's are merged:

Introduce HashedPostStateProvider #12607

Introduce HashedStorageProvider #12894

Introduce KeyHasherProvider #12895

an idea is to change the base in that case? so prs are stacked on top of each other, could speed up review process but may slow you down

The issue is that the PR's would then target my private fork if I changed the base to the preceding PR.

frisitano · 2024-11-27T10:27:00Z

@emhane very fair comment regarding the poor documentation of the introduced StateRootProviderExt trait. I will address this shortly.

frisitano · 2024-11-27T12:25:11Z

@emhane I've update the doc comments, please let me know what you think.

frisitano added 19 commits November 16, 2024 19:10

feat: introduce StateCommitment in StateProviders

51fa6bb

refactor: introduce StateCommimentProvider

52426db

Merge remote-tracking branch 'upstream/main' into feat/provider-state…

3a9fa49

…-commitment

feat: introduce HashedPostStateProvider

10af3b1

Merge remote-tracking branch 'upstream/main' into feat/provider-state…

57544dd

…-commitment

Merge branch 'feat/provider-state-commitment' into feat/hashed-post-s…

5f6977e

…tate-provider

feat: HashedPostState from reverts

500f8c2

feat: introduce HashedStorageProvider

8b24bc3

lint: revm/test-utils feature propogation

0e79635

Merge branch 'feat/hashed-post-state-provider' into feat/hashed-stora…

9a66c9d

…ge-provider

fix: add Send + Sync bound on introduced storage state api methods

593587e

Merge branch 'feat/hashed-post-state-provider' into feat/hashed-stora…

59a99c8

…ge-provider

feat: introduce KeyHasherProvider

dbeb344

feat: introduce StateRootProviderExt and integrate it (and StateRootP…

60b4f7a

…rovider) with StateCommitment

Merge remote-tracking branch 'upstream/main' into feat/hashed-post-st…

06f01c4

…ate-provider

Merge branch 'feat/hashed-post-state-provider' into feat/hashed-stora…

c824c52

…ge-provider

Merge branch 'feat/hashed-storage-provider' into feat/key-hasher-prov…

a665975

…ider

fix: use KeyHasherProvider to hash key for multiproof

776d374

Merge branch 'feat/key-hasher-provider' into feat/state-root-provider

2762ffd

frisitano requested review from rkrasiuk, Rjected, shekhirin, joshieDo, rakita, onbjerg, mattsse, emhane, fgimenez and gakonst as code owners November 27, 2024 04:28

frisitano mentioned this pull request Nov 27, 2024

Tracking: StateCommitment abstraction #11830

Open

11 tasks

fix: lint

bf1a85d

emhane requested changes Nov 27, 2024

View reviewed changes

chore: improve doc comments

f93f6a9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `StateRootProviderExt` and Integrate with `StateCommitment` #12896

Introduce `StateRootProviderExt` and Integrate with `StateCommitment` #12896

frisitano commented Nov 27, 2024

emhane Nov 27, 2024

emhane Nov 27, 2024

emhane Nov 27, 2024

emhane Nov 27, 2024

emhane Nov 27, 2024

emhane Nov 27, 2024

emhane Nov 27, 2024

emhane Nov 27, 2024

emhane Nov 27, 2024

emhane Nov 27, 2024

frisitano Nov 27, 2024

emhane Nov 27, 2024

frisitano Nov 27, 2024

emhane Nov 27, 2024

frisitano Nov 27, 2024

frisitano commented Nov 27, 2024

frisitano commented Nov 27, 2024

	/// Returns the state root of the current state and trie updates.
	/// Tries to compute the state root of the current state and [`TrieUpdates`].

	/// Returns the state root with trie updates associated with the given block range.
	/// Tries to compute the state root with [`TrieUpdates`] associated with the given block range.

	/// Returns the state root progress.
	/// Tries to compute the [`StateRootProgress`].

	/// Calculates the state root for the current state stored in the database.
	/// Tries to calculate the state root for the current state stored in the database. Returns the state root hash or [`StateRootError`].

		/// Calculates the state root for the current state stored in the database and returns
		/// trie updates.

		/// Calculates the state root for the current state stored in the database updating the paths
		/// associated with the provided prefix sets and returns the trie updates.

		/// Calculates the state root for the current state stored in the database and returns the
		/// intermediate progress of the computation.

Introduce StateRootProviderExt and Integrate with StateCommitment #12896

Are you sure you want to change the base?

Introduce StateRootProviderExt and Integrate with StateCommitment #12896

Conversation

frisitano commented Nov 27, 2024

Overview

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

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

frisitano commented Nov 27, 2024

frisitano commented Nov 27, 2024

Introduce `StateRootProviderExt` and Integrate with `StateCommitment` #12896

Introduce `StateRootProviderExt` and Integrate with `StateCommitment` #12896