PreCommitPolicy: Don't try to align expirations on proving period boundaries #7018
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Subsumes #6729 |
arajasek
commented
Aug 10, 2021
| expected := h + policy.GetMaxSectorExpirationExtension() - (pBuffer * 2) | ||
| // as set just before returning within Expiration() | ||
| expected += miner.WPoStProvingPeriod - (expected % miner.WPoStProvingPeriod) + pBoundary - 1 | ||
| expected := h + policy.GetMaxSectorExpirationExtension() - pBuffer |
There was a problem hiding this comment.
I don't know why these were subtracting out pBuffer * 2, since that isn't what the cc lifetime method seems to do.
There was a problem hiding this comment.
I did ask for extra scrutiny, but my implementation is definitely wrong here, hidden by the chosen constants. Thanks for catching this. Only rationale I can come with here was to match this but it's external to the Policy entirely. Not sure why I landed with that.
Stebalien
approved these changes
Aug 10, 2021
storage/miner.go
Outdated
| provingBoundary = md.PeriodStart % md.WPoStProvingPeriod | ||
| provingBuffer = md.WPoStProvingPeriod * 2 | ||
| cfg = sealing.GetSealingConfigFunc(m.getSealConfig) | ||
| provingBuffer = md.WPoStProvingPeriod * 2 | ||
|
|
||
| // TODO: Maybe we update this policy after actor upgrades? |
There was a problem hiding this comment.
Can drop the comment. That comment was probably about removing the proving boundary (which we used to require).
arajasek
force-pushed
the
asr/sector-exp-alignment
branch
from
0caaf6b to
a52352b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After a thorough examination of actors code, the PreCommitPolicy trying to align expirations on proving period boundaries is entirely unnecessary (and not even doing what it thinks it should be doing). Removing this also fixes #6065.
Also included in this PR is a check that the proposed PCP expiration isn't at risk of being invalidly small as a result of the PC message taking too long to land on chain. "At risk" is currently defined as "within 24 hours", though this should probably be a config setting.