From a7e97fb4f384ac34bb1500a3ea7ab03b9deb5335 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 10 May 2024 17:53:39 +0300 Subject: [PATCH 1/8] Moved transition to the next epoch to common state machine. Updated tests --- .../epochs/base_statemachine.go | 31 +++++++ .../epochs/fallback_statemachine.go | 7 -- .../epochs/fallback_statemachine_test.go | 89 +++++++++++++++++-- .../epochs/happy_path_statemachine.go | 30 ------- 4 files changed, 112 insertions(+), 45 deletions(-) diff --git a/state/protocol/protocol_state/epochs/base_statemachine.go b/state/protocol/protocol_state/epochs/base_statemachine.go index 6b61f784ddd..306ea0eb726 100644 --- a/state/protocol/protocol_state/epochs/base_statemachine.go +++ b/state/protocol/protocol_state/epochs/base_statemachine.go @@ -1,6 +1,7 @@ package epochs import ( + "fmt" "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/state/protocol" ) @@ -95,3 +96,33 @@ func (u *baseStateMachine) EjectIdentity(nodeID flow.Identifier) error { } return nil } + +// TransitionToNextEpoch updates the notion of 'current epoch', 'previous' and 'next epoch' in the protocol +// state. An epoch transition is only allowed when: +// - next epoch has been set up, +// - next epoch has been committed, +// - invalid state transition has not been attempted (this is ensured by constructor), +// - candidate block is in the next epoch. +// No errors are expected during normal operations. +func (u *baseStateMachine) TransitionToNextEpoch() error { + nextEpoch := u.state.NextEpoch + // Check if there is next epoch protocol state + if nextEpoch == nil { + return fmt.Errorf("protocol state has not been setup yet") + } + // Check if there is a commit event for next epoch + if nextEpoch.CommitID == flow.ZeroID { + return fmt.Errorf("protocol state has not been committed yet") + } + // Check if we are at the next epoch, only then a transition is allowed + if u.view < u.parentState.NextEpochSetup.FirstView { + return fmt.Errorf("protocol state transition is only allowed when enterring next epoch") + } + u.state = &flow.ProtocolStateEntry{ + PreviousEpoch: &u.state.CurrentEpoch, + CurrentEpoch: *u.state.NextEpoch, + InvalidEpochTransitionAttempted: u.state.InvalidEpochTransitionAttempted, + } + u.rebuildIdentityLookup() + return nil +} diff --git a/state/protocol/protocol_state/epochs/fallback_statemachine.go b/state/protocol/protocol_state/epochs/fallback_statemachine.go index 86c38374a3c..d9109d7fc35 100644 --- a/state/protocol/protocol_state/epochs/fallback_statemachine.go +++ b/state/protocol/protocol_state/epochs/fallback_statemachine.go @@ -98,10 +98,3 @@ func (m *FallbackStateMachine) ProcessEpochCommit(_ *flow.EpochCommit) (bool, er // won't process if we are in fallback mode return false, nil } - -// TransitionToNextEpoch performs transition to next epoch, in epoch fallback no transitions are possible. -// TODO for 'leaving Epoch Fallback via special service event' this might need to change. -func (m *FallbackStateMachine) TransitionToNextEpoch() error { - // won't process if we are in fallback mode - return nil -} diff --git a/state/protocol/protocol_state/epochs/fallback_statemachine_test.go b/state/protocol/protocol_state/epochs/fallback_statemachine_test.go index 5ed50283cca..f7656a96584 100644 --- a/state/protocol/protocol_state/epochs/fallback_statemachine_test.go +++ b/state/protocol/protocol_state/epochs/fallback_statemachine_test.go @@ -61,14 +61,88 @@ func (s *EpochFallbackStateMachineSuite) TestProcessEpochCommitIsNoop() { require.Equal(s.T(), s.parentProtocolState.ID(), s.stateMachine.ParentState().ID()) } -// TestTransitionToNextEpoch ensures that transition to next epoch is not possible. +// TestTransitionToNextEpoch tests a scenario where the FallbackStateMachine processes first block from next epoch. +// It has to discard the parent state and build a new state with data from next epoch. func (s *EpochFallbackStateMachineSuite) TestTransitionToNextEpoch() { - err := s.stateMachine.TransitionToNextEpoch() - require.NoError(s.T(), err) - updatedState, updateStateID, hasChanges := s.stateMachine.Build() - require.False(s.T(), hasChanges) - require.Equal(s.T(), updatedState.ID(), updateStateID) - require.Equal(s.T(), s.parentProtocolState.ID(), updateStateID) + // update protocol state with next epoch information + unittest.WithNextEpochProtocolState()(s.parentProtocolState) + + candidate := unittest.BlockHeaderFixture( + unittest.HeaderWithView(s.parentProtocolState.CurrentEpochSetup.FinalView + 1)) + + expectedState := &flow.ProtocolStateEntry{ + PreviousEpoch: s.parentProtocolState.CurrentEpoch.Copy(), + CurrentEpoch: *s.parentProtocolState.NextEpoch.Copy(), + NextEpoch: nil, + InvalidEpochTransitionAttempted: true, + } + + s.Run("happy-path", func() { + parentProtocolState := s.parentProtocolState.Copy() + parentProtocolState.InvalidEpochTransitionAttempted = false + + var err error + // since the candidate block is from next epoch, HappyPathStateMachine should transition to next epoch + s.stateMachine, err = NewFallbackStateMachine(s.params, candidate.View, parentProtocolState.Copy()) + require.NoError(s.T(), err) + err = s.stateMachine.TransitionToNextEpoch() + require.NoError(s.T(), err) + updatedState, stateID, hasChanges := s.stateMachine.Build() + require.True(s.T(), hasChanges) + require.NotEqual(s.T(), parentProtocolState.ID(), updatedState.ID()) + require.Equal(s.T(), updatedState.ID(), stateID) + require.Equal(s.T(), updatedState, expectedState, "state should be equal to expected one") + }) + s.Run("epoch-fallback-mode", func() { + parentProtocolState := s.parentProtocolState.Copy() + parentProtocolState.InvalidEpochTransitionAttempted = true // just to be explicit + + var err error + // since the candidate block is from next epoch, HappyPathStateMachine should transition to next epoch + s.stateMachine, err = NewFallbackStateMachine(s.params, candidate.View, parentProtocolState.Copy()) + require.NoError(s.T(), err) + err = s.stateMachine.TransitionToNextEpoch() + require.NoError(s.T(), err) + updatedState, stateID, hasChanges := s.stateMachine.Build() + require.True(s.T(), hasChanges) + require.NotEqual(s.T(), parentProtocolState.ID(), updatedState.ID()) + require.Equal(s.T(), updatedState.ID(), stateID) + require.Equal(s.T(), updatedState, expectedState, "state should be equal to expected one") + }) +} + +// TestTransitionToNextEpochNotAllowed tests different scenarios where transition to next epoch is not allowed. +func (s *EpochFallbackStateMachineSuite) TestTransitionToNextEpochNotAllowed() { + s.Run("no next epoch protocol state", func() { + protocolState := unittest.EpochStateFixture() + candidate := unittest.BlockHeaderFixture( + unittest.HeaderWithView(protocolState.CurrentEpochSetup.FinalView + 1)) + stateMachine, err := NewFallbackStateMachine(s.params, candidate.View, protocolState) + require.NoError(s.T(), err) + err = stateMachine.TransitionToNextEpoch() + require.Error(s.T(), err, "should not allow transition to next epoch if there is no next epoch protocol state") + }) + s.Run("next epoch not committed", func() { + protocolState := unittest.EpochStateFixture(unittest.WithNextEpochProtocolState(), func(entry *flow.RichProtocolStateEntry) { + entry.NextEpoch.CommitID = flow.ZeroID + entry.NextEpochCommit = nil + }) + candidate := unittest.BlockHeaderFixture( + unittest.HeaderWithView(protocolState.CurrentEpochSetup.FinalView + 1)) + stateMachine, err := NewFallbackStateMachine(s.params, candidate.View, protocolState) + require.NoError(s.T(), err) + err = stateMachine.TransitionToNextEpoch() + require.Error(s.T(), err, "should not allow transition to next epoch if it is not committed") + }) + s.Run("candidate block is not from next epoch", func() { + protocolState := unittest.EpochStateFixture(unittest.WithNextEpochProtocolState()) + candidate := unittest.BlockHeaderFixture( + unittest.HeaderWithView(protocolState.CurrentEpochSetup.FinalView)) + stateMachine, err := NewFallbackStateMachine(s.params, candidate.View, protocolState) + require.NoError(s.T(), err) + err = stateMachine.TransitionToNextEpoch() + require.Error(s.T(), err, "should not allow transition to next epoch if next block is not first block from next epoch") + }) } // TestNewEpochFallbackStateMachine tests that creating epoch fallback state machine sets @@ -423,5 +497,4 @@ func (s *EpochFallbackStateMachineSuite) TestEpochFallbackStateMachineInjectsMul require.Greater(s.T(), parentProtocolState.CurrentEpochFinalView(), candidateView, "final view should be greater than final view of test") } - } diff --git a/state/protocol/protocol_state/epochs/happy_path_statemachine.go b/state/protocol/protocol_state/epochs/happy_path_statemachine.go index 8bbc38e3076..b73bbde1995 100644 --- a/state/protocol/protocol_state/epochs/happy_path_statemachine.go +++ b/state/protocol/protocol_state/epochs/happy_path_statemachine.go @@ -152,33 +152,3 @@ func (u *HappyPathStateMachine) ProcessEpochCommit(epochCommit *flow.EpochCommit u.state.NextEpoch.CommitID = epochCommit.ID() return true, nil } - -// TransitionToNextEpoch updates the notion of 'current epoch', 'previous' and 'next epoch' in the protocol -// state. An epoch transition is only allowed when: -// - next epoch has been set up, -// - next epoch has been committed, -// - invalid state transition has not been attempted (this is ensured by constructor), -// - candidate block is in the next epoch. -// No errors are expected during normal operations. -func (u *HappyPathStateMachine) TransitionToNextEpoch() error { - nextEpoch := u.state.NextEpoch - // Check if there is next epoch protocol state - if nextEpoch == nil { - return fmt.Errorf("protocol state has not been setup yet") - } - // Check if there is a commit event for next epoch - if nextEpoch.CommitID == flow.ZeroID { - return fmt.Errorf("protocol state has not been committed yet") - } - // Check if we are at the next epoch, only then a transition is allowed - if u.view < u.parentState.NextEpochSetup.FirstView { - return fmt.Errorf("protocol state transition is only allowed when enterring next epoch") - } - u.state = &flow.ProtocolStateEntry{ - PreviousEpoch: &u.state.CurrentEpoch, - CurrentEpoch: *u.state.NextEpoch, - InvalidEpochTransitionAttempted: false, - } - u.rebuildIdentityLookup() - return nil -} From 4463557304760cb1f502daa8de9c2652cc5b38cc Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 10 May 2024 17:57:00 +0300 Subject: [PATCH 2/8] Linted --- state/protocol/protocol_state/epochs/base_statemachine.go | 1 + 1 file changed, 1 insertion(+) diff --git a/state/protocol/protocol_state/epochs/base_statemachine.go b/state/protocol/protocol_state/epochs/base_statemachine.go index 306ea0eb726..5787751a9a7 100644 --- a/state/protocol/protocol_state/epochs/base_statemachine.go +++ b/state/protocol/protocol_state/epochs/base_statemachine.go @@ -2,6 +2,7 @@ package epochs import ( "fmt" + "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/state/protocol" ) From bec50e0081d4a0def0a1e65a489a27e3287e72c7 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 10 May 2024 18:28:44 +0300 Subject: [PATCH 3/8] Enabled and fixed tests for epoch transition in EFM mode and injecting epoch extensions --- .../epochs/fallback_statemachine_test.go | 39 ++++++++----------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/state/protocol/protocol_state/epochs/fallback_statemachine_test.go b/state/protocol/protocol_state/epochs/fallback_statemachine_test.go index f7656a96584..e03d7c949a2 100644 --- a/state/protocol/protocol_state/epochs/fallback_statemachine_test.go +++ b/state/protocol/protocol_state/epochs/fallback_statemachine_test.go @@ -390,8 +390,6 @@ func (s *EpochFallbackStateMachineSuite) TestEpochFallbackStateMachineInjectsMul // It is expected that it will transition into the next epoch (since it was committed) // then reach the safety threshold and add the extension to the next epoch, which at that point will be considered 'current'. func (s *EpochFallbackStateMachineSuite) TestEpochFallbackStateMachineInjectsMultipleExtensions_NextEpochCommitted() { - unittest.SkipUnless(s.T(), unittest.TEST_TODO, - "This test doesn't work since it misses logic to transition to the next epoch when we are in EFM.") originalParentState := s.parentProtocolState.Copy() originalParentState.InvalidEpochTransitionAttempted = false unittest.WithNextEpochProtocolState()(originalParentState) @@ -421,7 +419,6 @@ func (s *EpochFallbackStateMachineSuite) TestEpochFallbackStateMachineInjectsMul firstExtensionViewThreshold := originalParentState.NextEpochSetup.FinalView + DefaultEpochExtensionViewCount - s.params.EpochCommitSafetyThreshold() secondExtensionViewThreshold := originalParentState.NextEpochSetup.FinalView + 2*DefaultEpochExtensionViewCount - s.params.EpochCommitSafetyThreshold() thirdExtensionViewThreshold := originalParentState.NextEpochSetup.FinalView + 3*DefaultEpochExtensionViewCount - s.params.EpochCommitSafetyThreshold() - currentEpochLastView := originalParentState.CurrentEpochSetup.FinalView parentProtocolState := originalParentState.Copy() candidateView := originalParentState.CurrentEpochSetup.FirstView + 1 @@ -432,28 +429,26 @@ func (s *EpochFallbackStateMachineSuite) TestEpochFallbackStateMachineInjectsMul stateMachine, err := NewFallbackStateMachine(s.params, candidateView, parentProtocolState.Copy()) require.NoError(s.T(), err) - var ( - nextEpochSetup *flow.EpochSetup - nextEpochCommit *flow.EpochCommit - ) + previousEpochSetup, previousEpochCommit := parentProtocolState.PreviousEpochSetup, parentProtocolState.PreviousEpochCommit + currentEpochSetup, currentEpochCommit := parentProtocolState.CurrentEpochSetup, parentProtocolState.CurrentEpochCommit + nextEpochSetup, nextEpochCommit := parentProtocolState.NextEpochSetup, parentProtocolState.NextEpochCommit if candidateView > parentProtocolState.CurrentEpochFinalView() { require.NoError(s.T(), stateMachine.TransitionToNextEpoch()) + + // after we have transitioned to the next epoch, we need to update the current epoch + // for the next iteration we can use parent protocol state again + previousEpochSetup, previousEpochCommit = currentEpochSetup, currentEpochCommit + currentEpochSetup, currentEpochCommit = parentProtocolState.NextEpochSetup, parentProtocolState.NextEpochCommit + nextEpochSetup, nextEpochCommit = nil, nil } updatedState, _, _ := stateMachine.Build() - if candidateView < currentEpochLastView { - // as the next epoch has been committed, we need to respect that in current epoch - // when we transition to the next epoch we will pass nil instead. - nextEpochSetup = parentProtocolState.NextEpochSetup - nextEpochCommit = parentProtocolState.NextEpochCommit - } - parentProtocolState, err = flow.NewRichProtocolStateEntry(updatedState, - parentProtocolState.PreviousEpochSetup, - parentProtocolState.PreviousEpochCommit, - parentProtocolState.CurrentEpochSetup, - parentProtocolState.CurrentEpochCommit, + previousEpochSetup, + previousEpochCommit, + currentEpochSetup, + currentEpochCommit, nextEpochSetup, nextEpochCommit) @@ -483,11 +478,11 @@ func (s *EpochFallbackStateMachineSuite) TestEpochFallbackStateMachineInjectsMul evolveStateToView(data.TargetView) expectedState := &flow.ProtocolStateEntry{ - PreviousEpoch: originalParentState.PreviousEpoch, + PreviousEpoch: originalParentState.CurrentEpoch.Copy(), CurrentEpoch: flow.EpochStateContainer{ - SetupID: originalParentState.CurrentEpoch.SetupID, - CommitID: originalParentState.CurrentEpoch.CommitID, - ActiveIdentities: originalParentState.CurrentEpoch.ActiveIdentities, + SetupID: originalParentState.NextEpoch.SetupID, + CommitID: originalParentState.NextEpoch.CommitID, + ActiveIdentities: originalParentState.NextEpoch.ActiveIdentities, EpochExtensions: data.ExpectedExtensions, }, NextEpoch: nil, From 9ec93b95673658f813937881de0ab25d16143c04 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 13 May 2024 15:32:33 +0300 Subject: [PATCH 4/8] Added TODO --- state/protocol/protocol_state/epochs/statemachine.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/state/protocol/protocol_state/epochs/statemachine.go b/state/protocol/protocol_state/epochs/statemachine.go index 8f5b73ab1a5..523f6f3ebf8 100644 --- a/state/protocol/protocol_state/epochs/statemachine.go +++ b/state/protocol/protocol_state/epochs/statemachine.go @@ -205,6 +205,11 @@ func (e *EpochStateMachine) EvolveState(sealedServiceEvents []flow.ServiceEvent) phase := parentProtocolState.EpochPhase() if phase == flow.EpochPhaseCommitted { activeSetup := parentProtocolState.CurrentEpochSetup + // TODO(efm-recovery): This logic needs to be updated when injection of EFM recovery service event is implemented. + // For now, we are only checking transition to next epoch when the next epoch is committed. + // There is no epoch transition logic out of EFM, but once we have it, the following logic + // will be incorrect as we need to check the epoch final view including the extensions but not + // only the final view of the setup. if e.activeStateMachine.View() > activeSetup.FinalView { err := e.activeStateMachine.TransitionToNextEpoch() if err != nil { From 9e576ad1a14ece691225290f5bc847f539ad8779 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 15 May 2024 13:30:36 +0300 Subject: [PATCH 5/8] Apply suggestions from code review Co-authored-by: Alexander Hentschel --- .../protocol_state/epochs/base_statemachine.go | 15 ++++++--------- .../epochs/fallback_statemachine_test.go | 5 ++--- .../protocol_state/epochs/statemachine.go | 2 +- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/state/protocol/protocol_state/epochs/base_statemachine.go b/state/protocol/protocol_state/epochs/base_statemachine.go index 5787751a9a7..f5f8c685b89 100644 --- a/state/protocol/protocol_state/epochs/base_statemachine.go +++ b/state/protocol/protocol_state/epochs/base_statemachine.go @@ -99,7 +99,7 @@ func (u *baseStateMachine) EjectIdentity(nodeID flow.Identifier) error { } // TransitionToNextEpoch updates the notion of 'current epoch', 'previous' and 'next epoch' in the protocol -// state. An epoch transition is only allowed when: +// state. An epoch transition is only allowed when _all_ of the following conditions are satisfied: // - next epoch has been set up, // - next epoch has been committed, // - invalid state transition has not been attempted (this is ensured by constructor), @@ -107,17 +107,14 @@ func (u *baseStateMachine) EjectIdentity(nodeID flow.Identifier) error { // No errors are expected during normal operations. func (u *baseStateMachine) TransitionToNextEpoch() error { nextEpoch := u.state.NextEpoch - // Check if there is next epoch protocol state - if nextEpoch == nil { - return fmt.Errorf("protocol state has not been setup yet") - } - // Check if there is a commit event for next epoch - if nextEpoch.CommitID == flow.ZeroID { - return fmt.Errorf("protocol state has not been committed yet") + if nextEpoch == nil { // nextEpoch ≠ nil if and only if next epoch was already set up (on the happy path) + return fmt.Errorf("protocol state for next epoch has not yet been setup") } + if nextEpoch.CommitID == flow.ZeroID { // nextEpoch.CommitID ≠ flow.ZeroID if and only if next epoch was already committed (on the happy path) + return fmt.Errorf("protocol state for next epoch has not yet been committed") // Check if we are at the next epoch, only then a transition is allowed if u.view < u.parentState.NextEpochSetup.FirstView { - return fmt.Errorf("protocol state transition is only allowed when enterring next epoch") + return fmt.Errorf("epoch transition is only allowed when entering next epoch") } u.state = &flow.ProtocolStateEntry{ PreviousEpoch: &u.state.CurrentEpoch, diff --git a/state/protocol/protocol_state/epochs/fallback_statemachine_test.go b/state/protocol/protocol_state/epochs/fallback_statemachine_test.go index e03d7c949a2..f60ee1b4d6e 100644 --- a/state/protocol/protocol_state/epochs/fallback_statemachine_test.go +++ b/state/protocol/protocol_state/epochs/fallback_statemachine_test.go @@ -126,6 +126,7 @@ func (s *EpochFallbackStateMachineSuite) TestTransitionToNextEpochNotAllowed() { protocolState := unittest.EpochStateFixture(unittest.WithNextEpochProtocolState(), func(entry *flow.RichProtocolStateEntry) { entry.NextEpoch.CommitID = flow.ZeroID entry.NextEpochCommit = nil + entry.NextEpochIdentityTable = nil }) candidate := unittest.BlockHeaderFixture( unittest.HeaderWithView(protocolState.CurrentEpochSetup.FinalView + 1)) @@ -387,7 +388,7 @@ func (s *EpochFallbackStateMachineSuite) TestEpochFallbackStateMachineInjectsMul // TestEpochFallbackStateMachineInjectsMultipleExtensions_NextEpochCommitted tests that the state machine injects multiple extensions // as it reaches the safety threshold of the current epoch and the extensions themselves. // In this test we are simulating the scenario where the current epoch enters fallback mode when the next epoch has been committed. -// It is expected that it will transition into the next epoch (since it was committed) +// It is expected that it will transition into the next epoch (since it was committed), // then reach the safety threshold and add the extension to the next epoch, which at that point will be considered 'current'. func (s *EpochFallbackStateMachineSuite) TestEpochFallbackStateMachineInjectsMultipleExtensions_NextEpochCommitted() { originalParentState := s.parentProtocolState.Copy() @@ -443,7 +444,6 @@ func (s *EpochFallbackStateMachineSuite) TestEpochFallbackStateMachineInjectsMul } updatedState, _, _ := stateMachine.Build() - parentProtocolState, err = flow.NewRichProtocolStateEntry(updatedState, previousEpochSetup, previousEpochCommit, @@ -451,7 +451,6 @@ func (s *EpochFallbackStateMachineSuite) TestEpochFallbackStateMachineInjectsMul currentEpochCommit, nextEpochSetup, nextEpochCommit) - require.NoError(s.T(), err) } } diff --git a/state/protocol/protocol_state/epochs/statemachine.go b/state/protocol/protocol_state/epochs/statemachine.go index 523f6f3ebf8..c26051132ed 100644 --- a/state/protocol/protocol_state/epochs/statemachine.go +++ b/state/protocol/protocol_state/epochs/statemachine.go @@ -207,7 +207,7 @@ func (e *EpochStateMachine) EvolveState(sealedServiceEvents []flow.ServiceEvent) activeSetup := parentProtocolState.CurrentEpochSetup // TODO(efm-recovery): This logic needs to be updated when injection of EFM recovery service event is implemented. // For now, we are only checking transition to next epoch when the next epoch is committed. - // There is no epoch transition logic out of EFM, but once we have it, the following logic + // There is no logic yet to leave EFM, but once we have it, the following logic // will be incorrect as we need to check the epoch final view including the extensions but not // only the final view of the setup. if e.activeStateMachine.View() > activeSetup.FinalView { From 4d849817b8f34139e77dbf3f3c80caa5ca49e402 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 15 May 2024 13:51:06 +0300 Subject: [PATCH 6/8] Apply suggestions from code review Co-authored-by: Alexander Hentschel --- state/protocol/protocol_state/epochs/base_statemachine.go | 1 - 1 file changed, 1 deletion(-) diff --git a/state/protocol/protocol_state/epochs/base_statemachine.go b/state/protocol/protocol_state/epochs/base_statemachine.go index f5f8c685b89..5fd7338215e 100644 --- a/state/protocol/protocol_state/epochs/base_statemachine.go +++ b/state/protocol/protocol_state/epochs/base_statemachine.go @@ -102,7 +102,6 @@ func (u *baseStateMachine) EjectIdentity(nodeID flow.Identifier) error { // state. An epoch transition is only allowed when _all_ of the following conditions are satisfied: // - next epoch has been set up, // - next epoch has been committed, -// - invalid state transition has not been attempted (this is ensured by constructor), // - candidate block is in the next epoch. // No errors are expected during normal operations. func (u *baseStateMachine) TransitionToNextEpoch() error { From 79f85a054380c6fbdac473165a7caa0f2088623d Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 15 May 2024 13:51:55 +0300 Subject: [PATCH 7/8] Update state/protocol/protocol_state/epochs/fallback_statemachine_test.go Co-authored-by: Alexander Hentschel --- .../epochs/fallback_statemachine_test.go | 30 +++++-------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/state/protocol/protocol_state/epochs/fallback_statemachine_test.go b/state/protocol/protocol_state/epochs/fallback_statemachine_test.go index f60ee1b4d6e..76d40d58267 100644 --- a/state/protocol/protocol_state/epochs/fallback_statemachine_test.go +++ b/state/protocol/protocol_state/epochs/fallback_statemachine_test.go @@ -77,28 +77,14 @@ func (s *EpochFallbackStateMachineSuite) TestTransitionToNextEpoch() { InvalidEpochTransitionAttempted: true, } - s.Run("happy-path", func() { - parentProtocolState := s.parentProtocolState.Copy() - parentProtocolState.InvalidEpochTransitionAttempted = false - - var err error - // since the candidate block is from next epoch, HappyPathStateMachine should transition to next epoch - s.stateMachine, err = NewFallbackStateMachine(s.params, candidate.View, parentProtocolState.Copy()) - require.NoError(s.T(), err) - err = s.stateMachine.TransitionToNextEpoch() - require.NoError(s.T(), err) - updatedState, stateID, hasChanges := s.stateMachine.Build() - require.True(s.T(), hasChanges) - require.NotEqual(s.T(), parentProtocolState.ID(), updatedState.ID()) - require.Equal(s.T(), updatedState.ID(), stateID) - require.Equal(s.T(), updatedState, expectedState, "state should be equal to expected one") - }) - s.Run("epoch-fallback-mode", func() { + // Irrespective of whether the parent state is in EFM, the FallbackStateMachine should always set + // `InvalidEpochTransitionAttempted` to true and transition the next epoch, because the candidate block + // belongs to the next epoch. + var err error + for _, parentAlreadyInEFM := range []bool{true, false} { parentProtocolState := s.parentProtocolState.Copy() - parentProtocolState.InvalidEpochTransitionAttempted = true // just to be explicit + parentProtocolState.InvalidEpochTransitionAttempted = parentAlreadyInEFM - var err error - // since the candidate block is from next epoch, HappyPathStateMachine should transition to next epoch s.stateMachine, err = NewFallbackStateMachine(s.params, candidate.View, parentProtocolState.Copy()) require.NoError(s.T(), err) err = s.stateMachine.TransitionToNextEpoch() @@ -107,8 +93,8 @@ func (s *EpochFallbackStateMachineSuite) TestTransitionToNextEpoch() { require.True(s.T(), hasChanges) require.NotEqual(s.T(), parentProtocolState.ID(), updatedState.ID()) require.Equal(s.T(), updatedState.ID(), stateID) - require.Equal(s.T(), updatedState, expectedState, "state should be equal to expected one") - }) + require.Equal(s.T(), updatedState, expectedState, "FallbackStateMachine produced unexpected Protocol State") + } } // TestTransitionToNextEpochNotAllowed tests different scenarios where transition to next epoch is not allowed. From ab926545643aae20354be2ee4978cf1afaaaaea5 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 15 May 2024 13:52:10 +0300 Subject: [PATCH 8/8] Linted --- state/protocol/protocol_state/epochs/base_statemachine.go | 2 +- .../protocol_state/epochs/fallback_statemachine_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/state/protocol/protocol_state/epochs/base_statemachine.go b/state/protocol/protocol_state/epochs/base_statemachine.go index f5f8c685b89..171cc5da239 100644 --- a/state/protocol/protocol_state/epochs/base_statemachine.go +++ b/state/protocol/protocol_state/epochs/base_statemachine.go @@ -102,7 +102,6 @@ func (u *baseStateMachine) EjectIdentity(nodeID flow.Identifier) error { // state. An epoch transition is only allowed when _all_ of the following conditions are satisfied: // - next epoch has been set up, // - next epoch has been committed, -// - invalid state transition has not been attempted (this is ensured by constructor), // - candidate block is in the next epoch. // No errors are expected during normal operations. func (u *baseStateMachine) TransitionToNextEpoch() error { @@ -112,6 +111,7 @@ func (u *baseStateMachine) TransitionToNextEpoch() error { } if nextEpoch.CommitID == flow.ZeroID { // nextEpoch.CommitID ≠ flow.ZeroID if and only if next epoch was already committed (on the happy path) return fmt.Errorf("protocol state for next epoch has not yet been committed") + } // Check if we are at the next epoch, only then a transition is allowed if u.view < u.parentState.NextEpochSetup.FirstView { return fmt.Errorf("epoch transition is only allowed when entering next epoch") diff --git a/state/protocol/protocol_state/epochs/fallback_statemachine_test.go b/state/protocol/protocol_state/epochs/fallback_statemachine_test.go index f60ee1b4d6e..aa5c47dc75d 100644 --- a/state/protocol/protocol_state/epochs/fallback_statemachine_test.go +++ b/state/protocol/protocol_state/epochs/fallback_statemachine_test.go @@ -126,7 +126,7 @@ func (s *EpochFallbackStateMachineSuite) TestTransitionToNextEpochNotAllowed() { protocolState := unittest.EpochStateFixture(unittest.WithNextEpochProtocolState(), func(entry *flow.RichProtocolStateEntry) { entry.NextEpoch.CommitID = flow.ZeroID entry.NextEpochCommit = nil - entry.NextEpochIdentityTable = nil + entry.NextEpochIdentityTable = nil }) candidate := unittest.BlockHeaderFixture( unittest.HeaderWithView(protocolState.CurrentEpochSetup.FinalView + 1))