Epoch Interface Refactor: Split into TentativeEpoch and CommittedEpoch #6941
Conversation
Updates EpochQuery.Next() to return a TentativeEpoch. (Will later be renamed to NextUnsafe). All users of methods not present on TentativeEpoch now use new method EpochQuery.NextCommitted(), which returns a regular Epoch (to later be renamed to CommittedEpoch). If NextUnsafe is called during the EpochCommitted phase, it returns an error.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/epoch-interface-refactor #6941 +/- ##
====================================================================
- Coverage 41.13% 41.01% -0.12%
====================================================================
Files 2119 2121 +2
Lines 185938 186408 +470
====================================================================
- Hits 76479 76460 -19
- Misses 103054 103534 +480
- Partials 6405 6414 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Use conflicting Clustering/collector assignments. When simply setting a different list of participants without going through a staking or unstaking process, Epoch Fallback mode was often but not always triggered.
| @@ -338,7 +338,7 @@ func (e *ReactorEngine) handleEpochCommittedPhaseStarted(currentEpochCounter uin | |||
| // TODO document error returns | |||
| func (e *ReactorEngine) getDKGInfo(firstBlockID flow.Identifier) (*dkgInfo, error) { | |||
| currEpoch := e.State.AtBlockID(firstBlockID).Epochs().Current() | |||
| nextEpoch := e.State.AtBlockID(firstBlockID).Epochs().Next() | |||
| nextEpoch := e.State.AtBlockID(firstBlockID).Epochs().NextUnsafe() | |||
There was a problem hiding this comment.
This function is only called from ReactorEngine.startDKGForEpoch during the EpochSetup phase
| @@ -18,7 +18,7 @@ type ClusterRootQCVoter interface { | |||
| // Error returns: | |||
| // - epochs.ClusterQCNoVoteError if we fail to vote for a benign reason | |||
| // - generic error in case of critical unexpected failure | |||
| Vote(context.Context, protocol.Epoch) error | |||
| Vote(context.Context, protocol.TentativeEpoch) error | |||
There was a problem hiding this comment.
QC Voting occurs during the EpochSetup phase
|
|
||
| setup2FinalView, err := state.AtBlockID(block8.ID()).Epochs().Next().FinalView() | ||
| phase, err := state.AtBlockID(block8.ID()).EpochPhase() |
There was a problem hiding this comment.
I initially tried changing the participants (InitialIdentities) in the EpochSetup event, without having any nodes stake or unstake - this would often trigger Epoch Fallback Mode at block 8, but inconsistently. Not sure if it was a race condition or if it should be investigated more.
| // epoch setup service event. The input must be a valid, set up epoch. | ||
| // Error returns: | ||
| // * protocol.ErrNoPreviousEpoch - if the epoch represents a previous epoch which does not exist. | ||
| // * protocol.ErrNextEpochNotSetup - if the epoch represents a next epoch which has not been set up. | ||
| // * state.ErrUnknownSnapshotReference - if the epoch is queried from an unresolvable snapshot. | ||
| func ToEpochSetup(epoch Epoch) (*flow.EpochSetup, error) { | ||
| func ToEpochSetup(epoch CommittedEpoch) (*flow.EpochSetup, error) { |
There was a problem hiding this comment.
This is only a helper function used in tests; even though it only requires the data from the EpochSetup event, we no longer make all of that information available through the TentativeEpoch interface, and the tests using it can use committed epochs instead.
| @@ -262,7 +272,7 @@ func (e *heightBoundedEpoch) FinalHeight() (uint64, error) { | |||
| // NewSetupEpoch returns a memory-backed epoch implementation based on an EpochSetup event. | |||
| // Epoch information available after the setup phase will not be accessible in the resulting epoch instance. | |||
| // No errors are expected during normal operations. | |||
| func NewSetupEpoch(setupEvent *flow.EpochSetup, extensions []flow.EpochExtension) protocol.Epoch { | |||
| func NewSetupEpoch(setupEvent *flow.EpochSetup, extensions []flow.EpochExtension) protocol.TentativeEpoch { | |||
There was a problem hiding this comment.
Only called during the EpochSetup phase.
| @@ -343,7 +343,7 @@ func (e *Engine) handleEpochEvents(ctx irrecoverable.SignalerContext, ready comp | |||
| ctx.Throw(err) | |||
| } | |||
| case firstBlock := <-e.epochSetupPhaseStartedEvents: | |||
| nextEpoch := e.state.AtBlockID(firstBlock.ID()).Epochs().Next() | |||
| nextEpoch := e.state.AtBlockID(firstBlock.ID()).Epochs().NextUnsafe() | |||
There was a problem hiding this comment.
Should only occur at the start of the EpochSetup phase
Split protocol.Epoch interface into TentativeEpoch (representing a setup but not yet committed epoch) and CommittedEpoch. The TentativeEpoch interface is deliberately restricted to avoid accidental misuse.
EpochQuery.Next method is split into
NextCommittedto retrieve a committed epoch andNextUnsafeto retrieve a tentative epoch.PreviousandCurrentare changed to return a CommittedEpoch.This is the first part of the Epoch Interface refactor; see #6191 and https://www.notion.so/flowfoundation/Differentiating-Committed-vs-Uncommitted-Tentative-Epoch-State-17a1aee1232480bc8b37e2adb3e19b28