fix(evm): meter CALLCODE new-account storage gas against caller context#284
fix(evm): meter CALLCODE new-account storage gas against caller context#284TroublorBot wants to merge 7 commits into
Conversation
…xt (#283) Pre-Rex5, the storage-gas wrapper for CALLCODE used the stack `to` address — the code-source — for both the emptiness check and the `new_account_storage_gas(...)` charge. For CALLCODE, however, execution happens in the caller's account context: the stack `to` only selects which code to load, while the storage / account context remains the caller's. As a result, value-transferring CALLCODE could spuriously charge new-account storage gas based on the code-source's bucket and emptiness rather than the caller's storage context. Rex5 splits CALLCODE out of the shared `wrap_call_with_storage_gas!` macro and meters new-account storage gas against `interpreter.input.target_address()` (the current frame's storage account). The stack `to` is still used solely as the code-source for the underlying CALLCODE instruction. Pre-Rex5 specs preserve the (frozen) prior behavior for backward compatibility. CALL semantics are unchanged. Adds regression tests in `crates/mega-evm/tests/rex5/callcode_storage_gas.rs` that pin both the Rex5 fix and the pre-Rex5 frozen behavior, and that confirm CALL is unaffected. Updates the Rex5 spec page accordingly. Generated-by: engineer-agent
TroublorBot
requested review from
RealiCZ,
Troublor,
flyq and
vincent-k2026
as code owners
Troublor
added
api:unchanged
|
🔧 Pushed CI fixes. Agent log |
|
Labels |
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ReviewThe fix is semantically correct. The spec-gating, backward-compatibility freeze, and doc update all look correct. Minor observation: Under Rex5, Tests cover the four key scenarios (Rex5/Rex4 × CALLCODE/CALL) and correctly pin the frozen pre-Rex5 behaviour. LGTM. |
|
@TroublorBot fix ci |
Troublor
left a comment
Troublor
left a comment
There was a problem hiding this comment.
Refactor suggestion inline: the new call_code is largely a copy of the wrap_call_with_storage_gas! expansion. Consider parameterizing the macro with an optional storage-address selector so CALL/CALLCODE/DELEGATECALL/STATICCALL stay in lockstep on the shared logic. Non-blocking — fix is correct and well-tested.
| /// `CALLCODE` opcode implementation modified from `revm` with compute gas tracking and | ||
| /// dynamically-scaled storage gas costs. | ||
| /// | ||
| /// This is intentionally not generated via [`wrap_call_with_storage_gas!`] because `CALLCODE` |
There was a problem hiding this comment.
The hand-written call_code is ~90% identical to the macro expansion — the only real differences are:
- The storage address:
to(macro) vs. spec-gatedtarget_address()/to(here). - Stack[2] inspect being unconditional (CALLCODE always has a value operand).
Everything else — stack inspect for to, inspect_account_delegated, state_clear_aware_is_empty, the is_empty && has_transfer charge, the new_account_storage_gas lookup, the halt/abort branches, and run_inner_instruction_or_abort! — is line-for-line the same. If the shared logic ever changes (e.g. stack-inspect ordering, halt-branch additions), we'd have to remember to update both sites.
Suggestion: parameterize the macro with an optional storage-address selector that defaults to to. Then CALLCODE becomes a normal wrap_call_with_storage_gas! invocation:
macro_rules! wrap_call_with_storage_gas {
// Default: storage address is the stack `to`.
($fn_name:ident, $opcode_name:expr, $wrapped_fn:path, $has_transfer_logic:expr) => {
wrap_call_with_storage_gas!(
$fn_name, $opcode_name, $wrapped_fn, $has_transfer_logic, to
);
};
// Custom storage-address selector: `to`, `mega_spec`, `context` are in scope.
($fn_name:ident, $opcode_name:expr, $wrapped_fn:path, $has_transfer_logic:expr, $storage_addr:expr) => {
#[doc = concat!("`", $opcode_name, "` opcode implementation modified from `revm` with compute gas tracking and dynamically-scaled storage gas costs.")]
pub fn $fn_name<
WIRE: InterpreterTypes<Stack: StackInspectTr>,
H: HostExt + ContextTr + JournalInspectTr + ?Sized,
>(
context: InstructionContext<'_, H, WIRE>,
) {
let spec = context.interpreter.runtime_flag.spec_id();
let Some(to) = context.interpreter.stack.inspect::<1>() else {
context.interpreter.halt(InstructionResult::StackUnderflow);
return;
};
let to = to.into_address();
let mega_spec = context.host.spec_id();
let storage_address: Address = $storage_addr;
let Ok(storage_account) =
context.host.inspect_account_delegated(mega_spec, storage_address)
else {
context.interpreter.halt(InstructionResult::FatalExternalError);
return;
};
let is_empty = storage_account.state_clear_aware_is_empty(spec);
let has_transfer = if $has_transfer_logic {
let Some(value) = context.interpreter.stack.inspect::<2>() else {
context.interpreter.halt(InstructionResult::StackUnderflow);
return;
};
!value.is_zero()
} else {
false
};
if is_empty && has_transfer {
let Some(new_account_storage_gas) =
context.host.new_account_storage_gas(storage_address)
else {
context.interpreter.halt(InstructionResult::FatalExternalError);
return;
};
gas!(context.interpreter, new_account_storage_gas);
}
run_inner_instruction_or_abort!($wrapped_fn, context);
}
};
}
wrap_call_with_storage_gas!(call, "CALL", compute_gas_ext::call, true);
wrap_call_with_storage_gas!(delegate_call, "DELEGATECALL", compute_gas_ext::delegate_call, false);
wrap_call_with_storage_gas!(static_call, "STATICCALL", compute_gas_ext::static_call, false);
wrap_call_with_storage_gas!(
call_code, "CALLCODE", compute_gas_ext::call_code, true,
if mega_spec.is_enabled(MegaSpecId::REX5) {
context.interpreter.input.target_address()
} else {
to
}
);Trade-off: the macro grows one knob (and a touch more macro-magic), but ~60 lines of duplicated logic go away and CALL/CALLCODE/DELEGATECALL/STATICCALL stay in lockstep on the shared parts. The CALLCODE-specific rationale (why the storage address differs) can stay as a comment at the invocation site.
Not blocking — happy to leave as-is if you prefer the explicit form.
|
🔧 Pushed CI fixes. Agent log |
Autosaved by mega-engineer so the next run can resume. Generated-by: engineer-agent
Generated-by: engineer-agent
|
The Codecov report is a success notification — all modified lines are covered and project coverage is 91.4%, above the threshold. No action required. |
|
The open CHANGES_REQUESTED from @Troublor (macro refactoring) has not been addressed in the latest push — the TroublorBot CI-fix push has an empty agent log, so nothing substantive changed. Either implement the refactor or resolve the review so the PR can progress. One gap in the error-path coverage worth closing: the two Everything else — the fix semantics, backward-compat gating, and doc update — was covered in the prior review. |
|
❌ I was unable to process this feedback. A human should take a look. |
|
🔍 Looking into this... |
|
The macro refactoring requested by @Troublor (parameterising One gap remains unaddressed from the prior review: the Under Rex5, Note: the Everything else looks good — fix is semantically correct, backward-compat gating is right, and the four functional regression tests are solid. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// When the SALT environment fails during `new_account_storage_gas` inside CALLCODE | ||
| /// (in `storage_gas_ext::call_code`), the EVM should halt with `FatalExternalError`. | ||
| /// This path is reached under Rex4 when the code-source is empty and value is non-zero. | ||
| #[test] |
There was a problem hiding this comment.
The inspect_account_delegated failure path under Rex5 isn't covered. Under Rex5, storage_address = CALLEE; a DB error on that lookup triggers FatalExternalError — the same path this test exercises under Rex4 for EMPTY_TARGET.
| #[test] | |
| } | |
| /// When `inspect_account_delegated` fails during CALLCODE under Rex5, the EVM should | |
| /// halt with `FatalExternalError`. Under Rex5, `storage_address = target_address() = CALLEE`. | |
| #[test] | |
| fn test_rex5_callcode_db_error_on_inspect_account() { | |
| let bytecode = callcode_bytecode(EMPTY_TARGET); | |
| let inner_db = MemoryDatabase::default() | |
| .account_balance(CALLER, U256::from(1_000_000_000_000u64)) | |
| .account_balance(CALLEE, U256::from(1_000_000_000u64)) | |
| .account_code(CALLEE, bytecode); | |
| let mut db = ErrorInjectingDatabase::new(inner_db); | |
| db.fail_on_account = Some(CALLEE); | |
| let result = transact_with_error_db(MegaSpecId::REX5, db, CALLER, CALLEE, 1_000_000); | |
| match result { | |
| Err(EVMError::Custom(msg)) => { | |
| assert!( | |
| msg.contains("injected basic()"), | |
| "error message should contain injected error, got: {msg}" | |
| ); | |
| } | |
| Err(other) => panic!("expected EVMError::Custom, got: {other:?}"), | |
| Ok(result) => panic!("expected error, got success: {:?}", result.result), | |
| } |
|
🔍 Looking into this... |
|
🔍 Looking into this... |
|
🔍 Looking into this... |
|
The macro refactoring requested by @Troublor has been implemented — One doc bug in the new tests: the comment on Everything else — the fix semantics, backward-compat gating, doc update, and the four functional regression tests — remains solid. LGTM modulo the comment cleanup. |
|
|
||
| /// When the SALT environment fails during `new_account_storage_gas` inside CALLCODE | ||
| /// (in `storage_gas_ext::call_code`), the EVM should halt with `FatalExternalError`. | ||
| /// This path is reached under Rex4 when the code-source is empty and value is non-zero. | ||
| /// When `inspect_account_delegated` fails during CALLCODE under Rex5, the EVM should halt with | ||
| /// `FatalExternalError` and return `EVMError::Custom`. Under Rex5, the storage address is the |
There was a problem hiding this comment.
This doc comment has two issues:
- The first three lines describe the SALT-error path (
test_callcode_salt_error_on_new_account_storage_gas), not this test — they appear to have been accidentally merged into this block. - Line 4–5 say the function returns
EVMError::Custom, but the test assertsEVMError::Database. The assertion is correct:db.fail_on_account = Some(CALLEE)fires at transaction setup (loading CALLEE's bytecode before any instruction runs), so the error surfaces asEVMError::Databaseat the outer level, not through theFatalExternalError→EVMError::Custompath insidecall_code.
The comment should be rewritten to accurately describe what this test exercises.
|
🔍 Looking into this... |
|
LGTM. All prior review feedback has been addressed:
Fix is semantically sound (CALLCODE executes in the caller's storage context, so new-account metering against the caller is the right address), backward-compat gating is correct, and test coverage is thorough. |
|
The |
|
Added the missing bullet to the REX5 section of |
…k-spec summary Generated-by: engineer-agent
|
The Codecov report is a success notification — all modified lines are covered and project coverage is 91.5%, above the threshold. No action required. |
|
LGTM. All prior review feedback has been addressed:
@Troublor — the refactoring you requested has been implemented; please dismiss your CHANGES_REQUESTED so this can merge. |
|
Both comments are informational — the Codecov report confirms 100% patch coverage at 91.5% project coverage, and the claude bot summary confirms all review feedback has been addressed. No action required. |
|
/benchmark |
Criterion Benchmark Comparison147 benchmarks total, 5 with >5% change
Significant changes (>5%):
147 benchmarks: 0 regressions, 0 warnings, 0 improvements |
|
The re-run of run |
5 participants
Summary
Fixes a bug where
CALLCODEwas charging new-account storage gas based on the code-source address instead of the caller's storage context. ForCALLCODE, execution happens in the caller's account, so the emptiness check must be against the caller, not the code-source. Rex5 gates the fix by usinginterpreter.input.target_address()for new-account metering, while pre-Rex5 specs preserve frozen behavior for backward compatibility. Includes 4 regression tests covering both the fix and frozen behavior forCALLCODEandCALL.Fixes #283