Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(lib/grandpa): check equivocatory votes count #2497

Merged
merged 5 commits into from
May 3, 2022
Merged

fix(lib/grandpa): check equivocatory votes count #2497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f337624
fix(lib/grandpa): check equivocatory votes count
kishansagathiya Apr 26, 2022
e2538be
Update lib/grandpa/message_handler_test.go
kishansagathiya Apr 26, 2022
cd39891
Update lib/grandpa/message_handler.go
kishansagathiya Apr 27, 2022
c337f31
addressed comments
kishansagathiya May 3, 2022
7ddd666
Merge branch 'kishan/fix/equivocatory-vote-multiplicity' of github.co…
kishansagathiya May 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/grandpa/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
)
29 changes: 21 additions & 8 deletions lib/grandpa/message_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,27 +284,33 @@ 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit named return values would be nice

Suggested change
func getEquivocatoryVoters(votes []AuthData) (map[ed25519.PublicKeyBytes]struct{}, error) {
func getEquivocatoryVoters(votes []AuthData) (eqvVoters map[ed25519.PublicKeyBytes]struct{}, err 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] > 1 {
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
return eqvVoters, nil
}

func (h *MessageHandler) verifyCommitMessageJustification(fm *CommitMessage) error {
if len(fm.Precommits) != len(fm.AuthData) {
return ErrPrecommitSignatureMismatch
}

eqvVoters := getEquivocatoryVoters(fm.AuthData)
eqvVoters, err := getEquivocatoryVoters(fm.AuthData)
if err != nil {
return fmt.Errorf("could not get valid equivocatory voters: %w", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this wrapper is not needed as the getEquivocatoryVoters has enough wrapper

}

var count int
for i, pc := range fm.Precommits {
Expand Down Expand Up @@ -436,7 +442,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 voters: %w", err)
}

// verify pre-commit justification
var count uint64
Expand Down Expand Up @@ -549,7 +558,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 voters: %w", err)
}

var count int

logger.Debugf(
Expand Down
19 changes: 11 additions & 8 deletions lib/grandpa/message_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
kishansagathiya marked this conversation as resolved.
Show resolved Hide resolved
just := newJustification(round, testHash, number, precommits)
data, err := scale.Marshal(*just)
require.NoError(t, err)
Expand Down Expand Up @@ -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),
Expand All @@ -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 returns an error 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) {
Expand All @@ -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),
Expand Down Expand Up @@ -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),
Expand Down