From f3376241b48670140c8eabb34a5c446156d86714 Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Tue, 26 Apr 2022 18:38:11 +0530 Subject: [PATCH 1/4] fix(lib/grandpa): check equivocatory votes count A valid justification can't have more than two equivocatory votes per voter. Check the number of equivocatory votes per voter and throw an error if more than two. Fixes #2401 --- lib/grandpa/errors.go | 1 + lib/grandpa/message_handler.go | 26 +++++++++++++++++++------- lib/grandpa/message_handler_test.go | 19 +++++++++++-------- 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/lib/grandpa/errors.go b/lib/grandpa/errors.go index d7f8f5555b..705316dc8f 100644 --- a/lib/grandpa/errors.go +++ b/lib/grandpa/errors.go @@ -98,4 +98,5 @@ var ( errVoteToSignatureMismatch = errors.New("votes and authority count mismatch") errInvalidVoteBlock = errors.New("block in vote is not descendant of previously finalised block") errVoteFromSelf = errors.New("got vote from ourselves") + errInvalidMultiplicity = errors.New("more than two equivocatory votes for a voter") ) diff --git a/lib/grandpa/message_handler.go b/lib/grandpa/message_handler.go index ec4e78c8a3..750507908b 100644 --- a/lib/grandpa/message_handler.go +++ b/lib/grandpa/message_handler.go @@ -284,19 +284,21 @@ func (h *MessageHandler) verifyCatchUpResponseCompletability(prevote, precommit return nil } -func getEquivocatoryVoters(votes []AuthData) map[ed25519.PublicKeyBytes]struct{} { +func getEquivocatoryVoters(votes []AuthData) (map[ed25519.PublicKeyBytes]struct{}, error) { eqvVoters := make(map[ed25519.PublicKeyBytes]struct{}) voters := make(map[ed25519.PublicKeyBytes]int, len(votes)) for _, v := range votes { voters[v.AuthorityID]++ + if voters[v.AuthorityID] > 2 { + return nil, errInvalidMultiplicity + } - if voters[v.AuthorityID] > 1 { + if voters[v.AuthorityID] == 2 { eqvVoters[v.AuthorityID] = struct{}{} } } - - return eqvVoters + return eqvVoters, nil } func (h *MessageHandler) verifyCommitMessageJustification(fm *CommitMessage) error { @@ -304,7 +306,10 @@ func (h *MessageHandler) verifyCommitMessageJustification(fm *CommitMessage) err return ErrPrecommitSignatureMismatch } - eqvVoters := getEquivocatoryVoters(fm.AuthData) + eqvVoters, err := getEquivocatoryVoters(fm.AuthData) + if err != nil { + return fmt.Errorf("could not get valid equivocatory votes: %w", err) + } var count int for i, pc := range fm.Precommits { @@ -436,7 +441,10 @@ func (h *MessageHandler) verifyPreCommitJustification(msg *CatchUpResponse) erro auths[i] = AuthData{AuthorityID: pcj.AuthorityID} } - eqvVoters := getEquivocatoryVoters(auths) + eqvVoters, err := getEquivocatoryVoters(auths) + if err != nil { + return fmt.Errorf("could not get valid equivocatory votes: %w", err) + } // verify pre-commit justification var count uint64 @@ -549,7 +557,11 @@ func (s *Service) VerifyBlockJustification(hash common.Hash, justification []byt authPubKeys[i] = AuthData{AuthorityID: pcj.AuthorityID} } - equivocatoryVoters := getEquivocatoryVoters(authPubKeys) + equivocatoryVoters, err := getEquivocatoryVoters(authPubKeys) + if err != nil { + return fmt.Errorf("could not get valid equivocatory votes: %w", err) + } + var count int logger.Debugf( diff --git a/lib/grandpa/message_handler_test.go b/lib/grandpa/message_handler_test.go index 381575be60..1ded0bb5e1 100644 --- a/lib/grandpa/message_handler_test.go +++ b/lib/grandpa/message_handler_test.go @@ -649,7 +649,7 @@ func TestMessageHandler_VerifyBlockJustification_WithEquivocatoryVotes(t *testin round := uint64(1) number := uint32(1) - precommits := buildTestJustification(t, 20, round, setID, kr, precommit) + precommits := buildTestJustification(t, 18, round, setID, kr, precommit) just := newJustification(round, testHash, number, precommits) data, err := scale.Marshal(*just) require.NoError(t, err) @@ -788,13 +788,11 @@ func Test_getEquivocatoryVoters(t *testing.T) { ed25519Keyring, err := keystore.NewEd25519Keyring() require.NoError(t, err) fakeAuthorities := []*ed25519.Keypair{ - ed25519Keyring.Alice().(*ed25519.Keypair), ed25519Keyring.Alice().(*ed25519.Keypair), ed25519Keyring.Alice().(*ed25519.Keypair), ed25519Keyring.Bob().(*ed25519.Keypair), ed25519Keyring.Charlie().(*ed25519.Keypair), ed25519Keyring.Charlie().(*ed25519.Keypair), - ed25519Keyring.Charlie().(*ed25519.Keypair), ed25519Keyring.Dave().(*ed25519.Keypair), ed25519Keyring.Dave().(*ed25519.Keypair), ed25519Keyring.Eve().(*ed25519.Keypair), @@ -813,8 +811,17 @@ func Test_getEquivocatoryVoters(t *testing.T) { } } - eqv := getEquivocatoryVoters(authData) + eqv, err := getEquivocatoryVoters(authData) + require.NoError(t, err) require.Len(t, eqv, 5) + + // test that getEquivocatoryVoters if a voter has more than two equivocatory votes + authData = append(authData, AuthData{ + AuthorityID: ed25519Keyring.Alice().Public().(*ed25519.PublicKey).AsBytes(), + }) + + _, err = getEquivocatoryVoters(authData) + require.ErrorIs(t, err, errInvalidMultiplicity) } func Test_VerifyCommitMessageJustification_ShouldRemoveEquivocatoryVotes(t *testing.T) { @@ -838,13 +845,11 @@ func Test_VerifyCommitMessageJustification_ShouldRemoveEquivocatoryVotes(t *test ed25519Keyring, err := keystore.NewEd25519Keyring() require.NoError(t, err) fakeAuthorities := []*ed25519.Keypair{ - ed25519Keyring.Alice().(*ed25519.Keypair), ed25519Keyring.Alice().(*ed25519.Keypair), ed25519Keyring.Alice().(*ed25519.Keypair), ed25519Keyring.Bob().(*ed25519.Keypair), ed25519Keyring.Charlie().(*ed25519.Keypair), ed25519Keyring.Charlie().(*ed25519.Keypair), - ed25519Keyring.Charlie().(*ed25519.Keypair), ed25519Keyring.Dave().(*ed25519.Keypair), ed25519Keyring.Dave().(*ed25519.Keypair), ed25519Keyring.Eve().(*ed25519.Keypair), @@ -989,13 +994,11 @@ func Test_VerifyPreCommitJustification(t *testing.T) { // total of votes 4 legit + 3 equivocatory // the threshold for testing is 9, so 2/3 of 9 = 6 fakeAuthorities := []*ed25519.Keypair{ - ed25519Keyring.Alice().(*ed25519.Keypair), ed25519Keyring.Alice().(*ed25519.Keypair), ed25519Keyring.Alice().(*ed25519.Keypair), ed25519Keyring.Bob().(*ed25519.Keypair), ed25519Keyring.Charlie().(*ed25519.Keypair), ed25519Keyring.Charlie().(*ed25519.Keypair), - ed25519Keyring.Charlie().(*ed25519.Keypair), ed25519Keyring.Dave().(*ed25519.Keypair), ed25519Keyring.Dave().(*ed25519.Keypair), ed25519Keyring.Eve().(*ed25519.Keypair), From e2538be6b8d693e91cd4b56f7d956b4f40854509 Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Tue, 26 Apr 2022 20:58:18 +0530 Subject: [PATCH 2/4] Update lib/grandpa/message_handler_test.go Co-authored-by: Quentin McGaw --- lib/grandpa/message_handler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/grandpa/message_handler_test.go b/lib/grandpa/message_handler_test.go index 1ded0bb5e1..73d10568ef 100644 --- a/lib/grandpa/message_handler_test.go +++ b/lib/grandpa/message_handler_test.go @@ -815,7 +815,7 @@ func Test_getEquivocatoryVoters(t *testing.T) { require.NoError(t, err) require.Len(t, eqv, 5) - // test that getEquivocatoryVoters if a voter has more than two equivocatory votes + // test that getEquivocatoryVoters returns an error if a voter has more than two equivocatory votes authData = append(authData, AuthData{ AuthorityID: ed25519Keyring.Alice().Public().(*ed25519.PublicKey).AsBytes(), }) From cd39891bcf1a1ebc15b6f9767ebac18a1fb389d9 Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Wed, 27 Apr 2022 13:10:35 +0530 Subject: [PATCH 3/4] 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 750507908b..94681f045b 100644 --- a/lib/grandpa/message_handler.go +++ b/lib/grandpa/message_handler.go @@ -291,7 +291,8 @@ func getEquivocatoryVoters(votes []AuthData) (map[ed25519.PublicKeyBytes]struct{ for _, v := range votes { voters[v.AuthorityID]++ if voters[v.AuthorityID] > 2 { - return nil, errInvalidMultiplicity + return nil, fmt.Errorf("%w: authority id %x has %d votes", + errInvalidMultiplicity, v.AuthorityID, voters[v.AuthorityID]) } if voters[v.AuthorityID] == 2 { From c337f3121b359fede06ef4242afea6bd4b70ded8 Mon Sep 17 00:00:00 2001 From: Kishan Sagathiya Date: Tue, 3 May 2022 11:27:44 +0530 Subject: [PATCH 4/4] addressed comments --- lib/grandpa/message_handler.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/grandpa/message_handler.go b/lib/grandpa/message_handler.go index 750507908b..f0a7662326 100644 --- a/lib/grandpa/message_handler.go +++ b/lib/grandpa/message_handler.go @@ -290,12 +290,13 @@ func getEquivocatoryVoters(votes []AuthData) (map[ed25519.PublicKeyBytes]struct{ for _, v := range votes { voters[v.AuthorityID]++ - if voters[v.AuthorityID] > 2 { - return nil, errInvalidMultiplicity - } - - if voters[v.AuthorityID] == 2 { + switch voters[v.AuthorityID] { + case 1: + case 2: eqvVoters[v.AuthorityID] = struct{}{} + default: + return nil, fmt.Errorf("%w: authority id %x has %d votes", + errInvalidMultiplicity, v.AuthorityID, voters[v.AuthorityID]) } } return eqvVoters, nil @@ -308,7 +309,7 @@ func (h *MessageHandler) verifyCommitMessageJustification(fm *CommitMessage) err eqvVoters, err := getEquivocatoryVoters(fm.AuthData) if err != nil { - return fmt.Errorf("could not get valid equivocatory votes: %w", err) + return fmt.Errorf("could not get valid equivocatory voters: %w", err) } var count int @@ -443,7 +444,7 @@ func (h *MessageHandler) verifyPreCommitJustification(msg *CatchUpResponse) erro eqvVoters, err := getEquivocatoryVoters(auths) if err != nil { - return fmt.Errorf("could not get valid equivocatory votes: %w", err) + return fmt.Errorf("could not get valid equivocatory voters: %w", err) } // verify pre-commit justification @@ -559,7 +560,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 votes: %w", err) + return fmt.Errorf("could not get valid equivocatory voters: %w", err) } var count int