refactor: remove hooks (#69)

The hooks pattern seems to be outdated since we no longer doing post tx checking. The only reason to have hook is to reuse the rlp bytes, which we can do it in a trivial way -- returning it after handling the block.

This pr removes the ExecutionHook and changes the way we reusing the rlp bytes.

Author: lightsing

crates/bin/src/commands/run_file.rs

@@ -1,14 +1,17 @@
 use crate::utils;
 use anyhow::{anyhow, bail};
 use clap::Args;
-use sbv::primitives::types::LegacyStorageTrace;
 use sbv::{
-    core::{ChunkInfo, EvmExecutorBuilder, HardforkConfig},
-    primitives::{types::BlockTrace, zk_trie::db::kv::HashMapDb, Block, B256},
+    core::{BlockExecutionResult, ChunkInfo, EvmExecutorBuilder, HardforkConfig},
+    primitives::{
+        types::{BlockTrace, LegacyStorageTrace},
+        zk_trie::db::kv::HashMapDb,
+        Block, B256,
+    },
 };
 use serde::Deserialize;
 use std::panic::catch_unwind;
-use std::{cell::RefCell, path::PathBuf};
+use std::path::PathBuf;
 use tiny_keccak::{Hasher, Keccak};
 use tokio::task::JoinSet;
 
@@ -80,22 +83,21 @@ impl RunFileCommand {
         let (chunk_info, mut zktrie_db) = ChunkInfo::from_block_traces(&traces);
         let mut code_db = HashMapDb::default();
 
-        let tx_bytes_hasher = RefCell::new(Keccak::v256());
+        let mut tx_bytes_hasher = Keccak::v256();
 
         let mut executor = EvmExecutorBuilder::new(&mut code_db, &mut zktrie_db)
             .hardfork_config(fork_config)
             .chain_id(traces[0].chain_id())
-            .with_hooks(traces[0].root_before(), |hooks| {
-                hooks.add_tx_rlp_handler(|_, rlp| {
-                    tx_bytes_hasher.borrow_mut().update(rlp);
-                });
-            })?;
+            .build(traces[0].root_before())?;
         for trace in traces.iter() {
             executor.insert_codes(trace)?;
         }
 
         for trace in traces.iter() {
-            executor.handle_block(trace)?;
+            let BlockExecutionResult { tx_rlps, .. } = executor.handle_block(trace)?;
+            for tx_rlp in tx_rlps {
+                tx_bytes_hasher.update(&tx_rlp);
+            }
         }
 
         let post_state_root = executor.commit_changes()?;
@@ -105,7 +107,7 @@ impl RunFileCommand {
         drop(executor);
 
         let mut tx_bytes_hash = B256::ZERO;
-        tx_bytes_hasher.into_inner().finalize(&mut tx_bytes_hash.0);
+        tx_bytes_hasher.finalize(&mut tx_bytes_hash.0);
         let _public_input_hash = chunk_info.public_input_hash(&tx_bytes_hash);
 
         dev_info!("[chunk mode] public input hash: {:?}", _public_input_hash);

crates/core/src/chunk.rs

@@ -117,10 +117,9 @@ impl ChunkInfo {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::{EvmExecutorBuilder, HardforkConfig};
+    use crate::{BlockExecutionResult, EvmExecutorBuilder, HardforkConfig};
     use revm::primitives::b256;
     use sbv_primitives::types::BlockTrace;
-    use std::cell::RefCell;
 
     const TRACES_STR: [&str; 4] = [
         include_str!("../../../testdata/mainnet_blocks/8370400.json"),
@@ -145,31 +144,30 @@ mod tests {
         let (chunk_info, mut zktrie_db) = ChunkInfo::from_block_traces(&traces);
         let mut code_db = HashMapDb::default();
 
-        let tx_bytes_hasher = RefCell::new(Keccak::v256());
+        let mut tx_bytes_hasher = Keccak::v256();
 
         let mut executor = EvmExecutorBuilder::new(&mut code_db, &mut zktrie_db)
             .hardfork_config(fork_config)
             .chain_id(traces[0].chain_id())
-            .with_hooks(traces[0].root_before(), |hooks| {
-                hooks.add_tx_rlp_handler(|_, rlp| {
-                    tx_bytes_hasher.borrow_mut().update(rlp);
-                });
-            })
+            .build(traces[0].root_before())
             .unwrap();
         for trace in traces.iter() {
             executor.insert_codes(trace).unwrap();
         }
 
         for trace in traces.iter() {
-            executor.handle_block(trace).unwrap();
+            let BlockExecutionResult { tx_rlps, .. } = executor.handle_block(trace).unwrap();
+            for tx_rlp in tx_rlps {
+                tx_bytes_hasher.update(&tx_rlp);
+            }
         }
 
         let post_state_root = executor.commit_changes().unwrap();
         assert_eq!(post_state_root, chunk_info.post_state_root);
         drop(executor); // drop executor to release Rc<Keccek>
 
         let mut tx_bytes_hash = B256::ZERO;
-        tx_bytes_hasher.into_inner().finalize(&mut tx_bytes_hash.0);
+        tx_bytes_hasher.finalize(&mut tx_bytes_hash.0);
         let public_input_hash = chunk_info.public_input_hash(&tx_bytes_hash);
         assert_eq!(
             public_input_hash,

crates/core/src/executor/builder.rs

@@ -1,10 +1,8 @@
-use crate::error::DatabaseError;
-use crate::{executor::hooks::ExecuteHooks, EvmDatabase, EvmExecutor, HardforkConfig};
+use crate::{error::DatabaseError, EvmDatabase, EvmExecutor, HardforkConfig};
 use revm::db::CacheDB;
-use sbv_primitives::alloy_primitives::ChainId;
-use sbv_primitives::zk_trie::db::kv::KVDatabase;
-use sbv_primitives::zk_trie::db::NodeDb;
-use sbv_primitives::B256;
+use sbv_primitives::{
+    alloy_primitives::ChainId, zk_trie::db::kv::KVDatabase, zk_trie::db::NodeDb, B256,
+};
 use std::fmt::{self, Debug};
 
 /// Builder for EVM executor.
@@ -93,14 +91,7 @@ impl<'a, CodeDb: KVDatabase, ZkDb: KVDatabase + 'static>
     EvmExecutorBuilder<'a, HardforkConfig, ChainId, CodeDb, ZkDb>
 {
     /// Initialize an EVM executor from a block trace as the initial state.
-    pub fn with_hooks<'h, F: FnOnce(&mut ExecuteHooks<'h, CodeDb, ZkDb>)>(
-        self,
-        root: B256,
-        with_execute_hooks: F,
-    ) -> Result<EvmExecutor<'a, 'h, CodeDb, ZkDb>, DatabaseError> {
-        let mut execute_hooks = ExecuteHooks::new();
-        with_execute_hooks(&mut execute_hooks);
-
+    pub fn build(self, root: B256) -> Result<EvmExecutor<'a, CodeDb, ZkDb>, DatabaseError> {
         let db = cycle_track!(
             CacheDB::new(EvmDatabase::new_from_root(
                 root,
@@ -114,12 +105,6 @@ impl<'a, CodeDb: KVDatabase, ZkDb: KVDatabase + 'static>
             hardfork_config: self.hardfork_config,
             chain_id: self.chain_id,
             db,
-            hooks: execute_hooks,
         })
     }
-
-    /// Initialize an EVM executor from a block trace as the initial state.
-    pub fn build<'e>(self, root: B256) -> Result<EvmExecutor<'a, 'e, CodeDb, ZkDb>, DatabaseError> {
-        self.with_hooks(root, |_| {})
-    }
 }

crates/core/src/executor/hooks.rs

@@ -9,6 +9,7 @@ use revm::{
     primitives::{AccountInfo, Env, B256, KECCAK_EMPTY, POSEIDON_EMPTY},
 };
 use sbv_primitives::{
+    alloy_primitives::Bytes,
     zk_trie::{
         db::kv::KVDatabase,
         hash::{key_hasher::NoCacheHasher, poseidon::Poseidon},
@@ -22,18 +23,23 @@ use std::fmt::Debug;
 mod builder;
 pub use builder::EvmExecutorBuilder;
 
-/// Execute hooks
-pub mod hooks;
-
 /// EVM executor that handles the block.
-pub struct EvmExecutor<'db, 'h, CodeDb, ZkDb> {
+pub struct EvmExecutor<'db, CodeDb, ZkDb> {
     chain_id: ChainId,
     hardfork_config: HardforkConfig,
     db: CacheDB<EvmDatabase<'db, CodeDb, ZkDb>>,
-    hooks: hooks::ExecuteHooks<'h, CodeDb, ZkDb>,
 }
 
-impl<CodeDb: KVDatabase, ZkDb: KVDatabase + 'static> EvmExecutor<'_, '_, CodeDb, ZkDb> {
+/// Block execution result
+#[derive(Debug, Clone)]
+pub struct BlockExecutionResult {
+    /// Gas used in this block
+    pub gas_used: u64,
+    /// RLP bytes of transactions
+    pub tx_rlps: Vec<Bytes>,
+}
+
+impl<CodeDb: KVDatabase, ZkDb: KVDatabase + 'static> EvmExecutor<'_, CodeDb, ZkDb> {
     /// Get reference to the DB
     pub fn db(&self) -> &CacheDB<EvmDatabase<CodeDb, ZkDb>> {
         &self.db
@@ -45,7 +51,10 @@ impl<CodeDb: KVDatabase, ZkDb: KVDatabase + 'static> EvmExecutor<'_, '_, CodeDb,
     }
 
     /// Handle a block.
-    pub fn handle_block<T: Block>(&mut self, l2_trace: &T) -> Result<u64, VerificationError> {
+    pub fn handle_block<T: Block>(
+        &mut self,
+        l2_trace: &T,
+    ) -> Result<BlockExecutionResult, VerificationError> {
         #[allow(clippy::let_and_return)]
         let gas_used = measure_duration_millis!(
             handle_block_duration_milliseconds,
@@ -59,7 +68,10 @@ impl<CodeDb: KVDatabase, ZkDb: KVDatabase + 'static> EvmExecutor<'_, '_, CodeDb,
     }
 
     #[inline(always)]
-    fn handle_block_inner<T: Block>(&mut self, l2_trace: &T) -> Result<u64, VerificationError> {
+    fn handle_block_inner<T: Block>(
+        &mut self,
+        l2_trace: &T,
+    ) -> Result<BlockExecutionResult, VerificationError> {
         let spec_id = self.hardfork_config.get_spec_id(l2_trace.number());
         dev_trace!("use spec id {spec_id:?}",);
         self.hardfork_config
@@ -81,6 +93,7 @@ impl<CodeDb: KVDatabase, ZkDb: KVDatabase + 'static> EvmExecutor<'_, '_, CodeDb,
         };
 
         let mut gas_used = 0;
+        let mut tx_rlps = Vec::with_capacity(l2_trace.num_txs());
 
         for (idx, tx) in l2_trace.transactions().enumerate() {
             cycle_tracker_start!("handle tx");
@@ -129,7 +142,7 @@ impl<CodeDb: KVDatabase, ZkDb: KVDatabase + 'static> EvmExecutor<'_, '_, CodeDb,
             }
             env.tx.scroll.is_l1_msg = tx.is_l1_msg();
             let rlp_bytes = tx.rlp();
-            self.hooks.tx_rlp(self, &rlp_bytes);
+            tx_rlps.push(rlp_bytes.clone());
             env.tx.scroll.rlp_bytes = Some(rlp_bytes);
 
             dev_trace!("{env:#?}");
@@ -161,12 +174,11 @@ impl<CodeDb: KVDatabase, ZkDb: KVDatabase + 'static> EvmExecutor<'_, '_, CodeDb,
 
                 dev_trace!("{result:#?}");
             }
-            self.hooks.post_tx_execution(self, idx);
 
             dev_debug!("handle {idx}th tx done");
             cycle_tracker_end!("handle tx");
         }
-        Ok(gas_used)
+        Ok(BlockExecutionResult { gas_used, tx_rlps })
     }
 
     /// Commit pending changes in cache db to zktrie
@@ -345,7 +357,7 @@ impl<CodeDb: KVDatabase, ZkDb: KVDatabase + 'static> EvmExecutor<'_, '_, CodeDb,
     }
 }
 
-impl<CodeDb, ZkDb> Debug for EvmExecutor<'_, '_, CodeDb, ZkDb> {
+impl<CodeDb, ZkDb> Debug for EvmExecutor<'_, CodeDb, ZkDb> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.debug_struct("EvmExecutor").field("db", &self.db).finish()
     }

crates/core/src/executor/mod.rs

@@ -14,7 +14,7 @@ mod error;
 pub use error::{DatabaseError, VerificationError};
 
 mod executor;
-pub use executor::{hooks, EvmExecutor, EvmExecutorBuilder};
+pub use executor::{BlockExecutionResult, EvmExecutor, EvmExecutorBuilder};
 
 mod genesis;
 pub use genesis::GenesisConfig;

crates/core/src/lib.rs

@@ -69,6 +69,9 @@ pub trait Block: Debug {
     /// transactions
     fn transactions(&self) -> impl Iterator<Item = &Self::Tx>;
 
+    /// Number of l1 transactions
+    fn num_txs(&self) -> usize;
+
     /// root before
     fn root_before(&self) -> B256;
     /// root after
@@ -314,6 +317,10 @@ impl<T: Block> Block for &T {
         (*self).transactions()
     }
 
+    fn num_txs(&self) -> usize {
+        (*self).num_txs()
+    }
+
     fn root_before(&self) -> B256 {
         (*self).root_before()
     }

crates/primitives/src/lib.rs

@@ -380,6 +380,10 @@ where
         self.transactions.iter()
     }
 
+    fn num_txs(&self) -> usize {
+        self.transactions.len()
+    }
+
     fn root_before(&self) -> B256 {
         self.storage_trace.root_before()
     }
@@ -459,6 +463,10 @@ where
         self.transactions.iter()
     }
 
+    fn num_txs(&self) -> usize {
+        self.transactions.len()
+    }
+
     fn root_before(&self) -> B256 {
         self.storage_trace.root_before()
     }
@@ -672,6 +680,10 @@ impl Block for alloy::rpc::types::Block<AlloyTransaction, alloy::rpc::types::Hea
         self.transactions.txns()
     }
 
+    fn num_txs(&self) -> usize {
+        self.transactions.len()
+    }
+
     fn root_before(&self) -> B256 {
         unimplemented!()
     }