Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: provider StateCommitment integration #11867

Closed
wants to merge 1 commit into from
Closed

feat: provider StateCommitment integration #11867

wants to merge 1 commit into from

Conversation

frisitano
Copy link
Contributor

@frisitano frisitano commented Oct 18, 2024 •
edited
Loading

Dependencies

This PR builds on #11842 and as such it should be reviewed after #11842 has been merged.

Overview

This PR introduces the StateCommitment type in StateProviders such that the type can be used for state commitment operations. In this PR we do not utilise the types for state commitment operations, that will be implemented in follow up PR's.

ToLatestStateProviderRef trait

We introduce ToLatestStateProviderRef trait and implement it on DatabaseProvider such that we can instantiate a LatestStateProviderRef from a DatabaseProvider without leaking StateCommitment type data. We update components that instantiate LatestStateProviderRef objects from a DatabaseProvider to leverage this trait.

supports: #11830

@frisitano frisitano mentioned this pull request Oct 18, 2024
Open
11 tasks
@frisitano frisitano changed the title Feat/provider state commitment feat: provider StateCommitment integration Oct 18, 2024
@frisitano frisitano changed the title feat: provider StateCommitment integration feat: provider StateCommitment integration Oct 18, 2024
@frisitano frisitano force-pushed the feat/provider-state-commitment branch from 871cbc7 to cb77bce Compare October 18, 2024 06:14
This was referenced Oct 18, 2024
Closed
Closed
@frisitano frisitano force-pushed the feat/provider-state-commitment branch from cb77bce to 03ea70e Compare October 18, 2024 13:03
@frisitano frisitano force-pushed the feat/provider-state-commitment branch 4 times, most recently from 27f08e0 to 6f3498c Compare October 29, 2024 17:52
joshieDo
joshieDo reviewed Oct 30, 2024
View reviewed changes
crates/storage/storage-api/src/state.rs Outdated
@@ -91,6 +91,13 @@ pub trait TryIntoHistoricalStateProvider {
) -> ProviderResult<StateProviderBox>;
}

/// Trait implemented for database providers that can be converted into a latest state provider
/// reference.
pub trait ToLatestStateProviderRef {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe AsLatestStateProviderRef? When I read To i thought it was consuming the tx

Copy link
Contributor Author

@frisitano frisitano Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think generally we would use Into when the tx is being consumed. I chose To because I believe it best fits the guidelines here as there is a clone of the static_file_provider. However, I'm not strongly opinionated and happy to change this to AsLatestStateProviderRef.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to AsLatestStateProviderRef as suggested.

@frisitano frisitano force-pushed the feat/provider-state-commitment branch 3 times, most recently from 4ab021d to a271400 Compare October 30, 2024 09:41
mattsse
mattsse reviewed Oct 30, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this looks a bit invasive, looking at #11872 (review) it appears that we only this to have access to the keyhasher,
are there more use cases where the state provider needs to directly access the State.

I assume eventually all types of the StateCommitment must be integrated into the XStateProvider? types?

then we def need to add the phantom type in there, so this makes sense

crates/storage/provider/src/providers/state/historical.rs Outdated
@@ -41,7 +41,7 @@ use std::fmt::Debug;
/// - [`tables::AccountChangeSets`]
/// - [`tables::StorageChangeSets`]
#[derive(Debug)]
pub struct HistoricalStateProviderRef<'b, TX: DbTx> {
pub struct HistoricalStateProviderRef<'b, TX: DbTx, SC: StateCommitment> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this additional generic looks weird to me, because we only use it to capture the type

@Rjected Rjected added C-enhancement New feature or request A-trie Related to Merkle Patricia Trie implementation A-sdk Related to reth's use as a library labels Oct 30, 2024
@frisitano
Copy link
Contributor Author

frisitano commented Oct 31, 2024 •
edited
Loading

this looks a bit invasive, looking at #11872 (review) it appears that we only this to have access to the keyhasher, are there more use cases where the state provider needs to directly access the State.

I assume eventually all types of the StateCommitment must be integrated into the XStateProvider? types?

then we def need to add the phantom type in there, so this makes sense

Yes the idea is that all types of StateCommitment will be integrated into XStateProvider. We will do this incrementally one impl of a trait at a time. We start with the newly introduced HasehdPostStateProvider as this is a precursor to other state commitment operations. We will then follow up by integrating StateCommitment types into StateRootProvider, StorageRootProvider, StateProofProvider, etc impls. We will do this trait by trait / impl by impl to keep the surface area of the change concise and easily reviewable. I would direct you to the initial POC PR that serves as an example of the StateCommitment type being used more extensively - #11786.

@frisitano frisitano force-pushed the feat/provider-state-commitment branch from a271400 to 133614d Compare October 31, 2024 06:24
@frisitano frisitano requested a review from mattsse October 31, 2024 10:13
This was referenced Nov 14, 2024
Closed
Merged
@frisitano
Copy link
Contributor Author

superseded by #12602

@frisitano frisitano closed this Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshieDo joshieDo joshieDo approved these changes

@rkrasiuk rkrasiuk Awaiting requested review from rkrasiuk rkrasiuk is a code owner

@Rjected Rjected Awaiting requested review from Rjected

@shekhirin shekhirin Awaiting requested review from shekhirin shekhirin is a code owner

@rakita rakita Awaiting requested review from rakita rakita is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg onbjerg is a code owner

@fgimenez fgimenez Awaiting requested review from fgimenez

@emhane emhane Awaiting requested review from emhane

@gakonst gakonst Awaiting requested review from gakonst

@mattsse mattsse Awaiting requested review from mattsse

Assignees
No one assigned
Labels
A-sdk Related to reth's use as a library A-trie Related to Merkle Patricia Trie implementation C-enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@frisitano @mattsse @joshieDo @Rjected