diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cbc3555503..e4b84cadfde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ ## Perf +### 2026-08-04 + +- Cut the cost of a cold contract-code access: store jump destinations as a 1-bit-per-byte bitmap instead of a persisted RLP list of `u32` offsets, count the bytecode in the code cache's byte budget, answer `EXTCODESIZE` from the code-length table instead of materializing the bytecode, and give the account-code column families a bloom filter (4KB data blocks on the blob-backed bytecode CF). Raises the code cache's byte budget from an effective 64 MiB of jump tables to 256 MiB of bytecode, and bumps the store schema version so an older binary warns rather than failing on the new value format. `COLD_ACCOUNT_CODE_ACCESS` drops from 7736 to 4652 gas in the EIP-8038 repricing fit, and `COLD_ACCOUNT_CODE_WRITE` from 10415 to 6355 [#7095](https://github.com/lambdaclass/ethrex/pull/7095) + ### 2026-07-22 - Unify full-sync batch import onto the per-block execution pipeline, validating every block's state root and reusing the pipeline's BAL-driven parallel execution instead of the bespoke "execute all, apply once" batch path [#7008](https://github.com/lambdaclass/ethrex/pull/7008) diff --git a/crates/blockchain/blockchain.rs b/crates/blockchain/blockchain.rs index 693b63f76ec..775c044291b 100644 --- a/crates/blockchain/blockchain.rs +++ b/crates/blockchain/blockchain.rs @@ -1885,15 +1885,23 @@ impl Blockchain { } } - // Store all the accessed evm bytecodes - for code_hash in logger - .code_accessed - .lock() - .map_err(|_e| { + // Store all the accessed evm bytecodes. `code_accessed` records one entry + // per read, and a contract read for both its bytecode and its length is + // recorded twice, so dedup before embedding: a repeated hash would put the + // same (up to 24KB) bytecode in the witness more than once. + let accessed_codes: Vec = { + let accessed = logger.code_accessed.lock().map_err(|_e| { ChainError::WitnessGeneration("Failed to gather used bytecodes".to_string()) - })? - .iter() - { + })?; + let mut seen = + FxHashSet::with_capacity_and_hasher(accessed.len(), Default::default()); + accessed + .iter() + .copied() + .filter(|h| seen.insert(*h)) + .collect() + }; + for code_hash in &accessed_codes { let code = self .storage .get_account_code(*code_hash) @@ -2150,15 +2158,22 @@ impl Blockchain { } } - // Store all the accessed evm bytecodes - for code_hash in logger - .code_accessed - .lock() - .map_err(|_e| { + // Store all the accessed evm bytecodes. `code_accessed` records one entry + // per read, and a contract read for both its bytecode and its length is + // recorded twice, so dedup before embedding: a repeated hash would put the + // same (up to 24KB) bytecode in the witness more than once. + let accessed_codes: Vec = { + let accessed = logger.code_accessed.lock().map_err(|_e| { ChainError::WitnessGeneration("Failed to gather used bytecodes".to_string()) - })? - .iter() - { + })?; + let mut seen = FxHashSet::with_capacity_and_hasher(accessed.len(), Default::default()); + accessed + .iter() + .copied() + .filter(|h| seen.insert(*h)) + .collect() + }; + for code_hash in &accessed_codes { let code = self .storage .get_account_code(*code_hash) diff --git a/crates/common/types/account.rs b/crates/common/types/account.rs index a01aca48004..3b287c2a750 100644 --- a/crates/common/types/account.rs +++ b/crates/common/types/account.rs @@ -18,11 +18,11 @@ use ethrex_rlp::{ use super::GenesisAccount; use crate::constants::{EMPTY_KECCAK_HASH, EMPTY_TRIE_HASH}; -/// Shared empty jump-target table. `Code::default()` and any bytecode without a +/// Shared empty jumpdest bitmap. `Code::default()` and any bytecode without a /// `JUMPDEST` clone this (a refcount bump) instead of allocating a fresh empty /// `Arc` header each time. This matters because the per-tx `Code::default()` /// placeholder and every EOA / empty-code load would otherwise each allocate. -static EMPTY_JUMP_TARGETS: LazyLock> = LazyLock::new(|| Arc::from(Vec::new())); +static EMPTY_JUMPDESTS: LazyLock> = LazyLock::new(|| Arc::from(Vec::new())); /// Trailing STOP bytes appended to every bytecode so the dispatch loop can read /// the next opcode without a bounds check. 33 is the widest single-opcode advance @@ -42,13 +42,19 @@ pub struct Code { bytecode: Bytes, /// The real bytecode length, needed for some opcodes, `bytecode` is padded with 33 STOPs to avoid checked adds on hot loop. bytecode_len: usize, - // `Arc<[u32]>` so cloning `Code` (hot: every message-call resolves and clones - // the callee's code) is a refcount bump instead of deep-copying the table. + /// One bit per bytecode byte, set when that offset holds a `JUMPDEST` that is not + /// part of a `PUSH` immediate. Costs `ceil(len / 8)` bytes regardless of how dense + /// the jump destinations are, and validating a jump is a bit test rather than a + /// search. + /// + /// Bytecode with no jump destination stores a zero-length bitmap ([`EMPTY_JUMPDESTS`]) + /// rather than an all-zero one, so this is not always `ceil(len / 8)` bytes long; + /// [`Code::is_valid_jumpdest`] reads a missing byte as "no jump destination". + // + // `Arc<[u8]>` so cloning `Code` (hot: every message-call resolves and clones + // the callee's code) is a refcount bump instead of deep-copying the bitmap. // Serializes via serde's `rc` feature (enabled workspace-wide). - // The valid addresses are 32-bit because, despite EIP-3860 restricting initcode size, - // this does not apply to previous forks. This is tested in the EEST tests, which would - // panic in debug mode. - pub jump_targets: Arc<[u32]>, + jumpdests: Arc<[u8]>, } impl Code { @@ -59,26 +65,26 @@ impl Code { // `code` is the logical, unpadded bytecode; `BYTECODE_PADDING` STOP bytes are // appended internally by `from_parts_unchecked`. pub fn from_bytecode_unchecked(code: Bytes, hash: H256) -> Self { - let jump_targets = Self::compute_jump_targets(&code); - Self::from_parts_unchecked(hash, &code, jump_targets) + let jumpdests = Self::compute_jumpdests(&code); + Self::from_parts_unchecked(hash, &code, jumpdests) } /// `code` is the logical, unpadded bytecode; `BYTECODE_PADDING` STOP bytes are /// appended internally by `from_parts_unchecked`. pub fn from_bytecode(code: Bytes, crypto: &dyn Crypto) -> Self { - let jump_targets = Self::compute_jump_targets(&code); + let jumpdests = Self::compute_jumpdests(&code); let hash = H256(crypto.keccak256(code.as_ref())); - Self::from_parts_unchecked(hash, &code, jump_targets) + Self::from_parts_unchecked(hash, &code, jumpdests) } /// Builds a `Code` from precomputed parts. The caller must guarantee `hash` - /// and `jump_targets` correspond to `code`; neither is recomputed or validated. + /// and `jumpdests` correspond to `code`; neither is recomputed or validated. /// /// `code` is the logical, unpadded bytecode: this function appends /// `BYTECODE_PADDING` STOP bytes and records the original length in /// `bytecode_len`. Never pass a pre-padded buffer, or the logical length and /// every `JUMPDEST`/`PUSH` offset derived from it would be wrong. - pub fn from_parts_unchecked(hash: H256, code: &[u8], jump_targets: Arc<[u32]>) -> Self { + pub fn from_parts_unchecked(hash: H256, code: &[u8], jumpdests: Arc<[u8]>) -> Self { let bytecode_len = code.len(); let mut padded_code = Vec::with_capacity(bytecode_len + BYTECODE_PADDING); padded_code.extend_from_slice(code); @@ -87,20 +93,38 @@ impl Code { hash, bytecode: Bytes::from_owner(padded_code), bytecode_len, - jump_targets, + jumpdests, } } - fn compute_jump_targets(code: &[u8]) -> Arc<[u32]> { - debug_assert!(code.len() <= u32::MAX as usize); - let mut targets = Vec::new(); + /// Builds the [`Code::jumpdests`] bitmap: one pass over the bytecode, setting the + /// bit for every `JUMPDEST` while skipping `PUSH` immediates. + /// + /// The bits of a byte are accumulated in a register and written once the scan leaves + /// that byte, which the monotonic `i` makes safe. Reading the bitmap back inside the + /// loop instead would turn each `JUMPDEST` into a read-modify-write, and indexing it + /// would put a bounds-check panic path in the loop body, which inhibits optimization + /// of every iteration rather than only the ones that find a destination. + pub fn compute_jumpdests(code: &[u8]) -> Arc<[u8]> { + let mut bitmap = vec![0u8; code.len().div_ceil(8)]; + let mut any = false; + let mut current_byte = usize::MAX; + let mut bits = 0u8; let mut i = 0; while i < code.len() { // TODO: we don't use the constants from the vm module to avoid a circular dependency match code[i] { // OP_JUMPDEST 0x5B => { - targets.push(i as u32); + if i / 8 != current_byte { + if let Some(byte) = bitmap.get_mut(current_byte) { + *byte = bits; + } + current_byte = i / 8; + bits = 0; + } + bits |= 1 << (i % 8); + any = true; } // OP_PUSH1..32 c @ 0x60..0x80 => { @@ -111,15 +135,34 @@ impl Code { } i += 1; } - // Share the single empty table for jumpless bytecode (very common: EOAs, - // tiny contracts) so we don't allocate an `Arc` header for an empty slice. - if targets.is_empty() { - EMPTY_JUMP_TARGETS.clone() + if let Some(byte) = bitmap.get_mut(current_byte) { + *byte = bits; + } + // Share the single empty bitmap for jumpless bytecode (very common: EOAs, + // tiny contracts) so we don't allocate for an all-zero map; `is_valid_jumpdest` + // reads a missing byte as "no jump destination". + if any { + Arc::from(bitmap) } else { - Arc::from(targets) + EMPTY_JUMPDESTS.clone() } } + /// Whether `offset` is a valid jump destination, i.e. it holds a `JUMPDEST` that is + /// not part of a `PUSH` immediate. Offsets past the bytecode are not valid. + #[inline] + pub fn is_valid_jumpdest(&self, offset: usize) -> bool { + self.jumpdests + .get(offset / 8) + .is_some_and(|byte| byte & (1 << (offset % 8)) != 0) + } + + /// The raw [`Code::jumpdests`] bitmap, for persisting it alongside the bytecode. + #[inline] + pub fn jumpdests(&self) -> &[u8] { + &self.jumpdests + } + #[inline] pub fn code(&self) -> &[u8] { self.bytecode.get(..self.bytecode_len).unwrap_or_default() @@ -151,16 +194,18 @@ impl Code { /// Estimates the size of the Code struct in bytes /// (including stack size and heap allocation). /// - /// Note: This is an estimation and may not be exact. + /// Note: an estimate. It ignores allocator overhead and the `Arc`/`Bytes` control + /// blocks, so it slightly under-counts, and a shared allocation is attributed in + /// full to every entry holding it. /// /// # Returns /// /// usize - Estimated size in bytes pub fn size(&self) -> usize { let hash_size = size_of::(); - let bytes_size = size_of::(); - let vec_size = size_of::>() + self.jump_targets.len() * size_of::(); - hash_size + bytes_size + vec_size + let bytes_size = size_of::() + self.bytecode.len(); + let bitmap_size = size_of::>() + self.jumpdests.len(); + hash_size + bytes_size + bitmap_size } } @@ -170,11 +215,14 @@ impl Code { /// `Code` is padded with [`BYTECODE_PADDING`] trailing STOPs) sound regardless of /// where the bytes came from. Deserializing the padded buffer directly would /// otherwise let unpadded input through and cause OOB reads during execution. +/// The jump destinations are deliberately absent: they are a pure function of the +/// bytecode, so carrying them would put a derived value in the wire format of everything +/// that embeds a `Code` (notably `AccountUpdate`, which the L2 rollup store persists with +/// bincode) and couple that format to how they happen to be represented. #[derive(Serialize, Deserialize)] struct CodeSerde { hash: H256, code: Bytes, - jump_targets: Arc<[u32]>, } impl Serialize for Code { @@ -182,7 +230,6 @@ impl Serialize for Code { CodeSerde { hash: self.hash, code: self.code_bytes(), - jump_targets: self.jump_targets.clone(), } .serialize(serializer) } @@ -190,12 +237,12 @@ impl Serialize for Code { impl<'de> Deserialize<'de> for Code { fn deserialize>(deserializer: D) -> Result { - let CodeSerde { + let CodeSerde { hash, code } = CodeSerde::deserialize(deserializer)?; + Ok(Self::from_parts_unchecked( hash, - code, - jump_targets, - } = CodeSerde::deserialize(deserializer)?; - Ok(Self::from_parts_unchecked(hash, &code, jump_targets)) + &code, + Self::compute_jumpdests(&code), + )) } } @@ -263,7 +310,7 @@ impl Default for Code { bytecode: Bytes::from_static(&[0u8; BYTECODE_PADDING]), bytecode_len: 0, hash: *EMPTY_KECCAK_HASH, - jump_targets: EMPTY_JUMP_TARGETS.clone(), + jumpdests: EMPTY_JUMPDESTS.clone(), } } } diff --git a/crates/storage/backend/rocksdb.rs b/crates/storage/backend/rocksdb.rs index 81eeac3249e..8dc69201200 100644 --- a/crates/storage/backend/rocksdb.rs +++ b/crates/storage/backend/rocksdb.rs @@ -1,6 +1,6 @@ use crate::api::tables::{ - ACCOUNT_CODES, ACCOUNT_FLATKEYVALUE, ACCOUNT_TRIE_NODES, BLOCK_NUMBERS, BODIES, - CANONICAL_BLOCK_HASHES, FULLSYNC_HEADERS, HEADERS, RECEIPTS_V2, STORAGE_FLATKEYVALUE, + ACCOUNT_CODE_METADATA, ACCOUNT_CODES, ACCOUNT_FLATKEYVALUE, ACCOUNT_TRIE_NODES, BLOCK_NUMBERS, + BODIES, CANONICAL_BLOCK_HASHES, FULLSYNC_HEADERS, HEADERS, RECEIPTS_V2, STORAGE_FLATKEYVALUE, STORAGE_TRIE_NODES, TRANSACTION_LOCATIONS, }; use crate::api::{ @@ -208,18 +208,40 @@ impl RocksDBBackend { configure_block_cache(&mut block_opts); cf_opts.set_block_based_table_factory(&block_opts); } - ACCOUNT_CODES => { + ACCOUNT_CODES | ACCOUNT_CODE_METADATA => { cf_opts.set_write_buffer_size(128 * 1024 * 1024); // 128MB cf_opts.set_max_write_buffer_number(3); cf_opts.set_target_file_size_base(256 * 1024 * 1024); // 256MB - cf_opts.set_enable_blob_files(true); - // Small bytecodes should go inline (mainly for delegation indicators) - cf_opts.set_min_blob_size(32); - cf_opts.set_blob_compression_type(rocksdb::DBCompressionType::Lz4); + if cf_name == ACCOUNT_CODES { + cf_opts.set_enable_blob_files(true); + // Small bytecodes should go inline (mainly for delegation indicators) + cf_opts.set_min_blob_size(32); + cf_opts.set_blob_compression_type(rocksdb::DBCompressionType::Lz4); + } let mut block_opts = BlockBasedOptions::default(); - block_opts.set_block_size(32 * 1024); // 32KB + // Both CFs answer exact-key point lookups on the execution read path: + // EXT*/CALL* resolve a code hash to its bytecode or its length. The + // filter is what pays here, pruning the levels that cannot hold the + // hash instead of reading a data block per level to find out. + block_opts.set_bloom_filter(10.0, false); // 10 bits per key + if cf_name == ACCOUNT_CODES { + // With blob files the SST value is only a blob reference, so a + // large block buys nothing; page-sized keeps per-get read + // amplification down. + block_opts.set_block_size(4 * 1024); // 4KB + // Lookups here are almost always positive, since the hash comes + // from an account that references it. Dropping the last level's + // filter is most of the filter memory for no hit-rate loss. + cf_opts.set_optimize_filters_for_hits(true); + } else { + // Metadata rows are 32-byte key + 8-byte length with no blob + // indirection, so ~100 of them share a 4KB block and shrinking + // the block only multiplies index entries. Keep the 16KB this CF + // used before it had an arm of its own. + block_opts.set_block_size(16 * 1024); // 16KB + } configure_block_cache(&mut block_opts); cf_opts.set_block_based_table_factory(&block_opts); } diff --git a/crates/storage/lib.rs b/crates/storage/lib.rs index b6a23e2985a..7f00b2901a9 100644 --- a/crates/storage/lib.rs +++ b/crates/storage/lib.rs @@ -88,7 +88,7 @@ pub use store::{ /// When bumping this version, add a corresponding migration function to /// `migrations::MIGRATIONS`. The migration framework will automatically /// upgrade existing databases instead of requiring a full resync. -pub const STORE_SCHEMA_VERSION: u64 = 3; +pub const STORE_SCHEMA_VERSION: u64 = 4; /// Name of the file storing the metadata about the database. /// diff --git a/crates/storage/migrations.rs b/crates/storage/migrations.rs index f2946bd6a0f..8f27663bae5 100644 --- a/crates/storage/migrations.rs +++ b/crates/storage/migrations.rs @@ -30,7 +30,7 @@ pub type MigrationFn = fn(backend: &dyn StorageBackend) -> Result<(), StoreError /// /// **Invariant**: `MIGRATIONS.len() == (STORE_SCHEMA_VERSION - 1) as usize` /// (empty when `STORE_SCHEMA_VERSION == 1`, one entry when it's 2, etc.) -pub const MIGRATIONS: &[MigrationFn] = &[migrate_1_to_2, migrate_2_to_3]; +pub const MIGRATIONS: &[MigrationFn] = &[migrate_1_to_2, migrate_2_to_3, migrate_3_to_4]; // Compile-time check: the number of migration functions must match the number // of version gaps (i.e. STORE_SCHEMA_VERSION - 1). @@ -44,6 +44,19 @@ fn migration_for_version(version: u64) -> MigrationFn { MIGRATIONS[(version - 1) as usize] } +/// v3 → v4: no data change. +/// +/// `ACCOUNT_CODES` values carry their JUMPDEST positions as a bitmap rather than an RLP +/// list of `u32` offsets. Both forms are readable, so a v3 database needs no rewriting +/// and this migration only moves the version marker. +/// +/// The bump exists for the other direction: a v3 binary cannot decode a bitmap, and +/// `run_pending_migrations` warns that the database is ahead of the binary instead of +/// letting it fail on the first code read. +fn migrate_3_to_4(_backend: &dyn StorageBackend) -> Result<(), StoreError> { + Ok(()) +} + /// Minimum interval between migration progress log lines. const PROGRESS_LOG_INTERVAL: Duration = Duration::from_secs(10); diff --git a/crates/storage/store.rs b/crates/storage/store.rs index 6a736852d58..6b970cb1f46 100644 --- a/crates/storage/store.rs +++ b/crates/storage/store.rs @@ -36,7 +36,7 @@ use ethrex_common::{ }; use ethrex_crypto::{NativeCrypto, keccak::keccak_hash}; use ethrex_rlp::{ - decode::{RLPDecode, decode_bytes}, + decode::{RLPDecode, decode_bytes, decode_rlp_item}, encode::RLPEncode, }; use ethrex_trie::{EMPTY_TRIE_HASH, Nibbles, Trie, TrieLogger, TrieNode, TrieWitness}; @@ -49,6 +49,7 @@ use std::{ collections::{BTreeMap, HashMap, HashSet, hash_map::Entry}, fmt::Debug, io::Write, + num::NonZeroUsize, path::{Path, PathBuf}, sync::{ Arc, Condvar, Mutex, RwLock, @@ -134,8 +135,20 @@ enum FKVGeneratorControlMessage { Continue, } -// 64mb -const CODE_CACHE_MAX_SIZE: u64 = 64 * 1024 * 1024; +/// Byte budget for the in-memory bytecode cache. +/// +/// Bytecode is a small fraction of the working set next to trie nodes and flat +/// key-values (which the RocksDB block cache serves), but a cold code read costs a +/// blob-file fetch, so caching contracts pays for itself: 256 MiB holds ~10k max-size +/// (24 KiB) contracts, or ~100k of typical size. +const CODE_CACHE_MAX_SIZE: u64 = 256 * 1024 * 1024; + +/// Entry bound for [`Store::code_metadata_cache`], derived from a 16 MiB ceiling at the +/// ~64 B an `LruCache` entry costs (32 B key + 8 B value + list/table overhead). Sized +/// against the code cache it shadows: at 256 MiB that holds ~10k max-size or ~100k +/// typical contracts, so this keeps a length for every code that could be resident and +/// then some, while bounding what an `EXTCODESIZE` sweep over unique contracts can pin. +const CODE_METADATA_CACHE_MAX_ENTRIES: usize = (16 * 1024 * 1024) / 64; /// Key used to persist the `flushed_upto` block number in `MISC_VALUES`. const FLUSHED_UPTO_KEY: &[u8] = b"bodies_flushed_upto"; @@ -150,6 +163,7 @@ const MAX_BAD_BLOCKS: usize = 16; struct CodeCache { inner_cache: LruCache, cache_size: u64, + max_size: u64, } impl Default for CodeCache { @@ -157,6 +171,7 @@ impl Default for CodeCache { Self { inner_cache: LruCache::unbounded_with_hasher(FxBuildHasher), cache_size: 0, + max_size: CODE_CACHE_MAX_SIZE, } } } @@ -167,15 +182,17 @@ impl CodeCache { } fn insert(&mut self, code: &Code) -> Result<(), StoreError> { - let code_size = code.size(); - let cache_len = self.inner_cache.len() + 1; - self.cache_size += code_size as u64; - let current_size = self.cache_size; - debug!( - "[ACCOUNT CODE CACHE] cache elements (): {cache_len}, total size: {current_size} bytes" - ); + // A hash already cached must not be added to `cache_size` again, or the counter + // drifts up permanently and evicts entries that fit. `get` also refreshes + // recency, which is what a repeated read should do. + if self.inner_cache.get(&code.hash).is_some() { + return Ok(()); + } + + self.cache_size += code.size() as u64; + self.inner_cache.put(code.hash, code.clone()); - while self.cache_size > CODE_CACHE_MAX_SIZE { + while self.cache_size > self.max_size { if let Some((_, code)) = self.inner_cache.pop_lru() { self.cache_size -= code.size() as u64; } else { @@ -183,7 +200,11 @@ impl CodeCache { } } - self.inner_cache.get_or_insert(code.hash, || code.clone()); + let cache_len = self.inner_cache.len(); + let current_size = self.cache_size; + debug!( + "[ACCOUNT CODE CACHE] cache elements (): {cache_len}, total size: {current_size} bytes" + ); Ok(()) } } @@ -228,8 +249,11 @@ pub struct Store { account_code_cache: Arc>, /// Cache for code metadata (code length), keyed by the bytecode hash. - /// Uses FxHashMap for efficient lookups, much smaller than code cache. - code_metadata_cache: Arc>>, + /// + /// Bounded: `EXTCODESIZE` reads this on the execution path, so an unbounded map + /// would grow by one entry per distinct contract ever asked for a length and never + /// give the memory back. See [`CODE_METADATA_CACHE_MAX_ENTRIES`]. + code_metadata_cache: Arc>>, /// Serializes concurrent `forkchoice_update` callers so that the cache /// update and the DB write transaction remain mutually ordered. @@ -926,11 +950,11 @@ impl Store { else { return Ok(None); }; - let (bytecode_slice, targets) = decode_bytes(&bytes)?; + let (bytecode_slice, jumpdests) = decode_bytes(&bytes)?; let code = Code::from_parts_unchecked( code_hash, bytecode_slice, - >::decode(targets)?.into(), + decode_jumpdests(bytecode_slice, jumpdests)?, ); // insert into cache and evict if needed @@ -944,7 +968,7 @@ impl Store { /// Check if account code exists by its hash, without constructing the full `Code` struct. /// More efficient than `get_account_code` for existence checks since it skips - /// RLP decoding and `Code` struct construction (no `jump_targets` deserialization). + /// RLP decoding and `Code` struct construction (no jumpdest-bitmap decoding). /// Note: The underlying `get()` still reads the value from RocksDB (including blob files). pub fn code_exists(&self, code_hash: H256) -> Result { // Code introduced by a not-yet-flushed block lives only in the buffer; check @@ -1014,21 +1038,29 @@ impl Store { length: code.len() as u64, }; - // Write metadata for future use (async, fire and forget) - let metadata_buf = metadata.length.to_be_bytes().to_vec(); - let hash_key = code_hash.0.to_vec(); - let backend = self.backend.clone(); - tokio::task::spawn(async move { - if let Err(e) = async { - let mut tx = backend.begin_write()?; - tx.put(ACCOUNT_CODE_METADATA, &hash_key, &metadata_buf)?; - tx.commit() - } - .await - { - tracing::warn!("Failed to write code metadata during auto-migration: {}", e); - } - }); + // Backfill the row for future reads, fire and forget. + // + // Only when a Tokio runtime is reachable: this read is on the execution + // path (`EXTCODESIZE`), which runs on rayon workers, and + // `tokio::task::spawn` panics outside a runtime. Skipping the backfill + // costs one code read the next time that hash is asked for; panicking + // would take the node down. + if let Ok(handle) = tokio::runtime::Handle::try_current() { + let metadata_buf = metadata.length.to_be_bytes().to_vec(); + let hash_key = code_hash.0.to_vec(); + let backend = self.backend.clone(); + handle.spawn(async move { + if let Err(e) = async { + let mut tx = backend.begin_write()?; + tx.put(ACCOUNT_CODE_METADATA, &hash_key, &metadata_buf)?; + tx.commit() + } + .await + { + tracing::warn!("Failed to write code metadata during backfill: {}", e); + } + }); + } metadata }; @@ -1037,7 +1069,7 @@ impl Store { self.code_metadata_cache .lock() .map_err(|_| StoreError::LockError)? - .insert(code_hash, metadata); + .put(code_hash, metadata); Ok(Some(metadata)) } @@ -1981,7 +2013,10 @@ impl Store { pending_trie_roots: Arc::new(PendingTrieRoots::default()), last_computed_flatkeyvalue: Arc::new(RwLock::new(last_written)), account_code_cache: Arc::new(Mutex::new(CodeCache::default())), - code_metadata_cache: Arc::new(Mutex::new(rustc_hash::FxHashMap::default())), + code_metadata_cache: Arc::new(Mutex::new(LruCache::with_hasher( + NonZeroUsize::new(CODE_METADATA_CACHE_MAX_ENTRIES).unwrap_or(NonZeroUsize::MIN), + FxBuildHasher, + ))), fcu_lock: Arc::new(tokio::sync::Mutex::new(())), safe_commit_root, journal_pruning_paused: Arc::new(std::sync::atomic::AtomicBool::new(false)), @@ -5229,15 +5264,27 @@ pub fn receipt_key(block_hash: &BlockHash, index: u64) -> Vec { } fn encode_code(code: &Code) -> Vec { - let mut buf = - Vec::with_capacity(6 + code.len() + std::mem::size_of_val::<[u32]>(&code.jump_targets)); + let jumpdests = code.jumpdests(); + let mut buf = Vec::with_capacity(6 + code.len() + jumpdests.len()); code.code().encode(&mut buf); - // `Arc<[u32]>` (the in-memory share) has no `RLPEncode` impl; encode through an - // owned `Vec` on this cold DB-write path (code is persisted once per hash). - code.jump_targets.to_vec().encode(&mut buf); + jumpdests.encode(&mut buf); buf } +/// Decodes the JUMPDEST bitmap stored after the bytecode in an [`ACCOUNT_CODES`] value. +/// +/// Values written before the bitmap representation hold an RLP *list* of `u32` offsets +/// there instead. The RLP item header distinguishes a list from a byte string, so both +/// forms are readable: for the older one the bitmap is rebuilt from the bytecode, which +/// is cheaper than decoding the list. +fn decode_jumpdests(code: &[u8], encoded: &[u8]) -> Result, StoreError> { + let (is_list, payload, _) = decode_rlp_item(encoded)?; + if is_list { + return Ok(Code::compute_jumpdests(code)); + } + Ok(Arc::from(payload)) +} + #[derive(Debug, Default, Clone)] struct LatestBlockHeaderCache { current: Arc>>, @@ -5411,6 +5458,186 @@ pub fn read_chain_id_from_db(path: &Path) -> Option { } } +#[cfg(test)] +mod account_code_tests { + use super::*; + + const JUMPDEST: u8 = 0x5b; + const PUSH1: u8 = 0x60; + + /// `EXTCODESIZE` resolves a length through [`Store::get_code_metadata`], and + /// execution runs on rayon workers, which are not inside a Tokio runtime. A hash + /// with no metadata row SHALL still answer from the bytecode there rather than + /// panicking on the backfill spawn. This test is deliberately NOT `#[tokio::test]`: + /// the absence of a runtime is the condition under test. + #[test] + fn metadata_falls_back_to_the_bytecode_without_a_tokio_runtime() { + use crate::backend::in_memory::InMemoryBackend; + + let backend: Arc = Arc::new(InMemoryBackend::open().unwrap()); + let dir = tempfile::tempdir().unwrap(); + let store = Store::from_backend( + backend.clone(), + dir.path().to_path_buf(), + 1, + DEFAULT_PERSIST_CHANNEL_CAPACITY, + ) + .unwrap(); + + // Code present, metadata row absent: the shape of any database written before + // ACCOUNT_CODE_METADATA existed, which no migration backfills. + let code = jumpdest_dense_code(); + let mut tx = backend.begin_write().unwrap(); + tx.put(ACCOUNT_CODES, code.hash.as_bytes(), &encode_code(&code)) + .unwrap(); + tx.commit().unwrap(); + + let metadata = store + .get_code_metadata(code.hash) + .expect("metadata read must not fail off-runtime"); + assert_eq!(metadata.map(|m| m.length), Some(code.len() as u64)); + } + + /// A max-size contract that is almost entirely JUMPDESTs, the shape that makes the + /// jump-destination representation matter: as a list of offsets it is ~4x the size + /// of the bytecode it describes. + fn jumpdest_dense_code() -> Code { + let mut bytecode = vec![JUMPDEST; 24576]; + bytecode[0] = 0x00; + Code::from_bytecode_unchecked(bytecode.into(), H256::zero()) + } + + /// Encodes an `ACCOUNT_CODES` value the way it was written before the bitmap: the + /// bytecode followed by an RLP list of `u32` JUMPDEST offsets. + fn encode_code_legacy(code: &Code) -> Vec { + let offsets: Vec = (0..code.len()) + .filter(|offset| code.is_valid_jumpdest(*offset)) + .map(|offset| offset as u32) + .collect(); + let mut buf = Vec::new(); + code.code().encode(&mut buf); + offsets.encode(&mut buf); + buf + } + + #[test] + fn encoded_value_carries_one_bitmap_bit_per_code_byte() { + let code = jumpdest_dense_code(); + let encoded = encode_code(&code); + + assert_eq!(encoded.len(), 6 + code.len() + code.len().div_ceil(8)); + } + + #[test] + fn round_trip_preserves_the_bitmap() { + let code = jumpdest_dense_code(); + let encoded = encode_code(&code); + + let (bytecode, jumpdests) = decode_bytes(&encoded).unwrap(); + assert_eq!(bytecode, code.code()); + assert_eq!( + decode_jumpdests(bytecode, jumpdests).unwrap().as_ref(), + code.jumpdests() + ); + } + + /// Values written before the bitmap SHALL still decode, with the bitmap rebuilt from + /// the bytecode, so an existing database needs no rewriting. + /// + /// The expectation is built from the offsets the legacy value itself names, not from + /// `Code::compute_jumpdests`, so this pins the rebuilt bitmap against the old format + /// rather than against the implementation that produces it. + #[test] + fn legacy_offset_list_values_decode_to_the_same_bitmap() { + for (bytecode, offsets) in [ + (vec![0x00, JUMPDEST, 0x00, JUMPDEST], vec![1u32, 3]), + // The byte after PUSH1 is its immediate, so only offset 2 is a destination. + (vec![PUSH1, JUMPDEST, JUMPDEST], vec![2u32]), + (vec![0x00; 32], vec![]), + ] { + let code = Code::from_bytecode_unchecked(bytecode.clone().into(), H256::zero()); + let legacy = encode_code_legacy(&code); + + let mut expected = vec![0u8; bytecode.len().div_ceil(8)]; + for offset in &offsets { + let index = usize::try_from(*offset).expect("offset fits usize"); + expected[index / 8] |= 1 << (index % 8); + } + // A jumpless contract stores no bitmap at all rather than an all-zero one. + if offsets.is_empty() { + expected.clear(); + } + + let (decoded_bytecode, jumpdests) = decode_bytes(&legacy).unwrap(); + assert_eq!(decoded_bytecode, code.code()); + assert_eq!( + decode_jumpdests(decoded_bytecode, jumpdests) + .unwrap() + .as_ref(), + expected.as_slice(), + "rebuilt bitmap disagrees with the legacy offsets for {bytecode:?}" + ); + } + } + + /// Re-inserting a cached hash SHALL NOT grow the accounted size, or the counter + /// drifts up until the cache evicts entries that fit. + #[test] + fn repeated_inserts_do_not_inflate_the_accounted_size() { + let code = jumpdest_dense_code(); + let mut cache = CodeCache::default(); + + cache.insert(&code).unwrap(); + let after_first = cache.cache_size; + for _ in 0..8 { + cache.insert(&code).unwrap(); + } + + assert_eq!(cache.cache_size, after_first); + assert_eq!(cache.inner_cache.len(), 1); + } + + /// The accounted size SHALL include the bytecode itself, so the cache honors its + /// memory budget instead of holding orders of magnitude more than it accounts for. + #[test] + fn accounted_size_covers_the_bytecode_and_bitmap() { + let code = jumpdest_dense_code(); + let mut cache = CodeCache::default(); + cache.insert(&code).unwrap(); + + assert!(cache.cache_size >= (code.len() + code.jumpdests().len()) as u64); + } + + #[test] + fn cache_evicts_down_to_its_budget() { + let mut cache = CodeCache { + max_size: 128 * 1024, + ..Default::default() + }; + + for i in 0..16u8 { + let mut bytecode = vec![JUMPDEST; 24576]; + bytecode[0] = i; + cache + .insert(&Code::from_bytecode_unchecked( + bytecode.into(), + H256::from_low_u64_be(i.into()), + )) + .unwrap(); + } + + assert!(cache.cache_size <= cache.max_size); + assert!(cache.inner_cache.len() < 16); + } + + /// The default budget SHALL be the one the cache actually enforces, so a change to + /// the constant cannot silently leave the cache unbounded. + #[test] + fn default_cache_uses_the_configured_budget() { + assert_eq!(CodeCache::default().max_size, CODE_CACHE_MAX_SIZE); + } +} + #[cfg(test)] mod state_history_tests { use super::*; diff --git a/crates/vm/backends/levm/db.rs b/crates/vm/backends/levm/db.rs index 9fb01b81343..387b341c32a 100644 --- a/crates/vm/backends/levm/db.rs +++ b/crates/vm/backends/levm/db.rs @@ -80,6 +80,15 @@ impl LevmDatabase for DatabaseLogger { } fn get_code_metadata(&self, code_hash: CoreH256) -> Result { + // A size-only read still observes the bytecode, so the witness must carry it: + // EIP-8025 stateless validation recomputes the length from the code itself, and + // that is also how `ExecutionWitness::get_code_metadata` answers. + if code_hash != *EMPTY_KECCAK_HASH { + self.code_accessed + .lock() + .map_err(|_| DatabaseError::Custom("Could not lock mutex".to_string()))? + .push(code_hash); + } self.store.get_code_metadata(code_hash) } } diff --git a/crates/vm/levm/src/db/gen_db.rs b/crates/vm/levm/src/db/gen_db.rs index dac556b0880..1bbc04cd7a3 100644 --- a/crates/vm/levm/src/db/gen_db.rs +++ b/crates/vm/levm/src/db/gen_db.rs @@ -492,29 +492,18 @@ impl GeneralizedDatabase { match self.code_metadata.entry(code_hash) { Entry::Occupied(entry) => Ok(entry.into_mut()), Entry::Vacant(entry) => { - // First ensure code is loaded into cache by calling get_code - // This handles witness fallbacks and other code loading logic correctly + // Answer from the size-only store lookup rather than the bytecode: a + // contract's code is up to 24KB and lives out-of-line, so materializing it + // to return a length is the dominant cost of EXTCODESIZE. Code already + // loaded for another reason answers for free. #[expect(clippy::as_conversions, reason = "same sized types (on 64bit)")] - let code_length = { - // Note: `self.get_code(code_hash)` has been inlined due to mutability borrow issues. - // To avoid this inlinement, self.get_code has to be moved into `self.codes` so that it's called - // like this: `self.codes.get(code_hash)`. - let code = match self.codes.entry(code_hash) { - Entry::Occupied(entry) => entry.into_mut(), - Entry::Vacant(entry) => { - entry.insert(self.store.get_account_code(code_hash)?) - } - }; - - code.len() as u64 - }; - - let metadata = CodeMetadata { - length: code_length, + let length = match self.codes.get(&code_hash) { + Some(code) => code.len() as u64, + None => self.store.get_code_metadata(code_hash)?.length, }; // Insert into cache and return reference - Ok(entry.insert(metadata)) + Ok(entry.insert(CodeMetadata { length })) } } } diff --git a/crates/vm/levm/src/db/mod.rs b/crates/vm/levm/src/db/mod.rs index be0ecc4b382..f4ad12a7929 100644 --- a/crates/vm/levm/src/db/mod.rs +++ b/crates/vm/levm/src/db/mod.rs @@ -305,9 +305,16 @@ impl Database for CachingDatabase { } fn get_code_metadata(&self, code_hash: H256) -> Result { - // Delegate directly to the underlying database. - // The underlying Store already has its own code_metadata_cache, - // so we don't need to duplicate caching here. + // Answer from resident code when there is any. The BAL warmer loads the block's + // bytecode into this cache, so a length read usually has the answer here under a + // shared read lock. Falling straight through would instead take the store's + // single mutex-guarded metadata cache, serializing every `EXTCODESIZE` across + // the parallel executor's threads. + if let Some(code) = self.read_code()?.get(&code_hash) { + return Ok(CodeMetadata { + length: u64::try_from(code.len()).unwrap_or(u64::MAX), + }); + } self.inner.get_code_metadata(code_hash) } diff --git a/crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs b/crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs index 0e05cdb706a..4986a26f26d 100644 --- a/crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs +++ b/crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs @@ -467,25 +467,10 @@ impl OpcodeHandler for OpJumpIHandler { /// JUMPDEST charge), and the JUMPDEST step is pushed directly via /// `synthesize_step` after the gas is charged. fn jump(vm: &mut VM<'_>, target: usize, parent_gas_cost: u64) -> Result<(), VMError> { - // Check target address validity. - // - Target bytecode has to be a JUMPDEST. - // - Target address must not be blacklisted (aka. the JUMPDEST must not be part of a literal). - #[expect(clippy::as_conversions, reason = "safe")] - if vm - .current_call_frame - .bytecode - .dispatch_buf() - .get(target) - .is_some_and(|&value| { - value == Opcode::JUMPDEST as u8 - && vm - .current_call_frame - .bytecode - .jump_targets - .binary_search(&(target as u32)) - .is_ok() - }) - { + // Check target address validity: the target has to be a JUMPDEST that is not part + // of a literal (aka. a PUSH immediate). Both are answered by the bitmap, which only + // has bits set for JUMPDESTs reached by the opcode walk. + if vm.current_call_frame.bytecode.is_valid_jumpdest(target) { if vm.opcode_tracer.active { // Override the parent JUMP/JUMPI's gasCost so the dispatch loop // doesn't roll the upcoming JUMPDEST charge into it. diff --git a/test/tests/common/code_serde_tests.rs b/test/tests/common/code_serde_tests.rs index 1bedb27b413..5579460f95f 100644 --- a/test/tests/common/code_serde_tests.rs +++ b/test/tests/common/code_serde_tests.rs @@ -19,7 +19,7 @@ fn code_serde_roundtrip_preserves_logical_code_and_repads() { assert_eq!(restored.code(), code.code()); assert_eq!(restored.len(), code.len()); assert_eq!(restored.hash, code.hash); - assert_eq!(restored.jump_targets, code.jump_targets); + assert_eq!(restored.jumpdests(), code.jumpdests()); // The dispatch buffer must carry the trailing padding after a round-trip. assert_eq!(restored.dispatch_buf().len(), code.len() + BYTECODE_PADDING); assert_eq!(restored, code); @@ -42,3 +42,36 @@ fn default_code_is_padded() { assert!(code.is_empty()); assert_eq!(code.dispatch_buf().len(), BYTECODE_PADDING); } + +/// The wire format SHALL carry only the hash and the bytecode. Jump destinations are a +/// pure function of the bytecode, and every type embedding a `Code` inherits this format +/// (`AccountUpdate`, which the L2 rollup store persists with bincode), so a derived field +/// here would couple that stored format to how jump destinations are represented. +#[test] +fn code_serde_does_not_persist_derived_jumpdests() { + let bytecode = vec![0x5b /* JUMPDEST */; 512]; + let code = Code::from_bytecode(bytecode.into(), &NativeCrypto); + assert!( + !code.jumpdests().is_empty(), + "fixture must have a non-empty bitmap for this to prove anything" + ); + + let json = serde_json::to_value(&code).expect("serialize"); + // `serde_json` orders its map keys, so compare against a sorted expectation. + let fields: Vec<&str> = json + .as_object() + .expect("Code serializes as a struct") + .keys() + .map(String::as_str) + .collect(); + assert_eq!( + fields, + vec!["code", "hash"], + "unexpected field in the Code wire format" + ); + + // Recomputed on the way back in, so the round trip is still lossless. + let restored: Code = serde_json::from_value(json).expect("deserialize"); + assert_eq!(restored.jumpdests(), code.jumpdests()); + assert_eq!(restored, code); +} diff --git a/test/tests/common/jumpdest_bitmap_tests.rs b/test/tests/common/jumpdest_bitmap_tests.rs new file mode 100644 index 00000000000..f17d89de5a9 --- /dev/null +++ b/test/tests/common/jumpdest_bitmap_tests.rs @@ -0,0 +1,79 @@ +//! [`Code`]'s JUMPDEST bitmap: which offsets are valid jump destinations. + +use ethrex_common::H256; +use ethrex_common::types::Code; + +const JUMPDEST: u8 = 0x5b; +const PUSH1: u8 = 0x60; +const PUSH32: u8 = 0x7f; +const STOP: u8 = 0x00; + +fn code_of(bytecode: Vec) -> Code { + Code::from_bytecode_unchecked(bytecode.into(), H256::zero()) +} + +/// A `JUMPDEST` reached by the opcode walk SHALL be a valid destination. +#[test] +fn jumpdests_outside_immediates_are_valid() { + let code = code_of(vec![STOP, JUMPDEST, STOP, JUMPDEST]); + + assert!(code.is_valid_jumpdest(1)); + assert!(code.is_valid_jumpdest(3)); + assert!(!code.is_valid_jumpdest(0)); + assert!(!code.is_valid_jumpdest(2)); +} + +/// A `0x5b` byte that is part of a `PUSH` immediate SHALL NOT be a valid destination: +/// it is data, not an opcode. +#[test] +fn jumpdest_bytes_inside_push_immediates_are_not_valid() { + // PUSH1 0x5b | JUMPDEST | PUSH32 <32 x 0x5b> | JUMPDEST + let mut bytecode = vec![PUSH1, JUMPDEST, JUMPDEST, PUSH32]; + bytecode.extend([JUMPDEST; 32]); + bytecode.push(JUMPDEST); + let code = code_of(bytecode); + + assert!(!code.is_valid_jumpdest(1), "PUSH1 immediate"); + assert!(code.is_valid_jumpdest(2), "opcode between the two PUSHes"); + for offset in 4..36 { + assert!(!code.is_valid_jumpdest(offset), "PUSH32 immediate {offset}"); + } + assert!(code.is_valid_jumpdest(36), "opcode after the immediates"); +} + +/// Offsets past the bytecode SHALL NOT be valid, including offsets inside the trailing +/// [`BYTECODE_PADDING`](ethrex_common::types::BYTECODE_PADDING) and inside the last, +/// partially used bitmap byte. +#[test] +fn offsets_past_the_bytecode_are_not_valid() { + // 9 bytes of code, so the bitmap's second byte covers offsets 8..16 and only its + // lowest bit is meaningful. + let code = code_of(vec![JUMPDEST; 9]); + + assert!(code.is_valid_jumpdest(8)); + for offset in [9, 10, 15, 16, 100, usize::MAX] { + assert!(!code.is_valid_jumpdest(offset)); + } +} + +/// Jumpless bytecode SHALL have an empty bitmap, as SHALL empty code. +#[test] +fn jumpless_bytecode_has_an_empty_bitmap() { + assert!(code_of(vec![STOP; 64]).jumpdests().is_empty()); + assert!(code_of(vec![PUSH1, JUMPDEST]).jumpdests().is_empty()); + assert!(Code::default().jumpdests().is_empty()); + assert!(!Code::default().is_valid_jumpdest(0)); +} + +/// The bitmap SHALL be one bit per bytecode byte. +#[test] +fn bitmap_is_one_bit_per_bytecode_byte() { + for len in [1usize, 7, 8, 9, 24576] { + let mut bytecode = vec![STOP; len]; + bytecode[len - 1] = JUMPDEST; + let code = code_of(bytecode); + + assert_eq!(code.jumpdests().len(), len.div_ceil(8), "len {len}"); + assert!(code.is_valid_jumpdest(len - 1), "len {len}"); + } +} diff --git a/test/tests/common/mod.rs b/test/tests/common/mod.rs index edda100bdf9..08239d4a4d7 100644 --- a/test/tests/common/mod.rs +++ b/test/tests/common/mod.rs @@ -6,6 +6,7 @@ mod blobs_bundle_tests; mod code_serde_tests; mod eip7702_authorization_tests; mod frame_tx_validation_tests; +mod jumpdest_bitmap_tests; mod legacy_signature_tests; mod logs_bloom_validation_tests; mod requests_eip8282_tests;