From 262f817641f86e88df53909585335fed69c7756c Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Fri, 8 Mar 2024 18:34:27 +0300 Subject: [PATCH] timer: drop HV interface, use this struct less often * HV is not a perfect name * in most cases we can have simpler interface with one/two values * the only problem is LastSeenMessage where we need to have this pair, so it's kept for this purpose only, but with a better name Signed-off-by: Roman Khimov --- CHANGELOG.md | 1 + README.md | 6 +-- config.go | 11 ----- context.go | 19 ++++++--- dbft.go | 19 ++++----- dbft_test.go | 38 +++++++---------- internal/consensus/consensus.go | 1 - internal/simulation/main.go | 2 +- timer.go | 14 +++--- timer/timer.go | 76 +++++++++++---------------------- timer/timer_test.go | 30 +++++++------ 11 files changed, 87 insertions(+), 130 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e331e11..850b55fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ Behaviour changes: * drop outdated dBFT `Service` interface (#95) * move all default implementations to `internal` package (#97) * remove unused APIs of dBFT and payload interfaces (#104) + * timer interface refactoring (#105) Improvements: * add MIT License (#78, #79) diff --git a/README.md b/README.md index 99b4faba..5b97a410 100644 --- a/README.md +++ b/README.md @@ -24,9 +24,9 @@ provided. for main fields. `Transaction` is an entity which can be hashed. Two entities having equal hashes are considered equal. No default implementation is provided. 5. `dbft` contains generic interfaces for payloads. No default implementation is provided. -6. `dbft` contains generic interfaces for time-related operations (`Timer` and `HV`). `timer` package contains -default time and height-view providers, it contains minimal required timer functionality and may safely be used in -production code. It should make it easier to write tests concerning dBFT's time depending behaviour. +6. `dbft` contains generic `Timer` interface for time-related operations. `timer` package contains +default `Timer` provider that can safely be used in production code. The interface itself +is mostly created for tests dealing with dBFT's time-dependant behaviour. 7. `internal` contains an example of custom identity types and payloads implementation used to implement an example of dBFT's usage with 6-node consensus. Refer to `internal` subpackages for type-specific dBFT implementation and tests. Refer to `internal/simulation` for an example of dBFT library usage. diff --git a/config.go b/config.go index 31bea995..a65962de 100644 --- a/config.go +++ b/config.go @@ -14,8 +14,6 @@ type Config[H Hash] struct { Logger *zap.Logger // Timer Timer Timer - // NewHeightView is a constructor for [dbft.HV] object. - NewHeightView func(height uint32, view byte) HV // SecondsPerBlock is the number of seconds that // need to pass before another block will be accepted. SecondsPerBlock time.Duration @@ -112,8 +110,6 @@ func checkConfig[H Hash](cfg *Config[H]) error { return errors.New("private key is nil") } else if cfg.Timer == nil { return errors.New("Timer is nil") - } else if cfg.NewHeightView == nil { - return errors.New("NewHeightView is nil") } else if cfg.CurrentHeight == nil { return errors.New("CurrentHeight is nil") } else if cfg.CurrentBlockHash == nil { @@ -186,13 +182,6 @@ func WithTimer[H Hash](t Timer) func(config *Config[H]) { } } -// WithNewHeightView sets NewHeightView constructor. -func WithNewHeightView[H Hash](f func(height uint32, view byte) HV) func(config *Config[H]) { - return func(cfg *Config[H]) { - cfg.NewHeightView = f - } -} - // WithSecondsPerBlock sets SecondsPerBlock. func WithSecondsPerBlock[H Hash](d time.Duration) func(config *Config[H]) { return func(cfg *Config[H]) { diff --git a/context.go b/context.go index 334a17b6..4a7ce73d 100644 --- a/context.go +++ b/context.go @@ -6,6 +6,12 @@ import ( "time" ) +// HeightView is a block height/consensus view pair. +type HeightView struct { + Height uint32 + View byte +} + // Context is a main dBFT structure which // contains all information needed for performing transitions. type Context[H Hash] struct { @@ -63,9 +69,9 @@ type Context[H Hash] struct { ChangeViewPayloads []ConsensusPayload[H] // LastChangeViewPayloads stores consensus ChangeView payloads for the last epoch. LastChangeViewPayloads []ConsensusPayload[H] - // LastSeenMessage array stores the height of the last seen message, for each validator. - // if this node never heard from validator i, LastSeenMessage[i] will be -1. - LastSeenMessage []*HV + // LastSeenMessage array stores the height and view of the last seen message, for each validator. + // If this node never heard a thing from validator i, LastSeenMessage[i] will be nil. + LastSeenMessage []*HeightView lastBlockTimestamp uint64 // ns-precision timestamp from the last header (used for the next block timestamp calculations). lastBlockTime time.Time // Wall clock time of when the last block was first seen (used for timer adjustments). @@ -118,7 +124,7 @@ func (c *Context[H]) CountCommitted() (count int) { // for this view and that hasn't sent the Commit message at the previous views. func (c *Context[H]) CountFailed() (count int) { for i, hv := range c.LastSeenMessage { - if c.CommitPayloads[i] == nil && (hv == nil || (*hv).Height() < c.BlockIndex || (*hv).View() < c.ViewNumber) { + if c.CommitPayloads[i] == nil && (hv == nil || hv.Height < c.BlockIndex || hv.View < c.ViewNumber) { count++ } } @@ -198,7 +204,7 @@ func (c *Context[H]) reset(view byte, ts uint64) { c.LastChangeViewPayloads = make([]ConsensusPayload[H], n) if c.LastSeenMessage == nil { - c.LastSeenMessage = make([]*HV, n) + c.LastSeenMessage = make([]*HeightView, n) } c.blockProcessed = false } else { @@ -231,8 +237,7 @@ func (c *Context[H]) reset(view byte, ts uint64) { c.ViewNumber = view if c.MyIndex >= 0 { - hv := c.Config.NewHeightView(c.BlockIndex, c.ViewNumber) - c.LastSeenMessage[c.MyIndex] = &hv + c.LastSeenMessage[c.MyIndex] = &HeightView{c.BlockIndex, c.ViewNumber} } } diff --git a/dbft.go b/dbft.go index ab50d44f..39f72db9 100644 --- a/dbft.go +++ b/dbft.go @@ -168,22 +168,22 @@ func (d *DBFT[H]) OnTransaction(tx Transaction[H]) { } // OnTimeout advances state machine as if timeout was fired. -func (d *DBFT[H]) OnTimeout(hv HV) { +func (d *DBFT[H]) OnTimeout(height uint32, view byte) { if d.Context.WatchOnly() || d.BlockSent() { return } - if hv.Height() != d.BlockIndex || hv.View() != d.ViewNumber { + if height != d.BlockIndex || view != d.ViewNumber { d.Logger.Debug("timeout: ignore old timer", - zap.Uint32("height", hv.Height()), - zap.Uint("view", uint(hv.View()))) + zap.Uint32("height", height), + zap.Uint("view", uint(view))) return } d.Logger.Debug("timeout", - zap.Uint32("height", hv.Height()), - zap.Uint("view", uint(hv.View()))) + zap.Uint32("height", height), + zap.Uint("view", uint(view))) if d.IsPrimary() && !d.RequestSentOrReceived() { d.sendPrepareRequest() @@ -236,9 +236,8 @@ func (d *DBFT[H]) OnReceive(msg ConsensusPayload[H]) { } hv := d.LastSeenMessage[msg.ValidatorIndex()] - if hv == nil || (*hv).Height() < msg.Height() || (*hv).View() < msg.ViewNumber() { - hv := d.Config.NewHeightView(msg.Height(), msg.ViewNumber()) - d.LastSeenMessage[msg.ValidatorIndex()] = &hv + if hv == nil || hv.Height < msg.Height() || hv.View < msg.ViewNumber() { + d.LastSeenMessage[msg.ValidatorIndex()] = &HeightView{msg.Height(), msg.ViewNumber()} } if d.BlockSent() && msg.Type() != RecoveryRequestType { @@ -609,7 +608,7 @@ func (d *DBFT[H]) changeTimer(delay time.Duration) { zap.Uint32("h", d.BlockIndex), zap.Int("v", int(d.ViewNumber)), zap.Duration("delay", delay)) - d.Timer.Reset(d.Config.NewHeightView(d.BlockIndex, d.ViewNumber), delay) + d.Timer.Reset(d.BlockIndex, d.ViewNumber, delay) } func (d *DBFT[H]) extendTimer(count time.Duration) { diff --git a/dbft_test.go b/dbft_test.go index 86826b4b..16ac328e 100644 --- a/dbft_test.go +++ b/dbft_test.go @@ -63,7 +63,7 @@ func TestDBFT_OnStartPrimarySendPrepareRequest(t *testing.T) { require.EqualValues(t, 2, p.ValidatorIndex()) t.Run("primary send ChangeView on timeout", func(t *testing.T) { - service.OnTimeout(timer.HV{H: s.currHeight + 1}) + service.OnTimeout(s.currHeight+1, 0) // if there are many faulty must send RecoveryRequest cv := s.tryRecv() @@ -73,10 +73,9 @@ func TestDBFT_OnStartPrimarySendPrepareRequest(t *testing.T) { // if all nodes are up must send ChangeView for i := range service.LastSeenMessage { - var hv dbft.HV = timer.HV{H: s.currHeight + 1} - service.LastSeenMessage[i] = &hv + service.LastSeenMessage[i] = &dbft.HeightView{s.currHeight + 1, 0} } - service.OnTimeout(timer.HV{H: s.currHeight + 1}) + service.OnTimeout(s.currHeight+1, 0) cv = s.tryRecv() require.NotNil(t, cv) @@ -167,8 +166,7 @@ func TestDBFT_OnReceiveRequestSendResponse(t *testing.T) { service.Start(0) for i := range service.LastSeenMessage { - var hv dbft.HV = timer.HV{H: s.currHeight + 1} - service.LastSeenMessage[i] = &hv + service.LastSeenMessage[i] = &dbft.HeightView{s.currHeight + 1, 0} } p := s.getPrepareRequest(5, txs[0].Hash()) @@ -305,10 +303,10 @@ func TestDBFT_OnReceiveCommit(t *testing.T) { require.NoError(t, service.Header().Verify(pub, cm.GetCommit().Signature())) t.Run("send recovery message on timeout", func(t *testing.T) { - service.OnTimeout(timer.HV{H: 1}) + service.OnTimeout(1, 0) require.Nil(t, s.tryRecv()) - service.OnTimeout(timer.HV{H: s.currHeight + 1}) + service.OnTimeout(s.currHeight+1, 0) r := s.tryRecv() require.NotNil(t, r) @@ -396,13 +394,13 @@ func TestDBFT_OnReceiveChangeView(t *testing.T) { service.OnReceive(resp) require.Nil(t, s.tryRecv()) - service.OnTimeout(timer.HV{H: s.currHeight + 1}) + service.OnTimeout(s.currHeight+1, 0) cv := s.tryRecv() require.NotNil(t, cv) require.Equal(t, dbft.ChangeViewType, cv.Type()) t.Run("primary sends prepare request after timeout", func(t *testing.T) { - service.OnTimeout(timer.HV{H: s.currHeight + 1, V: 1}) + service.OnTimeout(s.currHeight+1, 1) pr := s.tryRecv() require.NotNil(t, pr) require.Equal(t, dbft.PrepareRequestType, pr.Type()) @@ -425,11 +423,6 @@ func TestDBFT_Invalid(t *testing.T) { }) opts = append(opts, dbft.WithTimer[crypto.Uint256](timer.New())) - t.Run("without NewHeightView", func(t *testing.T) { - require.Nil(t, dbft.New(opts...)) - }) - - opts = append(opts, dbft.WithNewHeightView[crypto.Uint256](timer.NewHV)) t.Run("without CurrentHeight", func(t *testing.T) { require.Nil(t, dbft.New(opts...)) }) @@ -582,7 +575,7 @@ func TestDBFT_FourGoodNodesDeadlock(t *testing.T) { // (possible on timeout) and sends the ChangeView message. s3.OnReceive(resp0V0) s3.OnReceive(resp2V0) - s3.OnTimeout(timer.HV{H: r3.currHeight + 1, V: 0}) + s3.OnTimeout(r3.currHeight+1, 0) cv3V0 := r3.tryRecv() require.NotNil(t, cv3V0) require.Equal(t, dbft.ChangeViewType, cv3V0.Type()) @@ -592,7 +585,7 @@ func TestDBFT_FourGoodNodesDeadlock(t *testing.T) { // current view) and sends the ChangeView message. s1.OnReceive(resp0V0) s1.OnReceive(cv3V0) - s1.OnTimeout(timer.HV{H: r1.currHeight + 1, V: 0}) + s1.OnTimeout(r1.currHeight+1, 0) cv1V0 := r1.tryRecv() require.NotNil(t, cv1V0) require.Equal(t, dbft.ChangeViewType, cv1V0.Type()) @@ -601,7 +594,7 @@ func TestDBFT_FourGoodNodesDeadlock(t *testing.T) { // (possible on timeout after receiving at least M non-commit messages from the // current view) and sends the ChangeView message. s0.OnReceive(cv3V0) - s0.OnTimeout(timer.HV{H: r0.currHeight + 1, V: 0}) + s0.OnTimeout(r0.currHeight+1, 0) cv0V0 := r0.tryRecv() require.NotNil(t, cv0V0) require.Equal(t, dbft.ChangeViewType, cv0V0.Type()) @@ -617,7 +610,7 @@ func TestDBFT_FourGoodNodesDeadlock(t *testing.T) { require.Equal(t, uint8(1), s0.ViewNumber) // Step 10. The primary (at view 1) replica 0 sends the PrepareRequest message. - s0.OnTimeout(timer.HV{H: r0.currHeight + 1, V: 1}) + s0.OnTimeout(r0.currHeight+1, 1) reqV1 := r0.tryRecv() require.NotNil(t, reqV1) require.Equal(t, dbft.PrepareRequestType, reqV1.Type()) @@ -640,7 +633,7 @@ func TestDBFT_FourGoodNodesDeadlock(t *testing.T) { // Intermediate step A. It is added to make step 14 possible. The backup (at // view 1) replica 3 doesn't receive anything for a long time and sends // RecoveryRequest. - s3.OnTimeout(timer.HV{H: r3.currHeight + 1, V: 1}) + s3.OnTimeout(r3.currHeight+1, 1) rcvr3V1 := r3.tryRecv() require.NotNil(t, rcvr3V1) require.Equal(t, dbft.RecoveryRequestType, rcvr3V1.Type()) @@ -675,7 +668,7 @@ func TestDBFT_FourGoodNodesDeadlock(t *testing.T) { // of "lost" nodes. That's why we'he added Intermediate steps A and B. // // After that replica 1 is allowed to send the CV message. - s1.OnTimeout(timer.HV{H: r1.currHeight + 1, V: 1}) + s1.OnTimeout(r1.currHeight+1, 1) cv1V1 := r1.tryRecv() require.NotNil(t, cv1V1) require.Equal(t, dbft.ChangeViewType, cv1V1.Type()) @@ -683,7 +676,7 @@ func TestDBFT_FourGoodNodesDeadlock(t *testing.T) { // Step 13. The primary (at view 1) replica 0 decides to change its view // (possible on timeout) and sends the ChangeView message. s0.OnReceive(resp1V1) - s0.OnTimeout(timer.HV{H: r0.currHeight + 1, V: 1}) + s0.OnTimeout(r0.currHeight+1, 1) cv0V1 := r0.tryRecv() require.NotNil(t, cv0V1) require.Equal(t, dbft.ChangeViewType, cv0V1.Type()) @@ -819,7 +812,6 @@ func (s testState) copyWithIndex(myIndex int) *testState { func (s *testState) getOptions() []func(*dbft.Config[crypto.Uint256]) { opts := []func(*dbft.Config[crypto.Uint256]){ dbft.WithTimer[crypto.Uint256](timer.New()), - dbft.WithNewHeightView[crypto.Uint256](timer.NewHV), dbft.WithCurrentHeight[crypto.Uint256](func() uint32 { return s.currHeight }), dbft.WithCurrentBlockHash[crypto.Uint256](func() crypto.Uint256 { return s.currHash }), dbft.WithGetValidators[crypto.Uint256](func(...dbft.Transaction[crypto.Uint256]) []dbft.PublicKey { return s.pubs }), diff --git a/internal/consensus/consensus.go b/internal/consensus/consensus.go index 04265092..2db7c88c 100644 --- a/internal/consensus/consensus.go +++ b/internal/consensus/consensus.go @@ -20,7 +20,6 @@ func New(logger *zap.Logger, key dbft.PrivateKey, pub dbft.PublicKey, verifyPayload func(consensusPayload dbft.ConsensusPayload[crypto.Uint256]) error) *dbft.DBFT[crypto.Uint256] { return dbft.New[crypto.Uint256]( dbft.WithTimer[crypto.Uint256](timer.New()), - dbft.WithNewHeightView[crypto.Uint256](timer.NewHV), dbft.WithLogger[crypto.Uint256](logger), dbft.WithSecondsPerBlock[crypto.Uint256](time.Second*5), dbft.WithKeyPair[crypto.Uint256](key, pub), diff --git a/internal/simulation/main.go b/internal/simulation/main.go index c2cf1691..3c857727 100644 --- a/internal/simulation/main.go +++ b/internal/simulation/main.go @@ -94,7 +94,7 @@ func (n *simNode) Run(ctx context.Context) { n.log.Info("context cancelled") return case <-n.d.Timer.C(): - n.d.OnTimeout(n.d.Timer.HV()) + n.d.OnTimeout(n.d.Timer.Height(), n.d.Timer.View()) case msg := <-n.messages: n.d.OnReceive(msg) } diff --git a/timer.go b/timer.go index dedfb2a7..ba202485 100644 --- a/timer.go +++ b/timer.go @@ -10,21 +10,17 @@ type Timer interface { // Now returns current time. Now() time.Time // Reset resets timer to the specified block height and view. - Reset(hv HV, d time.Duration) + Reset(height uint32, view byte, d time.Duration) // Sleep stops execution for duration d. Sleep(d time.Duration) // Extend extends current timer with duration d. Extend(d time.Duration) // Stop stops timer. Stop() - // HV returns current height and view set for the timer. - HV() HV - // C returns channel for timer events. - C() <-chan time.Time -} - -// HV is an abstraction for pair of a Height and a View. -type HV interface { + // Height returns current height set for the timer. Height() uint32 + // View returns current view set for the timer. View() byte + // C returns channel for timer events. + C() <-chan time.Time } diff --git a/timer/timer.go b/timer/timer.go index 7bc961e3..a762679c 100644 --- a/timer/timer.go +++ b/timer/timer.go @@ -6,28 +6,17 @@ package timer import ( "time" - - "github.com/nspcc-dev/dbft" ) type ( - value struct { - HV - s time.Time - d time.Duration - } - - // HV is a pair of a H and a V that implements [dbft.HV] interface. - HV struct { - H uint32 - V byte - } - // Timer is a default [dbft.Timer] implementation. Timer struct { - val value - tt *time.Timer - ch chan time.Time + height uint32 + view byte + s time.Time + d time.Duration + tt *time.Timer + ch chan time.Time } ) @@ -49,28 +38,31 @@ func (t *Timer) C() <-chan time.Time { return t.tt.C } -// HV implements Timer interface. -func (t *Timer) HV() dbft.HV { - return t.val.HV +// Height returns current timer height. +func (t *Timer) Height() uint32 { + return t.height +} + +// View return current timer view. +func (t *Timer) View() byte { + return t.view } // Reset implements Timer interface. -func (t *Timer) Reset(hv dbft.HV, d time.Duration) { +func (t *Timer) Reset(height uint32, view byte, d time.Duration) { t.Stop() - t.val.s = t.Now() - t.val.d = d - t.val.HV = HV{ - H: hv.Height(), - V: hv.View(), - } + t.s = t.Now() + t.d = d + t.height = height + t.view = view - if t.val.d != 0 { - t.tt = time.NewTimer(t.val.d) + if t.d != 0 { + t.tt = time.NewTimer(t.d) } else { t.tt = nil drain(t.ch) - t.ch <- t.val.s + t.ch <- t.s } } @@ -96,11 +88,11 @@ func (t *Timer) Sleep(d time.Duration) { // Extend implements Timer interface. func (t *Timer) Extend(d time.Duration) { - t.val.d += d + t.d += d - if elapsed := time.Since(t.val.s); t.val.d > elapsed { + if elapsed := time.Since(t.s); t.d > elapsed { t.Stop() - t.tt = time.NewTimer(t.val.d - elapsed) + t.tt = time.NewTimer(t.d - elapsed) } } @@ -108,21 +100,3 @@ func (t *Timer) Extend(d time.Duration) { func (t *Timer) Now() time.Time { return time.Now() } - -// NewHV is a constructor of HV. -func NewHV(height uint32, view byte) dbft.HV { - return HV{ - H: height, - V: view, - } -} - -// Height implements [dbft.HV] interface. -func (hv HV) Height() uint32 { - return hv.H -} - -// View implements [dbft.HV] interface. -func (hv HV) View() byte { - return hv.V -} diff --git a/timer/timer_test.go b/timer/timer_test.go index df52c212..0f574867 100644 --- a/timer/timer_test.go +++ b/timer/timer_test.go @@ -10,21 +10,21 @@ import ( func TestTimer_Reset(t *testing.T) { tt := New() - tt.Reset(HV{H: 1, V: 2}, time.Millisecond*100) + tt.Reset(1, 2, time.Millisecond*100) tt.Sleep(time.Millisecond * 200) - shouldReceive(t, tt, HV{H: 1, V: 2}, "no value in timer") + shouldReceive(t, tt, 1, 2, "no value in timer") - tt.Reset(HV{H: 1, V: 2}, time.Second) - tt.Reset(HV{H: 2, V: 3}, 0) - shouldReceive(t, tt, HV{H: 2, V: 3}, "no value in timer after reset(0)") + tt.Reset(1, 2, time.Second) + tt.Reset(2, 3, 0) + shouldReceive(t, tt, 2, 3, "no value in timer after reset(0)") - tt.Reset(HV{H: 1, V: 2}, time.Millisecond*100) + tt.Reset(1, 2, time.Millisecond*100) tt.Sleep(time.Millisecond * 200) - tt.Reset(HV{H: 1, V: 3}, time.Millisecond*100) + tt.Reset(1, 3, time.Millisecond*100) tt.Sleep(time.Millisecond * 200) - shouldReceive(t, tt, HV{H: 1, V: 3}, "invalid value after reset") + shouldReceive(t, tt, 1, 3, "invalid value after reset") - tt.Reset(HV{H: 3, V: 1}, time.Millisecond*100) + tt.Reset(3, 1, time.Millisecond*100) shouldNotReceive(t, tt, "value arrived too early") tt.Extend(time.Millisecond * 300) @@ -32,19 +32,21 @@ func TestTimer_Reset(t *testing.T) { shouldNotReceive(t, tt, "value arrived too early after extend") tt.Sleep(time.Millisecond * 300) - shouldReceive(t, tt, HV{H: 3, V: 1}, "no value in timer after extend") + shouldReceive(t, tt, 3, 1, "no value in timer after extend") - tt.Reset(HV{1, 1}, time.Millisecond*100) + tt.Reset(1, 1, time.Millisecond*100) tt.Stop() tt.Sleep(time.Millisecond * 200) shouldNotReceive(t, tt, "timer was not stopped") } -func shouldReceive(t *testing.T, tt *Timer, hv HV, msg string) { +func shouldReceive(t *testing.T, tt *Timer, height uint32, view byte, msg string) { select { case <-tt.C(): - got := tt.HV() - require.Equal(t, hv, got) + gotHeight := tt.Height() + gotView := tt.View() + require.Equal(t, height, gotHeight) + require.Equal(t, view, gotView) default: require.Fail(t, msg) }