Refactor deadline handling #1009

Stebalien · 2020-08-19T23:25:54Z

Moves most logic into helper functions.
Avoids loading deadlines, vesting table, etc. unnecessarily.
Loads the vesting table at most twice. Once to unlock vested funds, once to unlock unvested funds for penalties.

fixes #983

codecov-commenter · 2020-08-19T23:28:35Z

Codecov Report

Merging #1009 into master will decrease coverage by 0.1%.
The diff coverage is 70.4%.

@@           Coverage Diff            @@
##           master   #1009     +/-   ##
========================================
- Coverage    74.0%   73.8%   -0.2%     
========================================
  Files          57      57             
  Lines        6549    6589     +40     
========================================
+ Hits         4848    4865     +17     
- Misses       1091    1105     +14     
- Partials      610     619      +9

actors/builtin/miner/miner_state.go

actors/builtin/miner/miner_actor.go

anorth

I would prefer to revert most of the AdvanceDeadline refactor. There may be small pure manipulations that could be pushed down, but it shouldn't take parameters like the reward estimate or return values like penalty amounts.

actors/builtin/miner/miner_actor.go

actors/builtin/miner/miner_state.go

Stebalien · 2020-08-20T05:14:44Z

I would prefer to revert most of the AdvanceDeadline refactor. There may be small pure manipulations that could be pushed down, but it shouldn't take parameters like the reward estimate or return values like penalty amounts.

Most of the AdvanceDeadline refactor doesn't actually have anything to do with penalty, just power changes. Let me know what you think about it now.

anorth

After investigating and writing up most of these comments I think I get what's going on, and it's probably right. But I left them all there because it's confusing and I think worth changing so as not to be.

actors/builtin/miner/miner_actor.go

actors/builtin/miner/miner_state.go

actors/builtin/miner/miner_actor.go

anorth · 2020-08-21T04:40:28Z

actors/builtin/miner/miner_state.go

+type AdvanceDeadlineResult struct {
+	PledgeDelta                                abi.TokenAmount
+	PowerDelta                                 PowerPair
+	UndeclaredFaultyPower, DeclaredFaultyPower PowerPair


It took me quite a while to figure out how (I think) these are being used. I think:

UndeclaredFaultyPower is the faults triggered by missing posts

DeclaredFaultyPower was not just declared here, but is the ongoing faulty power to be penalised

I.e. one is an "edge" and one is a "gauge", just reading a value from state. "Declared" is misleading here, since nothing is being declared and also it includes power from faults that were both previously, and in this call, undeclared.

Maybe names could help, but it definitely needs explanation. Maybe

DetectedFaultyPower sum of power of sectors from partitions which were not proven this deadline

OngoingFaultyPower sum of power of sectors that are (or were) faulty

Does the latter include the former? Maybe PreviouslyFaultyPower or AlreadyFaultyPower then.

Currently, "undeclared" is the amount of faulty power we discovered while processing the deadline. "declared" is the amount of faulty power we expected from declarations (and from previous discoveries). I'm using them to match up with the penalty function names.

I swapped out ongoing because it's a bit confusing to me. Before, ongoing actually included newly detected faulty power.

I'm going to change these to "detected" and "total". I think that'll resolve any confusion.

anorth · 2020-08-21T04:43:03Z

actors/builtin/miner/miner_state.go

+		if err != nil {
+			return nil, xerrors.Errorf("failed to process end of deadline %d: %w", dlInfo.Index, err)
+		}
+		declaredFaultyPower = deadline.FaultyPower.Sub(undeclaredFaultyPower)


I don't understand this, or at least it's appearance here. No "declared faults" can happen while processing missed PoSts. In combination with my confusion above, this line would make a bit more sense at the bottom of this method. But if it's just trying to count the faulty power before the new faults were added .... just read deadline.FaultyPower before processing the new faults?

That would have counted failed recoveries as "declared" faulty power. But yeah, this is confusing. I've switched this over to just returning the total faulty power.

anorth · 2020-08-21T04:46:27Z

actors/builtin/miner/miner_test.go

@@ -1207,7 +1207,7 @@ func TestWindowPost(t *testing.T) {
 		actor.submitWindowPoSt(rt, dlinfo, partitions, infos, cfg)

 		// expect declared fee to be charged during cron
-		dlinfo = advanceDeadline(rt, actor, &cronConfig{ongoingFaultsPenalty: declaredFee})
+		dlinfo = advanceDeadline(rt, actor, &cronConfig{declaredFaultsPenalty: declaredFee})


I see some of my earlier confusion now. Here, the original is better IMO. Nothing is being declared, and some of the power may have been an undeclared fault.

anorth · 2020-08-21T04:46:59Z

actors/builtin/miner/miner_test.go

-		ongoingPwr := miner.PowerForSectors(actor.sectorSize, allSectors)
-		ongoingPenalty := miner.PledgePenaltyForDeclaredFault(actor.epochRewardSmooth, actor.epochQAPowerSmooth, ongoingPwr.QA)
+		declaredPwr := miner.PowerForSectors(actor.sectorSize, allSectors[:1])
+		declaredPenalty := miner.PledgePenaltyForDeclaredFault(actor.epochRewardSmooth, actor.epochQAPowerSmooth, declaredPwr.QA)


Ditto, this rename confuses me.

Stebalien · 2020-08-21T17:01:15Z

I've reverted the fee names in the tests but they're still confusing. The "ongoing" power is actually "ongoing minus failed recoveries", that's why I've been avoiding it.

anorth · 2020-08-24T06:54:12Z

actors/builtin/miner/miner_actor.go

-				st.ProvingPeriodStart = st.ProvingPeriodStart + WPoStProvingPeriod
+			penaltyTarget := big.Add(declaredPenalty, undeclaredPenalty)
+			if !penaltyTarget.IsZero() {
+				unlockedBalance := st.GetUnlockedBalance(rt.CurrentBalance())


I think this was already wrong, but I this calculation is incorrect. Earlier on in this method, the pre-commit deposit amount was reduced and the associated amount to be burnt remembered in penaltyTotal. rt.CurrentBalance() still includes that amount that is to be burnt, and so this will calculate a greater unlocked balance than it should, if it's to be used as a bound on how much more is available to burn now. I think this could result in an attempt to burn more funds than the actor has (rather than accumulating fee debt in PenalizeFundsInPriorityOrder).

Since this was already wrong and @ZenGround0 is working on related changes in #1004 (in part to reduce the ease of errors like this) I'm ok with us landing this still broken, to be repaired in 1004.

A test case to show this needs to incur a penalty in the same epoch as expiring an overdue pre-commit (for a follow-up PR).

actors/builtin/miner/miner_state.go

* Moves most logic into helper functions. * Avoids loading deadlines, vesting table, etc. unnecessarily. * Loads the vesting table at most twice.

Stebalien commented Aug 19, 2020

View reviewed changes

actors/builtin/miner/miner_state.go Outdated Show resolved Hide resolved

actors/builtin/miner/miner_actor.go Show resolved Hide resolved

actors/builtin/miner/miner_actor.go Show resolved Hide resolved

actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved

Base automatically changed from steb/delay-power to master August 20, 2020 00:00

Stebalien force-pushed the steb/fast-deadline branch from cc50a71 to 7ebee1a Compare August 20, 2020 00:01

Stebalien requested a review from anorth August 20, 2020 01:09

anorth suggested changes Aug 20, 2020

View reviewed changes

Stebalien requested a review from anorth August 20, 2020 05:52

anorth reviewed Aug 21, 2020

View reviewed changes

Stebalien force-pushed the steb/fast-deadline branch 2 times, most recently from 011a9f3 to eb8dfa4 Compare August 21, 2020 17:00

anorth mentioned this pull request Aug 24, 2020

IP accounting refactor #1004

Merged

anorth approved these changes Aug 24, 2020

View reviewed changes

Stebalien added 5 commits August 24, 2020 11:33

Refactor deadline handling

1200fd2

* Moves most logic into helper functions. * Avoids loading deadlines, vesting table, etc. unnecessarily. * Loads the vesting table at most twice.

Address CR

d748111

Partially address CR

23f9db8

Revert fee rename in tests

aa8f68d

Address final CR

80c3e0d

Stebalien force-pushed the steb/fast-deadline branch from eb8dfa4 to 80c3e0d Compare August 24, 2020 18:42

Stebalien merged commit d67d2a7 into master Aug 24, 2020

Stebalien deleted the steb/fast-deadline branch August 24, 2020 18:47

anorth mentioned this pull request Sep 9, 2020

prevent faults from double subtracting cc upgrade power #1129

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor deadline handling #1009

Refactor deadline handling #1009

Stebalien commented Aug 19, 2020 •

edited

Loading

codecov-commenter commented Aug 19, 2020 •

edited

Loading

anorth left a comment

Stebalien commented Aug 20, 2020

anorth left a comment

anorth Aug 21, 2020

Stebalien Aug 21, 2020

anorth Aug 21, 2020

Stebalien Aug 21, 2020

anorth Aug 21, 2020

anorth Aug 21, 2020

Stebalien commented Aug 21, 2020

anorth Aug 24, 2020

Refactor deadline handling #1009

Refactor deadline handling #1009

Conversation

Stebalien commented Aug 19, 2020 • edited Loading

codecov-commenter commented Aug 19, 2020 • edited Loading

Codecov Report

anorth left a comment

Choose a reason for hiding this comment

Stebalien commented Aug 20, 2020

anorth left a comment

Choose a reason for hiding this comment

anorth Aug 21, 2020

Choose a reason for hiding this comment

Stebalien Aug 21, 2020

Choose a reason for hiding this comment

anorth Aug 21, 2020

Choose a reason for hiding this comment

Stebalien Aug 21, 2020

Choose a reason for hiding this comment

anorth Aug 21, 2020

Choose a reason for hiding this comment

anorth Aug 21, 2020

Choose a reason for hiding this comment

Stebalien commented Aug 21, 2020

anorth Aug 24, 2020

Choose a reason for hiding this comment

Stebalien commented Aug 19, 2020 •

edited

Loading

codecov-commenter commented Aug 19, 2020 •

edited

Loading