search

filecoin-project / specs-actors

DEPRECATED Specification of builtin actors, in the form of executable code.
Other
86 stars 102 forks source link

Miner: VerifyPledgeRequirementsAndRepayDebts should return balance available after paying debt #1020

Open wadealexc opened 3 years ago

wadealexc commented 3 years ago

Methods that invoke VerifyPledgeRequirementsAndRepayDebts must be careful to query GetAvailableBalance beforehand, as the returned value would otherwise be inaccurate:

https://github.com/filecoin-project/specs-actors/blob/f45cae407ea3b47ac8155de190826aa4010abc9f/actors/builtin/miner/miner_actor.go#L509-L513

This dependency on the order of invocations is easy to get wrong.

Recommendation

Have VerifyPledgeRequirementsAndRepayDebts return both the amount of debt to burn and the balance available to the miner (assuming the debt is eventually burned).

wadealexc commented 3 years ago

CC @ZenGround0

ZenGround0 commented 3 years ago

Adding onto this one more tweak for VerifyPledgeRequirementsAndRepayDebts (now RepayDebtsOrAbort), from @nicola, debt repayment from this function does not deduct from LockedFunds. It should for symmetry with RepayDebtsInPriorityOrder during initial fee payment.

repayDebts can just become RepayDebtsInPrioirtyOrder wrapped with an abort in the case fee debt is not zero after paying.

@wadeAlexC does anything sound wrong about this proposal?

wadealexc commented 3 years ago

It'll be easier to judge when I see the PR, but I think that sounds good!

anorth commented 3 years ago

We have determined that these are not critical changes for network launch. They represent a solid clean-up and simplification, but as we approach mainnet, changing introduces new risk. I hope to land these in a subsequent discretionary upgrade.

cc @wadeAlexC

nicola commented 3 years ago

cc @irenegia and @zixuanzh

anorth commented 3 years ago

Quoting @ZenGround0 elsewhere on RepayDebtsOrAbort:

While RepayDebts considers only funds from UnlockedBalance, recall that in order to be in fee debt in the first place a miner's vesting funds must be slashed down to zero. Additionally whenever vesting funds are added to a miner in fee debt they are immediately used to pay off the debt here, so I don't expect there to be anyway to hit RepayDebtsOrAbort with non-zero fee debt and non-zero vesting funds. This holds even in the case that we remove the "No fee debt" requirement from election eligibility, which I believe is still in place and will prevent a miner with fee debt from vesting funds in the first place.

Since loading the vesting table is expensive, it's probably worth maintaining this property, but explaining it clearly in the code.