-
Notifications
You must be signed in to change notification settings - Fork 207
feat(l1): bump zkEVM fixtures to v0.3.3 and tighten witness validation #6498
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
Changes from all commits
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,16 +151,27 @@ 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; | ||
| // Decode headers up front so we can validate the ordering invariant: | ||
| // an RPC witness must provide headers in ascending, contiguous block-number | ||
| // order. Reordered or gapped inputs would otherwise be silently normalized | ||
| // by the BTreeMap in `GuestProgramState::from_witness` and yield an | ||
| // incorrect conclusion about the block's validity. | ||
| let decoded_headers: Vec<BlockHeader> = self | ||
| .headers | ||
| .iter() | ||
| .map(|h| BlockHeader::decode(h)) | ||
| .collect::<Result<_, _>>()?; | ||
| for window in decoded_headers.windows(2) { | ||
| if window[1].number != window[0].number + 1 { | ||
| return Err(GuestProgramStateError::NoncontiguousBlockHeaders); | ||
| } | ||
| } | ||
|
|
||
| let initial_state_root = decoded_headers | ||
| .iter() | ||
| .find(|h| h.number == first_block_number - 1) | ||
| .map(|h| h.state_root); | ||
|
|
||
| let initial_state_root = initial_state_root.ok_or_else(|| { | ||
| GuestProgramStateError::Custom(format!( | ||
| "header for block {} not found", | ||
|
|
@@ -177,10 +188,13 @@ impl RpcExecutionWitness { | |
| // which would fail to decode in ours | ||
| return None; | ||
| } | ||
| // Tolerate extra unused witness entries that don't decode as trie nodes. | ||
|
Contributor
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. The tolerance is correct per EIP-8025 §Tolerance — extra unused witness entries should not be a hard error. But the change from Not blocking — the spec is permissive here — but a |
||
| // Nodes that are actually needed for state access are looked up by hash | ||
| // later; extra junk cannot be referenced. | ||
| let hash = keccak(&b); | ||
| Some(Node::decode(&b).map(|node| (hash, node))) | ||
| Node::decode(&b).ok().map(|node| (hash, 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, _) = | ||
|
|
@@ -589,28 +603,25 @@ impl GuestProgramState { | |
| } | ||
|
|
||
| /// Retrieves the account code for a specific account. | ||
| /// Returns an Err if the code is not found. | ||
| /// Returns an Err if the code is not found — a missing code hash means | ||
| /// the execution witness is invalid and the guest program must not | ||
| /// conclude anything about the state. | ||
| 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::Custom(format!( | ||
| "missing bytecode for hash {} in execution witness", | ||
| hex::encode(code_hash) | ||
| )) | ||
| }) | ||
| } | ||
|
|
||
| /// Retrieves code metadata (length) for a specific code hash. | ||
| /// This is an optimized path for EXTCODESIZE opcode. | ||
| /// Returns an Err if the code is not found — same rationale as | ||
| /// [`Self::get_account_code`]. | ||
| pub fn get_code_metadata( | ||
| &self, | ||
| code_hash: H256, | ||
|
|
@@ -620,19 +631,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::Custom(format!( | ||
| "missing bytecode for hash {} in execution 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. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| https://github.com/ethereum/execution-spec-tests/releases/download/zkevm%40v0.3.0/fixtures_zkevm.tar.gz | ||
| https://github.com/ethereum/execution-spec-tests/releases/download/zkevm%40v0.3.3/fixtures_zkevm.tar.gz |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -529,6 +529,11 @@ async fn re_run_stateless( | |
| /// trie nodes, codes, and ancestor headers needed for that specific block. | ||
| /// Following the spec, we execute each block | ||
| /// independently with its own witness. | ||
| /// | ||
| /// The fixture's `statelessOutputBytes` encodes the expected output as | ||
| /// `new_payload_request_root (32 bytes) || valid (1 byte) || padding`. When | ||
| /// `valid == 0`, the witness is intentionally invalid and stateless execution | ||
| /// is expected to fail (or conclude the block is invalid). | ||
| #[cfg(feature = "stateless")] | ||
| async fn run_stateless_from_fixture( | ||
| test: &TestUnit, | ||
|
|
@@ -551,6 +556,9 @@ async fn run_stateless_from_fixture( | |
| continue; | ||
| }; | ||
|
|
||
| let expect_stateless_valid = | ||
| expected_stateless_valid(block_data.stateless_output_bytes.as_deref()); | ||
|
|
||
| let block: CoreBlock = block_data.clone().into(); | ||
| let block_number = block.header.number; | ||
|
|
||
|
|
@@ -559,9 +567,21 @@ async fn run_stateless_from_fixture( | |
| format!("Failed to parse executionWitness for block {block_number}: {e}") | ||
| })?; | ||
|
|
||
| let execution_witness = rpc_witness | ||
| .into_execution_witness(*chain_config, block_number) | ||
| .map_err(|e| format!("Witness conversion failed for block {block_number}: {e}"))?; | ||
| let witness_conversion_result = | ||
| rpc_witness.into_execution_witness(*chain_config, block_number); | ||
| let execution_witness = match witness_conversion_result { | ||
| Ok(w) => w, | ||
| Err(e) => { | ||
| // An intentionally invalid witness may fail conversion — that's a valid | ||
| // "block is invalid" conclusion when the fixture marks this block invalid. | ||
| if !expect_stateless_valid { | ||
| continue; | ||
| } | ||
| return Err(format!( | ||
| "Witness conversion failed for block {block_number}: {e}" | ||
| )); | ||
| } | ||
| }; | ||
|
|
||
| let program_input = ProgramInput::new(vec![block], execution_witness); | ||
|
|
||
|
|
@@ -571,12 +591,39 @@ async fn run_stateless_from_fixture( | |
| BackendType::SP1 => Sp1Backend::new().execute(program_input), | ||
| }; | ||
|
|
||
| if let Err(e) = execute_result { | ||
| return Err(format!( | ||
| "Stateless execution from fixture failed for {test_key} block {block_number}: {e}" | ||
| )); | ||
| match execute_result { | ||
| Ok(()) => { | ||
| if !expect_stateless_valid { | ||
| return Err(format!( | ||
| "Expected stateless execution to fail for {test_key} block {block_number} (fixture valid=0x00) but it succeeded" | ||
| )); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| if expect_stateless_valid { | ||
| return Err(format!( | ||
| "Stateless execution from fixture failed for {test_key} block {block_number}: {e}" | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Parse the `valid` flag (byte 32) from a fixture's `statelessOutputBytes`. | ||
| /// | ||
| /// Layout: `new_payload_request_root (32 bytes) || valid (1 byte) || padding`. | ||
| /// If absent or unparseable, default to expecting success (true) to preserve | ||
| /// the prior behavior for fixtures without this field. | ||
| #[cfg(feature = "stateless")] | ||
| fn expected_stateless_valid(hex_str: Option<&str>) -> bool { | ||
| let Some(s) = hex_str else { return true }; | ||
| let s = s.strip_prefix("0x").unwrap_or(s); | ||
| // valid byte is at hex offset 64..66 (bytes[32]) | ||
| match s.get(64..66).and_then(|b| u8::from_str_radix(b, 16).ok()) { | ||
| Some(0) => false, | ||
|
Contributor
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. nit: the
Returning |
||
| _ => true, | ||
| } | ||
| } | ||
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.
Security observation: this only validates numbering, not chain linkage. An attacker could provide headers numbered correctly but with arbitrary
parent_hashvalues, fabricating anystate_rootfor the block being verified. The check is a useful guard against accidental reordering / gaps from honest peers, but a malicious witness can trivially bypass it.#6541 (open) adds the load-bearing check:
window[1].parent_hash == window[0].hash(). That's the one that closes the door on fabricated state roots. This PR's numbering check is a useful preliminary but not sufficient on its own.Worth either (a) rebasing on #6541 if the order works out, or (b) adding the parent_hash check here so this PR is self-sufficient even if #6541 lingers.