perf(l1): short-circuit KECCAK256 on empty input#6775
Conversation
Return the precomputed keccak256("") constant for zero-length input
instead of running the permutation. Benchmarkoor shows ethrex at ~19x
reth's gap on empty-input KECCAK256 while at parity for non-empty
hashing, isolating the cost to per-op hashing of empty data.
github-actions
Bot
added
L1
🤖 Codex Code ReviewNo findings.
Residual risk: I could not run the Rust test locally in this environment because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR short-circuits the
Confidence Score: 5/5Safe to merge — the change is narrowly scoped, the constant is mathematically verified by the accompanying test, and gas/memory accounting paths are untouched. The short-circuit only fires when len == 0, a path that was already a no-op in calculate_memory_size and load_range. The constant's limb ordering matches the u256_from_big_endian encoding used everywhere else in the codebase, and the unit test provides a runtime proof against the live hasher. No edge cases were left uncovered. No files require special attention.
|
| Filename | Overview |
|---|---|
| crates/vm/levm/src/opcode_handlers/keccak.rs | Adds a compile-time EMPTY_KECCAK_U256 constant and short-circuits the KECCAK256 opcode handler for zero-length input, skipping the permutation entirely; gas accounting and memory semantics are unchanged. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[KECCAK256 opcode] --> B[Pop offset, len from stack]
B --> C[Compute & charge gas\ncalculate_memory_size + keccak256 word cost]
C --> D{len == 0?}
D -- Yes --> E[Return EMPTY_KECCAK_U256\nprecomputed constant\nskip memory load & permutation]
D -- No --> F[memory.load_range offset, len]
F --> G[crypto.keccak256 bytes]
G --> H[u256_from_big_endian hash]
E --> I[stack.push hash]
H --> I
I --> J[OpcodeResult::Continue]
Reviews (1): Last reviewed commit: "perf(l1): short-circuit KECCAK256 on emp..." | Re-trigger Greptile
🤖 Kimi Code ReviewThe optimization to skip the keccak permutation for empty input is a standard performance improvement used by other clients (geth, revm, etc.). However, there's a potential consensus-critical issue regarding memory expansion. Critical Check Required
Verify that:
If memory expansion happens during
Code Correctness Lines 17-21: The constant values correctly represent the little-endian
Style Line 13: The import use ethrex_common::{U256, utils::u256_from_big_endian};Testing Lines 56-60: The unit test correctly verifies the constant against the cryptographic implementation. Consider also adding an integration test ensuring Recommendation: Confirm the memory expansion behavior of Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewHere is the review for PR #6775 — PR OverviewShort-circuits the CorrectnessConstant value — verified correct. The unit test confirms the constant against the live hasher, which is the right approach. Memory expansion — no behavioral change. For
Both paths were already no-ops before the branch was introduced. The branch does not alter observable memory state. Gas accounting — unchanged and correct. EVM spec compliance — KECCAK256 of zero-length input must return the well-known constant. This is correct. Minor IssuesImport style (line 14–15) — the two imports from // before
use ethrex_common::U256;
use ethrex_common::utils::u256_from_big_endian;
// after
use ethrex_common::{U256, utils::u256_from_big_endian};Comment wording (line 17–18) — "hot path skips the permutation" is misleading. Empty-input keccak is an optimised cold path, not the hot path. Consider: "Skips the permutation for zero-length input; the result is a well-known constant." Test hardening (optional) — the current test catches a wrong constant or a wrong hasher, but not a mutual bug where both return the same wrong value. Adding a literal byte assertion is a low-cost guard: assert_eq!(
NativeCrypto.keccak256(&[]),
hex_literal::hex!("c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470")
);This is advisory — the existing test is already meaningful. SummaryThe optimization is correct, spec-compliant, and well-tested. Gas and memory semantics are preserved. The only actionable change is merging the two Automated review by Claude (Anthropic) · sonnet · custom prompt |
Lines of code reportTotal lines added: Detailed view |
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
ElFantasma
left a comment
ElFantasma
left a comment
There was a problem hiding this comment.
Approved with a note... It's up to you to fix it or file a follow up issue.
|
|
||
| /// `keccak256("")` as a `U256`. Returned directly for zero-length input so the | ||
| /// hot path skips the permutation entirely (matches what other clients do). | ||
| const EMPTY_KECCAK_U256: U256 = U256([ |
There was a problem hiding this comment.
We now have two constants for keccak256("") — EMPTY_KECCACK_HASH: LazyLock<H256> in crates/common/constants.rs:38 (note the pre-existing typo — extra C) and this new EMPTY_KECCAK_U256: U256. They have to stay in lock-step forever, in two files, under two different names. That's the kind of duplication that tends to drift quietly.
The LazyLock was only needed because the existing definition uses hex::decode(...).expect(...) at runtime. A true const H256 is straightforward — H256 is just H256([u8; 32]) (verified: H256([2; 32]) construction is already used elsewhere in the tree), and hex_literal::hex! is already a workspace dep (used by crates/common/crypto/provider.rs and others). With that, both shapes become real consts side-by-side:
// in crates/common/constants.rs
pub const EMPTY_KECCAK_HASH: H256 = H256(hex_literal::hex!(
"c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470"
));
pub const EMPTY_KECCAK_U256: U256 = U256([
0x7bfad8045d85a470, 0xe500b653ca82273b,
0x927e7db2dcc703c0, 0xc5d2460186f7233c,
]);
// + a #[test] asserting the two encode the same bytes — catches drift.Two paths I can see:
- In this PR: define both constants together in
common::constants, deprecate (or rename)EMPTY_KECCACK_HASH. The rename touches ~17 files (git grep -l EMPTY_KECCACK_HASHreturns: vm.rs, account.rs, block.rs, block_execution_witness.rs, healing/state.rs, snap_sync.rs, types/block.rs, store.rs, levm db.rs/mod.rs/tracing.rs, levm/account.rs, levm/db/gen_db.rs, store_tests.rs, archive_sync, ef_tests test_runner.rs). Mechanicals/EMPTY_KECCACK_HASH/EMPTY_KECCAK_HASH/g. - Land this as-is, file a follow-up: this PR stays surgical (1 const, 1 test, hot-path fix); the unification + rename PR can stand on its own. If you go this way, a drift-catching test extension would be the cheapest in-PR guard:
assert_eq!(EMPTY_KECCAK_U256, u256_from_big_endian(EMPTY_KECCACK_HASH.as_bytes()));next to the existing assertion catches divergence if either ever drifts.
Your call. Non-blocking on the perf merit of this PR either way.
- merge ethrex_common imports - reword misleading hot-path comment - add drift guard test against EMPTY_KECCACK_HASH
Benchmark Block Execution Results Comparison Against Main
|
4 participants
Motivation
Benchmarkoor (ethrex
bal-fullsuite) shows ethrex is at parity with reth for non-empty KECCAK256 (msg_size 32/256/1024 ≈ 1.0x throughput ratio), but ~19x slower on zero-length input:The gap is isolated to running the keccak permutation on empty input.
keccak256("")is a constant; fast clients return it directly.Change
In the
KECCAK256handler, return the precomputedkeccak256("")value as aU256whenlen == 0, skipping both the memory load and the permutation. Gas accounting is unchanged. A unit test asserts the constant equalsNativeCrypto.keccak256(&[]).Testing
cargo test -p ethrex-levm --lib opcode_handlers::keccakpasses (const verified against the hasher).cargo clippy -p ethrex-levmclean.