Skip to content

Commit e48216e

Browse files
committed
fix(l1): address PR #6521 bot review findings
Two bot reviews (Codex, Claude) + one code reviewer (Greptile) flagged 7 issues. All 7 verified real and fixed in-PR per user request. Critical (L2 consensus/liveness): - `crates/l2/sequencer/block_producer/payload_builder.rs` now enforces the EIP-8037 PR #2703 per-tx 2D inclusion check against the L2's `configured_block_gas_limit` before executing each tx, matching the L1 builder. Without this, the L2 builder could reject valid txs or accept txs that violate one dimension of the block cap. - `fill_transactions` now snapshots and restores `block_regular_gas_used` / `block_state_gas_used` around `apply_plain_transaction` on the `undo_last_tx` rollback path (invalid L2 out-message). Previously those two counters stayed inflated after a rejected tx, polluting `gas_used()` and the final header `gas_used`. High (mempool DoS avenue): - `crates/blockchain/mempool.rs::transaction_intrinsic_gas` now enforces `max(intrinsic_regular + intrinsic_state, floor)` for Amsterdam+, matching the VM's `validate_min_gas_limit` check. Previously a tx with mostly zero calldata could pass mempool admission at the weighted EIP-2028 cost (400 gas for 100 zero bytes) but fail the VM's 6400-gas unweighted floor at block inclusion, polluting the pool. New standalone helper `intrinsic_gas_floor(tx, fork)` added in `crates/vm/levm/src/utils.rs` mirroring `VM::get_min_gas_used` so the mempool / payload builder can compute the floor without a VM instance. Re-exported from `ethrex-vm`. Medium: - `crates/vm/backends/levm/mod.rs` withdrawal index computation switched from `.map(|n| n + 1)` to `.map(|n| n.saturating_add(1))`. The prior form wraps to 0 in release builds when `n == u32::MAX` (the `debug_assert` only fires in debug). - `crates/vm/levm/src/opcode_handlers/system.rs` adds `debug_assert!` at the two reservoir-revert sites verifying `outstanding_delta >= credit_against_drain_delta`. If that invariant is ever violated, the `saturating_sub` silently mischarges the block's regular dimension; a loud debug panic is preferable. Style: - `crates/vm/levm/src/gas_cost.rs::access_list_bytes` — replace `keys.len() as u64` with `u64::try_from(...).unwrap_or(u64::MAX)` for consistency with the rest of the codebase. - `crates/vm/levm/src/hooks/default_hook.rs::refund_sender` — rename the currently-unused `gas_used_pre_refund` parameter to `_gas_used_pre_refund` at the signature and drop the interior `let _ =` that was silencing it. Expanded doc explains it's kept in the signature for future reintroduction. All 478 tests pass; no behavior changes except the three intentional ones (mempool floor, L2 2D check, L2 counter rollback) plus the already-exercised saturation edge.
1 parent 441f0ca commit e48216e

8 files changed

Lines changed: 144 additions & 15 deletions

File tree

crates/blockchain/mempool.rs

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ use ethrex_common::{
2121
},
2222
};
2323
use ethrex_storage::error::StoreError;
24-
use ethrex_vm::intrinsic_gas_dimensions;
24+
use ethrex_vm::{intrinsic_gas_dimensions, intrinsic_gas_floor};
2525
use tracing::warn;
2626

2727
#[derive(Debug, Default)]
@@ -517,14 +517,24 @@ pub fn transaction_intrinsic_gas(
517517
// `REGULAR_GAS_CREATE = 9000` + `STATE_BYTES_PER_NEW_ACCOUNT * cpsb` for CREATE
518518
// instead of the legacy `TX_CREATE_GAS_COST = 53000`. Mempool admission must
519519
// match VM charge or we spuriously reject (or admit) transactions.
520-
// EIP-7981 access-list data bytes + EIP-7976 floor are also handled there.
520+
//
521+
// The VM enforces `gas_limit >= max(intrinsic_regular + intrinsic_state,
522+
// floor)` via two separate checks in `validate_gas_allowance` +
523+
// `validate_min_gas_limit`. Apply the same max here so we don't admit
524+
// txs whose calldata floor exceeds the weighted intrinsic — those would
525+
// pass mempool and then fail at block inclusion, polluting the pool.
521526
if config.is_amsterdam_activated(header.timestamp) {
522527
let fork = config.fork(header.timestamp);
523528
let (regular, state) = intrinsic_gas_dimensions(tx, fork, header.gas_limit)
524529
.map_err(|_| MempoolError::TxGasOverflowError)?;
525-
return regular
530+
let intrinsic = regular
526531
.checked_add(state)
527-
.ok_or(MempoolError::TxGasOverflowError);
532+
.ok_or(MempoolError::TxGasOverflowError)?;
533+
let floor = intrinsic_gas_floor(tx, fork).map_err(|_| MempoolError::TxGasOverflowError)?;
534+
// Block-level gas = max(regular_dim, state_dim); regular_dim itself is
535+
// `max(tx_regular, calldata_floor)` per EIP-7778. Use the same max so
536+
// admission mirrors the VM's effective minimum.
537+
return Ok(intrinsic.max(floor));
528538
}
529539

530540
let is_contract_creation = tx.is_contract_creation();

crates/l2/sequencer/block_producer/payload_builder.rs

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@ use ethrex_blockchain::{
66
};
77
use ethrex_common::{
88
U256,
9-
types::{Block, EIP1559_DEFAULT_SERIALIZED_LENGTH, SAFE_BYTES_PER_BLOB, Transaction, TxKind},
9+
types::{
10+
Block, EIP1559_DEFAULT_SERIALIZED_LENGTH, Fork, SAFE_BYTES_PER_BLOB, Transaction, TxKind,
11+
},
1012
};
1113
use ethrex_l2_common::{
1214
messages::get_block_l2_out_messages, privileged_transactions::PRIVILEGED_TX_BUDGET,
@@ -20,6 +22,7 @@ use ethrex_metrics::{
2022
};
2123
use ethrex_rlp::encode::RLPEncode;
2224
use ethrex_storage::Store;
25+
use ethrex_vm::check_2d_gas_allowance;
2326
use std::sync::Arc;
2427
use std::{collections::HashMap, ops::Div};
2528
use tokio::time::Instant;
@@ -110,6 +113,14 @@ pub async fn fill_transactions(
110113
let chain_config = store.get_chain_config();
111114
let chain_id = chain_config.chain_id;
112115

116+
// EIP-8037 (Amsterdam+): the tx inclusion check enforces a 2D budget per
117+
// tx so a transaction's worst-case contribution in either dimension fits
118+
// in the remaining block budget. Gate on the block's timestamp and apply
119+
// in the inclusion loop below; the L2 builder uses
120+
// `configured_block_gas_limit` (possibly tighter than
121+
// `payload.header.gas_limit`) as the limit, keeping L2 tighter than L1.
122+
let is_amsterdam = chain_config.is_amsterdam_activated(context.payload.header.timestamp);
123+
113124
debug!("Fetching transactions from mempool");
114125
// Fetch mempool transactions
115126
let latest_block_number = store.get_latest_block_number().await?;
@@ -209,6 +220,25 @@ pub async fn fill_transactions(
209220
continue;
210221
}
211222

223+
// EIP-8037 (Amsterdam+, PR #2703): per-tx 2D inclusion check against
224+
// running block totals, using the L2-configured block gas limit
225+
// (which may be tighter than the header's). Must run BEFORE we touch
226+
// the BAL recorder so a rejected tx doesn't leave a sender/recipient
227+
// touch in the BAL.
228+
if is_amsterdam
229+
&& let Err(e) = check_2d_gas_allowance(
230+
&head_tx.tx,
231+
Fork::Amsterdam,
232+
context.block_regular_gas_used,
233+
context.block_state_gas_used,
234+
configured_block_gas_limit,
235+
)
236+
{
237+
debug!("Skipping tx {tx_hash:#x}: fails 2D inclusion check: {e}");
238+
txs.pop();
239+
continue;
240+
}
241+
212242
// Set BAL index for this transaction (1-indexed per EIP-7928)
213243
let tx_index =
214244
u32::try_from(context.payload.body.transactions.len() + 1).unwrap_or(u32::MAX);
@@ -233,10 +263,16 @@ pub async fn fill_transactions(
233263
}
234264
}
235265

236-
// Execute tx
266+
// Execute tx. Snapshot every PayloadBuildContext counter that
267+
// `apply_plain_transaction` mutates so the invalid-L2-message rollback
268+
// below can fully undo a tx's effect. Amsterdam's 2D accounting adds
269+
// `block_regular_gas_used` / `block_state_gas_used` to the set that
270+
// drive `gas_used()` and the final header `gas_used`.
237271
let previous_remaining_gas = context.remaining_gas;
238272
let previous_block_value = context.block_value;
239273
let previous_cumulative_gas_spent = context.cumulative_gas_spent;
274+
let previous_block_regular_gas_used = context.block_regular_gas_used;
275+
let previous_block_state_gas_used = context.block_state_gas_used;
240276
let receipt = match apply_plain_transaction(&head_tx, context) {
241277
Ok(receipt) => receipt,
242278
Err(e) => {
@@ -264,6 +300,12 @@ pub async fn fill_transactions(
264300
context.remaining_gas = previous_remaining_gas;
265301
context.block_value = previous_block_value;
266302
context.cumulative_gas_spent = previous_cumulative_gas_spent;
303+
// Amsterdam 2D accounting: restore the per-dimension counters
304+
// too. Without this, phantom gas from the rejected tx stays in
305+
// the payload context and skews subsequent inclusion decisions
306+
// plus the final header `gas_used`.
307+
context.block_regular_gas_used = previous_block_regular_gas_used;
308+
context.block_state_gas_used = previous_block_state_gas_used;
267309
// Roll back BAL touches from the aborted tx.
268310
if let (Some(recorder), Some(checkpoint)) =
269311
(context.vm.db.bal_recorder_mut(), bal_checkpoint)

crates/vm/backends/levm/mod.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -443,9 +443,11 @@ impl LEVM {
443443
// not from db — no need to call send_state_transitions_tx here.
444444

445445
// Validate BAL entries at the withdrawal index against actual
446-
// post-withdrawal/request state.
446+
// post-withdrawal/request state. `saturating_add(1)` prevents a
447+
// release-build wrap if `n == u32::MAX` (debug_assert on tx count
448+
// catches this upstream, but belt-and-braces).
447449
let withdrawal_idx = u32::try_from(block.body.transactions.len())
448-
.map(|n| n + 1)
450+
.map(|n| n.saturating_add(1))
449451
.unwrap_or(u32::MAX);
450452
Self::validate_bal_withdrawal_index(db, bal, withdrawal_idx, &validation_index)?;
451453

crates/vm/levm/src/gas_cost.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -662,7 +662,8 @@ pub fn access_list_bytes(access_list: &AccessList) -> u64 {
662662
let mut bytes: u64 = 0;
663663
for (_addr, keys) in access_list {
664664
bytes = bytes.saturating_add(20);
665-
bytes = bytes.saturating_add(32_u64.saturating_mul(keys.len() as u64));
665+
let keys_len = u64::try_from(keys.len()).unwrap_or(u64::MAX);
666+
bytes = bytes.saturating_add(32_u64.saturating_mul(keys_len));
666667
}
667668
bytes
668669
}

crates/vm/levm/src/hooks/default_hook.rs

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -238,9 +238,13 @@ pub fn refund_sender(
238238
ctx_result: &mut ContextResult,
239239
refunded_gas: u64,
240240
gas_spent: u64,
241-
// Pre-Amsterdam: gas used for receipt and user refund. Amsterdam+: unused
242-
// (block gas is computed dimensionally from vm fields; user pays gas_spent).
243-
gas_used_pre_refund: u64,
241+
// Historically used pre-Amsterdam for receipt + user refund; Amsterdam+
242+
// computes block gas dimensionally from VM fields and the user pays
243+
// `gas_spent`, so this parameter is currently unused in both branches.
244+
// Kept in the signature for call-site symmetry with pre-Amsterdam usage
245+
// and future reintroduction; rename without the `_` prefix once it's
246+
// read again.
247+
_gas_used_pre_refund: u64,
244248
) -> Result<(), VMError> {
245249
vm.substate.refunded_gas = refunded_gas;
246250

@@ -263,8 +267,9 @@ pub fn refund_sender(
263267
.state_gas_refund_absorbed
264268
.saturating_add(vm.state_gas_refund_pending);
265269
let state_gas = vm.state_gas_used.saturating_sub(execution_state_gas_refund);
266-
// gas_used_pre_refund here is raw - reservoir_current (user-paid). Compute
267-
// raw from scratch to avoid the reservoir-current subtraction interfering.
270+
// Compute raw consumption from scratch (gas_limit minus gas_remaining)
271+
// to avoid interference from any reservoir-current subtraction baked
272+
// into the caller's pre-refund number.
268273
#[expect(clippy::as_conversions, reason = "gas_remaining is >= 0 here")]
269274
let gas_remaining = vm.current_call_frame.gas_remaining.max(0) as u64;
270275
let raw_consumed = vm.env.gas_limit.saturating_sub(gas_remaining);
@@ -280,7 +285,6 @@ pub fn refund_sender(
280285
ctx_result.gas_spent = gas_spent;
281286
} else {
282287
// Pre-Amsterdam: both use post-refund value
283-
let _ = gas_used_pre_refund;
284288
ctx_result.gas_used = gas_spent;
285289
ctx_result.gas_spent = gas_spent;
286290
}

crates/vm/levm/src/opcode_handlers/system.rs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1186,6 +1186,18 @@ impl<'a> VM<'a> {
11861186
let credit_against_drain_delta = self
11871187
.state_gas_credit_against_drain
11881188
.saturating_sub(state_gas_credit_against_drain_snapshot);
1189+
// Invariant: credit_against_drain only accumulates the portion
1190+
// of a clamped refund that was NOT matched against outstanding
1191+
// spill, so it can never exceed the spill delta in the same
1192+
// subtree. If this ever fires, the reservoir math silently
1193+
// clamps (via saturating_sub) and the block's regular
1194+
// dimension gets mischarged — loud panic in debug is the goal.
1195+
debug_assert!(
1196+
outstanding_delta >= credit_against_drain_delta,
1197+
"reservoir revert invariant violated: credit_against_drain_delta \
1198+
({credit_against_drain_delta}) > outstanding_delta \
1199+
({outstanding_delta})"
1200+
);
11891201
self.state_gas_used = state_gas_used_snapshot;
11901202
self.state_gas_refund_pending = state_gas_refund_pending_snapshot;
11911203
self.state_gas_refund_absorbed = state_gas_refund_absorbed_snapshot;
@@ -1261,6 +1273,18 @@ impl<'a> VM<'a> {
12611273
let credit_against_drain_delta = self
12621274
.state_gas_credit_against_drain
12631275
.saturating_sub(state_gas_credit_against_drain_snapshot);
1276+
// Invariant: credit_against_drain only accumulates the portion
1277+
// of a clamped refund that was NOT matched against outstanding
1278+
// spill, so it can never exceed the spill delta in the same
1279+
// subtree. If this ever fires, the reservoir math silently
1280+
// clamps (via saturating_sub) and the block's regular
1281+
// dimension gets mischarged — loud panic in debug is the goal.
1282+
debug_assert!(
1283+
outstanding_delta >= credit_against_drain_delta,
1284+
"reservoir revert invariant violated: credit_against_drain_delta \
1285+
({credit_against_drain_delta}) > outstanding_delta \
1286+
({outstanding_delta})"
1287+
);
12641288
self.state_gas_used = state_gas_used_snapshot;
12651289
self.state_gas_refund_pending = state_gas_refund_pending_snapshot;
12661290
self.state_gas_refund_absorbed = state_gas_refund_absorbed_snapshot;

crates/vm/levm/src/utils.rs

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -750,6 +750,49 @@ pub fn intrinsic_gas_dimensions(
750750
Ok((regular_gas, state_gas))
751751
}
752752

753+
/// Standalone EIP-7623/7976/7981 floor gas for a transaction. Mirrors
754+
/// [`VM::get_min_gas_used`] but operates on the raw transaction + fork, so it
755+
/// can be called by mempool admission / the payload builder without needing a
756+
/// VM instance. Returns `TX_BASE_COST + floor_rate * total_floor_tokens`.
757+
///
758+
/// Amsterdam+ uses the unweighted EIP-7976 floor (16 gas/token = 64 gas/byte)
759+
/// and folds EIP-7981 access-list data bytes into the token count. Pre-
760+
/// Amsterdam uses the weighted EIP-7623 formula.
761+
///
762+
/// A mismatch between this and `VM::get_min_gas_used` would cause mempool
763+
/// admission to drift from VM rejection; keep the two in sync. The
764+
/// `test_intrinsic_parity_*` suite also guards this.
765+
pub fn intrinsic_gas_floor(tx: &Transaction, fork: Fork) -> Result<u64, VMError> {
766+
// EIP-7976: floor tokens count ALL calldata bytes unweighted. For CREATE
767+
// txs the calldata is the init code. Mirrors `get_min_gas_used`.
768+
let calldata = tx.data();
769+
770+
let mut tokens_in_calldata: u64 = if fork >= Fork::Amsterdam {
771+
let total_bytes: u64 = calldata
772+
.len()
773+
.try_into()
774+
.map_err(|_| InternalError::TypeConversion)?;
775+
total_bytes
776+
.checked_mul(STANDARD_TOKEN_COST)
777+
.ok_or(InternalError::Overflow)?
778+
} else {
779+
gas_cost::tx_calldata(calldata)? / STANDARD_TOKEN_COST
780+
};
781+
782+
if fork >= Fork::Amsterdam {
783+
let al_floor_tokens = floor_tokens_in_access_list(tx.access_list());
784+
tokens_in_calldata = tokens_in_calldata
785+
.checked_add(al_floor_tokens)
786+
.ok_or(InternalError::Overflow)?;
787+
}
788+
789+
tokens_in_calldata
790+
.checked_mul(total_cost_floor_per_token(fork))
791+
.ok_or(InternalError::Overflow)?
792+
.checked_add(TX_BASE_COST)
793+
.ok_or(InternalError::Overflow.into())
794+
}
795+
753796
/// Converts Account to LevmAccount
754797
/// The problem with this is that we don't have the storage root.
755798
pub fn account_to_levm_account(account: Account) -> (LevmAccount, Code) {

crates/vm/lib.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@ pub use ethrex_levm::precompiles::{PrecompileCache, precompiles_for_fork};
1616
/// EIP-8037 intrinsic gas split `(regular, state)` for a transaction.
1717
/// Re-exported for mempool / payload-builder use.
1818
pub use ethrex_levm::utils::intrinsic_gas_dimensions;
19+
/// EIP-7623/7976/7981 floor gas for a transaction. Re-exported so the mempool
20+
/// can match the VM's `validate_min_gas_limit` check at admission time.
21+
pub use ethrex_levm::utils::intrinsic_gas_floor;
1922
pub use execution_result::ExecutionResult;
2023
pub use witness_db::GuestProgramStateWrapper;
2124
pub mod system_contracts;

0 commit comments

Comments
 (0)