diff --git a/blocksync/reactor.go b/blocksync/reactor.go index 373ad908e5..82ad953a69 100644 --- a/blocksync/reactor.go +++ b/blocksync/reactor.go @@ -454,7 +454,7 @@ FOR_LOOP: // first.Hash() doesn't verify the tx contents, so MakePartSet() is // currently necessary. // TODO(sergio): Should we also validate against the extended commit? - err = state.Validators.VerifyCommitLight( + err = state.Validators.VerifyCommitLightAllSignatures( chainID, firstID, first.Height, second.LastCommit) if err == nil { diff --git a/evidence/pool.go b/evidence/pool.go index e36b66db38..b502341a86 100644 --- a/evidence/pool.go +++ b/evidence/pool.go @@ -132,11 +132,11 @@ func (evpool *Pool) Update(state sm.State, ev types.EvidenceList) { // AddEvidence checks the evidence is valid and adds it to the pool. func (evpool *Pool) AddEvidence(ev types.Evidence) error { - evpool.logger.Debug("Attempting to add evidence", "ev", ev) + evpool.logger.Info("Attempting to add evidence", "ev", ev) // We have already verified this piece of evidence - no need to do it again if evpool.isPending(ev) { - evpool.logger.Debug("Evidence already pending, ignoring this one", "ev", ev) + evpool.logger.Info("Evidence already pending, ignoring this one", "ev", ev) return nil } @@ -144,7 +144,7 @@ func (evpool *Pool) AddEvidence(ev types.Evidence) error { if evpool.isCommitted(ev) { // this can happen if the peer that sent us the evidence is behind so we shouldn't // punish the peer. - evpool.logger.Debug("Evidence was already committed, ignoring this one", "ev", ev) + evpool.logger.Info("Evidence was already committed, ignoring this one", "ev", ev) return nil } @@ -513,13 +513,13 @@ func (evpool *Pool) processConsensusBuffer(state sm.State) { // check if we already have this evidence if evpool.isPending(dve) { - evpool.logger.Debug("evidence already pending; ignoring", "evidence", dve) + evpool.logger.Info("evidence already pending; ignoring", "evidence", dve) continue } // check that the evidence is not already committed on chain if evpool.isCommitted(dve) { - evpool.logger.Debug("evidence already committed; ignoring", "evidence", dve) + evpool.logger.Info("evidence already committed; ignoring", "evidence", dve) continue } diff --git a/evidence/verify.go b/evidence/verify.go index 313a5b91a8..cd09285221 100644 --- a/evidence/verify.go +++ b/evidence/verify.go @@ -103,6 +103,7 @@ func (evpool *Pool) verify(evidence types.Evidence) error { // the conflicting header's commit // - 2/3+ of the conflicting validator set correctly signed the conflicting block // - the nodes trusted header at the same height as the conflicting header has a different hash +// - all signatures must be checked as this will be used as evidence // // CONTRACT: must run ValidateBasic() on the evidence before verifying // @@ -120,7 +121,7 @@ func VerifyLightClientAttack( // In the case of lunatic attack there will be a different commonHeader height. Therefore the node perform a single // verification jump between the common header and the conflicting one if commonHeader.Height != e.ConflictingBlock.Height { - err := commonVals.VerifyCommitLightTrusting(trustedHeader.ChainID, e.ConflictingBlock.Commit, light.DefaultTrustLevel) + err := commonVals.VerifyCommitLightTrustingAllSignatures(trustedHeader.ChainID, e.ConflictingBlock.Commit, light.DefaultTrustLevel) if err != nil { return fmt.Errorf("skipping verification of conflicting block failed: %w", err) } @@ -132,7 +133,7 @@ func VerifyLightClientAttack( } // Verify that the 2/3+ commits from the conflicting validator set were for the conflicting header - if err := e.ConflictingBlock.ValidatorSet.VerifyCommitLight(trustedHeader.ChainID, e.ConflictingBlock.Commit.BlockID, + if err := e.ConflictingBlock.ValidatorSet.VerifyCommitLightAllSignatures(trustedHeader.ChainID, e.ConflictingBlock.Commit.BlockID, e.ConflictingBlock.Height, e.ConflictingBlock.Commit); err != nil { return fmt.Errorf("invalid commit from conflicting block: %w", err) } diff --git a/test/e2e/app/app.go b/test/e2e/app/app.go index 9866353658..507c52c76e 100644 --- a/test/e2e/app/app.go +++ b/test/e2e/app/app.go @@ -204,6 +204,16 @@ func (app *Application) FinalizeBlock(_ context.Context, req *abci.RequestFinali txs[i] = &abci.ExecTxResult{Code: kvstore.CodeTypeOK} } + for _, ev := range req.Misbehavior { + app.logger.Info("Misbehavior. Slashing validator", + "validator_address", ev.GetValidator().Address, + "type", ev.GetType(), + "height", ev.GetHeight(), + "time", ev.GetTime(), + "total_voting_power", ev.GetTotalVotingPower(), + ) + } + valUpdates, err := app.validatorUpdates(uint64(req.Height)) if err != nil { panic(err) diff --git a/test/e2e/runner/evidence.go b/test/e2e/runner/evidence.go index ce778bb53a..9cc446761e 100644 --- a/test/e2e/runner/evidence.go +++ b/test/e2e/runner/evidence.go @@ -25,7 +25,7 @@ import ( // 1 in 4 evidence is light client evidence, the rest is duplicate vote evidence const lightClientEvidenceRatio = 4 -// InjectEvidence takes a running testnet and generates an amount of valid +// InjectEvidence takes a running testnet and generates an amount of valid/invalid // evidence and broadcasts it to a random node through the rpc endpoint `/broadcast_evidence`. // Evidence is random and can be a mixture of LightClientAttackEvidence and // DuplicateVoteEvidence. @@ -88,10 +88,12 @@ func InjectEvidence(ctx context.Context, r *rand.Rand, testnet *e2e.Testnet, amo } var ev types.Evidence - for i := 1; i <= amount; i++ { + for i := 0; i < amount; i++ { + validEv := true if i%lightClientEvidenceRatio == 0 { + validEv = i%(lightClientEvidenceRatio*2) != 0 // Alternate valid and invalid evidence ev, err = generateLightClientAttackEvidence( - ctx, privVals, evidenceHeight, valSet, testnet.Name, blockRes.Block.Time, + ctx, privVals, evidenceHeight, valSet, testnet.Name, blockRes.Block.Time, validEv, ) } else { var dve *types.DuplicateVoteEvidence @@ -111,7 +113,15 @@ func InjectEvidence(ctx context.Context, r *rand.Rand, testnet *e2e.Testnet, amo } _, err := client.BroadcastEvidence(ctx, ev) - if err != nil { + if !validEv { + // The tests will count committed evidences later on, + // and only valid evidences will make it + amount++ + } + if validEv != (err == nil) { + if err == nil { + return errors.New("submitting invalid evidence didn't return an error") + } return err } } @@ -156,6 +166,7 @@ func generateLightClientAttackEvidence( vals *types.ValidatorSet, chainID string, evTime time.Time, + validEvidence bool, ) (*types.LightClientAttackEvidence, error) { // forge a random header forgedHeight := height + 2 @@ -165,7 +176,7 @@ func generateLightClientAttackEvidence( // add a new bogus validator and remove an existing one to // vary the validator set slightly - pv, conflictingVals, err := mutateValidatorSet(ctx, privVals, vals) + pv, conflictingVals, err := mutateValidatorSet(ctx, privVals, vals, !validEvidence) if err != nil { return nil, err } @@ -180,6 +191,11 @@ func generateLightClientAttackEvidence( return nil, err } + // malleate the last signature of the commit by adding one to its first byte + if !validEvidence { + commit.Signatures[len(commit.Signatures)-1].Signature[0]++ + } + ev := &types.LightClientAttackEvidence{ ConflictingBlock: &types.LightBlock{ SignedHeader: &types.SignedHeader{ @@ -293,7 +309,11 @@ func makeBlockID(hash []byte, partSetSize uint32, partSetHash []byte) types.Bloc } } -func mutateValidatorSet(ctx context.Context, privVals []types.MockPV, vals *types.ValidatorSet, +func mutateValidatorSet( + ctx context.Context, + privVals []types.MockPV, + vals *types.ValidatorSet, + nop bool, ) ([]types.PrivValidator, *types.ValidatorSet, error) { newVal, newPrivVal, err := test.Validator(ctx, 10) if err != nil { @@ -301,10 +321,14 @@ func mutateValidatorSet(ctx context.Context, privVals []types.MockPV, vals *type } var newVals *types.ValidatorSet - if vals.Size() > 2 { - newVals = types.NewValidatorSet(append(vals.Copy().Validators[:vals.Size()-1], newVal)) + if nop { + newVals = types.NewValidatorSet(vals.Copy().Validators) } else { - newVals = types.NewValidatorSet(append(vals.Copy().Validators, newVal)) + if vals.Size() > 2 { + newVals = types.NewValidatorSet(append(vals.Copy().Validators[:vals.Size()-1], newVal)) + } else { + newVals = types.NewValidatorSet(append(vals.Copy().Validators, newVal)) + } } // we need to sort the priv validators with the same index as the validator set diff --git a/types/validation.go b/types/validation.go index ad3a13690c..b4e6523292 100644 --- a/types/validation.go +++ b/types/validation.go @@ -54,10 +54,40 @@ func VerifyCommit(chainID string, vals *ValidatorSet, blockID BlockID, // 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 +// This method is primarily used by the light client and does NOT check all the // signatures. -func VerifyCommitLight(chainID string, vals *ValidatorSet, blockID BlockID, - height int64, commit *Commit) error { +func VerifyCommitLight( + chainID string, + vals *ValidatorSet, + blockID BlockID, + height int64, + commit *Commit, +) error { + return verifyCommitLightInternal(chainID, vals, blockID, height, commit, false) +} + +// VerifyCommitLightAllSignatures verifies +2/3 of the set had signed the given commit. +// +// This method is primarily used by the light client and DOES check all the +// signatures. +func VerifyCommitLightAllSignatures( + chainID string, + vals *ValidatorSet, + blockID BlockID, + height int64, + commit *Commit, +) error { + return verifyCommitLightInternal(chainID, vals, blockID, height, commit, true) +} + +func verifyCommitLightInternal( + chainID string, + vals *ValidatorSet, + blockID BlockID, + height int64, + commit *Commit, + countAllSignatures bool, +) error { // run a basic validation of the arguments if err := verifyBasicValsAndCommit(vals, commit, height, blockID); err != nil { return err @@ -75,12 +105,12 @@ func VerifyCommitLight(chainID string, vals *ValidatorSet, blockID BlockID, // attempt to batch verify if shouldBatchVerify(vals, commit) { return verifyCommitBatch(chainID, vals, commit, - votingPowerNeeded, ignore, count, false, true) + votingPowerNeeded, ignore, count, countAllSignatures, true) } // if verification failed or is not supported then fallback to single verification return verifyCommitSingle(chainID, vals, commit, votingPowerNeeded, - ignore, count, false, true) + ignore, count, countAllSignatures, true) } // VerifyCommitLightTrusting verifies that trustLevel of the validator set signed @@ -89,9 +119,41 @@ func VerifyCommitLight(chainID string, vals *ValidatorSet, blockID BlockID, // NOTE the given validators do not necessarily correspond to the validator set // for this commit, but there may be some intersection. // -// This method is primarily used by the light client and does not check all the +// This method is primarily used by the light client and does NOT check all the +// signatures. +func VerifyCommitLightTrusting( + chainID string, + vals *ValidatorSet, + commit *Commit, + trustLevel cmtmath.Fraction, +) error { + return verifyCommitLightTrustingInternal(chainID, vals, commit, trustLevel, false) +} + +// VerifyCommitLightTrustingAllSignatures 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. +// +// This method is primarily used by the light client and DOES check all the // signatures. -func VerifyCommitLightTrusting(chainID string, vals *ValidatorSet, commit *Commit, trustLevel cmtmath.Fraction) error { +func VerifyCommitLightTrustingAllSignatures( + chainID string, + vals *ValidatorSet, + commit *Commit, + trustLevel cmtmath.Fraction, +) error { + return verifyCommitLightTrustingInternal(chainID, vals, commit, trustLevel, true) +} + +func verifyCommitLightTrustingInternal( + chainID string, + vals *ValidatorSet, + commit *Commit, + trustLevel cmtmath.Fraction, + countAllSignatures bool, +) error { // sanity checks if vals == nil { return errors.New("nil validator set") @@ -121,12 +183,12 @@ func VerifyCommitLightTrusting(chainID string, vals *ValidatorSet, commit *Commi // up by address rather than index. if shouldBatchVerify(vals, commit) { return verifyCommitBatch(chainID, vals, commit, - votingPowerNeeded, ignore, count, false, false) + votingPowerNeeded, ignore, count, countAllSignatures, false) } // attempt with single verification return verifyCommitSingle(chainID, vals, commit, votingPowerNeeded, - ignore, count, false, false) + ignore, count, countAllSignatures, false) } // ValidateHash returns an error if the hash is not empty, but its diff --git a/types/validation_test.go b/types/validation_test.go index a6cdf818c3..f42ac88515 100644 --- a/types/validation_test.go +++ b/types/validation_test.go @@ -1,6 +1,7 @@ package types import ( + "strconv" "testing" "time" @@ -24,7 +25,7 @@ func TestValidatorSet_VerifyCommit_All(t *testing.T) { ) testCases := []struct { - description string + description, description2 string // description2, if not empty, is checked against VerifyCommitLightTrusting // vote chainID chainID string // vote blockID @@ -41,25 +42,32 @@ func TestValidatorSet_VerifyCommit_All(t *testing.T) { expErr bool }{ - {"good (batch verification)", chainID, blockID, 3, height, 3, 0, 0, false}, - {"good (single verification)", chainID, blockID, 1, height, 1, 0, 0, false}, + {"good (batch verification)", "", chainID, blockID, 3, height, 3, 0, 0, false}, + {"good (single verification)", "", chainID, blockID, 1, height, 1, 0, 0, false}, - {"wrong signature (#0)", "EpsilonEridani", blockID, 2, height, 2, 0, 0, true}, - {"wrong block ID", chainID, makeBlockIDRandom(), 2, height, 2, 0, 0, true}, - {"wrong height", chainID, blockID, 1, height - 1, 1, 0, 0, true}, + {"wrong signature (#0)", "", "EpsilonEridani", blockID, 2, height, 2, 0, 0, true}, + {"wrong block ID", "", chainID, makeBlockIDRandom(), 2, height, 2, 0, 0, true}, + {"wrong height", "", chainID, blockID, 1, height - 1, 1, 0, 0, true}, - {"wrong set size: 4 vs 3", chainID, blockID, 4, height, 3, 0, 0, true}, - {"wrong set size: 1 vs 2", chainID, blockID, 1, height, 2, 0, 0, true}, + {"wrong set size: 4 vs 3", "", chainID, blockID, 4, height, 3, 0, 0, true}, + {"wrong set size: 1 vs 2", "double vote from Validator", chainID, blockID, 1, height, 2, 0, 0, true}, - {"insufficient voting power: got 30, needed more than 66", chainID, blockID, 10, height, 3, 2, 5, true}, - {"insufficient voting power: got 0, needed more than 6", chainID, blockID, 1, height, 0, 0, 1, true}, - {"insufficient voting power: got 60, needed more than 60", chainID, blockID, 9, height, 6, 3, 0, true}, + {"insufficient voting power: got 30, needed more than 66", "", chainID, blockID, 10, height, 3, 2, 5, true}, + {"insufficient voting power: got 0, needed more than 6", "", chainID, blockID, 1, height, 0, 0, 1, true}, // absent + {"insufficient voting power: got 0, needed more than 6", "", chainID, blockID, 1, height, 0, 1, 0, true}, // nil + {"insufficient voting power: got 60, needed more than 60", "", chainID, blockID, 9, height, 6, 3, 0, true}, } for _, tc := range testCases { tc := tc +<<<<<<< HEAD t.Run(tc.description, func(t *testing.T) { _, valSet, vals := randVoteSet(tc.height, round, cmtproto.PrecommitType, tc.valSize, 10, false) +======= + countAllSignatures := false + f := func(t *testing.T) { + _, valSet, vals := randVoteSet(tc.height, round, PrecommitType, tc.valSize, 10, false) +>>>>>>> b70c4ab96 (`VerifyCommitLight` and `VerifyCommitLightTrusting` _never_ check all signatures (#1750)) totalVotes := tc.blockVotes + tc.absentVotes + tc.nilVotes sigs := make([]CommitSig, totalVotes) vi := 0 @@ -110,7 +118,11 @@ func TestValidatorSet_VerifyCommit_All(t *testing.T) { assert.NoError(t, err, "VerifyCommit") } - err = valSet.VerifyCommitLight(chainID, blockID, height, commit) + if countAllSignatures { + err = valSet.VerifyCommitLightAllSignatures(chainID, blockID, height, commit) + } else { + err = valSet.VerifyCommitLight(chainID, blockID, height, commit) + } if tc.expErr { if assert.Error(t, err, "VerifyCommitLight") { assert.Contains(t, err.Error(), tc.description, "VerifyCommitLight") @@ -120,18 +132,30 @@ func TestValidatorSet_VerifyCommit_All(t *testing.T) { } // only a subsection of the tests apply to VerifyCommitLightTrusting - if totalVotes != tc.valSize || !tc.blockID.Equals(blockID) || tc.height != height { - tc.expErr = false + expErr := tc.expErr + if (!countAllSignatures && totalVotes != tc.valSize) || totalVotes < tc.valSize || !tc.blockID.Equals(blockID) || tc.height != height { + expErr = false } - err = valSet.VerifyCommitLightTrusting(chainID, commit, trustLevel) - if tc.expErr { + if countAllSignatures { + err = valSet.VerifyCommitLightTrustingAllSignatures(chainID, commit, trustLevel) + } else { + err = valSet.VerifyCommitLightTrusting(chainID, commit, trustLevel) + } + if expErr { if assert.Error(t, err, "VerifyCommitLightTrusting") { - assert.Contains(t, err.Error(), tc.description, "VerifyCommitLightTrusting") + errStr := tc.description2 + if len(errStr) == 0 { + errStr = tc.description + } + assert.Contains(t, err.Error(), errStr, "VerifyCommitLightTrusting") } } else { assert.NoError(t, err, "VerifyCommitLightTrusting") } - }) + } + t.Run(tc.description+"/"+strconv.FormatBool(countAllSignatures), f) + countAllSignatures = true + t.Run(tc.description+"/"+strconv.FormatBool(countAllSignatures), f) } } @@ -163,7 +187,7 @@ func TestValidatorSet_VerifyCommit_CheckAllSignatures(t *testing.T) { } } -func TestValidatorSet_VerifyCommitLight_ReturnsAsSoonAsMajorityOfVotingPowerSigned(t *testing.T) { +func TestValidatorSet_VerifyCommitLight_ReturnsAsSoonAsMajOfVotingPowerSignedIffNotAllSigs(t *testing.T) { var ( chainID = "test_chain_id" h = int64(3) @@ -187,9 +211,11 @@ func TestValidatorSet_VerifyCommitLight_ReturnsAsSoonAsMajorityOfVotingPowerSign err = valSet.VerifyCommitLight(chainID, blockID, h, commit) assert.NoError(t, err) + err = valSet.VerifyCommitLightAllSignatures(chainID, blockID, h, commit) + assert.Error(t, err) // counting all signatures detects the malleated signature } -func TestValidatorSet_VerifyCommitLightTrusting_ReturnsAsSoonAsTrustLevelOfVotingPowerSigned(t *testing.T) { +func TestValidatorSet_VerifyCommitLightTrusting_ReturnsAsSoonAsTrustLevelSignedIffNotAllSigs(t *testing.T) { var ( chainID = "test_chain_id" h = int64(3) @@ -213,6 +239,12 @@ func TestValidatorSet_VerifyCommitLightTrusting_ReturnsAsSoonAsTrustLevelOfVotin err = valSet.VerifyCommitLightTrusting(chainID, commit, cmtmath.Fraction{Numerator: 1, Denominator: 3}) assert.NoError(t, err) + err = valSet.VerifyCommitLightTrustingAllSignatures( + chainID, + commit, + cmtmath.Fraction{Numerator: 1, Denominator: 3}, + ) + assert.Error(t, err) // counting all signatures detects the malleated signature } func TestValidatorSet_VerifyCommitLightTrusting(t *testing.T) { diff --git a/types/validator_set.go b/types/validator_set.go index 330d540baf..6511321519 100644 --- a/types/validator_set.go +++ b/types/validator_set.go @@ -666,18 +666,43 @@ func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, // LIGHT CLIENT VERIFICATION METHODS // VerifyCommitLight verifies +2/3 of the set had signed the given commit. +// It does NOT count all signatures. func (vals *ValidatorSet) VerifyCommitLight(chainID string, blockID BlockID, height int64, commit *Commit, ) error { return VerifyCommitLight(chainID, vals, blockID, height, commit) } +// VerifyCommitLight verifies +2/3 of the set had signed the given commit. +// It DOES count all signatures. +func (vals *ValidatorSet) VerifyCommitLightAllSignatures(chainID string, blockID BlockID, + height int64, commit *Commit, +) error { + return VerifyCommitLightAllSignatures(chainID, vals, blockID, height, commit) +} + // VerifyCommitLightTrusting verifies that trustLevel of the validator set signed // this commit. -func (vals *ValidatorSet) VerifyCommitLightTrusting(chainID string, commit *Commit, trustLevel cmtmath.Fraction) error { +// It does NOT count all signatures. +func (vals *ValidatorSet) VerifyCommitLightTrusting( + chainID string, + commit *Commit, + trustLevel cmtmath.Fraction, +) error { return VerifyCommitLightTrusting(chainID, vals, commit, trustLevel) } +// VerifyCommitLightTrusting verifies that trustLevel of the validator set signed +// this commit. +// It DOES count all signatures. +func (vals *ValidatorSet) VerifyCommitLightTrustingAllSignatures( + chainID string, + commit *Commit, + trustLevel cmtmath.Fraction, +) error { + return VerifyCommitLightTrustingAllSignatures(chainID, vals, commit, trustLevel) +} + // findPreviousProposer reverses the compare proposer priority function to find the validator // with the lowest proposer priority which would have been the previous proposer. //