From 442b36d1e5e36f56896cf870d5a5a74cdc9e10be Mon Sep 17 00:00:00 2001 From: Hagar Meir Date: Sun, 18 Oct 2026 21:21:09 +0300 Subject: [PATCH 1/2] Do not block when informing the view changer of a new view The controller informs the view changer of a new view after a sync, from its run loop. InformNewView sent on a channel with a buffer of one, so if the view changer had not processed the previous view yet, the controller's run loop blocked until it did. Keep only the highest pending view instead, so InformNewView never blocks and the view changer still moves to the latest view. TestInformViewChanger relied on the second call blocking to know the first was processed; it now waits for the timers to be restarted. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Hagar Meir --- internal/bft/viewchanger.go | 20 ++++++--- internal/bft/viewchanger_test.go | 70 +++++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 7 deletions(-) diff --git a/internal/bft/viewchanger.go b/internal/bft/viewchanger.go index 58d9c9da..d260c586 100644 --- a/internal/bft/viewchanger.go +++ b/internal/bft/viewchanger.go @@ -330,12 +330,22 @@ func (v *ViewChanger) processMsg(sender uint64, m *protos.Message) { } } -// InformNewView tells the view changer to advance to a new view number +// InformNewView tells the view changer to advance to a new view number. +// It never blocks: if the view changer did not process a previous view yet, only the highest of the two is kept. func (v *ViewChanger) InformNewView(view uint64) { - select { - case v.informChan <- view: - case <-v.stopChan: - return + for { + select { + case v.informChan <- view: + return + case <-v.stopChan: + return + default: + } + select { + case pending := <-v.informChan: + view = max(view, pending) + default: + } } } diff --git a/internal/bft/viewchanger_test.go b/internal/bft/viewchanger_test.go index 0918d43f..f575a75b 100644 --- a/internal/bft/viewchanger_test.go +++ b/internal/bft/viewchanger_test.go @@ -1578,7 +1578,10 @@ func TestInformViewChanger(t *testing.T) { }) reqTimer := &mocks.RequestsTimer{} reqTimer.On("StopTimers").Once() - reqTimer.On("RestartTimers") + informed := make(chan struct{}, 1) + reqTimer.On("RestartTimers").Run(func(args mock.Arguments) { + informed <- struct{}{} // the view changer restarts the timers once it is informed of a new view + }) basicLog, err := zap.NewDevelopment() assert.NoError(t, err) log := basicLog.Sugar() @@ -1600,7 +1603,7 @@ func TestInformViewChanger(t *testing.T) { info := uint64(2) vc.InformNewView(info) // increase the view number - vc.InformNewView(info) // make sure that inform happened (channel size is 1) + <-informed // make sure that inform happened vc.StartViewChange(2, true) msg := <-msgChan @@ -1614,6 +1617,69 @@ func TestInformViewChanger(t *testing.T) { reqTimer.AssertCalled(t, "RestartTimers") } +// TestInformNewViewDoesNotBlock makes sure that informing the view changer of a new view never blocks, +// even when it is not processing the views it was already informed of, and that it ends up in the latest one. +func TestInformNewViewDoesNotBlock(t *testing.T) { + comm := &mocks.CommMock{} + msgChan := make(chan *protos.Message) + comm.On("BroadcastConsensus", mock.Anything).Run(func(args mock.Arguments) { + msgChan <- args.Get(0).(*protos.Message) + }) + reqTimer := &mocks.RequestsTimer{} + reqTimer.On("StopTimers") + informed := make(chan struct{}, 1) + reqTimer.On("RestartTimers").Run(func(args mock.Arguments) { + informed <- struct{}{} // the view changer restarts the timers once it is informed of a new view + }) + basicLog, err := zap.NewDevelopment() + assert.NoError(t, err) + controller := &mocks.ViewController{} + controller.On("AbortView", mock.Anything) + + vc := &bft.ViewChanger{ + N: 4, + NodesList: []uint64{0, 1, 2, 3}, + Comm: comm, + RequestsTimer: reqTimer, + Ticker: make(chan time.Time), + Logger: basicLog.Sugar(), + Controller: controller, + InMsqQSize: 100, + } + // the view changer does not run until the controller started, so it processes none of the views it is informed of + vc.ControllerStartedWG.Add(1) + vc.Start(0) + + returned := make(chan struct{}) + go func() { + defer close(returned) + for view := uint64(1); view <= 3; view++ { + vc.InformNewView(view) + } + }() + select { + case <-returned: + case <-time.After(3 * time.Second): + assert.Fail(t, "informing the view changer of a new view blocked") + } + + vc.ControllerStartedWG.Done() + select { + case <-informed: + case <-time.After(3 * time.Second): + assert.Fail(t, "the view changer did not process the views it was informed of") + } + vc.StartViewChange(3, true) + select { + case msg := <-msgChan: + assert.Equal(t, uint64(4), msg.GetViewChange().GetNextView(), "the view changer is not in the latest view it was informed of") + case <-time.After(3 * time.Second): + assert.Fail(t, "the view changer did not start a view change") + } + + vc.Stop() +} + func TestRestoreViewChange(t *testing.T) { comm := &mocks.CommMock{} broadcastChan := make(chan *protos.Message) From 1799c743381dab8699c3de171a531a1b22ad1e23 Mon Sep 17 00:00:00 2001 From: Hagar Meir Date: Mon, 19 Oct 2026 10:25:14 +0300 Subject: [PATCH 2/2] Explain why InformNewView must not block Document that InformNewView is called from the controller's run loop, why keeping only the higher pending view loses nothing, and that the pending view is held only in informChan so concurrent calls are safe. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Hagar Meir --- internal/bft/viewchanger.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/bft/viewchanger.go b/internal/bft/viewchanger.go index d260c586..85d814a6 100644 --- a/internal/bft/viewchanger.go +++ b/internal/bft/viewchanger.go @@ -331,7 +331,10 @@ func (v *ViewChanger) processMsg(sender uint64, m *protos.Message) { } // InformNewView tells the view changer to advance to a new view number. -// It never blocks: if the view changer did not process a previous view yet, only the highest of the two is kept. +// It is called from the controller's run loop after a sync, and so it must not wait for the view changer. +// If the view changer did not process a previous view yet, the pending view is replaced by the higher of the two, +// which loses nothing since informNewView ignores views lower than the current one. +// The pending view is held only in informChan, so concurrent calls are safe as well. func (v *ViewChanger) InformNewView(view uint64) { for { select {