feat(IValidator): Allow Forward Progress Despite Missing Proofs #321
Conversation
|
|
||
| // Count proven epochs and find the last proven epoch | ||
| (uint256 provenEpochCount, uint256 lastProvenEpoch) = | ||
| (uint256 provenEpochCount, uint256 settledUpTo) = |
There was a problem hiding this comment.
I feel like "settled" isn't the right language for (and in) _findProvenEpochs, maybe "settleUpTo" as an instruction would be more accurate? Or even "provenUpTo" as a description—although that's tricky because it's not necessarily all proven, it's more like "this is the last historical epoch within which you can make a settlement". The event itself has "settle" so I think it's just this internal usage that's making me trip over what on earth _findProvenEpochs is trying to do for us.
There was a problem hiding this comment.
I think settle is the right word based on how it's used in settleRail. Once the period is faulted it's not going to become unfaulted, so we should mark progress, and that does settle it.
There was a problem hiding this comment.
k, well let's remove the "d" from here and in _findProvenEpochs, it's odd
| revert Errors.InvalidChallengeEpoch(dataSetId, minWindow, maxWindow, challengeEpoch); | ||
| } | ||
| provingDeadlines[dataSetId] = firstDeadline; | ||
| provenThisPeriod[dataSetId] = false; |
There was a problem hiding this comment.
we can expect this to already be false when this data set's proving deadline is not initialized
| uint256 completedPeriodId = getProvingPeriodForEpoch(dataSetId, provingDeadlines[dataSetId] - 1); | ||
|
|
||
| // Record whether this period was proven | ||
| provenPeriods[dataSetId][completedPeriodId >> 8] |= 1 << (completedPeriodId & 255); |
There was a problem hiding this comment.
this is already set by possessionProven
There was a problem hiding this comment.
hah, odd, any idea why it's in here too? hopefully just an oversight and not some functionality I'm not seeing
There was a problem hiding this comment.
The code has changed over time. Before it was a bitmap, this could set both true and false. Since I also see things initialized to false elsewhere, perhaps the original author did not know that false was the default.
|
|
||
| function _isPeriodProven(uint256 dataSetId, uint256 periodId, uint256 currentPeriod) private view returns (bool) { | ||
| if (periodId == currentPeriod) { | ||
| return provenThisPeriod[dataSetId]; |
There was a problem hiding this comment.
because this is set by possessionProven, we do not need to also check it here. in fact it might be a bug to do so
| settledUpTo = _calcPeriodDeadline(activationEpoch, endingPeriod - 1); | ||
| settleUpTo = _calcPeriodDeadline(activationEpoch, endingPeriod - 1); | ||
|
|
||
| // handle the last period separately |
There was a problem hiding this comment.
| // handle the last period separately | |
| // handle the last period separately to account for partial epoch count (toEpoch < deadline) |
github-project-automation
bot
moved this from 📌 Triage
to ✔️ Approved by reviewer
in FS
Before this change, settleUpTo is ignored in the case where no epochs are proven. After this change it is not. |
Reviewer @rvagg
It is only important to block forward progress through the current proving period.
Currently we don't allow forward progress through unsettled periods without proofs.
This could be a problem if there was a rail unproven for a long time but proofs were later resumed.
Though much less likely after #258 and #267, we would not want it to be possible for the gap to exceed the block gas limit.
Also important to recall that the
fromEpochis an exclusive index whiletoEpochis an inclusive index.Changes