diff --git a/internal/bft/viewchanger.go b/internal/bft/viewchanger.go index 07951151..ce53c9af 100644 --- a/internal/bft/viewchanger.go +++ b/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() diff --git a/internal/bft/viewchanger_test.go b/internal/bft/viewchanger_test.go index c0410a3d..f3521a13 100644 --- a/internal/bft/viewchanger_test.go +++ b/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.