From fc9184372a725b8f411e141be96fa3dcfadce92e Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Fri, 10 Feb 2023 18:43:46 +0530 Subject: [PATCH] fix(lib/grandpa): on verifying block justification, compare given block 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 | 2 +- dot/state/block.go | 25 +++- dot/sync/chain_processor.go | 4 +- dot/sync/chain_processor_test.go | 14 +- dot/sync/interfaces.go | 2 +- dot/sync/mocks_test.go | 7 +- dot/sync/syncer_integration_test.go | 4 +- lib/blocktree/blocktree.go | 12 +- lib/grandpa/errors.go | 3 + lib/grandpa/message_handler.go | 56 ++++---- .../message_handler_integration_test.go | 134 +++++++++++++----- lib/grandpa/mocks_test.go | 15 ++ lib/grandpa/state.go | 3 +- 13 files changed, 197 insertions(+), 84 deletions(-) diff --git a/dot/interfaces.go b/dot/interfaces.go index 0ebf32c907..a9255d0d3c 100644 --- a/dot/interfaces.go +++ b/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. diff --git a/dot/state/block.go b/dot/state/block.go index 311b7c7e81..25a2854bd8 100644 --- a/dot/state/block.go +++ b/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. diff --git a/dot/sync/chain_processor.go b/dot/sync/chain_processor.go index 1271771127..439419dd5a 100644 --- a/dot/sync/chain_processor.go +++ b/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) } diff --git a/dot/sync/chain_processor_test.go b/dot/sync/chain_processor_test.go index 32a7d0d7f5..902d6626ba 100644 --- a/dot/sync/chain_processor_test.go +++ b/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, diff --git a/dot/sync/interfaces.go b/dot/sync/interfaces.go index 8eac5c99fa..c43b5a8d3c 100644 --- a/dot/sync/interfaces.go +++ b/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 diff --git a/dot/sync/mocks_test.go b/dot/sync/mocks_test.go index 45f1efd5d0..cdf596ff84 100644 --- a/dot/sync/mocks_test.go +++ b/dot/sync/mocks_test.go @@ -536,12 +536,11 @@ func (m *MockFinalityGadget) EXPECT() *MockFinalityGadgetMockRecorder { } // VerifyBlockJustification mocks base method. -func (m *MockFinalityGadget) VerifyBlockJustification(arg0 common.Hash, arg1 []byte) ([]byte, error) { +func (m *MockFinalityGadget) VerifyBlockJustification(arg0 common.Hash, arg1 []byte) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "VerifyBlockJustification", arg0, arg1) - ret0, _ := ret[0].([]byte) - ret1, _ := ret[1].(error) - return ret0, ret1 + ret0, _ := ret[0].(error) + return ret0 } // VerifyBlockJustification indicates an expected call of VerifyBlockJustification. diff --git a/dot/sync/syncer_integration_test.go b/dot/sync/syncer_integration_test.go index a34b478d7a..76a6e816ed 100644 --- a/dot/sync/syncer_integration_test.go +++ b/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 diff --git a/lib/blocktree/blocktree.go b/lib/blocktree/blocktree.go index 9e18182aed..b378d92225 100644 --- a/lib/blocktree/blocktree.go +++ b/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 } diff --git a/lib/grandpa/errors.go b/lib/grandpa/errors.go index 801e46d03f..97d21d0bc6 100644 --- a/lib/grandpa/errors.go +++ b/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") diff --git a/lib/grandpa/message_handler.go b/lib/grandpa/message_handler.go index dc509d2569..4051fe3bda 100644 --- a/lib/grandpa/message_handler.go +++ b/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,44 +404,52 @@ 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, @@ -449,7 +457,7 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt 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 { diff --git a/lib/grandpa/message_handler_integration_test.go b/lib/grandpa/message_handler_integration_test.go index 814c0344f0..f58f98a481 100644 --- a/lib/grandpa/message_handler_integration_test.go +++ b/lib/grandpa/message_handler_integration_test.go @@ -13,7 +13,6 @@ import ( "github.com/ChainSafe/gossamer/dot/state" "github.com/ChainSafe/gossamer/dot/types" - "github.com/ChainSafe/gossamer/lib/blocktree" "github.com/ChainSafe/gossamer/lib/common" "github.com/ChainSafe/gossamer/lib/crypto/ed25519" "github.com/ChainSafe/gossamer/lib/keystore" @@ -293,8 +292,6 @@ func TestMessageHandler_VerifyJustification_InvalidSig(t *testing.T) { } func TestMessageHandler_CommitMessage_NoCatchUpRequest_ValidSig(t *testing.T) { - t.Parallel() - kr, err := keystore.NewEd25519Keyring() require.NoError(t, err) aliceKeyPair := kr.Alice().(*ed25519.Keypair) @@ -658,8 +655,6 @@ func TestMessageHandler_VerifyPreVoteJustification(t *testing.T) { } func TestMessageHandler_VerifyPreCommitJustification(t *testing.T) { - t.Parallel() - kr, err := keystore.NewEd25519Keyring() require.NoError(t, err) aliceKeyPair := kr.Alice().(*ed25519.Keypair) @@ -736,8 +731,6 @@ func TestMessageHandler_HandleCatchUpResponse(t *testing.T) { } func TestMessageHandler_VerifyBlockJustification_WithEquivocatoryVotes(t *testing.T) { - t.Parallel() - kr, err := keystore.NewEd25519Keyring() require.NoError(t, err) aliceKeyPair := kr.Alice().(*ed25519.Keypair) @@ -797,13 +790,11 @@ func TestMessageHandler_VerifyBlockJustification_WithEquivocatoryVotes(t *testin just := newJustification(round, testHash, number, precommits) data, err := scale.Marshal(*just) require.NoError(t, err) - returnedJust, err := gs.VerifyBlockJustification(testHash, data) + err = gs.VerifyBlockJustification(testHash, data) require.NoError(t, err) - require.Equal(t, data, returnedJust) } func TestMessageHandler_VerifyBlockJustification(t *testing.T) { - t.Parallel() kr, err := keystore.NewEd25519Keyring() require.NoError(t, err) @@ -836,37 +827,51 @@ func TestMessageHandler_VerifyBlockJustification(t *testing.T) { err = st.Block.AddBlock(block) require.NoError(t, err) + digest2 := types.NewDigest() + prd2, _ := types.NewBabeSecondaryPlainPreDigest(0, 2).ToPreRuntimeDigest() + digest2.Add(*prd2) + + testHeader2 := types.Header{ + ParentHash: testGenesisHeader.Hash(), + Number: 1, + Digest: digest2, + } + + block2 := &types.Block{ + Header: testHeader2, + Body: *body, + } + + err = st.Block.AddBlock(block2) + require.NoError(t, err) + + err = st.Block.SetHeader(&testHeader2) + require.NoError(t, err) + setID, err := st.Grandpa.IncrementSetID() require.NoError(t, err) require.Equal(t, uint64(1), setID) - genhash := st.Block.GenesisHash() - round := uint64(1) number := uint32(1) precommits := buildTestJustification(t, 2, round, setID, kr, precommit) just := newJustification(round, testHash, number, precommits) data, err := scale.Marshal(*just) require.NoError(t, err) - returnedJust, err := gs.VerifyBlockJustification(testHash, data) + err = gs.VerifyBlockJustification(testHash, data) require.NoError(t, err) - require.Equal(t, data, returnedJust) // use wrong hash, shouldn't verify precommits = buildTestJustification(t, 2, round+1, setID, kr, precommit) just = newJustification(round+1, testHash, number, precommits) - just.Commit.Precommits[0].Vote.Hash = genhash + just.Commit.Precommits[0].Vote.Hash = testHeader2.Hash() data, err = scale.Marshal(*just) require.NoError(t, err) - returnedJust, err = gs.VerifyBlockJustification(testHash, data) - require.NotNil(t, err) - require.Equal(t, blocktree.ErrEndNodeNotFound, err) - require.Nil(t, returnedJust) + err = gs.VerifyBlockJustification(testHash, data) + require.Equal(t, ErrPrecommitBlockMismatch, err) } func TestMessageHandler_VerifyBlockJustification_invalid(t *testing.T) { - t.Parallel() - kr, err := keystore.NewEd25519Keyring() require.NoError(t, err) aliceKeyPair := kr.Alice().(*ed25519.Keypair) @@ -912,38 +917,32 @@ func TestMessageHandler_VerifyBlockJustification_invalid(t *testing.T) { just.Commit.Precommits[0].Vote.Hash = genhash data, err := scale.Marshal(*just) require.NoError(t, err) - returnedJust, err := gs.VerifyBlockJustification(testHash, data) - require.NotNil(t, err) + err = gs.VerifyBlockJustification(testHash, data) require.Equal(t, ErrPrecommitBlockMismatch, err) - require.Nil(t, returnedJust) // use wrong round, shouldn't verify precommits = buildTestJustification(t, 2, round+1, setID, kr, precommit) just = newJustification(round+2, testHash, number, precommits) data, err = scale.Marshal(*just) require.NoError(t, err) - returnedJust, err = gs.VerifyBlockJustification(testHash, data) - require.NotNil(t, err) + err = gs.VerifyBlockJustification(testHash, data) require.Equal(t, ErrInvalidSignature, err) - require.Nil(t, returnedJust) // add authority not in set, shouldn't verify precommits = buildTestJustification(t, len(auths)+1, round+1, setID, kr, precommit) just = newJustification(round+1, testHash, number, precommits) data, err = scale.Marshal(*just) require.NoError(t, err) - returnedJust, err = gs.VerifyBlockJustification(testHash, data) + err = gs.VerifyBlockJustification(testHash, data) require.Equal(t, ErrAuthorityNotInSet, err) - require.Nil(t, returnedJust) // not enough signatures, shouldn't verify precommits = buildTestJustification(t, 1, round+1, setID, kr, precommit) just = newJustification(round+1, testHash, number, precommits) data, err = scale.Marshal(*just) require.NoError(t, err) - returnedJust, err = gs.VerifyBlockJustification(testHash, data) + err = gs.VerifyBlockJustification(testHash, data) require.Equal(t, ErrMinVotesNotMet, err) - require.Nil(t, returnedJust) // mismatch justification header and block header precommits = buildTestJustification(t, 1, round+1, setID, kr, precommit) @@ -951,7 +950,7 @@ func TestMessageHandler_VerifyBlockJustification_invalid(t *testing.T) { data, err = scale.Marshal(*just) require.NoError(t, err) otherHeader := types.NewEmptyHeader() - _, err = gs.VerifyBlockJustification(otherHeader.Hash(), data) + err = gs.VerifyBlockJustification(otherHeader.Hash(), data) require.ErrorIs(t, err, ErrJustificationMismatch) expectedErr := fmt.Sprintf("%s: justification %s and block hash %s", ErrJustificationMismatch, @@ -960,6 +959,70 @@ func TestMessageHandler_VerifyBlockJustification_invalid(t *testing.T) { require.EqualError(t, err, expectedErr) } +func TestMessageHandler_VerifyBlockJustification_ErrFinalisedBlockMismatch(t *testing.T) { + t.Parallel() + + kr, err := keystore.NewEd25519Keyring() + require.NoError(t, err) + aliceKeyPair := kr.Alice().(*ed25519.Keypair) + + auths := []types.GrandpaVoter{ + { + Key: *kr.Alice().Public().(*ed25519.PublicKey), + }, + { + Key: *kr.Bob().Public().(*ed25519.PublicKey), + }, + { + Key: *kr.Charlie().Public().(*ed25519.PublicKey), + }, + } + + gs, st := newTestService(t, aliceKeyPair) + err = st.Grandpa.SetNextChange(auths, 1) + require.NoError(t, err) + + body, err := types.NewBodyFromBytes([]byte{0}) + require.NoError(t, err) + + block := &types.Block{ + Header: *testHeader, + Body: *body, + } + + err = st.Block.AddBlock(block) + require.NoError(t, err) + + setID := uint64(0) + round := uint64(1) + number := uint32(1) + + err = st.Block.SetFinalisedHash(block.Header.Hash(), round, setID) + require.NoError(t, err) + + var testHeader2 = &types.Header{ + ParentHash: testHeader.Hash(), + Number: 2, + Digest: newTestDigest(), + } + + testHash = testHeader2.Hash() + block2 := &types.Block{ + Header: *testHeader2, + Body: *body, + } + err = st.Block.AddBlock(block2) + require.NoError(t, err) + + // justification fails since there is already a block finalised in this round and set id + precommits := buildTestJustification(t, 18, round, setID, kr, precommit) + just := newJustification(round, testHash, number, precommits) + data, err := scale.Marshal(*just) + require.NoError(t, err) + err = gs.VerifyBlockJustification(testHash, data) + require.ErrorIs(t, err, errFinalisedBlocksMismatch) +} + func Test_getEquivocatoryVoters(t *testing.T) { t.Parallel() @@ -1426,9 +1489,7 @@ func signFakeFullVote( return sig } -func TestService_VerifyBlockJustification(t *testing.T) { - t.Parallel() - +func TestService_VerifyBlockJustification(t *testing.T) { //nolint kr, err := keystore.NewEd25519Keyring() require.NoError(t, err) @@ -1538,13 +1599,12 @@ func TestService_VerifyBlockJustification(t *testing.T) { blockState: tt.fields.blockStateBuilder(ctrl), grandpaState: tt.fields.grandpaStateBuilder(ctrl), } - got, err := s.VerifyBlockJustification(tt.args.hash, tt.args.justification) + err := s.VerifyBlockJustification(tt.args.hash, tt.args.justification) if tt.wantErr != nil { assert.ErrorContains(t, err, tt.wantErr.Error()) } else { require.NoError(t, err) } - assert.Equalf(t, tt.want, got, "VerifyBlockJustification(%v, %v)", tt.args.hash, tt.args.justification) }) } } diff --git a/lib/grandpa/mocks_test.go b/lib/grandpa/mocks_test.go index c3bfe8d942..55fedb5212 100644 --- a/lib/grandpa/mocks_test.go +++ b/lib/grandpa/mocks_test.go @@ -121,6 +121,21 @@ func (mr *MockBlockStateMockRecorder) GenesisHash() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GenesisHash", reflect.TypeOf((*MockBlockState)(nil).GenesisHash)) } +// GetFinalisedHash mocks base method. +func (m *MockBlockState) GetFinalisedHash(arg0, arg1 uint64) (common.Hash, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetFinalisedHash", arg0, arg1) + ret0, _ := ret[0].(common.Hash) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetFinalisedHash indicates an expected call of GetFinalisedHash. +func (mr *MockBlockStateMockRecorder) GetFinalisedHash(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetFinalisedHash", reflect.TypeOf((*MockBlockState)(nil).GetFinalisedHash), arg0, arg1) +} + // GetFinalisedHeader mocks base method. func (m *MockBlockState) GetFinalisedHeader(arg0, arg1 uint64) (*types.Header, error) { m.ctrl.T.Helper() diff --git a/lib/grandpa/state.go b/lib/grandpa/state.go index 6e9ea8598b..20bb968be3 100644 --- a/lib/grandpa/state.go +++ b/lib/grandpa/state.go @@ -22,7 +22,8 @@ type BlockState interface { IsDescendantOf(parent, child common.Hash) (bool, error) LowestCommonAncestor(a, b common.Hash) (common.Hash, error) HasFinalisedBlock(round, setID uint64) (bool, error) - GetFinalisedHeader(uint64, uint64) (*types.Header, error) + GetFinalisedHeader(round, setID uint64) (*types.Header, error) + GetFinalisedHash(round, setID uint64) (common.Hash, error) SetFinalisedHash(common.Hash, uint64, uint64) error BestBlockHeader() (*types.Header, error) GetHighestFinalisedHeader() (*types.Header, error)