fix(l1,l2): fix occasional error with debug_executionWitness
#5212
Conversation
Lines of code reportTotal lines added: Detailed view |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues with the debug_executionWitness RPC method by addressing two main problems:
-
BLOCKHASH opcode handling: Previously, the code assumed block hash data was always available in a map, but this failed when blocks referenced via the BLOCKHASH opcode weren't included. The fix properly fetches all intermediate block headers needed for witness generation.
-
Pre-computed block hashes: The guest program now accepts blocks with hashes already computed, validating them against recomputed values instead of requiring them to be uninitialized.
Key Changes
- Modified
initialize_block_header_hashesto validate pre-set hashes instead of rejecting them - Replaced
block_hashes_mapwith directblock.header.hash()calls using the OnceCell caching mechanism - Refactored header fetching logic to walk backwards from the last block to the first needed block, fetching headers by hash rather than by number
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/common/types/block_execution_witness.rs |
Updated hash initialization logic to validate pre-set hashes and avoid double-hashing headers that appear in both the witness and blocks array |
crates/common/types/block.rs |
Changed compute_block_hash visibility from private to public to support external hash computation |
crates/blockchain/blockchain.rs |
Removed block_hashes_map construction and refactored header fetching to properly handle BLOCKHASH opcode references by walking backwards through parent hashes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut block_numbers_in_common = vec![]; | ||
| for block in blocks { | ||
| // Verify each block's header hash is uninitialized | ||
| if block.header.hash.get().is_some() { | ||
| return Err(GuestProgramStateError::Custom(format!( | ||
| "Block header hash is already set for {}", | ||
| block.header.number | ||
| ))); | ||
| let hash = block.header.compute_block_hash(); | ||
| set_hash_or_validate(&block.header, hash)?; | ||
|
|
||
| let number = block.header.number; | ||
| if let Some(header) = self.block_headers.get(&number) { | ||
| block_numbers_in_common.push(number); | ||
| set_hash_or_validate(header, hash)?; | ||
| } | ||
| let header = self | ||
| .block_headers | ||
| .get(&block.header.number) | ||
| .unwrap_or(&block.header); | ||
|
|
||
| let hash = header.hash(); | ||
| // this returns err if it's already set, so we drop the Result as we don't | ||
| // care if it was already initialized. | ||
| let _ = block.header.hash.set(hash); | ||
| } | ||
|
|
||
| for header in self.block_headers.values() { | ||
| if block_numbers_in_common.contains(&header.number) { | ||
| // We have already set this hash in the previous step |
There was a problem hiding this comment.
[nitpick] Using Vec::contains in the loop makes this O(n*m) where n is the number of headers and m is the number of blocks in common. Consider using a HashSet<u64> instead of Vec<u64> for block_numbers_in_common to make the lookup O(1), improving overall performance to O(n+m).
There was a problem hiding this comment.
Okay, I will use a BTreeSet because HashSet is not zkvm friendly
**Motivation** <!-- Why does this pull request exist? What are its goals? --> - Sometimes when we want a witness ethrex returns an error, this aims to fix it. **Description** <!-- A clear and concise general description of the changes this PR introduces --> 1. There's a bug that assumes that we have some data in a map that we don't necessarily have. This is usually triggered when there's a call to the BLOCKHASH opcode during a block. The idea is to requests all the headers of intermediate blocks so that we can generate the witness correctly. 2. Now the Guest Program accepts blocks with hash already computed, it just recomputes it and compares the hash with the already set one (if any), returning an error in case they differ. This was suggested in eth-act/zkevm-benchmark-workload#177 and we liked the idea. Both changes are in the same PR because the goal was just to solve `1` but as I was hashing the blocks when generating the witness I decided to fix `2` elegantly so we can pass the Blockchain tests. Closes #5250 --------- Co-authored-by: Ivan Litteri <67517699+ilitteri@users.noreply.github.com>
Motivation
Description
Both changes are in the same PR because the goal was just to solve
1but as I was hashing the blocks when generating the witness I decided to fix2elegantly so we can pass the Blockchain tests.Closes #5250