From 56c26953ff7d3231f53b3d3fe7a312bcb739fea7 Mon Sep 17 00:00:00 2001 From: Mikers Date: Fri, 14 Nov 2025 12:13:50 -1000 Subject: [PATCH 01/12] feat: add engine-managed gas reservations to ref-fvm --- fvm/src/call_manager/default.rs | 48 +- fvm/src/call_manager/mod.rs | 4 + fvm/src/executor/default.rs | 1399 ++++++++++++++++- fvm/src/executor/mod.rs | 34 +- fvm/src/executor/telemetry.rs | 128 ++ fvm/src/kernel/default.rs | 11 +- fvm/tests/dummy.rs | 3 + .../tests/reservation_transfer_enforcement.rs | 192 +++ 8 files changed, 1793 insertions(+), 26 deletions(-) create mode 100644 fvm/src/executor/telemetry.rs create mode 100644 testing/integration/tests/reservation_transfer_enforcement.rs diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index ed56939ac..cb58c2960 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -1,6 +1,7 @@ // Copyright 2021-2023 Protocol Labs // SPDX-License-Identifier: Apache-2.0, MIT use std::rc::Rc; +use std::sync::{Arc, Mutex}; use anyhow::{Context, anyhow}; use cid::Cid; @@ -33,6 +34,7 @@ use crate::syscalls::error::Abort; use crate::syscalls::{charge_for_exec, update_gas_available}; use crate::trace::{ExecutionEvent, ExecutionTrace}; use crate::{syscall_error, system_actor}; +use crate::executor::ReservationSession; /// The default [`CallManager`] implementation. #[repr(transparent)] @@ -77,6 +79,8 @@ pub struct InnerDefaultCallManager { events: EventsAccumulator, /// The actor call stack (ActorID and entrypoint name tuple). actor_call_stack: Vec<(ActorID, &'static str)>, + /// Shared reservation session ledger for the current tipset, if any. + reservation_session: Arc>, } #[doc(hidden)] @@ -111,6 +115,7 @@ where receiver_address: Address, nonce: u64, gas_premium: TokenAmount, + reservation_session: Arc>, ) -> Self { let limits = machine.new_limiter(); let gas_tracker = @@ -162,6 +167,7 @@ where events: Default::default(), state_access_tracker, actor_call_stack: vec![], + reservation_session, }))) } @@ -489,8 +495,46 @@ where .get_actor(from)? .ok_or_else(||syscall_error!(InsufficientFunds; "insufficient funds to transfer {value}FIL from {from} to {to})"))?; - if &from_actor.balance < value { - return Err(syscall_error!(InsufficientFunds; "sender does not have funds to transfer (balance {}, transfer {})", &from_actor.balance, value).into()); + // In reservation mode, ensure the sender cannot spend funds that are reserved for gas. + // Free balance is defined as balance - reserved_remaining. To avoid negative intermediates, + // we enforce the equivalent inequality: value + reserved_remaining <= balance. + let (reservation_open, reserved_remaining) = { + let session = self + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + if session.open { + let reserved = session + .reservations + .get(&from) + .cloned() + .unwrap_or_else(TokenAmount::zero); + (true, reserved) + } else { + (false, TokenAmount::zero()) + } + }; + + if reservation_open { + let required = &reserved_remaining + value; + if &from_actor.balance < &required { + return Err(syscall_error!( + InsufficientFunds; + "sender does not have free funds to transfer (balance {}, transfer {}, reserved {})", + &from_actor.balance, + value, + &reserved_remaining + ) + .into()); + } + } else if &from_actor.balance < value { + return Err(syscall_error!( + InsufficientFunds; + "sender does not have funds to transfer (balance {}, transfer {})", + &from_actor.balance, + value + ) + .into()); } if from == to { diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index a315741e9..a982b058a 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -1,5 +1,7 @@ // Copyright 2021-2023 Protocol Labs // SPDX-License-Identifier: Apache-2.0, MIT +use std::sync::{Arc, Mutex}; + use cid::Cid; use fvm_ipld_encoding::{CBOR, to_vec}; use fvm_shared::address::Address; @@ -13,6 +15,7 @@ use crate::engine::Engine; use crate::gas::{Gas, GasCharge, GasTimer, GasTracker, PriceList}; use crate::kernel::{self, BlockRegistry, ClassifyResult, Context, Result}; use crate::machine::{Machine, MachineContext}; +use crate::executor::ReservationSession; use crate::state_tree::ActorState; pub mod backtrace; @@ -60,6 +63,7 @@ pub trait CallManager: 'static { receiver_address: Address, nonce: u64, gas_premium: TokenAmount, + reservation_session: Arc>, ) -> Self; /// Calls an actor at the given address and entrypoint. The type parameter `K` specifies the the _kernel_ on top of which the target diff --git a/fvm/src/executor/default.rs b/fvm/src/executor/default.rs index 949ff5f46..4a73d4b0c 100644 --- a/fvm/src/executor/default.rs +++ b/fvm/src/executor/default.rs @@ -1,12 +1,14 @@ // Copyright 2021-2023 Protocol Labs // SPDX-License-Identifier: Apache-2.0, MIT +use std::collections::{HashMap, hash_map::Entry}; use std::ops::{Deref, DerefMut}; use std::result::Result as StdResult; +use std::sync::{Arc, Mutex}; use anyhow::{Result, anyhow}; use cid::Cid; use fvm_ipld_encoding::{CBOR, RawBytes}; -use fvm_shared::address::Payload; +use fvm_shared::address::{Address, Payload}; use fvm_shared::econ::TokenAmount; use fvm_shared::error::{ErrorNumber, ExitCode}; use fvm_shared::event::StampedEvent; @@ -15,15 +17,31 @@ use fvm_shared::receipt::Receipt; use fvm_shared::{ActorID, IPLD_RAW, METHOD_SEND}; use num_traits::Zero; -use super::{ApplyFailure, ApplyKind, ApplyRet, Executor}; +use super::{ApplyFailure, ApplyKind, ApplyRet, Executor, ReservationError}; use crate::call_manager::{Backtrace, CallManager, Entrypoint, InvocationResult, backtrace}; use crate::eam_actor::EAM_ACTOR_ID; use crate::engine::EnginePool; +use crate::executor::telemetry; use crate::gas::{Gas, GasCharge, GasOutputs}; use crate::kernel::{Block, ClassifyResult, Context as _, ExecutionError, Kernel}; use crate::machine::{BURNT_FUNDS_ACTOR_ID, Machine, REWARD_ACTOR_ID}; use crate::trace::ExecutionTrace; +pub use self::reservation::ReservationSession; + +mod reservation { + use std::collections::HashMap; + + use fvm_shared::econ::TokenAmount; + use fvm_shared::ActorID; + + #[derive(Default)] + pub struct ReservationSession { + pub reservations: HashMap, + pub open: bool, + } +} + /// The default [`Executor`]. /// /// # Warning @@ -35,6 +53,7 @@ pub struct DefaultExecutor { engine_pool: EnginePool, // If the inner value is `None` it means the machine got poisoned and is unusable. machine: Option<::Machine>, + reservation_session: Arc>, } impl Deref for DefaultExecutor { @@ -99,6 +118,7 @@ where let engine = self.engine_pool.acquire(); // Apply the message. + let reservation_session = self.reservation_session.clone(); let ret = self.map_machine(|machine| { // We're processing a chain message, so the sender is the origin of the call stack. let mut cm = K::CallManager::new( @@ -111,6 +131,7 @@ where msg.to, msg.sequence, effective_premium, + reservation_session.clone(), ); // This error is fatal because it should have already been accounted for inside // preflight_message. @@ -315,6 +336,7 @@ where Ok(Self { engine_pool, machine: Some(machine), + reservation_session: Arc::new(Mutex::new(ReservationSession::default())), }) } @@ -324,6 +346,79 @@ where self.machine } + /// Assert that the current reservation session, if any, fully covers the gas cost for the + /// specified sender. A coverage violation indicates a host/engine invariant breach and is + /// treated as a fatal error by callers. + fn reservation_assert_coverage( + &self, + sender: ActorID, + gas_cost: &TokenAmount, + ) -> StdResult<(), ReservationError> { + let session = self + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + + if !session.open { + return Ok(()); + } + + let reserved = session + .reservations + .get(&sender) + .cloned() + .unwrap_or_else(TokenAmount::zero); + + if reserved < *gas_cost { + return Err(ReservationError::ReservationInvariant(format!( + "reserved total for sender {} ({}) below gas cost ({})", + sender, reserved, gas_cost + ))); + } + + Ok(()) + } + + /// Decrement the reservation for the given sender on a prevalidation failure. This keeps the + /// session ledger consistent even when the message never executes. + fn reservation_prevalidation_decrement( + &mut self, + sender: ActorID, + gas_cost: &TokenAmount, + ) -> StdResult<(), ReservationError> { + let mut session = self + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + + if !session.open { + return Ok(()); + } + + match session.reservations.entry(sender) { + Entry::Occupied(mut entry) => { + let current = entry.get().clone(); + if current < *gas_cost { + return Err(ReservationError::Overflow); + } + let remaining = current - gas_cost.clone(); + if remaining.is_zero() { + entry.remove(); + } else { + *entry.get_mut() = remaining.clone(); + } + + // Keep the reserved_remaining_gauge{sender} telemetry in sync with the ledger. + telemetry::reservation_remaining_update(sender, &remaining); + Ok(()) + } + Entry::Vacant(_) => Err(ReservationError::ReservationInvariant(format!( + "no reservation entry for sender {} on prevalidation failure", + sender + ))), + } + } + // TODO: The return type here is very strange because we have three cases: // 1. Continue: Return sender ID, & gas. // 2. Short-circuit: Return ApplyRet. @@ -340,27 +435,23 @@ where // TODO We don't like having price lists _inside_ the FVM, but passing // these across the boundary is also a no-go. let pl = &self.context().price_list; + let reservation_mode = self + .reservation_session + .lock() + .expect("reservation session mutex poisoned") + .open; - let (inclusion_cost, miner_penalty_amount) = match apply_kind { + let (inclusion_cost, inclusion_total, miner_penalty_amount) = match apply_kind { ApplyKind::Implicit => ( GasCharge::new("none", Gas::zero(), Gas::zero()), + None, Default::default(), ), ApplyKind::Explicit => { let inclusion_cost = pl.on_chain_message(raw_length); let inclusion_total = inclusion_cost.total().round_up(); - - // Verify the cost of the message is not over the message gas limit. - if inclusion_total > msg.gas_limit { - return Ok(Err(ApplyRet::prevalidation_fail( - ExitCode::SYS_OUT_OF_GAS, - format!("Out of gas ({} > {})", inclusion_total, msg.gas_limit), - &self.context().base_fee * inclusion_total, - ))); - } - let miner_penalty_amount = &self.context().base_fee * msg.gas_limit; - (inclusion_cost, miner_penalty_amount) + (inclusion_cost, Some(inclusion_total), miner_penalty_amount) } }; @@ -372,6 +463,10 @@ where { Some(id) => id, None => { + // If we can't resolve the sender address to an actor ID, this is a + // prevalidation failure. Reservation sessions (when active) are keyed + // by ActorID, so this case should have been rejected when building the + // reservation plan. return Ok(Err(ApplyRet::prevalidation_fail( ExitCode::SYS_SENDER_INVALID, "Sender invalid", @@ -384,6 +479,31 @@ where return Ok(Ok((sender_id, TokenAmount::zero(), inclusion_cost))); } + // Compute the maximum gas cost this message can charge. This uses big-int arithmetic and + // is expected not to overflow; a negative result would indicate an arithmetic bug. + let gas_cost: TokenAmount = msg.gas_fee_cap.clone() * msg.gas_limit; + if gas_cost.is_negative() { + return Err(ReservationError::Overflow.into()); + } + + // Verify the cost of the message is not over the message gas limit. In reservation mode we + // must also decrement the reservation for this message so the session can end at zero. + if let Some(inclusion_total) = inclusion_total { + if inclusion_total > msg.gas_limit { + if reservation_mode { + self.reservation_prevalidation_decrement(sender_id, &gas_cost)?; + } + return Ok(Err(ApplyRet::prevalidation_fail( + ExitCode::SYS_OUT_OF_GAS, + format!( + "Out of gas ({} > {})", + inclusion_total, msg.gas_limit + ), + &self.context().base_fee * inclusion_total, + ))); + } + } + let mut sender_state = match self .state_tree() .get_actor(sender_id) @@ -391,6 +511,9 @@ where { Some(act) => act, None => { + if reservation_mode { + self.reservation_prevalidation_decrement(sender_id, &gas_cost)?; + } return Ok(Err(ApplyRet::prevalidation_fail( ExitCode::SYS_SENDER_INVALID, "Sender invalid", @@ -420,6 +543,9 @@ where } if !sender_is_valid { + if reservation_mode { + self.reservation_prevalidation_decrement(sender_id, &gas_cost)?; + } return Ok(Err(ApplyRet::prevalidation_fail( ExitCode::SYS_SENDER_INVALID, "Send not from valid sender", @@ -429,6 +555,9 @@ where // Check sequence is correct if msg.sequence != sender_state.sequence { + if reservation_mode { + self.reservation_prevalidation_decrement(sender_id, &gas_cost)?; + } return Ok(Err(ApplyRet::prevalidation_fail( ExitCode::SYS_SENDER_STATE_INVALID, format!( @@ -439,10 +568,22 @@ where ))); }; + // At this point the message is syntactically valid and has the correct nonce. + if reservation_mode { + // In reservation mode we assert that the ledger fully covers the maximum gas cost, but + // we _do not_ deduct funds here; settlement is handled in finish_message. + self.reservation_assert_coverage(sender_id, &gas_cost)?; + + sender_state.sequence += 1; + self.state_tree_mut().set_actor(sender_id, sender_state); + + return Ok(Ok((sender_id, gas_cost, inclusion_cost))); + } + + // Legacy behavior: ensure from actor has enough balance to cover the gas cost of the + // message and pre-deduct it from the sender balance. sender_state.sequence += 1; - // Ensure from actor has enough balance to cover the gas cost of the message. - let gas_cost: TokenAmount = msg.gas_fee_cap.clone() * msg.gas_limit; if sender_state.balance < gas_cost { return Ok(Err(ApplyRet::prevalidation_fail( ExitCode::SYS_SENDER_STATE_INVALID, @@ -473,6 +614,12 @@ where exec_trace: ExecutionTrace, events: Vec, ) -> anyhow::Result { + let reservation_mode = self + .reservation_session + .lock() + .expect("reservation session mutex poisoned") + .open; + // NOTE: we don't support old network versions in the FVM, so we always burn. let GasOutputs { base_fee_burn, @@ -504,14 +651,48 @@ where Ok(()) }; + // Pay base-fee burn, miner tip, and over-estimation burn as today. transfer_to_actor(BURNT_FUNDS_ACTOR_ID, &base_fee_burn)?; transfer_to_actor(REWARD_ACTOR_ID, &miner_tip)?; transfer_to_actor(BURNT_FUNDS_ACTOR_ID, &over_estimation_burn)?; - // refund unused gas - transfer_to_actor(sender_id, &refund)?; + if reservation_mode { + // In reservation mode we net-charge the sender for the actual gas consumption and + // realize the refund by releasing the reservation instead of depositing it. + let consumption = &base_fee_burn + &over_estimation_burn + &miner_tip; + + self.state_tree_mut() + .mutate_actor(sender_id, |act| { + act.deduct_funds(&consumption).or_fatal() + }) + .context("failed to lookup sender actor for settlement")?; + + // Decrement this message's reservation; underflow or a missing entry indicates a fatal + // reservation invariant breach. + self.reservation_prevalidation_decrement(sender_id, &gas_cost)?; + + // Track settlement metrics, including the virtual refund realized via reservation + // release. + telemetry::settlement_record( + &base_fee_burn, + &miner_tip, + &over_estimation_burn, + Some(&refund), + ); + } else { + // Legacy behavior: refund unused gas directly to the sender. + transfer_to_actor(sender_id, &refund)?; + + // Track settlement metrics in legacy mode as well, without a virtual refund component. + telemetry::settlement_record( + &base_fee_burn, + &miner_tip, + &over_estimation_burn, + None, + ); + } if (&base_fee_burn + &over_estimation_burn + &refund + &miner_tip) != gas_cost { // Sanity check. This could be a fatal error. @@ -547,4 +728,1186 @@ where }, ) } + + /// Begin a tipset-scope gas reservation session from a per-sender plan. + /// + /// The plan is keyed by address; this method resolves each address to an ActorID and + /// aggregates per-actor totals before checking affordability. + pub fn begin_reservation_session( + &mut self, + plan: &[(Address, TokenAmount)], + ) -> StdResult<(), ReservationError> { + // Empty plan is a no-op and must not enter reservation mode. + if plan.is_empty() { + return Ok(()); + } + + const MAX_SENDERS: usize = 65_536; + if plan.len() > MAX_SENDERS { + telemetry::reservation_begin_failed(); + return Err(ReservationError::PlanTooLarge); + } + + let mut session = self + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + + if session.open { + telemetry::reservation_begin_failed(); + return Err(ReservationError::SessionOpen); + } + + // Aggregate per-actor reservations. + let mut reservations: HashMap = HashMap::new(); + + for (addr, amount) in plan { + // Resolve address to ActorID via the state tree. + let sender_id = self + .state_tree() + .lookup_id(addr) + .map_err(|e| { + ReservationError::ReservationInvariant(format!( + "failed to lookup actor {addr}: {e}" + )) + })? + .ok_or_else(|| { + ReservationError::ReservationInvariant(format!( + "failed to resolve address {addr} to actor ID" + )) + })?; + + if amount.is_zero() { + continue; + } + + reservations + .entry(sender_id) + .and_modify(|total| *total += amount.clone()) + .or_insert_with(|| amount.clone()); + } + + // Check affordability per sender: Σ(plan) ≤ actor.balance. + for (actor_id, reserved) in &reservations { + let actor_state = self + .state_tree() + .get_actor(*actor_id) + .map_err(|e| { + ReservationError::ReservationInvariant(format!( + "failed to load actor {actor_id}: {e}" + )) + })? + .ok_or_else(|| { + ReservationError::ReservationInvariant(format!( + "reservation plan includes unknown actor {actor_id}" + )) + })?; + + if &actor_state.balance < reserved { + telemetry::reservation_begin_failed(); + return Err(ReservationError::InsufficientFundsAtBegin { + sender: *actor_id, + }); + } + } + + telemetry::reservation_begin_succeeded(&reservations); + + session.reservations = reservations; + session.open = true; + Ok(()) + } + + /// End the active reservation session, ensuring the ledger has returned to zero. + pub fn end_reservation_session(&mut self) -> StdResult<(), ReservationError> { + let mut session = self + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + + if !session.open { + return Err(ReservationError::SessionClosed); + } + + let has_non_zero = session.reservations.values().any(|amt| !amt.is_zero()); + + if has_non_zero { + return Err(ReservationError::NonZeroRemainder); + } + + session.reservations.clear(); + session.open = false; + + telemetry::reservation_end_succeeded(); + + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use cid::Cid; + use fvm_ipld_blockstore::MemoryBlockstore; + use fvm_ipld_encoding::{CborStore, DAG_CBOR, RawBytes}; + use fvm_shared::address::Address; + use fvm_shared::error::ExitCode; + use fvm_shared::state::{ActorState, StateTreeVersion}; + use fvm_shared::IDENTITY_HASH; + use multihash_codetable::{Code, Multihash}; + + use crate::call_manager::DefaultCallManager; + use crate::engine::EnginePool; + use crate::externs::{Chain, Consensus, Externs, Rand}; + use crate::call_manager::NO_DATA_BLOCK_ID; + use crate::kernel::filecoin::DefaultFilecoinKernel; + use crate::kernel::default::DefaultKernel; + use crate::kernel::{BlockRegistry, SelfOps, SendOps}; + use crate::machine::{DefaultMachine, Manifest, NetworkConfig}; + use crate::state_tree::StateTree; + use fvm_shared::sys::SendFlags; + + struct DummyExterns; + + impl Externs for DummyExterns {} + + impl Rand for DummyExterns { + fn get_chain_randomness( + &self, + round: fvm_shared::clock::ChainEpoch, + ) -> anyhow::Result<[u8; 32]> { + let msg = "reservation-test".as_bytes(); + let mut out = [0u8; 32]; + out[..msg.len()].copy_from_slice(msg); + // Make the randomness depend on the round so tests can distinguish calls. + out[31] ^= (round as u8).wrapping_mul(31); + Ok(out) + } + + fn get_beacon_randomness( + &self, + _round: fvm_shared::clock::ChainEpoch, + ) -> anyhow::Result<[u8; 32]> { + Ok([0u8; 32]) + } + } + + impl Consensus for DummyExterns { + fn verify_consensus_fault( + &self, + _h1: &[u8], + _h2: &[u8], + _extra: &[u8], + ) -> anyhow::Result<(Option, i64)> { + Ok((None, 0)) + } + } + + impl Chain for DummyExterns { + fn get_tipset_cid( + &self, + epoch: fvm_shared::clock::ChainEpoch, + ) -> anyhow::Result { + Ok(Cid::new_v1( + DAG_CBOR, + Multihash::wrap(IDENTITY_HASH, &epoch.to_be_bytes()).unwrap(), + )) + } + } + + type TestMachine = Box>; + type TestKernel = DefaultFilecoinKernel>; + type TestCallManager = ::CallManager; + type TestExecutor = DefaultExecutor; + + fn new_executor_with_base_fee(base_fee: TokenAmount) -> TestExecutor { + // Construct an empty state-tree and machine, mirroring the lib.rs constructor test, but + // overriding the base-fee so settlement paths exercise non-trivial gas outputs. + let mut bs = MemoryBlockstore::default(); + let mut st = StateTree::new(bs, StateTreeVersion::V5).unwrap(); + let root = st.flush().unwrap(); + bs = st.into_store(); + + // An empty built-in actors manifest. + let manifest_cid = bs.put_cbor(&Manifest::DUMMY_CODES, Code::Blake2b256).unwrap(); + let actors_cid = bs.put_cbor(&(1, manifest_cid), Code::Blake2b256).unwrap(); + + let mut net_cfg = + NetworkConfig::new(fvm_shared::version::NetworkVersion::V21); + net_cfg.override_actors(actors_cid); + let mut mc = net_cfg.for_epoch(0, 0, root); + mc.set_base_fee(base_fee); + + let machine = DefaultMachine::new(&mc, bs, DummyExterns).unwrap(); + let engine = EnginePool::new((&mc.network).into()).unwrap(); + + TestExecutor::new(engine, Box::new(machine)).unwrap() + } + + fn new_executor() -> TestExecutor { + // Construct an empty state-tree and machine, mirroring the lib.rs constructor test. + let mut bs = MemoryBlockstore::default(); + let mut st = StateTree::new(bs, StateTreeVersion::V5).unwrap(); + let root = st.flush().unwrap(); + bs = st.into_store(); + + // An empty built-in actors manifest. + let manifest_cid = bs.put_cbor(&Manifest::DUMMY_CODES, Code::Blake2b256).unwrap(); + let actors_cid = bs.put_cbor(&(1, manifest_cid), Code::Blake2b256).unwrap(); + + let mc = NetworkConfig::new(fvm_shared::version::NetworkVersion::V21) + .override_actors(actors_cid) + .for_epoch(0, 0, root); + + let machine = DefaultMachine::new(&mc, bs, DummyExterns).unwrap(); + let engine = EnginePool::new((&mc.network).into()).unwrap(); + + TestExecutor::new(engine, Box::new(machine)).unwrap() + } + + fn new_executor_with_actor( + id: ActorID, + balance: TokenAmount, + ) -> TestExecutor { + let mut exec = new_executor(); + + let account_code = *exec.builtin_actors().get_account_code(); + let mut actor = ActorState::new_empty(account_code, None); + actor.balance = balance; + exec.state_tree_mut().set_actor(id, actor); + + exec + } + + #[test] + fn begin_empty_plan_is_noop() { + let mut exec = new_executor(); + + { + let session = exec + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + assert!(!session.open); + assert!(session.reservations.is_empty()); + } + + exec.begin_reservation_session(&[]).unwrap(); + + { + let session = exec + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + assert!(!session.open); + assert!(session.reservations.is_empty()); + } + + // Ending without an open session yields SessionClosed. + assert_eq!( + exec.end_reservation_session().unwrap_err(), + ReservationError::SessionClosed + ); + } + + #[test] + fn begin_and_end_with_zero_remainder_succeeds() { + let sender: ActorID = 1000; + let mut exec = + new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); + + let plan = vec![(Address::new_id(sender), TokenAmount::from_atto(500u64))]; + + exec.begin_reservation_session(&plan).unwrap(); + { + let mut session = exec + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + assert!(session.open); + assert_eq!( + session + .reservations + .get(&sender) + .cloned() + .unwrap(), + TokenAmount::from_atto(500u64) + ); + + // Simulate full consumption of all reservations so the session can end cleanly. + for amt in session.reservations.values_mut() { + *amt = TokenAmount::zero(); + } + } + + exec.end_reservation_session().unwrap(); + { + let session = exec + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + assert!(!session.open); + assert!(session.reservations.is_empty()); + } + } + + #[test] + fn begin_twice_errors_with_session_open() { + let sender: ActorID = 42; + let mut exec = + new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); + let plan = vec![(Address::new_id(sender), TokenAmount::from_atto(100u64))]; + + exec.begin_reservation_session(&plan).unwrap(); + let err = exec.begin_reservation_session(&plan).unwrap_err(); + assert_eq!(err, ReservationError::SessionOpen); + } + + #[test] + fn end_with_non_zero_remainder_errors() { + let sender: ActorID = 7; + let mut exec = + new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); + let plan = vec![(Address::new_id(sender), TokenAmount::from_atto(1234u64))]; + + exec.begin_reservation_session(&plan).unwrap(); + // Reservations are still non-zero, so ending should fail. + let err = exec.end_reservation_session().unwrap_err(); + assert_eq!(err, ReservationError::NonZeroRemainder); + } + + #[test] + fn plan_too_large_by_sender_count() { + let mut exec = new_executor(); + + const MAX_SENDERS: usize = 65_536; + let mut plan = Vec::with_capacity(MAX_SENDERS + 1); + for i in 0..=MAX_SENDERS { + plan.push((Address::new_id(i as u64), TokenAmount::from_atto(1u64))); + } + + let err = exec.begin_reservation_session(&plan).unwrap_err(); + assert_eq!(err, ReservationError::PlanTooLarge); + } + + #[test] + fn insufficient_funds_at_begin() { + let sender: ActorID = 5; + let mut exec = + new_executor_with_actor(sender, TokenAmount::from_atto(10u64)); + let plan = vec![(Address::new_id(sender), TokenAmount::from_atto(11u64))]; + + let err = exec.begin_reservation_session(&plan).unwrap_err(); + assert_eq!( + err, + ReservationError::InsufficientFundsAtBegin { sender } + ); + } + + #[test] + fn unknown_actor_in_plan_yields_reservation_invariant() { + let mut exec = new_executor(); + let sender: ActorID = 9999; + let plan = vec![(Address::new_id(sender), TokenAmount::from_atto(1u64))]; + + let err = exec.begin_reservation_session(&plan).unwrap_err(); + match err { + ReservationError::ReservationInvariant(msg) => { + assert!(msg.contains(&format!("unknown actor {}", sender))); + } + other => panic!( + "expected ReservationInvariant for unknown actor, got {:?}", + other + ), + } + } + + #[test] + fn preflight_with_reservations_does_not_deduct_balance() { + let sender: ActorID = 1001; + let initial_balance = TokenAmount::from_atto(1_000_000u64); + let mut exec = new_executor_with_actor(sender, initial_balance.clone()); + + let raw_length = 100usize; + let pl = &exec.context().price_list; + let inclusion_cost = pl.on_chain_message(raw_length); + let inclusion_total = inclusion_cost.total().round_up(); + let gas_limit = inclusion_total + 100; + + let gas_fee_cap = TokenAmount::from_atto(1u64); + let gas_cost = gas_fee_cap.clone() * gas_limit; + + let plan = vec![(Address::new_id(sender), gas_cost.clone())]; + exec.begin_reservation_session(&plan).unwrap(); + + let msg = Message { + version: 0, + from: Address::new_id(sender), + to: Address::new_id(1), + sequence: 0, + value: TokenAmount::zero(), + method_num: 0, + params: RawBytes::default(), + gas_limit, + gas_fee_cap: gas_fee_cap.clone(), + gas_premium: TokenAmount::zero(), + }; + + let res = exec + .preflight_message(&msg, ApplyKind::Explicit, raw_length) + .unwrap(); + + let (seen_sender, seen_gas_cost, _inclusion) = + res.expect("expected successful preflight under reservations"); + assert_eq!(seen_sender, sender); + assert_eq!(seen_gas_cost, gas_cost); + + // Balance is untouched in reservation mode; only the nonce is incremented. + let actor = exec + .state_tree() + .get_actor(sender) + .unwrap() + .expect("actor must exist"); + assert_eq!(actor.balance, initial_balance); + assert_eq!(actor.sequence, 1); + + // Reservation ledger is unchanged; consumption happens during settlement. + { + let session = exec + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + assert_eq!( + session + .reservations + .get(&sender) + .cloned() + .unwrap(), + gas_cost + ); + } + } + + #[test] + fn preflight_prevalidation_failure_decrements_reservation_and_allows_zero_remainder() { + let sender: ActorID = 2000; + let initial_balance = TokenAmount::from_atto(1_000_000u64); + let mut exec = new_executor_with_actor(sender, initial_balance.clone()); + + let raw_length = 100usize; + let pl = &exec.context().price_list; + let inclusion_cost = pl.on_chain_message(raw_length); + let inclusion_total = inclusion_cost.total().round_up(); + let gas_limit = inclusion_total + 100; + + let gas_fee_cap = TokenAmount::from_atto(1u64); + let gas_cost = gas_fee_cap.clone() * gas_limit; + + exec.begin_reservation_session(&[(Address::new_id(sender), gas_cost.clone())]) + .unwrap(); + + // Use an incorrect nonce to trigger prevalidation failure. + let msg = Message { + version: 0, + from: Address::new_id(sender), + to: Address::new_id(1), + sequence: 42, + value: TokenAmount::zero(), + method_num: 0, + params: RawBytes::default(), + gas_limit, + gas_fee_cap: gas_fee_cap.clone(), + gas_premium: TokenAmount::zero(), + }; + + let res = exec + .preflight_message(&msg, ApplyKind::Explicit, raw_length) + .unwrap(); + + let apply_ret = res.expect_err("expected prevalidation failure for bad nonce"); + assert_eq!( + apply_ret.msg_receipt.exit_code, + ExitCode::SYS_SENDER_STATE_INVALID + ); + + // Actor state is unchanged on prevalidation failure. + let actor = exec + .state_tree() + .get_actor(sender) + .unwrap() + .expect("actor must exist"); + assert_eq!(actor.balance, initial_balance); + assert_eq!(actor.sequence, 0); + + // Reservation for this sender is fully released, allowing the session to end with zero + // remainder. + { + let session = exec + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + assert!(session.reservations.get(&sender).is_none()); + } + exec.end_reservation_session().unwrap(); + { + let session = exec + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + assert!(!session.open); + } + } + + #[test] + fn preflight_negative_fee_cap_yields_overflow() { + let sender: ActorID = 2500; + let mut exec = + new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); + + let raw_length = 100usize; + let pl = &exec.context().price_list; + let inclusion_cost = pl.on_chain_message(raw_length); + let inclusion_total = inclusion_cost.total().round_up(); + let gas_limit = inclusion_total + 100; + + // Construct a message with a negative gas_fee_cap so that the + // computed gas_cost is negative and triggers the overflow guard. + let gas_fee_cap = TokenAmount::from_atto(-1i64); + + let msg = Message { + version: 0, + from: Address::new_id(sender), + to: Address::new_id(1), + sequence: 0, + value: TokenAmount::zero(), + method_num: 0, + params: RawBytes::default(), + gas_limit, + gas_fee_cap: gas_fee_cap.clone(), + gas_premium: TokenAmount::zero(), + }; + + let res = exec.preflight_message(&msg, ApplyKind::Explicit, raw_length); + + match res { + Ok(_) => panic!("expected fatal overflow error for negative gas fee cap"), + Err(err) => { + let reservation_err = err + .downcast_ref::() + .expect("expected ReservationError"); + assert_eq!(reservation_err, &ReservationError::Overflow); + } + } + } + + #[test] + fn preflight_inclusion_too_low_decrements_reservation() { + let sender: ActorID = 3000; + let initial_balance = TokenAmount::from_atto(1_000_000u64); + let mut exec = new_executor_with_actor(sender, initial_balance.clone()); + + let raw_length = 100usize; + let pl = &exec.context().price_list; + let inclusion_cost = pl.on_chain_message(raw_length); + let inclusion_total = inclusion_cost.total().round_up(); + assert!(inclusion_total > 0); + let gas_limit = inclusion_total - 1; + + let gas_fee_cap = TokenAmount::from_atto(1u64); + let gas_cost = gas_fee_cap.clone() * gas_limit; + + exec.begin_reservation_session(&[(Address::new_id(sender), gas_cost.clone())]) + .unwrap(); + + let msg = Message { + version: 0, + from: Address::new_id(sender), + to: Address::new_id(1), + sequence: 0, + value: TokenAmount::zero(), + method_num: 0, + params: RawBytes::default(), + gas_limit, + gas_fee_cap: gas_fee_cap.clone(), + gas_premium: TokenAmount::zero(), + }; + + let res = exec + .preflight_message(&msg, ApplyKind::Explicit, raw_length) + .unwrap(); + + let apply_ret = + res.expect_err("expected prevalidation failure for inclusion gas too low"); + assert_eq!(apply_ret.msg_receipt.exit_code, ExitCode::SYS_OUT_OF_GAS); + + // Actor state is unchanged on prevalidation failure. + let actor = exec + .state_tree() + .get_actor(sender) + .unwrap() + .expect("actor must exist"); + assert_eq!(actor.balance, initial_balance); + assert_eq!(actor.sequence, 0); + + // Reservation is released, so the session can end with zero remainder. + { + let session = exec + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + assert!(session.reservations.get(&sender).is_none()); + } + exec.end_reservation_session().unwrap(); + } + + #[test] + fn reservation_coverage_violation_yields_reservation_invariant() { + let sender: ActorID = 4000; + let mut exec = + new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); + + let raw_length = 100usize; + let pl = &exec.context().price_list; + let inclusion_cost = pl.on_chain_message(raw_length); + let inclusion_total = inclusion_cost.total().round_up(); + let gas_limit = inclusion_total + 100; + + let gas_fee_cap = TokenAmount::from_atto(1u64); + let gas_cost = gas_fee_cap.clone() * gas_limit; + let under_reserved = &gas_cost - &TokenAmount::from_atto(1u64); + + exec.begin_reservation_session(&[(Address::new_id(sender), under_reserved.clone())]) + .unwrap(); + + let msg = Message { + version: 0, + from: Address::new_id(sender), + to: Address::new_id(1), + sequence: 0, + value: TokenAmount::zero(), + method_num: 0, + params: RawBytes::default(), + gas_limit, + gas_fee_cap: gas_fee_cap.clone(), + gas_premium: TokenAmount::zero(), + }; + + let res = exec.preflight_message(&msg, ApplyKind::Explicit, raw_length); + + match res { + Ok(_) => panic!("expected fatal error for reservation coverage violation"), + Err(err) => { + let reservation_err = err + .downcast_ref::() + .expect("expected ReservationError"); + match reservation_err { + ReservationError::ReservationInvariant(msg) => { + assert!(msg.contains("below gas cost")); + } + other => panic!("expected ReservationInvariant, got {:?}", other), + } + } + } + } + + #[test] + fn reservation_prevalidation_decrement_underflow_yields_overflow() { + let sender: ActorID = 5000; + let mut exec = + new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); + + // Manually open a reservation session with an under-sized reservation to trigger + // arithmetic underflow when we attempt to decrement it. + { + let mut session = exec + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + session.open = true; + session + .reservations + .insert(sender, TokenAmount::from_atto(1u64)); + } + + let raw_length = 100usize; + let gas_fee_cap = TokenAmount::from_atto(1u64); + let gas_limit = 2u64; + + let msg = Message { + version: 0, + from: Address::new_id(sender), + to: Address::new_id(1), + sequence: 1, + value: TokenAmount::zero(), + method_num: 0, + params: RawBytes::default(), + gas_limit, + gas_fee_cap: gas_fee_cap.clone(), + gas_premium: TokenAmount::zero(), + }; + + let res = exec.preflight_message(&msg, ApplyKind::Explicit, raw_length); + + match res { + Ok(_) => panic!("expected fatal error from reservation underflow"), + Err(err) => { + let reservation_err = err + .downcast_ref::() + .expect("expected ReservationError"); + assert_eq!(reservation_err, &ReservationError::Overflow); + } + } + } + + #[test] + fn transfer_enforces_reservations_for_message_send() { + let sender: ActorID = 6000; + let receiver: ActorID = 6001; + let mut exec = new_executor(); + + let account_code = *exec.builtin_actors().get_account_code(); + + let raw_length = 100usize; + let pl = &exec.context().price_list; + let inclusion_cost = pl.on_chain_message(raw_length); + let inclusion_total = inclusion_cost.total().round_up(); + let gas_limit = inclusion_total * 10 + 1_000; + + let gas_fee_cap = TokenAmount::from_atto(1u64); + let gas_cost = gas_fee_cap.clone() * gas_limit; + + // Choose value and balance such that: + // - balance >= gas_cost (reservation begin succeeds). + // - value + gas_cost > balance (free < value, so transfer must fail). + let value = TokenAmount::from_atto(10u64); + let balance = &gas_cost + &value - &TokenAmount::from_atto(1u64); + + let mut sender_state = ActorState::new_empty(account_code, None); + sender_state.balance = balance.clone(); + exec.state_tree_mut().set_actor(sender, sender_state); + + let mut receiver_state = ActorState::new_empty(account_code, None); + receiver_state.balance = TokenAmount::zero(); + exec.state_tree_mut().set_actor(receiver, receiver_state); + + exec.begin_reservation_session(&[(Address::new_id(sender), gas_cost.clone())]) + .unwrap(); + + let engine = exec.engine_pool.acquire(); + let reservation_session = exec.reservation_session.clone(); + + let res = exec.map_machine(|machine| { + let mut cm = TestCallManager::new( + machine, + engine, + gas_limit, + sender, + Address::new_id(sender), + Some(receiver), + Address::new_id(receiver), + 0, + TokenAmount::zero(), + reservation_session, + ); + + let transfer_res = cm.transfer(sender, receiver, &value); + let (_, machine) = cm.finish(); + (transfer_res, machine) + }); + + match res { + Ok(()) => panic!("expected transfer to fail with insufficient funds"), + Err(ExecutionError::Syscall(err)) => { + assert_eq!(err.1, ErrorNumber::InsufficientFunds); + } + Err(other) => panic!("unexpected error from transfer: {:?}", other), + } + } + + #[test] + fn send_enforces_reservations_for_existing_actor() { + let sender: ActorID = 8000; + let receiver: ActorID = 8001; + let mut exec = new_executor(); + + let account_code = *exec.builtin_actors().get_account_code(); + + let raw_length = 100usize; + let pl = &exec.context().price_list; + let inclusion_cost = pl.on_chain_message(raw_length); + let inclusion_total = inclusion_cost.total().round_up(); + let gas_limit = inclusion_total * 10 + 1_000; + + let gas_fee_cap = TokenAmount::from_atto(1u64); + let gas_cost = gas_fee_cap.clone() * gas_limit; + + let value = TokenAmount::from_atto(10u64); + let balance = &gas_cost + &value - &TokenAmount::from_atto(1u64); + + let mut sender_state = ActorState::new_empty(account_code, None); + sender_state.balance = balance.clone(); + exec.state_tree_mut().set_actor(sender, sender_state); + + let mut receiver_state = ActorState::new_empty(account_code, None); + receiver_state.balance = TokenAmount::zero(); + exec.state_tree_mut().set_actor(receiver, receiver_state); + + exec.begin_reservation_session(&[(Address::new_id(sender), gas_cost.clone())]) + .unwrap(); + + let engine = exec.engine_pool.acquire(); + let reservation_session = exec.reservation_session.clone(); + + type SendKernel = DefaultKernel; + + let res = exec.map_machine(|machine| { + let cm = TestCallManager::new( + machine, + engine, + gas_limit, + sender, + Address::new_id(sender), + Some(sender), + Address::new_id(sender), + 0, + TokenAmount::zero(), + reservation_session, + ); + + let blocks = BlockRegistry::new(); + let mut kernel = ::new( + cm, + blocks, + sender, + sender, + METHOD_SEND, + TokenAmount::zero(), + false, + ); + + let send_res = SendOps::::send( + &mut kernel, + &Address::new_id(receiver), + METHOD_SEND, + NO_DATA_BLOCK_ID, + &value, + None, + SendFlags::empty(), + ); + + let (cm, _blocks) = kernel.into_inner(); + let (_, machine) = cm.finish(); + (send_res, machine) + }); + + match res { + Ok(_) => panic!("expected send to fail with insufficient funds"), + Err(ExecutionError::Syscall(err)) => { + assert_eq!(err.1, ErrorNumber::InsufficientFunds); + } + Err(other) => panic!("unexpected error from send: {:?}", other), + } + } + + #[test] + fn self_destruct_enforces_reservations() { + let sender: ActorID = 8100; + let mut exec = new_executor(); + + let account_code = *exec.builtin_actors().get_account_code(); + let initial_balance = TokenAmount::from_atto(1_000_000u64); + + let mut sender_state = ActorState::new_empty(account_code, None); + sender_state.balance = initial_balance.clone(); + exec.state_tree_mut().set_actor(sender, sender_state); + + let mut burnt_state = ActorState::new_empty(account_code, None); + burnt_state.balance = TokenAmount::zero(); + exec.state_tree_mut() + .set_actor(BURNT_FUNDS_ACTOR_ID, burnt_state); + + let reserved = TokenAmount::from_atto(100u64); + exec.begin_reservation_session(&[(Address::new_id(sender), reserved.clone())]) + .unwrap(); + + let engine = exec.engine_pool.acquire(); + let reservation_session = exec.reservation_session.clone(); + + type SelfDestructKernel = DefaultKernel; + + let res = exec.map_machine(|machine| { + let cm = TestCallManager::new( + machine, + engine, + 1_000_000, + sender, + Address::new_id(sender), + Some(sender), + Address::new_id(sender), + 0, + TokenAmount::zero(), + reservation_session, + ); + + let blocks = BlockRegistry::new(); + let mut kernel = ::new( + cm, + blocks, + sender, + sender, + METHOD_SEND, + TokenAmount::zero(), + false, + ); + + let sd_res = SelfOps::self_destruct(&mut kernel, true); + + let (cm, _blocks) = kernel.into_inner(); + let (_, machine) = cm.finish(); + (sd_res, machine) + }); + + match res { + Ok(()) => panic!("expected self_destruct to fail with insufficient funds"), + Err(ExecutionError::Syscall(err)) => { + assert_eq!(err.1, ErrorNumber::InsufficientFunds); + } + Err(other) => panic!("unexpected error from self_destruct: {:?}", other), + } + + // Actor is still present and balance unchanged because the transaction was reverted. + let actor = exec + .state_tree() + .get_actor(sender) + .unwrap() + .expect("sender actor must exist"); + assert_eq!(actor.balance, initial_balance); + } + + #[test] + fn settlement_under_reservations_net_charges_and_clears_ledger() { + let sender: ActorID = 7000; + let initial_balance = TokenAmount::from_atto(1_000_000u64); + let base_fee = TokenAmount::from_atto(10u64); + let mut exec = new_executor_with_base_fee(base_fee); + + // Install sender, burnt-funds, and reward actors so settlement can move funds. + let account_code = *exec.builtin_actors().get_account_code(); + + let mut sender_state = ActorState::new_empty(account_code, None); + sender_state.balance = initial_balance.clone(); + exec.state_tree_mut().set_actor(sender, sender_state); + + let mut burnt_state = ActorState::new_empty(account_code, None); + burnt_state.balance = TokenAmount::zero(); + exec.state_tree_mut() + .set_actor(BURNT_FUNDS_ACTOR_ID, burnt_state); + + let mut reward_state = ActorState::new_empty(account_code, None); + reward_state.balance = TokenAmount::zero(); + exec.state_tree_mut() + .set_actor(REWARD_ACTOR_ID, reward_state); + + let gas_limit = 1_000u64; + let gas_fee_cap = TokenAmount::from_atto(1u64); + let gas_cost = gas_fee_cap.clone() * gas_limit; + + exec.begin_reservation_session(&[(Address::new_id(sender), gas_cost.clone())]) + .unwrap(); + + let msg = Message { + version: 0, + from: Address::new_id(sender), + to: Address::new_id(1), + sequence: 0, + value: TokenAmount::zero(), + method_num: 0, + params: RawBytes::default(), + gas_limit, + gas_fee_cap: gas_fee_cap.clone(), + gas_premium: TokenAmount::zero(), + }; + + let gas_used = gas_limit / 2; + + let receipt = Receipt { + exit_code: ExitCode::OK, + return_data: RawBytes::default(), + gas_used, + events_root: None, + }; + + let apply_ret = exec + .finish_message( + sender, + msg, + receipt, + None, + gas_cost.clone(), + vec![], + Vec::new(), + ) + .expect("finish_message must succeed"); + + let base_fee_burn = apply_ret.base_fee_burn.clone(); + let over_estimation_burn = apply_ret.over_estimation_burn.clone(); + let miner_tip = apply_ret.miner_tip.clone(); + let refund = apply_ret.refund.clone(); + + // GasOutputs invariants: base_fee_burn + over_estimation_burn + refund + miner_tip == + // gas_cost. + assert_eq!( + &base_fee_burn + &over_estimation_burn + &refund + &miner_tip, + gas_cost + ); + + let consumption = &base_fee_burn + &over_estimation_burn + &miner_tip; + + // Net sender balance delta equals the gas consumption. + let actor = exec + .state_tree() + .get_actor(sender) + .unwrap() + .expect("sender actor must exist"); + assert_eq!(actor.balance, initial_balance - consumption.clone()); + + // Burns and tips are deposited to the appropriate actors. + let burnt_actor = exec + .state_tree() + .get_actor(BURNT_FUNDS_ACTOR_ID) + .unwrap() + .expect("burnt funds actor must exist"); + assert_eq!( + burnt_actor.balance, + &base_fee_burn + &over_estimation_burn + ); + + let reward_actor = exec + .state_tree() + .get_actor(REWARD_ACTOR_ID) + .unwrap() + .expect("reward actor must exist"); + assert_eq!(reward_actor.balance, miner_tip); + + // The reservation ledger is fully cleared for this sender so the session can end with zero + // remainder. + { + let session = exec + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + assert!(session.reservations.get(&sender).is_none()); + } + + exec.end_reservation_session().unwrap(); + } + + #[test] + #[ignore] + fn reservation_begin_end_performance_smoke() { + use std::time::Instant; + + let sender_base: ActorID = 10_000; + let num_senders: u64 = 10_000; + let mut exec = new_executor(); + + let account_code = *exec.builtin_actors().get_account_code(); + let balance = TokenAmount::from_atto(1_000_000u64); + + for offset in 0..num_senders { + let id = sender_base + offset; + let mut actor = ActorState::new_empty(account_code, None); + actor.balance = balance.clone(); + exec.state_tree_mut().set_actor(id, actor); + } + + let reservation = TokenAmount::from_atto(1_000u64); + let mut plan = Vec::with_capacity(num_senders as usize); + for offset in 0..num_senders { + let id = sender_base + offset; + plan.push((Address::new_id(id), reservation.clone())); + } + + let begin_start = Instant::now(); + exec.begin_reservation_session(&plan).unwrap(); + let begin_duration = begin_start.elapsed(); + + { + let mut session = exec + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + for amt in session.reservations.values_mut() { + *amt = TokenAmount::zero(); + } + } + + let end_start = Instant::now(); + exec.end_reservation_session().unwrap(); + let end_duration = end_start.elapsed(); + + println!( + "reservation_begin_end_performance_smoke: begin_ms={} end_ms={} senders={}", + begin_duration.as_secs_f64() * 1000.0, + end_duration.as_secs_f64() * 1000.0, + num_senders + ); + } + + #[cfg(feature = "arb")] + #[test] + fn gas_outputs_quickcheck_invariants_hold() { + use quickcheck::{QuickCheck, TestResult}; + + fn prop( + gas_limit: u64, + gas_used_seed: u64, + fee_cap: TokenAmount, + premium: TokenAmount, + ) -> TestResult { + if gas_limit == 0 { + return TestResult::discard(); + } + + // Ensure 0 <= gas_used <= gas_limit without overflowing when gas_limit == u64::MAX. + let gas_used = gas_used_seed % gas_limit.saturating_add(1); + + // Constrain fee_cap and premium to be non-negative to match protocol assumptions. + let fee_cap = if fee_cap.is_negative() { -fee_cap } else { fee_cap }; + let premium = if premium.is_negative() { -premium } else { premium }; + + let base_fee = TokenAmount::from_atto(10u64); + + let outputs = + GasOutputs::compute(gas_used, gas_limit, &base_fee, &fee_cap, &premium); + + // All gas accounting components must be non-negative. + if outputs.base_fee_burn.is_negative() + || outputs.over_estimation_burn.is_negative() + || outputs.miner_penalty.is_negative() + || outputs.miner_tip.is_negative() + || outputs.refund.is_negative() + { + return TestResult::failed(); + } + + // Gas outputs must conserve the total required funds. + let gas_cost = fee_cap.clone() * gas_limit; + if (&outputs.base_fee_burn + + &outputs.over_estimation_burn + + &outputs.refund + + &outputs.miner_tip) + != gas_cost + { + return TestResult::failed(); + } + + TestResult::passed() + } + + QuickCheck::new() + .tests(100) + .quickcheck(prop as fn(u64, u64, TokenAmount, TokenAmount) -> TestResult); + } + } diff --git a/fvm/src/executor/mod.rs b/fvm/src/executor/mod.rs index 1d423c57b..3d240dcfc 100644 --- a/fvm/src/executor/mod.rs +++ b/fvm/src/executor/mod.rs @@ -2,18 +2,21 @@ // SPDX-License-Identifier: Apache-2.0, MIT mod default; mod threaded; +mod telemetry; use std::fmt::Display; use cid::Cid; -pub use default::DefaultExecutor; +pub use default::{DefaultExecutor, ReservationSession}; use fvm_ipld_encoding::RawBytes; use fvm_shared::econ::TokenAmount; use fvm_shared::error::ExitCode; use fvm_shared::event::StampedEvent; use fvm_shared::message::Message; use fvm_shared::receipt::Receipt; +use fvm_shared::ActorID; use num_traits::Zero; +use thiserror::Error; pub use threaded::ThreadedExecutor; use crate::Kernel; @@ -46,6 +49,35 @@ pub trait Executor { fn flush(&mut self) -> anyhow::Result; } +/// Errors that can occur while managing gas reservation sessions. +#[derive(Debug, Clone, PartialEq, Eq, Error)] +pub enum ReservationError { + /// Reservations are not implemented for this engine version. + #[error("reservations not implemented")] + NotImplemented, + /// A sender does not have enough balance to cover its reserved total at begin. + #[error("insufficient funds at begin for sender {sender}")] + InsufficientFundsAtBegin { sender: ActorID }, + /// A reservation session is already open. + #[error("reservation session already open")] + SessionOpen, + /// No reservation session is currently open. + #[error("no reservation session open")] + SessionClosed, + /// The reservation ledger has non-zero entries at session end. + #[error("reservation ledger has non-zero remainder at session end")] + NonZeroRemainder, + /// The reservation plan exceeds resource limits (senders or encoded bytes). + #[error("reservation plan too large")] + PlanTooLarge, + /// Arithmetic overflow or underflow while accounting reservations. + #[error("reservation arithmetic overflow or underflow")] + Overflow, + /// Any inconsistency between host plan and engine state (e.g., unknown sender). + #[error("reservation invariant violated: {0}")] + ReservationInvariant(String), +} + /// A description of some failure encountered when applying a message. #[derive(Debug, Clone)] pub enum ApplyFailure { diff --git a/fvm/src/executor/telemetry.rs b/fvm/src/executor/telemetry.rs new file mode 100644 index 000000000..b3fcc7c29 --- /dev/null +++ b/fvm/src/executor/telemetry.rs @@ -0,0 +1,128 @@ +// Copyright 2021-2023 Protocol Labs +// SPDX-License-Identifier: Apache-2.0, MIT + +use std::collections::HashMap; +use std::sync::{Mutex, OnceLock}; + +use fvm_shared::econ::TokenAmount; +use fvm_shared::ActorID; + +/// Telemetry for reservation sessions and settlement. +/// +/// This module keeps lightweight, process-local counters and gauges for +/// reservation lifecycle and settlement behavior. It is intentionally simple +/// and embedder-agnostic; embedders may choose to periodically snapshot these +/// values and export them to their metrics backends. +#[derive(Default, Clone)] +pub struct ReservationTelemetry { + /// Gauge for the number of open reservation sessions. + pub reservations_open: u64, + + /// Counter for failed reservation session begins. + pub reservation_begin_failed: u64, + + /// Sum of base-fee burn amounts settled across messages. + pub settle_basefee_burn: TokenAmount, + + /// Sum of miner tip credits settled across messages. + pub settle_tip_credit: TokenAmount, + + /// Sum of over-estimation burn amounts settled across messages. + pub settle_overburn: TokenAmount, + + /// Sum of virtual refunds realized via reservation release (reservation mode only). + pub settle_refund_virtual: TokenAmount, + + /// Total reservation per sender at session begin, keyed by ActorID. + pub reservation_total_per_sender: HashMap, + + /// Remaining reserved amount per sender, keyed by ActorID. + pub reserved_remaining_per_sender: HashMap, +} + +static TELEMETRY: OnceLock> = OnceLock::new(); + +fn metrics() -> &'static Mutex { + TELEMETRY.get_or_init(|| Mutex::new(ReservationTelemetry::default())) +} + +/// Record a successful reservation session begin with the per-sender totals. +pub fn reservation_begin_succeeded(reservations: &HashMap) { + let mut m = metrics() + .lock() + .expect("reservation telemetry mutex poisoned"); + m.reservations_open = m.reservations_open.saturating_add(1); + m.reservation_total_per_sender = reservations.clone(); + m.reserved_remaining_per_sender = reservations.clone(); +} + +/// Record a failed reservation session begin. +pub fn reservation_begin_failed() { + let mut m = metrics() + .lock() + .expect("reservation telemetry mutex poisoned"); + m.reservation_begin_failed = m.reservation_begin_failed.saturating_add(1); +} + +/// Record a successful reservation session end and clear per-sender gauges. +pub fn reservation_end_succeeded() { + let mut m = metrics() + .lock() + .expect("reservation telemetry mutex poisoned"); + m.reservations_open = m.reservations_open.saturating_sub(1); + m.reservation_total_per_sender.clear(); + m.reserved_remaining_per_sender.clear(); +} + +/// Update the remaining reserved amount for a sender. +pub fn reservation_remaining_update(sender: ActorID, remaining: &TokenAmount) { + let mut m = metrics() + .lock() + .expect("reservation telemetry mutex poisoned"); + + if remaining.is_zero() { + m.reserved_remaining_per_sender.remove(&sender); + } else { + m.reserved_remaining_per_sender + .insert(sender, remaining.clone()); + } +} + +/// Record settlement amounts for a single message. +/// +/// The `refund_virtual` argument should be `Some(refund)` in reservation mode, +/// where refunds are realized via reservation release instead of a direct +/// balance transfer. +pub fn settlement_record( + base_fee_burn: &TokenAmount, + miner_tip: &TokenAmount, + over_estimation_burn: &TokenAmount, + refund_virtual: Option<&TokenAmount>, +) { + let mut m = metrics() + .lock() + .expect("reservation telemetry mutex poisoned"); + m.settle_basefee_burn += base_fee_burn.clone(); + m.settle_tip_credit += miner_tip.clone(); + m.settle_overburn += over_estimation_burn.clone(); + + if let Some(refund) = refund_virtual { + m.settle_refund_virtual += refund.clone(); + } +} + +/// Snapshot the current reservation telemetry. +pub fn snapshot() -> ReservationTelemetry { + metrics() + .lock() + .expect("reservation telemetry mutex poisoned") + .clone() +} + +#[cfg(test)] +pub fn reset() { + *metrics() + .lock() + .expect("reservation telemetry mutex poisoned") = ReservationTelemetry::default(); +} + diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index adb6b656f..bd0f91717 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -371,13 +371,14 @@ where let balance = self.current_balance()?; if !balance.is_zero() { if !burn_unspent { - return Err( - syscall_error!(IllegalOperation; "self-destruct with unspent funds").into(), - ); + return Err(syscall_error!( + IllegalOperation; + "self-destruct with unspent funds" + ) + .into()); } self.call_manager - .transfer(self.actor_id, BURNT_FUNDS_ACTOR_ID, &balance) - .or_fatal()?; + .transfer(self.actor_id, BURNT_FUNDS_ACTOR_ID, &balance)?; } // Delete the executing actor. diff --git a/fvm/tests/dummy.rs b/fvm/tests/dummy.rs index c353838ab..79ee0066c 100644 --- a/fvm/tests/dummy.rs +++ b/fvm/tests/dummy.rs @@ -3,6 +3,7 @@ use std::borrow::Borrow; use std::cell::RefCell; use std::rc::Rc; +use std::sync::{Arc, Mutex}; use anyhow::Context; use cid::Cid; @@ -15,6 +16,7 @@ use fvm::machine::{Machine, MachineContext, Manifest, NetworkConfig}; use fvm::state_tree::StateTree; use fvm::trace::IpldOperation; use fvm::{Kernel, kernel}; +use fvm::executor::ReservationSession; use fvm_ipld_blockstore::{Blockstore, MemoryBlockstore}; use fvm_ipld_encoding::{CborStore, DAG_CBOR}; use fvm_shared::address::Address; @@ -258,6 +260,7 @@ impl CallManager for DummyCallManager { _receiver_address: Address, nonce: u64, gas_premium: TokenAmount, + _reservation_session: Arc>, ) -> Self { let rc = Rc::new(RefCell::new(TestData { charge_gas_calls: 0, diff --git a/testing/integration/tests/reservation_transfer_enforcement.rs b/testing/integration/tests/reservation_transfer_enforcement.rs new file mode 100644 index 000000000..b99a81a7c --- /dev/null +++ b/testing/integration/tests/reservation_transfer_enforcement.rs @@ -0,0 +1,192 @@ +// Copyright 2021-2023 Protocol Labs +// SPDX-License-Identifier: Apache-2.0, MIT + +use bundles::*; +use fvm::executor::{ApplyKind, Executor}; +use fvm::machine::Machine; +use fvm_integration_tests::dummy::DummyExterns; +use fvm_integration_tests::tester::{BasicExecutor, INITIAL_ACCOUNT_BALANCE}; +use fvm_ipld_blockstore::MemoryBlockstore; +use fvm_shared::address::{Address, SECP_PUB_LEN}; +use fvm_shared::econ::TokenAmount; +use fvm_shared::error::ExitCode; +use fvm_shared::message::Message; +use fvm_shared::state::StateTreeVersion; +use fvm_shared::version::NetworkVersion; +use fvm_shared::{ActorID, METHOD_SEND}; + +mod bundles; + +fn sender_balance_and_id( + executor: &BasicExecutor, + sender_address: &Address, +) -> (ActorID, TokenAmount) { + let state_tree = executor.state_tree(); + let actor = state_tree + .get_actor_by_address(sender_address) + .expect("failed to load sender actor") + .expect("sender actor missing from state tree"); + let id = state_tree + .lookup_id(sender_address) + .expect("failed to resolve sender address") + .expect("sender address has no ID mapping"); + (id, actor.balance) +} + +#[test] +fn reservation_blocks_value_over_free_on_send() { + let mut tester = new_tester( + NetworkVersion::V21, + StateTreeVersion::V5, + MemoryBlockstore::default(), + ) + .unwrap(); + + let [(sender_id, sender_address), (_receiver_id, receiver_address)] = + tester.create_accounts().unwrap(); + + tester.instantiate_machine(DummyExterns).unwrap(); + let executor = tester.executor.as_mut().unwrap(); + + // Top up the sender balance so we can use large gas limits without running out of funds when + // building the reservation plan. + let topup_balance = TokenAmount::from_atto(1_000_000_000u64); + executor + .state_tree_mut() + .mutate_actor(sender_id, |actor| { + actor.balance = topup_balance.clone(); + Ok(()) + }) + .expect("failed to top up sender balance"); + + let (_id, balance) = sender_balance_and_id(executor, &sender_address); + assert_eq!(balance, topup_balance); + + // Reserve exactly the cap×limit for this sender. + let gas_fee_cap = TokenAmount::from_atto(1); + let gas_limit = 1_000_000u64; + let gas_cost = gas_fee_cap.clone() * gas_limit; + let plan = vec![(sender_address, gas_cost.clone())]; + executor + .begin_reservation_session(&plan) + .expect("begin reservation session"); + + // Free balance during the session is balance − reserved. + let free = balance - gas_cost; + let value = &free + TokenAmount::from_atto(1u8); + + let message = Message { + from: sender_address, + to: receiver_address, + gas_limit, + gas_fee_cap: gas_fee_cap.clone(), + method_num: METHOD_SEND, + sequence: 0, + value: value.clone(), + ..Message::default() + }; + + let res = executor + .execute_message(message, ApplyKind::Explicit, 100) + .expect("execution failed"); + + assert_eq!( + res.msg_receipt.exit_code, + ExitCode::SYS_INSUFFICIENT_FUNDS, + "send should fail when value exceeds free balance under reservations", + ); + + // Destination balance should be unchanged. + let dest_balance = executor + .state_tree() + .get_actor_by_address(&receiver_address) + .expect("failed to load receiver actor") + .expect("receiver actor missing") + .balance; + assert_eq!( + dest_balance, *INITIAL_ACCOUNT_BALANCE, + "receiver balance should not change on failed send" + ); +} + +#[test] +fn reservation_blocks_actor_creation_value_over_free() { + let mut tester = new_tester( + NetworkVersion::V21, + StateTreeVersion::V5, + MemoryBlockstore::default(), + ) + .unwrap(); + + let [(sender_id, sender_address)] = tester.create_accounts().unwrap(); + + tester.instantiate_machine(DummyExterns).unwrap(); + let executor = tester.executor.as_mut().unwrap(); + + let topup_balance = TokenAmount::from_atto(1_000_000_000u64); + executor + .state_tree_mut() + .mutate_actor(sender_id, |actor| { + actor.balance = topup_balance.clone(); + Ok(()) + }) + .expect("failed to top up sender balance"); + + let (_id, balance) = sender_balance_and_id(executor, &sender_address); + assert_eq!(balance, topup_balance); + + let gas_fee_cap = TokenAmount::from_atto(1); + let gas_limit = 1_000_000u64; + let gas_cost = gas_fee_cap.clone() * gas_limit; + let plan = vec![(sender_address, gas_cost.clone())]; + executor + .begin_reservation_session(&plan) + .expect("begin reservation session"); + + let free = balance - gas_cost; + let value = &free + TokenAmount::from_atto(1u8); + + // Choose a new Secp256k1 address that does not yet exist in the state tree. + let new_addr = Address::new_secp256k1(&[1u8; SECP_PUB_LEN]).expect("invalid secp address"); + assert!( + executor + .state_tree() + .get_actor_by_address(&new_addr) + .expect("lookup failed") + .is_none(), + "new address unexpectedly already has an actor" + ); + + let message = Message { + from: sender_address, + to: new_addr, + gas_limit, + gas_fee_cap, + method_num: METHOD_SEND, + sequence: 0, + value: value.clone(), + ..Message::default() + }; + + let res = executor + .execute_message(message, ApplyKind::Explicit, 100) + .expect("execution failed"); + + assert!( + !res.msg_receipt.exit_code.is_success(), + "actor-creation send should fail when value exceeds free balance under reservations, got {:?}", + res.msg_receipt.exit_code, + ); + + // The auto-created account actor must not receive funds when the transfer fails. + let created_actor = executor + .state_tree() + .get_actor_by_address(&new_addr) + .expect("lookup failed"); + if let Some(actor) = created_actor { + assert!( + actor.balance.is_zero(), + "newly created actor should not receive funds on failed transfer" + ); + } +} From ac85696938afda94fce6a391caf3351c049e3aea Mon Sep 17 00:00:00 2001 From: Mikers Date: Tue, 18 Nov 2025 09:30:21 -1000 Subject: [PATCH 02/12] rust fmt --- fvm/src/call_manager/default.rs | 2 +- fvm/src/call_manager/mod.rs | 2 +- fvm/src/executor/default.rs | 111 +++++++----------- fvm/src/executor/mod.rs | 4 +- fvm/src/executor/telemetry.rs | 3 +- fvm/tests/dummy.rs | 2 +- .../tests/reservation_transfer_enforcement.rs | 6 +- 7 files changed, 50 insertions(+), 80 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index cb58c2960..2f389eb45 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -23,6 +23,7 @@ use crate::call_manager::FinishRet; use crate::call_manager::backtrace::Frame; use crate::eam_actor::EAM_ACTOR_ID; use crate::engine::Engine; +use crate::executor::ReservationSession; use crate::gas::{Gas, GasTracker}; use crate::kernel::{ Block, BlockRegistry, ClassifyResult, ExecutionError, Kernel, Result, SyscallError, @@ -34,7 +35,6 @@ use crate::syscalls::error::Abort; use crate::syscalls::{charge_for_exec, update_gas_available}; use crate::trace::{ExecutionEvent, ExecutionTrace}; use crate::{syscall_error, system_actor}; -use crate::executor::ReservationSession; /// The default [`CallManager`] implementation. #[repr(transparent)] diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index a982b058a..5807f40b6 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -12,10 +12,10 @@ use fvm_shared::{ActorID, METHOD_CONSTRUCTOR, MethodNum}; use crate::Kernel; use crate::engine::Engine; +use crate::executor::ReservationSession; use crate::gas::{Gas, GasCharge, GasTimer, GasTracker, PriceList}; use crate::kernel::{self, BlockRegistry, ClassifyResult, Context, Result}; use crate::machine::{Machine, MachineContext}; -use crate::executor::ReservationSession; use crate::state_tree::ActorState; pub mod backtrace; diff --git a/fvm/src/executor/default.rs b/fvm/src/executor/default.rs index 4a73d4b0c..ec9c5ceb9 100644 --- a/fvm/src/executor/default.rs +++ b/fvm/src/executor/default.rs @@ -32,8 +32,8 @@ pub use self::reservation::ReservationSession; mod reservation { use std::collections::HashMap; - use fvm_shared::econ::TokenAmount; use fvm_shared::ActorID; + use fvm_shared::econ::TokenAmount; #[derive(Default)] pub struct ReservationSession { @@ -495,10 +495,7 @@ where } return Ok(Err(ApplyRet::prevalidation_fail( ExitCode::SYS_OUT_OF_GAS, - format!( - "Out of gas ({} > {})", - inclusion_total, msg.gas_limit - ), + format!("Out of gas ({} > {})", inclusion_total, msg.gas_limit), &self.context().base_fee * inclusion_total, ))); } @@ -664,9 +661,7 @@ where let consumption = &base_fee_burn + &over_estimation_burn + &miner_tip; self.state_tree_mut() - .mutate_actor(sender_id, |act| { - act.deduct_funds(&consumption).or_fatal() - }) + .mutate_actor(sender_id, |act| act.deduct_funds(&consumption).or_fatal()) .context("failed to lookup sender actor for settlement")?; // Decrement this message's reservation; underflow or a missing entry indicates a fatal @@ -686,12 +681,7 @@ where transfer_to_actor(sender_id, &refund)?; // Track settlement metrics in legacy mode as well, without a virtual refund component. - telemetry::settlement_record( - &base_fee_burn, - &miner_tip, - &over_estimation_burn, - None, - ); + telemetry::settlement_record(&base_fee_burn, &miner_tip, &over_estimation_burn, None); } if (&base_fee_burn + &over_estimation_burn + &refund + &miner_tip) != gas_cost { @@ -805,9 +795,7 @@ where if &actor_state.balance < reserved { telemetry::reservation_begin_failed(); - return Err(ReservationError::InsufficientFundsAtBegin { - sender: *actor_id, - }); + return Err(ReservationError::InsufficientFundsAtBegin { sender: *actor_id }); } } @@ -850,18 +838,18 @@ mod tests { use cid::Cid; use fvm_ipld_blockstore::MemoryBlockstore; use fvm_ipld_encoding::{CborStore, DAG_CBOR, RawBytes}; + use fvm_shared::IDENTITY_HASH; use fvm_shared::address::Address; use fvm_shared::error::ExitCode; use fvm_shared::state::{ActorState, StateTreeVersion}; - use fvm_shared::IDENTITY_HASH; use multihash_codetable::{Code, Multihash}; use crate::call_manager::DefaultCallManager; + use crate::call_manager::NO_DATA_BLOCK_ID; use crate::engine::EnginePool; use crate::externs::{Chain, Consensus, Externs, Rand}; - use crate::call_manager::NO_DATA_BLOCK_ID; - use crate::kernel::filecoin::DefaultFilecoinKernel; use crate::kernel::default::DefaultKernel; + use crate::kernel::filecoin::DefaultFilecoinKernel; use crate::kernel::{BlockRegistry, SelfOps, SendOps}; use crate::machine::{DefaultMachine, Manifest, NetworkConfig}; use crate::state_tree::StateTree; @@ -904,10 +892,7 @@ mod tests { } impl Chain for DummyExterns { - fn get_tipset_cid( - &self, - epoch: fvm_shared::clock::ChainEpoch, - ) -> anyhow::Result { + fn get_tipset_cid(&self, epoch: fvm_shared::clock::ChainEpoch) -> anyhow::Result { Ok(Cid::new_v1( DAG_CBOR, Multihash::wrap(IDENTITY_HASH, &epoch.to_be_bytes()).unwrap(), @@ -929,11 +914,12 @@ mod tests { bs = st.into_store(); // An empty built-in actors manifest. - let manifest_cid = bs.put_cbor(&Manifest::DUMMY_CODES, Code::Blake2b256).unwrap(); + let manifest_cid = bs + .put_cbor(&Manifest::DUMMY_CODES, Code::Blake2b256) + .unwrap(); let actors_cid = bs.put_cbor(&(1, manifest_cid), Code::Blake2b256).unwrap(); - let mut net_cfg = - NetworkConfig::new(fvm_shared::version::NetworkVersion::V21); + let mut net_cfg = NetworkConfig::new(fvm_shared::version::NetworkVersion::V21); net_cfg.override_actors(actors_cid); let mut mc = net_cfg.for_epoch(0, 0, root); mc.set_base_fee(base_fee); @@ -952,7 +938,9 @@ mod tests { bs = st.into_store(); // An empty built-in actors manifest. - let manifest_cid = bs.put_cbor(&Manifest::DUMMY_CODES, Code::Blake2b256).unwrap(); + let manifest_cid = bs + .put_cbor(&Manifest::DUMMY_CODES, Code::Blake2b256) + .unwrap(); let actors_cid = bs.put_cbor(&(1, manifest_cid), Code::Blake2b256).unwrap(); let mc = NetworkConfig::new(fvm_shared::version::NetworkVersion::V21) @@ -965,10 +953,7 @@ mod tests { TestExecutor::new(engine, Box::new(machine)).unwrap() } - fn new_executor_with_actor( - id: ActorID, - balance: TokenAmount, - ) -> TestExecutor { + fn new_executor_with_actor(id: ActorID, balance: TokenAmount) -> TestExecutor { let mut exec = new_executor(); let account_code = *exec.builtin_actors().get_account_code(); @@ -1013,8 +998,7 @@ mod tests { #[test] fn begin_and_end_with_zero_remainder_succeeds() { let sender: ActorID = 1000; - let mut exec = - new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); + let mut exec = new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); let plan = vec![(Address::new_id(sender), TokenAmount::from_atto(500u64))]; @@ -1026,11 +1010,7 @@ mod tests { .expect("reservation session mutex poisoned"); assert!(session.open); assert_eq!( - session - .reservations - .get(&sender) - .cloned() - .unwrap(), + session.reservations.get(&sender).cloned().unwrap(), TokenAmount::from_atto(500u64) ); @@ -1054,8 +1034,7 @@ mod tests { #[test] fn begin_twice_errors_with_session_open() { let sender: ActorID = 42; - let mut exec = - new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); + let mut exec = new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); let plan = vec![(Address::new_id(sender), TokenAmount::from_atto(100u64))]; exec.begin_reservation_session(&plan).unwrap(); @@ -1066,8 +1045,7 @@ mod tests { #[test] fn end_with_non_zero_remainder_errors() { let sender: ActorID = 7; - let mut exec = - new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); + let mut exec = new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); let plan = vec![(Address::new_id(sender), TokenAmount::from_atto(1234u64))]; exec.begin_reservation_session(&plan).unwrap(); @@ -1093,15 +1071,11 @@ mod tests { #[test] fn insufficient_funds_at_begin() { let sender: ActorID = 5; - let mut exec = - new_executor_with_actor(sender, TokenAmount::from_atto(10u64)); + let mut exec = new_executor_with_actor(sender, TokenAmount::from_atto(10u64)); let plan = vec![(Address::new_id(sender), TokenAmount::from_atto(11u64))]; let err = exec.begin_reservation_session(&plan).unwrap_err(); - assert_eq!( - err, - ReservationError::InsufficientFundsAtBegin { sender } - ); + assert_eq!(err, ReservationError::InsufficientFundsAtBegin { sender }); } #[test] @@ -1178,11 +1152,7 @@ mod tests { .lock() .expect("reservation session mutex poisoned"); assert_eq!( - session - .reservations - .get(&sender) - .cloned() - .unwrap(), + session.reservations.get(&sender).cloned().unwrap(), gas_cost ); } @@ -1261,8 +1231,7 @@ mod tests { #[test] fn preflight_negative_fee_cap_yields_overflow() { let sender: ActorID = 2500; - let mut exec = - new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); + let mut exec = new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); let raw_length = 100usize; let pl = &exec.context().price_list; @@ -1336,8 +1305,7 @@ mod tests { .preflight_message(&msg, ApplyKind::Explicit, raw_length) .unwrap(); - let apply_ret = - res.expect_err("expected prevalidation failure for inclusion gas too low"); + let apply_ret = res.expect_err("expected prevalidation failure for inclusion gas too low"); assert_eq!(apply_ret.msg_receipt.exit_code, ExitCode::SYS_OUT_OF_GAS); // Actor state is unchanged on prevalidation failure. @@ -1363,8 +1331,7 @@ mod tests { #[test] fn reservation_coverage_violation_yields_reservation_invariant() { let sender: ActorID = 4000; - let mut exec = - new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); + let mut exec = new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); let raw_length = 100usize; let pl = &exec.context().price_list; @@ -1413,8 +1380,7 @@ mod tests { #[test] fn reservation_prevalidation_decrement_underflow_yields_overflow() { let sender: ActorID = 5000; - let mut exec = - new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); + let mut exec = new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); // Manually open a reservation session with an under-sized reservation to trigger // arithmetic underflow when we attempt to decrement it. @@ -1777,10 +1743,7 @@ mod tests { .get_actor(BURNT_FUNDS_ACTOR_ID) .unwrap() .expect("burnt funds actor must exist"); - assert_eq!( - burnt_actor.balance, - &base_fee_burn + &over_estimation_burn - ); + assert_eq!(burnt_actor.balance, &base_fee_burn + &over_estimation_burn); let reward_actor = exec .state_tree() @@ -1873,13 +1836,20 @@ mod tests { let gas_used = gas_used_seed % gas_limit.saturating_add(1); // Constrain fee_cap and premium to be non-negative to match protocol assumptions. - let fee_cap = if fee_cap.is_negative() { -fee_cap } else { fee_cap }; - let premium = if premium.is_negative() { -premium } else { premium }; + let fee_cap = if fee_cap.is_negative() { + -fee_cap + } else { + fee_cap + }; + let premium = if premium.is_negative() { + -premium + } else { + premium + }; let base_fee = TokenAmount::from_atto(10u64); - let outputs = - GasOutputs::compute(gas_used, gas_limit, &base_fee, &fee_cap, &premium); + let outputs = GasOutputs::compute(gas_used, gas_limit, &base_fee, &fee_cap, &premium); // All gas accounting components must be non-negative. if outputs.base_fee_burn.is_negative() @@ -1909,5 +1879,4 @@ mod tests { .tests(100) .quickcheck(prop as fn(u64, u64, TokenAmount, TokenAmount) -> TestResult); } - } diff --git a/fvm/src/executor/mod.rs b/fvm/src/executor/mod.rs index 3d240dcfc..72c72002b 100644 --- a/fvm/src/executor/mod.rs +++ b/fvm/src/executor/mod.rs @@ -1,20 +1,20 @@ // Copyright 2021-2023 Protocol Labs // SPDX-License-Identifier: Apache-2.0, MIT mod default; -mod threaded; mod telemetry; +mod threaded; use std::fmt::Display; use cid::Cid; pub use default::{DefaultExecutor, ReservationSession}; use fvm_ipld_encoding::RawBytes; +use fvm_shared::ActorID; use fvm_shared::econ::TokenAmount; use fvm_shared::error::ExitCode; use fvm_shared::event::StampedEvent; use fvm_shared::message::Message; use fvm_shared::receipt::Receipt; -use fvm_shared::ActorID; use num_traits::Zero; use thiserror::Error; pub use threaded::ThreadedExecutor; diff --git a/fvm/src/executor/telemetry.rs b/fvm/src/executor/telemetry.rs index b3fcc7c29..26e22c345 100644 --- a/fvm/src/executor/telemetry.rs +++ b/fvm/src/executor/telemetry.rs @@ -4,8 +4,8 @@ use std::collections::HashMap; use std::sync::{Mutex, OnceLock}; -use fvm_shared::econ::TokenAmount; use fvm_shared::ActorID; +use fvm_shared::econ::TokenAmount; /// Telemetry for reservation sessions and settlement. /// @@ -125,4 +125,3 @@ pub fn reset() { .lock() .expect("reservation telemetry mutex poisoned") = ReservationTelemetry::default(); } - diff --git a/fvm/tests/dummy.rs b/fvm/tests/dummy.rs index 79ee0066c..4428a8211 100644 --- a/fvm/tests/dummy.rs +++ b/fvm/tests/dummy.rs @@ -9,6 +9,7 @@ use anyhow::Context; use cid::Cid; use fvm::call_manager::{Backtrace, CallManager, Entrypoint, FinishRet, InvocationResult}; use fvm::engine::Engine; +use fvm::executor::ReservationSession; use fvm::externs::{Chain, Consensus, Externs, Rand}; use fvm::gas::{Gas, GasCharge, GasTimer, GasTracker}; use fvm::machine::limiter::MemoryLimiter; @@ -16,7 +17,6 @@ use fvm::machine::{Machine, MachineContext, Manifest, NetworkConfig}; use fvm::state_tree::StateTree; use fvm::trace::IpldOperation; use fvm::{Kernel, kernel}; -use fvm::executor::ReservationSession; use fvm_ipld_blockstore::{Blockstore, MemoryBlockstore}; use fvm_ipld_encoding::{CborStore, DAG_CBOR}; use fvm_shared::address::Address; diff --git a/testing/integration/tests/reservation_transfer_enforcement.rs b/testing/integration/tests/reservation_transfer_enforcement.rs index b99a81a7c..4065872ba 100644 --- a/testing/integration/tests/reservation_transfer_enforcement.rs +++ b/testing/integration/tests/reservation_transfer_enforcement.rs @@ -42,8 +42,10 @@ fn reservation_blocks_value_over_free_on_send() { ) .unwrap(); - let [(sender_id, sender_address), (_receiver_id, receiver_address)] = - tester.create_accounts().unwrap(); + let [ + (sender_id, sender_address), + (_receiver_id, receiver_address), + ] = tester.create_accounts().unwrap(); tester.instantiate_machine(DummyExterns).unwrap(); let executor = tester.executor.as_mut().unwrap(); From 054b8e857afd4df0b5642afa3569779470ee88f9 Mon Sep 17 00:00:00 2001 From: Mikers Date: Thu, 20 Nov 2025 12:00:22 -1000 Subject: [PATCH 03/12] Expose executor telemetry --- fvm/src/executor/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fvm/src/executor/mod.rs b/fvm/src/executor/mod.rs index 72c72002b..0a88213cb 100644 --- a/fvm/src/executor/mod.rs +++ b/fvm/src/executor/mod.rs @@ -1,7 +1,7 @@ // Copyright 2021-2023 Protocol Labs // SPDX-License-Identifier: Apache-2.0, MIT mod default; -mod telemetry; +pub mod telemetry; mod threaded; use std::fmt::Display; From d73628f826e3bc2e87e1462bd036f60fb3a87bee Mon Sep 17 00:00:00 2001 From: Mikers Date: Thu, 20 Nov 2025 12:08:00 -1000 Subject: [PATCH 04/12] Fix transfer balance comparison --- fvm/src/call_manager/default.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 2f389eb45..0780cf31e 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -517,7 +517,7 @@ where if reservation_open { let required = &reserved_remaining + value; - if &from_actor.balance < &required { + if from_actor.balance < required { return Err(syscall_error!( InsufficientFunds; "sender does not have free funds to transfer (balance {}, transfer {}, reserved {})", From e4678359c6eb73741472618235eff784552e3cd4 Mon Sep 17 00:00:00 2001 From: Mikers Date: Thu, 20 Nov 2025 12:14:09 -1000 Subject: [PATCH 05/12] Fix clippy unnecessary get-check in reservation tests --- fvm/src/executor/default.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fvm/src/executor/default.rs b/fvm/src/executor/default.rs index ec9c5ceb9..2b803114e 100644 --- a/fvm/src/executor/default.rs +++ b/fvm/src/executor/default.rs @@ -1216,7 +1216,7 @@ mod tests { .reservation_session .lock() .expect("reservation session mutex poisoned"); - assert!(session.reservations.get(&sender).is_none()); + assert!(!session.reservations.contains_key(&sender)); } exec.end_reservation_session().unwrap(); { @@ -1323,7 +1323,7 @@ mod tests { .reservation_session .lock() .expect("reservation session mutex poisoned"); - assert!(session.reservations.get(&sender).is_none()); + assert!(!session.reservations.contains_key(&sender)); } exec.end_reservation_session().unwrap(); } @@ -1759,7 +1759,7 @@ mod tests { .reservation_session .lock() .expect("reservation session mutex poisoned"); - assert!(session.reservations.get(&sender).is_none()); + assert!(!session.reservations.contains_key(&sender)); } exec.end_reservation_session().unwrap(); From 2bf38fd2d456fea8da4d45e81e158192a22cac41 Mon Sep 17 00:00:00 2001 From: Gemini Agent Date: Fri, 5 Dec 2025 12:28:15 -1000 Subject: [PATCH 06/12] Fix: Make FVM telemetry per-executor to avoid concurrency issues Refactor FVM's reservation telemetry to be per-executor rather than a global singleton. This resolves a potential issue where concurrent reservation sessions (e.g., in multi-threaded environments or parallel block validation) could overwrite telemetry data, leading to incorrect metrics. The global and were removed from and the telemetry functions were converted into methods of the struct. The struct is now embedded within the in , ensuring each executor's session maintains its own isolated telemetry. All call sites were updated to use this per-session instance, acquiring the lock where necessary. --- fvm/src/executor/default.rs | 47 +++++++++---- fvm/src/executor/telemetry.rs | 125 +++++++++++++--------------------- 2 files changed, 79 insertions(+), 93 deletions(-) diff --git a/fvm/src/executor/default.rs b/fvm/src/executor/default.rs index 2b803114e..57e7ebbd2 100644 --- a/fvm/src/executor/default.rs +++ b/fvm/src/executor/default.rs @@ -21,7 +21,6 @@ use super::{ApplyFailure, ApplyKind, ApplyRet, Executor, ReservationError}; use crate::call_manager::{Backtrace, CallManager, Entrypoint, InvocationResult, backtrace}; use crate::eam_actor::EAM_ACTOR_ID; use crate::engine::EnginePool; -use crate::executor::telemetry; use crate::gas::{Gas, GasCharge, GasOutputs}; use crate::kernel::{Block, ClassifyResult, Context as _, ExecutionError, Kernel}; use crate::machine::{BURNT_FUNDS_ACTOR_ID, Machine, REWARD_ACTOR_ID}; @@ -35,10 +34,13 @@ mod reservation { use fvm_shared::ActorID; use fvm_shared::econ::TokenAmount; + use crate::executor::telemetry::ReservationTelemetry; + #[derive(Default)] pub struct ReservationSession { pub reservations: HashMap, pub open: bool, + pub telemetry: ReservationTelemetry, } } @@ -409,7 +411,7 @@ where } // Keep the reserved_remaining_gauge{sender} telemetry in sync with the ledger. - telemetry::reservation_remaining_update(sender, &remaining); + session.telemetry.reservation_remaining_update(sender, &remaining); Ok(()) } Entry::Vacant(_) => Err(ReservationError::ReservationInvariant(format!( @@ -670,18 +672,31 @@ where // Track settlement metrics, including the virtual refund realized via reservation // release. - telemetry::settlement_record( - &base_fee_burn, - &miner_tip, - &over_estimation_burn, - Some(&refund), - ); + self.reservation_session + .lock() + .expect("reservation session mutex poisoned") + .telemetry + .settlement_record( + &base_fee_burn, + &miner_tip, + &over_estimation_burn, + Some(&refund), + ); } else { // Legacy behavior: refund unused gas directly to the sender. transfer_to_actor(sender_id, &refund)?; // Track settlement metrics in legacy mode as well, without a virtual refund component. - telemetry::settlement_record(&base_fee_burn, &miner_tip, &over_estimation_burn, None); + self.reservation_session + .lock() + .expect("reservation session mutex poisoned") + .telemetry + .settlement_record( + &base_fee_burn, + &miner_tip, + &over_estimation_burn, + None, + ); } if (&base_fee_burn + &over_estimation_burn + &refund + &miner_tip) != gas_cost { @@ -734,7 +749,11 @@ where const MAX_SENDERS: usize = 65_536; if plan.len() > MAX_SENDERS { - telemetry::reservation_begin_failed(); + self.reservation_session + .lock() + .expect("reservation session mutex poisoned") + .telemetry + .reservation_begin_failed(); return Err(ReservationError::PlanTooLarge); } @@ -744,7 +763,7 @@ where .expect("reservation session mutex poisoned"); if session.open { - telemetry::reservation_begin_failed(); + session.telemetry.reservation_begin_failed(); return Err(ReservationError::SessionOpen); } @@ -794,12 +813,12 @@ where })?; if &actor_state.balance < reserved { - telemetry::reservation_begin_failed(); + session.telemetry.reservation_begin_failed(); return Err(ReservationError::InsufficientFundsAtBegin { sender: *actor_id }); } } - telemetry::reservation_begin_succeeded(&reservations); + session.telemetry.reservation_begin_succeeded(&reservations); session.reservations = reservations; session.open = true; @@ -826,7 +845,7 @@ where session.reservations.clear(); session.open = false; - telemetry::reservation_end_succeeded(); + session.telemetry.reservation_end_succeeded(); Ok(()) } diff --git a/fvm/src/executor/telemetry.rs b/fvm/src/executor/telemetry.rs index 26e22c345..3ddf5dce5 100644 --- a/fvm/src/executor/telemetry.rs +++ b/fvm/src/executor/telemetry.rs @@ -2,7 +2,6 @@ // SPDX-License-Identifier: Apache-2.0, MIT use std::collections::HashMap; -use std::sync::{Mutex, OnceLock}; use fvm_shared::ActorID; use fvm_shared::econ::TokenAmount; @@ -40,88 +39,56 @@ pub struct ReservationTelemetry { pub reserved_remaining_per_sender: HashMap, } -static TELEMETRY: OnceLock> = OnceLock::new(); - -fn metrics() -> &'static Mutex { - TELEMETRY.get_or_init(|| Mutex::new(ReservationTelemetry::default())) -} - -/// Record a successful reservation session begin with the per-sender totals. -pub fn reservation_begin_succeeded(reservations: &HashMap) { - let mut m = metrics() - .lock() - .expect("reservation telemetry mutex poisoned"); - m.reservations_open = m.reservations_open.saturating_add(1); - m.reservation_total_per_sender = reservations.clone(); - m.reserved_remaining_per_sender = reservations.clone(); -} - -/// Record a failed reservation session begin. -pub fn reservation_begin_failed() { - let mut m = metrics() - .lock() - .expect("reservation telemetry mutex poisoned"); - m.reservation_begin_failed = m.reservation_begin_failed.saturating_add(1); -} - -/// Record a successful reservation session end and clear per-sender gauges. -pub fn reservation_end_succeeded() { - let mut m = metrics() - .lock() - .expect("reservation telemetry mutex poisoned"); - m.reservations_open = m.reservations_open.saturating_sub(1); - m.reservation_total_per_sender.clear(); - m.reserved_remaining_per_sender.clear(); -} +impl ReservationTelemetry { + /// Record a successful reservation session begin with the per-sender totals. + pub fn reservation_begin_succeeded(&mut self, reservations: &HashMap) { + self.reservations_open = self.reservations_open.saturating_add(1); + self.reservation_total_per_sender = reservations.clone(); + self.reserved_remaining_per_sender = reservations.clone(); + } -/// Update the remaining reserved amount for a sender. -pub fn reservation_remaining_update(sender: ActorID, remaining: &TokenAmount) { - let mut m = metrics() - .lock() - .expect("reservation telemetry mutex poisoned"); - - if remaining.is_zero() { - m.reserved_remaining_per_sender.remove(&sender); - } else { - m.reserved_remaining_per_sender - .insert(sender, remaining.clone()); + /// Record a failed reservation session begin. + pub fn reservation_begin_failed(&mut self) { + self.reservation_begin_failed = self + .reservation_begin_failed + .saturating_add(1); } -} -/// Record settlement amounts for a single message. -/// -/// The `refund_virtual` argument should be `Some(refund)` in reservation mode, -/// where refunds are realized via reservation release instead of a direct -/// balance transfer. -pub fn settlement_record( - base_fee_burn: &TokenAmount, - miner_tip: &TokenAmount, - over_estimation_burn: &TokenAmount, - refund_virtual: Option<&TokenAmount>, -) { - let mut m = metrics() - .lock() - .expect("reservation telemetry mutex poisoned"); - m.settle_basefee_burn += base_fee_burn.clone(); - m.settle_tip_credit += miner_tip.clone(); - m.settle_overburn += over_estimation_burn.clone(); - - if let Some(refund) = refund_virtual { - m.settle_refund_virtual += refund.clone(); + /// Record a successful reservation session end and clear per-sender gauges. + pub fn reservation_end_succeeded(&mut self) { + self.reservations_open = self.reservations_open.saturating_sub(1); + self.reservation_total_per_sender.clear(); + self.reserved_remaining_per_sender.clear(); } -} -/// Snapshot the current reservation telemetry. -pub fn snapshot() -> ReservationTelemetry { - metrics() - .lock() - .expect("reservation telemetry mutex poisoned") - .clone() -} + /// Update the remaining reserved amount for a sender. + pub fn reservation_remaining_update(&mut self, sender: ActorID, remaining: &TokenAmount) { + if remaining.is_zero() { + self.reserved_remaining_per_sender.remove(&sender); + } else { + self.reserved_remaining_per_sender + .insert(sender, remaining.clone()); + } + } -#[cfg(test)] -pub fn reset() { - *metrics() - .lock() - .expect("reservation telemetry mutex poisoned") = ReservationTelemetry::default(); + /// Record settlement amounts for a single message. + /// + /// The `refund_virtual` argument should be `Some(refund)` in reservation mode, + /// where refunds are realized via reservation release instead of a direct + /// balance transfer. + pub fn settlement_record( + &mut self, + base_fee_burn: &TokenAmount, + miner_tip: &TokenAmount, + over_estimation_burn: &TokenAmount, + refund_virtual: Option<&TokenAmount>, + ) { + self.settle_basefee_burn += base_fee_burn.clone(); + self.settle_tip_credit += miner_tip.clone(); + self.settle_overburn += over_estimation_burn.clone(); + + if let Some(refund) = refund_virtual { + self.settle_refund_virtual += refund.clone(); + } + } } From f16e3bcd047160a206908a581d3e38b90ea326b5 Mon Sep 17 00:00:00 2001 From: Gemini Agent Date: Fri, 5 Dec 2025 12:33:53 -1000 Subject: [PATCH 07/12] Refactor: Minimize reservation_session lock scope in FVM executor The reservation session mutex in the FVM executor's method was previously held during I/O-intensive state tree lookups and actor state loads. This could lead to increased contention and block other threads in multi-threaded environments or when dealing with slow blockstores. This commit refactors the method to: 1. Perform all state tree lookups, address resolution, and affordability checks *before* acquiring the mutex. 2. Acquire the mutex only when it's necessary to update the state and its associated telemetry. 3. Introduce a helper (using a clone) to enable telemetry updates in error paths without holding the main session lock, ensuring failures are still logged appropriately. 4. Re-check after acquiring the lock to handle potential race conditions where another thread might have opened a session concurrently. This significantly reduces the critical section of the code, improving concurrency and performance. --- fvm/src/executor/default.rs | 81 +++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 34 deletions(-) diff --git a/fvm/src/executor/default.rs b/fvm/src/executor/default.rs index 57e7ebbd2..737cc62d8 100644 --- a/fvm/src/executor/default.rs +++ b/fvm/src/executor/default.rs @@ -757,34 +757,35 @@ where return Err(ReservationError::PlanTooLarge); } - let mut session = self - .reservation_session - .lock() - .expect("reservation session mutex poisoned"); - - if session.open { - session.telemetry.reservation_begin_failed(); - return Err(ReservationError::SessionOpen); - } + let session_arc = self.reservation_session.clone(); + let record_failure = || { + session_arc + .lock() + .expect("reservation session mutex poisoned") + .telemetry + .reservation_begin_failed(); + }; // Aggregate per-actor reservations. let mut reservations: HashMap = HashMap::new(); for (addr, amount) in plan { // Resolve address to ActorID via the state tree. - let sender_id = self - .state_tree() - .lookup_id(addr) - .map_err(|e| { - ReservationError::ReservationInvariant(format!( - "failed to lookup actor {addr}: {e}" - )) - })? - .ok_or_else(|| { - ReservationError::ReservationInvariant(format!( + let sender_id = match self.state_tree().lookup_id(addr) { + Ok(Some(id)) => id, + Ok(None) => { + record_failure(); + return Err(ReservationError::ReservationInvariant(format!( "failed to resolve address {addr} to actor ID" - )) - })?; + ))); + } + Err(e) => { + record_failure(); + return Err(ReservationError::ReservationInvariant(format!( + "failed to lookup actor {addr}: {e}" + ))); + } + }; if amount.is_zero() { continue; @@ -798,26 +799,38 @@ where // Check affordability per sender: Σ(plan) ≤ actor.balance. for (actor_id, reserved) in &reservations { - let actor_state = self - .state_tree() - .get_actor(*actor_id) - .map_err(|e| { - ReservationError::ReservationInvariant(format!( - "failed to load actor {actor_id}: {e}" - )) - })? - .ok_or_else(|| { - ReservationError::ReservationInvariant(format!( + let actor_state = match self.state_tree().get_actor(*actor_id) { + Ok(Some(state)) => state, + Ok(None) => { + record_failure(); + return Err(ReservationError::ReservationInvariant(format!( "reservation plan includes unknown actor {actor_id}" - )) - })?; + ))); + } + Err(e) => { + record_failure(); + return Err(ReservationError::ReservationInvariant(format!( + "failed to load actor {actor_id}: {e}" + ))); + } + }; if &actor_state.balance < reserved { - session.telemetry.reservation_begin_failed(); + record_failure(); return Err(ReservationError::InsufficientFundsAtBegin { sender: *actor_id }); } } + let mut session = self + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + + if session.open { + session.telemetry.reservation_begin_failed(); + return Err(ReservationError::SessionOpen); + } + session.telemetry.reservation_begin_succeeded(&reservations); session.reservations = reservations; From 5f78b87559b9cb1c30855437e24a99d1cd4bbf33 Mon Sep 17 00:00:00 2001 From: Mikers Date: Fri, 5 Dec 2025 12:35:32 -1000 Subject: [PATCH 08/12] Update fvm/src/executor/default.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- fvm/src/executor/default.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/fvm/src/executor/default.rs b/fvm/src/executor/default.rs index 737cc62d8..cbbada46b 100644 --- a/fvm/src/executor/default.rs +++ b/fvm/src/executor/default.rs @@ -787,6 +787,12 @@ where } }; + if amount.is_negative() { + telemetry::reservation_begin_failed(); + return Err(ReservationError::ReservationInvariant( + format!("negative reservation amount for {addr}: {amount}") + )); + } if amount.is_zero() { continue; } From 7d7d0aa7b72fb8f60ba9cd1d4eeca5bf7065d8bb Mon Sep 17 00:00:00 2001 From: Gemini Agent Date: Fri, 5 Dec 2025 12:37:17 -1000 Subject: [PATCH 09/12] cargo fmt --- fvm/src/executor/default.rs | 17 +++++++---------- fvm/src/executor/telemetry.rs | 4 +--- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/fvm/src/executor/default.rs b/fvm/src/executor/default.rs index cbbada46b..020a6aaf9 100644 --- a/fvm/src/executor/default.rs +++ b/fvm/src/executor/default.rs @@ -411,7 +411,9 @@ where } // Keep the reserved_remaining_gauge{sender} telemetry in sync with the ledger. - session.telemetry.reservation_remaining_update(sender, &remaining); + session + .telemetry + .reservation_remaining_update(sender, &remaining); Ok(()) } Entry::Vacant(_) => Err(ReservationError::ReservationInvariant(format!( @@ -691,12 +693,7 @@ where .lock() .expect("reservation session mutex poisoned") .telemetry - .settlement_record( - &base_fee_burn, - &miner_tip, - &over_estimation_burn, - None, - ); + .settlement_record(&base_fee_burn, &miner_tip, &over_estimation_burn, None); } if (&base_fee_burn + &over_estimation_burn + &refund + &miner_tip) != gas_cost { @@ -789,9 +786,9 @@ where if amount.is_negative() { telemetry::reservation_begin_failed(); - return Err(ReservationError::ReservationInvariant( - format!("negative reservation amount for {addr}: {amount}") - )); + return Err(ReservationError::ReservationInvariant(format!( + "negative reservation amount for {addr}: {amount}" + ))); } if amount.is_zero() { continue; diff --git a/fvm/src/executor/telemetry.rs b/fvm/src/executor/telemetry.rs index 3ddf5dce5..84c987124 100644 --- a/fvm/src/executor/telemetry.rs +++ b/fvm/src/executor/telemetry.rs @@ -49,9 +49,7 @@ impl ReservationTelemetry { /// Record a failed reservation session begin. pub fn reservation_begin_failed(&mut self) { - self.reservation_begin_failed = self - .reservation_begin_failed - .saturating_add(1); + self.reservation_begin_failed = self.reservation_begin_failed.saturating_add(1); } /// Record a successful reservation session end and clear per-sender gauges. From 04d3957cfb82f42feb17f3d9a4da448c441c7da8 Mon Sep 17 00:00:00 2001 From: Gemini Agent Date: Fri, 5 Dec 2025 12:40:49 -1000 Subject: [PATCH 10/12] Docs: Add documentation for ReservationSession struct Also fix a build error in where the module was being accessed directly instead of using the closure. --- fvm/src/executor/default.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/fvm/src/executor/default.rs b/fvm/src/executor/default.rs index 020a6aaf9..6a64211c3 100644 --- a/fvm/src/executor/default.rs +++ b/fvm/src/executor/default.rs @@ -36,6 +36,11 @@ mod reservation { use crate::executor::telemetry::ReservationTelemetry; + /// Tracks the gas reservation ledger for a tipset-scope session. + /// + /// The ledger maintains per-actor reservation amounts that are decremented + /// as messages are processed. All entries must reach zero before the session + /// can be closed. #[derive(Default)] pub struct ReservationSession { pub reservations: HashMap, @@ -785,7 +790,7 @@ where }; if amount.is_negative() { - telemetry::reservation_begin_failed(); + record_failure(); return Err(ReservationError::ReservationInvariant(format!( "negative reservation amount for {addr}: {amount}" ))); From 280b781bd8ff6ce0dbfefae8d817e19b5acfb8ef Mon Sep 17 00:00:00 2001 From: Gemini Agent Date: Fri, 5 Dec 2025 12:50:54 -1000 Subject: [PATCH 11/12] Test: Add coverage for negative reservations and telemetry updates This commit adds two new test cases to : 1. : Verifies that providing a negative reservation amount returns a error and correctly increments the failure telemetry counter. 2. : Comprehensively tests the lifecycle of the telemetry state, ensuring success/failure counters and open session gauges are updated correctly during session begin and end. These tests address missing patch coverage reported by Codecov. --- fvm/src/executor/default.rs | 75 +++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/fvm/src/executor/default.rs b/fvm/src/executor/default.rs index 6a64211c3..3d959f5dc 100644 --- a/fvm/src/executor/default.rs +++ b/fvm/src/executor/default.rs @@ -1919,4 +1919,79 @@ mod tests { .tests(100) .quickcheck(prop as fn(u64, u64, TokenAmount, TokenAmount) -> TestResult); } + + #[test] + fn negative_reservation_amount_fails() { + let sender: ActorID = 50; + let mut exec = new_executor_with_actor(sender, TokenAmount::from_atto(1000u64)); + // Construct a negative amount. + // TokenAmount is a wrapper around BigInt. + let negative_amt = TokenAmount::from_atto(100u64) - TokenAmount::from_atto(200u64); + assert!(negative_amt.is_negative()); + + let plan = vec![(Address::new_id(sender), negative_amt.clone())]; + + let err = exec.begin_reservation_session(&plan).unwrap_err(); + match err { + ReservationError::ReservationInvariant(msg) => { + assert!(msg.contains("negative reservation amount")); + } + other => panic!("expected ReservationInvariant, got {:?}", other), + } + + // Verify telemetry recorded the failure + let session = exec + .reservation_session + .lock() + .expect("reservation session mutex poisoned"); + assert_eq!(session.telemetry.reservation_begin_failed, 1); + } + + #[test] + fn telemetry_state_updates() { + let sender: ActorID = 60; + let mut exec = new_executor_with_actor(sender, TokenAmount::from_atto(1_000_000u64)); + let plan = vec![(Address::new_id(sender), TokenAmount::from_atto(500u64))]; + + // 1. Test failure increment + // Use a too-large plan to force failure + { + let poor_sender = 61; + let account_code = *exec.builtin_actors().get_account_code(); + exec.state_tree_mut() + .set_actor(poor_sender, ActorState::new_empty(account_code, None)); + let fail_plan = vec![(Address::new_id(poor_sender), TokenAmount::from_atto(10u64))]; + + exec.begin_reservation_session(&fail_plan).unwrap_err(); + + let session = exec.reservation_session.lock().unwrap(); + assert_eq!(session.telemetry.reservation_begin_failed, 1); + assert_eq!(session.telemetry.reservations_open, 0); + } + + // 2. Test success increment + exec.begin_reservation_session(&plan).unwrap(); + { + let session = exec.reservation_session.lock().unwrap(); + assert_eq!(session.telemetry.reservation_begin_failed, 1); // unchanged + assert_eq!(session.telemetry.reservations_open, 1); + assert_eq!(session.telemetry.reservation_total_per_sender.len(), 1); + assert_eq!(session.telemetry.reserved_remaining_per_sender.len(), 1); + } + + // 3. Test end session decrement + // We must clear reservations manually to allow end_session (simulating consumption) + { + let mut session = exec.reservation_session.lock().unwrap(); + session.reservations.clear(); + } + + exec.end_reservation_session().unwrap(); + { + let session = exec.reservation_session.lock().unwrap(); + assert_eq!(session.telemetry.reservations_open, 0); + assert!(session.telemetry.reservation_total_per_sender.is_empty()); + assert!(session.telemetry.reserved_remaining_per_sender.is_empty()); + } + } } From fc0efc90030ed65f1644c73174d3f04373064de8 Mon Sep 17 00:00:00 2001 From: Gemini Agent Date: Fri, 5 Dec 2025 13:11:34 -1000 Subject: [PATCH 12/12] Docs: Clarify self_destruct failure modes in comment --- fvm/src/kernel/default.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index bd0f91717..a96a1cfc7 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -364,8 +364,8 @@ where // If there are remaining funds, burn them. We do this instead of letting the user to // specify the beneficiary as: // - // 1. This lets the user handle transfer failure cases themselves. The only way _this_ can - // fail is for the caller to run out of gas. + // 1. This lets the user handle transfer failure cases themselves. This can fail if the + // caller runs out of gas, or if the actor has active reservations preventing the transfer. // 2. If we ever decide to allow code on method 0, allowing transfers here would be // unfortunate. let balance = self.current_balance()?;