Conversation
🟡 Heimdall Review Status
|
refcell
added
K-enhancement
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| // If the input is a deposit transaction or empty, the default value is zero. | ||
| let tx_l1_cost = if input.is_empty() || input.first() == Some(&0x7E) { | ||
| return U256::ZERO; |
There was a problem hiding this comment.
This early return U256::ZERO bypasses the cache assignment on line 296, so deposit/empty inputs are recalculated on every call instead of being cached. Not a correctness bug (deposits are always zero-cost), but inconsistent with the caching intent. Consider caching zero as well:
| return U256::ZERO; | |
| let tx_l1_cost = if input.is_empty() || input.first() == Some(&0x7E) { | |
| U256::ZERO |
| let operator_cost_gas_limit = | ||
| self.operator_fee_charge_inner(U256::from(gas.limit()), spec_id); | ||
| let operator_cost_gas_used = self.operator_fee_charge_inner( | ||
| U256::from(gas.limit() - (gas.remaining() + gas.refunded() as u64)), |
There was a problem hiding this comment.
gas.refunded() returns i64. Casting to u64 when refunded is negative would produce a very large value, causing the subtraction gas.limit() - (gas.remaining() + gas.refunded() as u64) to underflow/panic. While the refunded value should be non-negative at this point in the handler flow, this is fragile. Consider using saturating_sub or explicit bounds:
let gas_used = gas.limit().saturating_sub(gas.remaining()).saturating_sub(gas.refunded().max(0) as u64);
| } else { | ||
| U256::ZERO | ||
| }; | ||
| let base_fee_amount = U256::from(basefee.saturating_mul(frame_result.gas().used() as u128)); |
There was a problem hiding this comment.
frame_result.gas().used() returns u64. Casting to u128 and then multiplying by basefee (also u128) could in theory overflow a u128 before being wrapped in U256::from(), though in practice basefee * gas_used fits in u128. A more defensive approach:
| let base_fee_amount = U256::from(basefee.saturating_mul(frame_result.gas().used() as u128)); | |
| let base_fee_amount = U256::from(basefee) * U256::from(frame_result.gas().used()); |
| let effective_balance_spending = tx | ||
| .effective_balance_spending(basefee, blob_price) | ||
| .expect("Deposit transaction effective balance spending overflow") | ||
| - tx.value(); |
There was a problem hiding this comment.
.expect() in consensus-critical deposit handling will panic the node if effective_balance_spending returns None (overflow). The subsequent - tx.value() is also an unchecked subtraction on U256 that will panic in debug or wrap in release if value > effective_balance_spending.
Consider propagating the error instead:
| - tx.value(); | |
| let effective_balance_spending = tx | |
| .effective_balance_spending(basefee, blob_price) | |
| .ok_or_else(|| ERROR::from_string( | |
| "[OPTIMISM] Deposit transaction effective balance spending overflow".into(), | |
| ))? | |
| .saturating_sub(tx.value()); |
| std = [ | ||
| "alloy-primitives/std", | ||
| "alloy-sol-types/std", | ||
| "revm/std", | ||
| "serde?/std", | ||
| "serde_json/std", | ||
| "sha2/std", | ||
| ] |
There was a problem hiding this comment.
The std feature enables /std on alloy-primitives, alloy-sol-types, serde_json, and sha2, but these are all dev-dependencies only. Enabling their std feature from a regular feature flag is a no-op for library consumers (since dev-deps aren't resolved downstream), but it's misleading — it implies these are regular dependencies. The std feature should only list crates that appear in [dependencies]:
| std = [ | |
| "alloy-primitives/std", | |
| "alloy-sol-types/std", | |
| "revm/std", | |
| "serde?/std", | |
| "serde_json/std", | |
| "sha2/std", | |
| ] | |
| std = [ | |
| "revm/std", | |
| "serde?/std", | |
| ] |
| } | ||
|
|
||
| // Pre-Jovian | ||
| if spec_id.is_enabled_in(OpSpecId::JOVIAN) { |
There was a problem hiding this comment.
Misleading comment says // Pre-Jovian but the guard is is_enabled_in(OpSpecId::JOVIAN), which means post-Jovian (Jovian and later). Should be:
| if spec_id.is_enabled_in(OpSpecId::JOVIAN) { | |
| // Post-Jovian | |
| if spec_id.is_enabled_in(OpSpecId::JOVIAN) { |
| /// Calculate the operator fee for the given `gas`. | ||
| fn operator_fee_charge_inner(&self, gas: U256, spec_id: OpSpecId) -> U256 { | ||
| let operator_fee_scalar = | ||
| self.operator_fee_scalar.expect("Missing operator fee scalar for isthmus L1 Block"); |
There was a problem hiding this comment.
operator_fee_charge_inner panics via .expect() if operator_fee_scalar or operator_fee_constant are None. These should always be populated post-Isthmus by try_fetch_isthmus, but if the L1Block contract state is corrupted or the storage read fails silently, this will crash the node.
Consider returning Option<U256> or propagating the error, consistent with the existing comment on handler.rs:121 about avoiding panics in consensus-critical paths.
| }; | ||
|
|
||
| mod constants; | ||
| pub use constants::*; |
There was a problem hiding this comment.
pub use constants::* and pub use spec::* are wildcard re-exports. Unlike the other modules where re-exports are explicit (e.g. pub use l1block::L1BlockInfo), these make the public API surface implicit and fragile — adding a new pub item to constants.rs or spec.rs automatically expands the crate's public API without any change to lib.rs. Consider enumerating the re-exports explicitly as is done for the other modules.
| OpTxEnvelope::Legacy(tx) => Self { | ||
| base: TxEnv::from_recovered_tx(tx.tx(), caller), | ||
| enveloped_tx: Some(encoded), | ||
| deposit: Default::default(), | ||
| }, | ||
| OpTxEnvelope::Eip1559(tx) => Self { | ||
| base: TxEnv::from_recovered_tx(tx.tx(), caller), | ||
| enveloped_tx: Some(encoded), | ||
| deposit: Default::default(), | ||
| }, | ||
| OpTxEnvelope::Eip2930(tx) => Self { | ||
| base: TxEnv::from_recovered_tx(tx.tx(), caller), | ||
| enveloped_tx: Some(encoded), | ||
| deposit: Default::default(), | ||
| }, | ||
| OpTxEnvelope::Eip7702(tx) => Self { | ||
| base: TxEnv::from_recovered_tx(tx.tx(), caller), | ||
| enveloped_tx: Some(encoded), | ||
| deposit: Default::default(), | ||
| }, |
There was a problem hiding this comment.
The four non-deposit arms are identical — each constructs Self { base: TxEnv::from_recovered_tx(...), enveloped_tx: Some(encoded), deposit: Default::default() }. Consider collapsing them into a single match arm to reduce duplication:
match tx {
OpTxEnvelope::Deposit(tx) => Self::from_encoded_tx(tx.inner(), caller, encoded),
other => Self {
base: TxEnv::from_recovered_tx(other.as_consensus_tx(), caller),
enveloped_tx: Some(encoded),
deposit: Default::default(),
},
}(Adjust method name based on what trait provides the inner tx reference for the non-deposit variants.)
| ..Default::default() | ||
| }; | ||
|
|
||
| // Post-Ecotone |
There was a problem hiding this comment.
Misleading comment: "Post-Ecotone" but the guard is !spec_id.is_enabled_in(OpSpecId::ECOTONE), which means this branch is taken for Pre-Ecotone (Bedrock/Regolith/Canyon). Same issue as the "Pre-Jovian" comment on line 161 (which is actually Post-Jovian).
| // Post-Ecotone | |
| // Pre-Ecotone (Bedrock cost function) |
|
|
||
| /// As of the Jovian upgrade, this storage slot stores the 16-bit daFootprintGasScalar attribute at | ||
| /// offset [DA_FOOTPRINT_GAS_SCALAR_OFFSET]. | ||
| pub const DA_FOOTPRINT_GAS_SCALAR_SLOT: U256 = U256::from_limbs([8u64, 0, 0, 0]); |
There was a problem hiding this comment.
DA_FOOTPRINT_GAS_SCALAR_SLOT and OPERATOR_FEE_SCALARS_SLOT (line 49) are both storage slot 8. This is correct — they share a slot with different packed fields at different offsets — but a comment would help prevent confusion:
| pub const DA_FOOTPRINT_GAS_SCALAR_SLOT: U256 = U256::from_limbs([8u64, 0, 0, 0]); | |
| /// As of the Jovian upgrade, this storage slot stores the 16-bit daFootprintGasScalar attribute at | |
| /// offset [DA_FOOTPRINT_GAS_SCALAR_OFFSET]. | |
| /// | |
| /// Note: shares the same storage slot as [`OPERATOR_FEE_SCALARS_SLOT`]; different fields are packed at different offsets. | |
| pub const DA_FOOTPRINT_GAS_SCALAR_SLOT: U256 = U256::from_limbs([8u64, 0, 0, 0]); |
| impl Default for OpPrecompiles { | ||
| fn default() -> Self { | ||
| Self::new_with_spec(OpSpecId::JOVIAN) | ||
| } |
There was a problem hiding this comment.
OpPrecompiles::default() hardcodes OpSpecId::JOVIAN, but OpSpecId::default() is ISTHMUS. The test test_default_precompiles_is_latest (line 474) appears to validate parity between these, but it only compares address counts via .len() and .intersection() — it does not verify that the underlying precompile implementations match. Since JOVIAN replaces several precompile functions (bn254 pair, BLS12 G1/G2 MSM, pairing) with stricter input limits, the test passes despite the defaults actually differing in behavior.
Either align the defaults (e.g. OpPrecompiles::default() should use OpSpecId::default()), or rename/fix the test to clarify what it actually validates.
| pub l1_blob_base_fee: Option<U256>, | ||
| /// The current L1 blob base fee scalar. None if Ecotone is not activated. | ||
| pub l1_blob_base_fee_scalar: Option<U256>, | ||
| /// The current L1 blob base fee. None if Isthmus is not activated, except if `empty_ecotone_scalars` is `true`. |
There was a problem hiding this comment.
Doc comments for operator_fee_scalar and operator_fee_constant are copy-pasted from the blob base fee fields:
/// The current L1 blob base fee. None if Isthmus is not activated...
pub operator_fee_scalar: Option<U256>,
/// The current L1 blob base fee scalar. None if Isthmus is not activated.
pub operator_fee_constant: Option<U256>,
These should describe the operator fee fields, not blob base fee fields.
| let effective_balance_spending = tx | ||
| .effective_balance_spending(basefee, blob_price) | ||
| .expect("Deposit transaction effective balance spending overflow") | ||
| - tx.value(); |
There was a problem hiding this comment.
Unchecked U256 subtraction: effective_balance_spending - tx.value() will panic in debug mode (or wrap in release) if tx.value() > effective_balance_spending. While effective_balance_spending is computed from effective_gas_price * gas_limit + blob_gas_price * blob_gas + value, so in theory it should always be >= value, the correctness of this depends on effective_balance_spending being faithfully computed by Transaction::effective_balance_spending. Using saturating_sub here would be more defensive, consistent with the surrounding arithmetic.
| hash as u16 & 0x1fff | ||
| } | ||
|
|
||
| fn u24(input: &[u8], idx: u32) -> u32 { |
There was a problem hiding this comment.
Potential panic: u24 indexes into input using idx as usize, (idx+1) as usize, and (idx+2) as usize without bounds checking. While the main loop in flz_compress_len guards idx < idx_limit where idx_limit = len - 13, the function cmp also calls array indexing with (p + l) as usize and (q + l) as usize where bounds depend on r - q being within range.
More critically, set_next_hash calls u24(input, idx) after idx has been incremented by len — if len is large enough, this could index past the end of the input. The Go reference implementation uses checked bounds in its equivalent. Consider adding bounds checks or documenting the safety invariants that prevent out-of-bounds access.
| let basefee = evm.ctx().block().basefee() as u128; | ||
|
|
||
| let ctx = evm.ctx(); | ||
| let enveloped = ctx.tx().enveloped_tx().cloned(); |
There was a problem hiding this comment.
.cloned() on a Bytes (which wraps an Arc<[u8]>) is cheap (reference count bump), but this clone is taken on every non-deposit transaction in reward_beneficiary — a hot path. Since enveloped_tx is only read (passed as &[u8]), consider restructuring to avoid the clone by computing costs before dropping the borrow on ctx:
let (l1_cost, operator_fee_cost, base_fee_amount) = {
let ctx = evm.ctx();
let enveloped = ctx.tx().enveloped_tx().ok_or_else(|| {
ERROR::from_string("[OPTIMISM] Failed to load enveloped transaction.".into())
})?;
let spec = ctx.cfg().spec();
let l1_cost = ctx.chain().calculate_tx_l1_cost(enveloped, spec);
// ... compute other values
(l1_cost, operator_fee_cost, base_fee_amount)
};This would avoid the let enveloped = ctx.tx().enveloped_tx().cloned() pattern entirely. Though this may require chain() to be split from the immutable borrow — evaluate if the borrow checker allows it.
| } | ||
|
|
||
| /// Calculate the gas cost of a transaction based on L1 block data posted on L2, depending on the [`OpSpecId`] passed. | ||
| pub fn calculate_tx_l1_cost(&mut self, input: &[u8], spec_id: OpSpecId) -> U256 { |
There was a problem hiding this comment.
calculate_tx_l1_cost takes &mut self solely because it writes to self.tx_l1_cost (the cache field). This forces callers (like reward_beneficiary in handler.rs) to take a mutable reference to L1BlockInfo through ctx.chain_mut(), which creates borrow-checker friction (e.g., forcing the .cloned() on enveloped_tx at line 269 in handler.rs).
Consider using Cell<Option<U256>> for the cache field, which would let this method take &self instead.
| ..Default::default() | ||
| }; | ||
|
|
||
| // Post-Ecotone |
There was a problem hiding this comment.
The try_fetch function has a guard at line 147 !spec_id.is_enabled_in(OpSpecId::ECOTONE) which returns early for pre-Ecotone specs. But the comment on line 146 says // Post-Ecotone. The negation makes this confusing — the early return is taken for pre-Ecotone specs (Bedrock/Regolith/Canyon), while the code after it handles Ecotone and later. The comment should say // Pre-Ecotone to match the early return:
| // Post-Ecotone | |
| // Pre-Ecotone | |
| if !spec_id.is_enabled_in(OpSpecId::ECOTONE) { |
| /// Optimism transaction validation error. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | ||
| pub enum OpTransactionError { |
There was a problem hiding this comment.
This type implements Display and Error manually but does not use thiserror. Per the project's error handling conventions (thiserror enums with From impls), consider migrating:
#[derive(Debug, Clone, PartialEq, Eq, Hash, thiserror::Error)]
pub enum OpTransactionError {
#[error("{0}")]
Base(InvalidTransaction),
#[error("deposit system transactions post regolith hardfork are not supported")]
DepositSystemTxPostRegolith,
#[error("deposit transaction halted post-regolith; error will be bubbled up to main return handler")]
HaltedDepositPostRegolith,
#[error("missing enveloped transaction bytes for non-deposit transaction")]
MissingEnvelopedTx,
}Note: this would require adding thiserror to the crate's dependencies. If this was intentionally avoided to keep the crate dependency-light, disregard.
| } | ||
|
|
||
| self.mainnet.reward_beneficiary(evm, frame_result)?; | ||
| let basefee = evm.ctx().block().basefee() as u128; |
There was a problem hiding this comment.
basefee as u128: basefee() returns u64, so this cast is safe. But on line 289, frame_result.gas().used() as u128 followed by saturating_mul is fine for now, though gas.used() returns u64 and basefee fits in u64, so the multiplication fits in u128.
However, the real concern is that l1_cost, base_fee_amount, and operator_fee_cost are all sent to their respective recipients via balance_incr without any check that the total doesn't exceed what was originally deducted from the caller. If fee calculations diverge between validate_against_state_and_deduct_caller (which uses tx_cost = l1_cost + operator_fee) and reward_beneficiary (which independently recalculates both), the node could credit more than it debited, effectively minting ETH. Both paths should ideally share the same computation or at minimum be covered by an integration test that verifies the invariant total_credited <= total_debited.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
| MissingEnvelopedTxBytes, | ||
| /// Missing source hash for deposit transaction | ||
| MissingSourceHashForDeposit, | ||
| } |
There was a problem hiding this comment.
OpBuildError is missing Display and Error trait implementations. As a public error type returned from build() -> Result<OpTransaction<TxEnv>, OpBuildError>, it can't be used with the ? operator in contexts expecting std::error::Error. This is inconsistent with OpTransactionError which does implement both traits.
Consider adding implementations (either manually or via thiserror):
impl core::fmt::Display for OpBuildError {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
match self {
Self::Base(e) => write!(f, "base transaction build error: {e}"),
Self::MissingEnvelopedTxBytes => write!(f, "missing enveloped transaction bytes"),
Self::MissingSourceHashForDeposit => write!(f, "missing source hash for deposit transaction"),
}
}
}
impl core::error::Error for OpBuildError {}
|
Review Error for 0x00101010 @ 2026-02-25 00:30:12 UTC |
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
- Fix 58 clippy warnings in base-revm (doc_markdown, use_self, missing_const_for_fn, collapsible_if) - Fix private module access: change precompiles::OpPrecompiles import in reth-optimism-node IT tests to use re-exported OpPrecompiles - Remove hashbrown feature from base-revm to prevent alloy-primitives/map-hashbrown being enabled with --all-features, which caused reth-testing-utils compilation failure Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Resolve merge conflicts in block_assembler.rs, validation.rs - Fix private module path access in executor.rs tests (use base_revm::* re-exports instead of base_revm::constants::*) - Remove unused hashbrown dep from execution/trie - Remove unused imports flagged by cargo fix (handler.rs, fast_lz.rs) - Regenerate Cargo.lock after rebase Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| let gas_used = | ||
| if spec.is_enabled_in(OpSpecId::REGOLITH) || !is_system_tx { gas_limit } else { 0 }; | ||
| // clear the journal | ||
| output = Ok(ExecutionResult::Halt { reason: OpHaltReason::FailedDeposit, gas_used }) |
There was a problem hiding this comment.
In catch_error, the cleanup block (lines 371-373) runs unconditionally, but the deposit error handling on lines 341-368 uses let mut output = Err(error) and conditionally replaces it. If is_tx_error && is_deposit is true but load_account_mut fails (line 355), the ? will return early, skipping the cleanup (clearing tx_l1_cost, local, and frame_stack). This leaks stale state into subsequent transaction processing.
Consider using a guard/defer pattern or restructuring to ensure cleanup always runs:
// Always perform cleanup
let result = if is_tx_error && is_deposit {
// ... deposit handling ...
} else {
Err(error)
};
evm.ctx().chain_mut().clear_tx_l1_cost();
evm.ctx().local_mut().clear();
evm.frame_stack().clear();
result
Review Summary —
|
|
Review Error for meyer9 @ 2026-02-25 01:07:38 UTC |
6 participants
Summary
Rolls
op-revminto base/base to allow customization of op-revm logic as well as trait implementations on op-revm types. This allows us to remove allopfeature flags which transitively enableop-alloytypes.