VerifyCommitLight and VerifyCommitLightTrusting _never_ check all…

… signatures (cometbft#1750)

* [e2e] Repro evidence bug: not checking all signatures

* _Only_ callsites in the light client should be skipping verification of commit signatures

* Address PR comments

* Rename UT

---------

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>

internal/blocksync/reactor.go

@@ -460,7 +460,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 {

internal/evidence/pool.go

@@ -133,19 +133,19 @@ 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
 	}
 
 	// check that the evidence isn't already committed
 	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
 	}
 
@@ -514,13 +514,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
 		}
 

internal/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 ErrConflictingBlock{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 ErrConflictingBlock{fmt.Errorf("invalid commit from conflicting block: %w", err)}
 	}

test/e2e/app/app.go

@@ -238,6 +238,16 @@ func (app *Application) FinalizeBlock(_ context.Context, req *abci.FinalizeBlock
 		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)

test/e2e/runner/evidence.go

@@ -24,7 +24,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.
@@ -87,10 +87,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
@@ -110,7 +112,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
 		}
 	}
@@ -155,6 +165,7 @@ func generateLightClientAttackEvidence(
 	vals *types.ValidatorSet,
 	chainID string,
 	evTime time.Time,
+	validEvidence bool,
 ) (*types.LightClientAttackEvidence, error) {
 	// forge a random header
 	forgedHeight := height + 2
@@ -164,7 +175,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
 	}
@@ -179,6 +190,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{
@@ -292,18 +308,26 @@ 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 {
 		return nil, nil, err
 	}
 
 	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

types/validation.go

@@ -55,10 +55,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
@@ -76,12 +106,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
@@ -90,9 +120,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")
@@ -122,12 +184,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