Skip to content

feat(metering): replace state root time with state root gas in priority fee estimation#1934

Draft
niran wants to merge 1 commit intomainfrom
worktree-metering-state-root-gas
Draft

feat(metering): replace state root time with state root gas in priority fee estimation#1934
niran wants to merge 1 commit intomainfrom
worktree-metering-state-root-gas

Conversation

@niran
Copy link
Copy Markdown
Contributor

@niran niran commented Apr 4, 2026

Summary

  • Aligns meteredPriorityFeePerGas with the builder's state root gas model (sr_gas = gas_used × (1 + K × max(0, SR_ms - anchor_ms))) instead of using raw state root time as the resource dimension
  • Adds shared compute_state_root_gas and StateRootGasConfig to base-bundles so both builder and metering use one implementation
  • Renames ResourceKind::StateRootTimeStateRootGas, updates cache, collector, estimator, extension, and node CLI accordingly
  • New CLI args: --metering.state-root-gas-limit, --metering.state-root-gas-coefficient, --metering.state-root-gas-anchor-us

Test plan

  • All 86 base-metering lib tests pass
  • All 47 base-builder-core lib tests pass
  • All 36 base-bundles tests pass
  • Clippy and fmt clean
  • Verify priority fee estimation behavior on devnet with state root gas limits configured

type=routine
risk=low
impact=sev5

…ty fee estimation

The builder already uses state root gas (a synthetic resource derived from
gas_used and SR time) instead of raw state root time for block building
budgets. This aligns the metering/estimation side to use the same metric.

Adds a shared `compute_state_root_gas` function and `StateRootGasConfig`
to `base-bundles` so both builder and metering use one implementation.
@cb-heimdall
Copy link
Copy Markdown
Collaborator

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 4, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
base Ignored Ignored Apr 4, 2026 2:56pm

Request Review

let excess_us = state_root_time_us.saturating_sub(config.anchor_us);
let excess_ms = excess_us as f64 / 1000.0;
let multiplier = 1.0 + config.coefficient * excess_ms;
(gas_used as f64 * multiplier) as u64
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coefficient field is user-configurable via --metering.state-root-gas-coefficient with no validation. If a negative value is passed, multiplier can go below zero (e.g. with large excess_ms), and the as u64 cast would silently clamp the result to 0. Consider either:

  1. Validating coefficient >= 0.0 and !coefficient.is_nan() at CLI parse / config construction time, or
  2. Clamping multiplier to >= 1.0 here defensively (matching the formula's intent that penalty can only inflate gas).

This also applies to anchor_us — a u128 can be set to absurdly large values that would zero out the penalty for all transactions.

/// Computes state root gas from gas used and state root time.
///
/// `sr_gas = gas_used × (1 + K × max(0, SR_ms - anchor_ms))`
pub fn compute_state_root_gas(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the project conventions (CLAUDE.md): "Prefer placing functions as methods on a type rather than as bare functions, so the public API exports types, not loose functions."

This should be a method on StateRootGasConfig:

Suggested change
pub fn compute_state_root_gas(
impl StateRootGasConfig {
/// Computes state root gas from gas used and state root time.
///
/// `sr_gas = gas_used × (1 + K × max(0, SR_ms - anchor_ms))`
pub fn compute(&self, gas_used: u64, state_root_time_us: u128) -> u64 {
let excess_us = state_root_time_us.saturating_sub(self.anchor_us);
let excess_ms = excess_us as f64 / 1000.0;
let multiplier = 1.0 + self.coefficient * excess_ms;
(gas_used as f64 * multiplier) as u64
}
}

This would change call sites from compute_state_root_gas(gas, time, &config) to config.compute(gas, time), and removes a bare function from the public API.

Comment on lines +70 to +76
/// State root gas coefficient (K) for priority fee estimation.
#[arg(long = "metering.state-root-gas-coefficient", default_value = "0.02")]
pub metering_state_root_gas_coefficient: f64,

/// State root gas anchor in microseconds for priority fee estimation.
#[arg(long = "metering.state-root-gas-anchor-us", default_value = "5000")]
pub metering_state_root_gas_anchor_us: u128,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two CLI args have default_value but no requires = "enable_metering", unlike every other metering.* arg. They'll be silently accepted even without --enable-metering. Since they're only meaningful when metering is active, they should either:

  • Add requires = "enable_metering" (but that conflicts with having a default — clap would require the flag to be explicitly set), or
  • Be Option<f64> / Option<u128> with requires = "enable_metering", falling back to the StateRootGasConfig::default() values when None.

As-is, passing --metering.state-root-gas-coefficient 0.05 without --enable-metering is silently accepted, which could confuse operators into thinking they've configured something.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 4, 2026

Review Summary

The refactor cleanly extracts the state-root-gas formula into a shared compute_state_root_gas function in base-bundles, replacing the previous StateRootTime resource dimension with StateRootGas throughout the metering and builder stacks. The mechanical rename and plumbing are consistent across all touched files.

Findings

  1. compute_state_root_gas should be a method on StateRootGasConfig (crates/common/bundles/src/meter.rs:30): Project conventions prefer methods on types over bare functions in the public API. config.compute(gas, time) reads better than compute_state_root_gas(gas, time, &config).

  2. No input validation on StateRootGasConfig fields (crates/common/bundles/src/meter.rs:38): The coefficient (f64) is user-configurable via CLI with no validation. Negative, NaN, or infinity values would produce incorrect results (silently clamped to 0 by as u64). The function's formula assumes multiplier >= 1.0 but doesn't enforce it.

  3. CLI args --metering.state-root-gas-coefficient and --metering.state-root-gas-anchor-us lack requires = "enable_metering" (bin/node/src/cli.rs:70-76): Unlike all other metering.* args, these are accepted without --enable-metering, which could confuse operators.

No correctness issues found in the core logic — the formula extraction preserves the original behavior from context.rs, and the StateRootGas resource kind is correctly wired through the cache, collector, estimator, and RPC layers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants