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

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

Built on #1635.

actors/miner/src/lib.rs

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.

actors/miner/src/vesting_state.rs

actors/miner/src/state.rs

actors/miner/src/vesting_state.rs

actors/miner/src/lib.rs

actors/miner/src/state.rs

actors/miner/src/vesting_state.rs

actors/miner/tests/vesting_unvested_funds.rs

actors/miner/src/state.rs

actors/miner/src/lib.rs

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

I've also changed the `LoadVestingFunds` to return a `[]VestingFund` so we can abstract across the different underlying representations. related to filecoin-project/builtin-actors#1636

Stebalien · 2025-02-27T00:15:27Z

Re-do of filecoin-project/go-state-types#353, merging in the correct order.

I've also changed the `LoadVestingFunds` to return a `[]VestingFund` so we can abstract across the different underlying representations. related to filecoin-project/builtin-actors#1636

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

actors/miner/src/state.rs

actors/miner/src/vesting_state.rs

rvagg · 2025-02-27T04:34:39Z

I've done another pass and this lgtm but I'll wait to see it squashed and rebased on feat/fip-0100 before 👍

actors/miner/tests/util.rs

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/tests/util.rs

rvagg · 2025-02-27T07:21:36Z

All good, but:

I think you've unnecessarily overcomplicated the bottom of advance_and_submit_posts()
Comment at the top of add_locked_funds() still needs to be made accurate, and if you add a test that demonstrates precisely where vesting happens then that's a bonus

Stebalien · 2025-02-27T18:39:26Z

I think you've unnecessarily overcomplicated the bottom of advance_and_submit_posts()

You're right. The real issue was that the test was using stale state (which I also fixed).

* 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. * 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 * doc: explain deadline alignment * review: simplify test

I've also changed the `LoadVestingFunds` to return a `[]VestingFund` so we can abstract across the different underlying representations. related to filecoin-project/builtin-actors#1636

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 2 times, most recently from accd9fc to 9565521 Compare February 21, 2025 05:28

rvagg assigned Stebalien Feb 21, 2025

ZenGround0 reviewed Feb 21, 2025

View reviewed changes

actors/miner/src/lib.rs Show resolved Hide resolved

actors/miner/src/lib.rs Show resolved Hide resolved

actors/miner/src/state.rs Outdated Show resolved Hide resolved

actors/miner/src/state.rs Show resolved Hide resolved

actors/miner/src/state.rs Outdated Show resolved Hide resolved

ZenGround0 reviewed Feb 21, 2025

View reviewed changes

actors/miner/src/state.rs Show resolved Hide resolved