fix(lib/grandpa): various finality fixes, improves cross-client finality (#2368)

Author: noot

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")
 )

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)
+	}
+}

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]))
+}

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)