Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No vesting during cron #1594

Open
ZenGround0 opened this issue Jan 13, 2025 · 5 comments · May be fixed by #1636
Open

No vesting during cron #1594

ZenGround0 opened this issue Jan 13, 2025 · 5 comments · May be fixed by #1636

Comments

@ZenGround0
Copy link
Contributor

Funds should be vesting during cron. However there are instances where an active deadline cron does not vest any funds such that there are unclaimed vesting table entries in the past.

The culprit is likely related to faulty assumptions about the vesting table structure and deadline cron epoch alignment. It was probably introduced in #1424.

@github-project-automation github-project-automation bot moved this to 📌 Triage in FilOz Jan 13, 2025
@ZenGround0
Copy link
Contributor Author

Since everyone appears to be fine with this issue and we've heard no complaints for the last year I think once we understand and confirm that there is nothing going wrong here that this should be a wont fix. We should go back and remove the if-guarded vesting unlock entirely to avoid any future confusion.

@rvagg
Copy link
Member

rvagg commented Jan 14, 2025

filecoin-project/lotus#12828 to inspect the scale of the problem

@rvagg
Copy link
Member

rvagg commented Feb 19, 2025

Noting this while I stumble upon it: I think it comes from #1424

This:

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

Which is only called on handle_proving_deadline() which is the last epoch in a deadline. But the offset is offset: state.proving_period_start and we're trying to match q.quantize_up(curr_epoch) == curr_epoch. So I think we have an off-by-one here, we probably need to do one of:

  1. == curr_epoch + 1 to match a deadline start
  2. set the offset to be - 1 what it is now to match deadline end
  3. quantize the RHS of the comparison to match a deadline start, like State::quant_spec_every_deadline()

The question is - why do some miners seem to get properly vested funds if this is universally wrong? Using my little tool to have a look (filecoin-project/lotus#12828 - although I have a modified version locally that just runs through the power actor for miners with claims) my current stats say:

Total miners with power: 1669
Total miners with non-empty vesting schedules: 1662
Total miners with stale should-have-vested locked funds: 729
Total locked vested funds: 7867.239523341960108035 FIL

So a lot of them appear to be vesting properly. There's another way that you can get your vesting unlocked, add_locked_funds does it before adding anything new. That's done in apply_rewards. So that would seem to mean that if you're earning block rewards then you're getting your unlocks, but if you're not then you're relying on cron and it's off-by-one and not doing the unlock for you.

@ZenGround0
Copy link
Contributor Author

I agree with your assessment and that

  1. == curr_epoch + 1 is a good solution

Alternatively we could just remove vesting entirely from cron since SPs have worked around this for the last year and now know how to handle this. This would save subsidized cron compute which is nice.

I don't have very strong feelings either way.

@Stebalien
Copy link
Member

Failing test: #1635 case.

Alternatively we could just remove vesting entirely from cron since SPs have worked around this for the last year and now know how to handle this. This would save subsidized cron compute which is nice.

Assuming FIP-0100 passes, we'll be charging a fee from vesting funds every epoch so we should probably just vest in cron anyways (I'm currently working on making this not have to load/store the entire vesting queue every time...).

Stebalien added a commit that referenced this issue Feb 21, 2025
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 added a commit that referenced this issue Feb 21, 2025
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 added a commit that referenced this issue Feb 21, 2025
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 added a commit that referenced this issue Feb 21, 2025
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 added a commit that referenced this issue Feb 21, 2025
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 added a commit that referenced this issue Feb 21, 2025
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 added a commit that referenced this issue Feb 21, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📌 Triage
Development

Successfully merging a pull request may close this issue.

3 participants