fix(lib/grandpa): on verifying block justification, compare given blo…

…ck hash with finalised hash (#3081)

So far, while verifying block justification in grandpa, we were just checking if we already know a finalised block that has the same set id and round as the block in question.

We should check if the finalised block that we know already and the block that we are verifying are not the same, and not throw error in this case.

We should through error if we see two finalised blocks with the same set id and round. In this case, both blocks that we were comparing were the same. We were not comparing their hashes and were throwing an error thinking that we have two finalised blocks in the same set id and round.

After this change, I was able to sync further than 198656.

dot/interfaces.go

@@ -32,7 +32,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/state/block.go

@@ -684,12 +684,33 @@ func (bs *BlockState) RangeInMemory(start, end common.Hash) ([]common.Hash, erro
 
 // IsDescendantOf returns true if child is a descendant of parent, false otherwise.
 // it returns an error if parent or child are not in the blocktree.
-func (bs *BlockState) IsDescendantOf(parent, child common.Hash) (bool, error) {
+func (bs *BlockState) IsDescendantOf(ancestor, descendant common.Hash) (bool, error) {
 	if bs.bt == nil {
 		return false, fmt.Errorf("%w", errNilBlockTree)
 	}
 
-	return bs.bt.IsDescendantOf(parent, child)
+	isDescendant, err := bs.bt.IsDescendantOf(ancestor, descendant)
+	if err != nil {
+		descendantHeader, err2 := bs.GetHeader(descendant)
+		if err2 != nil {
+			return false, fmt.Errorf("getting header: %w", err2)
+		}
+
+		ancestorHeader, err2 := bs.GetHeader(ancestor)
+		if err2 != nil {
+			return false, fmt.Errorf("getting header: %w", err2)
+		}
+
+		for current := descendantHeader; descendantHeader.Number < ancestorHeader.Number; {
+			if current.ParentHash == ancestor {
+				return true, nil
+			}
+		}
+
+		return false, nil
+	}
+
+	return isDescendant, nil
 }
 
 // LowestCommonAncestor returns the lowest common ancestor between two blocks in the tree.

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"
@@ -330,17 +331,22 @@ func (bt *BlockTree) BestBlockHash() Hash {
 
 // IsDescendantOf returns true if the child is a descendant of parent, false otherwise.
 // it returns an error if either the child or parent are not in the blocktree.
+// If parent and child are the same, we return true.
 func (bt *BlockTree) IsDescendantOf(parent, child Hash) (bool, error) {
+	if parent == child {
+		return true, nil
+	}
+
 	bt.RLock()
 	defer bt.RUnlock()
 
 	pn := bt.getNode(parent)
 	if pn == nil {
-		return false, ErrStartNodeNotFound
+		return false, fmt.Errorf("%w: node hash %s", ErrStartNodeNotFound, parent)
 	}
 	cn := bt.getNode(child)
 	if cn == nil {
-		return false, ErrEndNodeNotFound
+		return false, fmt.Errorf("%w: node hash %s", ErrEndNodeNotFound, child)
 	}
 	return cn.isDescendantOf(pn), nil
 }
@@ -513,7 +519,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

@@ -356,7 +356,7 @@ func (h *MessageHandler) verifyPreCommitJustification(msg *CatchUpResponse) erro
 
 	isDescendant, err := isDescendantOfHighestFinalisedBlock(h.blockState, msg.Hash)
 	if err != nil {
-		return err
+		return fmt.Errorf("checking if descendant of highest block: %w", err)
 	}
 	if !isDescendant {
 		return errVoteBlockMismatch
@@ -376,7 +376,7 @@ func (h *MessageHandler) verifyPreCommitJustification(msg *CatchUpResponse) erro
 				logger.Infof("we might not have synced to the given block %s yet: %s", just.Vote.Hash, err)
 				continue
 			}
-			return err
+			return fmt.Errorf("verifying block hash against block number: %w", err)
 		}
 
 		err := verifyJustification(just, msg.Round, msg.SetID, precommit, h.grandpa.authorityKeySet())
@@ -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 fmt.Errorf("checking if round and set id has finalised block: %w", 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 fmt.Errorf("checking if descendant of highest block: %w", 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 fmt.Errorf("verifying block hash against block number: %w", 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 fmt.Errorf("verifying block hash against block number: %w", err)
 		}
 	}
 
 	err = s.blockState.SetFinalisedHash(hash, fj.Round, setID)
 	if err != nil {
-		return nil, err
+		return fmt.Errorf("setting finalised hash: %w", 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 {