From 81647e0884e6e97b2ef4d5efd57c40ebbd97e514 Mon Sep 17 00:00:00 2001 From: mariko Date: Mon, 29 Mar 2021 13:03:59 +0900 Subject: [PATCH 1/4] fix: Address to the problem that the signature of bls voter cannot be verified --- types/block.go | 33 +++++++++++++++++++++++++++------ types/vote_set.go | 34 ++++++++++++++++++++++++++++------ 2 files changed, 55 insertions(+), 12 deletions(-) diff --git a/types/block.go b/types/block.go index 1d6e00fd1..0c834444e 100644 --- a/types/block.go +++ b/types/block.go @@ -761,17 +761,38 @@ func (commit *Commit) MaxCommitBytes() int64 { // Panics if signatures from the commit can't be added to the voteset. // Inverse of VoteSet.MakeCommit(). func CommitToVoteSet(chainID string, commit *Commit, voters *VoterSet) *VoteSet { - if commit.AggregatedSignature != nil { - panic("Aggregated commit cannot make a VoteSet") - } voteSet := NewVoteSet(chainID, commit.Height, commit.Round, PrecommitType, voters) + var blsPubkeys []bls.PubKeyBLS12 + var msgs [][]byte for idx, commitSig := range commit.Signatures { if commitSig.Absent() { continue // OK, some precommits can be missing. } - added, err := voteSet.AddVote(commit.GetVote(idx)) - if !added || err != nil { - panic(fmt.Sprintf("Failed to reconstruct LastCommit: %v", err)) + vote := commit.GetVote(idx) + // if signature != nil + if vote.Signature != nil { + added, err := voteSet.AddVote(vote) + if !added || err != nil { + panic(fmt.Sprintf("Failed to reconstruct LastCommit: %v", err)) + } + } else { + added, err := voteSet.AddAggregatedVote(vote) + if !added || err != nil { + panic(fmt.Sprintf("Failed to reconstruct LastCommit: %v", err)) + } + _, voter := voters.GetByIndex(vote.ValidatorIndex) + msg, err := cdc.MarshalBinaryLengthPrefixed(CanonicalizeVote(chainID, vote)) + if err != nil { + panic(err) + } + blsPubkeys = append(blsPubkeys, voter.PubKey.(composite.PubKeyComposite).SignKey.(bls.PubKeyBLS12)) + msgs = append(msgs, msg) + } + } + if commit.AggregatedSignature != nil { + err := bls.VerifyAggregatedSignature(commit.AggregatedSignature, blsPubkeys, msgs) + if err != nil { + panic(err) } } return voteSet diff --git a/types/vote_set.go b/types/vote_set.go index 5bcb885f5..22fb3ca9a 100644 --- a/types/vote_set.go +++ b/types/vote_set.go @@ -7,7 +7,7 @@ import ( "sync" "github.com/pkg/errors" - + "github.com/tendermint/tendermint/crypto" "github.com/tendermint/tendermint/libs/bits" ) @@ -148,11 +148,35 @@ func (voteSet *VoteSet) AddVote(vote *Vote) (added bool, err error) { voteSet.mtx.Lock() defer voteSet.mtx.Unlock() - return voteSet.addVote(vote) + return voteSet.addVote(vote, func(chainID string, pubKey crypto.PubKey) (added bool, err error) { + if !bytes.Equal(pubKey.Address(), vote.ValidatorAddress) { + return false, ErrVoteInvalidValidatorAddress + } + if !pubKey.VerifyBytes(vote.SignBytes(chainID), vote.Signature) { + return false, ErrVoteInvalidSignature + } + return true, nil + }) +} + +func (voteSet *VoteSet) AddAggregatedVote(vote *Vote) (added bool, err error) { + if voteSet == nil { + panic("AddVote() on nil VoteSet") + } + voteSet.mtx.Lock() + defer voteSet.mtx.Unlock() + + return voteSet.addVote(vote, func(chainID string, pubKey crypto.PubKey) (added bool, err error) { + if !bytes.Equal(pubKey.Address(), vote.ValidatorAddress) { + return false, ErrVoteInvalidValidatorAddress + } + return true, nil + }) } // NOTE: Validates as much as possible before attempting to verify the signature. -func (voteSet *VoteSet) addVote(vote *Vote) (added bool, err error) { +func (voteSet *VoteSet) addVote(vote *Vote, execVerify func(chainID string, + pub crypto.PubKey) (added bool, err error)) (added bool, err error) { if vote == nil { return false, ErrVoteNil } @@ -200,9 +224,7 @@ func (voteSet *VoteSet) addVote(vote *Vote) (added bool, err error) { } // Check signature. - if err := vote.Verify(voteSet.chainID, voter.PubKey); err != nil { - return false, errors.Wrapf(err, "Failed to verify vote with ChainID %s and PubKey %s", voteSet.chainID, voter.PubKey) - } + execVerify(voteSet.chainID, voter.PubKey) // Add vote and get conflicting vote if any. added, conflicting := voteSet.addVerifiedVote(vote, blockKey, voter.VotingPower) From a34c7cc7a1fbc0bc30f8a3e8a21422ed5df4d7e7 Mon Sep 17 00:00:00 2001 From: mariko Date: Mon, 29 Mar 2021 16:21:42 +0900 Subject: [PATCH 2/4] fix: fix error handling --- types/block.go | 21 ++++++++++++--------- types/vote_set.go | 26 ++++++++++---------------- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/types/block.go b/types/block.go index 0c834444e..3a1684751 100644 --- a/types/block.go +++ b/types/block.go @@ -762,37 +762,40 @@ func (commit *Commit) MaxCommitBytes() int64 { // Inverse of VoteSet.MakeCommit(). func CommitToVoteSet(chainID string, commit *Commit, voters *VoterSet) *VoteSet { voteSet := NewVoteSet(chainID, commit.Height, commit.Round, PrecommitType, voters) - var blsPubkeys []bls.PubKeyBLS12 + var blsPubKeys []bls.PubKeyBLS12 var msgs [][]byte for idx, commitSig := range commit.Signatures { if commitSig.Absent() { continue // OK, some precommits can be missing. } vote := commit.GetVote(idx) - // if signature != nil if vote.Signature != nil { added, err := voteSet.AddVote(vote) if !added || err != nil { - panic(fmt.Sprintf("Failed to reconstruct LastCommit: %v", err)) + panic(fmt.Sprintf("Failed to reconstruct LastCommit from Votes: %v", err)) } } else { added, err := voteSet.AddAggregatedVote(vote) if !added || err != nil { - panic(fmt.Sprintf("Failed to reconstruct LastCommit: %v", err)) + panic(fmt.Sprintf("Failed to reconstruct LastCommit from AggregatedVotes : %v", err)) + } + // Ensure that signer is a validator. + addr, voter := voters.GetByIndex(vote.ValidatorIndex) + if voter == nil || addr == nil { + panic(fmt.Sprintf("Cannot find voter %d in voterSet of size %d", vote.ValidatorIndex, voters.Size())) } - _, voter := voters.GetByIndex(vote.ValidatorIndex) msg, err := cdc.MarshalBinaryLengthPrefixed(CanonicalizeVote(chainID, vote)) if err != nil { - panic(err) + panic(fmt.Sprintf("Failed to MarshalBinaryLengthPrefixed : %v", err)) } - blsPubkeys = append(blsPubkeys, voter.PubKey.(composite.PubKeyComposite).SignKey.(bls.PubKeyBLS12)) + blsPubKeys = append(blsPubKeys, voter.PubKey.(composite.PubKeyComposite).SignKey.(bls.PubKeyBLS12)) msgs = append(msgs, msg) } } if commit.AggregatedSignature != nil { - err := bls.VerifyAggregatedSignature(commit.AggregatedSignature, blsPubkeys, msgs) + err := bls.VerifyAggregatedSignature(commit.AggregatedSignature, blsPubKeys, msgs) if err != nil { - panic(err) + panic(fmt.Sprintf("Failed to VerifyAggregatedSignature : %v", err)) } } return voteSet diff --git a/types/vote_set.go b/types/vote_set.go index 22fb3ca9a..cdcbb5a0b 100644 --- a/types/vote_set.go +++ b/types/vote_set.go @@ -148,35 +148,27 @@ func (voteSet *VoteSet) AddVote(vote *Vote) (added bool, err error) { voteSet.mtx.Lock() defer voteSet.mtx.Unlock() - return voteSet.addVote(vote, func(chainID string, pubKey crypto.PubKey) (added bool, err error) { - if !bytes.Equal(pubKey.Address(), vote.ValidatorAddress) { - return false, ErrVoteInvalidValidatorAddress - } - if !pubKey.VerifyBytes(vote.SignBytes(chainID), vote.Signature) { - return false, ErrVoteInvalidSignature - } - return true, nil - }) + return voteSet.addVote(vote, vote.Verify) } func (voteSet *VoteSet) AddAggregatedVote(vote *Vote) (added bool, err error) { if voteSet == nil { - panic("AddVote() on nil VoteSet") + panic("AddAggregatedVote() on nil VoteSet") } voteSet.mtx.Lock() defer voteSet.mtx.Unlock() - return voteSet.addVote(vote, func(chainID string, pubKey crypto.PubKey) (added bool, err error) { + return voteSet.addVote(vote, func(chainID string, pubKey crypto.PubKey) (err error) { if !bytes.Equal(pubKey.Address(), vote.ValidatorAddress) { - return false, ErrVoteInvalidValidatorAddress + return ErrVoteInvalidValidatorAddress } - return true, nil + return nil }) } // NOTE: Validates as much as possible before attempting to verify the signature. -func (voteSet *VoteSet) addVote(vote *Vote, execVerify func(chainID string, - pub crypto.PubKey) (added bool, err error)) (added bool, err error) { +func (voteSet *VoteSet) addVote(vote *Vote, execVoteVerify func(chainID string, + pub crypto.PubKey) (err error)) (added bool, err error) { if vote == nil { return false, ErrVoteNil } @@ -224,7 +216,9 @@ func (voteSet *VoteSet) addVote(vote *Vote, execVerify func(chainID string, } // Check signature. - execVerify(voteSet.chainID, voter.PubKey) + if err := execVoteVerify(voteSet.chainID, voter.PubKey); err != nil { + errors.Wrapf(err, "Failed to verify vote with ChainID %s and PubKey %s", voteSet.chainID, voter.PubKey) + } // Add vote and get conflicting vote if any. added, conflicting := voteSet.addVerifiedVote(vote, blockKey, voter.VotingPower) From 09f64e053a83012b73edc85d1bc16d2b4dd11d88 Mon Sep 17 00:00:00 2001 From: mariko Date: Tue, 30 Mar 2021 13:22:40 +0900 Subject: [PATCH 3/4] fix: fix error handling omission --- types/vote_set.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/types/vote_set.go b/types/vote_set.go index cdcbb5a0b..1400d6b1d 100644 --- a/types/vote_set.go +++ b/types/vote_set.go @@ -217,7 +217,7 @@ func (voteSet *VoteSet) addVote(vote *Vote, execVoteVerify func(chainID string, // Check signature. if err := execVoteVerify(voteSet.chainID, voter.PubKey); err != nil { - errors.Wrapf(err, "Failed to verify vote with ChainID %s and PubKey %s", voteSet.chainID, voter.PubKey) + return false, errors.Wrapf(err, "Failed to verify vote with ChainID %s and PubKey %s", voteSet.chainID, voter.PubKey) } // Add vote and get conflicting vote if any. From 4b976de1f001e838474bdc4c08a9a75445fc7e5c Mon Sep 17 00:00:00 2001 From: mariko Date: Tue, 30 Mar 2021 14:28:07 +0900 Subject: [PATCH 4/4] fix: Specify the capacity of the slice `blsPubKeys` and `msgs` in advance --- types/block.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/types/block.go b/types/block.go index 3a1684751..1d0f176e3 100644 --- a/types/block.go +++ b/types/block.go @@ -762,8 +762,8 @@ func (commit *Commit) MaxCommitBytes() int64 { // Inverse of VoteSet.MakeCommit(). func CommitToVoteSet(chainID string, commit *Commit, voters *VoterSet) *VoteSet { voteSet := NewVoteSet(chainID, commit.Height, commit.Round, PrecommitType, voters) - var blsPubKeys []bls.PubKeyBLS12 - var msgs [][]byte + blsPubKeys := make([]bls.PubKeyBLS12, 0, len(commit.Signatures)) + msgs := make([][]byte, 0, len(commit.Signatures)) for idx, commitSig := range commit.Signatures { if commitSig.Absent() { continue // OK, some precommits can be missing.