fix: pre-audit security hardening and bug fixes#48
Merged
Conversation
Fixes from internal security audit before external audit engagement: - consensus: eliminate AtomicU64 call-order coupling in L1 message validation, split into independent stateless checks (validate_l1_messages_in_block + header monotonicity check) - consensus: add timestamp validation for pre-Emerald (strict >) vs post-Emerald (>=) - revm/handler: add disable_fee_charge check in token fee path for eth_call simulation correctness - revm/handler: handle missing cached_token_fee_info in reimburse_caller when fee charge was disabled - revm/handler: use checked_sub in transfer_erc20_with_slot for from balance - revm/l1block: cap L1 data fee to u64::MAX for circuit compatibility - engine-api: fix ValidateL2Block error semantics (internal vs invalid) - engine-api: fix FeeVault coinbase override - rpc: use TxMorph::validate() instead of validate_version() for complete validation at RPC layer - primitives: minor TxMorph validation improvements - txpool: improve error types for MorphTx validation
- consensus: use checked_add for queue_index overflow protection (C-H1) - handler: add ETH value balance check in token fee path (X-6) - handler: checked_add for token recipient balance overflow (R-2) - handler: propagate error from effective_balance_spending (R-3) - handler: skip self-transfer post-check when from == to (GPT-3) - txpool: add TxMorph::validate() in shared validation path (GPT-4) - txpool: add InvalidFormat error variant for structural violations
Precompile divergences with go-eth (H-1 ~ H-4): - Add modexp 32-byte input limit for Bernoulli~Viridian hardforks - Add BN256 pairing 4-pair limit from Morph203 onwards - Rewrite emerald() to explicitly list precompiles, excluding 0x0a (KZG) - Upgrade emerald modexp to eip7823+eip7883 (removing 32-byte limit) Balance check fix (M-1): - Use max_fee_per_gas for balance validation, effective_gas_price for deduction, matching go-eth's buyGas() semantics Token transfer safety (M-5): - Replace saturating_sub with checked_sub in ERC20 balance verification to catch underflow instead of silently returning zero Transaction pool (D-1): - Add disable_balance_check() to MorphPoolBuilder so MorphTx with fee_token_id > 0 can pass inner EthTransactionValidator Compact decode documentation (M-2): - Document panic! in MorphTxType::from_compact as intentional for DB corruption (Compact trait is infallible by reth convention) Defensive error handling (L-2): - Replace .expect() with .ok_or() in L1 message queue index validation Documentation (L-5): - Improve take_revert_logs() comment explaining the workaround context
…efund) - Add block payload size validation in consensus (M-1), matching go-eth IsValidBlockSize(block.PayloadSize()) - Add MorphTx version validation in consensus (M-2), matching go-eth ValidateMorphTxVersion() with Jade fork gating for V1 - Fix token fee refund to silently continue on failure, matching go-eth "refund should not cause transaction to fail" semantics - Add defensive .max(0) for gas.refunded() i64→u64 cast (M-4)
- Remove from==to skip in fee token transfer validation to match go-ethereum's transferAltTokenByEVM (self-transfers should fail) - Use remove_transactions_and_descendants in txpool maintain to clean up nonce-dependent orphans immediately - Remove unnecessary RECEIPT_CAPACITY_HINT pre-allocation
Set --engine.persistence-threshold 256 and --engine.memory-block-buffer-target 16 to batch MDBX writes and avoid competing with Tendermint's LevelDB fsyncs.
…ceipt ERC20 Transfer event logs were lost during token fee deduction/reimbursement because revm's system_call pipeline drains logs from the journal via execution_result() → take_logs(). This caused receipt root mismatch at blocks with MorphTx (0x7F) using ERC20 token gas payment. Replace system_call_one_with_caller with evm_call() that only runs the handler's execution() phase, keeping logs in the journal naturally — matching go-ethereum's evm.Call() semantics. Also remove dead code: evm.logs field, take_logs(), take_revert_logs().
go-ethereum's StateDB.logs is independent of the state snapshot/revert mechanism, so Transfer events from buyAltTokenGas() and refundAltTokenGas() always appear in receipts regardless of whether the main tx reverts. revm's ExecutionResult::Revert has no logs field — the handler pipeline's take_logs() discards all logs on revert. Previously, fee Transfer logs were kept in the journal and restored via a saved_logs save/restore hack, which only worked for the success path. This commit caches fee Transfer logs in MorphEvm (pre_fee_logs for deduction, post_fee_logs for reimbursement) separately from the journal. The receipt builder merges them in chronological order: [deduct Transfer] + [main tx logs] + [refund Transfer] This handles both success and revert uniformly without overriding execution_result, and removes the saved_logs save/restore workaround.
…semantic go-ethereum uses evm.StaticCall() for balanceOf queries which cannot emit events. Our evm_call() runs a regular CALL, so a non-standard token could emit logs that leak into pre_fee_logs or post_fee_logs. Truncate journal logs after evm_call_balance_of to match the read-only semantic. Also fix a misplaced doc comment on EVM_CALL_GAS_LIMIT.
- Pre-allocate receipt log Vec with exact capacity instead of growing through repeated extend calls - Use direct assignment for post_fee_logs instead of extend (the Vec is always empty at that point)
Replace inline version/field validation in validate_morph_txs() with a call to TxMorph::validate(), which already implements the same checks (version constraints, memo length, fee_limit rules, gas price ordering). Only the hardfork-gated V1 rejection remains in the consensus layer, as it requires chain_spec context unavailable in the primitive layer.
The manual impl Compact for TxMorph was fundamentally broken: - to_compact wrote variable-length fields without flags bytes - from_compact passed total buffer length to each field's decoder, causing u64::from_compact(buf, huge_len) to panic on arr[8-len] This made stage unwind and any static file read of MorphTx crash. Fix: follow reth's standard pattern (same as TxEip1559 in reth-codecs): - TxMorphCompact helper struct with #[derive(Compact)] for automatic bitfield/flags generation - u8/u16 fields stored as u64 (reth_codecs doesn't support u8/u16) - memo + input packed into single Bytes field (derive allows only one, must be last): [memo_len: u8][memo_bytes][input_bytes] Note: new encoding is incompatible with old broken data in static files. Existing reth databases with MorphTx must be rebuilt.
Adapted from local-test (mainnet) with Hoodi-specific config: - chain: hoodi (chain ID 2910) - L1 RPC: ethereum-hoodi-rpc.publicnode.com - deposit contract: 0xd7f39d837f... - rollup contract: 0x57e0e6dde8... - config bundle from morph-l2/run-morph-node hoodi/
The L1 Gas Price Oracle can be updated by a regular L2 transaction (from the external gas-oracle service) within the same block. Caching L1BlockInfo once per block in apply_pre_execution_changes caused subsequent user txs to use stale fee parameters, leading to "lack of funds" errors (observed at mainnet block 152685). Restore per-tx L1BlockInfo::try_fetch() in the handler's validate_and_deduct_eth_fee / validate_and_deduct_token_fee, matching go-ethereum's per-tx read behavior.
The validate_l1_messages_in_block header consistency check used strict equality (header.next_l1_msg_index == last_queue_index + 1), which rejected blocks where the sequencer skipped L1 messages. Morph allows L1 messages to be skipped (not included in block body but counted as processed), as documented by go-eth's NumL1MessagesProcessed(): "This count includes both skipped and included messages." Block 628697 (mainnet) triggered this with queue_index 2572, 2573 and header next_l1_msg_index=2575, meaning queue_index 2574 was skipped. Change the check from != to < so header.next_l1_msg_index must be at least last_queue_index + 1 but may be greater when messages are skipped.
- Fix base fee doc: "after Curie" → "always active" (matches code) - Fix receipt doc: token_fee_info → morph_tx_fields, add missing fields - Fix get_morph_tx_fields doc: add missing sender parameter - Fix transfer ABI: transfer(address,amount) → transfer(address,uint256) - Fix checked_add comment: clarify difference from go-eth unbounded Add - Fix reimburse comment: remove misleading "system transactions" mention - Fix L1 message validation doc: equality → >= (skipped messages) - Fix evm.rs typo: "Consumed" → "Consumes" - Remove ~12 redundant "restates the code" comments in handler.rs
The ForkchoiceState sent to reth's engine tree always had finalized_block_hash = B256::ZERO. Because reth's changeset-cache eviction, sidechain pruning, and block-buffer cleanup all depend on a non-zero finalized hash, in-memory state grew without bound during historical sync, eventually causing OOM on 32 GiB servers. Morph uses Tendermint instant finality so every committed block is final. During historical sync — before BlockTagService can provide L1-based values (all batches already finalized, state roots may be deleted) — FCU now falls back to head as finalized, enabling memory cleanup. Once BlockTagService starts providing L1 finalized hashes (via set_block_tags), FCU forwards those instead, keeping RPC-visible tags consistent with actual L1 finalization status. EngineStateTracker is extended with a BlockTagCache so set_block_tags values survive across FCU calls without race conditions.
…anup Validator / derivation nodes do not run BlockTagService, so set_block_tags is never called externally. With the new resolve_fcu_block_tag_hash fallback, once a derived block's timestamp is within 60 s of wall-clock time the FCU would send finalized_block_hash = B256::ZERO indefinitely, stopping changeset-cache eviction and causing a slow memory leak. Pass block_hash as the finalized hint in set_block_tags so the EngineStateTracker cache is seeded and the FCU always finds a non-zero fallback. When validators adopt BlockTagService the L1-derived finalized value will naturally supersede this hint.
Replace logs.truncate with checkpoint_revert in evm_call_balance_of and transfer_erc20_with_evm to properly isolate side effects (storage writes, logs, account touches) from helper EVM calls, matching go-ethereum's StaticCall semantics. Remove unused track_forced_cold_slot_from_state.
…geth_disk_root Remove custom should_compute_state_root to use the default trait behavior. Make fetch_geth_disk_root public for use by the state-root-check CLI tool.
Standalone binary that opens reth DB read-only and computes the MPT state root at a given block, with optional geth morph_diskRoot cross-validation and bisect mode to find the first divergent block.
- Remove RETH_LEGACY_STATE_ROOT env var and --engine.legacy-state-root from both mainnet and hoodi reth-start scripts (always use parallel). - Fix MORPH_GETH_RPC_URL default assignment: use := -> = so that an explicitly empty value is preserved instead of being overridden.
bench-sync.sh: automated Geth vs Reth sync speed comparison with BPS sampling and ETA calculation. bench-sync-multi.sh: multi-round wrapper with mean/min/max aggregation.
Remove --morph.geth-rpc-url CLI arg and all per-block geth diskRoot cross-validation during sync. This significantly improves sync speed by eliminating HTTP round-trips on every block. The standalone state-root-check tool (bin/state-root-check) is preserved for offline verification when needed.
Add check-state-root.sh for both mainnet and hoodi that builds state-root-check and supports three modes: local-only, compare with geth, and bisect to find first mismatch.
The block executor's set_state_hook was a no-op, silently discarding the hook set by the engine tree's StateRootTask. This meant the sparse trie never received per-transaction state updates, producing incorrect trie data in AccountsTrie/StoragesTrie tables while PlainAccountState remained correct. Since Morph L2 drives every block through the engine API (no pipeline sync), the StateRootTask is active for all blocks. Without the hook, state root mismatches appear starting at the first block containing a transaction (block 98894 on Hoodi). Changes: - Add state_hook field to MorphBlockExecutor - Store the hook in set_state_hook instead of discarding it - Call hook.on_state(StateChangeSource::Transaction(idx), &state) before db.commit(state) in commit_transaction, matching upstream EthBlockExecutor behavior
After rebasing onto main (which uses reth rev 1dd7227 with revm 33.1.0), the custom SLOAD/SSTORE opcode overrides and MorphTxRuntime tracking are no longer needed. revm 33.1.0 does not have the mark_warm_with_transaction_id behavior that caused gas mismatches, so the workarounds can be removed. - Remove runtime.rs (MorphTxRuntime, forced_cold_slot_originals tracking) - Remove track_forced_cold_slot / restore_tracked_original_values calls - Change MorphContext chain type from MorphTxRuntime to L1BlockInfo - Fix caller.account().info -> caller.info for revm 33.1.0 API - Remove unused ReceiptRootBloom and BlockTy imports - Remove tests that tested forced-cold-slot behavior
Remove bin/state-root-check CLI, local-test-hoodi directory, benchmark scripts, and geth RPC cross-validation code from validator.rs. Restore local-test scripts to main branch versions. These are test/debug utilities not needed in the core pre-audit changes.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLarge functional changes across consensus, EVM/revm, txpool, engine-api, primitives, and node: L1/L2 fee and L1-message validation rework, per-tx L1 fee/token caching and separated fee logs, block-tag caching in engine-api, MorphTx type/serialization and gas_limit narrowing, and txpool/provider integration updates. Changes
Sequence Diagram(s)sequenceDiagram
participant RPC as RPC / Client
participant Node as Node (provider / txpool)
participant Engine as EngineStateTracker / Forkchoice
participant EVM as MorphEvm / REVM handler
participant DB as Database
RPC->>Node: submit tx / RPC morph_tx
Node->>Node: construct MorphPooledTransaction, validate (morph_tx.validate)
Node->>Engine: forkchoice update / record_block_tags
Engine->>Engine: resolve_fcu_block_tag_hash (use cached L1 tag or fallback)
Node->>EVM: execute block / commit transaction (use cached_l1_data_fee, token fee info)
EVM->>EVM: evm_call / transfer_erc20_with_evm or slot transfer
EVM->>DB: journal checkpoint & commit state root
EVM-->>Node: return receipt (pre_fee_logs + logs + post_fee_logs)
Node-->>RPC: return result / receipt
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/revm/src/evm.rs (1)
165-173:⚠️ Potential issue | 🟠 MajorPreserve the new fee caches when rebuilding
MorphEvm.
MorphEvm::new_inner()reinitializescached_token_fee_info,cached_l1_data_fee,pre_fee_logs, andpost_fee_logs. After this change,with_inspector()/with_precompiles()no longer behave like pure wrapper transforms—they drop per-transaction fee state and can produce incomplete refunds or receipts if they're called after fee deduction starts.Suggested fix
pub fn with_inspector<OINSP>(self, inspector: OINSP) -> MorphEvm<DB, OINSP> { - MorphEvm::new_inner(self.inner.with_inspector(inspector)) + let Self { + inner, + cached_token_fee_info, + cached_l1_data_fee, + pre_fee_logs, + post_fee_logs, + } = self; + + MorphEvm { + inner: inner.with_inspector(inspector), + cached_token_fee_info, + cached_l1_data_fee, + pre_fee_logs, + post_fee_logs, + } } /// Consumes self and returns a new Evm type with given Precompiles. pub fn with_precompiles(self, precompiles: MorphPrecompiles) -> Self { - Self::new_inner(self.inner.with_precompiles(precompiles)) + let Self { + inner, + cached_token_fee_info, + cached_l1_data_fee, + pre_fee_logs, + post_fee_logs, + } = self; + + Self { + inner: inner.with_precompiles(precompiles), + cached_token_fee_info, + cached_l1_data_fee, + pre_fee_logs, + post_fee_logs, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/revm/src/evm.rs` around lines 165 - 173, with_inspector and with_precompiles currently call MorphEvm::new_inner which resets per-transaction fee state (cached_token_fee_info, cached_l1_data_fee, pre_fee_logs, post_fee_logs), causing lost fee caches; change these methods to preserve and carry over the existing fee-related fields when constructing the new MorphEvm (either by adding a new constructor/new_inner overload that accepts the existing cached_token_fee_info, cached_l1_data_fee, pre_fee_logs, post_fee_logs or by copying those fields from self.inner into the returned MorphEvm), ensuring with_inspector and with_precompiles behave as pure wrapper transforms and do not drop in-flight fee state.crates/revm/src/handler.rs (1)
112-117:⚠️ Potential issue | 🔴 CriticalReject missing MorphTx fee fields instead of defaulting them to zero.
unwrap_or_default()makes a malformed0x7ftransaction execute as ETH-fee (fee_token_id == 0) or with an implicit full-balancefee_limit. Elsewhere in this PR,crates/evm/src/block/mod.rs::get_morph_tx_fieldstreats those fields as mandatory, so block execution can diverge from the validator and receipt path. Reject missing fee fields here before branching on token-vs-ETH fees.Also applies to: 497-503
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/revm/src/handler.rs` around lines 112 - 117, The code currently uses unwrap_or_default() on MorphTx fee fields (e.g., evm.ctx_ref().tx().fee_token_id) which masks missing/invalid fields and causes malformed 0x7f transactions to be treated as ETH-fee; replace the unwrap_or_default() usage with an explicit presence check of the Option fields and reject the transaction when fee_token_id (and other required MorphTx fee fields like fee_limit if present) are None instead of defaulting to zero—do this before branching to validate_and_deduct_token_fee or validate_and_deduct_eth_fee so missing fields return the same validation error path used elsewhere; apply the same fix to the other similar block that also uses unwrap_or_default() for MorphTx fee fields.
🧹 Nitpick comments (3)
crates/txpool/src/maintain.rs (1)
84-88: Consider logging whenstate_token_balanceisNonefor the first token transaction.When
state_token_balanceisNoneon the first occurrence of atoken_id, the budget defaults toU256::ZERO, causing immediate rejection. While this is a safe fail-closed behavior, it could mask upstream issues wherevalidation.token_infowas unexpectedlyNonedespiteuses_token_feebeingtrue.Consider adding a debug/trace log when this fallback occurs to aid debugging:
let token_budget = budget .token_balances .entry(token_id) - .or_insert(state_token_balance.unwrap_or(U256::ZERO)); + .or_insert_with(|| { + state_token_balance.unwrap_or_else(|| { + tracing::debug!( + target: "morph::txpool::maintain", + token_id, + "Token balance unknown, defaulting to zero" + ); + U256::ZERO + }) + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/txpool/src/maintain.rs` around lines 84 - 88, When inserting the initial token balance fallback in maintain.rs (the call to budget.token_balances.entry(token_id).or_insert(state_token_balance.unwrap_or(U256::ZERO))), add a debug/trace log when state_token_balance is None so we record the unexpected fallback to U256::ZERO; locate the use of state_token_balance and token_id in the same block (around token_budget assignment) and before calling or_insert emit a log via the crate logger (e.g., debug! or trace!) that includes token_id and context (e.g., "state_token_balance missing for token_id, defaulting to ZERO") to help debug cases where validation.token_info was None despite uses_token_fee being true.crates/engine-api/src/builder.rs (1)
958-984: Good test coverage for the three main paths.The tests correctly verify:
- L1 tag takes precedence when available
- Historical blocks (300s > 60s threshold) fall back to head
- Near-live blocks (5s < 60s threshold) return ZERO
Consider adding a boundary test for exactly 60 seconds if you want exhaustive edge-case coverage, though the current tests are sufficient for the main behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/engine-api/src/builder.rs` around lines 958 - 984, Add a boundary test to assert behavior when block age equals the 60-second threshold: create a test (e.g., test_resolve_fcu_block_tag_hash_boundary_at_sixty_seconds) that calls resolve_fcu_block_tag_hash with None for l1_tag, a head value (use B256::from([...]) as in other tests), head_time and block_time such that block_time == head_time + 60, and assert the function returns the expected value for that exact boundary (either head or B256::ZERO depending on the intended policy); place this new test alongside the existing tests referencing resolve_fcu_block_tag_hash so the edge case is covered.crates/rpc/src/eth/transaction.rs (1)
181-190: Consider validating MorphTx-specific constraints in TryIntoTxEnv.The
TryIntoTxEnvimplementation detects MorphTx based on heuristics (lines 182-184) and sets the transaction type, but unliketry_build_morph_tx_from_request, it doesn't validate that the combination of fields is valid for a MorphTx. For consistency, consider adding validation whenis_morph_txis true to catch invalid configurations at this entry point as well.💡 Potential enhancement
if is_morph_tx { tx_env.inner.tx_type = morph_primitives::MORPH_TX_TYPE_ID; tx_env.version = Some(morph_primitives::transaction::morph_transaction::MORPH_TX_VERSION_1); + // Consider validating MorphTx constraints here for consistency + // with try_build_morph_tx_from_request validation }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rpc/src/eth/transaction.rs` around lines 181 - 190, The TryIntoTxEnv path currently detects MorphTx via the is_morph_tx heuristic (fee_token_id, reference, memo) and sets tx_env.inner.tx_type/version but doesn't enforce the same Morph-specific field constraints as try_build_morph_tx_from_request; update TryIntoTxEnv to invoke the same validation logic (or replicate the checks) used by try_build_morph_tx_from_request to verify allowed/required field combinations for Morph transactions (e.g., valid fee_token_id range, mutually-compatible/forbidden fields, non-empty memo rules, reference validity) and return a clear error when the configuration is invalid so invalid MorphTx configurations are rejected consistently at this entry point.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/revm/src/handler.rs`:
- Around line 674-686: The sender's sstore happens before the recipient overflow
check, so failures leave the sender mutated; to make the transfer atomic, first
compute to_storage_slot (compute_mapping_slot_for_address), sload the recipient
balance and perform checked_add to produce new_to_balance (and return the
MorphInvalidTransaction::TokenTransferFailed on overflow) without performing any
sstore, then after all checks succeed perform the two journal.sstore calls (for
the sender and for the recipient) so both writes happen only after validation;
update the block containing journal.sstore(token, from_storage_slot,
new_balance) and the subsequent sload/checked_add so the write is moved to after
the checked_add.
In `@crates/txpool/src/morph_tx_validation.rs`:
- Around line 57-68: Add a hardfork gate before calling morph_tx.validate():
inspect input.hardfork and the morph_tx's version/type (the MorphTx obtained
from matching MorphTxEnvelope::Morph) and if the transaction is a V1 MorphTx but
input.hardfork is pre-Jade, return Err(MorphTxError::InvalidFormat { reason:
"<clear message>" .to_string() }); then proceed to call morph_tx.validate() as
before; reference the symbols morph_tx, input.hardfork, MorphTxEnvelope::Morph
and MorphTxError::InvalidFormat to locate where to insert this check.
---
Outside diff comments:
In `@crates/revm/src/evm.rs`:
- Around line 165-173: with_inspector and with_precompiles currently call
MorphEvm::new_inner which resets per-transaction fee state
(cached_token_fee_info, cached_l1_data_fee, pre_fee_logs, post_fee_logs),
causing lost fee caches; change these methods to preserve and carry over the
existing fee-related fields when constructing the new MorphEvm (either by adding
a new constructor/new_inner overload that accepts the existing
cached_token_fee_info, cached_l1_data_fee, pre_fee_logs, post_fee_logs or by
copying those fields from self.inner into the returned MorphEvm), ensuring
with_inspector and with_precompiles behave as pure wrapper transforms and do not
drop in-flight fee state.
In `@crates/revm/src/handler.rs`:
- Around line 112-117: The code currently uses unwrap_or_default() on MorphTx
fee fields (e.g., evm.ctx_ref().tx().fee_token_id) which masks missing/invalid
fields and causes malformed 0x7f transactions to be treated as ETH-fee; replace
the unwrap_or_default() usage with an explicit presence check of the Option
fields and reject the transaction when fee_token_id (and other required MorphTx
fee fields like fee_limit if present) are None instead of defaulting to zero—do
this before branching to validate_and_deduct_token_fee or
validate_and_deduct_eth_fee so missing fields return the same validation error
path used elsewhere; apply the same fix to the other similar block that also
uses unwrap_or_default() for MorphTx fee fields.
---
Nitpick comments:
In `@crates/engine-api/src/builder.rs`:
- Around line 958-984: Add a boundary test to assert behavior when block age
equals the 60-second threshold: create a test (e.g.,
test_resolve_fcu_block_tag_hash_boundary_at_sixty_seconds) that calls
resolve_fcu_block_tag_hash with None for l1_tag, a head value (use
B256::from([...]) as in other tests), head_time and block_time such that
block_time == head_time + 60, and assert the function returns the expected value
for that exact boundary (either head or B256::ZERO depending on the intended
policy); place this new test alongside the existing tests referencing
resolve_fcu_block_tag_hash so the edge case is covered.
In `@crates/rpc/src/eth/transaction.rs`:
- Around line 181-190: The TryIntoTxEnv path currently detects MorphTx via the
is_morph_tx heuristic (fee_token_id, reference, memo) and sets
tx_env.inner.tx_type/version but doesn't enforce the same Morph-specific field
constraints as try_build_morph_tx_from_request; update TryIntoTxEnv to invoke
the same validation logic (or replicate the checks) used by
try_build_morph_tx_from_request to verify allowed/required field combinations
for Morph transactions (e.g., valid fee_token_id range,
mutually-compatible/forbidden fields, non-empty memo rules, reference validity)
and return a clear error when the configuration is invalid so invalid MorphTx
configurations are rejected consistently at this entry point.
In `@crates/txpool/src/maintain.rs`:
- Around line 84-88: When inserting the initial token balance fallback in
maintain.rs (the call to
budget.token_balances.entry(token_id).or_insert(state_token_balance.unwrap_or(U256::ZERO))),
add a debug/trace log when state_token_balance is None so we record the
unexpected fallback to U256::ZERO; locate the use of state_token_balance and
token_id in the same block (around token_budget assignment) and before calling
or_insert emit a log via the crate logger (e.g., debug! or trace!) that includes
token_id and context (e.g., "state_token_balance missing for token_id,
defaulting to ZERO") to help debug cases where validation.token_info was None
despite uses_token_fee being true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef1b04ad-ba2b-4f53-a9ef-34efd7155f91
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
bin/morph-reth/src/main.rscrates/consensus/src/validation.rscrates/engine-api/src/builder.rscrates/engine-api/src/rpc.rscrates/evm/src/block/mod.rscrates/evm/src/block/receipt.rscrates/evm/src/evm.rscrates/node/Cargo.tomlcrates/node/src/components/pool.rscrates/node/src/node.rscrates/node/src/validator.rscrates/payload/builder/src/builder.rscrates/payload/builder/src/error.rscrates/primitives/src/receipt/envelope.rscrates/primitives/src/transaction/envelope.rscrates/primitives/src/transaction/morph_transaction.rscrates/revm/Cargo.tomlcrates/revm/src/evm.rscrates/revm/src/handler.rscrates/revm/src/l1block.rscrates/revm/src/precompiles.rscrates/revm/src/token_fee.rscrates/revm/src/tx.rscrates/rpc/src/eth/transaction.rscrates/txpool/src/error.rscrates/txpool/src/maintain.rscrates/txpool/src/morph_tx_validation.rscrates/txpool/src/validator.rs
💤 Files with no reviewable changes (3)
- crates/node/Cargo.toml
- crates/payload/builder/src/error.rs
- crates/revm/Cargo.toml
…or token fees - Wrap transfer_erc20_with_slot in checkpoint/revert for atomicity - Add V1 MorphTx pre-Jade rejection in txpool validator - Use effective_gas_price instead of max_fee_per_gas for token fee calculation - Thread base_fee_per_gas through MorphL1BlockInfo and txpool validation - Fix gas limit validation doc comment to match >= semantics
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/txpool/src/maintain.rs (1)
71-105: Potential issue: fee_limit capping logic may differ from REVM semantics.The
consume_token_budgetfunction capseffective_limitwhenfee_limit > *token_budget(line 92-96). However, this differs from how the shared validation inmorph_tx_validation.rshandles it at lines 143-147:
- Shared validation:
fee_limit > token_info.balance(state balance)- Maintenance:
fee_limit > *token_budget(rolling budget)This is likely intentional for cumulative budget tracking, but the comment at line 89-91 references "REVM semantics" which may be misleading since REVM uses fresh state balance, not a rolling budget.
Consider clarifying the comment to indicate this is cumulative budget validation, not direct REVM matching.
📝 Suggested comment clarification
- // Match REVM semantics with rolling sender budget: + // Cumulative budget validation (inspired by REVM's fee_limit semantics): // - fee_limit == 0 => use remaining token budget // - fee_limit > remaining => cap by remaining token budget🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/txpool/src/maintain.rs` around lines 71 - 105, The comment in consume_token_budget incorrectly implies full parity with REVM semantics; update the comment above the effective_limit calculation to explicitly state that this logic implements a cumulative/rolling sender budget using token_budget (the entry in budget.token_balances) — i.e., fee_limit is compared against the remaining rolling token_budget rather than the fresh state balance used by the shared validation in morph_tx_validation (which compares fee_limit to token_info.balance); mention the variables effective_limit, fee_limit and token_budget and clarify this intentional difference so the comment no longer suggests exact REVM matching.crates/txpool/src/morph_tx_validation.rs (1)
86-92: Clarify the intent: effective gas price for token path only.Line 92 calculates
effective_gas_pricebut it's only used in the token-fee path (lines 136-137). The ETH-fee path at line 93-95 still usesmax_fee_per_gasforgas_fee. This asymmetry appears intentional — the token-fee path uses effective price for actual cost estimation while the ETH-fee path uses max for worst-case balance validation.Consider adding a brief comment to clarify this design choice for future readers.
📝 Suggested comment
// Shared fee components used by both ETH-fee and token-fee branches. +// Note: ETH-fee path uses max_fee_per_gas for worst-case balance validation, +// while token-fee path uses effective_gas_price for actual cost estimation. let gas_limit = U256::from(morph_tx.gas_limit);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/txpool/src/morph_tx_validation.rs` around lines 86 - 92, Add a brief clarifying comment near the shared fee component calculations (around the let effective_gas_price = U256::from(...)) that explains effective_gas_price is computed for use only by the token-fee branch (used to estimate actual token cost) while the ETH-fee branch deliberately uses max_fee_per_gas to compute gas_fee as a worst-case balance check; reference the symbols effective_gas_price, max_fee_per_gas, gas_fee and the morph_tx token/ETH fee branches so future readers understand the asymmetry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/txpool/src/maintain.rs`:
- Around line 71-105: The comment in consume_token_budget incorrectly implies
full parity with REVM semantics; update the comment above the effective_limit
calculation to explicitly state that this logic implements a cumulative/rolling
sender budget using token_budget (the entry in budget.token_balances) — i.e.,
fee_limit is compared against the remaining rolling token_budget rather than the
fresh state balance used by the shared validation in morph_tx_validation (which
compares fee_limit to token_info.balance); mention the variables
effective_limit, fee_limit and token_budget and clarify this intentional
difference so the comment no longer suggests exact REVM matching.
In `@crates/txpool/src/morph_tx_validation.rs`:
- Around line 86-92: Add a brief clarifying comment near the shared fee
component calculations (around the let effective_gas_price = U256::from(...))
that explains effective_gas_price is computed for use only by the token-fee
branch (used to estimate actual token cost) while the ETH-fee branch
deliberately uses max_fee_per_gas to compute gas_fee as a worst-case balance
check; reference the symbols effective_gas_price, max_fee_per_gas, gas_fee and
the morph_tx token/ETH fee branches so future readers understand the asymmetry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e43f7ce-3594-45c6-ac2b-09df07de8dc0
📒 Files selected for processing (5)
crates/consensus/src/validation.rscrates/revm/src/handler.rscrates/txpool/src/maintain.rscrates/txpool/src/morph_tx_validation.rscrates/txpool/src/validator.rs
…t/revert Introduce three reusable helpers that centralize all checkpoint logic: - with_journal_checkpoint: commit on Ok, revert on Err - with_evm_checkpoint: same + saves/restores evm.tx - with_evm_snapshot: always reverts + saves/restores evm.tx Also fixes evm_call_balance_of not restoring evm.tx after evm_call.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/revm/src/handler.rs`:
- Around line 887-905: The code around computing expected_balance (the let
expected_balance = from_balance_before .checked_sub(token_amount) .ok_or(...)?;)
is misformatted for rustfmt; reformat that chained call and surrounding block to
standard Rust formatting (put the .checked_sub(token_amount) call on the same
line as the receiver or align the method chain according to rustfmt), ensure the
ok_or(...) closure and ? are properly indented, and then run `cargo fmt --all`
to normalize formatting for the expected_balance computation in handler.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ddeeb11c-4acf-4439-878f-bb127e99dd50
📒 Files selected for processing (1)
crates/revm/src/handler.rs
- handler.rs: only assign post_fee_logs when refund succeeds, matching go-ethereum which emits no logs on refund failure - evm.rs: fix MorphContext doc -- L1BlockInfo is fetched per-tx, not per-block (per-block cache would miss mid-block oracle updates) - validation.rs: fix L1 message rules doc to reflect that cross-block queue index gaps are allowed (sequencer may skip indices) - validation.rs: clarify withdrawals doc -- field must be absent (None), not merely empty - rpc.rs: use "morph::engine" log target per project convention
The with_evm_checkpoint wrapper already reverts the journal (including any Transfer logs) on failure, so the drain after the call captures an empty slice when transfer_erc20_with_evm returns Err. The is_ok() guard was defensive but unnecessary.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/revm/src/evm.rs (1)
165-172:⚠️ Potential issue | 🟠 MajorPreserve the cached fee state when swapping inspectors/precompiles.
Both helpers rebuild through
new_inner(), which reinitializescached_token_fee_info,cached_l1_data_fee,pre_fee_logs, andpost_fee_logs. If either method is used after fee deduction, reimbursement and receipt assembly will silently lose the per-tx state.🩹 Suggested fix
pub fn with_inspector<OINSP>(self, inspector: OINSP) -> MorphEvm<DB, OINSP> { - MorphEvm::new_inner(self.inner.with_inspector(inspector)) + let Self { + inner, + cached_token_fee_info, + cached_l1_data_fee, + pre_fee_logs, + post_fee_logs, + } = self; + MorphEvm { + inner: inner.with_inspector(inspector), + cached_token_fee_info, + cached_l1_data_fee, + pre_fee_logs, + post_fee_logs, + } } pub fn with_precompiles(self, precompiles: MorphPrecompiles) -> Self { - Self::new_inner(self.inner.with_precompiles(precompiles)) + let Self { + inner, + cached_token_fee_info, + cached_l1_data_fee, + pre_fee_logs, + post_fee_logs, + } = self; + Self { + inner: inner.with_precompiles(precompiles), + cached_token_fee_info, + cached_l1_data_fee, + pre_fee_logs, + post_fee_logs, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/revm/src/evm.rs` around lines 165 - 172, The with_inspector and with_precompiles helpers call MorphEvm::new_inner which resets per-tx cached state (cached_token_fee_info, cached_l1_data_fee, pre_fee_logs, post_fee_logs); modify these helpers to construct the new MorphEvm by reusing self.inner.with_inspector(...) or self.inner.with_precompiles(...) but copy/move the existing cached_token_fee_info, cached_l1_data_fee, pre_fee_logs and post_fee_logs into the returned MorphEvm instead of letting new_inner reinitialize them so the per-transaction fee/reimbursement state is preserved across inspector/precompile swaps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/revm/src/handler.rs`:
- Around line 378-381: The refund calculation currently multiplies
effective_gas_price by reimbursable gas in u128 and then widens to U256, which
can overflow; change the computation in the block that defines refunded and
reimburse_eth so you convert the reimbursable gas amount to U256 first (use
U256::from(gas.remaining().saturating_add(refunded) as u128)) and then perform
the multiplication with effective_gas_price (a U256) using U256's safe multiply
(e.g., saturating_mul or checked_mul as appropriate) so reimburse_eth is
computed entirely in U256; update the variables referenced (refunded,
reimburse_eth, effective_gas_price, gas.remaining(), gas.refunded())
accordingly.
- Around line 810-826: evm_call_balance_of currently swallows all errors and
returns U256::ZERO; change its signature to return Result<U256,
EVMError<DB::Error>> (or the concrete EVM error type used) and propagate any
errors from evm_call and the snapshot via the ? operator instead of mapping to
zero. Update the with_evm_snapshot closure to return Result<U256, _> and
map/propagate evm_call(evm, ...) failures (and any DB/snapshot errors) into the
Result so callers can handle infrastructure failures; then update
transfer_erc20_with_evm to call the new evm_call_balance_of(...)?, comparing
real balances and mapping/returning TokenTransferFailed or the propagated
EVMError where appropriate.
---
Outside diff comments:
In `@crates/revm/src/evm.rs`:
- Around line 165-172: The with_inspector and with_precompiles helpers call
MorphEvm::new_inner which resets per-tx cached state (cached_token_fee_info,
cached_l1_data_fee, pre_fee_logs, post_fee_logs); modify these helpers to
construct the new MorphEvm by reusing self.inner.with_inspector(...) or
self.inner.with_precompiles(...) but copy/move the existing
cached_token_fee_info, cached_l1_data_fee, pre_fee_logs and post_fee_logs into
the returned MorphEvm instead of letting new_inner reinitialize them so the
per-transaction fee/reimbursement state is preserved across inspector/precompile
swaps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b6cfcb2-03f7-413a-afde-ac0f4fcdb026
📒 Files selected for processing (3)
crates/consensus/src/validation.rscrates/revm/src/evm.rscrates/revm/src/handler.rs
Comment on lines
+378
to
381
| let refunded = gas.refunded().max(0) as u64; | ||
| let reimburse_eth = U256::from( | ||
| effective_gas_price.saturating_mul((gas.remaining() + gas.refunded() as u64) as u128), | ||
| effective_gas_price.saturating_mul(gas.remaining().saturating_add(refunded) as u128), | ||
| ); |
There was a problem hiding this comment.
Use U256 for the refund multiplication.
This path multiplies effective_gas_price * reimbursable_gas in u128 and only widens afterward. Large MorphTx fee caps can saturate here and under-refund the caller even though the deduction path uses full U256 arithmetic.
🧮 Suggested fix
- let refunded = gas.refunded().max(0) as u64;
- let reimburse_eth = U256::from(
- effective_gas_price.saturating_mul(gas.remaining().saturating_add(refunded) as u128),
- );
+ let refunded = gas.refunded().max(0) as u64;
+ let reimbursable_gas = U256::from(gas.remaining().saturating_add(refunded));
+ let reimburse_eth =
+ U256::from(effective_gas_price).saturating_mul(reimbursable_gas);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/revm/src/handler.rs` around lines 378 - 381, The refund calculation
currently multiplies effective_gas_price by reimbursable gas in u128 and then
widens to U256, which can overflow; change the computation in the block that
defines refunded and reimburse_eth so you convert the reimbursable gas amount to
U256 first (use U256::from(gas.remaining().saturating_add(refunded) as u128))
and then perform the multiplication with effective_gas_price (a U256) using
U256's safe multiply (e.g., saturating_mul or checked_mul as appropriate) so
reimburse_eth is computed entirely in U256; update the variables referenced
(refunded, reimburse_eth, effective_gas_price, gas.remaining(), gas.refunded())
accordingly.
Comment on lines
+810
to
+826
| fn evm_call_balance_of<DB, I>(evm: &mut MorphEvm<DB, I>, token: Address, account: Address) -> U256 | ||
| where | ||
| DB: alloy_evm::Database, | ||
| { | ||
| with_evm_snapshot(evm, |evm| { | ||
| let calldata = encode_balance_of_calldata(account); | ||
| match evm_call(evm, Address::ZERO, token, calldata) { | ||
| Ok(ref result) if result.instruction_result().is_ok() => { | ||
| let output = &result.interpreter_result().output; | ||
| if output.len() >= 32 { | ||
| U256::from_be_slice(&output[..32]) | ||
| } else { | ||
| U256::ZERO | ||
| } | ||
| } | ||
| _ => U256::ZERO, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Extract the full implementation of both functions
rg -A 30 'fn evm_call_balance_of' crates/revm/src/handler.rs
echo "---"
rg -A 50 'fn transfer_erc20_with_evm' crates/revm/src/handler.rsRepository: morph-l2/morph-reth
Length of output: 8100
Propagate infrastructure errors instead of converting them to zero values.
evm_call_balance_of() masks all errors—DB faults, call failures—as U256::ZERO. In transfer_erc20_with_evm(), this causes:
evm_call_balance_of(evm, token_address, from)on line 2 (balance before) returns0on any error- Validation then compares against this phantom
0value, potentially masking the infrastructure fault
While evm_call() errors are explicitly wrapped as TokenTransferFailed, the balance query silently discards infrastructure failures. Return Result<U256, EVMError<...>> and propagate errors with ?.
Suggested changes
-fn evm_call_balance_of<DB, I>(evm: &mut MorphEvm<DB, I>, token: Address, account: Address) -> U256
+fn evm_call_balance_of<DB, I>(
+ evm: &mut MorphEvm<DB, I>,
+ token: Address,
+ account: Address,
+) -> Result<U256, EVMError<DB::Error, MorphInvalidTransaction>>
where
DB: alloy_evm::Database,
{
with_evm_snapshot(evm, |evm| {
let calldata = encode_balance_of_calldata(account);
match evm_call(evm, Address::ZERO, token, calldata) {
- Ok(ref result) if result.instruction_result().is_ok() => {
+ Ok(ref result) if result.instruction_result().is_ok() => {
let output = &result.interpreter_result().output;
if output.len() >= 32 {
- U256::from_be_slice(&output[..32])
+ Ok(U256::from_be_slice(&output[..32]))
} else {
- U256::ZERO
+ Ok(U256::ZERO)
}
}
- _ => U256::ZERO,
+ Ok(_) => Ok(U256::ZERO),
+ Err(err) => Err(err),
}
})
}
- None => evm_call_balance_of(evm, token_address, from),
+ None => evm_call_balance_of(evm, token_address, from)?,
...
- let frame_result = evm_call(evm, from, token_address, calldata).map_err(|e| {
- EVMError::Transaction(MorphInvalidTransaction::TokenTransferFailed {
- reason: format!("Error: {e:?}"),
- })
- })?;
+ let frame_result = evm_call(evm, from, token_address, calldata)?;
...
- let from_balance_after = evm_call_balance_of(evm, token_address, from);
+ let from_balance_after = evm_call_balance_of(evm, token_address, from)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/revm/src/handler.rs` around lines 810 - 826, evm_call_balance_of
currently swallows all errors and returns U256::ZERO; change its signature to
return Result<U256, EVMError<DB::Error>> (or the concrete EVM error type used)
and propagate any errors from evm_call and the snapshot via the ? operator
instead of mapping to zero. Update the with_evm_snapshot closure to return
Result<U256, _> and map/propagate evm_call(evm, ...) failures (and any
DB/snapshot errors) into the Result so callers can handle infrastructure
failures; then update transfer_erc20_with_evm to call the new
evm_call_balance_of(...)?, comparing real balances and mapping/returning
TokenTransferFailed or the propagated EVMError where appropriate.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
crates/revm/src/handler.rs (3)
378-381:⚠️ Potential issue | 🟠 MajorCompute
reimburse_ethinU256.This still multiplies in
u128and only widens afterward, so large fee caps can saturate before conversion and under-refund the caller even though the deduction path uses full-width arithmetic.Proposed fix
- let refunded = gas.refunded().max(0) as u64; - let reimburse_eth = U256::from( - effective_gas_price.saturating_mul(gas.remaining().saturating_add(refunded) as u128), - ); + let refunded = gas.refunded().max(0) as u64; + let reimbursable_gas = U256::from(gas.remaining().saturating_add(refunded)); + let reimburse_eth = + U256::from(effective_gas_price).saturating_mul(reimbursable_gas);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/revm/src/handler.rs` around lines 378 - 381, The multiplication is happening in u128 then converted to U256, causing saturation; change the reimburse_eth computation to perform the multiplication in U256 by converting the operands to U256 before multiplying (e.g. convert effective_gas_price and the sum gas.remaining().saturating_add(refunded) into U256 and then use U256 multiplication or saturating_mul). Update the expression that defines reimburse_eth (the variable named reimburse_eth that uses effective_gas_price, gas.remaining(), and refunded) so arithmetic happens at U256 width to avoid under-refunding.
806-823:⚠️ Potential issue | 🟠 MajorDon't collapse
balanceOffailures into a zero balance.Returning
U256::ZEROon DB/call errors can fabricate sender-balance mismatches in the charge path and silently downgrade infrastructure faults into a best-effort refund log in the reimburse path. Make this helper returnResult<U256, EVMError<...>>and propagate real failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/revm/src/handler.rs` around lines 806 - 823, The helper evm_call_balance_of currently swallows all call/DB errors and returns U256::ZERO; change its signature to return Result<U256, EVMError<DB::Error>> (or the crate's EVMError type) and propagate the real error instead of mapping failures to zero: inside evm_call_balance_of (and the with_evm_snapshot closure) map the evm_call(...) Err and non-ok instruction_result() cases to Err(...) with the underlying error/context, and return Ok(U256) only when output parsing succeeds; then update all callers of evm_call_balance_of to handle the Result (propagate ? where appropriate) and adjust any calling functions (e.g., charge/reimburse paths) to surface infrastructure failures instead of treating them as zero balances.
855-861:⚠️ Potential issue | 🟠 MajorPreserve non-transaction
EVMErrorvariants from the internal transfer call.This
map_errconverts database/runtime faults intoTokenTransferFailed, so infra errors get misclassified as transaction invalidation and may then be swallowed by the refund path's "continue on failure" logic. Only wrap genuine transfer-result validation failures; propagate otherEVMErrorvariants unchanged.Based on learnings:
reimburse_caller_token_feeintentionally continues on genuine refund failures; this concern is about misclassifying infrastructure errors before that point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/revm/src/handler.rs` around lines 855 - 861, The current map_err on evm_call(...) unconditionally wraps any EVMError into MorphInvalidTransaction::TokenTransferFailed, misclassifying infra/runtime errors; instead call evm_call(from, token_address, calldata) and map_err by matching the returned EVMError: if it's already an EVMError::Transaction (i.e. a genuine transaction validation/failure) then convert/wrap it into EVMError::Transaction(MorphInvalidTransaction::TokenTransferFailed { .. }) as before, but for any other EVMError variant return it unchanged (propagate the original error) so infrastructure/database faults are not masked and can be handled upstream (see with_evm_checkpoint, build_transfer_calldata, evm_call and reimburse_caller_token_fee for context).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/revm/src/handler.rs`:
- Around line 378-381: The multiplication is happening in u128 then converted to
U256, causing saturation; change the reimburse_eth computation to perform the
multiplication in U256 by converting the operands to U256 before multiplying
(e.g. convert effective_gas_price and the sum
gas.remaining().saturating_add(refunded) into U256 and then use U256
multiplication or saturating_mul). Update the expression that defines
reimburse_eth (the variable named reimburse_eth that uses effective_gas_price,
gas.remaining(), and refunded) so arithmetic happens at U256 width to avoid
under-refunding.
- Around line 806-823: The helper evm_call_balance_of currently swallows all
call/DB errors and returns U256::ZERO; change its signature to return
Result<U256, EVMError<DB::Error>> (or the crate's EVMError type) and propagate
the real error instead of mapping failures to zero: inside evm_call_balance_of
(and the with_evm_snapshot closure) map the evm_call(...) Err and non-ok
instruction_result() cases to Err(...) with the underlying error/context, and
return Ok(U256) only when output parsing succeeds; then update all callers of
evm_call_balance_of to handle the Result (propagate ? where appropriate) and
adjust any calling functions (e.g., charge/reimburse paths) to surface
infrastructure failures instead of treating them as zero balances.
- Around line 855-861: The current map_err on evm_call(...) unconditionally
wraps any EVMError into MorphInvalidTransaction::TokenTransferFailed,
misclassifying infra/runtime errors; instead call evm_call(from, token_address,
calldata) and map_err by matching the returned EVMError: if it's already an
EVMError::Transaction (i.e. a genuine transaction validation/failure) then
convert/wrap it into
EVMError::Transaction(MorphInvalidTransaction::TokenTransferFailed { .. }) as
before, but for any other EVMError variant return it unchanged (propagate the
original error) so infrastructure/database faults are not masked and can be
handled upstream (see with_evm_checkpoint, build_transfer_calldata, evm_call and
reimburse_caller_token_fee for context).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 946108b9-6e21-472d-b0a8-cb110a59b10e
📒 Files selected for processing (1)
crates/revm/src/handler.rs
Contributor
Code reviewFound 2 issues:
morph-reth/crates/revm/src/handler.rs Lines 416 to 437 in 78c6a31
morph-reth/crates/primitives/src/transaction/morph_transaction.rs Lines 81 to 83 in 78c6a31 morph-reth/crates/primitives/src/transaction/morph_transaction.rs Lines 891 to 896 in 78c6a31 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Contributor
Author
|
chengwenxi
approved these changes
Mar 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
evm_callfor ERC20, decouple from handler pipeline to survive tx reverts, discardbalanceOflogsnew_safe_l2_block, limit FCU tag fallback to historical sync, prevent unbounded memory growthOnStateHookinMorphBlockExecutorforStateRootTaskTest plan
cargo checkpassescargo testpassesSummary by CodeRabbit
New Features
Bug Fixes
Chores