-
Notifications
You must be signed in to change notification settings - Fork 217
fix(levm): perform the EIP-8272 native write on every path into the predeploy #7086
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: hegota-devnet
Are you sure you want to change the base?
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 |
|---|---|---|
|
|
@@ -2058,6 +2058,7 @@ impl<'a> VM<'a> { | |
| // depth-driven rather than a single revert/commit. | ||
| let mut body_substate_depth: Option<usize> = None; | ||
| let mut body_logs_start: usize = 0; | ||
| let mut body_frame_start: usize = 0; | ||
| let mut state_gas_used_at_body_entry: i64 = 0; | ||
| let mut post_tx_reverted = false; | ||
|
|
||
|
|
@@ -2165,6 +2166,7 @@ impl<'a> VM<'a> { | |
| body_substate_depth = Some(self.substate.backup_depth()); | ||
| body_backup.bal_checkpoint = self.db.bal_recorder.as_ref().map(|r| r.checkpoint()); | ||
| body_logs_start = all_logs.len(); | ||
| body_frame_start = frame_idx; | ||
| state_gas_used_at_body_entry = self.state_gas_used; | ||
| } | ||
| absorbing_body = is_body_frame; | ||
|
|
@@ -2693,8 +2695,16 @@ impl<'a> VM<'a> { | |
| } | ||
| crate::utils::restore_cache_state(self.db, mem::take(&mut body_backup))?; | ||
| // Logs the body emitted are gone with its state. The prefix's logs | ||
| // (an APPROVE-side EIP-7708 transfer log, say) survive. | ||
| // (an APPROVE-side EIP-7708 transfer log, say) survive. The per-frame | ||
| // receipts keep their status and gas but lose those logs too — the | ||
| // consensus receipt carries only them, so the header bloom is built from | ||
| // them and would otherwise commit to logs that no longer happened. | ||
| all_logs.truncate(body_logs_start); | ||
| if let Some(ctx) = self.frame_tx_context.as_mut() { | ||
| for (_, _, logs) in ctx.frame_results.iter_mut().skip(body_frame_start) { | ||
| logs.clear(); | ||
| } | ||
| } | ||
| // EIP-8037: the body was unrolled, so it created no state and owes no | ||
| // state gas. Mirrors the atomic-batch unroll, which drops the state | ||
| // gas accumulated since batch entry for the same reason. EIP-7906 does | ||
|
|
@@ -3443,6 +3453,57 @@ impl<'a> VM<'a> { | |
| self.env.config.fork, | ||
| self.vm_type, | ||
| ) | ||
| // EIP-8272's predeploy is codeless too, but a call into it performs the native | ||
| // recent-root write, so it must reach `run_execution`. | ||
| && !(self.env.config.fork >= Fork::Hegota | ||
| && self.current_call_frame.to == ethrex_common::types::frame_tx_recent_root()) | ||
| } | ||
|
|
||
| /// EIP-8272 native write for a transaction whose recipient is the predeploy itself. | ||
| fn run_top_level_recent_root_write(&mut self) -> Result<ContextResult, VMError> { | ||
| let recent_root_addr = ethrex_common::types::frame_tx_recent_root(); | ||
| if let Some(recorder) = self.db.bal_recorder.as_mut() { | ||
| recorder.record_touched_address(recent_root_addr); | ||
| } | ||
|
|
||
| let gas_limit = self.current_call_frame.gas_limit; | ||
| let revert = |gas_used: u64| ContextResult { | ||
| result: TxResult::Revert(VMError::RevertOpcode), | ||
| gas_used, | ||
| gas_spent: gas_used, | ||
| output: Bytes::new(), | ||
| }; | ||
|
|
||
| // `gas_remaining` enters this frame already net of intrinsic gas, so what the | ||
| // transaction owes is always measured against `gas_limit`, never the write cost alone. | ||
| #[expect(clippy::as_conversions, reason = "gas_remaining is non-negative here")] | ||
| let consumed = |frame: &crate::call_frame::CallFrame| { | ||
| frame.gas_limit.saturating_sub(frame.gas_remaining.max(0) as u64) | ||
| }; | ||
|
|
||
| if self.current_call_frame.calldata.len() != 64 || !self.current_call_frame.msg_value.is_zero() { | ||
| return Ok(revert(consumed(&self.current_call_frame))); | ||
| } | ||
| if self.current_call_frame.gas_remaining < crate::gas_cost::RECENT_ROOT_WRITE_GAS as i64 { | ||
| self.current_call_frame.gas_remaining = 0; | ||
| return Ok(revert(gas_limit)); | ||
| } | ||
|
|
||
| let caller = self.current_call_frame.msg_sender; | ||
| let calldata = self.current_call_frame.calldata.clone(); | ||
| let (salt, root) = calldata.split_at(32); | ||
| self.recent_root_native_write(caller, salt, root)?; | ||
| self.current_call_frame.gas_remaining = self | ||
| .current_call_frame | ||
| .gas_remaining | ||
| .saturating_sub(crate::gas_cost::RECENT_ROOT_WRITE_GAS as i64); | ||
| let gas_used = consumed(&self.current_call_frame); | ||
| Ok(ContextResult { | ||
| result: TxResult::Success, | ||
| gas_used, | ||
| gas_spent: gas_used, | ||
| output: Bytes::new(), | ||
| }) | ||
| } | ||
|
|
||
| /// Main execution loop. | ||
|
|
@@ -3481,6 +3542,17 @@ impl<'a> VM<'a> { | |
| // `refill_frame_state_gas`). Set in-region by `prepare_execution`. | ||
| let top_frame_new_account_charged = self.value_new_account_charged; | ||
|
|
||
| // EIP-8272: the predeploy is codeless, so a transaction sent straight to it would | ||
| // otherwise run as a transfer to an ordinary account and write nothing. Same native | ||
| // write the CALL-opcode and frame paths perform. Only the top-level entry reaches | ||
| // here: a frame targeting the predeploy is codeless, so it takes the default-code | ||
| // path, where `execute_default_code` performs the write and never calls back in. | ||
| if self.env.config.fork >= Fork::Hegota | ||
| && self.current_call_frame.to == ethrex_common::types::frame_tx_recent_root() | ||
| { | ||
| return self.run_top_level_recent_root_write(); | ||
|
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.
So this guard fires for any frame whose The gas comment inside makes the same assumption explicit:
That holds for the top-level frame, where I haven't traced what a predeploy-targeted frame did before this change, so I can't say whether the new path is wrong for it or merely undocumented. Worth confirming, since "undercharged by exactly its intrinsic gas" is the bug this PR exists to fix and the frame path is where the asymmetry would hide. If it is meant to be top-level only, the guard needs a depth/call-site condition to match the name.
Author
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 guard is reachable only from |
||
| } | ||
|
|
||
| #[expect(clippy::as_conversions, reason = "remaining gas conversion")] | ||
| if precompiles::is_precompile( | ||
| &self.current_call_frame.to, | ||
|
|
||
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.
This is a fourth change, and the description lists three. It's also the one with the widest blast radius: clearing per-frame receipt logs on a body unroll changes what the consensus receipt carries, and your own comment says the header bloom is built from those logs.
That's a separate bug from the EIP-8272 native write — different EIP, different failure mode (a bloom committing to logs that didn't survive, versus a root that never gets written). It stands on its own and reads correct to me:
all_logs.truncate(body_logs_start)already dropped them from the transaction-level set, so leaving them inframe_resultswas an inconsistency between the two views.Two asks, no rework implied:
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.
Added to the description. It was found by inspection while reading the unroll path, not from an observed divergence, and it has no unit coverage, so it is the least supported part of the diff.