Feat/aggregate porep less harsh error #1412
Conversation
actors/builtin/miner/miner_test.go
Outdated
| @@ -2122,7 +2122,7 @@ func (h *cronControl) preCommitToStartCron(t *testing.T, preCommitEpoch abi.Chai | |||
| // PCD != 0 so cron must be active | |||
| h.requireCronActive(t) | |||
|
|
|||
| expiryEpoch := preCommitEpoch + miner.MaxProveCommitDuration[h.actor.sealProofType] + abi.ChainEpoch(1) | |||
| expiryEpoch := preCommitEpoch + miner.MaxProveCommitDuration[h.actor.sealProofType] + miner.PreCommitExpiryDelay | |||
There was a problem hiding this comment.
Update the comment to describe the return value as the epoch at which the precommit should be cleaned up, removed from state by cron. Or maybe return both the logical expiration and the clean-up epoch.
actors/builtin/miner/policy.go
Outdated
| @@ -310,3 +310,4 @@ func RewardForDisputedWindowPoSt(proofType abi.RegisteredPoStProof, disputedPowe | |||
| const MaxAggregatedSectors = 819 | |||
| const MinAggregatedSectors = 1 | |||
| const MaxAggregateProofSize = 192000 | |||
| const PreCommitExpiryDelay = 8 * builtin.EpochsInHour | |||
There was a problem hiding this comment.
Please document the intent here: to leave expired pre-commits in state for a while so they don't take down all the good sectors in a late-running aggregated prove-commit.
Prompted by the comments I made elsewhere, let's also rename this to distinguish expiration from clean-up. So maybe something like [Expired]PreCommitCleanUpDelay.
actors/test/commit_post_test.go
Outdated
| @@ -99,6 +99,10 @@ func TestCommitPoStFlow(t *testing.T) { | |||
| // | |||
|
|
|||
| t.Run("missed prove commit results in precommit expiry", func(t *testing.T) { | |||
| // advance time to precommit expiry | |||
There was a problem hiding this comment.
I suggest being precise in the language here, distinguishing the logical expiration (after which it cannot be proven) from clean-up or garbage-collection epoch.
| balances := vm.GetMinerBalances(t, v, minerAddrs.IDAddress) | ||
| assert.True(t, balances.InitialPledge.GreaterThan(big.Zero())) | ||
| assert.True(t, balances.PreCommitDeposit.GreaterThan(big.Zero())) | ||
|
|
There was a problem hiding this comment.
I suggest rolling forward cron until the expired precommit is cleaned up, too
With the upcoming test vectors project all scenario tests will now generate conformance tests so I now have a slight bias towards testing with scenario tests if it is not too hard to do. In this case I was already warmed up writing similar tests from measuring aggregate porep gas so it seemed worth the extra effort. |
f41487f to
5f76287
Compare
- expired precommits don't cause AggregateProveCommit failure, expired precommit sectors just skipped - precommits removed off chain 8 hours after they have expired from prove commit eligibility
b5eeebe to
fa31738
Compare
Codecov Report
@@ Coverage Diff @@
## spike/aggregate-porep #1412 +/- ##
=====================================================
Coverage 69.8% 69.8%
=====================================================
Files 72 72
Lines 7700 7792 +92
=====================================================
+ Hits 5378 5446 +68
- Misses 1438 1451 +13
- Partials 884 895 +11 |
1ee21c9 to
f7aefcb
Compare
TODO: