feat: refactor vesting funds into a head and a tail #1636

Stebalien · 2025-02-21T04:49:55Z

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

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.

Stebalien · 2025-02-21T04:50:18Z

Built on #1635.

Stebalien · 2025-02-21T04:52:52Z

actors/miner/src/lib.rs

@@ -4464,25 +4460,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.


Moved to the end, see the comment.

What is the benefit of moving this below ?

actors/miner/src/state.rs

actors/miner/src/vesting_state.rs

Stebalien · 2025-02-21T05:15:47Z

This is significantly more than just "fixing #1594" so it probably requires a FIP update to FIP-0100. But, IMO, we should strongly consider this because without it, we'll be re-writing the vesting queue every deadline. I guess that isn't terrible in the grand scheme of things but.... it's not great.

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/vesting_state.rs

ZenGround0 · 2025-02-21T20:26:57Z

actors/miner/src/vesting_state.rs

-    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(


Please note that changing the interface at the same time as the implementation like this is not the most friendly for review. I get that you are in cleanup mode and this is how it is. Decoupled changes at commit level would probably halve my "wtf are we doing again" moments.

There's not much I can do about that here, I'm changing the behavior from:

Unlock unvested only, leaving unvested untouched.

Unlock vested and unvested.

This didn't matter much before because the VestingFunds had all the state locally (so we could just unlock vested, then unlock unvested). But it matters now because I need to read the "tail" which may be behind a CID, so we get better performance by doing it in one go.

I could have done this in two commits, but then I would have had to have implemented it the other way first.

But it matters now because I need to read the "tail" which may be behind a CID, so we get better performance by doing it in one go.

Yeah I'm questioning whether that performance is worth it

but then I would have had to have implemented it the other way first.

That's a good point

actors/miner/src/state.rs

ZenGround0 · 2025-02-21T20:29:22Z

actors/miner/src/vesting_state.rs

+        Ok(unlocked)
+    }
+
+    // Adds locked funds and unlocks everything that has already vested.


Noting this second part looks like a behavior change. I can't see any problems with the new additional behavior as long as all the callers are doing the necessary book keeping with LockedFunds field

Ok looks like this is just pushing down this coupled behavior to this level. Since only caller of this appears to be doing the same thing its not a real behavior change at all.

ZenGround0 · 2025-02-21T20:36:47Z

actors/miner/src/lib.rs

@@ -4464,25 +4460,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.


What is the benefit of moving this below ?

actors/miner/src/lib.rs

actors/miner/src/state.rs

ZenGround0 · 2025-02-21T21:49:37Z

actors/miner/src/state.rs


-        Ok((from_vesting, from_balance))
+        Ok((to_burn, total_unlocked))


Despite not liking touching this code the change does read as correct.

…funds

actors/miner/src/vesting_state.rs

actors/miner/tests/vesting_unvested_funds.rs

actors/miner/src/state.rs

rvagg · 2025-02-24T10:23:36Z

actors/miner/src/lib.rs

+        // 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.


yes, but in the case where we haven't either had to pay fees or penalties we won't have done this, so going back to the 1/2 day quantisation might be a good idea here because now we're doing it on every deadline regardless

We quantize when we put funds into the vesting queue itself, so we'll vest on that schedule regardless. We only had that buggy check as an optimization to avoid loading the vesting queue on every deadline.

Now that we don't need to load the queue, we can just check every deadline (and make it much harder to have vesting funds). Basically, the queue is now the source of truth.

OK, this makes sense, so confirm my reading of this: when giving block rewards we spread them out so that you get a chunk every 24 hours for a period of 180 days; but we quantize at 12 hours so we could have vesting at a maximum of once every 12 hours. As long as we're not nibbling too much into the next vesting epoch with our fees, we'll always retain that in the head until it actually vests at one of the 12 hour increments and then we get a new head and a rewritten VestingFundsInner block.

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.

Stebalien · 2025-02-24T22:21:24Z

I think I've addressed all the feedback so far.

rvagg · 2025-02-25T03:19:20Z

actors/miner/src/state.rs


-        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)?;


Suggested change

let unlocked_balance = self.get_unlocked_balance(curr_balance)?;

// 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)?;

rvagg · 2025-02-25T03:48:57Z

actors/miner/src/vesting_state.rs

+#[derive(Serialize, Deserialize, Debug, Clone, Default)]
+#[serde(transparent)]
+pub struct VestingFunds(Option<VestingFundsInner>);
+
+#[derive(Serialize_tuple, Deserialize_tuple, Debug, Clone)]
+struct VestingFundsInner {


OK, so this means that in the miner actor state we have a potential null field, but we don't make it Optional there so we have a concrete VestingFunds on which to attach methods that can vary behaviour depending on whether it's null or not?

rvagg · 2025-02-25T04:03:08Z

actors/miner/src/vesting_state.rs

        current_epoch: ChainEpoch,
        vesting_sum: &TokenAmount,
        proving_period_start: ChainEpoch,
        spec: &VestSpec,
-    ) {
+    ) -> Result<TokenAmount, ActorError> {
        // Quantization is aligned with when regular cron will be invoked, in the last epoch of deadlines.


can you sanity check this comment @Stebalien, it doesn't seem true to me; we offset at proving_period_start

rvagg · 2025-02-25T05:02:39Z

actors/miner/src/vesting_state.rs

+    // The "next" batch of vesting funds.
+    head: VestingFund,
+    // The rest of the vesting funds, if any.
+    tail: Cid, // Vec<VestingFund>


This still could be Optional<Cid> so we could avoid storing the empty array CID when you get down to having nothing beyond the head left.

rvagg · 2025-02-25T05:13:54Z

Documenting this and how a migration would work in here: filecoin-project/FIPs#1130

I've said that a migration shouldn't make any attempt to perform vesting in the case that it happens to put a head that's already past its epoch, it'll be resolved properly the next time its touched (and therefore solve #1594 for everyone that's got an active cron).

test: correctly handle cron vesting in tests

66b64bf

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.

Stebalien force-pushed the steb/vesting-head branch from c0f87e1 to c1c4df2 Compare February 21, 2025 04:52

Stebalien commented Feb 21, 2025

View reviewed changes

Stebalien force-pushed the steb/vesting-head branch 2 times, most recently from d4e6f8f to 8c53bc7 Compare February 21, 2025 05:14

Stebalien force-pushed the steb/vesting-head branch from 8c53bc7 to accd9fc Compare February 21, 2025 05:25

Stebalien force-pushed the steb/vesting-head branch from accd9fc to 9565521 Compare February 21, 2025 05:28

rvagg assigned Stebalien Feb 21, 2025

ZenGround0 reviewed Feb 21, 2025

View reviewed changes

Stebalien added 2 commits February 23, 2025 10:08

review: rename unlock_unvested_funds to account for unlocking vested …

94019db

…funds

refactor: small simplifications

e2742fa