Ignore a NewView message for a view the node already installed

internal/bft/viewchanger.go

@@ -92,6 +92,7 @@ type ViewChanger struct {
 	checkTimeout        bool
 	backOffFactor       uint64
 	syncedForNewView    bool
+	acceptedNewView     bool // whether a new view message was already accepted for currView
 
 	// Runtime
 	MetricsViewChange         *api.MetricsViewChange
@@ -321,6 +322,10 @@ func (v *ViewChanger) processMsg(sender uint64, m *protos.Message) {
 			v.Logger.Warnf("Node %d got newView message %v from %d, expected sender to be %d the next leader", v.SelfID, MsgToString(m), sender, leader)
 			return
 		}
+		if v.acceptedNewView {
+			v.Logger.Debugf("Node %d got newView message %v from %d, but it is already in view %d, ignoring it", v.SelfID, MsgToString(m), sender, v.currView)
+			return
+		}
 		v.processNewViewMsg(nv)
 	}
 }
@@ -352,6 +357,7 @@ func (v *ViewChanger) informNewView(view uint64) {
 	v.checkTimeout = false
 	v.backOffFactor = 1 // reset
 	v.syncedForNewView = false
+	v.acceptedNewView = true // the node is already in this view, so a new view message for it is not needed
 	v.RequestsTimer.RestartTimers()
 }
 
@@ -424,6 +430,7 @@ func (v *ViewChanger) processViewChangeMsg(restore bool) {
 	v.viewChangeMsgs.clear(v.N)
 	v.viewDataMsgs.clear(v.N)  // clear because currView changed
 	v.syncedForNewView = false // clear because currView changed
+	v.acceptedNewView = false  // clear because currView changed
 	msg := v.prepareViewDataMsg()
 	leader := v.getLeader()
 	if leader == v.SelfID {
@@ -1192,6 +1199,7 @@ func (v *ViewChanger) processNewViewMsg(msg *protos.NewView) {
 	default:
 	}
 
+	v.acceptedNewView = true
 	v.realView = v.currView
 	v.MetricsViewChange.RealView.Set(float64(v.realView))
 	v.nvs.clear()

internal/bft/viewchanger_test.go

@@ -2563,6 +2563,95 @@ func TestNewViewWithoutQuorumSyncs(t *testing.T) {
 	}
 }
 
+// TestNewViewOfCurrentViewIsIgnored makes sure that once a node accepted a new view message,
+// another new view message for the same view does not install the view again.
+func TestNewViewOfCurrentViewIsIgnored(t *testing.T) {
+	for _, test := range []struct {
+		description string
+		viewData    func() *protos.ViewData
+	}{
+		{
+			description: "without an in flight proposal",
+			viewData: func() *protos.ViewData {
+				return proto.Clone(vd).(*protos.ViewData)
+			},
+		},
+		{
+			// the node already decided the in flight proposal, and then decides the next sequence
+			// before it gets the new view message again
+			description: "with an in flight proposal",
+			viewData:    preparedInFlightViewData,
+		},
+	} {
+		t.Run(test.description, func(t *testing.T) {
+			verifier := &mocks.VerifierMock{}
+			verifier.On("VerifySignature", mock.Anything).Return(nil)
+			verifier.On("VerifyConsenterSig", mock.Anything, mock.Anything).Return(nil, nil)
+
+			vc, app, viewChangedChan := newViewChangerWithInFlight(t, verifier)
+			panicked := make(chan string, 1)
+			baseLogger := vc.Logger.(*zap.SugaredLogger).Desugar()
+			vc.Logger = baseLogger.WithOptions(zap.WithPanicHook(zapcore.WriteThenGoexit), zap.Hooks(func(entry zapcore.Entry) error {
+				if entry.Level == zapcore.PanicLevel {
+					select {
+					case panicked <- entry.Message:
+					default:
+					}
+				}
+				return nil
+			})).Sugar()
+
+			vc.Start(1)
+
+			raw := bft.MarshalOrPanic(test.viewData())
+			msg := &protos.Message{
+				Content: &protos.Message_NewView{
+					NewView: &protos.NewView{
+						SignedViewData: []*protos.SignedViewData{
+							{RawViewData: raw, Signer: 0},
+							{RawViewData: raw, Signer: 1},
+							{RawViewData: raw, Signer: 2},
+						},
+					},
+				},
+			}
+
+			vc.HandleMessage(1, msg)
+			select {
+			case view := <-viewChangedChan:
+				assert.Equal(t, uint64(1), view)
+			case <-time.After(3 * time.Second):
+				assert.Fail(t, "the view was not changed by the new view message")
+			}
+
+			// the node makes progress in the new view
+			vc.Checkpoint.Set(types.Proposal{
+				Payload: []byte{3},
+				Header:  []byte{4},
+				Metadata: bft.MarshalOrPanic(&protos.ViewMetadata{
+					LatestSequence: 3,
+					ViewId:         1,
+				}),
+				VerificationSequence: 1,
+			}, lastDecisionSignatures)
+
+			vc.HandleMessage(1, msg)
+			select {
+			case <-viewChangedChan:
+				assert.Fail(t, "the view was changed again by a new view message for the current view")
+			case message := <-panicked:
+				assert.Fail(t, "a new view message for the current view made the node panic", message)
+				return // the view changer is gone, so it cannot be stopped
+			case <-time.After(100 * time.Millisecond):
+			}
+
+			vc.Stop()
+
+			app.AssertNotCalled(t, "Deliver")
+		})
+	}
+}
+
 // The new leader is the node that collected the view data messages of the others,
 // so a newView message that does not carry the new leader's own signed view data
 // message must not be accepted, regardless of where in the list the entries sit.