diff --git a/lite2/client.go b/lite2/client.go index 08561a2c9..03c7f7a70 100644 --- a/lite2/client.go +++ b/lite2/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) } @@ -954,7 +954,7 @@ func (c *Client) compareNewHeaderWithWitnesses(h *types.SignedHeader) error { } if !bytes.Equal(h.Hash(), altH.Hash()) { - if err = c.latestTrustedVals.VerifyCommitTrusting(c.chainID, altH.Commit.BlockID, + if err = c.latestTrustedVals.VerifyCommitLightTrusting(c.chainID, altH.Commit.BlockID, altH.Height, altH.Commit, c.trustLevel); err != nil { c.logger.Error("Witness sent us incorrect header", "err", err, "witness", witness) witnessesToRemove = append(witnessesToRemove, i) diff --git a/lite2/verifier.go b/lite2/verifier.go index 1ef54677b..d754e9e7b 100644 --- a/lite2/verifier.go +++ b/lite2/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.BlockID, untrustedHeader.Height, + err := trustedVals.VerifyCommitLightTrusting(chainID, untrustedHeader.Commit.BlockID, untrustedHeader.Height, untrustedHeader.Commit, trustLevel) if err != nil { switch e := err.(type) { @@ -73,7 +73,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} } @@ -128,7 +128,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/validator_set.go b/types/validator_set.go index ecfe865a3..f4f11fcc6 100644 --- a/types/validator_set.go +++ b/types/validator_set.go @@ -627,6 +627,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 { @@ -661,6 +667,58 @@ func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, // It's OK that the BlockID doesn't match. 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, 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 { @@ -668,7 +726,6 @@ func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, } } - // talliedVotingPower <= needed, thus return error return ErrNotEnoughVotingPowerSigned{Got: talliedVotingPower, Needed: votingPowerNeeded} } @@ -748,14 +805,17 @@ func (vals *ValidatorSet) VerifyFutureCommit(newSet *ValidatorSet, chainID strin return nil } -// VerifyCommitTrusting verifies that trustLevel ([1/3, 1]) of the validator -// set signed this commit. +// VerifyCommitLightTrusting verifies that trustLevel of the validator set signed +// this commit. +// +// This method is primarily used by the light client and does not check all the +// signatures. // // NOTE the given validators do not necessarily correspond to the validator set // for this commit, but there may be some intersection. // // Panics if trustLevel is invalid. -func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, blockID BlockID, +func (vals *ValidatorSet) VerifyCommitLightTrusting(chainID string, blockID BlockID, height int64, commit *Commit, trustLevel tmmath.Fraction) error { // sanity check @@ -781,8 +841,9 @@ func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, blockID BlockID, 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 @@ -803,14 +864,7 @@ func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, blockID BlockID, return errors.Errorf("wrong signature (#%d): %X", idx, commitSig.Signature) } - // Good! - if blockID.Equals(commitSig.BlockID(commit.BlockID)) { - talliedVotingPower += val.VotingPower - } - // else { - // It's OK that the BlockID doesn't match. 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 b28f138d8..059d043df 100644 --- a/types/validator_set_test.go +++ b/types/validator_set_test.go @@ -17,7 +17,6 @@ import ( "github.com/tendermint/tendermint/crypto/ed25519" tmmath "github.com/tendermint/tendermint/libs/math" tmrand "github.com/tendermint/tendermint/libs/rand" - tmtime "github.com/tendermint/tendermint/types/time" ) func TestValidatorSetBasic(t *testing.T) { @@ -584,62 +583,147 @@ func TestSafeSubClip(t *testing.T) { //------------------------------------------------------------------- -func TestValidatorSetVerifyCommit(t *testing.T) { - privKey := ed25519.GenPrivKey() - pubKey := privKey.PubKey() - v1 := NewValidator(pubKey, 1000) - vset := NewValidatorSet([]*Validator{v1}) - - // good +// Check VerifyCommit, VerifyCommitLight and VerifyCommitLightTrusting basic +// verification. +func TestValidatorSet_VerifyCommit_All(t *testing.T) { var ( - chainID = "mychainID" - blockID = makeBlockIDRandom() - height = int64(5) + privKey = ed25519.GenPrivKey() + pubKey = privKey.PubKey() + v1 = NewValidator(pubKey, 1000) + vset = NewValidatorSet([]*Validator{v1}) + + chainID = "Lalande21185" ) - vote := &Vote{ - ValidatorAddress: v1.Address, - ValidatorIndex: 0, - Height: height, - Round: 0, - Timestamp: tmtime.Now(), - Type: PrecommitType, - BlockID: blockID, - } + + vote := examplePrecommit() + vote.ValidatorAddress = pubKey.Address() sig, err := privKey.Sign(vote.SignBytes(chainID)) assert.NoError(t, err) vote.Signature = sig - commit := NewCommit(vote.Height, vote.Round, blockID, []CommitSig{vote.CommitSig()}) - // bad + commit := NewCommit(vote.Height, vote.Round, vote.BlockID, []CommitSig{vote.CommitSig()}) + + vote2 := *vote + sig2, err := privKey.Sign(vote2.SignBytes("EpsilonEridani")) + require.NoError(t, err) + vote2.Signature = sig2 + + testCases := []struct { + description string + chainID string + blockID BlockID + height int64 + commit *Commit + expErr bool + }{ + {"good", chainID, vote.BlockID, vote.Height, commit, false}, + + {"wrong signature (#0)", "EpsilonEridani", vote.BlockID, vote.Height, commit, true}, + {"wrong block ID", chainID, makeBlockIDRandom(), vote.Height, commit, true}, + {"wrong height", chainID, vote.BlockID, vote.Height - 1, commit, true}, + + {"wrong set size: 1 vs 0", chainID, vote.BlockID, vote.Height, + NewCommit(vote.Height, vote.Round, vote.BlockID, []CommitSig{}), true}, + + {"wrong set size: 1 vs 2", chainID, vote.BlockID, vote.Height, + NewCommit(vote.Height, vote.Round, vote.BlockID, + []CommitSig{vote.CommitSig(), {BlockIDFlag: BlockIDFlagAbsent}}), true}, + + {"insufficient voting power: got 0, needed more than 666", chainID, vote.BlockID, vote.Height, + NewCommit(vote.Height, vote.Round, vote.BlockID, []CommitSig{{BlockIDFlag: BlockIDFlagAbsent}}), true}, + + {"wrong signature (#0)", chainID, vote.BlockID, vote.Height, + NewCommit(vote.Height, vote.Round, vote.BlockID, []CommitSig{vote2.CommitSig()}), true}, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.description, func(t *testing.T) { + err := vset.VerifyCommit(tc.chainID, tc.blockID, tc.height, tc.commit) + if tc.expErr { + if assert.Error(t, err, "VerifyCommit") { + assert.Contains(t, err.Error(), tc.description, "VerifyCommit") + } + } else { + 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 ( - badChainID = "notmychainID" - badBlockID = BlockID{Hash: []byte("goodbye")} - badHeight = height + 1 - badCommit = NewCommit(badHeight, 0, blockID, []CommitSig{{BlockIDFlag: BlockIDFlagAbsent}}) + chainID = "test_chain_id" + h = int64(3) + blockID = makeBlockIDRandom() ) - // test some error cases - // TODO: test more cases! - cases := []struct { - chainID string - blockID BlockID - height int64 - commit *Commit - }{ - {badChainID, blockID, height, commit}, - {chainID, badBlockID, height, commit}, - {chainID, blockID, badHeight, commit}, - {chainID, blockID, height, badCommit}, - } + voteSet, valSet, vals := randVoteSet(h, 0, PrecommitType, 4, 10) + commit, err := MakeCommit(blockID, h, 0, voteSet, vals, time.Now()) + require.NoError(t, err) - for i, c := range cases { - err := vset.VerifyCommit(c.chainID, c.blockID, c.height, c.commit) - assert.NotNil(t, err, i) - } + // malleate 4th signature + vote := voteSet.GetByIndex(3) + err = vals[3].SignVote("CentaurusA", vote) + require.NoError(t, err) + commit.Signatures[3] = vote.CommitSig() - // test a good one - err = vset.VerifyCommit(chainID, blockID, height, commit) - assert.Nil(t, err) + 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, 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) + err = vals[3].SignVote("CentaurusA", vote) + require.NoError(t, err) + 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, 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) + err = vals[2].SignVote("CentaurusA", vote) + require.NoError(t, err) + commit.Signatures[2] = vote.CommitSig() + + err = valSet.VerifyCommitLightTrusting(chainID, blockID, h, commit, tmmath.Fraction{Numerator: 1, Denominator: 3}) + assert.NoError(t, err) } func TestEmptySet(t *testing.T) { @@ -1326,7 +1410,7 @@ func TestValSetUpdateOverflowRelated(t *testing.T) { } } -func TestVerifyCommitTrusting(t *testing.T) { +func TestValidatorSet_VerifyCommitLightTrusting(t *testing.T) { var ( blockID = makeBlockIDRandom() voteSet, originalValset, vals = randVoteSet(1, 1, PrecommitType, 6, 1) @@ -1357,7 +1441,7 @@ func TestVerifyCommitTrusting(t *testing.T) { } for _, tc := range testCases { - err = tc.valSet.VerifyCommitTrusting("test_chain_id", blockID, commit.Height, commit, + err = tc.valSet.VerifyCommitLightTrusting("test_chain_id", blockID, commit.Height, commit, tmmath.Fraction{Numerator: 1, Denominator: 3}) if tc.err { assert.Error(t, err) @@ -1367,7 +1451,7 @@ func TestVerifyCommitTrusting(t *testing.T) { } } -func TestVerifyCommitTrustingErrorsOnOverflow(t *testing.T) { +func TestValidatorSet_VerifyCommitLightTrustingErrorsOnOverflow(t *testing.T) { var ( blockID = makeBlockIDRandom() voteSet, valSet, vals = randVoteSet(1, 1, PrecommitType, 1, MaxTotalVotingPower) @@ -1375,7 +1459,7 @@ func TestVerifyCommitTrustingErrorsOnOverflow(t *testing.T) { ) require.NoError(t, err) - err = valSet.VerifyCommitTrusting("test_chain_id", blockID, commit.Height, commit, + err = valSet.VerifyCommitLightTrusting("test_chain_id", blockID, commit.Height, commit, tmmath.Fraction{Numerator: 25, Denominator: 55}) if assert.Error(t, err) { assert.Contains(t, err.Error(), "int64 overflow")