-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: Add TrieUpdatesSorted and HashedPostStateSorted in all ExEx notifications
#20333
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
base: main
Are you sure you want to change the base?
feat: Add TrieUpdatesSorted and HashedPostStateSorted in all ExEx notifications
#20333
Conversation
This changes the TrieUpdates field of the Chain type (which is used in ExEx notifications) from an Option into a BTreeMap mapping the block number to the updates pulled from the ExecutedBlock. With this in place all ExEx subscribes have full access to all trie updates produced by each block.
hashed_state [WIP]
hashed_state [WIP] hashed_state
|
seems like needs rebase. probably easier if you simply cherry-pick the commits that are new to this pr (the ones that don't have a pr link in brackets at end), onto the base branch, and then force push @dhyaniarun1993 |
I think it's upto date with main. The base branch is not updated. |
crates/exex/exex/src/wal/storage.rs
Outdated
| let block_number = block.header().number(); | ||
|
|
||
| // Create some non-empty trie updates and hashed state to ensure the 4-field format is used | ||
| let trie_updates = TrieUpdatesSorted::default(); |
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.
Can we include some nominal trie updates/state changes to make this fully end-to-end? If someone accidentally breaks either of those serialization formats we would want this new .wal file test to break.
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.
Sure. I add some cases here.
|
Just a minor nit about the test case, and then the feature propagation CI error, other than that this looks good, thanks! 👍 |
hashed_stateTrieUpdatesSorted and HashedPostStateSorted in all ExEx notifications
emhane
left a comment
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.
we want to
| /// Bincode-compatible [`super::TrieUpdatesSorted`] serde implementation. | ||
| /// | ||
| /// Intended to use with the [`serde_with::serde_as`] macro in the following way: | ||
| /// ```rust | ||
| /// use reth_trie_common::{serde_bincode_compat, updates::TrieUpdatesSorted}; | ||
| /// use serde::{Deserialize, Serialize}; | ||
| /// use serde_with::serde_as; | ||
| /// | ||
| /// #[serde_as] | ||
| /// #[derive(Serialize, Deserialize)] | ||
| /// struct Data { | ||
| /// #[serde_as(as = "serde_bincode_compat::updates::TrieUpdatesSorted")] | ||
| /// trie_updates: TrieUpdatesSorted, | ||
| /// } | ||
| /// ``` | ||
| #[derive(Debug, Serialize, Deserialize)] | ||
| pub struct TrieUpdatesSorted<'a> { | ||
| account_nodes: Cow<'a, [(Nibbles, Option<BranchNodeCompact>)]>, | ||
| storage_tries: B256Map<StorageTrieUpdatesSorted<'a>>, | ||
| } |
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 think we want to create a new module (can be part of this file) called serde_bincode_compat, and feature gate this code behind the existing feature for this crate serde-bincode-compat. same for the hashed post state .
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.
Yep, we are already doing that https://github.com/op-rs/op-reth/blob/8605dc3b378f5bbce808e16a0dc7c0f0d7650433/crates/trie/common/src/lib.rs#L72-L78
Based onincludes changes from #19355