-
Notifications
You must be signed in to change notification settings - Fork 199
fix(l1): close EIP-8025 stateless witness validation gaps (re-enable 9 zkevm tests) #6541
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.
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
base: feat/l1-ef-tests-bump-zkevm-fixtures-v0.3.3
Are you sure you want to change the base?
Changes from all commits
c10ae6b
4a4a696
3ecf85c
e72f5ea
22d4e39
3de476f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -151,36 +151,31 @@ impl RpcExecutionWitness { | |||||||||||||||
| )); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| let mut initial_state_root = None; | ||||||||||||||||
|
|
||||||||||||||||
| for h in &self.headers { | ||||||||||||||||
| let header = BlockHeader::decode(h)?; | ||||||||||||||||
| if header.number == first_block_number - 1 { | ||||||||||||||||
| initial_state_root = Some(header.state_root); | ||||||||||||||||
| break; | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| let initial_state_root = initial_state_root.ok_or_else(|| { | ||||||||||||||||
| GuestProgramStateError::Custom(format!( | ||||||||||||||||
| "header for block {} not found", | ||||||||||||||||
| first_block_number - 1 | ||||||||||||||||
| )) | ||||||||||||||||
| })?; | ||||||||||||||||
| // Skip headers that fail to decode, then pick parent by number. | ||||||||||||||||
| let initial_state_root = self | ||||||||||||||||
| .headers | ||||||||||||||||
| .iter() | ||||||||||||||||
| .filter_map(|h| BlockHeader::decode(h).ok()) | ||||||||||||||||
| .find(|header| header.number == first_block_number - 1) | ||||||||||||||||
| .map(|header| header.state_root) | ||||||||||||||||
| .ok_or_else(|| { | ||||||||||||||||
| GuestProgramStateError::Custom(format!( | ||||||||||||||||
| "header for block {} not found", | ||||||||||||||||
| first_block_number - 1 | ||||||||||||||||
| )) | ||||||||||||||||
| })?; | ||||||||||||||||
|
|
||||||||||||||||
| // EIP-8025: drop entries that don't decode. They can't be looked up by | ||||||||||||||||
| // hash anyway; if execution needs them, the trie walk fails there. | ||||||||||||||||
| // Ref: https://github.com/ethereum/execution-specs/blob/projects/zkevm/src/ethereum/forks/amsterdam/witness_state.py#L37-L42 | ||||||||||||||||
| let nodes: BTreeMap<H256, Node> = self | ||||||||||||||||
| .state | ||||||||||||||||
| .into_iter() | ||||||||||||||||
| .filter_map(|b| { | ||||||||||||||||
| if b == Bytes::from_static(&[0x80]) { | ||||||||||||||||
| // other implementations of debug_executionWitness allow for a `Null` node, | ||||||||||||||||
| // which would fail to decode in ours | ||||||||||||||||
| return None; | ||||||||||||||||
| } | ||||||||||||||||
| let hash = keccak(&b); | ||||||||||||||||
| Some(Node::decode(&b).map(|node| (hash, node))) | ||||||||||||||||
| let node = Node::decode(&b).ok()?; | ||||||||||||||||
| Some((keccak(&b), node)) | ||||||||||||||||
| }) | ||||||||||||||||
| .collect::<Result<_, RLPDecodeError>>()?; | ||||||||||||||||
| .collect(); | ||||||||||||||||
|
|
||||||||||||||||
| // get state trie root and embed the rest of the trie into it | ||||||||||||||||
| let state_trie_root = if let NodeRef::Node(state_trie_root, _) = | ||||||||||||||||
|
|
@@ -325,17 +320,28 @@ impl GuestProgramState { | |||||||||||||||
| value: ExecutionWitness, | ||||||||||||||||
| crypto: &dyn Crypto, | ||||||||||||||||
| ) -> Result<Self, GuestProgramStateError> { | ||||||||||||||||
| let block_headers: BTreeMap<u64, BlockHeader> = value | ||||||||||||||||
| .block_headers_bytes | ||||||||||||||||
| .into_iter() | ||||||||||||||||
| .map(|bytes| BlockHeader::decode(bytes.as_ref())) | ||||||||||||||||
| .collect::<Result<Vec<_>, _>>() | ||||||||||||||||
| .map_err(|e| { | ||||||||||||||||
| GuestProgramStateError::Custom(format!("Failed to decode block headers: {}", e)) | ||||||||||||||||
| })? | ||||||||||||||||
| .into_iter() | ||||||||||||||||
| .map(|header| (header.number, header)) | ||||||||||||||||
| .collect(); | ||||||||||||||||
| // EIP-8025: headers must form a contiguous chain in list order (each | ||||||||||||||||
| // `parent_hash` matches keccak of the previous header bytes). Reordered | ||||||||||||||||
| // or fragmented chains are invalid even if by-number lookup would resolve. | ||||||||||||||||
| // Ref: https://github.com/ethereum/execution-specs/blob/projects/zkevm/src/ethereum/forks/amsterdam/stateless.py#L171-L191 | ||||||||||||||||
| let mut block_headers: BTreeMap<u64, BlockHeader> = BTreeMap::new(); | ||||||||||||||||
| let mut prev_hash: Option<H256> = None; | ||||||||||||||||
| for bytes in &value.block_headers_bytes { | ||||||||||||||||
| let Ok(header) = BlockHeader::decode(bytes.as_ref()) else { | ||||||||||||||||
| // Malformed entry is a chain break; the next parent_hash check fails. | ||||||||||||||||
| prev_hash = None; | ||||||||||||||||
| continue; | ||||||||||||||||
| }; | ||||||||||||||||
| if let Some(expected_parent) = prev_hash | ||||||||||||||||
| && header.parent_hash != expected_parent | ||||||||||||||||
| { | ||||||||||||||||
| return Err(GuestProgramStateError::Custom( | ||||||||||||||||
| "witness headers are not contiguous".to_string(), | ||||||||||||||||
| )); | ||||||||||||||||
|
Comment on lines
+337
to
+340
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There is already a typed variant
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/common/types/block_execution_witness.rs
Line: 337-340
Comment:
**Use the existing `NoncontiguousBlockHeaders` error variant**
There is already a typed variant `GuestProgramStateError::NoncontiguousBlockHeaders` (defined a few lines above) with the message "Non-contiguous block headers (there's a gap in the block headers list)". Using `Custom(...)` here bypasses that variant and makes error matching on the call-site harder.
```suggestion
{
return Err(GuestProgramStateError::NoncontiguousBlockHeaders);
}
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||
| } | ||||||||||||||||
| prev_hash = Some(H256(crypto.keccak256(bytes))); | ||||||||||||||||
| block_headers.insert(header.number, header); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| let parent_number = | ||||||||||||||||
| value | ||||||||||||||||
|
|
@@ -588,28 +594,22 @@ impl GuestProgramState { | |||||||||||||||
| Ok(self.chain_config) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /// Retrieves the account code for a specific account. | ||||||||||||||||
| /// Returns an Err if the code is not found. | ||||||||||||||||
| /// Retrieves bytecode by code hash. Errors if missing — EIP-8025. | ||||||||||||||||
| /// | ||||||||||||||||
| /// Ref: https://github.com/ethereum/execution-specs/blob/projects/zkevm/src/ethereum/forks/amsterdam/witness_state.py#L204-L212 | ||||||||||||||||
| pub fn get_account_code(&self, code_hash: H256) -> Result<Code, GuestProgramStateError> { | ||||||||||||||||
| if code_hash == *EMPTY_KECCACK_HASH { | ||||||||||||||||
| return Ok(Code::default()); | ||||||||||||||||
| } | ||||||||||||||||
| match self.codes_hashed.get(&code_hash) { | ||||||||||||||||
| Some(code) => Ok(code.clone()), | ||||||||||||||||
| None => { | ||||||||||||||||
| // We do this because what usually happens is that the Witness doesn't have the code we asked for but it is because it isn't relevant for that particular case. | ||||||||||||||||
| // In client implementations there are differences and it's natural for some clients to access more/less information in some edge cases. | ||||||||||||||||
| // Sidenote: logger doesn't work inside SP1, that's why we use println! | ||||||||||||||||
| println!( | ||||||||||||||||
| "Missing bytecode for hash {} in witness. Defaulting to empty code.", // If there's a state root mismatch and this prints we have to see if it's the cause or not. | ||||||||||||||||
| hex::encode(code_hash) | ||||||||||||||||
| ); | ||||||||||||||||
| Ok(Code::default()) | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| self.codes_hashed.get(&code_hash).cloned().ok_or_else(|| { | ||||||||||||||||
| GuestProgramStateError::Database(format!( | ||||||||||||||||
| "missing bytecode for hash {} in witness", | ||||||||||||||||
| hex::encode(code_hash) | ||||||||||||||||
| )) | ||||||||||||||||
| }) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /// Retrieves code metadata (length) for a specific code hash. | ||||||||||||||||
| /// Code length by hash. Errors on miss, like `get_account_code`. | ||||||||||||||||
| /// This is an optimized path for EXTCODESIZE opcode. | ||||||||||||||||
| pub fn get_code_metadata( | ||||||||||||||||
| &self, | ||||||||||||||||
|
|
@@ -620,19 +620,17 @@ impl GuestProgramState { | |||||||||||||||
| if code_hash == *EMPTY_KECCACK_HASH { | ||||||||||||||||
| return Ok(CodeMetadata { length: 0 }); | ||||||||||||||||
| } | ||||||||||||||||
| match self.codes_hashed.get(&code_hash) { | ||||||||||||||||
| Some(code) => Ok(CodeMetadata { | ||||||||||||||||
| self.codes_hashed | ||||||||||||||||
| .get(&code_hash) | ||||||||||||||||
| .map(|code| CodeMetadata { | ||||||||||||||||
| length: code.bytecode.len() as u64, | ||||||||||||||||
| }), | ||||||||||||||||
| None => { | ||||||||||||||||
| // Same as get_account_code - default to empty for missing bytecode | ||||||||||||||||
| println!( | ||||||||||||||||
| "Missing bytecode for hash {} in witness. Defaulting to empty code metadata.", | ||||||||||||||||
| }) | ||||||||||||||||
| .ok_or_else(|| { | ||||||||||||||||
| GuestProgramStateError::Database(format!( | ||||||||||||||||
| "missing bytecode for hash {} in witness", | ||||||||||||||||
| hex::encode(code_hash) | ||||||||||||||||
| ); | ||||||||||||||||
| Ok(CodeMetadata { length: 0 }) | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| )) | ||||||||||||||||
| }) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /// When executing multiple blocks in the L2 it happens that the headers in block_headers correspond to the same block headers that we have in the blocks array. The main goal is to hash these only once and set them in both places. | ||||||||||||||||
|
|
||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says "the next parent_hash check fails," but when
prev_hashis set toNone, the subsequentif let Some(expected_parent) = prev_hashguard in the next iteration simply does not fire — the check is bypassed, not triggered-and-failed. A validator reading this could mistake the skipping for active rejection.Prompt To Fix With AI