From 4f870f7a72240778ba7bf5db2c9af14e92112f94 Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Wed, 6 Apr 2022 12:25:56 +0530 Subject: [PATCH 01/15] task(lib/grandpa): Verify block hashes and number Verify block hashes against block number in justification --- lib/grandpa/errors.go | 2 ++ lib/grandpa/message_handler.go | 47 ++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/lib/grandpa/errors.go b/lib/grandpa/errors.go index 54937cf1ef..d3e437eb80 100644 --- a/lib/grandpa/errors.go +++ b/lib/grandpa/errors.go @@ -63,6 +63,8 @@ var ( // ErrNoJustification is returned when no justification can be found for a block, ie. it has not been finalised ErrNoJustification = errors.New("no justification found for block") + ErrCommitBlockHashMismatch = errors.New("cannot verify block hash against block number") + // ErrMinVotesNotMet is returned when the number of votes is less than the required minimum in a Justification ErrMinVotesNotMet = errors.New("minimum number of votes not met in a Justification") diff --git a/lib/grandpa/message_handler.go b/lib/grandpa/message_handler.go index 3402e8e8d3..c8feee41b6 100644 --- a/lib/grandpa/message_handler.go +++ b/lib/grandpa/message_handler.go @@ -104,6 +104,11 @@ func (h *MessageHandler) handleNeighbourMessage(msg *NeighbourMessage) error { func (h *MessageHandler) handleCommitMessage(msg *CommitMessage) error { logger.Debugf("received commit message, msg: %+v", msg) + err := verifyBlockHashAgainstBlockNumber(h.blockState, msg.Vote.Hash, uint(msg.Vote.Number)) + if err != nil { + return err + } + containsPrecommitsSignedBy := make([]string, len(msg.AuthData)) for i, authData := range msg.AuthData { containsPrecommitsSignedBy[i] = authData.AuthorityID.String() @@ -184,6 +189,11 @@ func (h *MessageHandler) handleCatchUpResponse(msg *CatchUpResponse) error { "received catch up response with hash %s for round %d and set id %d", msg.Hash, msg.Round, msg.SetID) + err := verifyBlockHashAgainstBlockNumber(h.blockState, msg.Hash, uint(msg.Number)) + if err != nil { + return err + } + // TODO: re-add catch-up logic (#1531) if true { return nil @@ -330,6 +340,12 @@ func (h *MessageHandler) verifyPreVoteJustification(msg *CatchUpResponse) (commo voters := make(map[ed25519.PublicKeyBytes]map[common.Hash]int, len(msg.PreVoteJustification)) eqVotesByHash := make(map[common.Hash]map[ed25519.PublicKeyBytes]struct{}) + for _, pvj := range msg.PreVoteJustification { + err := verifyBlockHashAgainstBlockNumber(h.blockState, pvj.Vote.Hash, uint(pvj.Vote.Number)) + if err != nil { + return common.Hash{}, err + } + } // identify equivocatory votes by hash for _, justification := range msg.PreVoteJustification { hashsToCount, ok := voters[justification.AuthorityID] @@ -386,6 +402,14 @@ func (h *MessageHandler) verifyPreVoteJustification(msg *CatchUpResponse) (commo } func (h *MessageHandler) verifyPreCommitJustification(msg *CatchUpResponse) error { + + for _, pcj := range msg.PreCommitJustification { + err := verifyBlockHashAgainstBlockNumber(h.blockState, pcj.Vote.Hash, uint(pcj.Vote.Number)) + if err != nil { + return err + } + } + auths := make([]AuthData, len(msg.PreCommitJustification)) for i, pcj := range msg.PreCommitJustification { auths[i] = AuthData{AuthorityID: pcj.AuthorityID} @@ -472,6 +496,17 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt return err } + if err := verifyBlockHashAgainstBlockNumber(s.blockState, fj.Commit.Hash, uint(fj.Commit.Number)); err != nil { + return err + } + + for _, preCommit := range fj.Commit.Precommits { + err := verifyBlockHashAgainstBlockNumber(s.blockState, preCommit.Vote.Hash, uint(preCommit.Vote.Number)) + if err != nil { + return 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) @@ -573,6 +608,18 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt return nil } +func verifyBlockHashAgainstBlockNumber(bs BlockState, hash common.Hash, number uint) error { + blockHashFromBlockState, err := bs.GetHashByNumber(number) + if err != nil { + return fmt.Errorf("could not find block hash by block number: %w", err) + } + + if blockHashFromBlockState != hash { + return ErrCommitBlockHashMismatch + } + return nil +} + func isInAuthSet(auth *ed25519.PublicKey, set []types.GrandpaVoter) bool { for _, a := range set { if bytes.Equal(a.Key.Encode(), auth.Encode()) { From 89da3a118bc3d6fdbc01c1bb8226ee63cb426083 Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Wed, 6 Apr 2022 20:07:07 +0530 Subject: [PATCH 02/15] improvements --- dot/state/block_finalisation.go | 5 ++++- lib/grandpa/errors.go | 2 +- lib/grandpa/message_handler.go | 30 +++++++++++++++--------------- 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/dot/state/block_finalisation.go b/dot/state/block_finalisation.go index d7677bebc4..08342da62c 100644 --- a/dot/state/block_finalisation.go +++ b/dot/state/block_finalisation.go @@ -118,7 +118,10 @@ func (bs *BlockState) SetFinalisedHash(hash common.Hash, round, setID uint64) er bs.Lock() defer bs.Unlock() - has, _ := bs.HasHeader(hash) + has, err := bs.HasHeader(hash) + if err != nil { + return fmt.Errorf("could not check header for this hash: %w", err) + } if !has { return fmt.Errorf("cannot finalise unknown block %s", hash) } diff --git a/lib/grandpa/errors.go b/lib/grandpa/errors.go index d3e437eb80..9a1295f728 100644 --- a/lib/grandpa/errors.go +++ b/lib/grandpa/errors.go @@ -63,7 +63,7 @@ var ( // ErrNoJustification is returned when no justification can be found for a block, ie. it has not been finalised ErrNoJustification = errors.New("no justification found for block") - ErrCommitBlockHashMismatch = errors.New("cannot verify block hash against block number") + ErrBlockHashMismatch = errors.New("cannot verify block number against block hash") // ErrMinVotesNotMet is returned when the number of votes is less than the required minimum in a Justification ErrMinVotesNotMet = errors.New("minimum number of votes not met in a Justification") diff --git a/lib/grandpa/message_handler.go b/lib/grandpa/message_handler.go index c8feee41b6..890c6ac1e5 100644 --- a/lib/grandpa/message_handler.go +++ b/lib/grandpa/message_handler.go @@ -496,17 +496,6 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt return err } - if err := verifyBlockHashAgainstBlockNumber(s.blockState, fj.Commit.Hash, uint(fj.Commit.Number)); err != nil { - return err - } - - for _, preCommit := range fj.Commit.Precommits { - err := verifyBlockHashAgainstBlockNumber(s.blockState, preCommit.Vote.Hash, uint(preCommit.Vote.Number)) - if err != nil { - return 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) @@ -597,6 +586,17 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt return ErrMinVotesNotMet } + if err := verifyBlockHashAgainstBlockNumber(s.blockState, fj.Commit.Hash, uint(fj.Commit.Number)); err != nil { + return err + } + + for _, preCommit := range fj.Commit.Precommits { + err := verifyBlockHashAgainstBlockNumber(s.blockState, preCommit.Vote.Hash, uint(preCommit.Vote.Number)) + if err != nil { + return err + } + } + err = s.blockState.SetFinalisedHash(hash, fj.Round, setID) if err != nil { return err @@ -609,13 +609,13 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt } func verifyBlockHashAgainstBlockNumber(bs BlockState, hash common.Hash, number uint) error { - blockHashFromBlockState, err := bs.GetHashByNumber(number) + header, err := bs.GetHeader(hash) if err != nil { - return fmt.Errorf("could not find block hash by block number: %w", err) + return fmt.Errorf("could not get header from block hash: %w", err) } - if blockHashFromBlockState != hash { - return ErrCommitBlockHashMismatch + if header.Number != number { + return ErrBlockHashMismatch } return nil } From af3f62bb9887040d0f7e1976ee4865d6dd0e7d6a Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Tue, 12 Apr 2022 18:41:25 +0530 Subject: [PATCH 03/15] fixed tests --- dot/state/grandpa.go | 11 ++++---- lib/grandpa/errors.go | 2 +- lib/grandpa/grandpa.go | 2 ++ lib/grandpa/message_handler.go | 3 ++- lib/grandpa/message_handler_test.go | 42 ++++++++++++++++++++++------- 5 files changed, 44 insertions(+), 16 deletions(-) diff --git a/dot/state/grandpa.go b/dot/state/grandpa.go index 7b29387e25..f1ab4d8cc5 100644 --- a/dot/state/grandpa.go +++ b/dot/state/grandpa.go @@ -138,7 +138,8 @@ func (s *GrandpaState) GetLatestRound() (uint64, error) { return round, nil } -// SetNextChange sets the next authority change +// SetNextChange sets the next authority change at given block number. +// NOTE: This block number will be the last block in the current set and not part of the next set. func (s *GrandpaState) SetNextChange(authorities []types.GrandpaVoter, number uint) error { currSetID, err := s.GetCurrentSetID() if err != nil { @@ -180,7 +181,7 @@ func (s *GrandpaState) setSetIDChangeAtBlock(setID uint64, number uint) error { return s.db.Put(setIDChangeKey(setID), common.UintToBytes(number)) } -// GetSetIDChange returs the block number where the set ID was updated +// GetSetIDChange returns the block number where the set ID was updated func (s *GrandpaState) GetSetIDChange(setID uint64) (blockNumber uint, err error) { num, err := s.db.Get(setIDChangeKey(setID)) if err != nil { @@ -191,7 +192,7 @@ func (s *GrandpaState) GetSetIDChange(setID uint64) (blockNumber uint, err error } // GetSetIDByBlockNumber returns the set ID for a given block number -func (s *GrandpaState) GetSetIDByBlockNumber(num uint) (uint64, error) { +func (s *GrandpaState) GetSetIDByBlockNumber(blockNumber uint) (uint64, error) { curr, err := s.GetCurrentSetID() if err != nil { return 0, err @@ -217,11 +218,11 @@ func (s *GrandpaState) GetSetIDByBlockNumber(num uint) (uint64, error) { // if the given block number is greater or equal to the block number of the set ID change, // return the current set ID - if num <= changeUpper && num > changeLower { + if blockNumber <= changeUpper && blockNumber > changeLower { return curr, nil } - if num > changeUpper { + if blockNumber > changeUpper { return curr + 1, nil } diff --git a/lib/grandpa/errors.go b/lib/grandpa/errors.go index 9a1295f728..2525ab1bb3 100644 --- a/lib/grandpa/errors.go +++ b/lib/grandpa/errors.go @@ -63,7 +63,7 @@ var ( // ErrNoJustification is returned when no justification can be found for a block, ie. it has not been finalised ErrNoJustification = errors.New("no justification found for block") - ErrBlockHashMismatch = errors.New("cannot verify block number against block hash") + ErrBlockHashMismatch = errors.New("block hash does not belong to given block number") // ErrMinVotesNotMet is returned when the number of votes is less than the required minimum in a Justification ErrMinVotesNotMet = errors.New("minimum number of votes not met in a Justification") diff --git a/lib/grandpa/grandpa.go b/lib/grandpa/grandpa.go index ffc2a01d9c..00e2b3fc80 100644 --- a/lib/grandpa/grandpa.go +++ b/lib/grandpa/grandpa.go @@ -852,6 +852,8 @@ func (s *Service) finalise() error { return s.grandpaState.SetLatestRound(s.state.round) } +// TODO: Given that this functions returns []SignedVote and not Justification, it should be renamed +// accordingly. // createJustification collects the signed precommits received for this round and turns them into // a justification by adding all signed precommits that are for the best finalised candidate or // a descendent of the bfc diff --git a/lib/grandpa/message_handler.go b/lib/grandpa/message_handler.go index 890c6ac1e5..fcb710cd43 100644 --- a/lib/grandpa/message_handler.go +++ b/lib/grandpa/message_handler.go @@ -310,12 +310,13 @@ func (h *MessageHandler) verifyCommitMessageJustification(fm *CommitMessage) err err := h.verifyJustification(just, fm.Round, h.grandpa.state.setID, precommit) if err != nil { + logger.Errorf("could not verify justification: %s", err) continue } isDescendant, err := h.blockState.IsDescendantOf(fm.Vote.Hash, just.Vote.Hash) if err != nil { - logger.Warnf("verifyCommitMessageJustification: %s", err) + logger.Warnf("could not check for descendant: %s", err) continue } diff --git a/lib/grandpa/message_handler_test.go b/lib/grandpa/message_handler_test.go index 5e30d15a92..381575be60 100644 --- a/lib/grandpa/message_handler_test.go +++ b/lib/grandpa/message_handler_test.go @@ -250,7 +250,7 @@ func TestMessageHandler_VerifyJustification_InvalidSig(t *testing.T) { func TestMessageHandler_CommitMessage_NoCatchUpRequest_ValidSig(t *testing.T) { gs, st := newTestService(t) - round := uint64(77) + round := uint64(1) gs.state.round = round just := buildTestJustification(t, int(gs.state.threshold()), round, gs.state.setID, kr, precommit) err := st.Grandpa.SetPrecommits(round, gs.state.setID, just) @@ -505,6 +505,18 @@ func TestMessageHandler_VerifyPreVoteJustification(t *testing.T) { telemetryMock.EXPECT().SendMessage(gomock.Any()).AnyTimes() gs, st := newTestService(t) + + 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) + h := NewMessageHandler(gs, st.Block, telemetryMock) just := buildTestJustification(t, int(gs.state.threshold()), 1, gs.state.setID, kr, prevote) @@ -525,6 +537,18 @@ func TestMessageHandler_VerifyPreCommitJustification(t *testing.T) { telemetryMock.EXPECT().SendMessage(gomock.Any()).AnyTimes() gs, st := newTestService(t) + + 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) + h := NewMessageHandler(gs, st.Block, telemetryMock) round := uint64(1) @@ -537,7 +561,7 @@ func TestMessageHandler_VerifyPreCommitJustification(t *testing.T) { Number: uint32(round), } - err := h.verifyPreCommitJustification(msg) + err = h.verifyPreCommitJustification(msg) require.NoError(t, err) } @@ -553,7 +577,7 @@ func TestMessageHandler_HandleCatchUpResponse(t *testing.T) { h := NewMessageHandler(gs, st.Block, telemetryMock) - round := uint64(77) + round := uint64(1) gs.state.round = round + 1 pvJust := buildTestJustification(t, int(gs.state.threshold()), round, gs.state.setID, kr, prevote) @@ -605,7 +629,7 @@ func TestMessageHandler_VerifyBlockJustification_WithEquivocatoryVotes(t *testin } gs, st := newTestService(t) - err := st.Grandpa.SetNextChange(auths, 1) + err := st.Grandpa.SetNextChange(auths, 0) require.NoError(t, err) body, err := types.NewBodyFromBytes([]byte{0}) @@ -623,8 +647,8 @@ func TestMessageHandler_VerifyBlockJustification_WithEquivocatoryVotes(t *testin require.NoError(t, err) require.Equal(t, uint64(1), setID) - round := uint64(2) - number := uint32(2) + round := uint64(1) + number := uint32(1) precommits := buildTestJustification(t, 20, round, setID, kr, precommit) just := newJustification(round, testHash, number, precommits) data, err := scale.Marshal(*just) @@ -647,7 +671,7 @@ func TestMessageHandler_VerifyBlockJustification(t *testing.T) { } gs, st := newTestService(t) - err := st.Grandpa.SetNextChange(auths, 1) + err := st.Grandpa.SetNextChange(auths, 0) require.NoError(t, err) body, err := types.NewBodyFromBytes([]byte{0}) @@ -667,8 +691,8 @@ func TestMessageHandler_VerifyBlockJustification(t *testing.T) { genhash := st.Block.GenesisHash() - round := uint64(2) - number := uint32(2) + 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) From 5f252bba5649bc9809e6bdaa07b5881ed1a8bfc8 Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Thu, 14 Apr 2022 16:13:08 +0530 Subject: [PATCH 04/15] Update dot/state/block_finalisation.go Co-authored-by: Quentin McGaw --- dot/state/block_finalisation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dot/state/block_finalisation.go b/dot/state/block_finalisation.go index 08342da62c..cf867c9f45 100644 --- a/dot/state/block_finalisation.go +++ b/dot/state/block_finalisation.go @@ -120,7 +120,7 @@ func (bs *BlockState) SetFinalisedHash(hash common.Hash, round, setID uint64) er has, err := bs.HasHeader(hash) if err != nil { - return fmt.Errorf("could not check header for this hash: %w", err) + return fmt.Errorf("could not check header for hash %s: %w", hash, err) } if !has { return fmt.Errorf("cannot finalise unknown block %s", hash) From f15ef41fe3d165c20cc095c666d1d0b1d975fb4d Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Thu, 14 Apr 2022 16:13:15 +0530 Subject: [PATCH 05/15] Update dot/state/grandpa.go Co-authored-by: Quentin McGaw --- dot/state/grandpa.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dot/state/grandpa.go b/dot/state/grandpa.go index f1ab4d8cc5..b90dd1e8a6 100644 --- a/dot/state/grandpa.go +++ b/dot/state/grandpa.go @@ -138,7 +138,7 @@ func (s *GrandpaState) GetLatestRound() (uint64, error) { return round, nil } -// SetNextChange sets the next authority change at given block number. +// SetNextChange sets the next authority change at the given block number. // NOTE: This block number will be the last block in the current set and not part of the next set. func (s *GrandpaState) SetNextChange(authorities []types.GrandpaVoter, number uint) error { currSetID, err := s.GetCurrentSetID() From 0588a91fb67205a64b6967bf0cc5e7dfbcd50bb0 Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Thu, 14 Apr 2022 16:13:27 +0530 Subject: [PATCH 06/15] Update lib/grandpa/errors.go Co-authored-by: Quentin McGaw --- lib/grandpa/errors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/grandpa/errors.go b/lib/grandpa/errors.go index 2525ab1bb3..d7f8f5555b 100644 --- a/lib/grandpa/errors.go +++ b/lib/grandpa/errors.go @@ -63,7 +63,7 @@ var ( // ErrNoJustification is returned when no justification can be found for a block, ie. it has not been finalised ErrNoJustification = errors.New("no justification found for block") - ErrBlockHashMismatch = errors.New("block hash does not belong to given block number") + ErrBlockHashMismatch = errors.New("block hash does not correspond to given block number") // ErrMinVotesNotMet is returned when the number of votes is less than the required minimum in a Justification ErrMinVotesNotMet = errors.New("minimum number of votes not met in a Justification") From 58775347d6762d81e8326277660d9431e126d955 Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Thu, 14 Apr 2022 16:16:02 +0530 Subject: [PATCH 07/15] Update lib/grandpa/message_handler.go Co-authored-by: Quentin McGaw --- lib/grandpa/message_handler.go | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/grandpa/message_handler.go b/lib/grandpa/message_handler.go index fcb710cd43..e263cec458 100644 --- a/lib/grandpa/message_handler.go +++ b/lib/grandpa/message_handler.go @@ -403,7 +403,6 @@ func (h *MessageHandler) verifyPreVoteJustification(msg *CatchUpResponse) (commo } func (h *MessageHandler) verifyPreCommitJustification(msg *CatchUpResponse) error { - for _, pcj := range msg.PreCommitJustification { err := verifyBlockHashAgainstBlockNumber(h.blockState, pcj.Vote.Hash, uint(pcj.Vote.Number)) if err != nil { From e5dcca322ebd5eb050e4c12d8089ffe553b99d7f Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Thu, 14 Apr 2022 16:16:33 +0530 Subject: [PATCH 08/15] Update lib/grandpa/message_handler.go Co-authored-by: Quentin McGaw --- lib/grandpa/message_handler.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/grandpa/message_handler.go b/lib/grandpa/message_handler.go index e263cec458..4f2c5c0b17 100644 --- a/lib/grandpa/message_handler.go +++ b/lib/grandpa/message_handler.go @@ -615,7 +615,8 @@ func verifyBlockHashAgainstBlockNumber(bs BlockState, hash common.Hash, number u } if header.Number != number { - return ErrBlockHashMismatch + return fmt.Errorf("%w: expected number %d from header but got number %d", + ErrBlockHashMismatch, header.Number, number) } return nil } From 73c7c51bb9c190e9933cb84c1027bfee154b9a98 Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Thu, 14 Apr 2022 19:24:44 +0530 Subject: [PATCH 09/15] addressed elizabeth's comments --- dot/state/block.go | 3 +-- lib/grandpa/message_handler.go | 20 +++++++++++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/dot/state/block.go b/dot/state/block.go index 0ce071a6e4..5ac6233200 100644 --- a/dot/state/block.go +++ b/dot/state/block.go @@ -205,8 +205,6 @@ func (bs *BlockState) GetHeader(hash common.Hash) (header *types.Header, err err return header, nil } - result := types.NewEmptyHeader() - if bs.db == nil { return nil, fmt.Errorf("database is nil") } @@ -220,6 +218,7 @@ func (bs *BlockState) GetHeader(hash common.Hash) (header *types.Header, err err return nil, err } + result := types.NewEmptyHeader() err = scale.Unmarshal(data, result) if err != nil { return nil, err diff --git a/lib/grandpa/message_handler.go b/lib/grandpa/message_handler.go index 4f2c5c0b17..f1bcbd9572 100644 --- a/lib/grandpa/message_handler.go +++ b/lib/grandpa/message_handler.go @@ -9,6 +9,7 @@ import ( "fmt" "reflect" + "github.com/ChainSafe/chaindb" "github.com/ChainSafe/gossamer/dot/network" "github.com/ChainSafe/gossamer/dot/telemetry" "github.com/ChainSafe/gossamer/dot/types" @@ -105,6 +106,12 @@ func (h *MessageHandler) handleCommitMessage(msg *CommitMessage) error { logger.Debugf("received commit message, msg: %+v", msg) err := verifyBlockHashAgainstBlockNumber(h.blockState, msg.Vote.Hash, uint(msg.Vote.Number)) + if errors.Is(err, chaindb.ErrKeyNotFound) { + h.grandpa.tracker.addCommit(msg) + logger.Infof("we might not have synced to the given block %s yet: %s", msg.Vote.Hash, err) + return nil + } + if err != nil { return err } @@ -190,6 +197,11 @@ func (h *MessageHandler) handleCatchUpResponse(msg *CatchUpResponse) error { msg.Hash, msg.Round, msg.SetID) err := verifyBlockHashAgainstBlockNumber(h.blockState, msg.Hash, uint(msg.Number)) + if errors.Is(err, chaindb.ErrKeyNotFound) { + h.grandpa.tracker.addCatchUpResponse(msg) + logger.Infof("we might not have synced to the given block %s yet: %s", msg.Hash, err) + return nil + } if err != nil { return err } @@ -215,10 +227,16 @@ func (h *MessageHandler) handleCatchUpResponse(msg *CatchUpResponse) error { prevote, err := h.verifyPreVoteJustification(msg) if err != nil { + // no need to check for chaindb.ErrKeyNotFound + // Since highest block was present (otherwise execution would not reach here), + // we expect previous blocks to exists as well. If they don't exist, we should error. return err } if err = h.verifyPreCommitJustification(msg); err != nil { + // no need to check for chaindb.ErrKeyNotFound + // Since highest block was present (otherwise execution would not reach here), + // we expect previous blocks to exists as well. If they don't exist, we should error. return err } @@ -615,7 +633,7 @@ func verifyBlockHashAgainstBlockNumber(bs BlockState, hash common.Hash, number u } if header.Number != number { - return fmt.Errorf("%w: expected number %d from header but got number %d", + return fmt.Errorf("%w: expected number %d from header but got number %d", ErrBlockHashMismatch, header.Number, number) } return nil From 246c510b60eab0c5b6b4bcbe5f4938651cdf14f0 Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Fri, 15 Apr 2022 11:19:29 +0530 Subject: [PATCH 10/15] more fixes --- lib/grandpa/message_handler.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/grandpa/message_handler.go b/lib/grandpa/message_handler.go index f1bcbd9572..19c8dee294 100644 --- a/lib/grandpa/message_handler.go +++ b/lib/grandpa/message_handler.go @@ -227,16 +227,10 @@ func (h *MessageHandler) handleCatchUpResponse(msg *CatchUpResponse) error { prevote, err := h.verifyPreVoteJustification(msg) if err != nil { - // no need to check for chaindb.ErrKeyNotFound - // Since highest block was present (otherwise execution would not reach here), - // we expect previous blocks to exists as well. If they don't exist, we should error. return err } if err = h.verifyPreCommitJustification(msg); err != nil { - // no need to check for chaindb.ErrKeyNotFound - // Since highest block was present (otherwise execution would not reach here), - // we expect previous blocks to exists as well. If they don't exist, we should error. return err } @@ -361,6 +355,11 @@ func (h *MessageHandler) verifyPreVoteJustification(msg *CatchUpResponse) (commo for _, pvj := range msg.PreVoteJustification { err := verifyBlockHashAgainstBlockNumber(h.blockState, pvj.Vote.Hash, uint(pvj.Vote.Number)) + if errors.Is(err, chaindb.ErrKeyNotFound) { + h.grandpa.tracker.addCatchUpResponse(msg) + logger.Infof("we might not have synced to the given block %s yet: %s", pvj.Vote.Hash, err) + continue + } if err != nil { return common.Hash{}, err } @@ -423,6 +422,11 @@ func (h *MessageHandler) verifyPreVoteJustification(msg *CatchUpResponse) (commo func (h *MessageHandler) verifyPreCommitJustification(msg *CatchUpResponse) error { for _, pcj := range msg.PreCommitJustification { err := verifyBlockHashAgainstBlockNumber(h.blockState, pcj.Vote.Hash, uint(pcj.Vote.Number)) + if errors.Is(err, chaindb.ErrKeyNotFound) { + h.grandpa.tracker.addCatchUpResponse(msg) + logger.Infof("we might not have synced to the given block %s yet: %s", pcj.Vote.Hash, err) + continue + } if err != nil { return err } From 77e4fb014d29ea68787eb976e9f3c883d3658356 Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Mon, 18 Apr 2022 21:05:56 +0530 Subject: [PATCH 11/15] removed comment --- lib/grandpa/grandpa.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/grandpa/grandpa.go b/lib/grandpa/grandpa.go index 00e2b3fc80..ffc2a01d9c 100644 --- a/lib/grandpa/grandpa.go +++ b/lib/grandpa/grandpa.go @@ -852,8 +852,6 @@ func (s *Service) finalise() error { return s.grandpaState.SetLatestRound(s.state.round) } -// TODO: Given that this functions returns []SignedVote and not Justification, it should be renamed -// accordingly. // createJustification collects the signed precommits received for this round and turns them into // a justification by adding all signed precommits that are for the best finalised candidate or // a descendent of the bfc From 55ce1f047e7ac8d1a9e4aad0774431e328ad8015 Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Tue, 19 Apr 2022 17:57:37 +0530 Subject: [PATCH 12/15] split error handling --- lib/grandpa/message_handler.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/grandpa/message_handler.go b/lib/grandpa/message_handler.go index 19c8dee294..3102ebc812 100644 --- a/lib/grandpa/message_handler.go +++ b/lib/grandpa/message_handler.go @@ -608,7 +608,8 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt return ErrMinVotesNotMet } - if err := verifyBlockHashAgainstBlockNumber(s.blockState, fj.Commit.Hash, uint(fj.Commit.Number)); err != nil { + err = verifyBlockHashAgainstBlockNumber(s.blockState, fj.Commit.Hash, uint(fj.Commit.Number)) + if err != nil { return err } From 7b9d8769a406931e1a1cf02423f219f2002b37db Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Fri, 22 Apr 2022 18:40:04 +0530 Subject: [PATCH 13/15] Update lib/grandpa/message_handler.go Co-authored-by: Timothy Wu --- lib/grandpa/message_handler.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/grandpa/message_handler.go b/lib/grandpa/message_handler.go index 3102ebc812..ccd214864d 100644 --- a/lib/grandpa/message_handler.go +++ b/lib/grandpa/message_handler.go @@ -106,16 +106,17 @@ func (h *MessageHandler) handleCommitMessage(msg *CommitMessage) error { logger.Debugf("received commit message, msg: %+v", msg) err := verifyBlockHashAgainstBlockNumber(h.blockState, msg.Vote.Hash, uint(msg.Vote.Number)) - if errors.Is(err, chaindb.ErrKeyNotFound) { - h.grandpa.tracker.addCommit(msg) - logger.Infof("we might not have synced to the given block %s yet: %s", msg.Vote.Hash, err) - return nil - } - + err := verifyBlockHashAgainstBlockNumber(h.blockState, msg.Vote.Hash, uint(msg.Vote.Number)) if err != nil { + if errors.Is(err, chaindb.ErrKeyNotFound) { + h.grandpa.tracker.addCommit(msg) + logger.Infof("we might not have synced to the given block %s yet: %s", msg.Vote.Hash, err) + return nil + } return err } + containsPrecommitsSignedBy := make([]string, len(msg.AuthData)) for i, authData := range msg.AuthData { containsPrecommitsSignedBy[i] = authData.AuthorityID.String() From 26911c6ebcbc1b5a2cfe75d18eb6ceee5d227c1f Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Fri, 22 Apr 2022 18:43:17 +0530 Subject: [PATCH 14/15] addressed reviews --- lib/grandpa/message_handler.go | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/lib/grandpa/message_handler.go b/lib/grandpa/message_handler.go index ccd214864d..ec4e78c8a3 100644 --- a/lib/grandpa/message_handler.go +++ b/lib/grandpa/message_handler.go @@ -105,7 +105,6 @@ func (h *MessageHandler) handleNeighbourMessage(msg *NeighbourMessage) error { func (h *MessageHandler) handleCommitMessage(msg *CommitMessage) error { logger.Debugf("received commit message, msg: %+v", msg) - err := verifyBlockHashAgainstBlockNumber(h.blockState, msg.Vote.Hash, uint(msg.Vote.Number)) err := verifyBlockHashAgainstBlockNumber(h.blockState, msg.Vote.Hash, uint(msg.Vote.Number)) if err != nil { if errors.Is(err, chaindb.ErrKeyNotFound) { @@ -116,7 +115,6 @@ func (h *MessageHandler) handleCommitMessage(msg *CommitMessage) error { return err } - containsPrecommitsSignedBy := make([]string, len(msg.AuthData)) for i, authData := range msg.AuthData { containsPrecommitsSignedBy[i] = authData.AuthorityID.String() @@ -198,12 +196,12 @@ func (h *MessageHandler) handleCatchUpResponse(msg *CatchUpResponse) error { msg.Hash, msg.Round, msg.SetID) err := verifyBlockHashAgainstBlockNumber(h.blockState, msg.Hash, uint(msg.Number)) - if errors.Is(err, chaindb.ErrKeyNotFound) { - h.grandpa.tracker.addCatchUpResponse(msg) - logger.Infof("we might not have synced to the given block %s yet: %s", msg.Hash, err) - return nil - } if err != nil { + if errors.Is(err, chaindb.ErrKeyNotFound) { + h.grandpa.tracker.addCatchUpResponse(msg) + logger.Infof("we might not have synced to the given block %s yet: %s", msg.Hash, err) + return nil + } return err } @@ -356,12 +354,12 @@ func (h *MessageHandler) verifyPreVoteJustification(msg *CatchUpResponse) (commo for _, pvj := range msg.PreVoteJustification { err := verifyBlockHashAgainstBlockNumber(h.blockState, pvj.Vote.Hash, uint(pvj.Vote.Number)) - if errors.Is(err, chaindb.ErrKeyNotFound) { - h.grandpa.tracker.addCatchUpResponse(msg) - logger.Infof("we might not have synced to the given block %s yet: %s", pvj.Vote.Hash, err) - continue - } if err != nil { + if errors.Is(err, chaindb.ErrKeyNotFound) { + h.grandpa.tracker.addCatchUpResponse(msg) + logger.Infof("we might not have synced to the given block %s yet: %s", pvj.Vote.Hash, err) + continue + } return common.Hash{}, err } } @@ -423,12 +421,12 @@ func (h *MessageHandler) verifyPreVoteJustification(msg *CatchUpResponse) (commo func (h *MessageHandler) verifyPreCommitJustification(msg *CatchUpResponse) error { for _, pcj := range msg.PreCommitJustification { err := verifyBlockHashAgainstBlockNumber(h.blockState, pcj.Vote.Hash, uint(pcj.Vote.Number)) - if errors.Is(err, chaindb.ErrKeyNotFound) { - h.grandpa.tracker.addCatchUpResponse(msg) - logger.Infof("we might not have synced to the given block %s yet: %s", pcj.Vote.Hash, err) - continue - } if err != nil { + if errors.Is(err, chaindb.ErrKeyNotFound) { + h.grandpa.tracker.addCatchUpResponse(msg) + logger.Infof("we might not have synced to the given block %s yet: %s", pcj.Vote.Hash, err) + continue + } return err } } From cb620da6466b72b7900e843cab6f43cca04e7a4b Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Tue, 26 Apr 2022 14:26:09 +0530 Subject: [PATCH 15/15] added a comment --- dot/state/grandpa.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/dot/state/grandpa.go b/dot/state/grandpa.go index b90dd1e8a6..32850e9dec 100644 --- a/dot/state/grandpa.go +++ b/dot/state/grandpa.go @@ -216,8 +216,11 @@ func (s *GrandpaState) GetSetIDByBlockNumber(blockNumber uint) (uint64, error) { return 0, err } - // if the given block number is greater or equal to the block number of the set ID change, - // return the current set ID + // Set id changes at the last block in the set. So, block (changeLower) at which current + // set id was set, does not belong to current set. Thus, all block numbers in given set + // would be more than changeLower. + // Next set id change happens at the last block of current set. Thus, a block number from + // given set could be lower or equal to changeUpper. if blockNumber <= changeUpper && blockNumber > changeLower { return curr, nil }