Skip to content
This repository has been archived by the owner on Jun 6, 2023. It is now read-only.

Miner: Treat initial pledge like precommit deposits #1007

Closed
wadealexc opened this issue Aug 19, 2020 · 11 comments
Closed

Miner: Treat initial pledge like precommit deposits #1007

wadealexc opened this issue Aug 19, 2020 · 11 comments
Assignees
Labels
discussion P2 Medium priority, beneficial for network functionality and growth robustness Related to correctness
Milestone

Comments

@wadealexc
Copy link

This issue proposes to treat the miner's st.InitialPledgeRequirement the same as st.PreCommitDeposits.

When incurring penalties, the Miner's balance should never be reduced below the amount required to cover st.LockedFunds + st.PreCommitDeposits + st.InitialPledgeRequirement, with the remainder accumulated in st.FeeDebt.

Penalties are incurred via the method st.PenalizeFundsInPriorityOrder, which is invoked in 4 locations:

  • SubmitWindowedPoSt: No changes required.
  • ReportConsensusFault: Consensus fault penalties no longer come out of the miner's initial pledge. Although this may lower the possible reward for reporting a consensus fault, it also prevents miners from intentionally creating consensus faults in order to recover portions of their pledge.
  • processEarlyTerminations: Removal of initial pledge requirement should come before the termination penalty is applied:
    // Unlock funds for penalties.
    // TODO: handle bankrupt miner: https://github.com/filecoin-project/specs-actors/issues/627
    // We're intentionally reducing the penalty paid to what we have.
    unlockedBalance := st.GetUnlockedBalance(rt.CurrentBalance())
    penaltyFromVesting, penaltyFromBalance, err := st.PenalizeFundsInPriorityOrder(store, rt.CurrEpoch(), penalty, unlockedBalance)
    builtin.RequireNoErr(rt, err, exitcode.ErrIllegalState, "failed to unlock unvested funds")
    penalty = big.Add(penaltyFromVesting, penaltyFromBalance)
    // Remove pledge requirement.
    st.AddInitialPledgeRequirement(totalInitialPledge.Neg())
    pledgeDelta = big.Add(totalInitialPledge, penaltyFromVesting).Neg()
  • handleProvingDeadline: Penalties for ongoing faults and undeclared faults should be applied once, and only after initial pledge is adjusted to account for expired sectors:
    // Release pledge requirements for the sectors expiring on-time.
    // Pledge for the sectors expiring early is retained to support the termination fee that will be assessed
    // when the early termination is processed.
    pledgeDelta = big.Sub(pledgeDelta, expired.OnTimePledge)
    st.AddInitialPledgeRequirement(expired.OnTimePledge.Neg())

Changes Required

The following changes are already implemented in #1004:

  • GetUnlockedBalance should additionally subtract st.InitialPledgeRequirement:
    unlockedBalance := big.Subtract(actorBalance, st.LockedFunds, st.PreCommitDeposits)
  • GetAvailableBalance should no longer subtract st.InitialPledgeRequirement:
    unlockedBalance := st.GetUnlockedBalance(actorBalance)
    return big.Subtract(unlockedBalance, st.InitialPledgeRequirement, st.FeeDebt)
  • AssertBalanceInvariants should additionally assert that:
    • st.InitialPledgeRequirement >= 0
    • rt.CurrentBalance() >= Sum(st.LockedFunds, st.PreCommitDeposits, st.InitialPledgeRequirement)
  • MeetsInitialPledgeCondition should return false IFF the miner has positive FeeDebt, as positive FeeDebt indicates that the miner is in "IP debt"

The following changes have yet to be implemented:

  • processEarlyTerminations: Remove IP requirement for terminated sectors before termination penalty is applied
  • handleProvingDeadline: Apply penalties in bulk after sector expiries are accounted for.
@wadealexc
Copy link
Author

cc @ZenGround0 @anorth @acruikshank

@wadealexc
Copy link
Author

The only open question for me is whether to handle debt repayment in handleProvingDeadline.

I think implementing a method RepayAsMuchDebtAsPossible (more succinctly named) would make sense to invoke during handleProvingDeadline, in order to handle the case brought up in #1004.

I'm not convinced it's necessary, though.

@wadealexc
Copy link
Author

Also may be worth noting - I think it's necessary to have an order of operations for debt repayment / penalty calculation, in case these are ever in the same method (like the above idea to put debt repayment in handleProvingDeadline)

Debt repayment needs to happen first, and penalties calculated / applied last.

@Stebalien
Copy link
Member

Note: this changes how we calculate total network pledge collateral.

cc @nicola and @irenegia.

@Stebalien
Copy link
Member

Note: this changes how we calculate total network pledge collateral.

This is wrong. When we burn from "pledge", we don't reduce total network pledge accounting.

@anorth
Copy link
Member

anorth commented Aug 20, 2020

There's currently a wider unsolved problem of how genesis miners will cover their initial pledge requirement. In the current code, we have a policy option that they don't cover it: a genesis miner could have balance below the IP required by genesis sectors. Right now this isn't a practical option because we don't allow miners in IP debt to win blocks, so they couldn't win a reward in order to eventually meet the requirement. But if we change this, that option won't be possible at all.

@anorth anorth added P2 Medium priority, beneficial for network functionality and growth robustness Related to correctness labels Aug 20, 2020
@irenegia
Copy link

This issue proposes to treat the miner's st.InitialPledgeRequirement the same as st.PreCommitDeposits.

Note that there is a intrinsic difference between InitialPledgeRequirement and PreCommitDeposits: Tokens from InitialPledgeRequirement can be burn to pay penalties (from storage or consensus faults), while tokens from PreCommitDeposits can not be used for this. PreCommitDeposits is used only when a ProveCommit fails.

@wadealexc
Copy link
Author

This issue proposes to treat the miner's st.InitialPledgeRequirement the same as st.PreCommitDeposits.

Note that there is a intrinsic difference between InitialPledgeRequirement and PreCommitDeposits: Tokens from InitialPledgeRequirement can be burn to pay penalties (from storage or consensus faults), while tokens from PreCommitDeposits can not be used for this. PreCommitDeposits is used only when a ProveCommit fails.

Yes - this issue proposes to remove that difference.

@nicola
Copy link
Contributor

nicola commented Aug 22, 2020

Hey @wadealexc what is the motivation for this change? (I may be missing context)

@anorth
Copy link
Member

anorth commented Aug 24, 2020

The motivation is simplicity. We currently have three or four different concepts for ways that part of a balance can be locked or unlocked, covered or un-covered. This removes one of them while maintaining the same properties of restricting a miner from election, growth etc.

Rather than having both an IP debt and fee debt concepts, this removes the former. Rather than having both "required" and "locked" funds, this removes the former.

@anorth anorth added this to the 💙 Mainnet milestone Aug 30, 2020
@anorth
Copy link
Member

anorth commented Aug 30, 2020

Done in #1004. Some follow-ups noted in #1032.

@anorth anorth closed this as completed Aug 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion P2 Medium priority, beneficial for network functionality and growth robustness Related to correctness
Projects
None yet
Development

No branches or pull requests

6 participants