diff --git a/internal/client/consensus/grandpa/grandpa.go b/internal/client/consensus/grandpa/grandpa.go index 4c592b3546..94ac233f10 100644 --- a/internal/client/consensus/grandpa/grandpa.go +++ b/internal/client/consensus/grandpa/grandpa.go @@ -436,7 +436,7 @@ type voterWork[ E runtime.Extrinsic, ] struct { voter *grandpa.Voter[H, N, primitives.AuthoritySignature, primitives.AuthorityID] - voterErrChan <-chan error + voterDone <-chan error sharedVoterState *SharedVoterState[primitives.AuthorityID] env *environment[H, N, Hasher, Header, E] voterCommandsRx <-chan voterCommand @@ -554,14 +554,9 @@ func (vw *voterWork[H, N, Hasher, Header, E]) rebuildVoter() { // Repoint shared_voter_state so that the RPC endpoint can query the state vw.sharedVoterState.reset(voter.VoterState()) + // NewVoter runs the voter; Done yields why it stopped, once it has. vw.voter = voter - errChan := make(chan error) - go func() { - err := voter.Start() - errChan <- err - close(errChan) - }() - vw.voterErrChan = errChan + vw.voterDone = voter.Done() case voterSetStatePaused[H, N]: default: panic("unreachable") @@ -651,9 +646,9 @@ func (vw *voterWork[H, N, Hasher, Header, E]) handleVoterCommand(command voterCo func (vw *voterWork[H, N, Hasher, Header, E]) poll() error { select { - case err := <-vw.voterErrChan: + case err := <-vw.voterDone: if err == nil { - // voters don't conclude naturally + // nothing here closes globalIn, so the voter has no orderly way to stop return fmt.Errorf("consensus-grandpa inner voter has concluded: %w", ErrSafety) } vc, isVoterCommand := err.(voterCommand) diff --git a/pkg/finality-grandpa/bridge_state.go b/pkg/finality-grandpa/bridge_state.go index 1e18608bb1..4fba41a423 100644 --- a/pkg/finality-grandpa/bridge_state.go +++ b/pkg/finality-grandpa/bridge_state.go @@ -17,13 +17,16 @@ func newWaker() *waker { } func (w *waker) wake() { + // Read under the lock and hand the value to the goroutine, which outlives the + // lock and would otherwise race register's write. w.RLock() - defer w.RUnlock() - if w.wakeCh == nil { + ch := w.wakeCh + w.RUnlock() + if ch == nil { return } go func() { - w.wakeCh <- struct{}{} + ch <- struct{}{} }() } diff --git a/pkg/finality-grandpa/environment_test.go b/pkg/finality-grandpa/environment_test.go index e6286b4f99..c7e7236f18 100644 --- a/pkg/finality-grandpa/environment_test.go +++ b/pkg/finality-grandpa/environment_test.go @@ -26,7 +26,11 @@ type environment struct { network *Network listeners []chan listenerItem lastCompleteAndConcluded [2]uint64 - mtx sync.Mutex + // roundIn holds the inbound channels handed to the voter, per round, so they + // can be closed once the round concludes. RoundData is called more than once + // for a round number, hence a slice. + roundIn map[uint64][]chan SignedMessageError[string, uint32, Signature, ID] + mtx sync.Mutex concludedCalled chan struct{} } @@ -36,6 +40,7 @@ func newEnvironment(network *Network, localID ID) environment { chain: newDummyChain(), localID: localID, network: network, + roundIn: make(map[uint64][]chan SignedMessageError[string, uint32, Signature, ID]), concludedCalled: make(chan struct{}), } } @@ -84,6 +89,12 @@ func (e *environment) RoundData( outgoing := make(Output[string, uint32]) incoming := e.network.MakeRoundComms(round, e.localID, outgoing) + // Remember it so Concluded can close it: the voter reads this channel through + // a forwarding goroutine that ends only when the channel does. + e.mtx.Lock() + e.roundIn[round] = append(e.roundIn[round], incoming) + e.mtx.Unlock() + var outgoingFunc = func(m Message[string, uint32]) error { outgoing <- m return nil @@ -123,8 +134,16 @@ func (e *environment) Concluded( _ HistoricalVotes[string, uint32, Signature, ID], ) error { e.mtx.Lock() - defer e.mtx.Unlock() e.lastCompleteAndConcluded[1] = round + incoming := e.roundIn[round] + delete(e.roundIn, round) + e.mtx.Unlock() + + // The round is over, so release the inbound channels handed out for it. + for _, in := range incoming { + e.network.StopRoundComms(round, in) + } + go func() { e.concludedCalled <- struct{}{} }() @@ -259,13 +278,29 @@ func (bm *BroadcastNetwork[M, N]) AddNode(f func(N) M, out chan N) (in chan M) { func (bm *BroadcastNetwork[M, N]) route() { defer bm.routeWG.Done() for msg := range bm.receiver { + // Under the lock: RemoveNode closes a node's channel, and closing one a + // producer is about to send on panics. Senders are buffered, so holding it + // across the delivery does not block. bm.mu.Lock() bm.history = append(bm.history, msg) - senders := append([]chan M(nil), bm.senders...) - bm.mu.Unlock() - for _, sender := range senders { + for _, sender := range bm.senders { sender <- msg } + bm.mu.Unlock() + } +} + +// RemoveNode deregisters a node's inbound channel and closes it, shutting down +// the voter reading it. Held under bm.mu so it cannot race a delivery in route. +func (bm *BroadcastNetwork[M, N]) RemoveNode(in chan M) { + bm.mu.Lock() + defer bm.mu.Unlock() + for i, sender := range bm.senders { + if sender == in { + bm.senders = append(bm.senders[:i], bm.senders[i+1:]...) + close(in) + return + } } } @@ -402,6 +437,31 @@ func (n *Network) MakeGlobalComms( }, out) } +// StopRoundComms closes one inbound channel handed out by MakeRoundComms. Only +// that node's channel: the round network is shared, and other voters may still +// be in this round. +func (n *Network) StopRoundComms( + roundNumber uint64, + in chan SignedMessageError[string, uint32, Signature, ID], +) { + n.mtx.Lock() + round, ok := n.rounds[roundNumber] + n.mtx.Unlock() + + if ok { + round.RemoveNode(in) + } +} + +// StopGlobalComms closes the inbound channel handed to a voter by +// MakeGlobalComms, which is how that voter is shut down. +func (n *Network) StopGlobalComms(in chan GlobalInItem[string, uint32, Signature, ID]) { + n.mtx.Lock() + defer n.mtx.Unlock() + + n.globalMessages.RemoveNode(in) +} + func (n *Network) SendMessage(message CommunicationIn[string, uint32, Signature, ID]) { n.globalMessages.SendMessage(GlobalInItem[string, uint32, Signature, ID]{message, nil}) } diff --git a/pkg/finality-grandpa/timer.go b/pkg/finality-grandpa/timer.go index 97c2755f7a..d798ec6798 100644 --- a/pkg/finality-grandpa/timer.go +++ b/pkg/finality-grandpa/timer.go @@ -9,39 +9,46 @@ import ( "time" ) +// timer reports whether a deadline has passed and wakes whoever is polling it +// when that changes. Rounds create several and discard them as they advance, so +// Close releases one whose round finished before it fired. type timer struct { - wakerChan *wakerChan[error] + waker atomic.Pointer[waker] + stop chan struct{} closeOnce sync.Once expired atomic.Bool } func newTimer(in <-chan time.Time) *timer { - inErr := make(chan error) - wc := newWakerChan(inErr) - t := timer{wakerChan: wc} + t := timer{stop: make(chan struct{})} go t.poll(in) return &t } func (t *timer) poll(in <-chan time.Time) { - <-in - t.closeOnce.Do(func() { - t.wakerChan.in <- nil - close(t.wakerChan.in) - }) + select { + case <-in: + case <-t.stop: + return + } + // Ordered: waking before expired is set would send the poller back to sleep + // having seen the timer as still pending. t.expired.Store(true) + if w := t.waker.Load(); w != nil { + w.wake() + } } func (t *timer) SetWaker(waker *waker) { - t.wakerChan.setWaker(waker) + t.waker.Store(waker) } func (t *timer) Elapsed() (bool, error) { return t.expired.Load(), nil } +// Close releases a timer that has not fired. Idempotent, and a no-op once the +// timer has elapsed. func (t *timer) Close() { - t.closeOnce.Do(func() { - close(t.wakerChan.in) - }) + t.closeOnce.Do(func() { close(t.stop) }) } diff --git a/pkg/finality-grandpa/voter.go b/pkg/finality-grandpa/voter.go index 2b0d48afac..f0dfb8c236 100644 --- a/pkg/finality-grandpa/voter.go +++ b/pkg/finality-grandpa/voter.go @@ -4,6 +4,7 @@ package grandpa import ( + "errors" "fmt" "sync" "sync/atomic" @@ -29,7 +30,14 @@ func newWakerChan[Item any](in chan Item) *wakerChan[Item] { } func (wc *wakerChan[Item]) start() { - defer close(wc.out) + defer func() { + close(wc.out) + // Wake the consumer so it polls once more and sees the close, which is how + // a shutdown reaches it. + if w := wc.waker.Load(); w != nil { + w.wake() + } + }() if wc.in == nil { return } @@ -119,6 +127,12 @@ type Environment[Hash comparable, Number constraints.Unsigned, Signature compara // // Furthermore, this means that actual logic of creating and verifying // signatures is flexible and can be maintained outside this crate. + // + // The Incoming channel belongs to the implementation, which must close it once + // the round has concluded, and release any still open when it shuts down. The + // voter reads it through a forwarding goroutine that ends only when the + // channel does, so a round input left open outlives its round. RoundData may + // be called more than once for a round number; each call owns its channel. RoundData( round uint64, ) RoundData[Hash, Number, Signature, ID, Message[Hash, Number]] @@ -535,12 +549,22 @@ type Voter[Hash constraints.Ordered, Number constraints.Unsigned, Signature comp // assumptions from round-to-round. lastFinalizedInRounds HashNumber[Hash, Number] - stopTimeout time.Duration - stopChan chan struct{} - wg sync.WaitGroup + // done carries the terminal error, published after teardown and closed behind + // it. Buffered, so the voter never waits on an owner that has stopped + // listening. + done chan error } -// NewVoter creates a new `Voter` tracker with given round number and base block. +// errVoterShutdown marks an orderly shutdown as it unwinds through the poll +// path. The run loop translates it to a nil error, so it never reaches a caller. +var errVoterShutdown = errors.New("voter shutdown: global incoming stream closed") + +// NewVoter creates a new `Voter` tracker with given round number and base block, +// and starts it running. +// +// Close globalIn to shut the voter down. It belongs to the caller, who must be +// its only writer by that point or must serialise its writers against the close. +// Done then yields why the voter stopped. // // Provide data about the last completed round. If there is no // known last completed round, the genesis state (round number 0, no votes, genesis base), @@ -561,6 +585,13 @@ func NewVoter[Hash constraints.Ordered, Number constraints.Unsigned, Signature c lastRoundBase HashNumber[Hash, Number], lastFinalized HashNumber[Hash, Number], ) *Voter[Hash, Number, Signature, ID] { + // A nil globalIn would otherwise yield a voter that stops immediately and + // reports a clean shutdown, since closing that channel is the shutdown signal + // and a nil one reads as already closed. + if globalIn == nil { + panic("grandpa: NewVoter requires a non-nil globalIn; closing it is how the voter is shut down") + } + finalizedSender := make(chan finalizedNotification[Hash, Number, Signature, ID], 1) finalizedNotifications := finalizedSender lastFinalizedNumber := lastFinalized.Number @@ -597,7 +628,7 @@ func NewVoter[Hash constraints.Ordered, Number constraints.Unsigned, Signature c bestRound: bestRound, pastRounds: *pastRounds, } - return &Voter[Hash, Number, Signature, ID]{ + v := &Voter[Hash, Number, Signature, ID]{ env: env, voters: voters, inner: inner, @@ -606,16 +637,23 @@ func NewVoter[Hash constraints.Ordered, Number constraints.Unsigned, Signature c lastFinalizedInRounds: lastFinalized, globalIn: newWakerChan(globalIn), globalOut: newBuffered(globalOutPresend), - stopChan: make(chan struct{}), - stopTimeout: 30 * time.Second, + done: make(chan error, 1), } + go func() { + err := v.run() + // Before publishing, so a receive on done means teardown is complete. + v.teardown() + v.done <- err + close(v.done) + }() + return v } func (v *Voter[Hash, Number, Signature, ID]) pruneBackgroundRounds(waker *waker) error { // Collect finalize notifications under the lock, then invoke // env.FinalizeBlock outside it. Holding inner.Mutex across user-supplied // callbacks is a deadlock hazard: a slow environment can block readers - // of the voter state and stall Stop(). + // of the voter state and stall the voter's teardown. v.inner.Lock() pastRounds: @@ -644,7 +682,15 @@ pastRounds: finalizedNotifications: for { select { - case notif := <-v.finalizedNotifications.channel(): + case notif, ok := <-v.finalizedNotifications.channel(): + // The voter's own channel, closed only by teardown, so a live poll loop + // cannot legitimately see it closed: finalization is gone and cannot be + // reopened. A closed channel is permanently ready, so this check is what + // ends the loop rather than the default arm. + if !ok { + v.inner.Unlock() + return fmt.Errorf("finalization notification stream closed") + } fNum := notif.Number v.inner.pastRounds.UpdateFinalized(fNum) if v.setLastFinalizedNumber(fNum) { @@ -681,7 +727,13 @@ func (v *Voter[Hash, Number, Signature, ID]) processIncoming(waker *waker) error loop: for { select { - case item := <-v.globalIn.channel(): + case item, ok := <-v.globalIn.channel(): + // Closed once globalIn is: the caller's shutdown signal. A closed channel + // is permanently ready, so this check is what ends the loop rather than + // the default arm. + if !ok { + return errVoterShutdown + } if item.Error != nil { return item.Error } @@ -894,41 +946,49 @@ func (v *Voter[Hash, Number, Signature, ID]) setLastFinalizedNumber(finalizedNum return false } -func (v *Voter[Hash, Number, Signature, ID]) Start() error { //skipcq: RVV-B0001 - v.wg.Add(1) - defer v.wg.Done() +// run is the voter's poll loop. NewVoter starts it; it ends when the globalIn +// channel closes, or on the first error. +func (v *Voter[Hash, Number, Signature, ID]) run() error { waker := newWaker() for { ready, err := v.poll(waker) if err != nil { + if errors.Is(err, errVoterShutdown) { + return nil + } return err } if ready { return nil } - select { - case <-waker.channel(): - case <-v.stopChan: - return fmt.Errorf("early voter stop") - } + <-waker.channel() } } -func (v *Voter[Hash, Number, Signature, ID]) Stop() error { - close(v.stopChan) - v.globalOut.Close() - timeout := time.NewTimer(v.stopTimeout) - wgDone := make(chan struct{}) - go func() { - defer close(wgDone) - v.wg.Wait() - }() - select { - case <-timeout.C: - return fmt.Errorf("timeout for Voter.Stop()") - case <-wgDone: - } +// Done yields why the voter stopped: nil if globalIn was closed, otherwise the +// error it failed with. Every error is terminal. Receiving here means the voter +// has finished and released what it owned, so there is nothing further to call. +// +// Select on it to supervise the voter alongside other work: +// +// select { +// case err := <-voter.Done(): +// // the voter has stopped; err says why +// case cmd := <-commands: +// } +// +// The error goes to one receiver and the channel closes behind it, so later +// receives yield nil. A voter has a single owner; if others need the outcome, +// that owner fans it out. +func (v *Voter[Hash, Number, Signature, ID]) Done() <-chan error { + return v.done +} +// teardown releases what the voter owns: its finalization channel and every +// round timer. It runs on the voter's own goroutine, so callers have no teardown +// obligation. +func (v *Voter[Hash, Number, Signature, ID]) teardown() { + v.globalOut.Close() close(v.finalizedNotifications.in) switch state := v.inner.bestRound.state.(type) { case statePrecommitted: @@ -964,8 +1024,6 @@ func (v *Voter[Hash, Number, Signature, ID]) Stop() error { close(round.roundCommitter.importCommits.in) } } - - return nil } func (v *Voter[Hash, Number, Signature, ID]) poll(waker *waker) (bool, error) { //skipcq: RVV-B0001 diff --git a/pkg/finality-grandpa/voter_lifecycle_test.go b/pkg/finality-grandpa/voter_lifecycle_test.go new file mode 100644 index 0000000000..81203e92e3 --- /dev/null +++ b/pkg/finality-grandpa/voter_lifecycle_test.go @@ -0,0 +1,233 @@ +// Copyright 2023 ChainSafe Systems (ON) +// SPDX-License-Identifier: LGPL-3.0-only + +package grandpa + +import ( + "runtime" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type lifecycleItem = GlobalInItem[string, uint32, Signature, ID] + +// newLifecycleVoter builds a voter over its own inbound channel, for tests about +// shutting down rather than about voting. NewVoter starts it. +func newLifecycleVoter(t *testing.T, network *Network, globalIn chan lifecycleItem, +) *Voter[string, uint32, Signature, ID] { + t.Helper() + var localID ID = 5 + voters := NewVoterSet([]IDWeight[ID]{{localID, 100}}) + + env := newEnvironment(network, localID) + var lastFinalized HashNumber[string, uint32] + env.WithChain(func(chain *dummyChain) { + chain.PushBlocks(GenesisHash, []string{"A", "B", "C"}) + lastFinalized.Hash, lastFinalized.Number = chain.LastFinalized() + }) + + return NewVoter[string, uint32, Signature, ID]( + &env, *voters, globalIn, + func(CommunicationOut[string, uint32, Signature, ID]) error { return nil }, + 0, nil, lastFinalized, lastFinalized, + ) +} + +// forwardersInState counts wakerChan.start goroutines blocked in the given +// runtime state: "chan send" is one parked on the unbuffered `out` holding an +// item, "chan receive" one waiting on `in`. +func forwardersInState(state string) int { + buf := make([]byte, 1<<22) + n := runtime.Stack(buf, true) + count := 0 + for _, blk := range strings.Split(string(buf[:n]), "\ngoroutine ") { + nl := strings.Index(blk, "\n") + if nl < 0 { + continue + } + if strings.Contains(blk[:nl], state) && + strings.Contains(blk[nl:], "wakerChan") && strings.Contains(blk[nl:], ".start(") { + count++ + } + } + return count +} + +// Closing globalIn is the shutdown signal, and Done reports nil for it: an +// orderly shutdown is not a failure. +func TestVoter_CloseGlobalInShutsDown(t *testing.T) { + network := NewNetwork() + defer network.Stop() + + globalIn := make(chan lifecycleItem, 10) + v := newLifecycleVoter(t, network, globalIn) + time.Sleep(50 * time.Millisecond) + + select { + case <-v.Done(): + t.Fatal("Done fired while the voter was still running") + default: + } + + close(globalIn) + select { + case err := <-v.Done(): + assert.NoError(t, err, "an orderly shutdown is not an error") + case <-time.After(5 * time.Second): + t.Fatal("the voter did not stop after globalIn was closed") + } +} + +// Receiving from Done means the voter has finished releasing what it owned, not +// merely that its loop stopped. +func TestVoter_DoneImpliesTeardown(t *testing.T) { + network := NewNetwork() + defer network.Stop() + + globalIn := make(chan lifecycleItem, 10) + v := newLifecycleVoter(t, network, globalIn) + time.Sleep(50 * time.Millisecond) + + close(globalIn) + require.NoError(t, <-v.Done()) + + // Teardown closes the voter's own finalization channel, which ends its + // forwarder and closes the channel the voter was reading. + require.Eventually(t, func() bool { + select { + case _, open := <-v.finalizedNotifications.channel(): + return !open + default: + return false + } + }, 2*time.Second, 10*time.Millisecond, "teardown did not release the finalization channel") +} + +// The error goes to one receiver and the channel closes behind it. A voter has a +// single owner; this pins the contract so it is not discovered by accident. +func TestVoter_DoneDeliversToOneReceiver(t *testing.T) { + network := NewNetwork() + defer network.Stop() + + globalIn := make(chan lifecycleItem, 10) + v := newLifecycleVoter(t, network, globalIn) + time.Sleep(50 * time.Millisecond) + close(globalIn) + + require.NoError(t, <-v.Done()) + select { + case err, open := <-v.Done(): + assert.False(t, open, "Done should be closed after delivering") + assert.NoError(t, err) + case <-time.After(time.Second): + t.Fatal("Done blocked after delivering; it should be closed") + } +} + +// The retired voter's forwarder must be gone once Done fires. There is no early +// exit from the poll loop, so it can only leave by observing the close, which +// means first draining whatever the forwarder is holding. +func TestVoter_RotationLeavesNoForwarder(t *testing.T) { + network := NewNetwork() + defer network.Stop() + + baseSend := forwardersInState("chan send") + + const rotations = 10 + for i := 0; i < rotations; i++ { + globalIn := make(chan lifecycleItem, 100) + v := newLifecycleVoter(t, network, globalIn) + time.Sleep(30 * time.Millisecond) + + // Inbound traffic in flight when the set rotates. + for j := 0; j < 100; j++ { + select { + case globalIn <- lifecycleItem{}: + default: + } + } + + close(globalIn) + require.NoError(t, <-v.Done()) + } + + time.Sleep(300 * time.Millisecond) + assert.Equal(t, baseSend, forwardersInState("chan send"), + "a retired voter's globalIn forwarder is still holding an item") +} + +// Rebuilding the voter across an authority-set rotation. With no separate start +// step there is no window in which a half-live voter can be torn down. +func TestVoter_RebuildAcrossRotations(t *testing.T) { + network := NewNetwork() + defer network.Stop() + + globalIn := make(chan lifecycleItem, 100) + voter := newLifecycleVoter(t, network, globalIn) + + const rotations = 100 + for i := 0; i < rotations; i++ { + close(globalIn) + require.NoError(t, <-voter.Done()) + + globalIn = make(chan lifecycleItem, 100) + voter = newLifecycleVoter(t, network, globalIn) + } + close(globalIn) + require.NoError(t, <-voter.Done()) +} + +// A voter that keeps running must not accumulate goroutines. Every round wraps +// its inbound stream and its timers, and both have to be released as rounds +// advance rather than only at shutdown. +func TestVoter_RoundsDoNotAccumulateForwarders(t *testing.T) { + network := NewNetwork() + defer network.Stop() + + globalIn := make(chan lifecycleItem, 10) + v := newLifecycleVoter(t, network, globalIn) + defer func() { + close(globalIn) + <-v.Done() + }() + + live := func() int { + return forwardersInState("chan receive") + forwardersInState("chan send") + } + + // Let the voter settle into a steady state before taking the baseline, so + // start-up rounds are not counted as growth. + time.Sleep(2 * time.Second) + v.inner.Lock() + firstRound := v.inner.bestRound.roundNumber() + v.inner.Unlock() + base := live() + + time.Sleep(8 * time.Second) + v.inner.Lock() + lastRound := v.inner.bestRound.roundNumber() + v.inner.Unlock() + grew := live() - base + + rounds := lastRound - firstRound + require.Greater(t, rounds, uint64(2), "test needs several rounds to have elapsed") + t.Logf("%d rounds elapsed, forwarders grew by %+d", rounds, grew) + assert.LessOrEqual(t, grew, 2, + "forwarders grow with rounds: %d over %d rounds", grew, rounds) +} + +// globalIn is mandatory: closing it is the shutdown signal, and a nil channel +// reads as already closed, so a voter built on one would stop immediately and +// report a clean shutdown. +func TestVoter_NilGlobalInPanics(t *testing.T) { + network := NewNetwork() + defer network.Stop() + + assert.PanicsWithValue(t, + "grandpa: NewVoter requires a non-nil globalIn; closing it is how the voter is shut down", + func() { newLifecycleVoter(t, network, nil) }) +} diff --git a/pkg/finality-grandpa/voter_test.go b/pkg/finality-grandpa/voter_test.go index da0a4cef72..188a8b61d1 100644 --- a/pkg/finality-grandpa/voter_test.go +++ b/pkg/finality-grandpa/voter_test.go @@ -31,11 +31,12 @@ func TestVoter_TalkingToMyself(t *testing.T) { }) globalOut := make(chan CommunicationOut[string, uint32, Signature, ID]) + globalIn := network.MakeGlobalComms(globalOut) finalized := env.FinalizedStream() voter := NewVoter[string, uint32, Signature, ID]( &env, *voters, - nil, + globalIn, func(co CommunicationOut[string, uint32, Signature, ID]) error { globalOut <- co; return nil }, 0, nil, @@ -43,21 +44,11 @@ func TestVoter_TalkingToMyself(t *testing.T) { lastFinalized, ) - globalIn := network.MakeGlobalComms(globalOut) - voter.globalIn = newWakerChan(globalIn) - - done := make(chan struct{}) - go func() { - defer close(done) - err := voter.Start() - // stops early, so this should return an error - assert.Error(t, err) - }() - <-finalized - err := voter.Stop() + network.StopGlobalComms(globalIn) + // closing globalIn is an orderly shutdown, not a failure + err := <-voter.Done() assert.NoError(t, err) - <-done } func TestVoter_FinalizingAtFaultThreshold(t *testing.T) { @@ -85,10 +76,11 @@ func TestVoter_FinalizingAtFaultThreshold(t *testing.T) { // run voter in background. scheduling it to shut down at the end. finalized := env.FinalizedStream() globalOut := make(chan CommunicationOut[string, uint32, Signature, ID]) + globalIn := network.MakeGlobalComms(globalOut) voter := NewVoter[string, uint32, Signature, ID]( &env, *voters, - make(chan GlobalInItem[string, uint32, Signature, ID]), + globalIn, func(co CommunicationOut[string, uint32, Signature, ID]) error { globalOut <- co; return nil }, 0, nil, @@ -96,15 +88,12 @@ func TestVoter_FinalizingAtFaultThreshold(t *testing.T) { lastFinalized, ) - globalIn := network.MakeGlobalComms(globalOut) - voter.globalIn = newWakerChan(globalIn) - wg.Add(1) - go voter.Start() go func() { defer wg.Done() <-finalized - err := voter.Stop() + network.StopGlobalComms(globalIn) + err := <-voter.Done() assert.NoError(t, err) }() } @@ -127,6 +116,7 @@ func TestVoter_ExposingVoterState(t *testing.T) { var wg sync.WaitGroup voters := make([]*Voter[string, uint32, Signature, ID], votersOnline) voterStates := make([]VoterState[ID], votersOnline) + globalIns := make([]chan GlobalInItem[string, uint32, Signature, ID], votersOnline) // some voters offline for i := 0; i < votersOnline; i++ { localID := ID(i) @@ -141,10 +131,11 @@ func TestVoter_ExposingVoterState(t *testing.T) { // run voter in background. scheduling it to shut down at the end. finalized := env.FinalizedStream() globalOut := make(chan CommunicationOut[string, uint32, Signature, ID]) + globalIn := network.MakeGlobalComms(globalOut) voter := NewVoter[string, uint32, Signature, ID]( &env, *voterSet, - make(chan GlobalInItem[string, uint32, Signature, ID]), + globalIn, func(co CommunicationOut[string, uint32, Signature, ID]) error { globalOut <- co; return nil }, 0, nil, @@ -152,11 +143,9 @@ func TestVoter_ExposingVoterState(t *testing.T) { lastFinalized, ) - globalIn := network.MakeGlobalComms(globalOut) - voter.globalIn = newWakerChan(globalIn) - voters[i] = voter voterStates[i] = voter.VoterState() + globalIns[i] = globalIn wg.Add(1) go func() { @@ -190,9 +179,6 @@ func TestVoter_ExposingVoterState(t *testing.T) { voterState.Get(), ) - for _, v := range voters { - go v.Start() - } wg.Wait() assert.Equal(t, @@ -203,8 +189,9 @@ func TestVoter_ExposingVoterState(t *testing.T) { }{2, expectedRoundState}, ) - for _, v := range voters { - err := v.Stop() + for i, v := range voters { + network.StopGlobalComms(globalIns[i]) + err := <-v.Done() assert.NoError(t, err) } } @@ -227,10 +214,11 @@ func TestVoter_BroadcastCommit(t *testing.T) { // run voter in background. scheduling it to shut down at the end. globalOut := make(chan CommunicationOut[string, uint32, Signature, ID]) + globalIn := network.MakeGlobalComms(globalOut) voter := NewVoter[string, uint32, Signature, ID]( &env, *voterSet, - make(chan GlobalInItem[string, uint32, Signature, ID]), + globalIn, func(co CommunicationOut[string, uint32, Signature, ID]) error { globalOut <- co; return nil }, 0, nil, @@ -240,13 +228,10 @@ func TestVoter_BroadcastCommit(t *testing.T) { commitsIn := network.MakeGlobalComms(globalOut) - globalIn := network.MakeGlobalComms(globalOut) - voter.globalIn = newWakerChan(globalIn) - - go voter.Start() <-commitsIn - err := voter.Stop() + network.StopGlobalComms(globalIn) + err := <-voter.Done() assert.NoError(t, err) } @@ -292,22 +277,17 @@ func TestVoter_BroadcastCommitOnlyIfNewer(t *testing.T) { // run voter in background. scheduling it to shut down at the end. globalOut := make(chan CommunicationOut[string, uint32, Signature, ID]) + globalIn := network.MakeGlobalComms(globalOut) voter := NewVoter[string, uint32, Signature, ID]( &env, *voterSet, - nil, + globalIn, func(co CommunicationOut[string, uint32, Signature, ID]) error { globalOut <- co; return nil }, 0, nil, lastFinalized, lastFinalized, ) - globalIn := network.MakeGlobalComms(globalOut) - voter.globalIn = newWakerChan(globalIn) - - go func() { - voter.Start() - }() item := <-roundIn // wait for a prevote @@ -351,7 +331,8 @@ waitForCommits: } assert.Equal(t, 1, commitCount) - err := voter.Stop() + network.StopGlobalComms(globalIn) + err := <-voter.Done() assert.NoError(t, err) } @@ -389,10 +370,11 @@ func TestVoter_ImportCommitForAnyRound(t *testing.T) { // run voter in background. scheduling it to shut down at the end. globalOut := make(chan CommunicationOut[string, uint32, Signature, ID]) + globalIn := network.MakeGlobalComms(globalOut) voter := NewVoter[string, uint32, Signature, ID]( &env, *voterSet, - nil, + globalIn, func(co CommunicationOut[string, uint32, Signature, ID]) error { globalOut <- co; return nil }, 0, nil, @@ -400,13 +382,6 @@ func TestVoter_ImportCommitForAnyRound(t *testing.T) { lastFinalized, ) - globalIn := network.MakeGlobalComms(globalOut) - voter.globalIn = newWakerChan(globalIn) - - go func() { - voter.Start() - }() - // Send the commit message co := CommunicationOutCommit[string, uint32, Signature, ID]{ Number: 0, @@ -417,7 +392,8 @@ func TestVoter_ImportCommitForAnyRound(t *testing.T) { finalized := <-env.FinalizedStream() assert.Equal(t, finalized.Commit, commit) - err := voter.Stop() + network.StopGlobalComms(globalIn) + err := <-voter.Done() assert.NoError(t, err) } @@ -447,18 +423,17 @@ func TestVoter_SkipsToLatestRoundAfterCatchUp(t *testing.T) { }) globalOut := make(chan CommunicationOut[string, uint32, Signature, ID]) + globalIn := network.MakeGlobalComms(globalOut) unsyncedVoter := NewVoter[string, uint32, Signature, ID]( &env, *voterSet, - nil, + globalIn, func(co CommunicationOut[string, uint32, Signature, ID]) error { globalOut <- co; return nil }, 0, nil, lastFinalized, lastFinalized, ) - globalIn := network.MakeGlobalComms(globalOut) - unsyncedVoter.globalIn = newWakerChan(globalIn) prevote := func(id uint32) SignedPrevote[string, uint32, Signature, ID] { return SignedPrevote[string, uint32, Signature, ID]{ @@ -492,9 +467,6 @@ func TestVoter_SkipsToLatestRoundAfterCatchUp(t *testing.T) { _, ok := voterState.Get().BackgroundRounds[5] assert.False(t, ok) - // spawn the voter in the background - go unsyncedVoter.Start() - finalized := env.FinalizedStream() // wait until it's caught up, it should skip to round 6 and send a @@ -542,7 +514,8 @@ func TestVoter_SkipsToLatestRoundAfterCatchUp(t *testing.T) { }, voterState.Get().BackgroundRounds[5]) - err := unsyncedVoter.Stop() + network.StopGlobalComms(globalIn) + err := <-unsyncedVoter.Done() assert.NoError(t, err) } @@ -564,27 +537,26 @@ func TestVoter_PickUpFromPriorWithoutGrandparentState(t *testing.T) { // run voter in background. scheduling it to shut down at the end. globalOut := make(chan CommunicationOut[string, uint32, Signature, ID]) + globalIn := network.MakeGlobalComms(globalOut) voter := NewVoter[string, uint32, Signature, ID]( &env, *voterSet, - nil, + globalIn, func(co CommunicationOut[string, uint32, Signature, ID]) error { globalOut <- co; return nil }, 10, nil, lastFinalized, lastFinalized, ) - globalIn := network.MakeGlobalComms(globalOut) - voter.globalIn = newWakerChan(globalIn) - go voter.Start() for finalized := range env.FinalizedStream() { if finalized.Number >= 6 { break } } - err := voter.Stop() + network.StopGlobalComms(globalIn) + err := <-voter.Done() assert.NoError(t, err) } @@ -655,19 +627,17 @@ func TestVoter_PickUpFromPriorWithGrandparentStatus(t *testing.T) { // run voter in background. scheduling it to shut down at the end. globalOut := make(chan CommunicationOut[string, uint32, Signature, ID]) + globalIn := network.MakeGlobalComms(globalOut) voter := NewVoter[string, uint32, Signature, ID]( &env, *voterSet, - nil, + globalIn, func(co CommunicationOut[string, uint32, Signature, ID]) error { globalOut <- co; return nil }, 1, lastRoundVotes, lastFinalized, lastFinalized, ) - globalIn := network.MakeGlobalComms(globalOut) - voter.globalIn = newWakerChan(globalIn) - go voter.Start() // wait until we see a prevote on round 3 from our local ID, // indicating that the round 3 has started. @@ -690,7 +660,8 @@ waitForPrevote: <-env.concludedCalled assert.Equal(t, [2]uint64{2, 1}, env.LastCompletedAndConcluded()) - err := voter.Stop() + network.StopGlobalComms(globalIn) + err := <-voter.Done() assert.NoError(t, err) } diff --git a/pkg/finality-grandpa/voting_round.go b/pkg/finality-grandpa/voting_round.go index 70808b3558..9c251fc826 100644 --- a/pkg/finality-grandpa/voting_round.go +++ b/pkg/finality-grandpa/voting_round.go @@ -4,6 +4,7 @@ package grandpa import ( + "fmt" "time" "golang.org/x/exp/constraints" @@ -459,7 +460,13 @@ func (vr *votingRound[Hash, Number, Signature, ID, E]) processIncoming(waker *wa while: for { select { - case incoming := <-vr.incoming.channel(): + case incoming, ok := <-vr.incoming.channel(): + // roundData.Incoming belongs to the environment. A round still voting + // cannot recover from losing it, and the zero value carries a nil Message + // that handleVote would dereference. + if !ok { + return fmt.Errorf("round %d: incoming message stream closed", vr.roundNumber()) + } log.Tracef("Round %d: Got incoming message", vr.roundNumber()) if timer != nil { timer.Stop() @@ -590,8 +597,13 @@ func (vr *votingRound[Hash, Number, Signature, ID, E]) prevote(w *waker, lastRou wakerChan := newWakerChan(bestChain) wakerChan.setWaker(waker) var best *HashNumber[Hash, Number] - res := <-wakerChan.channel() + res, ok := <-wakerChan.channel() switch { + case !ok: + // An empty closed channel means the stream went away. Distinct from the + // default arm below, where a real {nil, nil} value means "no best chain + // yet" and legitimately parks the round in prevoting. + return fmt.Errorf("round %d: best chain stream closed", vr.roundNumber()) case res.Error != nil: return res.Error case res.Value != nil: