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

docs(trie): hashed post state & cursors #3572

Merged
merged 2 commits into from
Jul 4, 2023
Merged

Conversation

rkrasiuk
Copy link
Member

@rkrasiuk rkrasiuk commented Jul 4, 2023

Description

Improve documentation for the HashedPostState and cursors.

@mattsse PTAL.

@rkrasiuk rkrasiuk added C-docs An addition or correction to our documentation A-trie Related to Merkle Patricia Trie implementation labels Jul 4, 2023
@rkrasiuk rkrasiuk requested a review from mattsse July 4, 2023 08:29
@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #3572 (fb79551) into main (4f32f56) will decrease coverage by 0.16%.
The diff coverage is 32.77%.

Impacted file tree graph

Impacted Files Coverage Δ
crates/rpc/rpc/src/eth/api/fees.rs 48.41% <0.00%> (ø)
crates/rpc/rpc/src/eth/pubsub.rs 7.38% <0.00%> (ø)
crates/storage/libmdbx-rs/src/cursor.rs 90.45% <ø> (ø)
crates/storage/provider/src/post_state/mod.rs 95.73% <ø> (ø)
crates/storage/provider/src/traits/state.rs 28.07% <ø> (ø)
crates/transaction-pool/src/lib.rs 36.00% <ø> (ø)
crates/transaction-pool/src/test_utils/mod.rs 100.00% <ø> (+5.55%) ⬆️
crates/transaction-pool/src/traits.rs 5.12% <0.00%> (-0.14%) ⬇️
crates/transaction-pool/src/noop.rs 11.38% <11.38%> (ø)
crates/blockchain-tree/src/blockchain_tree.rs 81.84% <50.00%> (ø)
... and 16 more

... and 40 files with indirect coverage changes

Flag Coverage Δ
integration-tests 16.12% <21.11%> (-0.12%) ⬇️
unit-tests 63.99% <25.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 26.46% <ø> (+3.45%) ⬆️
blockchain tree 81.25% <75.00%> (ø)
pipeline 86.89% <100.00%> (-0.09%) ⬇️
storage (db) 73.48% <95.23%> (-0.34%) ⬇️
trie 95.64% <100.00%> (+<0.01%) ⬆️
txpool 49.46% <10.93%> (-1.69%) ⬇️
networking 77.88% <100.00%> (+0.01%) ⬆️
rpc 57.98% <72.22%> (+0.10%) ⬆️
consensus 62.58% <ø> (ø)
revm 34.95% <100.00%> (-0.05%) ⬇️
payload builder 6.83% <ø> (ø)
primitives 88.29% <ø> (-0.21%) ⬇️

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.

ty,

what would be useful is an explainer how this works:

/// Returns the state root of the PostState on top of the current state.
fn state_root(&self, post_state: PostState) -> Result<H256>;

especially on PostState, it's not obvious how all the pieces tie together here which makes it very hard to understand why this can be a perf bottleneck:

there are like 4 steps required here to get the state root:

let canonical_fork = post_state_data_provider.canonical_fork();
let state_provider = db.history_by_block_number(canonical_fork.number)?;
let provider = PostStateProvider::new(state_provider, post_state_data_provider);
let mut executor = externals.executor_factory.with_sp(&provider);
let post_state = executor.execute_and_verify_receipt(&block, U256::MAX, Some(senders))?;
// check state root if the block extends the canonical chain.
if block_kind.extends_canonical_head() {
// check state root
let state_root = provider.state_root(post_state.clone())?;
if block.state_root != state_root {
return Err(ConsensusError::BodyStateRootDiff {
got: state_root,
expected: block.state_root,
}
.into())
}
}

@rkrasiuk
Copy link
Member Author

rkrasiuk commented Jul 4, 2023

@mattsse since this is already documented on PostState::state_root_slow, i simply referenced it in StateRootProvider::state_root docs

/// Calculate the state root for this [PostState].
/// Internally, function calls [Self::hash_state_slow] to obtain the [HashedPostState].
/// Afterwards, it retrieves the [PrefixSets](reth_trie::prefix_set::PrefixSet) of changed keys
/// from the [HashedPostState] and uses them to calculate the incremental state root.
///
/// # Example
///
/// ```
/// use reth_primitives::{Address, Account};
/// use reth_provider::PostState;
/// use reth_db::{test_utils::create_test_rw_db, database::Database};
///
/// // Initialize the database
/// let db = create_test_rw_db();
///
/// // Initialize the post state
/// let mut post_state = PostState::new();
///
/// // Create an account
/// let block_number = 1;
/// let address = Address::random();
/// post_state.create_account(1, address, Account { nonce: 1, ..Default::default() });
///
/// // Calculate the state root
/// let tx = db.tx().expect("failed to create transaction");
/// let state_root = post_state.state_root_slow(&tx);
/// ```
///
/// # Returns
///
/// The state root for this [PostState].
pub fn state_root_slow<'a, 'tx, TX: DbTx<'tx>>(

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.

send it

@rkrasiuk rkrasiuk added this pull request to the merge queue Jul 4, 2023
Merged via the queue into main with commit c236521 Jul 4, 2023
@rkrasiuk rkrasiuk deleted the rkrasiuk/state-root-docs branch July 4, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trie Related to Merkle Patricia Trie implementation C-docs An addition or correction to our documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants