Skip to content

Thread &dyn Crypto through all keccak call sites in block validation #6365

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.

Sign up for GitHub

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

Open
ilitteri wants to merge 2 commits into
base: main
Choose a base branch
from
+355 −292
Open

Thread &dyn Crypto through all keccak call sites in block validation #6365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a3b0eff
Thread &dyn Crypto through all keccak call sites in the block validat…
ilitteri Mar 13, 2026
c4b8503
Fix three NativeCrypto leaks in the guest program execution path
ilitteri Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/ethrex/cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,7 +736,7 @@ pub async fn import_blocks(
continue;
}

validate_block_body(&block.header, &block.body)
validate_block_body(&block.header, &block.body, &ethrex_crypto::NativeCrypto)
.map_err(InvalidBlockError::InvalidBody)?;

if index + MIN_FULL_BLOCKS < size {
Expand Down Expand Up @@ -854,7 +854,7 @@ pub async fn import_blocks_bench(
continue;
}

validate_block_body(&block.header, &block.body)
validate_block_body(&block.header, &block.body, &ethrex_crypto::NativeCrypto)
.map_err(InvalidBlockError::InvalidBody)?;

blockchain
Expand Down
30 changes: 15 additions & 15 deletions crates/blockchain/blockchain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ impl Blockchain {

// Validate execution went alright
validate_gas_used(execution_result.block_gas_used, &block.header)?;
validate_receipts_root(&block.header, &execution_result.receipts)?;
validate_receipts_root(&block.header, &execution_result.receipts, &NativeCrypto)?;
validate_requests_hash(&block.header, &chain_config, &execution_result.requests)?;
if let Some(bal) = &bal {
validate_block_access_list_hash(
Expand Down Expand Up @@ -458,7 +458,7 @@ impl Blockchain {

// Validate execution went alright
validate_gas_used(execution_result.block_gas_used, &block.header)?;
validate_receipts_root(&block.header, &execution_result.receipts)?;
validate_receipts_root(&block.header, &execution_result.receipts, &NativeCrypto)?;
validate_requests_hash(
&block.header,
&chain_config,
Expand Down Expand Up @@ -725,8 +725,8 @@ impl Blockchain {
let state_trie_hash =
if let Some(root) = self.collapse_root_node(parent_header, None, root)? {
let mut root = NodeRef::from(root);
let hash = root.commit(Nibbles::default(), &mut state_updates);
hash.finalize()
let hash = root.commit(Nibbles::default(), &mut state_updates, &NativeCrypto);
hash.finalize(&NativeCrypto)
} else {
state_updates.push((Nibbles::default(), vec![RLP_NULL]));
*EMPTY_TRIE_HASH
Expand Down Expand Up @@ -914,7 +914,7 @@ impl Blockchain {
}

let (root_hash, nodes) =
trie.collect_changes_since_last_hash();
trie.collect_changes_since_last_hash(&NativeCrypto);
results.push((idx, root_hash, nodes));
}
Ok(results)
Expand Down Expand Up @@ -1033,8 +1033,8 @@ impl Blockchain {
let state_trie_hash =
if let Some(root) = self.collapse_root_node(parent_header, None, root)? {
let mut root = NodeRef::from(root);
let hash = root.commit(Nibbles::default(), &mut state_updates);
hash.finalize()
let hash = root.commit(Nibbles::default(), &mut state_updates, &NativeCrypto);
hash.finalize(&NativeCrypto)
} else {
state_updates.push((Nibbles::default(), vec![RLP_NULL]));
*EMPTY_TRIE_HASH
Expand Down Expand Up @@ -1183,8 +1183,8 @@ impl Blockchain {
self.collapse_root_node(parent_header, Some(hashed_account), *root)?
{
let mut root = NodeRef::from(root);
let hash = root.commit(Nibbles::default(), &mut state.nodes);
storage_root = Some(hash.finalize());
let hash = root.commit(Nibbles::default(), &mut state.nodes, &NativeCrypto);
storage_root = Some(hash.finalize(&NativeCrypto));
} else {
state.nodes.push((Nibbles::default(), vec![RLP_NULL]));
storage_root = Some(*EMPTY_TRIE_HASH);
Expand Down Expand Up @@ -1259,7 +1259,7 @@ impl Blockchain {
let (execution_result, bal) = vm.execute_block(block)?;
// Validate execution went alright
validate_gas_used(execution_result.block_gas_used, &block.header)?;
validate_receipts_root(&block.header, &execution_result.receipts)?;
validate_receipts_root(&block.header, &execution_result.receipts, &NativeCrypto)?;
validate_requests_hash(&block.header, chain_config, &execution_result.requests)?;
if let Some(bal) = &bal {
validate_block_access_list_hash(
Expand Down Expand Up @@ -1299,7 +1299,7 @@ impl Blockchain {
.state_trie(first_block_header.parent_hash)
.map_err(|_| ChainError::ParentStateNotFound)?
.ok_or(ChainError::ParentStateNotFound)?;
let initial_state_root = trie.hash_no_commit();
let initial_state_root = trie.hash_no_commit(&NativeCrypto);

let (mut current_trie_witness, mut trie) = TrieLogger::open_trie(trie);

Expand Down Expand Up @@ -1564,7 +1564,7 @@ impl Blockchain {
// Get initial state trie root and embed the rest of the trie into it
let nodes: BTreeMap<H256, Node> = used_trie_nodes
.into_iter()
.map(|node| (node.compute_hash().finalize(), node))
.map(|node| (node.compute_hash(&NativeCrypto).finalize(&NativeCrypto), node))
.collect();
let state_trie_root = if let NodeRef::Node(state_trie_root, _) =
Trie::get_embedded_root(&nodes, initial_state_root)?
Expand Down Expand Up @@ -1630,7 +1630,7 @@ impl Blockchain {
.state_trie(parent_header.hash())
.map_err(|_| ChainError::ParentStateNotFound)?
.ok_or(ChainError::ParentStateNotFound)?;
let initial_state_root = trie.hash_no_commit();
let initial_state_root = trie.hash_no_commit(&NativeCrypto);

let (trie_witness, trie) = TrieLogger::open_trie(trie);

Expand Down Expand Up @@ -1806,7 +1806,7 @@ impl Blockchain {
// Get initial state trie root and embed the rest of the trie into it
let nodes: BTreeMap<H256, Node> = used_trie_nodes
.into_iter()
.map(|node| (node.compute_hash().finalize(), node))
.map(|node| (node.compute_hash(&NativeCrypto).finalize(&NativeCrypto), node))
.collect();
let state_trie_root = if let NodeRef::Node(state_trie_root, _) =
Trie::get_embedded_root(&nodes, initial_state_root)?
Expand Down Expand Up @@ -2806,7 +2806,7 @@ fn collect_trie(index: u8, mut trie: Trie) -> Result<(Box<BranchNode>, Vec<TrieN
.unwrap_or_else(|| Node::Branch(Box::default())),
);
trie.root = Node::Branch(root).into();
let (_, mut nodes) = trie.collect_changes_since_last_hash();
let (_, mut nodes) = trie.collect_changes_since_last_hash(&NativeCrypto);
nodes.retain(|(nib, _)| nib.as_ref().first() == Some(&index));

let Some(Node::Branch(root)) = trie.root_node()?.map(Arc::unwrap_or_clone) else {
Expand Down
12 changes: 7 additions & 5 deletions crates/blockchain/payload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use ethrex_common::{
},
};

use ethrex_crypto::NativeCrypto;
use ethrex_crypto::keccak::Keccak256;
use ethrex_vm::{Evm, EvmError};

Expand Down Expand Up @@ -143,8 +144,8 @@ pub fn create_payload(
ommers_hash: *DEFAULT_OMMERS_HASH,
coinbase: args.fee_recipient,
state_root: parent_block.state_root,
transactions_root: compute_transactions_root(&[]),
receipts_root: compute_receipts_root(&[]),
transactions_root: compute_transactions_root(&[], &NativeCrypto),
receipts_root: compute_receipts_root(&[], &NativeCrypto),
logs_bloom: Bloom::default(),
difficulty: U256::zero(),
number: parent_block.number.saturating_add(1),
Expand All @@ -165,6 +166,7 @@ pub fn create_payload(
.is_shanghai_activated(args.timestamp)
.then_some(compute_withdrawals_root(
args.withdrawals.as_ref().unwrap_or(&Vec::new()),
&NativeCrypto,
)),
blob_gas_used: chain_config
.is_cancun_activated(args.timestamp)
Expand Down Expand Up @@ -745,8 +747,8 @@ impl Blockchain {

context.payload.header.state_root = state_root;
context.payload.header.transactions_root =
compute_transactions_root(&context.payload.body.transactions);
context.payload.header.receipts_root = compute_receipts_root(&context.receipts);
compute_transactions_root(&context.payload.body.transactions, &NativeCrypto);
context.payload.header.receipts_root = compute_receipts_root(&context.receipts, &NativeCrypto);
context.payload.header.requests_hash = context
.requests
.as_ref()
Expand All @@ -766,7 +768,7 @@ impl Blockchain {
}
}

context.payload.header.logs_bloom = bloom_from_logs(&logs);
context.payload.header.logs_bloom = bloom_from_logs(&logs, &NativeCrypto);
Ok(())
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/common/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub mod tracing;
pub mod utils;

pub use errors::InvalidBlockError;
pub use ethrex_crypto::CryptoError;
pub use ethrex_crypto::{CryptoError, NativeCrypto};
pub use validation::{
get_total_blob_gas, validate_block_access_list_hash, validate_block_pre_execution,
validate_gas_used, validate_receipts_root, validate_requests_hash,
Expand Down
2 changes: 1 addition & 1 deletion crates/common/trie/db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ impl InMemoryTrieDB {
) -> Result<Self, TrieError> {
let mut embedded_root = Trie::get_embedded_root(state_nodes, root_hash)?;
let mut hashed_nodes = vec![];
embedded_root.commit(Nibbles::default(), &mut hashed_nodes);
embedded_root.commit(Nibbles::default(), &mut hashed_nodes, &ethrex_crypto::NativeCrypto);

let hashed_nodes = hashed_nodes
.into_iter()
Expand Down
5 changes: 3 additions & 2 deletions crates/common/trie/logger.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::{
sync::{Arc, Mutex},
};

use ethrex_crypto::NativeCrypto;
use ethrex_rlp::decode::RLPDecode;

use crate::{Nibbles, Node, NodeHash, Trie, TrieDB, TrieError};
Expand All @@ -21,7 +22,7 @@ impl TrieLogger {
}

pub fn open_trie(trie: Trie) -> (TrieWitness, Trie) {
let root = trie.hash_no_commit();
let root = trie.hash_no_commit(&NativeCrypto);
let db = trie.db;
let witness = Arc::new(Mutex::new(HashMap::new()));
let logger = TrieLogger {
Expand All @@ -39,7 +40,7 @@ impl TrieDB for TrieLogger {
&& let Ok(decoded) = Node::decode(result)
{
let mut lock = self.witness.lock().map_err(|_| TrieError::LockError)?;
lock.insert(decoded.compute_hash(), decoded);
lock.insert(decoded.compute_hash(&NativeCrypto), decoded);
}
Ok(result)
}
Expand Down
67 changes: 39 additions & 28 deletions crates/common/trie/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ use rkyv::{
with::Skip,
};

use ethrex_crypto::{Crypto, NativeCrypto};

use crate::{NodeRLP, TrieDB, error::TrieError, nibbles::Nibbles};

use super::{ValueRLP, node_hash::NodeHash};
Expand Down Expand Up @@ -82,7 +84,8 @@ impl NodeRef {
.get(path)?
.filter(|rlp| !rlp.is_empty())
.and_then(|rlp| match Node::decode(&rlp) {
Ok(node) => (node.compute_hash() == *hash).then_some(Ok(Arc::new(node))),
Ok(node) => (node.compute_hash(&NativeCrypto) == *hash)
.then_some(Ok(Arc::new(node))),
Comment on lines +87 to +88
Copy link

@greptile-apps greptile-apps bot Mar 13, 2026

Choose a reason for hiding this comment

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

get_node_checked still uses NativeCrypto — missed keccak call site

get_node_checked verifies a loaded node's hash with NativeCrypto before returning it. Two consequences:

  1. Performance: if this branch is hit during zkVM execution (e.g. an incomplete witness that needs a DB lookup), the keccak is computed outside the zkVM's circuit accelerator, defeating the purpose of this PR for that node.
  2. Cache poisoning: node.compute_hash(&NativeCrypto) calls memoize_hashes internally and writes the result into the node's OnceLock. If the same NodeRef::Node is later commit()-ed with the injected crypto, the OnceLock is already set (from the NativeCrypto path) and the injected crypto is never used for that node.

Both crypto implementations produce the same Keccak-256 output, so there is no correctness bug in normal operation. But if a properly-formed witness is incomplete (any hash-referenced node must be fetched), the zkVM won't count those hashes against its circuit, under-reporting keccak usage — the same root cause the PR was written to fix.

Consider threading crypto: &dyn Crypto into get_node_checked (mirroring the treatment of commit / compute_hash_no_alloc), or adding a comment explaining why native keccak is intentional here (e.g. "only reachable outside the guest-program path").

Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/common/trie/node.rs
Line: 87-88

Comment:
**`get_node_checked` still uses `NativeCrypto` — missed keccak call site**

`get_node_checked` verifies a loaded node's hash with `NativeCrypto` before returning it. Two consequences:

1. **Performance**: if this branch is hit during zkVM execution (e.g. an incomplete witness that needs a DB lookup), the keccak is computed outside the zkVM's circuit accelerator, defeating the purpose of this PR for that node.
2. **Cache poisoning**: `node.compute_hash(&NativeCrypto)` calls `memoize_hashes` internally and writes the result into the node's `OnceLock`. If the same `NodeRef::Node` is later `commit()`-ed with the injected crypto, the `OnceLock` is already set (from the `NativeCrypto` path) and the injected crypto is never used for that node.

Both crypto implementations produce the same Keccak-256 output, so there is **no correctness bug** in normal operation. But if a properly-formed witness is incomplete (any hash-referenced node must be fetched), the zkVM won't count those hashes against its circuit, under-reporting keccak usage — the same root cause the PR was written to fix.

Consider threading `crypto: &dyn Crypto` into `get_node_checked` (mirroring the treatment of `commit` / `compute_hash_no_alloc`), or adding a comment explaining why native keccak is intentional here (e.g. "only reachable outside the guest-program path").

How can I resolve this? If you propose a fix, please make it concise.

Err(err) => Some(Err(TrieError::RLPDecode(err))),
})
.transpose(),
Expand Down Expand Up @@ -129,7 +132,12 @@ impl NodeRef {
}
}

pub fn commit(&mut self, path: Nibbles, acc: &mut Vec<(Nibbles, Vec<u8>)>) -> NodeHash {
pub fn commit(
&mut self,
path: Nibbles,
acc: &mut Vec<(Nibbles, Vec<u8>)>,
crypto: &dyn Crypto,
) -> NodeHash {
match *self {
NodeRef::Node(ref mut node, ref mut hash) => {
if let Some(hash) = hash.get() {
Expand All @@ -138,17 +146,17 @@ impl NodeRef {
match Arc::make_mut(node) {
Node::Branch(node) => {
for (choice, node) in &mut node.choices.iter_mut().enumerate() {
node.commit(path.append_new(choice as u8), acc);
node.commit(path.append_new(choice as u8), acc, crypto);
}
}
Node::Extension(node) => {
node.child.commit(path.concat(&node.prefix), acc);
node.child.commit(path.concat(&node.prefix), acc, crypto);
}
Node::Leaf(_) => {}
}
let mut buf = Vec::new();
node.encode(&mut buf);
let hash = *hash.get_or_init(|| NodeHash::from_encoded(&buf));
let hash = *hash.get_or_init(|| NodeHash::from_encoded(&buf, crypto));
if let Node::Leaf(leaf) = node.as_ref() {
acc.push((path.concat(&leaf.partial), leaf.value.clone()));
}
Expand All @@ -160,30 +168,32 @@ impl NodeRef {
}
}

pub fn compute_hash(&self) -> NodeHash {
*self.compute_hash_ref()
pub fn compute_hash(&self, crypto: &dyn Crypto) -> NodeHash {
*self.compute_hash_ref(crypto)
}

pub fn compute_hash_ref(&self) -> &NodeHash {
pub fn compute_hash_ref(&self, crypto: &dyn Crypto) -> &NodeHash {
match self {
NodeRef::Node(node, hash) => hash.get_or_init(|| node.compute_hash()),
NodeRef::Node(node, hash) => hash.get_or_init(|| node.compute_hash(crypto)),
NodeRef::Hash(hash) => hash,
}
}

pub fn compute_hash_no_alloc(&self, buf: &mut Vec<u8>) -> &NodeHash {
pub fn compute_hash_no_alloc(&self, buf: &mut Vec<u8>, crypto: &dyn Crypto) -> &NodeHash {
match self {
NodeRef::Node(node, hash) => hash.get_or_init(|| node.compute_hash_no_alloc(buf)),
NodeRef::Node(node, hash) => {
hash.get_or_init(|| node.compute_hash_no_alloc(buf, crypto))
}
NodeRef::Hash(hash) => hash,
}
}

pub fn memoize_hashes(&self, buf: &mut Vec<u8>) {
pub fn memoize_hashes(&self, buf: &mut Vec<u8>, crypto: &dyn Crypto) {
if let NodeRef::Node(node, hash) = &self
&& hash.get().is_none()
{
node.memoize_hashes(buf);
let _ = hash.set(node.compute_hash_no_alloc(buf));
node.memoize_hashes(buf, crypto);
let _ = hash.set(node.compute_hash_no_alloc(buf, crypto));
}
}

Expand Down Expand Up @@ -225,7 +235,8 @@ impl From<Arc<Node>> for NodeRef {
impl PartialEq for NodeRef {
fn eq(&self, other: &Self) -> bool {
let mut buf = Vec::new();
self.compute_hash_no_alloc(&mut buf) == other.compute_hash_no_alloc(&mut buf)
self.compute_hash_no_alloc(&mut buf, &NativeCrypto)
== other.compute_hash_no_alloc(&mut buf, &NativeCrypto)
}
}

Expand Down Expand Up @@ -365,36 +376,36 @@ impl Node {
}

/// Computes the node's hash
pub fn compute_hash(&self) -> NodeHash {
pub fn compute_hash(&self, crypto: &dyn Crypto) -> NodeHash {
let mut buf = Vec::new();
self.memoize_hashes(&mut buf);
self.memoize_hashes(&mut buf, crypto);
match self {
Node::Branch(n) => n.compute_hash_no_alloc(&mut buf),
Node::Extension(n) => n.compute_hash_no_alloc(&mut buf),
Node::Leaf(n) => n.compute_hash_no_alloc(&mut buf),
Node::Branch(n) => n.compute_hash_no_alloc(&mut buf, crypto),
Node::Extension(n) => n.compute_hash_no_alloc(&mut buf, crypto),
Node::Leaf(n) => n.compute_hash_no_alloc(&mut buf, crypto),
}
}

/// Computes the node's hash
pub fn compute_hash_no_alloc(&self, buf: &mut Vec<u8>) -> NodeHash {
self.memoize_hashes(buf);
pub fn compute_hash_no_alloc(&self, buf: &mut Vec<u8>, crypto: &dyn Crypto) -> NodeHash {
self.memoize_hashes(buf, crypto);
match self {
Node::Branch(n) => n.compute_hash_no_alloc(buf),
Node::Extension(n) => n.compute_hash_no_alloc(buf),
Node::Leaf(n) => n.compute_hash_no_alloc(buf),
Node::Branch(n) => n.compute_hash_no_alloc(buf, crypto),
Node::Extension(n) => n.compute_hash_no_alloc(buf, crypto),
Node::Leaf(n) => n.compute_hash_no_alloc(buf, crypto),
}
}

/// Recursively memoizes the hashes of all nodes of the subtrie that has
/// `self` as root (post-order traversal)
pub fn memoize_hashes(&self, buf: &mut Vec<u8>) {
pub fn memoize_hashes(&self, buf: &mut Vec<u8>, crypto: &dyn Crypto) {
match self {
Node::Branch(n) => {
for child in &n.choices {
child.memoize_hashes(buf);
child.memoize_hashes(buf, crypto);
}
}
Node::Extension(n) => n.child.memoize_hashes(buf),
Node::Extension(n) => n.child.memoize_hashes(buf, crypto),
_ => {}
}
}
Expand Down
Loading
Loading