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

actors/miner/src/lib.rs

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.

actors/miner/src/state.rs

actors/miner/src/vesting_state.rs

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

It's worse:

We round up to the next proving period start (or 12 hours from then). So, deadline start not deadline end (as you noted).

Then we unlock the epoch after that. I.e., we unlock everything where vest_epoch < current_epoch.

So, really, I think the actual unlock will happen the deadline after.

(I should write a better test)

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.

It could be, but I didn't think that was worth optimizing. Miners should almost never be in that state.

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).

Co-authored-by: Rod Vagg <[email protected]>

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