[Dynamic Protocol State] State machine handles invalid state transition

state/protocol/protocol_state/mutator_test.go

@@ -89,16 +89,9 @@ func (s *StateMutatorSuite) TestHappyPathHasChanges() {
 func (s *StateMutatorSuite) TestApplyServiceEvents_InvalidEpochSetup() {
 	s.Run("invalid-epoch-setup", func() {
 		parentState := unittest.ProtocolStateFixture()
-		rootSetup := parentState.CurrentEpochSetup
-
 		s.stateMachine.On("ParentState").Return(parentState)
 
-		epochSetup := unittest.EpochSetupFixture(
-			unittest.WithParticipants(rootSetup.Participants),
-			unittest.SetupWithCounter(rootSetup.Counter+1),
-			unittest.WithFinalView(rootSetup.FinalView+1000),
-			unittest.WithFirstView(rootSetup.FinalView+1),
-		)
+		epochSetup := unittest.EpochSetupFixture()
 		result := unittest.ExecutionResultFixture(func(result *flow.ExecutionResult) {
 			result.ServiceEvents = []flow.ServiceEvent{epochSetup.ServiceEvent()}
 		})
@@ -116,16 +109,9 @@ func (s *StateMutatorSuite) TestApplyServiceEvents_InvalidEpochSetup() {
 	})
 	s.Run("process-epoch-setup-exception", func() {
 		parentState := unittest.ProtocolStateFixture()
-		rootSetup := parentState.CurrentEpochSetup
-
 		s.stateMachine.On("ParentState").Return(parentState)
 
-		epochSetup := unittest.EpochSetupFixture(
-			unittest.WithParticipants(rootSetup.Participants),
-			unittest.SetupWithCounter(rootSetup.Counter+1),
-			unittest.WithFinalView(rootSetup.FinalView+1000),
-			unittest.WithFirstView(rootSetup.FinalView+1),
-		)
+		epochSetup := unittest.EpochSetupFixture()
 		result := unittest.ExecutionResultFixture(func(result *flow.ExecutionResult) {
 			result.ServiceEvents = []flow.ServiceEvent{epochSetup.ServiceEvent()}
 		})
@@ -149,7 +135,6 @@ func (s *StateMutatorSuite) TestApplyServiceEvents_InvalidEpochSetup() {
 func (s *StateMutatorSuite) TestApplyServiceEvents_InvalidEpochCommit() {
 	s.Run("invalid-epoch-commit", func() {
 		parentState := unittest.ProtocolStateFixture()
-
 		s.stateMachine.On("ParentState").Return(parentState)
 
 		epochCommit := unittest.EpochCommitFixture()
@@ -170,7 +155,6 @@ func (s *StateMutatorSuite) TestApplyServiceEvents_InvalidEpochCommit() {
 	})
 	s.Run("process-epoch-commit-exception", func() {
 		parentState := unittest.ProtocolStateFixture()
-
 		s.stateMachine.On("ParentState").Return(parentState)
 
 		epochCommit := unittest.EpochCommitFixture()

state/protocol/protocol_state/updater.go

@@ -46,7 +46,12 @@ func newStateMachine(view uint64, parentState *flow.RichProtocolStateEntry) *sta
 
 // Build returns updated protocol state entry, state ID and a flag indicating if there were any changes.
 func (u *stateMachine) Build() (updatedState *flow.ProtocolStateEntry, stateID flow.Identifier, hasChanges bool) {
-	updatedState = u.state.Copy()
+	if u.state.InvalidStateTransitionAttempted {
+		updatedState = u.parentState.ProtocolStateEntry.Copy()
+		updatedState.InvalidStateTransitionAttempted = true
+	} else {
+		updatedState = u.state.Copy()
+	}
 	stateID = updatedState.ID()
 	hasChanges = stateID != u.parentState.ID()
 	return
@@ -64,6 +69,23 @@ func (u *stateMachine) ProcessEpochSetup(epochSetup *flow.EpochSetup) error {
 	if u.state.InvalidStateTransitionAttempted {
 		return nil // won't process new events if we are in epoch fallback mode.
 	}
+
+	if err := u.processEpochSetup(epochSetup); err != nil {
+		u.SetInvalidStateTransitionAttempted()
+		return err
+	}
+	return nil
+}
+
+// processEpochSetup updates the protocol state with data from the epoch setup event.
+// Observing an epoch setup event also affects the identity table for current epoch:
+//   - it transitions the protocol state from Staking to Epoch Setup phase
+//   - we stop returning identities from previous+current epochs and instead returning identities from current+next epochs.
+//
+// As a result of this operation protocol state for the next epoch will be created.
+// Expected errors during normal operations:
+// - `protocol.InvalidServiceEventError` if the service event is invalid or is not a valid state transition for the current protocol state
+func (u *stateMachine) processEpochSetup(epochSetup *flow.EpochSetup) error {
 	err := protocol.IsValidExtendingEpochSetup(epochSetup, u.parentState.CurrentEpochSetup, u.parentState.EpochStatus())
 	if err != nil {
 		return fmt.Errorf("invalid epoch setup event: %w", err)
@@ -148,6 +170,20 @@ func (u *stateMachine) ProcessEpochCommit(epochCommit *flow.EpochCommit) error {
 	if u.state.InvalidStateTransitionAttempted {
 		return nil // won't process new events if we are going to enter epoch fallback mode
 	}
+	if err := u.processEpochCommit(epochCommit); err != nil {
+		u.SetInvalidStateTransitionAttempted()
+		return err
+	}
+	return nil
+}
+
+// processEpochCommit updates current protocol state with data from epoch commit event.
+// Observing an epoch setup commit, transitions protocol state from setup to commit phase, at this point we have
+// finished construction of the next epoch.
+// As a result of this operation protocol state for next epoch will be committed.
+// Expected errors during normal operations:
+// - `protocol.InvalidServiceEventError` if the service event is invalid or is not a valid state transition for the current protocol state
+func (u *stateMachine) processEpochCommit(epochCommit *flow.EpochCommit) error {
 	if u.state.NextEpoch == nil {
 		return protocol.NewInvalidServiceEventErrorf("protocol state has been setup yet")
 	}

state/protocol/protocol_state/updater_test.go

@@ -136,6 +136,7 @@ func (s *UpdaterSuite) TestSetInvalidStateTransitionAttempted() {
 // correctly behaves when invariants are violated.
 func (s *UpdaterSuite) TestProcessEpochCommit() {
 	s.Run("invalid counter", func() {
+		s.updater = newStateMachine(s.candidate.View, s.parentProtocolState)
 		commit := unittest.EpochCommitFixture(func(commit *flow.EpochCommit) {
 			commit.Counter = s.parentProtocolState.CurrentEpochSetup.Counter + 10 // set invalid counter for next epoch
 		})
@@ -144,6 +145,7 @@ func (s *UpdaterSuite) TestProcessEpochCommit() {
 		require.True(s.T(), protocol.IsInvalidServiceEventError(err))
 	})
 	s.Run("no next epoch protocol state", func() {
+		s.updater = newStateMachine(s.candidate.View, s.parentProtocolState)
 		commit := unittest.EpochCommitFixture(func(commit *flow.EpochCommit) {
 			commit.Counter = s.parentProtocolState.CurrentEpochSetup.Counter + 1
 		})
@@ -152,41 +154,41 @@ func (s *UpdaterSuite) TestProcessEpochCommit() {
 		require.True(s.T(), protocol.IsInvalidServiceEventError(err))
 	})
 	s.Run("invalid state transition attempted", func() {
-		updater := newStateMachine(s.candidate.View, s.parentProtocolState)
+		s.updater = newStateMachine(s.candidate.View, s.parentProtocolState)
 		setup := unittest.EpochSetupFixture(func(setup *flow.EpochSetup) {
 			setup.Counter = s.parentProtocolState.CurrentEpochSetup.Counter + 1
 			setup.FirstView = s.parentProtocolState.CurrentEpochSetup.FinalView + 1
 			setup.FinalView = s.parentProtocolState.CurrentEpochSetup.FinalView + 1000
 		})
 		// processing setup event results in creating next epoch protocol state
-		err := updater.ProcessEpochSetup(setup)
+		err := s.updater.ProcessEpochSetup(setup)
 		require.NoError(s.T(), err)
 
-		updater.SetInvalidStateTransitionAttempted()
+		s.updater.SetInvalidStateTransitionAttempted()
 
 		commit := unittest.EpochCommitFixture(func(commit *flow.EpochCommit) {
 			commit.Counter = s.parentProtocolState.CurrentEpochSetup.Counter + 1
 		})
 
 		// processing epoch commit should be no-op since we have observed an invalid state transition
-		err = updater.ProcessEpochCommit(commit)
+		err = s.updater.ProcessEpochCommit(commit)
 		require.NoError(s.T(), err)
 
-		newState, _, _ := updater.Build()
-		require.Equal(s.T(), flow.ZeroID, newState.NextEpoch.CommitID, "operation must be no-op")
+		newState, _, _ := s.updater.Build()
+		require.Nil(s.T(), newState.NextEpoch, "operation must be no-op")
 	})
-	s.Run("happy path processing", func() {
-		updater := newStateMachine(s.candidate.View, s.parentProtocolState)
+	s.Run("conflicting epoch commit", func() {
+		s.updater = newStateMachine(s.candidate.View, s.parentProtocolState)
 		setup := unittest.EpochSetupFixture(
 			unittest.SetupWithCounter(s.parentProtocolState.CurrentEpochSetup.Counter+1),
 			unittest.WithFirstView(s.parentProtocolState.CurrentEpochSetup.FinalView+1),
 			unittest.WithFinalView(s.parentProtocolState.CurrentEpochSetup.FinalView+1000),
 		)
 		// processing setup event results in creating next epoch protocol state
-		err := updater.ProcessEpochSetup(setup)
+		err := s.updater.ProcessEpochSetup(setup)
 		require.NoError(s.T(), err)
 
-		updatedState, _, _ := updater.Build()
+		updatedState, _, _ := s.updater.Build()
 
 		parentState, err := flow.NewRichProtocolStateEntry(updatedState,
 			s.parentProtocolState.PreviousEpochSetup,
@@ -197,22 +199,57 @@ func (s *UpdaterSuite) TestProcessEpochCommit() {
 			nil,
 		)
 		require.NoError(s.T(), err)
-		updater = newStateMachine(s.candidate.View+1, parentState)
+		s.updater = newStateMachine(s.candidate.View+1, parentState)
 		commit := unittest.EpochCommitFixture(
 			unittest.CommitWithCounter(setup.Counter),
 			unittest.WithDKGFromParticipants(setup.Participants),
 		)
 
-		err = updater.ProcessEpochCommit(commit)
+		err = s.updater.ProcessEpochCommit(commit)
 		require.NoError(s.T(), err)
 
 		// processing another epoch commit has to be an error since we have already processed one
-		err = updater.ProcessEpochCommit(commit)
+		err = s.updater.ProcessEpochCommit(commit)
 		require.Error(s.T(), err)
 		require.True(s.T(), protocol.IsInvalidServiceEventError(err))
 
-		newState, _, _ := updater.Build()
-		require.Equal(s.T(), commit.ID(), newState.NextEpoch.CommitID, "next epoch must be committed")
+		newState, _, _ := s.updater.Build()
+		require.Equal(s.T(), flow.ZeroID, newState.NextEpoch.CommitID, "next epoch shouldn't be committed, "+
+			"since we have observed invalid state transition")
+	})
+	s.Run("happy path processing", func() {
+		s.updater = newStateMachine(s.candidate.View, s.parentProtocolState)
+		setup := unittest.EpochSetupFixture(
+			unittest.SetupWithCounter(s.parentProtocolState.CurrentEpochSetup.Counter+1),
+			unittest.WithFirstView(s.parentProtocolState.CurrentEpochSetup.FinalView+1),
+			unittest.WithFinalView(s.parentProtocolState.CurrentEpochSetup.FinalView+1000),
+		)
+		// processing setup event results in creating next epoch protocol state
+		err := s.updater.ProcessEpochSetup(setup)
+		require.NoError(s.T(), err)
+
+		updatedState, _, _ := s.updater.Build()
+
+		parentState, err := flow.NewRichProtocolStateEntry(updatedState,
+			s.parentProtocolState.PreviousEpochSetup,
+			s.parentProtocolState.PreviousEpochCommit,
+			s.parentProtocolState.CurrentEpochSetup,
+			s.parentProtocolState.CurrentEpochCommit,
+			setup,
+			nil,
+		)
+		require.NoError(s.T(), err)
+		s.updater = newStateMachine(s.candidate.View+1, parentState)
+		commit := unittest.EpochCommitFixture(
+			unittest.CommitWithCounter(setup.Counter),
+			unittest.WithDKGFromParticipants(setup.Participants),
+		)
+
+		err = s.updater.ProcessEpochCommit(commit)
+		require.NoError(s.T(), err)
+
+		newState, _, _ := s.updater.Build()
+		require.Equal(s.T(), commit.ID(), newState.NextEpoch.CommitID, "next epoch should be committed")
 	})
 }