From c04d18564391995fb9265bc942f1052fad498ea8 Mon Sep 17 00:00:00 2001 From: noot <36753753+noot@users.noreply.github.com> Date: Fri, 25 Mar 2022 11:13:19 -0400 Subject: [PATCH] fix(lib/grandpa): various finality fixes, improves cross-client finality (#2368) --- lib/grandpa/errors.go | 2 +- lib/grandpa/message_tracker.go | 36 ++++++++++++++++++ lib/grandpa/message_tracker_test.go | 59 ++++++++++++++++++++++++++++- lib/grandpa/vote_message.go | 19 ++-------- 4 files changed, 98 insertions(+), 18 deletions(-) diff --git a/lib/grandpa/errors.go b/lib/grandpa/errors.go index 46ce29dbf6..54937cf1ef 100644 --- a/lib/grandpa/errors.go +++ b/lib/grandpa/errors.go @@ -93,7 +93,7 @@ var ( // ErrAuthorityNotInSet is returned when a precommit within a justification is signed by a key not in the authority set ErrAuthorityNotInSet = errors.New("authority is not in set") - errVoteExists = errors.New("already have vote") errVoteToSignatureMismatch = errors.New("votes and authority count mismatch") errInvalidVoteBlock = errors.New("block in vote is not descendant of previously finalised block") + errVoteFromSelf = errors.New("got vote from ourselves") ) diff --git a/lib/grandpa/message_tracker.go b/lib/grandpa/message_tracker.go index 4f540ac0dc..c05b9dce78 100644 --- a/lib/grandpa/message_tracker.go +++ b/lib/grandpa/message_tracker.go @@ -5,6 +5,7 @@ package grandpa import ( "sync" + "time" "github.com/ChainSafe/gossamer/dot/types" "github.com/ChainSafe/gossamer/lib/common" @@ -82,6 +83,10 @@ func (t *tracker) addCatchUpResponse(cr *CatchUpResponse) { } func (t *tracker) handleBlocks() { + const timeout = time.Second + ticker := time.NewTicker(timeout) + defer ticker.Stop() + for { select { case b := <-t.in: @@ -90,6 +95,8 @@ func (t *tracker) handleBlocks() { } t.handleBlock(b) + case <-ticker.C: + t.handleTick() case <-t.stopped: return } @@ -122,3 +129,32 @@ func (t *tracker) handleBlock(b *types.Block) { delete(t.commitMessages, h) } } + +func (t *tracker) handleTick() { + t.mapLock.Lock() + defer t.mapLock.Unlock() + + for _, vms := range t.voteMessages { + for _, v := range vms { + // handleMessage would never error for vote message + _, err := t.handler.handleMessage(v.from, v.msg) + if err != nil { + logger.Debugf("failed to handle vote message %v: %s", v, err) + } + + if v.msg.Round < t.handler.grandpa.state.round && v.msg.SetID == t.handler.grandpa.state.setID { + delete(t.voteMessages, v.msg.Message.Hash) + } + } + } + + for _, cm := range t.commitMessages { + _, err := t.handler.handleMessage("", cm) + if err != nil { + logger.Debugf("failed to handle commit message %v: %s", cm, err) + continue + } + + delete(t.commitMessages, cm.Vote.Hash) + } +} diff --git a/lib/grandpa/message_tracker_test.go b/lib/grandpa/message_tracker_test.go index ca4b3cdc28..4a97432122 100644 --- a/lib/grandpa/message_tracker_test.go +++ b/lib/grandpa/message_tracker_test.go @@ -9,6 +9,7 @@ import ( "github.com/ChainSafe/gossamer/dot/state" "github.com/ChainSafe/gossamer/dot/types" + "github.com/ChainSafe/gossamer/lib/common" "github.com/ChainSafe/gossamer/lib/crypto/ed25519" "github.com/ChainSafe/gossamer/lib/keystore" @@ -85,11 +86,12 @@ func TestMessageTracker_SendMessage(t *testing.T) { }) require.NoError(t, err) + const testTimeout = time.Second select { case v := <-in: require.Equal(t, msg, v.msg) case <-time.After(testTimeout): - t.Errorf("did not receive vote message") + t.Errorf("did not receive vote message %v", msg) } } @@ -180,3 +182,58 @@ func TestMessageTracker_MapInsideMap(t *testing.T) { _, ok = voteMsgs[authorityID] require.True(t, ok) } + +func TestMessageTracker_handleTick(t *testing.T) { + kr, err := keystore.NewEd25519Keyring() + require.NoError(t, err) + + gs, in, _, _ := setupGrandpa(t, kr.Bob().(*ed25519.Keypair)) + gs.tracker = newTracker(gs.blockState, gs.messageHandler) + + testHash := common.Hash{1, 2, 3} + msg := &VoteMessage{ + Round: 100, + Message: SignedMessage{ + Hash: testHash, + }, + } + gs.tracker.addVote(&networkVoteMessage{ + msg: msg, + }) + + gs.tracker.handleTick() + + const testTimeout = time.Second + select { + case v := <-in: + require.Equal(t, msg, v.msg) + case <-time.After(testTimeout): + t.Errorf("did not receive vote message %v", msg) + } + + // shouldn't be deleted as round in message >= grandpa round + require.Equal(t, 1, len(gs.tracker.voteMessages[testHash])) + + gs.state.round = 1 + msg = &VoteMessage{ + Round: 0, + Message: SignedMessage{ + Hash: testHash, + }, + } + gs.tracker.addVote(&networkVoteMessage{ + msg: msg, + }) + + gs.tracker.handleTick() + + select { + case v := <-in: + require.Equal(t, msg, v.msg) + case <-time.After(testTimeout): + t.Errorf("did not receive vote message %v", msg) + } + + // should be deleted as round in message < grandpa round + require.Empty(t, len(gs.tracker.voteMessages[testHash])) +} diff --git a/lib/grandpa/vote_message.go b/lib/grandpa/vote_message.go index 827d400cf2..f0c36bc66e 100644 --- a/lib/grandpa/vote_message.go +++ b/lib/grandpa/vote_message.go @@ -35,7 +35,7 @@ func (s *Service) receiveVoteMessages(ctx context.Context) { continue } - logger.Tracef("received vote message %v from %s", msg.msg, msg.from) + logger.Debugf("received vote message %v from %s", msg.msg, msg.from) vm := msg.msg switch vm.Message.Stage { @@ -129,19 +129,6 @@ func (s *Service) validateVoteMessage(from peer.ID, m *VoteMessage) (*Vote, erro return nil, err } - switch m.Message.Stage { - case prevote, primaryProposal: - pv, has := s.loadVote(pk.AsBytes(), prevote) - if has && pv.Vote.Hash.Equal(m.Message.Hash) { - return nil, errVoteExists - } - case precommit: - pc, has := s.loadVote(pk.AsBytes(), precommit) - if has && pc.Vote.Hash.Equal(m.Message.Hash) { - return nil, errVoteExists - } - } - err = validateMessageSignature(pk, m) if err != nil { return nil, err @@ -194,10 +181,10 @@ func (s *Service) validateVoteMessage(from peer.ID, m *VoteMessage) (*Vote, erro vote := NewVote(m.Message.Hash, m.Message.Number) - // if the vote is from ourselves, ignore + // if the vote is from ourselves, return an error kb := [32]byte(s.publicKeyBytes()) if bytes.Equal(m.Message.AuthorityID[:], kb[:]) { - return vote, nil + return nil, errVoteFromSelf } err = s.validateVote(vote)