feat: add engine‑managed tipset gas reservations to ref‑fvm #2236
Conversation
snissn
force-pushed
the
multistage-execution
branch
from
770523f to
56c2695
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2236 +/- ##
==========================================
+ Coverage 77.58% 78.80% +1.22%
==========================================
Files 147 148 +1
Lines 15789 16927 +1138
==========================================
+ Hits 12250 13340 +1090
- Misses 3539 3587 +48
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements engine-managed tipset-scope gas reservations in ref-fvm to prevent miners from being exposed to intra-tipset underfunded messages. The implementation adds a reservation session ledger that tracks per-actor gas commitments throughout tipset execution, with settlement realizing refunds through reservation release rather than direct balance transfers.
Key changes:
- Add
ReservationSessionledger and lifecycle methods (begin_reservation_session/end_reservation_session) with affordability checks - Modify preflight to assert coverage without pre-deducting funds in reservation mode
- Enforce transfer restrictions against free balance (
balance - reserved_remaining) - Update settlement to net-charge gas costs and realize refunds via reservation release
- Add reservation telemetry tracking and comprehensive test coverage
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
fvm/src/executor/mod.rs |
Defines ReservationError enum and re-exports ReservationSession struct for public API |
fvm/src/executor/default.rs |
Core implementation of reservation session lifecycle, preflight validation, settlement logic, and extensive unit tests |
fvm/src/executor/telemetry.rs |
New telemetry module for tracking reservation metrics using global singleton with per-sender gauges |
fvm/src/call_manager/mod.rs |
Updates CallManager::new trait signature to accept ReservationSession parameter |
fvm/src/call_manager/default.rs |
Implements reservation-aware transfer enforcement by checking free balance against reserved amounts |
fvm/src/kernel/default.rs |
Minor formatting change to self-destruct error handling to route through reservation-aware transfer |
fvm/tests/dummy.rs |
Updates dummy CallManager implementation to accept new ReservationSession argument |
testing/integration/tests/reservation_transfer_enforcement.rs |
New integration tests verifying reservation enforcement for sends and actor creation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn reservation_begin_succeeded(reservations: &HashMap<ActorID, TokenAmount>) { | ||
| 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(); | ||
| } |
There was a problem hiding this comment.
Potential issue with global telemetry state
The telemetry module uses a process-global OnceLock<Mutex<ReservationTelemetry>> singleton. When multiple reservation sessions are opened concurrently (e.g., in a multi-threaded test environment or if the host processes multiple tipsets in parallel), line 55-56 will overwrite reservation_total_per_sender and reserved_remaining_per_sender with the latest session's data, losing information about concurrent sessions.
This could lead to:
- Incorrect gauge values when sessions overlap
- Incomplete telemetry data
- Difficulty debugging issues in multi-executor scenarios
Consider either:
- Documenting that only one session should be active globally at a time
- Using per-executor telemetry instead of a global singleton
- Aggregating data from multiple concurrent sessions instead of replacing
| 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<ActorID, TokenAmount> = 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(()) | ||
| } |
There was a problem hiding this comment.
Holding mutex while performing I/O operations
The code acquires the reservation_session mutex on line 741-744 and holds it throughout the entire reservation plan validation, including:
- State tree lookups (line 757-768)
- Actor state loads (line 783-794)
These operations could potentially be slow (especially if the state tree is backed by a slow blockstore), and holding the mutex during these I/O operations could:
- Block other threads that need to check reservation status during transfers
- Increase contention in multi-threaded scenarios
Consider refactoring to:
- Release the mutex before performing state tree operations
- Re-acquire it only when updating
session.reservationsandsession.open - Add a check after re-acquiring to ensure the session wasn't opened by another thread in the meantime
| "failed to resolve address {addr} to actor ID" | ||
| )) | ||
| })?; | ||
|
|
There was a problem hiding this comment.
Missing validation for negative reservation amounts
The code checks if amount.is_zero() on line 770 and skips zero entries, but it doesn't validate that amount is positive. If the plan contains negative amounts, they will be:
- Added to the
reservationsmap (line 774-777), potentially creating negative reservation totals - Compared against actor balance on line 796, which could pass the check if the negative value is less than the balance
Negative reservations don't make semantic sense and could lead to undefined behavior. Consider adding validation:
if amount.is_negative() {
telemetry::reservation_begin_failed();
return Err(ReservationError::ReservationInvariant(
format!("negative reservation amount for {addr}: {amount}")
));
}
if amount.is_zero() {
continue;
}| if amount.is_negative() { | |
| telemetry::reservation_begin_failed(); | |
| return Err(ReservationError::ReservationInvariant( | |
| format!("negative reservation amount for {addr}: {amount}") | |
| )); | |
| } |
| #[derive(Default)] | ||
| pub struct ReservationSession { | ||
| pub reservations: HashMap<ActorID, TokenAmount>, | ||
| pub open: bool, | ||
| } |
There was a problem hiding this comment.
Missing documentation for public ReservationSession struct
The ReservationSession struct is exported publicly (line 30, re-exported from line 10 in mod.rs) but lacks any documentation comments explaining:
- Its purpose and role in the reservation system
- The meaning of the
reservationsandopenfields - The invariants that should be maintained (e.g., reservations should sum to <= actor balance)
- Thread-safety expectations (it's wrapped in Arc<Mutex<>> by the executor)
Consider adding a doc comment like:
/// 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 { ... }
feat: add engine‑managed tipset gas reservations to ref‑fvm
This PR implements engine‑managed tipset‑scope gas reservations inside ref‑fvm, as described in a pending FIP proposal.
Summary
ActorID.begin_reservation_session/end_reservation_sessionwith affordability checks and invariants.balance − reserved_remaining.Changes
Executor: session lifecycle and preflight
fvm/src/executor/mod.rsReservationSessionstruct (re‑exported fromdefault.rs) andReservationErrorenum:NotImplementedInsufficientFundsAtBegin { sender }SessionOpenSessionClosedNonZeroRemainderPlanTooLargeOverflowReservationInvariant(String)fvm/src/executor/default.rsReservationSession { reservations: HashMap<ActorID, TokenAmount>, open: bool }and anArc<Mutex<_>>onDefaultExecutor.begin_reservation_session(&mut self, plan: &[(Address, TokenAmount)]) -> Result<(), ReservationError>:MAX_SENDERS(65,536) and track plan failures via telemetry.ActorID).reserved_total <= balanceper sender.end_reservation_session(&mut self) -> Result<(), ReservationError>:open == trueand all reservation entries to be zero.gas_cost = gas_fee_cap * gas_limitusing big‑int; treat negative results asReservationError::Overflow.reservation_assert_coverage(sender, &gas_cost); do not pre‑deduct funds.reservation_prevalidation_decrementso the ledger can end at zero.Transfer enforcement and settlement
fvm/src/call_manager/default.rs&fvm/src/call_manager/mod.rsArc<Mutex<ReservationSession>>into the default call manager.transfer(from, to, value):reserved_remaining = reservations.get(from).unwrap_or(0).value + reserved_remaining <= from.balance; otherwise returnInsufficientFunds.value <= balancesemantics.fvm/src/executor/default.rsfinish_message) in reservation mode:GasOutputsas today.consumption = base_fee_burn + over_estimation_burn + miner_tip.consumptionfrom the sender’s actor balance.refundto the sender; the “refund effect” is realized by releasing the reservation ledger.reservations[sender]bygas_costusingreservation_prevalidation_decrement, update telemetry, and remove entries at zero.base_fee_burn + over_estimation_burn + refund + miner_tip == gas_cost.Telemetry
fvm/src/executor/telemetry.rsReservationTelemetryand helpers:reservations_openreservation_begin_failedsettle_basefee_burnsettle_tip_creditsettle_overburnsettle_refund_virtualsnapshot()(for potential host export) andreset()under#[cfg(test)].Kernel and test harness adjustments
fvm/src/kernel/default.rsCallManagertype where necessary so that all value‑moving operations (SendOps, SELFDESTRUCT, etc.) route through the reservation‑awaretransfer.fvm/tests/dummy.rsCallManagerimpl to accept the newReservationSessionargument inCallManager::new, keeping tests compiling against the updated trait.Tests
Unit tests in
fvm/src/executor/default.rs:ReservationError::Overflow.transfer, send to existing actors, SELFDESTRUCT, and implicit sends must respect free balance.consumption.end_reservation_sessionsucceeds.--features arb) continues to assert:base_fee_burn + over_estimation_burn + refund + miner_tip == gas_cost.Integration tests:
testing/integration/tests/reservation_transfer_enforcement.rs:value > free = balance − reserved_remaining.Activation and host behaviour
begin_reservation_session/end_reservation_session.ReservationErrorvariants (legacy fallback, tipset invalid, node error) based on network version and feature flags.Notes
ExitCode,GasUsed, events) andGasOutputsrelative to pre‑reservation behaviour, while removing miner exposure to intra‑tipset underfunded messages when hosts enable reservations.