-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: introduce StateCommitment type #11842
Conversation
e4c6294
to
5b5fe8f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very cool!
There was a problem hiding this 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
/// 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>; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
reth/crates/trie/db/src/state.rs
Lines 132 to 133 in 51594c9
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
.
There was a problem hiding this comment.
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
What is the preferred method or fixing conflicts? Rebase and force-push or merge upstream? |
There was a problem hiding this 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
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 |
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. |
00c874f
to
819a632
Compare
819a632
to
928bcaa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
There was a problem hiding this 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
Overview
This PR introduces
StateCommitment
andKeyHasher
traits. TheStateCommitment
trait is integrated as an associated type in theNodeTypes
trait. A implementation of theStateCommitment
trait is provided for theMerklePatriciaTrie
which represents the state commitment used in ethereum.supports: #11830