diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 39e3d2f1f..74cedb349 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -10,7 +10,6 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi - CLI/RPC/Config - - [consensus] \#4582 RoundState: `Round`, `LockedRound` & `CommitRound` are now int32 - [consensus] \#4582 HeightVoteSet: `round` is now int32 - [evidence] \#4725 Remove `Pubkey` from DuplicateVoteEvidence diff --git a/light/client.go b/light/client.go index 5e88f7813..92593d153 100644 --- a/light/client.go +++ b/light/client.go @@ -387,7 +387,7 @@ func (c *Client) initializeWithTrustOptions(options TrustOptions) error { } // Ensure that +2/3 of validators signed correctly. - err = vals.VerifyCommit(c.chainID, h.Commit.BlockID, h.Height, h.Commit) + err = vals.VerifyCommitLight(c.chainID, h.Commit.BlockID, h.Height, h.Commit) if err != nil { return fmt.Errorf("invalid commit: %w", err) } @@ -1020,9 +1020,9 @@ func (c *Client) compareNewHeaderWithWitness(errc chan error, h *types.SignedHea } if !bytes.Equal(h.Hash(), altH.Hash()) { - // FIXME: call bisection instead of VerifyCommitTrusting + // FIXME: call bisection instead of VerifyCommitLightTrusting // https://github.com/tendermint/tendermint/issues/4934 - if err = c.latestTrustedVals.VerifyCommitTrusting(c.chainID, altH.Commit, c.trustLevel); err != nil { + if err = c.latestTrustedVals.VerifyCommitLightTrusting(c.chainID, altH.Commit, c.trustLevel); err != nil { errc <- errBadWitness{err, invalidHeader, witnessIndex} return } diff --git a/light/verifier.go b/light/verifier.go index 54f0168df..173a30bf6 100644 --- a/light/verifier.go +++ b/light/verifier.go @@ -57,7 +57,7 @@ func VerifyNonAdjacent( } // Ensure that +`trustLevel` (default 1/3) or more of last trusted validators signed correctly. - err := trustedVals.VerifyCommitTrusting(chainID, untrustedHeader.Commit, trustLevel) + err := trustedVals.VerifyCommitLightTrusting(chainID, untrustedHeader.Commit, trustLevel) if err != nil { switch e := err.(type) { case types.ErrNotEnoughVotingPowerSigned: @@ -72,7 +72,7 @@ func VerifyNonAdjacent( // NOTE: this should always be the last check because untrustedVals can be // intentionally made very large to DOS the light client. not the case for // VerifyAdjacent, where validator set is known in advance. - if err := untrustedVals.VerifyCommit(chainID, untrustedHeader.Commit.BlockID, untrustedHeader.Height, + if err := untrustedVals.VerifyCommitLight(chainID, untrustedHeader.Commit.BlockID, untrustedHeader.Height, untrustedHeader.Commit); err != nil { return ErrInvalidHeader{err} } @@ -127,7 +127,7 @@ func VerifyAdjacent( } // Ensure that +2/3 of new validators signed correctly. - if err := untrustedVals.VerifyCommit(chainID, untrustedHeader.Commit.BlockID, untrustedHeader.Height, + if err := untrustedVals.VerifyCommitLight(chainID, untrustedHeader.Commit.BlockID, untrustedHeader.Height, untrustedHeader.Commit); err != nil { return ErrInvalidHeader{err} } diff --git a/types/evidence.go b/types/evidence.go index cb241db32..89b1ce38c 100644 --- a/types/evidence.go +++ b/types/evidence.go @@ -627,7 +627,7 @@ func (ev *ConflictingHeadersEvidence) VerifyComposite(committedHeader *Header, v // Header must be signed by at least 1/3+ of voting power of currently // trusted validator set. - if err := valSet.VerifyCommitTrusting( + if err := valSet.VerifyCommitLightTrusting( alternativeHeader.ChainID, alternativeHeader.Commit, tmmath.Fraction{Numerator: 1, Denominator: 3}); err != nil { diff --git a/types/validator_set.go b/types/validator_set.go index 591cd98b2..b97cc9d5c 100644 --- a/types/validator_set.go +++ b/types/validator_set.go @@ -656,6 +656,12 @@ func (vals *ValidatorSet) UpdateWithChangeSet(changes []*Validator) error { } // VerifyCommit verifies +2/3 of the set had signed the given commit. +// +// It checks all the signatures! While it's safe to exit as soon as we have +// 2/3+ signatures, doing so would impact incentivization logic in the ABCI +// application that depends on the LastCommitInfo sent in BeginBlock, which +// includes which validators signed. For instance, Gaia incentivizes proposers +// with a bonus for including more than +2/3 of the signatures. func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, height int64, commit *Commit) error { @@ -696,6 +702,58 @@ func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, // It's OK. We include stray signatures (~votes for nil) to measure // validator availability. // } + } + + if got, needed := talliedVotingPower, votingPowerNeeded; got <= needed { + return ErrNotEnoughVotingPowerSigned{Got: got, Needed: needed} + } + + return nil +} + +/////////////////////////////////////////////////////////////////////////////// +// LIGHT CLIENT VERIFICATION METHODS +/////////////////////////////////////////////////////////////////////////////// + +// VerifyCommitLight verifies +2/3 of the set had signed the given commit. +// +// This method is primarily used by the light client and does not check all the +// signatures. +func (vals *ValidatorSet) VerifyCommitLight(chainID string, blockID BlockID, + height int64, commit *Commit) error { + + if vals.Size() != len(commit.Signatures) { + return NewErrInvalidCommitSignatures(vals.Size(), len(commit.Signatures)) + } + + // Validate Height and BlockID. + if height != commit.Height { + return NewErrInvalidCommitHeight(height, commit.Height) + } + if !blockID.Equals(commit.BlockID) { + return fmt.Errorf("invalid commit -- wrong block ID: want %v, got %v", + blockID, commit.BlockID) + } + + talliedVotingPower := int64(0) + votingPowerNeeded := vals.TotalVotingPower() * 2 / 3 + for idx, commitSig := range commit.Signatures { + // No need to verify absent or nil votes. + if !commitSig.ForBlock() { + continue + } + + // The vals and commit have a 1-to-1 correspondance. + // This means we don't need the validator address or to do any lookup. + val := vals.Validators[idx] + + // Validate signature. + voteSignBytes := commit.VoteSignBytes(chainID, int32(idx)) + if !val.PubKey.VerifyBytes(voteSignBytes, commitSig.Signature) { + return fmt.Errorf("wrong signature (#%d): %X", idx, commitSig.Signature) + } + + talliedVotingPower += val.VotingPower // return as soon as +2/3 of the signatures are verified if talliedVotingPower > votingPowerNeeded { @@ -703,16 +761,18 @@ func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, } } - // talliedVotingPower <= needed, thus return error return ErrNotEnoughVotingPowerSigned{Got: talliedVotingPower, Needed: votingPowerNeeded} } -// VerifyCommitTrusting verifies that trustLevel of the validator set signed +// VerifyCommitLightTrusting verifies that trustLevel of the validator set signed // this commit. // // NOTE the given validators do not necessarily correspond to the validator set // for this commit, but there may be some intersection. -func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, commit *Commit, trustLevel tmmath.Fraction) error { +// +// This method is primarily used by the light client and does not check all the +// signatures. +func (vals *ValidatorSet) VerifyCommitLightTrusting(chainID string, commit *Commit, trustLevel tmmath.Fraction) error { // sanity check if trustLevel.Denominator == 0 { return errors.New("trustLevel has zero Denominator") @@ -731,8 +791,9 @@ func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, commit *Commit, t votingPowerNeeded := totalVotingPowerMulByNumerator / trustLevel.Denominator for idx, commitSig := range commit.Signatures { - if commitSig.Absent() { - continue // OK, some signatures can be absent. + // No need to verify absent or nil votes. + if !commitSig.ForBlock() { + continue } // We don't know the validators that committed this block, so we have to @@ -753,14 +814,7 @@ func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, commit *Commit, t return fmt.Errorf("wrong signature (#%d): %X", idx, commitSig.Signature) } - // Good! - if commitSig.ForBlock() { - talliedVotingPower += val.VotingPower - } - // else { - // It's OK. We include stray signatures (~votes for nil) to measure - // validator availability. - // } + talliedVotingPower += val.VotingPower if talliedVotingPower > votingPowerNeeded { return nil diff --git a/types/validator_set_test.go b/types/validator_set_test.go index 372694c37..1927d4931 100644 --- a/types/validator_set_test.go +++ b/types/validator_set_test.go @@ -662,7 +662,9 @@ func TestSafeSubClip(t *testing.T) { //------------------------------------------------------------------- -func TestValidatorSet_VerifyCommit(t *testing.T) { +// Check VerifyCommit, VerifyCommitLight and VerifyCommitLightTrusting basic +// verification. +func TestValidatorSet_VerifyCommit_All(t *testing.T) { var ( privKey = ed25519.GenPrivKey() pubKey = privKey.PubKey() @@ -673,6 +675,7 @@ func TestValidatorSet_VerifyCommit(t *testing.T) { ) vote := examplePrecommit() + vote.ValidatorAddress = pubKey.Address() v := vote.ToProto() sig, err := privKey.Sign(VoteSignBytes(chainID, v)) require.NoError(t, err) @@ -718,15 +721,96 @@ func TestValidatorSet_VerifyCommit(t *testing.T) { t.Run(tc.description, func(t *testing.T) { err := vset.VerifyCommit(tc.chainID, tc.blockID, tc.height, tc.commit) if tc.expErr { - assert.Error(t, err) - assert.Contains(t, err.Error(), tc.description) + if assert.Error(t, err, "VerifyCommit") { + assert.Contains(t, err.Error(), tc.description, "VerifyCommit") + } } else { - assert.NoError(t, err) + assert.NoError(t, err, "VerifyCommit") + } + + err = vset.VerifyCommitLight(tc.chainID, tc.blockID, tc.height, tc.commit) + if tc.expErr { + if assert.Error(t, err, "VerifyCommitLight") { + assert.Contains(t, err.Error(), tc.description, "VerifyCommitLight") + } + } else { + assert.NoError(t, err, "VerifyCommitLight") } }) } } +func TestValidatorSet_VerifyCommit_CheckAllSignatures(t *testing.T) { + var ( + chainID = "test_chain_id" + h = int64(3) + blockID = makeBlockIDRandom() + ) + + voteSet, valSet, vals := randVoteSet(h, 0, tmproto.PrecommitType, 4, 10) + commit, err := MakeCommit(blockID, h, 0, voteSet, vals, time.Now()) + require.NoError(t, err) + + // malleate 4th signature + vote := voteSet.GetByIndex(3) + v := vote.ToProto() + err = vals[3].SignVote("CentaurusA", v) + require.NoError(t, err) + vote.Signature = v.Signature + commit.Signatures[3] = vote.CommitSig() + + err = valSet.VerifyCommit(chainID, blockID, h, commit) + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "wrong signature (#3)") + } +} + +func TestValidatorSet_VerifyCommitLight_ReturnsAsSoonAsMajorityOfVotingPowerSigned(t *testing.T) { + var ( + chainID = "test_chain_id" + h = int64(3) + blockID = makeBlockIDRandom() + ) + + voteSet, valSet, vals := randVoteSet(h, 0, tmproto.PrecommitType, 4, 10) + commit, err := MakeCommit(blockID, h, 0, voteSet, vals, time.Now()) + require.NoError(t, err) + + // malleate 4th signature (3 signatures are enough for 2/3+) + vote := voteSet.GetByIndex(3) + v := vote.ToProto() + err = vals[3].SignVote("CentaurusA", v) + require.NoError(t, err) + vote.Signature = v.Signature + commit.Signatures[3] = vote.CommitSig() + + err = valSet.VerifyCommitLight(chainID, blockID, h, commit) + assert.NoError(t, err) +} + +func TestValidatorSet_VerifyCommitLightTrusting_ReturnsAsSoonAsTrustLevelOfVotingPowerSigned(t *testing.T) { + var ( + chainID = "test_chain_id" + h = int64(3) + blockID = makeBlockIDRandom() + ) + + voteSet, valSet, vals := randVoteSet(h, 0, tmproto.PrecommitType, 4, 10) + commit, err := MakeCommit(blockID, h, 0, voteSet, vals, time.Now()) + require.NoError(t, err) + + // malleate 3rd signature (2 signatures are enough for 1/3+ trust level) + vote := voteSet.GetByIndex(2) + v := vote.ToProto() + err = vals[2].SignVote("CentaurusA", v) + require.NoError(t, err) + vote.Signature = v.Signature + commit.Signatures[2] = vote.CommitSig() + + err = valSet.VerifyCommitLightTrusting(chainID, commit, tmmath.Fraction{Numerator: 1, Denominator: 3}) + assert.NoError(t, err) +} + func TestEmptySet(t *testing.T) { var valList []*Validator @@ -1411,7 +1495,7 @@ func TestValSetUpdateOverflowRelated(t *testing.T) { } } -func TestValidatorSet_VerifyCommitTrusting(t *testing.T) { +func TestValidatorSet_VerifyCommitLightTrusting(t *testing.T) { var ( blockID = makeBlockIDRandom() voteSet, originalValset, vals = randVoteSet(1, 1, tmproto.PrecommitType, 6, 1) @@ -1442,7 +1526,7 @@ func TestValidatorSet_VerifyCommitTrusting(t *testing.T) { } for _, tc := range testCases { - err = tc.valSet.VerifyCommitTrusting("test_chain_id", commit, + err = tc.valSet.VerifyCommitLightTrusting("test_chain_id", commit, tmmath.Fraction{Numerator: 1, Denominator: 3}) if tc.err { assert.Error(t, err) @@ -1452,7 +1536,7 @@ func TestValidatorSet_VerifyCommitTrusting(t *testing.T) { } } -func TestValidatorSet_VerifyCommitTrustingErrorsOnOverflow(t *testing.T) { +func TestValidatorSet_VerifyCommitLightTrustingErrorsOnOverflow(t *testing.T) { var ( blockID = makeBlockIDRandom() voteSet, valSet, vals = randVoteSet(1, 1, tmproto.PrecommitType, 1, MaxTotalVotingPower) @@ -1460,7 +1544,7 @@ func TestValidatorSet_VerifyCommitTrustingErrorsOnOverflow(t *testing.T) { ) require.NoError(t, err) - err = valSet.VerifyCommitTrusting("test_chain_id", commit, + err = valSet.VerifyCommitLightTrusting("test_chain_id", commit, tmmath.Fraction{Numerator: 25, Denominator: 55}) if assert.Error(t, err) { assert.Contains(t, err.Error(), "int64 overflow")