PreCommitPolicy: Fix edge-case when deal duration is close to the MaxSectorExpirationExtension #6729
Conversation
arajasek
force-pushed
the
asr/deal-edge-case
branch
2 times, most recently
from
36f4994 to
56283c4
Compare
| exp = *hardLimit | ||
| } | ||
|
|
||
| exp += miner.WPoStProvingPeriod - (exp % miner.WPoStProvingPeriod) + p.provingBoundary - 1 |
There was a problem hiding this comment.
Not sure if this is even needed anymore in v5 actors. At least could add a comment on what this is doing
| exp += miner.WPoStProvingPeriod - (exp % miner.WPoStProvingPeriod) + p.provingBoundary - 1 | |
| // align on proving period boundary | |
| exp += miner.WPoStProvingPeriod - (exp % miner.WPoStProvingPeriod) + p.provingBoundary - 1 |
There was a problem hiding this comment.
If it's not needed I'd love to take it out, I just don't know. Adding comment for now, with a TODO saying it could be taken out.
There was a problem hiding this comment.
I don't think it has been needed for a while. But @ZenGround0 would know fo rsure.
|
|
||
| if exp < minEnd { | ||
| // we need to add* ceil(end - maxEnd) days | ||
| daysToAdd := (minEnd-exp)/miner.WPoStProvingPeriod + 1 |
There was a problem hiding this comment.
Isn't this doing floor()+1? Ceil would be ((minEnd-exp) + miner.WPoStProvingPeriod - 1)/miner.WPoStProvingPeriod
There was a problem hiding this comment.
I mean, in this case with 2779/2880 probability, ceil = floor + 1 :P
There was a problem hiding this comment.
(and in the 1/2880 case, we're off by 1 day, so no biggie)
…MaxSectorExpirationExtension
arajasek
force-pushed
the
asr/deal-edge-case
branch
from
56283c4 to
ef67e38
Compare
|
@arajasek this needs a rebase. I've tried, but it looks like the code currently in master is slightly different and takes some form of "proving buffer" into account. |
|
Thank you for submitting the PR and contributing to lotus! Lotus maintainers need more of your input before merging it, please address the suggested changes or reply to the comments or this PR will be closed in 48 hours. You are always more than welcome to reopen the PR later as well! |
Fixes #6065
TODO: Add tests to precommit_policy_test