Skip to content

feat: more paranoid reth db checks#911

Open
dvush wants to merge 1 commit intodevelopfrom
dvush/paranoid_reth_trie_checks
Open

feat: more paranoid reth db checks#911
dvush wants to merge 1 commit intodevelopfrom
dvush/paranoid_reth_trie_checks

Conversation

@dvush
Copy link
Copy Markdown
Contributor

@dvush dvush commented Apr 1, 2026

Now we check reth db account trie table for correct state root of the parent instead of checking latest block.

📝 Summary

💡 Motivation and Context


✅ I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

Now we check reth db account trie table for correct state root of the
parent instead of checking latest block.
@dvush dvush requested a review from ZanCorDX as a code owner April 1, 2026 07:52
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 1, 2026

Claude finished @dvush's task in 1m 14s —— View job


Code Review: feat: more paranoid reth db checks

  • Read and analyze changed files
  • Review code quality and correctness
  • Post review feedback

Summary

This PR replaces the block-hash-based DB consistency check in v2 with a state-root-based check. Instead of verifying last_block_hash against the latest block in the DB, it now computes the state root via StateRoot::from_tx().root() and compares it to the expected parent_state_root.

Issues

  1. Performance — state root recomputation is expensive (inline comment posted): check_state_root_in_db calls StateRoot::from_tx(provider.tx_ref()).root(), which walks through trie cursors to compute the state root. This is called once per storage proof target (in into_par_iter) and once for account proofs. The old check was a cheap block hash/number lookup. This could be a meaningful regression for blocks touching many accounts.

  2. v_experimental still uses the old last_block_hash pattern: v_experimental/fetch.rs and v_experimental/mod.rs still use last_block_hash with the old block-hash-based check. Was this intentional (only upgrading v2), or should v_experimental be updated to match?

  3. Minor inconsistency — missing semicolon at line 119: check_state_root_in_db(&provider, parent_state_root)? at line 119 is missing a trailing semicolon, while the equivalent call at line 84 has one. Both compile, but the inconsistency is worth fixing for readability.

  4. Unused import: DatabaseStateRoot is imported at line 20 but not directly referenced in user code (it's a trait impl). Verify this is actually needed — if it's only providing the from_tx method on StateRoot, it should stay, but it may trigger an unused-import warning depending on compiler settings.

Comment on lines +24 to +36
pub fn check_state_root_in_db(
provider: &impl DBProvider,
expected_state_root: B256,
) -> Result<(), SparseTrieError> {
let db_state_root = StateRoot::from_tx(provider.tx_ref())
.root()
.map_err(SparseTrieError::other)?;
if db_state_root == expected_state_root {
Ok(())
} else {
Err(SparseTrieError::WrongDatabaseTrieError)
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Performance concern: StateRoot::from_tx(provider.tx_ref()).root() recomputes the state root by walking the trie cursors. This is significantly more expensive than the previous check which was a simple block hash lookup.

This function is called once per storage proof target (in a parallel iterator at line 84) plus once for account proofs (line 119). For blocks with many touched accounts, this could add substantial overhead.

Is this intentional? If the goal is just to verify the DB hasn't been updated underneath, could you read the root node directly from the account trie table instead of recomputing it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is this intentional?
Yes it is, is once per database transaction.

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.

1 participant