From 66b64bf86583cf14d018816f466009437fbf008e Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 20 Feb 2025 18:07:51 -0800 Subject: [PATCH 1/8] test: correctly handle cron vesting in tests 1. In the test harness, check if we should vest every epoch. The queue should already be quantized. 2. Correctly handle the fact that we vest at the _end_ of an epoch in the cron vesting test. --- actors/miner/tests/deadline_cron.rs | 11 ++++++----- actors/miner/tests/util.rs | 9 +-------- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/actors/miner/tests/deadline_cron.rs b/actors/miner/tests/deadline_cron.rs index 1adc3305f..945bd3f36 100644 --- a/actors/miner/tests/deadline_cron.rs +++ b/actors/miner/tests/deadline_cron.rs @@ -5,7 +5,7 @@ use fil_actor_miner::{ }; use fil_actors_runtime::runtime::RuntimePolicy; use fil_actors_runtime::test_utils::MockRuntime; -use fil_actors_runtime::{MessageAccumulator, EPOCHS_IN_DAY}; +use fil_actors_runtime::{MessageAccumulator, EPOCHS_IN_DAY, EPOCHS_IN_HOUR}; use fvm_ipld_bitfield::BitField; use fvm_shared::bigint::Zero; use fvm_shared::clock::ChainEpoch; @@ -91,16 +91,17 @@ fn test_vesting_on_cron() { } }; - // move current epoch by a day so funds get vested - let new_epoch = q.quantize_up(current_epoch + EPOCHS_IN_DAY + 10); + // move current epoch by a day so funds get vested. +1 because rewards are vested at the end of + // an epoch. + let new_epoch = q.quantize_up(current_epoch + 12 * EPOCHS_IN_HOUR) + 1; assert_locked_fn(new_epoch, true); // no funds get vested if epoch moves by <12 hours - let new_epoch = new_epoch + (EPOCHS_IN_DAY / 2) - 100; + let new_epoch = new_epoch + (12 * EPOCHS_IN_HOUR) - 100; assert_locked_fn(new_epoch, false); // funds get vested again if epoch is quantised - let new_epoch = q.quantize_up(new_epoch); + let new_epoch = q.quantize_up(new_epoch) + 1; assert_locked_fn(new_epoch, true); h.check_state(&rt); diff --git a/actors/miner/tests/util.rs b/actors/miner/tests/util.rs index fdbceb349..36154067f 100644 --- a/actors/miner/tests/util.rs +++ b/actors/miner/tests/util.rs @@ -78,7 +78,7 @@ use fil_actor_miner::{ SectorReturn, SectorUpdateManifest, Sectors, State, SubmitWindowedPoStParams, TerminateSectorsParams, TerminationDeclaration, VerifiedAllocationKey, VestingFunds, WindowedPoSt, WithdrawBalanceParams, WithdrawBalanceReturn, CRON_EVENT_PROVING_DEADLINE, - NI_AGGREGATE_FEE_BASE_SECTOR_COUNT, NO_QUANTIZATION, REWARD_VESTING_SPEC, SECTORS_AMT_BITWIDTH, + NI_AGGREGATE_FEE_BASE_SECTOR_COUNT, NO_QUANTIZATION, SECTORS_AMT_BITWIDTH, SECTOR_CONTENT_CHANGED, }; use fil_actor_miner::{ @@ -3307,13 +3307,6 @@ enum MhCode { fn immediately_vesting_funds(rt: &MockRuntime, state: &State) -> TokenAmount { let curr_epoch = *rt.epoch.borrow(); - - let q = - QuantSpec { unit: REWARD_VESTING_SPEC.quantization, offset: state.proving_period_start }; - if q.quantize_up(curr_epoch) != curr_epoch { - return TokenAmount::zero(); - } - let vesting = rt.store.get_cbor::(&state.vesting_funds).unwrap().unwrap(); let mut sum = TokenAmount::zero(); for vf in vesting.funds { From 956552126bc8ce598274a62057e66d09b4cf5d5d Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 20 Feb 2025 17:28:38 -0800 Subject: [PATCH 2/8] feat: refactor vesting funds into a head and a tail This lets us efficiently take fees from vesting funds without loading/storing this object each time. It also lets us check if there are funds to vest without having to load any additional state, because the "next" batch of vesting funds are always stored in the root state object. If FIP-0100 passes, we'll likely want this change as we'll otherwise read/write the vesting funds queue on every deadline for every miner. fixes #1594 --- actors/miner/src/lib.rs | 74 +++---- actors/miner/src/state.rs | 117 ++++------- actors/miner/src/testing.rs | 4 +- actors/miner/src/vesting_state.rs | 196 +++++++++++++------ actors/miner/tests/apply_rewards.rs | 12 +- actors/miner/tests/deadline_cron.rs | 4 +- actors/miner/tests/repay_debt_test.rs | 4 +- actors/miner/tests/state_harness.rs | 7 +- actors/miner/tests/util.rs | 23 +-- actors/miner/tests/vesting_unvested_funds.rs | 39 ++-- 10 files changed, 253 insertions(+), 227 deletions(-) diff --git a/actors/miner/src/lib.rs b/actors/miner/src/lib.rs index c8f047b3d..7199e68bc 100644 --- a/actors/miner/src/lib.rs +++ b/actors/miner/src/lib.rs @@ -308,11 +308,9 @@ impl Actor { fn get_vesting_funds(rt: &impl Runtime) -> Result { rt.validate_immediate_caller_accept_any()?; let state: State = rt.state()?; - let vesting_funds = state - .load_vesting_funds(rt.store()) - .map_err(|e| actor_error!(illegal_state, "failed to load vesting funds: {}", e))?; - let ret = vesting_funds.funds.into_iter().map(|v| (v.epoch, v.amount)).collect_vec(); - Ok(GetVestingFundsReturn { vesting_funds: ret }) + let vesting_funds = + state.vesting_funds.load(rt.store())?.map(|v| (v.epoch, v.amount)).collect_vec(); + Ok(GetVestingFundsReturn { vesting_funds }) } /// Will ALWAYS overwrite the existing control addresses with the control addresses passed in the params. @@ -1407,7 +1405,7 @@ impl Actor { let penalty_target = &penalty_base + &reward_target; st.apply_penalty(&penalty_target) .map_err(|e| actor_error!(illegal_state, "failed to apply penalty {}", e))?; - let (penalty_from_vesting, penalty_from_balance) = st + let (to_burn, total_unlocked) = st .repay_partial_debt_in_priority_order( rt.store(), current_epoch, @@ -1417,13 +1415,11 @@ impl Actor { e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to pay debt") })?; - let to_burn = &penalty_from_vesting + &penalty_from_balance; - // Now, move as much of the target reward as // we can from the burn to the reward. let to_reward = std::cmp::min(&to_burn, &reward_target); let to_burn = &to_burn - to_reward; - let pledge_delta = penalty_from_vesting.neg(); + let pledge_delta = total_unlocked.neg(); Ok((pledge_delta, to_burn, power_delta, to_reward.clone())) })?; @@ -3147,7 +3143,7 @@ impl Actor { // Attempt to repay all fee debt in this call. In most cases the miner will have enough // funds in the *reward alone* to cover the penalty. In the rare case a miner incurs more // penalty than it can pay for with reward and existing funds, it will go into fee debt. - let (penalty_from_vesting, penalty_from_balance) = st + let (to_burn, total_unlocked) = st .repay_partial_debt_in_priority_order( rt.store(), rt.curr_epoch(), @@ -3156,8 +3152,7 @@ impl Actor { .map_err(|e| { e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to repay penalty") })?; - pledge_delta_total -= &penalty_from_vesting; - let to_burn = penalty_from_vesting + penalty_from_balance; + pledge_delta_total -= &total_unlocked; Ok((pledge_delta_total, to_burn)) })?; @@ -3232,7 +3227,7 @@ impl Actor { })?; // Pay penalty - let (penalty_from_vesting, penalty_from_balance) = st + let (mut burn_amount, total_unlocked) = st .repay_partial_debt_in_priority_order( rt.store(), rt.curr_epoch(), @@ -3242,8 +3237,7 @@ impl Actor { e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to pay fees") })?; - let mut burn_amount = &penalty_from_vesting + &penalty_from_balance; - pledge_delta -= penalty_from_vesting; + pledge_delta -= total_unlocked; // clamp reward at funds burnt let reward_amount = std::cmp::min(&burn_amount, &slasher_reward).clone(); @@ -3524,14 +3518,14 @@ impl Actor { } fn repay_debt(rt: &impl Runtime) -> Result<(), ActorError> { - let (from_vesting, from_balance, state) = rt.transaction(|state: &mut State, rt| { + let (burn_amount, total_unlocked, state) = rt.transaction(|state: &mut State, rt| { let info = get_miner_info(rt.store(), state)?; rt.validate_immediate_caller_is( info.control_addresses.iter().chain(&[info.worker, info.owner]), )?; // Repay as much fee debt as possible. - let (from_vesting, from_balance) = state + let (burn_amount, total_unlocked) = state .repay_partial_debt_in_priority_order( rt.store(), rt.curr_epoch(), @@ -3541,11 +3535,10 @@ impl Actor { e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to unlock fee debt") })?; - Ok((from_vesting, from_balance, state.clone())) + Ok((burn_amount, total_unlocked, state.clone())) })?; - let burn_amount = from_balance + &from_vesting; - notify_pledge_changed(rt, &from_vesting.neg())?; + notify_pledge_changed(rt, &total_unlocked.neg())?; burn_funds(rt, burn_amount)?; state.check_balance_invariants(&rt.current_balance()).map_err(balance_invariants_broken)?; @@ -4400,7 +4393,7 @@ fn process_early_terminations( })?; // Use unlocked pledge to pay down outstanding fee debt - let (penalty_from_vesting, penalty_from_balance) = state + let (penalty, total_unlocked) = state .repay_partial_debt_in_priority_order( rt.store(), rt.curr_epoch(), @@ -4410,8 +4403,7 @@ fn process_early_terminations( e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to repay penalty") })?; - let penalty = &penalty_from_vesting + penalty_from_balance; - pledge_delta -= penalty_from_vesting; + pledge_delta -= total_unlocked; Ok((result, more, penalty, pledge_delta)) })?; @@ -4464,25 +4456,6 @@ fn handle_proving_deadline( let state: State = rt.transaction(|state: &mut State, rt| { let policy = rt.policy(); - // Vesting rewards for a miner are quantized to every 12 hours and we can determine what those "vesting epochs" are. - // So, only do the vesting here if the current epoch is a "vesting epoch" - let q = QuantSpec { - unit: REWARD_VESTING_SPEC.quantization, - offset: state.proving_period_start, - }; - - if q.quantize_up(curr_epoch) == curr_epoch { - // Vest locked funds. - // This happens first so that any subsequent penalties are taken - // from locked vesting funds before funds free this epoch. - let newly_vested = - state.unlock_vested_funds(rt.store(), rt.curr_epoch()).map_err(|e| { - e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to vest funds") - })?; - - pledge_delta_total -= newly_vested; - } - // Process pending worker change if any let mut info = get_miner_info(rt.store(), state)?; process_pending_worker(&mut info, rt, state)?; @@ -4534,7 +4507,7 @@ fn handle_proving_deadline( penalty_target ); - let (penalty_from_vesting, penalty_from_balance) = state + let (penalty, total_unlocked) = state .repay_partial_debt_in_priority_order( rt.store(), rt.curr_epoch(), @@ -4544,8 +4517,19 @@ fn handle_proving_deadline( e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to unlock penalty") })?; - penalty_total = &penalty_from_vesting + penalty_from_balance; - pledge_delta_total -= penalty_from_vesting; + penalty_total = penalty; + pledge_delta_total -= total_unlocked; + + // Vest locked funds. Locked funds will already have been vested automatically if we paid + // any fees/penalties, but we try to vest one more time just in case. + // + // If there's nothing to vest, this is an inexpensive operation as it'll just look at the + // "head" of the vesting queue, which is inlined into the root state object. + let newly_vested = state + .unlock_vested_funds(rt.store(), rt.curr_epoch()) + .map_err(|e| e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to vest funds"))?; + + pledge_delta_total -= newly_vested; continue_cron = state.continue_deadline_cron(); if !continue_cron { diff --git a/actors/miner/src/state.rs b/actors/miner/src/state.rs index 0ba88729a..f62359abc 100644 --- a/actors/miner/src/state.rs +++ b/actors/miner/src/state.rs @@ -64,7 +64,7 @@ pub struct State { pub locked_funds: TokenAmount, /// VestingFunds (Vesting Funds schedule for the miner). - pub vesting_funds: Cid, + pub vesting_funds: VestingFunds, /// Absolute value of debt this miner owes from unpaid fees. pub fee_debt: TokenAmount, @@ -164,10 +164,7 @@ impl State { e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to construct illegal state") })?; - let empty_vesting_funds_cid = - store.put_cbor(&VestingFunds::new(), Code::Blake2b256).map_err(|e| { - e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to construct illegal state") - })?; + let vesting_funds = VestingFunds::new(store)?; Ok(Self { info: info_cid, @@ -175,7 +172,7 @@ impl State { pre_commit_deposits: TokenAmount::default(), locked_funds: TokenAmount::default(), - vesting_funds: empty_vesting_funds_cid, + vesting_funds, initial_pledge: TokenAmount::default(), fee_debt: TokenAmount::default(), @@ -783,28 +780,6 @@ impl State { Ok(()) } - /// Loads the vesting funds table from the store. - pub fn load_vesting_funds(&self, store: &BS) -> anyhow::Result { - Ok(store - .get_cbor(&self.vesting_funds) - .map_err(|e| { - e.downcast_wrap(format!("failed to load vesting funds {}", self.vesting_funds)) - })? - .ok_or_else( - || actor_error!(not_found; "failed to load vesting funds {:?}", self.vesting_funds), - )?) - } - - /// Saves the vesting table to the store. - pub fn save_vesting_funds( - &mut self, - store: &BS, - funds: &VestingFunds, - ) -> anyhow::Result<()> { - self.vesting_funds = store.put_cbor(funds, Code::Blake2b256)?; - Ok(()) - } - // Return true when the miner actor needs to continue scheduling deadline crons pub fn continue_deadline_cron(&self) -> bool { !self.pre_commit_deposits.is_zero() @@ -864,11 +839,17 @@ impl State { if vesting_sum.is_negative() { return Err(anyhow!("negative vesting sum {}", vesting_sum)); } + // add new funds and unlock already vested funds. + let amount_unlocked = self.vesting_funds.add_locked_funds( + store, + current_epoch, + vesting_sum, + self.proving_period_start, + spec, + )?; - let mut vesting_funds = self.load_vesting_funds(store)?; - - // unlock vested funds first - let amount_unlocked = vesting_funds.unlock_vested_funds(current_epoch); + // We shouldn't unlock any of the new funds, so the locked funds should remain non-negative + // when we deduct the amount unlocked. self.locked_funds -= &amount_unlocked; if self.locked_funds.is_negative() { return Err(anyhow!( @@ -877,20 +858,17 @@ impl State { amount_unlocked )); } - // add locked funds now - vesting_funds.add_locked_funds(current_epoch, vesting_sum, self.proving_period_start, spec); - self.locked_funds += vesting_sum; - // save the updated vesting table state - self.save_vesting_funds(store, &vesting_funds)?; + // Finally, record the new locked-funds total. + self.locked_funds += vesting_sum; Ok(amount_unlocked) } /// Draws from vesting table and unlocked funds to repay up to the fee debt. - /// Returns the amount unlocked from the vesting table and the amount taken from - /// current balance. If the fee debt exceeds the total amount available for repayment - /// the fee debt field is updated to track the remaining debt. Otherwise it is set to zero. + /// Returns the amount to burn and the total amount unlocked from the vesting table (both vested + /// and unvested) If the fee debt exceeds the total amount available for repayment the fee debt + /// field is updated to track the remaining debt. Otherwise it is set to zero. pub fn repay_partial_debt_in_priority_order( &mut self, store: &BS, @@ -898,25 +876,24 @@ impl State { curr_balance: &TokenAmount, ) -> Result< ( - TokenAmount, // from vesting - TokenAmount, // from balance + TokenAmount, // fee to burn + TokenAmount, // total unlocked ), anyhow::Error, > { - let unlocked_balance = self.get_unlocked_balance(curr_balance)?; - let fee_debt = self.fee_debt.clone(); - let from_vesting = self.unlock_unvested_funds(store, current_epoch, &fee_debt)?; + let (from_vesting, total_unlocked) = + self.unlock_unvested_funds(store, current_epoch, &fee_debt)?; if from_vesting > self.fee_debt { return Err(anyhow!("should never unlock more than the debt we need to repay")); } - self.fee_debt -= &from_vesting; - let from_balance = cmp::min(&unlocked_balance, &self.fee_debt).clone(); - self.fee_debt -= &from_balance; + let unlocked_balance = self.get_unlocked_balance(curr_balance)?; + let to_burn = cmp::min(&unlocked_balance, &self.fee_debt).clone(); + self.fee_debt -= &to_burn; - Ok((from_vesting, from_balance)) + Ok((to_burn, total_unlocked)) } /// Repays the full miner actor fee debt. Returns the amount that must be @@ -937,32 +914,34 @@ impl State { Ok(std::mem::take(&mut self.fee_debt)) } - /// Unlocks an amount of funds that have *not yet vested*, if possible. - /// The soonest-vesting entries are unlocked first. - /// Returns the amount actually unlocked. + + /// Unlocks all vested funds and then unlocks an amount of funds that have *not yet vested*, if + /// possible. The soonest-vesting entries are unlocked first. + /// + /// Returns the amount of unvested funds unlocked, along with the total amount of funds unlocked. pub fn unlock_unvested_funds( &mut self, store: &BS, current_epoch: ChainEpoch, target: &TokenAmount, - ) -> anyhow::Result { + ) -> anyhow::Result<(TokenAmount, TokenAmount)> { if target.is_zero() || self.locked_funds.is_zero() { - return Ok(TokenAmount::zero()); + return Ok((TokenAmount::zero(), TokenAmount::zero())); } - let mut vesting_funds = self.load_vesting_funds(store)?; - let amount_unlocked = vesting_funds.unlock_unvested_funds(current_epoch, target); - self.locked_funds -= &amount_unlocked; + let (unlocked_vested, unlocked_unvested) = + self.vesting_funds.unlock_vested_and_unvested_funds(store, current_epoch, target)?; + let total_unlocked = &unlocked_vested + &unlocked_unvested; + self.locked_funds -= &total_unlocked; if self.locked_funds.is_negative() { return Err(anyhow!( "negative locked funds {} after unlocking {}", self.locked_funds, - amount_unlocked + total_unlocked, )); } - self.save_vesting_funds(store, &vesting_funds)?; - Ok(amount_unlocked) + Ok((unlocked_unvested, total_unlocked)) } /// Unlocks all vesting funds that have vested before the provided epoch. @@ -976,8 +955,7 @@ impl State { return Ok(TokenAmount::zero()); } - let mut vesting_funds = self.load_vesting_funds(store)?; - let amount_unlocked = vesting_funds.unlock_vested_funds(current_epoch); + let amount_unlocked = self.vesting_funds.unlock_vested_funds(store, current_epoch)?; self.locked_funds -= &amount_unlocked; if self.locked_funds.is_negative() { return Err(anyhow!( @@ -986,24 +964,9 @@ impl State { )); } - self.save_vesting_funds(store, &vesting_funds)?; Ok(amount_unlocked) } - /// CheckVestedFunds returns the amount of vested funds that have vested before the provided epoch. - pub fn check_vested_funds( - &self, - store: &BS, - current_epoch: ChainEpoch, - ) -> anyhow::Result { - let vesting_funds = self.load_vesting_funds(store)?; - Ok(vesting_funds - .funds - .iter() - .take_while(|fund| fund.epoch < current_epoch) - .fold(TokenAmount::zero(), |acc, fund| acc + &fund.amount)) - } - /// Unclaimed funds that are not locked -- includes funds used to cover initial pledge requirement. pub fn get_unlocked_balance(&self, actor_balance: &TokenAmount) -> anyhow::Result { let unlocked_balance = diff --git a/actors/miner/src/testing.rs b/actors/miner/src/testing.rs index 64beb603a..1533caa45 100644 --- a/actors/miner/src/testing.rs +++ b/actors/miner/src/testing.rs @@ -273,10 +273,10 @@ fn check_miner_balances( // locked funds must be sum of vesting table and vesting table payments must be quantized let mut vesting_sum = TokenAmount::zero(); - match state.load_vesting_funds(store) { + match state.vesting_funds.load(store) { Ok(funds) => { let quant = state.quant_spec_every_deadline(policy); - funds.funds.iter().for_each(|entry| { + funds.into_iter().for_each(|entry| { acc.require( entry.amount.is_positive(), format!("non-positive amount in miner vesting table entry {entry:?}"), diff --git a/actors/miner/src/vesting_state.rs b/actors/miner/src/vesting_state.rs index de247d383..31bf6f9bd 100644 --- a/actors/miner/src/vesting_state.rs +++ b/actors/miner/src/vesting_state.rs @@ -1,54 +1,114 @@ // Copyright 2019-2022 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0, MIT -use std::{iter, mem}; +use std::iter; +use cid::Cid; +use fil_actors_runtime::{ActorError, AsActorError}; +use fvm_ipld_blockstore::Blockstore; use fvm_ipld_encoding::tuple::*; +use fvm_ipld_encoding::CborStore; use fvm_shared::clock::ChainEpoch; use fvm_shared::econ::TokenAmount; +use fvm_shared::error::ExitCode; use itertools::{EitherOrBoth, Itertools}; +use multihash_codetable::Code; use num_traits::Zero; use super::{QuantSpec, VestSpec}; // Represents miner funds that will vest at the given epoch. -#[derive(Debug, Serialize_tuple, Deserialize_tuple)] +#[derive(Default, Debug, Serialize_tuple, Deserialize_tuple, Clone)] pub struct VestingFund { pub epoch: ChainEpoch, pub amount: TokenAmount, } -/// Represents the vesting table state for the miner. -/// It is a slice of (VestingEpoch, VestingAmount). -/// The slice will always be sorted by the VestingEpoch. -#[derive(Serialize_tuple, Deserialize_tuple, Default)] +/// Represents the vesting table state for the miner. It's composed of a `head` (stored inline) and +/// a tail (referenced by CID). +#[derive(Serialize_tuple, Deserialize_tuple, Debug, Clone)] pub struct VestingFunds { - pub funds: Vec, + // The "next" batch of vesting funds: + // - If this is None, there are no vesting funds. + // - All batches in `tail` are guaranteed to vest after this batch. + // - This batch _can_ be empty (have a zero amount) if we've burnt through it (fees & + // penalties). + head: Option, + // The rest of the vesting funds, if any. + tail: Cid, // Vec } impl VestingFunds { - pub fn new() -> Self { - Default::default() + pub fn new(store: &impl Blockstore) -> Result { + let tail = store + .put_cbor(&Vec::::new(), Code::Blake2b256) + .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to construct vesting funds")?; + Ok(Self { head: None, tail }) } - pub fn unlock_vested_funds(&mut self, current_epoch: ChainEpoch) -> TokenAmount { - // TODO: the funds are sorted by epoch, so we could do a binary search here - let i = self - .funds - .iter() - .position(|fund| fund.epoch >= current_epoch) - .unwrap_or(self.funds.len()); + pub fn load( + &self, + store: &impl Blockstore, + ) -> Result, ActorError> { + // NOTE: we allow head to be drawn down to zero through fees/penalties. However, when + // inspecting the vesting table, we never want to see a "zero" entry so we skip it in that case. + // We don't set it to "none" in that case because "none" means that we have _no_ vesting funds. + let head = self.head.as_ref().filter(|h| h.amount.is_positive()).cloned(); + let tail: Vec<_> = store + .get_cbor(&self.tail) + .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to load the vesting funds")? + .context_code(ExitCode::USR_ILLEGAL_STATE, "missing vesting funds state")?; + Ok(itertools::chain(head, tail)) + } + + fn save( + &mut self, + store: &impl Blockstore, + funds: impl IntoIterator, + ) -> Result<(), ActorError> { + let mut funds = funds.into_iter(); + let head = funds.next(); + let tail = funds.collect_vec(); + + self.tail = store + .put_cbor(&tail, Code::Blake2b256) + .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to store the vesting funds")?; + // We do this second just in case the first operation fails to try to maintain consistent + // state. + self.head = head; + Ok(()) + } - self.funds.drain(..i).map(|f| f.amount).sum() + pub fn can_vest(&self, current_epoch: ChainEpoch) -> bool { + matches!(&self.head, Some(VestingFund { epoch, .. }) if *epoch < current_epoch) } + pub fn unlock_vested_funds( + &mut self, + store: &impl Blockstore, + current_epoch: ChainEpoch, + ) -> Result { + if !self.can_vest(current_epoch) { + return Ok(TokenAmount::zero()); + } + let mut funds = self.load(store)?.peekable(); + let unlocked = funds + .peeking_take_while(|fund| fund.epoch < current_epoch) + .map(|f| f.amount) + .sum::(); + self.save(store, funds)?; + Ok(unlocked) + } + + // Adds locked funds and unlocks everything that has already vested. pub fn add_locked_funds( &mut self, + store: &impl Blockstore, current_epoch: ChainEpoch, vesting_sum: &TokenAmount, proving_period_start: ChainEpoch, spec: &VestSpec, - ) { + ) -> Result { // Quantization is aligned with when regular cron will be invoked, in the last epoch of deadlines. let vest_begin = current_epoch + spec.initial_delay; // Nothing unlocks here, this is just the start of the clock. let mut vested_so_far = TokenAmount::zero(); @@ -81,56 +141,80 @@ impl VestingFunds { Some(VestingFund { epoch: vest_epoch, amount: vest_this_time }) }); - // Take the old funds array and replace it with a new one. - let funds_len = self.funds.len(); - let old_funds = mem::replace(&mut self.funds, Vec::with_capacity(funds_len)); + let old_funds = self.load(store)?; // Fill back in the funds array, merging existing and new schedule. - self.funds.extend( - old_funds.into_iter().merge_join_by(new_funds, |a, b| a.epoch.cmp(&b.epoch)).map( - |item| match item { - EitherOrBoth::Left(a) => a, - EitherOrBoth::Right(b) => b, - EitherOrBoth::Both(a, b) => { - VestingFund { epoch: a.epoch, amount: a.amount + b.amount } - } - }, - ), - ); + let mut new_funds = old_funds + .into_iter() + .merge_join_by(new_funds, |a, b| a.epoch.cmp(&b.epoch)) + .map(|item| match item { + EitherOrBoth::Left(a) => a, + EitherOrBoth::Right(b) => b, + EitherOrBoth::Both(a, b) => { + VestingFund { epoch: a.epoch, amount: a.amount + b.amount } + } + }) + .peekable(); + + // Take any unlocked funds. + let unlocked = new_funds + .peeking_take_while(|fund| fund.epoch < current_epoch) + .map(|f| f.amount) + .sum::(); + + // Write back the new value. + self.save(store, new_funds.collect::>())?; + + Ok(unlocked) } - pub fn unlock_unvested_funds( + /// Unlock all vested (first return value) then unlock unvested funds up to at most the + /// specified target. + pub fn unlock_vested_and_unvested_funds( &mut self, + store: &impl Blockstore, current_epoch: ChainEpoch, target: &TokenAmount, - ) -> TokenAmount { - let mut amount_unlocked = TokenAmount::zero(); - let mut last = None; - let mut start = 0; - for (i, vf) in self.funds.iter_mut().enumerate() { - if &amount_unlocked >= target { - break; - } + ) -> Result<(TokenAmount, TokenAmount), ActorError> { + let mut target = target.clone(); + // Fast path: take it out of the head and don't touch the tail. + let Some(head) = &mut self.head else { + // If head is none, we have nothing to unlock. + return Ok(Default::default()); + }; + if head.epoch >= current_epoch && head.amount >= target { + head.amount -= ⌖ + return Ok((TokenAmount::zero(), target)); + } - if vf.epoch >= current_epoch { - let unlock_amount = std::cmp::min(target - &amount_unlocked, vf.amount.clone()); - amount_unlocked += &unlock_amount; - let new_amount = &vf.amount - &unlock_amount; + // Slow path, take it out of the tail. - if new_amount.is_zero() { - last = Some(i); - } else { - vf.amount = new_amount; - } - } else { - start = i + 1; + let mut unvested = TokenAmount::zero(); + let mut vested = TokenAmount::zero(); + + let mut funds = itertools::put_back(self.load(store)?); + while let Some(mut vf) = funds.next() { + // already vested + if vf.epoch < current_epoch { + vested += vf.amount; + continue; + } + + // take all + if vf.amount < target { + target -= &vf.amount; + unvested += &vf.amount; + continue; } - } - if let Some(end) = last { - self.funds.drain(start..=end); + // take some and stop. + unvested += ⌖ + vf.amount -= ⌖ + funds.put_back(vf); + break; } + self.save(store, funds)?; - amount_unlocked + Ok((vested, unvested)) } } diff --git a/actors/miner/tests/apply_rewards.rs b/actors/miner/tests/apply_rewards.rs index 55bfe2515..a69463595 100644 --- a/actors/miner/tests/apply_rewards.rs +++ b/actors/miner/tests/apply_rewards.rs @@ -45,25 +45,25 @@ fn funds_vest() { h.construct_and_verify(&rt); let st = h.get_state(&rt); - let vesting_funds = st.load_vesting_funds(&rt.store).unwrap(); + let vesting_funds = st.vesting_funds.load(&rt.store).unwrap(); // Nothing vesting to start - assert!(vesting_funds.funds.is_empty()); + assert_eq!(0, vesting_funds.count()); assert!(st.locked_funds.is_zero()); // Lock some funds with AddLockedFund let amt = TokenAmount::from_atto(600_000); h.apply_rewards(&rt, amt.clone(), TokenAmount::zero()); let st = h.get_state(&rt); - let vesting_funds = st.load_vesting_funds(&rt.store).unwrap(); + let vesting_funds: Vec<_> = st.vesting_funds.load(&rt.store).unwrap().collect(); - assert_eq!(180, vesting_funds.funds.len()); + assert_eq!(180, vesting_funds.len()); // Vested FIL pays out on epochs with expected offset let quant_spec = QuantSpec { unit: REWARD_VESTING_SPEC.quantization, offset: PERIOD_OFFSET }; let curr_epoch = *rt.epoch.borrow(); - for (i, vf) in vesting_funds.funds.iter().enumerate() { + for (i, vf) in vesting_funds.iter().enumerate() { let step = REWARD_VESTING_SPEC.initial_delay + (i as i64 + 1) * REWARD_VESTING_SPEC.step_duration; let expected_epoch = quant_spec.quantize_up(curr_epoch + step); @@ -71,7 +71,7 @@ fn funds_vest() { } let expected_offset = PERIOD_OFFSET % REWARD_VESTING_SPEC.quantization; - for vf in vesting_funds.funds.iter() { + for vf in vesting_funds.iter() { assert_eq!(expected_offset, vf.epoch % REWARD_VESTING_SPEC.quantization); } diff --git a/actors/miner/tests/deadline_cron.rs b/actors/miner/tests/deadline_cron.rs index 945bd3f36..23b3db38c 100644 --- a/actors/miner/tests/deadline_cron.rs +++ b/actors/miner/tests/deadline_cron.rs @@ -69,8 +69,8 @@ fn test_vesting_on_cron() { // --- ASSERT FUNDS TO BE VESTED let st = h.get_state(&rt); - let vesting_funds = st.load_vesting_funds(&rt.store).unwrap(); - assert_eq!(360, vesting_funds.funds.len()); + let vesting_funds = st.vesting_funds.load(&rt.store).unwrap(); + assert_eq!(360, vesting_funds.count()); let q = QuantSpec { unit: REWARD_VESTING_SPEC.quantization, offset: st.proving_period_start }; diff --git a/actors/miner/tests/repay_debt_test.rs b/actors/miner/tests/repay_debt_test.rs index a1b60c791..31a0b6238 100644 --- a/actors/miner/tests/repay_debt_test.rs +++ b/actors/miner/tests/repay_debt_test.rs @@ -16,8 +16,8 @@ fn repay_debt_in_priority_order() { let (penalty_from_vesting, penalty_from_balance) = h.st.repay_partial_debt_in_priority_order(&h.store, 0, ¤t_balance).unwrap(); - assert_eq!(penalty_from_vesting, TokenAmount::zero()); - assert_eq!(penalty_from_balance, current_balance); + assert_eq!(penalty_from_vesting, current_balance); + assert_eq!(penalty_from_balance, TokenAmount::zero()); let expected_debt = -(current_balance - fee); assert_eq!(expected_debt, h.st.fee_debt); diff --git a/actors/miner/tests/state_harness.rs b/actors/miner/tests/state_harness.rs index dc9cb1f05..3bb8fba44 100644 --- a/actors/miner/tests/state_harness.rs +++ b/actors/miner/tests/state_harness.rs @@ -1,7 +1,7 @@ #![allow(dead_code)] use fil_actor_miner::{ BitFieldQueue, CollisionPolicy, MinerInfo, QuantSpec, SectorOnChainInfo, - SectorPreCommitOnChainInfo, State, VestSpec, VestingFunds, + SectorPreCommitOnChainInfo, State, VestSpec, }; use fil_actors_runtime::test_blockstores::MemoryBlockstore; use fil_actors_runtime::{runtime::Policy, ActorError}; @@ -124,7 +124,7 @@ impl StateHarness { &mut self, current_epoch: ChainEpoch, target: &TokenAmount, - ) -> anyhow::Result { + ) -> anyhow::Result<(TokenAmount, TokenAmount)> { self.st.unlock_unvested_funds(&self.store, current_epoch, target) } @@ -151,8 +151,7 @@ impl StateHarness { #[allow(dead_code)] pub fn vesting_funds_store_empty(&self) -> bool { - let vesting = self.store.get_cbor::(&self.st.vesting_funds).unwrap().unwrap(); - vesting.funds.is_empty() + self.st.vesting_funds.load(&self.store).unwrap().next().is_none() } pub fn assign_sectors_to_deadlines( diff --git a/actors/miner/tests/util.rs b/actors/miner/tests/util.rs index 36154067f..d79ee706e 100644 --- a/actors/miner/tests/util.rs +++ b/actors/miner/tests/util.rs @@ -15,7 +15,7 @@ use fvm_ipld_blockstore::Blockstore; use fvm_ipld_encoding::de::Deserialize; use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_ipld_encoding::ser::Serialize; -use fvm_ipld_encoding::{BytesDe, CborStore, RawBytes}; +use fvm_ipld_encoding::{BytesDe, RawBytes}; use fvm_shared::address::Address; use fvm_shared::bigint::BigInt; use fvm_shared::bigint::Zero; @@ -76,8 +76,8 @@ use fil_actor_miner::{ SectorActivationManifest, SectorChanges, SectorContentChangedParams, SectorContentChangedReturn, SectorOnChainInfo, SectorPreCommitInfo, SectorPreCommitOnChainInfo, SectorReturn, SectorUpdateManifest, Sectors, State, SubmitWindowedPoStParams, - TerminateSectorsParams, TerminationDeclaration, VerifiedAllocationKey, VestingFunds, - WindowedPoSt, WithdrawBalanceParams, WithdrawBalanceReturn, CRON_EVENT_PROVING_DEADLINE, + TerminateSectorsParams, TerminationDeclaration, VerifiedAllocationKey, WindowedPoSt, + WithdrawBalanceParams, WithdrawBalanceReturn, CRON_EVENT_PROVING_DEADLINE, NI_AGGREGATE_FEE_BASE_SECTOR_COUNT, NO_QUANTIZATION, SECTORS_AMT_BITWIDTH, SECTOR_CONTENT_CHANGED, }; @@ -3307,16 +3307,13 @@ enum MhCode { fn immediately_vesting_funds(rt: &MockRuntime, state: &State) -> TokenAmount { let curr_epoch = *rt.epoch.borrow(); - let vesting = rt.store.get_cbor::(&state.vesting_funds).unwrap().unwrap(); - let mut sum = TokenAmount::zero(); - for vf in vesting.funds { - if vf.epoch < curr_epoch { - sum += vf.amount; - } else { - break; - } - } - sum + state + .vesting_funds + .load(&rt.store) + .unwrap() + .take_while(|vf| vf.epoch < curr_epoch) + .map(|vf| vf.amount) + .sum() } pub fn make_post_proofs(proof_type: RegisteredPoStProof) -> Vec { diff --git a/actors/miner/tests/vesting_unvested_funds.rs b/actors/miner/tests/vesting_unvested_funds.rs index 6cc1cae64..071028d11 100644 --- a/actors/miner/tests/vesting_unvested_funds.rs +++ b/actors/miner/tests/vesting_unvested_funds.rs @@ -1,6 +1,4 @@ use fil_actor_miner::VestSpec; -use fil_actor_miner::VestingFunds; -use fvm_ipld_encoding::CborStore; use fvm_shared::bigint::Zero; use fvm_shared::econ::TokenAmount; @@ -17,7 +15,8 @@ fn unlock_unvested_funds_leaving_bucket_with_non_zero_tokens() { h.add_locked_funds(vest_start, &vest_sum, &vspec).unwrap(); - let amount_unlocked = h.unlock_unvested_funds(vest_start, &TokenAmount::from_atto(39)).unwrap(); + let amount_unlocked = + h.unlock_unvested_funds(vest_start, &TokenAmount::from_atto(39)).unwrap().0; assert_eq!(TokenAmount::from_atto(39), amount_unlocked); // no vested funds available to unlock until strictly after first vesting epoch @@ -49,7 +48,8 @@ fn unlock_unvested_funds_leaving_bucket_with_zero_tokens() { h.add_locked_funds(vest_start, &vest_sum, &vspec).unwrap(); - let amount_unlocked = h.unlock_unvested_funds(vest_start, &TokenAmount::from_atto(40)).unwrap(); + let amount_unlocked = + h.unlock_unvested_funds(vest_start, &TokenAmount::from_atto(40)).unwrap().0; assert_eq!(TokenAmount::from_atto(40), amount_unlocked); assert_eq!(TokenAmount::zero(), h.unlock_vested_funds(vest_start).unwrap()); @@ -78,7 +78,7 @@ fn unlock_all_unvested_funds() { let vest_sum = TokenAmount::from_atto(100); h.add_locked_funds(vest_start, &vest_sum, &vspec).unwrap(); - let unvested_funds = h.unlock_unvested_funds(vest_start, &vest_sum).unwrap(); + let unvested_funds = h.unlock_unvested_funds(vest_start, &vest_sum).unwrap().0; assert_eq!(vest_sum, unvested_funds); assert!(h.st.locked_funds.is_zero()); @@ -93,7 +93,8 @@ fn unlock_unvested_funds_value_greater_than_locked_funds() { let vest_start = 10; let vest_sum = TokenAmount::from_atto(100); h.add_locked_funds(vest_start, &vest_sum, &vspec).unwrap(); - let unvested_funds = h.unlock_unvested_funds(vest_start, &TokenAmount::from_atto(200)).unwrap(); + let unvested_funds = + h.unlock_unvested_funds(vest_start, &TokenAmount::from_atto(200)).unwrap().0; assert_eq!(vest_sum, unvested_funds); assert!(h.st.locked_funds.is_zero()); @@ -109,25 +110,23 @@ fn unlock_unvested_funds_when_there_are_vested_funds_in_the_table() { let vest_sum = TokenAmount::from_atto(100); // will lock funds from epochs 11 to 60 - h.add_locked_funds(vest_start, &vest_sum, &vspec).unwrap(); + let vested = h.add_locked_funds(vest_start, &vest_sum, &vspec).unwrap(); + assert_eq!(TokenAmount::zero(), vested); - // unlock funds from epochs 30 to 60 + // unlock funds from epochs 30 through 59 let new_epoch = 30; let target = TokenAmount::from_atto(60); - let remaining = &vest_sum - ⌖ - let unvested_funds = h.unlock_unvested_funds(new_epoch, &target).unwrap(); + let remaining = TokenAmount::from_atto(2); + let (unvested_funds, total_unlocked) = h.unlock_unvested_funds(new_epoch, &target).unwrap(); assert_eq!(target, unvested_funds); + assert_eq!(vest_sum - &remaining, total_unlocked); + // we expect 2 left over, locked for epoch 60 assert_eq!(remaining, h.st.locked_funds); - // vesting funds should have all epochs from 11 to 29 - let vesting = h.store.get_cbor::(&h.st.vesting_funds).unwrap().unwrap(); - let mut epoch = 11; - for vf in vesting.funds { - assert_eq!(epoch, vf.epoch); - epoch += 1; - if epoch == 30 { - break; - } - } + // vesting funds should have epoch 60 and only epoch 60 + assert_eq!( + &[60][..], + &h.st.vesting_funds.load(&h.store).unwrap().map(|vf| vf.epoch).collect::>() + ); } From 94019dbaa2c23919a1ef28869056d4a65e57f72e Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Sun, 23 Feb 2025 10:08:30 -0700 Subject: [PATCH 3/8] review: rename unlock_unvested_funds to account for unlocking vested funds --- actors/miner/src/state.rs | 4 ++-- actors/miner/tests/state_harness.rs | 4 ++-- actors/miner/tests/vesting_unvested_funds.rs | 11 ++++++----- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/actors/miner/src/state.rs b/actors/miner/src/state.rs index f62359abc..7123febe5 100644 --- a/actors/miner/src/state.rs +++ b/actors/miner/src/state.rs @@ -883,7 +883,7 @@ impl State { > { let fee_debt = self.fee_debt.clone(); let (from_vesting, total_unlocked) = - self.unlock_unvested_funds(store, current_epoch, &fee_debt)?; + self.unlock_vested_and_unvested_funds(store, current_epoch, &fee_debt)?; if from_vesting > self.fee_debt { return Err(anyhow!("should never unlock more than the debt we need to repay")); @@ -919,7 +919,7 @@ impl State { /// possible. The soonest-vesting entries are unlocked first. /// /// Returns the amount of unvested funds unlocked, along with the total amount of funds unlocked. - pub fn unlock_unvested_funds( + pub fn unlock_vested_and_unvested_funds( &mut self, store: &BS, current_epoch: ChainEpoch, diff --git a/actors/miner/tests/state_harness.rs b/actors/miner/tests/state_harness.rs index 3bb8fba44..e1ff89e68 100644 --- a/actors/miner/tests/state_harness.rs +++ b/actors/miner/tests/state_harness.rs @@ -120,12 +120,12 @@ impl StateHarness { } #[allow(dead_code)] - pub fn unlock_unvested_funds( + pub fn unlock_vested_and_unvested_funds( &mut self, current_epoch: ChainEpoch, target: &TokenAmount, ) -> anyhow::Result<(TokenAmount, TokenAmount)> { - self.st.unlock_unvested_funds(&self.store, current_epoch, target) + self.st.unlock_vested_and_unvested_funds(&self.store, current_epoch, target) } pub fn has_sector_number(&self, sector_no: SectorNumber) -> bool { diff --git a/actors/miner/tests/vesting_unvested_funds.rs b/actors/miner/tests/vesting_unvested_funds.rs index 071028d11..b19fb27df 100644 --- a/actors/miner/tests/vesting_unvested_funds.rs +++ b/actors/miner/tests/vesting_unvested_funds.rs @@ -16,7 +16,7 @@ fn unlock_unvested_funds_leaving_bucket_with_non_zero_tokens() { h.add_locked_funds(vest_start, &vest_sum, &vspec).unwrap(); let amount_unlocked = - h.unlock_unvested_funds(vest_start, &TokenAmount::from_atto(39)).unwrap().0; + h.unlock_vested_and_unvested_funds(vest_start, &TokenAmount::from_atto(39)).unwrap().0; assert_eq!(TokenAmount::from_atto(39), amount_unlocked); // no vested funds available to unlock until strictly after first vesting epoch @@ -49,7 +49,7 @@ fn unlock_unvested_funds_leaving_bucket_with_zero_tokens() { h.add_locked_funds(vest_start, &vest_sum, &vspec).unwrap(); let amount_unlocked = - h.unlock_unvested_funds(vest_start, &TokenAmount::from_atto(40)).unwrap().0; + h.unlock_vested_and_unvested_funds(vest_start, &TokenAmount::from_atto(40)).unwrap().0; assert_eq!(TokenAmount::from_atto(40), amount_unlocked); assert_eq!(TokenAmount::zero(), h.unlock_vested_funds(vest_start).unwrap()); @@ -78,7 +78,7 @@ fn unlock_all_unvested_funds() { let vest_sum = TokenAmount::from_atto(100); h.add_locked_funds(vest_start, &vest_sum, &vspec).unwrap(); - let unvested_funds = h.unlock_unvested_funds(vest_start, &vest_sum).unwrap().0; + let unvested_funds = h.unlock_vested_and_unvested_funds(vest_start, &vest_sum).unwrap().0; assert_eq!(vest_sum, unvested_funds); assert!(h.st.locked_funds.is_zero()); @@ -94,7 +94,7 @@ fn unlock_unvested_funds_value_greater_than_locked_funds() { let vest_sum = TokenAmount::from_atto(100); h.add_locked_funds(vest_start, &vest_sum, &vspec).unwrap(); let unvested_funds = - h.unlock_unvested_funds(vest_start, &TokenAmount::from_atto(200)).unwrap().0; + h.unlock_vested_and_unvested_funds(vest_start, &TokenAmount::from_atto(200)).unwrap().0; assert_eq!(vest_sum, unvested_funds); assert!(h.st.locked_funds.is_zero()); @@ -117,7 +117,8 @@ fn unlock_unvested_funds_when_there_are_vested_funds_in_the_table() { let new_epoch = 30; let target = TokenAmount::from_atto(60); let remaining = TokenAmount::from_atto(2); - let (unvested_funds, total_unlocked) = h.unlock_unvested_funds(new_epoch, &target).unwrap(); + let (unvested_funds, total_unlocked) = + h.unlock_vested_and_unvested_funds(new_epoch, &target).unwrap(); assert_eq!(target, unvested_funds); assert_eq!(vest_sum - &remaining, total_unlocked); From e2742fa80c468a6ae9171748c3e39ae4829110ba Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Sun, 23 Feb 2025 11:19:08 -0700 Subject: [PATCH 4/8] refactor: small simplifications --- actors/miner/src/vesting_state.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/actors/miner/src/vesting_state.rs b/actors/miner/src/vesting_state.rs index 31bf6f9bd..064a20132 100644 --- a/actors/miner/src/vesting_state.rs +++ b/actors/miner/src/vesting_state.rs @@ -92,10 +92,8 @@ impl VestingFunds { return Ok(TokenAmount::zero()); } let mut funds = self.load(store)?.peekable(); - let unlocked = funds - .peeking_take_while(|fund| fund.epoch < current_epoch) - .map(|f| f.amount) - .sum::(); + let unlocked = + funds.peeking_take_while(|fund| fund.epoch < current_epoch).map(|f| f.amount).sum(); self.save(store, funds)?; Ok(unlocked) } @@ -157,13 +155,11 @@ impl VestingFunds { .peekable(); // Take any unlocked funds. - let unlocked = new_funds - .peeking_take_while(|fund| fund.epoch < current_epoch) - .map(|f| f.amount) - .sum::(); + let unlocked = + new_funds.peeking_take_while(|fund| fund.epoch < current_epoch).map(|f| f.amount).sum(); // Write back the new value. - self.save(store, new_funds.collect::>())?; + self.save(store, new_funds)?; Ok(unlocked) } From 18f24d2d4ca25567916dfcce56ed7d4a909b91a3 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Mon, 24 Feb 2025 11:18:47 -0700 Subject: [PATCH 5/8] review: make VestingFunds nullable instead of making head nullable This will save space when we have no vesting funds and, tbh, simplifies the code a bit. Also note: - I changed the return value of `load` to return a vector. I can make it work with iterators, but it's very... "rusty" (unreadable). - I made `can_vest` private. --- actors/miner/src/lib.rs | 8 +- actors/miner/src/state.rs | 4 +- actors/miner/src/vesting_state.rs | 80 ++++++++++---------- actors/miner/tests/apply_rewards.rs | 4 +- actors/miner/tests/deadline_cron.rs | 2 +- actors/miner/tests/state_harness.rs | 2 +- actors/miner/tests/util.rs | 1 + actors/miner/tests/vesting_unvested_funds.rs | 8 +- 8 files changed, 60 insertions(+), 49 deletions(-) diff --git a/actors/miner/src/lib.rs b/actors/miner/src/lib.rs index 7199e68bc..76159c4de 100644 --- a/actors/miner/src/lib.rs +++ b/actors/miner/src/lib.rs @@ -308,8 +308,12 @@ impl Actor { fn get_vesting_funds(rt: &impl Runtime) -> Result { rt.validate_immediate_caller_accept_any()?; let state: State = rt.state()?; - let vesting_funds = - state.vesting_funds.load(rt.store())?.map(|v| (v.epoch, v.amount)).collect_vec(); + let vesting_funds = state + .vesting_funds + .load(rt.store())? + .into_iter() + .map(|v| (v.epoch, v.amount)) + .collect_vec(); Ok(GetVestingFundsReturn { vesting_funds }) } diff --git a/actors/miner/src/state.rs b/actors/miner/src/state.rs index 7123febe5..41d6cd569 100644 --- a/actors/miner/src/state.rs +++ b/actors/miner/src/state.rs @@ -164,15 +164,13 @@ impl State { e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to construct illegal state") })?; - let vesting_funds = VestingFunds::new(store)?; - Ok(Self { info: info_cid, pre_commit_deposits: TokenAmount::default(), locked_funds: TokenAmount::default(), - vesting_funds, + vesting_funds: VestingFunds::new(), initial_pledge: TokenAmount::default(), fee_debt: TokenAmount::default(), diff --git a/actors/miner/src/vesting_state.rs b/actors/miner/src/vesting_state.rs index 064a20132..c2d738622 100644 --- a/actors/miner/src/vesting_state.rs +++ b/actors/miner/src/vesting_state.rs @@ -6,6 +6,7 @@ use std::iter; use cid::Cid; use fil_actors_runtime::{ActorError, AsActorError}; use fvm_ipld_blockstore::Blockstore; +use fvm_ipld_encoding::serde::{Deserialize, Serialize}; use fvm_ipld_encoding::tuple::*; use fvm_ipld_encoding::CborStore; use fvm_shared::clock::ChainEpoch; @@ -26,39 +27,37 @@ pub struct VestingFund { /// Represents the vesting table state for the miner. It's composed of a `head` (stored inline) and /// a tail (referenced by CID). +#[derive(Serialize, Deserialize, Debug, Clone, Default)] +#[serde(transparent)] +pub struct VestingFunds(Option); + #[derive(Serialize_tuple, Deserialize_tuple, Debug, Clone)] -pub struct VestingFunds { - // The "next" batch of vesting funds: - // - If this is None, there are no vesting funds. - // - All batches in `tail` are guaranteed to vest after this batch. - // - This batch _can_ be empty (have a zero amount) if we've burnt through it (fees & - // penalties). - head: Option, +struct VestingFundsInner { + // The "next" batch of vesting funds. + head: VestingFund, // The rest of the vesting funds, if any. tail: Cid, // Vec } impl VestingFunds { - pub fn new(store: &impl Blockstore) -> Result { - let tail = store - .put_cbor(&Vec::::new(), Code::Blake2b256) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to construct vesting funds")?; - Ok(Self { head: None, tail }) + pub fn new() -> Self { + Self(None) } - pub fn load( - &self, - store: &impl Blockstore, - ) -> Result, ActorError> { - // NOTE: we allow head to be drawn down to zero through fees/penalties. However, when - // inspecting the vesting table, we never want to see a "zero" entry so we skip it in that case. - // We don't set it to "none" in that case because "none" means that we have _no_ vesting funds. - let head = self.head.as_ref().filter(|h| h.amount.is_positive()).cloned(); - let tail: Vec<_> = store - .get_cbor(&self.tail) + pub fn load(&self, store: &impl Blockstore) -> Result, ActorError> { + let Some(this) = &self.0 else { return Ok(Vec::new()) }; + let mut funds: Vec<_> = store + .get_cbor(&this.tail) .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to load the vesting funds")? .context_code(ExitCode::USR_ILLEGAL_STATE, "missing vesting funds state")?; - Ok(itertools::chain(head, tail)) + + // NOTE: we allow head to be drawn down to zero through fees/penalties. However, when + // inspecting the vesting table, we never want to see a "zero" entry so we skip it in + // that case. + if this.head.amount.is_positive() { + funds.insert(0, this.head.clone()); + } + Ok(funds) } fn save( @@ -67,20 +66,21 @@ impl VestingFunds { funds: impl IntoIterator, ) -> Result<(), ActorError> { let mut funds = funds.into_iter(); - let head = funds.next(); - let tail = funds.collect_vec(); + let Some(head) = funds.next() else { + self.0 = None; + return Ok(()); + }; - self.tail = store - .put_cbor(&tail, Code::Blake2b256) + let tail = store + .put_cbor(&funds.collect_vec(), Code::Blake2b256) .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to store the vesting funds")?; - // We do this second just in case the first operation fails to try to maintain consistent - // state. - self.head = head; + + self.0 = Some(VestingFundsInner { head, tail }); Ok(()) } - pub fn can_vest(&self, current_epoch: ChainEpoch) -> bool { - matches!(&self.head, Some(VestingFund { epoch, .. }) if *epoch < current_epoch) + fn can_vest(&self, current_epoch: ChainEpoch) -> bool { + self.0.as_ref().map(|v| v.head.epoch < current_epoch).unwrap_or(false) } pub fn unlock_vested_funds( @@ -91,7 +91,7 @@ impl VestingFunds { if !self.can_vest(current_epoch) { return Ok(TokenAmount::zero()); } - let mut funds = self.load(store)?.peekable(); + let mut funds = self.load(store)?.into_iter().peekable(); let unlocked = funds.peeking_take_while(|fund| fund.epoch < current_epoch).map(|f| f.amount).sum(); self.save(store, funds)?; @@ -172,14 +172,16 @@ impl VestingFunds { current_epoch: ChainEpoch, target: &TokenAmount, ) -> Result<(TokenAmount, TokenAmount), ActorError> { - let mut target = target.clone(); - // Fast path: take it out of the head and don't touch the tail. - let Some(head) = &mut self.head else { - // If head is none, we have nothing to unlock. + // If our inner value is None, there's nothing to vest. + let Some(this) = &mut self.0 else { return Ok(Default::default()); }; - if head.epoch >= current_epoch && head.amount >= target { - head.amount -= ⌖ + + let mut target = target.clone(); + + // Fast path: take it out of the head and don't touch the tail. + if this.head.epoch >= current_epoch && this.head.amount >= target { + this.head.amount -= ⌖ return Ok((TokenAmount::zero(), target)); } diff --git a/actors/miner/tests/apply_rewards.rs b/actors/miner/tests/apply_rewards.rs index a69463595..389d5fdcc 100644 --- a/actors/miner/tests/apply_rewards.rs +++ b/actors/miner/tests/apply_rewards.rs @@ -48,14 +48,14 @@ fn funds_vest() { let vesting_funds = st.vesting_funds.load(&rt.store).unwrap(); // Nothing vesting to start - assert_eq!(0, vesting_funds.count()); + assert_eq!(0, vesting_funds.len()); assert!(st.locked_funds.is_zero()); // Lock some funds with AddLockedFund let amt = TokenAmount::from_atto(600_000); h.apply_rewards(&rt, amt.clone(), TokenAmount::zero()); let st = h.get_state(&rt); - let vesting_funds: Vec<_> = st.vesting_funds.load(&rt.store).unwrap().collect(); + let vesting_funds = st.vesting_funds.load(&rt.store).unwrap(); assert_eq!(180, vesting_funds.len()); diff --git a/actors/miner/tests/deadline_cron.rs b/actors/miner/tests/deadline_cron.rs index 23b3db38c..4a8d6e3a9 100644 --- a/actors/miner/tests/deadline_cron.rs +++ b/actors/miner/tests/deadline_cron.rs @@ -70,7 +70,7 @@ fn test_vesting_on_cron() { // --- ASSERT FUNDS TO BE VESTED let st = h.get_state(&rt); let vesting_funds = st.vesting_funds.load(&rt.store).unwrap(); - assert_eq!(360, vesting_funds.count()); + assert_eq!(360, vesting_funds.len()); let q = QuantSpec { unit: REWARD_VESTING_SPEC.quantization, offset: st.proving_period_start }; diff --git a/actors/miner/tests/state_harness.rs b/actors/miner/tests/state_harness.rs index e1ff89e68..4a964bdcd 100644 --- a/actors/miner/tests/state_harness.rs +++ b/actors/miner/tests/state_harness.rs @@ -151,7 +151,7 @@ impl StateHarness { #[allow(dead_code)] pub fn vesting_funds_store_empty(&self) -> bool { - self.st.vesting_funds.load(&self.store).unwrap().next().is_none() + self.st.vesting_funds.load(&self.store).unwrap().is_empty() } pub fn assign_sectors_to_deadlines( diff --git a/actors/miner/tests/util.rs b/actors/miner/tests/util.rs index d79ee706e..07e62666a 100644 --- a/actors/miner/tests/util.rs +++ b/actors/miner/tests/util.rs @@ -3311,6 +3311,7 @@ fn immediately_vesting_funds(rt: &MockRuntime, state: &State) -> TokenAmount { .vesting_funds .load(&rt.store) .unwrap() + .into_iter() .take_while(|vf| vf.epoch < curr_epoch) .map(|vf| vf.amount) .sum() diff --git a/actors/miner/tests/vesting_unvested_funds.rs b/actors/miner/tests/vesting_unvested_funds.rs index b19fb27df..c59a7ed10 100644 --- a/actors/miner/tests/vesting_unvested_funds.rs +++ b/actors/miner/tests/vesting_unvested_funds.rs @@ -128,6 +128,12 @@ fn unlock_unvested_funds_when_there_are_vested_funds_in_the_table() { // vesting funds should have epoch 60 and only epoch 60 assert_eq!( &[60][..], - &h.st.vesting_funds.load(&h.store).unwrap().map(|vf| vf.epoch).collect::>() + &h.st + .vesting_funds + .load(&h.store) + .unwrap() + .into_iter() + .map(|vf| vf.epoch) + .collect::>(), ); } From 2d66a4b4bbad10b4e9c65ba73c7bf53c47f99e37 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Mon, 24 Feb 2025 14:20:47 -0700 Subject: [PATCH 6/8] review: break take_vested out into a new function --- actors/miner/src/vesting_state.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/actors/miner/src/vesting_state.rs b/actors/miner/src/vesting_state.rs index c2d738622..8de8f785c 100644 --- a/actors/miner/src/vesting_state.rs +++ b/actors/miner/src/vesting_state.rs @@ -12,7 +12,7 @@ use fvm_ipld_encoding::CborStore; use fvm_shared::clock::ChainEpoch; use fvm_shared::econ::TokenAmount; use fvm_shared::error::ExitCode; -use itertools::{EitherOrBoth, Itertools}; +use itertools::{EitherOrBoth, Itertools, PeekingNext}; use multihash_codetable::Code; use num_traits::Zero; @@ -39,6 +39,15 @@ struct VestingFundsInner { tail: Cid, // Vec } +/// Take vested funds from the passed iterator. This assumes the iterator returns `VestingFund`s in +/// epoch order. +fn take_vested( + iter: &mut impl PeekingNext, + current_epoch: ChainEpoch, +) -> TokenAmount { + iter.peeking_take_while(|fund| fund.epoch < current_epoch).map(|f| f.amount).sum() +} + impl VestingFunds { pub fn new() -> Self { Self(None) @@ -92,8 +101,7 @@ impl VestingFunds { return Ok(TokenAmount::zero()); } let mut funds = self.load(store)?.into_iter().peekable(); - let unlocked = - funds.peeking_take_while(|fund| fund.epoch < current_epoch).map(|f| f.amount).sum(); + let unlocked = take_vested(&mut funds, current_epoch); self.save(store, funds)?; Ok(unlocked) } @@ -155,8 +163,7 @@ impl VestingFunds { .peekable(); // Take any unlocked funds. - let unlocked = - new_funds.peeking_take_while(|fund| fund.epoch < current_epoch).map(|f| f.amount).sum(); + let unlocked = take_vested(&mut new_funds, current_epoch); // Write back the new value. self.save(store, new_funds)?; From dfd6787d6e67ef62ba7547c428a875ab06e230e4 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Mon, 24 Feb 2025 14:36:36 -0700 Subject: [PATCH 7/8] test: test both total and vested unlocked --- actors/miner/tests/vesting_unvested_funds.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/actors/miner/tests/vesting_unvested_funds.rs b/actors/miner/tests/vesting_unvested_funds.rs index c59a7ed10..81ccb0a0a 100644 --- a/actors/miner/tests/vesting_unvested_funds.rs +++ b/actors/miner/tests/vesting_unvested_funds.rs @@ -15,9 +15,10 @@ fn unlock_unvested_funds_leaving_bucket_with_non_zero_tokens() { h.add_locked_funds(vest_start, &vest_sum, &vspec).unwrap(); - let amount_unlocked = - h.unlock_vested_and_unvested_funds(vest_start, &TokenAmount::from_atto(39)).unwrap().0; - assert_eq!(TokenAmount::from_atto(39), amount_unlocked); + let (vested_unlocked, total_unlocked) = + h.unlock_vested_and_unvested_funds(vest_start, &TokenAmount::from_atto(39)).unwrap(); + assert_eq!(TokenAmount::from_atto(39), vested_unlocked); + assert_eq!(TokenAmount::from_atto(39), total_unlocked); // no vested funds available to unlock until strictly after first vesting epoch assert_eq!(TokenAmount::zero(), h.unlock_vested_funds(vest_start).unwrap()); From c1f75c1e0c08854a81ad596f97d1201250be3f7f Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Tue, 25 Feb 2025 16:46:19 +0000 Subject: [PATCH 8/8] review: comment on unlocking Co-authored-by: Rod Vagg --- actors/miner/src/state.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/actors/miner/src/state.rs b/actors/miner/src/state.rs index 41d6cd569..d3220675a 100644 --- a/actors/miner/src/state.rs +++ b/actors/miner/src/state.rs @@ -887,6 +887,8 @@ impl State { return Err(anyhow!("should never unlock more than the debt we need to repay")); } + // locked unvested funds should now have been moved to unlocked balance if + // there was enough to cover the fee debt let unlocked_balance = self.get_unlocked_balance(curr_balance)?; let to_burn = cmp::min(&unlocked_balance, &self.fee_debt).clone(); self.fee_debt -= &to_burn;