diff --git a/internal/bft/viewchanger.go b/internal/bft/viewchanger.go index ce53c9af..2b1e2539 100644 --- a/internal/bft/viewchanger.go +++ b/internal/bft/viewchanger.go @@ -1377,9 +1377,9 @@ func (v *ViewChanger) Decide(proposal types.Proposal, signatures []types.Signatu } } -// Complain panics when a view change is requested +// Complain is called by the view of the in flight proposal, which then stops, and so it is only logged func (v *ViewChanger) Complain(viewNum uint64, stopView bool) { - v.Logger.Panicf("Node %d has complained while in the view for the in flight proposal", v.SelfID) + v.Logger.Warnf("Node %d has complained while in the view for the in flight proposal", v.SelfID) } // Sync calls the synchronizer and informs the view changer of the sync diff --git a/internal/bft/viewchanger_test.go b/internal/bft/viewchanger_test.go index f3521a13..0918d43f 100644 --- a/internal/bft/viewchanger_test.go +++ b/internal/bft/viewchanger_test.go @@ -2115,6 +2115,152 @@ func (h *recordedHistogram) Observe(value float64) { h.recorder.values[h.name] = append(h.recorder.values[h.name], value) } +// TestInFlightViewMessageFromItselfWithWrongView makes sure that the view of the in flight proposal does not panic +// when it gets a message with a wrong view from its leader, which is the node itself. +func TestInFlightViewMessageFromItselfWithWrongView(t *testing.T) { + comm := &mocks.CommMock{} + msgChan := make(chan *protos.Message, 10) + comm.On("BroadcastConsensus", mock.Anything).Run(func(args mock.Arguments) { + msgChan <- args.Get(0).(*protos.Message) + }) + basicLog, err := zap.NewDevelopment() + assert.NoError(t, err) + panicked := make(chan string, 1) + log := basicLog.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() + signer := &mocks.SignerMock{} + signer.On("Sign", mock.Anything).Return([]byte{1, 2, 3}) + signed := make(chan struct{}, 1) + signer.On("SignProposal", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + signed <- struct{}{} + }).Return(&types.Signature{ + ID: 1, + Value: []byte{4}, + }) + verifier := &mocks.VerifierMock{} + verifier.On("VerifySignature", mock.Anything).Return(nil) + verifier.On("VerifyConsenterSig", mock.Anything, mock.Anything).Return(nil, nil) + verifier.On("RequestsFromProposal", mock.Anything).Return(nil) + controller := &mocks.ViewController{} + controller.On("ViewChanged", mock.Anything, mock.Anything).Return(nil).Maybe() + controller.On("AbortView", mock.Anything) + reqTimer := &mocks.RequestsTimer{} + reqTimer.On("StopTimers") + reqTimer.On("RestartTimers") + checkpoint := types.Checkpoint{} + checkpoint.Set(lastDecision, lastDecisionSignatures) + synchronizer := &mocks.Synchronizer{} + synced := make(chan struct{}, 1) + synchronizer.On("Sync").Run(func(args mock.Arguments) { + select { + case synced <- struct{}{}: + default: + } + }).Return() + + sched := make(chan time.Time) + vc := &bft.ViewChanger{ + SelfID: 1, + N: 4, + NodesList: []uint64{0, 1, 2, 3}, + Comm: comm, + Logger: log, + Verifier: verifier, + Controller: controller, + Signer: signer, + RequestsTimer: reqTimer, + Ticker: sched, + InFlight: &bft.InFlightData{}, + Checkpoint: &checkpoint, + ViewSequences: &atomic.Value{}, + State: &bft.StateRecorder{}, + InMsqQSize: int(types.DefaultConfig.IncomingMessageBufferSize), + Synchronizer: synchronizer, + MetricsBlacklist: api.NewMetricsBlacklist(&disabled.Provider{}), + MetricsView: api.NewMetricsView(&disabled.Provider{}), + } + + vc.Start(0) + + vc.HandleMessage(2, viewChangeMsg) + vc.HandleMessage(3, viewChangeMsg) + m := <-msgChan + assert.NotNil(t, m.GetViewChange()) + + inFlightProposal := types.Proposal{ + Payload: []byte{1}, + Header: []byte{2}, + Metadata: bft.MarshalOrPanic(&protos.ViewMetadata{ + LatestSequence: 2, + ViewId: 0, + }), + VerificationSequence: 1, + } + viewData := proto.Clone(vd).(*protos.ViewData) + viewData.InFlightProposal = &protos.Proposal{ + Payload: inFlightProposal.Payload, + Header: inFlightProposal.Header, + Metadata: inFlightProposal.Metadata, + VerificationSequence: uint64(inFlightProposal.VerificationSequence), + } + viewData.InFlightPrepared = true + msg := proto.Clone(viewDataMsg1).(*protos.Message) + msg.GetViewData().RawViewData = bft.MarshalOrPanic(viewData) + vc.HandleMessage(0, msg) + msg2 := proto.Clone(msg).(*protos.Message) + msg2.GetViewData().Signer = 2 + vc.HandleMessage(2, msg2) + m = <-msgChan + assert.NotNil(t, m.GetNewView()) + + // the view of the in flight proposal starts after two ticks + <-signed + sched <- time.Now() + sched <- time.Now() + assert.Eventually(t, func() bool { + select { + case m := <-msgChan: + return m.GetCommit() != nil + default: + return false + } + }, 3*time.Second, 10*time.Millisecond, "the view of the in flight proposal did not start") + + // a commit from a newer view, as if sent by this node, which is the leader of the view of the in flight proposal + vc.HandleViewMessage(1, &protos.Message{ + Content: &protos.Message_Commit{ + Commit: &protos.Commit{ + View: 1, + Seq: 2, + Digest: inFlightProposal.Digest(), + Signature: &protos.Signature{ + Signer: 1, + Value: []byte{4}, + }, + }, + }, + }) + + select { + case message := <-panicked: + assert.Fail(t, "the view of the in flight proposal panicked", message) + return // the view changer is gone, so it cannot be stopped + case <-synced: + // the view of the in flight proposal stopped and asked to sync, since the message is from a newer view + case <-time.After(3 * time.Second): + assert.Fail(t, "the view of the in flight proposal neither stopped nor panicked") + } + + vc.Stop() +} + func TestCommitWhileHavingInFlight(t *testing.T) { comm := &mocks.CommMock{} msgChan := make(chan *protos.Message, 1)