Skip to content

Commit

Permalink
reject messages with invalid limits:
Browse files Browse the repository at this point in the history
- gas fee cap below minimal base fee.
- gas limit above maximum block gas limit.
  • Loading branch information
raulk committed Jan 2, 2025
1 parent 7e4b87c commit 60236b8
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 22 deletions.
24 changes: 22 additions & 2 deletions fendermint/vm/interpreter/src/chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ use fvm_ipld_encoding::RawBytes;
use fvm_shared::clock::ChainEpoch;
use fvm_shared::econ::TokenAmount;
use num_traits::Zero;
use std::any::Any;
use std::sync::Arc;

/// A resolution pool for bottom-up and top-down checkpoints.
Expand Down Expand Up @@ -190,7 +191,7 @@ where
msgs: Vec<Self::Message>,
) -> anyhow::Result<bool> {
let mut block_gas_usage = 0;
let base_fee = state.block_gas_tracker().base_fee();
let gas_market = state.block_gas_tracker().current_gas_market();

for msg in msgs {
match msg {
Expand Down Expand Up @@ -229,7 +230,9 @@ where
}
}
ChainMessage::Signed(signed) => {
if &signed.message.gas_fee_cap < base_fee {
// We do not need to check against the minimium base fee because the gas market
// is guaranteed to be capped at it anyway.
if signed.message.gas_fee_cap < gas_market.base_fee {
// We do not accept blocks containing transactions with gas parameters below the current base fee.
// Producing an invalid block like this should penalize the validator going forward.
return Ok(false);
Expand Down Expand Up @@ -469,6 +472,7 @@ impl<I, DB> CheckInterpreter for ChainMessageInterpreter<I, DB>
where
DB: Blockstore + Clone + 'static + Send + Sync,
I: CheckInterpreter<Message = VerifiableMessage, Output = SignedMessageCheckRes>,
I::State: Any,
{
type State = I::State;
type Message = ChainMessage;
Expand All @@ -482,6 +486,22 @@ where
) -> anyhow::Result<(Self::State, Self::Output)> {
match msg {
ChainMessage::Signed(msg) => {
// TODO The recursive, generic Interpreter design is extremely inflexible, and ultimately useless.
// We will untangle this later (see https://github.com/consensus-shipyard/ipc/issues/1241).
// In the meantime we are compelled to downcast to access the actual types in use.
// This check cannot be performed in the last interpreter (which has access to the FvmExecState) because
// that one returns an exit code, whereas illegal messages do not lead to any execution and therefore
// they do not benefit from an exit code.
let fvm_exec_state = (&state as &dyn Any)
.downcast_ref::<FvmExecState<DB>>()
.context("inner state not downcastable to FvmExecState")?;
let gas_market = fvm_exec_state.block_gas_tracker().current_gas_market();
if msg.message.gas_limit > gas_market.block_gas_limit
|| msg.message.gas_fee_cap < gas_market.min_base_fee
{
return Ok((state, Err(IllegalMessage)));
}

let (state, ret) = self
.inner
.check(state, VerifiableMessage::Signed(msg), is_recheck)
Expand Down
30 changes: 11 additions & 19 deletions fendermint/vm/interpreter/src/fvm/gas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,39 +15,31 @@ use num_traits::Zero;

#[derive(Debug, Clone)]
pub struct BlockGasTracker {
/// The current base fee.
base_fee: TokenAmount,
/// The current block gas limit.
block_gas_limit: Gas,
/// The currently active gas market reading.
current_gas_market: Reading,
/// The cumulative gas premiums claimable by the block producer.
cumul_gas_premium: TokenAmount,
/// The accumulated gas usage throughout the block.
cumul_gas_used: Gas,
}

impl BlockGasTracker {
pub fn base_fee(&self) -> &TokenAmount {
&self.base_fee
pub fn current_gas_market(&self) -> &Reading {
&self.current_gas_market
}

pub fn create<E: Executor>(executor: &mut E) -> anyhow::Result<BlockGasTracker> {
let mut ret = Self {
base_fee: Zero::zero(),
block_gas_limit: Zero::zero(),
Self::read_gas_market(executor).map(|reading| Self {
current_gas_market: reading,
cumul_gas_premium: Zero::zero(),
cumul_gas_used: Zero::zero(),
};

let reading = Self::read_gas_market(executor)?;

ret.base_fee = reading.base_fee;
ret.block_gas_limit = reading.block_gas_limit;

Ok(ret)
})
}

pub fn available(&self) -> Gas {
self.block_gas_limit.saturating_sub(self.cumul_gas_used)
self.current_gas_market
.block_gas_limit
.saturating_sub(self.cumul_gas_used)
}

pub fn ensure_sufficient_gas(&self, msg: &FvmMessage) -> anyhow::Result<()> {
Expand All @@ -66,7 +58,7 @@ impl BlockGasTracker {
self.cumul_gas_used = self.cumul_gas_used.saturating_add(ret.msg_receipt.gas_used);

// sanity check, should not happen; only trace if it does so we can debug later.
if self.cumul_gas_used >= self.block_gas_limit {
if self.cumul_gas_used >= self.current_gas_market.block_gas_limit {
tracing::warn!("out of block gas; cumulative gas used exceeds block gas limit!");
}
}
Expand Down
2 changes: 1 addition & 1 deletion fendermint/vm/interpreter/src/fvm/state/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ where
let mut executor = DefaultExecutor::new(engine.clone(), machine)?;

let block_gas_tracker = BlockGasTracker::create(&mut executor)?;
let base_fee = block_gas_tracker.base_fee().clone();
let base_fee = block_gas_tracker.current_gas_market().base_fee.clone();

Ok(Self {
executor,
Expand Down

0 comments on commit 60236b8

Please sign in to comment.