From 36a1b7e540e473c07cd01e8dbee20693aa73b165 Mon Sep 17 00:00:00 2001 From: John Ingve Olsen Date: Sat, 12 Aug 2023 16:54:07 +0200 Subject: [PATCH 1/4] synchronizer: fix broken timer The timer used by the synchronizer used to be resettable, but after 7776a29, the closure passed to the timer should only be used once because it stores the current view. This was done so that we can avoid the need for any synchronization of access to the current view in the synchronizer. However, I failed to realize that this would make the timer only usable once. Thus, resetting it is not possible as the view that is stored in the timer's closure would not change upon reset. This commit wraps the timer in a `oneShotTimer` struct that is intended to prevent the timer from being reused. Instead, the methods `startTimeoutTimer` and `stopTimeoutTimer` should be used instead of using the timer directly. These functions ensure that each timer is unique and thus a unique closure is used each time, ensuring that we get the correct view and avoid the need for synchronization code. --- synchronizer/synchronizer.go | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/synchronizer/synchronizer.go b/synchronizer/synchronizer.go index 1aa9714c..03e9204c 100644 --- a/synchronizer/synchronizer.go +++ b/synchronizer/synchronizer.go @@ -36,7 +36,7 @@ type Synchronizer struct { lastTimeout *hotstuff.TimeoutMsg duration ViewDuration - timer *time.Timer + timer oneShotTimer // map of collected timeout messages per view timeouts map[hotstuff.View]map[hotstuff.ID]hotstuff.TimeoutMsg @@ -89,17 +89,34 @@ func New(viewDuration ViewDuration) modules.Synchronizer { currentView: 1, duration: viewDuration, - timer: time.AfterFunc(0, func() {}), // dummy timer that will be replaced after start() is called + timer: oneShotTimer{time.AfterFunc(0, func() {})}, // dummy timer that will be replaced after start() is called timeouts: make(map[hotstuff.View]map[hotstuff.ID]hotstuff.TimeoutMsg), } } +// A oneShotTimer is a timer that should not be reused. +type oneShotTimer struct { + timerDoNotUse *time.Timer +} + +func (t oneShotTimer) Stop() bool { + return t.timerDoNotUse.Stop() +} + func (s *Synchronizer) startTimeoutTimer() { - s.timer = time.AfterFunc(s.duration.Duration(), func() { + // Store the view in a local variable so that we can avoid calling s.View() in the closure below, + // thus avoiding a data race. + view := s.View() + // It is important that the timer is NOT reused because then the view would be wrong. + s.timer = oneShotTimer{time.AfterFunc(s.duration.Duration(), func() { // The event loop will execute onLocalTimeout for us. - s.eventLoop.AddEvent(TimeoutEvent{s.View()}) - }) + s.eventLoop.AddEvent(TimeoutEvent{view}) + })} +} + +func (s *Synchronizer) stopTimeoutTimer() { + s.timer.Stop() } // Start starts the synchronizer with the given context. @@ -108,7 +125,7 @@ func (s *Synchronizer) Start(ctx context.Context) { go func() { <-ctx.Done() - s.timer.Stop() + s.stopTimeoutTimer() }() // start the initial proposal @@ -305,7 +322,7 @@ func (s *Synchronizer) AdvanceView(syncInfo hotstuff.SyncInfo) { return } - s.timer.Stop() + s.stopTimeoutTimer() if !timeout { s.duration.ViewSucceeded() @@ -318,8 +335,8 @@ func (s *Synchronizer) AdvanceView(syncInfo hotstuff.SyncInfo) { s.lastTimeout = nil s.duration.ViewStarted() - duration := s.duration.Duration() - s.timer.Reset(duration) + s.stopTimeoutTimer() + s.startTimeoutTimer() s.logger.Debugf("advanced to view %d", newView) s.eventLoop.AddEvent(ViewChangeEvent{View: newView, Timeout: timeout}) From 03f0f7ba6d695681d52a21b79641aeb4c3beb59e Mon Sep 17 00:00:00 2001 From: hanish gogada Date: Sat, 9 Mar 2024 17:07:29 +0100 Subject: [PATCH 2/4] fix(twins): Twins basic test scenario is failing With the timer changes, the network timer is twins needs to be updated. --- synchronizer/synchronizer.go | 1 - twins/network.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/synchronizer/synchronizer.go b/synchronizer/synchronizer.go index 03e9204c..ff4a1bf9 100644 --- a/synchronizer/synchronizer.go +++ b/synchronizer/synchronizer.go @@ -335,7 +335,6 @@ func (s *Synchronizer) AdvanceView(syncInfo hotstuff.SyncInfo) { s.lastTimeout = nil s.duration.ViewStarted() - s.stopTimeoutTimer() s.startTimeoutTimer() s.logger.Debugf("advanced to view %d", newView) diff --git a/twins/network.go b/twins/network.go index 1c2b677d..3f3e029c 100644 --- a/twins/network.go +++ b/twins/network.go @@ -143,7 +143,7 @@ func (n *Network) createTwinsNodes(nodes []NodeID, _ Scenario, consensusName str consensus.New(consensusModule), consensus.NewVotingMachine(), crypto.NewCache(ecdsa.New(), 100), - synchronizer.New(FixedTimeout(0)), + synchronizer.New(FixedTimeout(1*time.Millisecond)), logging.NewWithDest(&node.log, fmt.Sprintf("r%dn%d", nodeID.ReplicaID, nodeID.NetworkID)), // twins-specific: &configuration{network: n, node: node}, From 1d4cea58e306e1e14ba77f48656f52c98291c54d Mon Sep 17 00:00:00 2001 From: hanish gogada Date: Sat, 9 Mar 2024 18:38:58 +0100 Subject: [PATCH 3/4] fix: test case failed due to local timeout. Due to local timeout the sychronizer is called stopVoting which the gmock is not expecting, so the test cast is failing. Increased the timeout delay to prevent this. --- consensus/consensus_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/consensus/consensus_test.go b/consensus/consensus_test.go index ccd8ff03..f5e33f5c 100644 --- a/consensus/consensus_test.go +++ b/consensus/consensus_test.go @@ -3,6 +3,7 @@ package consensus_test import ( "context" "testing" + "time" "github.com/golang/mock/gomock" "github.com/relab/hotstuff" @@ -19,7 +20,7 @@ func TestVote(t *testing.T) { ctrl := gomock.NewController(t) bl := testutil.CreateBuilders(t, ctrl, n) cs := mocks.NewMockConsensus(ctrl) - bl[0].Add(synchronizer.New(testutil.FixedTimeout(1000)), cs) + bl[0].Add(synchronizer.New(testutil.FixedTimeout(1*time.Millisecond)), cs) hl := bl.Build() hs := hl[0] From 27c2653c9b9601376672c6e9cf5c9741eba8edf3 Mon Sep 17 00:00:00 2001 From: Hein Meling Date: Sat, 9 Mar 2024 20:57:09 +0100 Subject: [PATCH 4/4] fix: minor simplify doc comment --- synchronizer/synchronizer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synchronizer/synchronizer.go b/synchronizer/synchronizer.go index ff4a1bf9..32aaa6ad 100644 --- a/synchronizer/synchronizer.go +++ b/synchronizer/synchronizer.go @@ -105,7 +105,7 @@ func (t oneShotTimer) Stop() bool { } func (s *Synchronizer) startTimeoutTimer() { - // Store the view in a local variable so that we can avoid calling s.View() in the closure below, + // Store the view in a local variable to avoid calling s.View() in the closure below, // thus avoiding a data race. view := s.View() // It is important that the timer is NOT reused because then the view would be wrong.