fix(lib/grandpa): on verifying block justification, compare given block hash with finalised hash

dot/interfaces.go

@@ -31,7 +31,7 @@ type ServiceRegisterer interface {
 
 // BlockJustificationVerifier has a verification method for block justifications.
 type BlockJustificationVerifier interface {
-	VerifyBlockJustification(common.Hash, []byte) ([]byte, error)
+	VerifyBlockJustification(common.Hash, []byte) error
 }
 
 // Telemetry is the telemetry client to send telemetry messages.

dot/sync/chain_processor.go

@@ -284,12 +284,12 @@ func (s *chainProcessor) handleJustification(header *types.Header, justification
 	logger.Debugf("handling justification for block %d...", header.Number)
 
 	headerHash := header.Hash()
-	returnedJustification, err := s.finalityGadget.VerifyBlockJustification(headerHash, justification)
+	err = s.finalityGadget.VerifyBlockJustification(headerHash, justification)
 	if err != nil {
 		return fmt.Errorf("verifying block number %d justification: %w", header.Number, err)
 	}
 
-	err = s.blockState.SetJustification(headerHash, returnedJustification)
+	err = s.blockState.SetJustification(headerHash, justification)
 	if err != nil {
 		return fmt.Errorf("setting justification for block number %d: %w", header.Number, err)
 	}

dot/sync/chain_processor_test.go

@@ -296,7 +296,7 @@ func Test_chainProcessor_handleJustification(t *testing.T) {
 			chainProcessorBuilder: func(ctrl *gomock.Controller) chainProcessor {
 				mockFinalityGadget := NewMockFinalityGadget(ctrl)
 				mockFinalityGadget.EXPECT().VerifyBlockJustification(headerHash,
-					[]byte(`x`)).Return(nil, errTest)
+					[]byte(`x`)).Return(errTest)
 				return chainProcessor{
 					finalityGadget: mockFinalityGadget,
 				}
@@ -313,7 +313,7 @@ func Test_chainProcessor_handleJustification(t *testing.T) {
 				mockBlockState := NewMockBlockState(ctrl)
 				mockBlockState.EXPECT().SetJustification(headerHash, []byte(`xx`)).Return(errTest)
 				mockFinalityGadget := NewMockFinalityGadget(ctrl)
-				mockFinalityGadget.EXPECT().VerifyBlockJustification(headerHash, []byte(`xx`)).Return([]byte(`xx`), nil)
+				mockFinalityGadget.EXPECT().VerifyBlockJustification(headerHash, []byte(`xx`)).Return(nil)
 				return chainProcessor{
 					blockState:     mockBlockState,
 					finalityGadget: mockFinalityGadget,
@@ -331,7 +331,7 @@ func Test_chainProcessor_handleJustification(t *testing.T) {
 				mockBlockState := NewMockBlockState(ctrl)
 				mockBlockState.EXPECT().SetJustification(headerHash, []byte(`1234`)).Return(nil)
 				mockFinalityGadget := NewMockFinalityGadget(ctrl)
-				mockFinalityGadget.EXPECT().VerifyBlockJustification(headerHash, []byte(`1234`)).Return([]byte(`1234`), nil)
+				mockFinalityGadget.EXPECT().VerifyBlockJustification(headerHash, []byte(`1234`)).Return(nil)
 				return chainProcessor{
 					blockState:     mockBlockState,
 					finalityGadget: mockFinalityGadget,
@@ -430,7 +430,7 @@ func Test_chainProcessor_processBlockData(t *testing.T) {
 				mockFinalityGadget := NewMockFinalityGadget(ctrl)
 				mockFinalityGadget.EXPECT().VerifyBlockJustification(common.MustHexToHash(
 					"0x6443a0b46e0412e626363028115a9f2cf963eeed526b8b33e5316f08b50d0dc3"), []byte{1, 2,
-					3}).Return([]byte{1, 2, 3}, nil)
+					3}).Return(nil)
 				mockStorageState := NewMockStorageState(ctrl)
 				mockStorageState.EXPECT().TrieState(&common.Hash{}).Return(nil, nil)
 
@@ -490,7 +490,7 @@ func Test_chainProcessor_processBlockData(t *testing.T) {
 				expectedBlockDataHeaderHash := expectedBlockDataHeader.Hash()
 				finalityGadget.EXPECT().
 					VerifyBlockJustification(expectedBlockDataHeaderHash, []byte{1, 2, 3}).
-					Return(nil, mockError)
+					Return(mockError)
 
 				mockChainSync := NewMockChainSync(ctrl)
 				mockChainSync.EXPECT().syncState().Return(bootstrap)
@@ -564,7 +564,7 @@ func Test_chainProcessor_processBlockData(t *testing.T) {
 				mockFinalityGadget := NewMockFinalityGadget(ctrl)
 				mockFinalityGadget.EXPECT().VerifyBlockJustification(
 					common.MustHexToHash("0xdcdd89927d8a348e00257e1ecc8617f45edb5118efff3ea2f9961b2ad9b7690a"),
-					[]byte{1, 2, 3}).Return([]byte{1, 2, 3}, nil)
+					[]byte{1, 2, 3}).Return(nil)
 				return chainProcessor{
 					chainSync:          mockChainSync,
 					blockState:         mockBlockState,
@@ -660,7 +660,7 @@ func Test_chainProcessor_processBlockDataWithStateHeaderAndBody(t *testing.T) {
 				finalityGadget := NewMockFinalityGadget(ctrl)
 				finalityGadget.EXPECT().
 					VerifyBlockJustification(blockHeaderHash, []byte{3}).
-					Return(nil, errTest)
+					Return(errTest)
 
 				return chainProcessor{
 					blockState:     blockState,

dot/sync/interfaces.go

@@ -61,7 +61,7 @@ type BabeVerifier interface {
 
 // FinalityGadget implements justification verification functionality
 type FinalityGadget interface {
-	VerifyBlockJustification(common.Hash, []byte) ([]byte, error)
+	VerifyBlockJustification(common.Hash, []byte) error
 }
 
 // BlockImportHandler is the interface for the handler of newly imported blocks

dot/sync/syncer_integration_test.go

@@ -109,8 +109,8 @@ func newTestSyncer(t *testing.T) *Service {
 	cfg.LogLvl = log.Trace
 	mockFinalityGadget := NewMockFinalityGadget(ctrl)
 	mockFinalityGadget.EXPECT().VerifyBlockJustification(gomock.AssignableToTypeOf(common.Hash{}),
-		gomock.AssignableToTypeOf([]byte{})).DoAndReturn(func(hash common.Hash, justification []byte) ([]byte, error) {
-		return justification, nil
+		gomock.AssignableToTypeOf([]byte{})).DoAndReturn(func(hash common.Hash, justification []byte) error {
+		return nil
 	}).AnyTimes()
 
 	cfg.FinalityGadget = mockFinalityGadget

lib/blocktree/blocktree.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/ChainSafe/gossamer/dot/types"
+
 	"github.com/ChainSafe/gossamer/lib/common"
 	"github.com/disiqueira/gotree"
 	"github.com/prometheus/client_golang/prometheus"
@@ -513,7 +514,7 @@ func (bt *BlockTree) StoreRuntime(hash common.Hash, in Runtime) {
 func (bt *BlockTree) GetBlockRuntime(hash common.Hash) (Runtime, error) {
 	ins := bt.runtimes.get(hash)
 	if ins == nil {
-		return nil, ErrFailedToGetRuntime
+		return nil, fmt.Errorf("%w for hash %s", ErrFailedToGetRuntime, hash)
 	}
 	return ins, nil
 }

lib/grandpa/errors.go

@@ -83,6 +83,9 @@ 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")
 
+	// errFinalisedBlocksMismatch is returned when we find another block finalised in the same set id and round
+	errFinalisedBlocksMismatch = errors.New("already have finalised block with the same setID and round")
+
 	errVoteToSignatureMismatch = errors.New("votes and authority count mismatch")
 	errVoteBlockMismatch       = errors.New("block in vote is not descendant of previously finalised block")
 	errVoteFromSelf            = errors.New("got vote from ourselves")

lib/grandpa/message_handler.go

@@ -404,52 +404,60 @@ func (h *MessageHandler) verifyPreCommitJustification(msg *CatchUpResponse) erro
 
 // VerifyBlockJustification verifies the finality justification for a block, returns scale encoded justification with
 // any extra bytes removed.
-func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byte) ([]byte, error) {
+func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byte) error {
 	fj := Justification{}
 	err := scale.Unmarshal(justification, &fj)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	if hash != fj.Commit.Hash {
-		return nil, fmt.Errorf("%w: justification %s and block hash %s",
+		return fmt.Errorf("%w: justification %s and block hash %s",
 			ErrJustificationMismatch, fj.Commit.Hash.Short(), hash.Short())
 	}
 
 	setID, err := s.grandpaState.GetSetIDByBlockNumber(uint(fj.Commit.Number))
 	if err != nil {
-		return nil, fmt.Errorf("cannot get set ID from block number: %w", err)
+		return fmt.Errorf("cannot get set ID from block number: %w", err)
 	}
 
 	has, err := s.blockState.HasFinalisedBlock(fj.Round, setID)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	if has {
-		return nil, fmt.Errorf("already have finalised block with setID=%d and round=%d", setID, fj.Round)
+		storedFinalisedHash, err := s.blockState.GetFinalisedHash(fj.Round, setID)
+		if err != nil {
+			return fmt.Errorf("getting finalised hash: %w", err)
+		}
+		if storedFinalisedHash != hash {
+			return fmt.Errorf("%w, setID=%d and round=%d", errFinalisedBlocksMismatch, setID, fj.Round)
+		}
+
+		return nil
 	}
 
 	isDescendant, err := isDescendantOfHighestFinalisedBlock(s.blockState, fj.Commit.Hash)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	if !isDescendant {
-		return nil, errVoteBlockMismatch
+		return errVoteBlockMismatch
 	}
 
 	auths, err := s.grandpaState.GetAuthorities(setID)
 	if err != nil {
-		return nil, fmt.Errorf("cannot get authorities for set ID: %w", err)
+		return fmt.Errorf("cannot get authorities for set ID: %w", err)
 	}
 
 	// threshold is two-thirds the number of authorities,
 	// uses the current set of authorities to define the threshold
 	threshold := (2 * len(auths) / 3)
 
 	if len(fj.Commit.Precommits) < threshold {
-		return nil, ErrMinVotesNotMet
+		return ErrMinVotesNotMet
 	}
 
 	authPubKeys := make([]AuthData, len(fj.Commit.Precommits))
@@ -469,20 +477,20 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt
 		// check if vote was for descendant of committed block
 		isDescendant, err := s.blockState.IsDescendantOf(hash, just.Vote.Hash)
 		if err != nil {
-			return nil, err
+			return err
 		}
 
 		if !isDescendant {
-			return nil, ErrPrecommitBlockMismatch
+			return ErrPrecommitBlockMismatch
 		}
 
 		publicKey, err := ed25519.NewPublicKey(just.AuthorityID[:])
 		if err != nil {
-			return nil, err
+			return err
 		}
 
 		if !isInAuthSet(publicKey, auths) {
-			return nil, ErrAuthorityNotInSet
+			return ErrAuthorityNotInSet
 		}
 
 		// verify signature for each precommit
@@ -493,16 +501,16 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt
 			SetID: setID,
 		})
 		if err != nil {
-			return nil, err
+			return err
 		}
 
 		ok, err := publicKey.Verify(msg, just.Signature[:])
 		if err != nil {
-			return nil, err
+			return err
 		}
 
 		if !ok {
-			return nil, ErrInvalidSignature
+			return ErrInvalidSignature
 		}
 
 		if _, ok := equivocatoryVoters[just.AuthorityID]; ok {
@@ -513,30 +521,30 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt
 	}
 
 	if count+len(equivocatoryVoters) < threshold {
-		return nil, ErrMinVotesNotMet
+		return ErrMinVotesNotMet
 	}
 
 	err = verifyBlockHashAgainstBlockNumber(s.blockState, fj.Commit.Hash, uint(fj.Commit.Number))
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	for _, preCommit := range fj.Commit.Precommits {
 		err := verifyBlockHashAgainstBlockNumber(s.blockState, preCommit.Vote.Hash, uint(preCommit.Vote.Number))
 		if err != nil {
-			return nil, err
+			return err
 		}
 	}
 
 	err = s.blockState.SetFinalisedHash(hash, fj.Round, setID)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	logger.Debugf(
 		"set finalised block with hash %s, round %d and set id %d",
 		hash, fj.Round, setID)
-	return scale.Marshal(fj)
+	return nil
 }
 
 func verifyBlockHashAgainstBlockNumber(bs BlockState, hash common.Hash, number uint) error {