fix(lib/grandpa): Storing Justification Allows Extra Bytes (GSR-13) (#…

…2618)

dot/sync/chain_processor.go

@@ -253,13 +253,13 @@ func (s *chainProcessor) handleJustification(header *types.Header, justification
 		return
 	}
 
-	err := s.finalityGadget.VerifyBlockJustification(header.Hash(), justification)
+	returnedJustification, err := s.finalityGadget.VerifyBlockJustification(header.Hash(), justification)
 	if err != nil {
 		logger.Warnf("failed to verify block number %d and hash %s justification: %s", header.Number, header.Hash(), err)
 		return
 	}
 
-	err = s.blockState.SetJustification(header.Hash(), justification)
+	err = s.blockState.SetJustification(header.Hash(), returnedJustification)
 	if err != nil {
 		logger.Errorf("failed tostore justification: %s", err)
 		return

dot/sync/chain_processor_test.go

@@ -258,7 +258,8 @@ func Test_chainProcessor_handleJustification(t *testing.T) {
 		"invalid justification": {
 			chainProcessorBuilder: func(ctrl *gomock.Controller) chainProcessor {
 				mockFinalityGadget := NewMockFinalityGadget(ctrl)
-				mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash, []byte(`x`)).Return(errors.New("error"))
+				mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash,
+					[]byte(`x`)).Return(nil, errors.New("error"))
 				return chainProcessor{
 					finalityGadget: mockFinalityGadget,
 				}
@@ -275,7 +276,7 @@ func Test_chainProcessor_handleJustification(t *testing.T) {
 				mockBlockState := NewMockBlockState(ctrl)
 				mockBlockState.EXPECT().SetJustification(expectedHash, []byte(`xx`)).Return(errors.New("fake error"))
 				mockFinalityGadget := NewMockFinalityGadget(ctrl)
-				mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash, []byte(`xx`)).Return(nil)
+				mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash, []byte(`xx`)).Return([]byte(`xx`), nil)
 				return chainProcessor{
 					blockState:     mockBlockState,
 					finalityGadget: mockFinalityGadget,
@@ -293,7 +294,7 @@ func Test_chainProcessor_handleJustification(t *testing.T) {
 				mockBlockState := NewMockBlockState(ctrl)
 				mockBlockState.EXPECT().SetJustification(expectedHash, []byte(`1234`)).Return(nil)
 				mockFinalityGadget := NewMockFinalityGadget(ctrl)
-				mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash, []byte(`1234`)).Return(nil)
+				mockFinalityGadget.EXPECT().VerifyBlockJustification(expectedHash, []byte(`1234`)).Return([]byte(`1234`), nil)
 				return chainProcessor{
 					blockState:     mockBlockState,
 					finalityGadget: mockFinalityGadget,
@@ -417,7 +418,7 @@ func Test_chainProcessor_processBlockData(t *testing.T) {
 				mockFinalityGadget := NewMockFinalityGadget(ctrl)
 				mockFinalityGadget.EXPECT().VerifyBlockJustification(common.MustHexToHash(
 					"0x6443a0b46e0412e626363028115a9f2cf963eeed526b8b33e5316f08b50d0dc3"), []byte{1, 2,
-					3})
+					3}).Return([]byte{1, 2, 3}, nil)
 				mockStorageState := NewMockStorageState(ctrl)
 				mockStorageState.EXPECT().TrieState(&common.Hash{}).Return(nil, nil)
 				mockBlockImportHandler := NewMockBlockImportHandler(ctrl)
@@ -452,7 +453,7 @@ func Test_chainProcessor_processBlockData(t *testing.T) {
 				mockFinalityGadget := NewMockFinalityGadget(ctrl)
 				mockFinalityGadget.EXPECT().VerifyBlockJustification(common.MustHexToHash(
 					"0x6443a0b46e0412e626363028115a9f2cf963eeed526b8b33e5316f08b50d0dc3"), []byte{1, 2,
-					3})
+					3}).Return([]byte{1, 2, 3}, nil)
 				mockStorageState := NewMockStorageState(ctrl)
 				mockStorageState.EXPECT().TrieState(&common.Hash{}).Return(nil, mockError)
 				return chainProcessor{
@@ -484,7 +485,7 @@ func Test_chainProcessor_processBlockData(t *testing.T) {
 				mockFinalityGadget := NewMockFinalityGadget(ctrl)
 				mockFinalityGadget.EXPECT().VerifyBlockJustification(common.MustHexToHash(
 					"0x6443a0b46e0412e626363028115a9f2cf963eeed526b8b33e5316f08b50d0dc3"), []byte{1, 2,
-					3})
+					3}).Return([]byte{1, 2, 3}, nil)
 				mockStorageState := NewMockStorageState(ctrl)
 				mockStorageState.EXPECT().TrieState(&common.Hash{}).Return(nil, nil)
 				mockBlockImportHandler := NewMockBlockImportHandler(ctrl)
@@ -687,7 +688,8 @@ func Test_chainProcessor_processBlockData(t *testing.T) {
 				mockTelemetry.EXPECT().SendMessage(gomock.Any()).AnyTimes()
 				mockFinalityGadget := NewMockFinalityGadget(ctrl)
 				mockFinalityGadget.EXPECT().VerifyBlockJustification(
-					common.MustHexToHash("0xdcdd89927d8a348e00257e1ecc8617f45edb5118efff3ea2f9961b2ad9b7690a"), justification)
+					common.MustHexToHash("0xdcdd89927d8a348e00257e1ecc8617f45edb5118efff3ea2f9961b2ad9b7690a"),
+					justification).Return(justification, nil)
 				return chainProcessor{
 					blockState:         mockBlockState,
 					babeVerifier:       mockBabeVerifier,

dot/sync/interface.go

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

dot/sync/syncer_integration_test.go

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

lib/grandpa/message_handler.go

@@ -553,48 +553,49 @@ func (h *MessageHandler) verifyJustification(just *SignedVote, round, setID uint
 	return nil
 }
 
-// VerifyBlockJustification verifies the finality justification for a block
-func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byte) error {
+// 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) {
 	fj := Justification{}
 	err := scale.Unmarshal(justification, &fj)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	setID, err := s.grandpaState.GetSetIDByBlockNumber(uint(fj.Commit.Number))
 	if err != nil {
-		return fmt.Errorf("cannot get set ID from block number: %w", err)
+		return nil, fmt.Errorf("cannot get set ID from block number: %w", err)
 	}
 
 	has, err := s.blockState.HasFinalisedBlock(fj.Round, setID)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	if has {
-		return fmt.Errorf("already have finalised block with setID=%d and round=%d", setID, fj.Round)
+		return nil, fmt.Errorf("already have finalised block with setID=%d and round=%d", setID, fj.Round)
 	}
 
 	isDescendant, err := isDescendantOfHighestFinalisedBlock(s.blockState, fj.Commit.Hash)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	if !isDescendant {
-		return errVoteBlockMismatch
+		return nil, errVoteBlockMismatch
 	}
 
 	auths, err := s.grandpaState.GetAuthorities(setID)
 	if err != nil {
-		return fmt.Errorf("cannot get authorities for set ID: %w", err)
+		return nil, 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 ErrMinVotesNotMet
+		return nil, ErrMinVotesNotMet
 	}
 
 	authPubKeys := make([]AuthData, len(fj.Commit.Precommits))
@@ -604,7 +605,7 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt
 
 	equivocatoryVoters, err := getEquivocatoryVoters(authPubKeys)
 	if err != nil {
-		return fmt.Errorf("could not get valid equivocatory voters: %w", err)
+		return nil, fmt.Errorf("could not get valid equivocatory voters: %w", err)
 	}
 
 	var count int
@@ -617,20 +618,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 err
+			return nil, err
 		}
 
 		if !isDescendant {
-			return ErrPrecommitBlockMismatch
+			return nil, ErrPrecommitBlockMismatch
 		}
 
 		pk, err := ed25519.NewPublicKey(just.AuthorityID[:])
 		if err != nil {
-			return err
+			return nil, err
 		}
 
 		if !isInAuthSet(pk, auths) {
-			return ErrAuthorityNotInSet
+			return nil, ErrAuthorityNotInSet
 		}
 
 		// verify signature for each precommit
@@ -641,16 +642,16 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt
 			SetID: setID,
 		})
 		if err != nil {
-			return err
+			return nil, err
 		}
 
 		ok, err := pk.Verify(msg, just.Signature[:])
 		if err != nil {
-			return err
+			return nil, err
 		}
 
 		if !ok {
-			return ErrInvalidSignature
+			return nil, ErrInvalidSignature
 		}
 
 		if _, ok := equivocatoryVoters[just.AuthorityID]; ok {
@@ -661,30 +662,30 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt
 	}
 
 	if count+len(equivocatoryVoters) < threshold {
-		return ErrMinVotesNotMet
+		return nil, ErrMinVotesNotMet
 	}
 
 	err = verifyBlockHashAgainstBlockNumber(s.blockState, fj.Commit.Hash, uint(fj.Commit.Number))
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	for _, preCommit := range fj.Commit.Precommits {
 		err := verifyBlockHashAgainstBlockNumber(s.blockState, preCommit.Vote.Hash, uint(preCommit.Vote.Number))
 		if err != nil {
-			return err
+			return nil, err
 		}
 	}
 
 	err = s.blockState.SetFinalisedHash(hash, fj.Round, setID)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	logger.Debugf(
 		"set finalised block with hash %s, round %d and set id %d",
 		hash, fj.Round, setID)
-	return nil
+	return scale.Marshal(fj)
 }
 
 func verifyBlockHashAgainstBlockNumber(bs BlockState, hash common.Hash, number uint) error {