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.

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

feat: introduce StateCommitment type #11842

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

frisitano
Copy link
Contributor

@frisitano frisitano commented Oct 17, 2024

Overview

This PR introduces StateCommitment and KeyHasher traits. The StateCommitment trait is integrated as an associated type in the NodeTypes trait. A implementation of the StateCommitment trait is provided for the MerklePatriciaTrie which represents the state commitment used in ethereum.

supports: #11830

Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

very cool!

Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

Cool! I just have one question about the TX generic in the GAT

Comment on lines +12 to +19
/// The state root type.
type StateRoot<'a, TX: DbTx + 'a>: DatabaseStateRoot<'a, TX>;
/// The storage root type.
type StorageRoot<'a, TX: DbTx + 'a>: DatabaseStorageRoot<'a, TX>;
/// The state proof type.
type StateProof<'a, TX: DbTx + 'a>: DatabaseProof<'a, TX>;
/// The state witness type.
type StateWitness<'a, TX: DbTx + 'a>: DatabaseTrieWitness<'a, TX>;
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused as to why these require DbTx bounds? Is it just because these must implement the Database* traits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a consequence of the impl of DatabaseStateRoot on StateRoot. As we see below we have a DbTx bound on TX which is required for certain methods used in the implementation.

impl<'a, TX: DbTx> DatabaseStateRoot<'a, TX>
for StateRoot<DatabaseTrieCursorFactory<'a, TX>, DatabaseHashedCursorFactory<'a, TX>>

As a consequence of this in the implementation of StateCommitment for the MerklePatriciaTrie we must also have a DbTx bound on the StateRoot type as seen below:

impl StateCommitment for MerklePatriciaTrie {
    type StateRoot<'a, TX: DbTx + 'a> =
        StateRoot<DatabaseTrieCursorFactory<'a, TX>, DatabaseHashedCursorFactory<'a, TX>>;
        ...
}

Rust does not allow for an impl of a trait to have more strict bounds than the trait itself and therefore me must propagate the bound to the trait itself. The above rationale is also applicable for the other types in StateCommitment.

Copy link
Member

Choose a reason for hiding this comment

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

I just added this issue: #12216

note that it should not block any of this work, but eventually these traits could be simplified

@frisitano
Copy link
Contributor Author

frisitano commented Oct 22, 2024

What is the preferred method or fixing conflicts? Rebase and force-push or merge upstream?

@frisitano frisitano requested a review from Rjected October 22, 2024 11:27
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.

do we expect that a Statecommitment instance will be stateful?

I still need to look at the other prs before we can make a decision on this direction, but I think this could work

crates/trie/db/src/commitment.rs Show resolved Hide resolved
crates/node/types/src/lib.rs Outdated Show resolved Hide resolved
@mattsse
Copy link
Collaborator

mattsse commented Oct 24, 2024

okay, after I looked at the other PRs I get it now.

this should work, I believe there'll likely be things we need to change down the road but this is the best we can do now

@frisitano
Copy link
Contributor Author

No we wouldn't expect that a StateCommitment instance would be state-full.

I'm out of office until Monday. I will address feedback and rebase upstream upon my return.

@frisitano frisitano force-pushed the feat/state-commitment-trait branch 3 times, most recently from 00c874f to 819a632 Compare October 29, 2024 16:40
@frisitano frisitano force-pushed the feat/state-commitment-trait branch from 819a632 to 928bcaa Compare October 29, 2024 16:40
@frisitano frisitano requested a review from mattsse October 29, 2024 16:59
@frisitano
Copy link
Contributor Author

@joshieDo could you provide a review on this when you have time please? We have a number of PR's lined up that will work towards #7434 in an incremental fashion.

Copy link
Member

@rkrasiuk rkrasiuk left a comment

Choose a reason for hiding this comment

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

nice

@joshieDo joshieDo added this pull request to the merge queue Oct 30, 2024
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.

cool, lets get all of this over the line before we introduce a ton of additional conflicts

Merged via the queue into paradigmxyz:main with commit 129f3ba Oct 30, 2024
41 checks passed
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.

6 participants