Introduce countAllSignatures in VerifyCommitLight & VerifyCommitLightTrusting (#1806) (#1815)

* Introduce `countAllSignatures` in `VerifyCommitLight` & `VerifyCommitLightTrusting`

* Revert unneded change

* Addressed @insumity's comments

---------

Co-authored-by: Jasmina Malicevic <[email protected]>

Author: sergio-mena

blockchain/v0/reactor.go

@@ -363,7 +363,7 @@ FOR_LOOP:
 			// NOTE: we can probably make this more efficient, but note that calling
 			// first.Hash() doesn't verify the tx contents, so MakePartSet() is
 			// currently necessary.
-			err := state.Validators.VerifyCommitLight(
+			err := state.Validators.VerifyCommitLightAllSignatures(
 				chainID, firstID, first.Height, second.LastCommit)
 
 			if err == nil {

blockchain/v1/reactor.go

@@ -475,7 +475,7 @@ func (bcR *BlockchainReactor) processBlock() error {
 	// NOTE: we can probably make this more efficient, but note that calling
 	// first.Hash() doesn't verify the tx contents, so MakePartSet() is
 	// currently necessary.
-	err = bcR.state.Validators.VerifyCommitLight(chainID, firstID, first.Height, second.LastCommit)
+	err = bcR.state.Validators.VerifyCommitLightAllSignatures(chainID, firstID, first.Height, second.LastCommit)
 	if err != nil {
 		bcR.Logger.Error("error during commit verification", "err", err,
 			"first", first.Height, "second", second.Height)

blockchain/v2/processor_context.go

@@ -44,7 +44,7 @@ func (pc *pContext) setState(state state.State) {
 }
 
 func (pc pContext) verifyCommit(chainID string, blockID types.BlockID, height int64, commit *types.Commit) error {
-	return pc.state.Validators.VerifyCommitLight(chainID, blockID, height, commit)
+	return pc.state.Validators.VerifyCommitLightAllSignatures(chainID, blockID, height, commit)
 }
 
 func (pc *pContext) saveBlock(block *types.Block, blockParts *types.PartSet, seenCommit *types.Commit) {

evidence/pool.go

@@ -132,19 +132,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
 	}
 
@@ -505,13 +505,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
 		}
 

evidence/verify.go

@@ -106,6 +106,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
 //
@@ -115,7 +116,7 @@ func VerifyLightClientAttack(e *types.LightClientAttackEvidence, commonHeader, t
 	// 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)
 		}
@@ -127,7 +128,7 @@ func VerifyLightClientAttack(e *types.LightClientAttackEvidence, commonHeader, t
 	}
 
 	// 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)
 	}

test/e2e/app/app.go

@@ -149,6 +149,19 @@ func (app *Application) DeliverTx(req abci.RequestDeliverTx) abci.ResponseDelive
 	return abci.ResponseDeliverTx{Code: code.CodeTypeOK}
 }
 
+func (app *Application) BeginBlock(req abci.RequestBeginBlock) abci.ResponseBeginBlock {
+	for _, ev := range req.ByzantineValidators {
+		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(),
+		)
+	}
+	return abci.ResponseBeginBlock{}
+}
+
 // EndBlock implements ABCI.
 func (app *Application) EndBlock(req abci.RequestEndBlock) abci.ResponseEndBlock {
 	valUpdates, err := app.validatorUpdates(uint64(req.Height))

types/validator_set.go

@@ -717,10 +717,36 @@ func (vals *ValidatorSet) VerifyCommit(chainID string, 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 (vals *ValidatorSet) VerifyCommitLight(chainID string, blockID BlockID,
-	height int64, commit *Commit) error {
+func (vals *ValidatorSet) VerifyCommitLight(
+	chainID string,
+	blockID BlockID,
+	height int64,
+	commit *Commit,
+) error {
+	return vals.verifyCommitLightInternal(chainID, blockID, height, commit, false)
+}
+
+// VerifyCommitLightAllSignatures verifies +2/3 of the set had signed the given commit.
+//
+// This method DOES check all the signatures.
+func (vals *ValidatorSet) VerifyCommitLightAllSignatures(
+	chainID string,
+	blockID BlockID,
+	height int64,
+	commit *Commit,
+) error {
+	return vals.verifyCommitLightInternal(chainID, blockID, height, commit, true)
+}
+
+func (vals *ValidatorSet) verifyCommitLightInternal(
+	chainID string,
+	blockID BlockID,
+	height int64,
+	commit *Commit,
+	countAllSignatures bool,
+) error {
 
 	if vals.Size() != len(commit.Signatures) {
 		return NewErrInvalidCommitSignatures(vals.Size(), len(commit.Signatures))
@@ -738,8 +764,9 @@ func (vals *ValidatorSet) VerifyCommitLight(chainID string, blockID 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() {
+		// No need to verify absent or nil votes if not counting all signatures,
+		// but need to verify nil votes if counting all signatures.
+		if (!countAllSignatures && !commitSig.ForBlock()) || (countAllSignatures && commitSig.Absent()) {
 			continue
 		}
 
@@ -756,11 +783,14 @@ func (vals *ValidatorSet) VerifyCommitLight(chainID string, blockID BlockID,
 		talliedVotingPower += val.VotingPower
 
 		// return as soon as +2/3 of the signatures are verified
-		if talliedVotingPower > votingPowerNeeded {
+		if !countAllSignatures && talliedVotingPower > votingPowerNeeded {
 			return nil
 		}
 	}
 
+	if talliedVotingPower > votingPowerNeeded {
+		return nil
+	}
 	return ErrNotEnoughVotingPowerSigned{Got: talliedVotingPower, Needed: votingPowerNeeded}
 }
 
@@ -770,9 +800,37 @@ func (vals *ValidatorSet) VerifyCommitLight(chainID string, 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 (vals *ValidatorSet) VerifyCommitLightTrusting(chainID string, commit *Commit, trustLevel cmtmath.Fraction) error {
+func (vals *ValidatorSet) VerifyCommitLightTrusting(
+	chainID string,
+	commit *Commit,
+	trustLevel cmtmath.Fraction,
+) error {
+	return vals.verifyCommitLightTrustingInternal(chainID, 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 DOES check all the signatures.
+func (vals *ValidatorSet) VerifyCommitLightTrustingAllSignatures(
+	chainID string,
+	commit *Commit,
+	trustLevel cmtmath.Fraction,
+) error {
+	return vals.verifyCommitLightTrustingInternal(chainID, commit, trustLevel, true)
+}
+
+func (vals *ValidatorSet) verifyCommitLightTrustingInternal(
+	chainID string,
+	commit *Commit,
+	trustLevel cmtmath.Fraction,
+	countAllSignatures bool,
+) error {
 	// sanity check
 	if trustLevel.Denominator == 0 {
 		return errors.New("trustLevel has zero Denominator")
@@ -791,8 +849,9 @@ func (vals *ValidatorSet) VerifyCommitLightTrusting(chainID string, commit *Comm
 	votingPowerNeeded := totalVotingPowerMulByNumerator / int64(trustLevel.Denominator)
 
 	for idx, commitSig := range commit.Signatures {
-		// No need to verify absent or nil votes.
-		if !commitSig.ForBlock() {
+		// No need to verify absent or nil votes if not counting all signatures,
+		// but need to verify nil votes if counting all signatures.
+		if (!countAllSignatures && !commitSig.ForBlock()) || (countAllSignatures && commitSig.Absent()) {
 			continue
 		}
 
@@ -816,12 +875,14 @@ func (vals *ValidatorSet) VerifyCommitLightTrusting(chainID string, commit *Comm
 
 			talliedVotingPower += val.VotingPower
 
-			if talliedVotingPower > votingPowerNeeded {
+			if !countAllSignatures && talliedVotingPower > votingPowerNeeded {
 				return nil
 			}
 		}
 	}
-
+	if talliedVotingPower > votingPowerNeeded {
+		return nil
+	}
 	return ErrNotEnoughVotingPowerSigned{Got: talliedVotingPower, Needed: votingPowerNeeded}
 }
 

types/validator_set_test.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"math"
 	"sort"
+	"strconv"
 	"strings"
 	"testing"
 	"testing/quick"
@@ -725,7 +726,8 @@ func TestValidatorSet_VerifyCommit_All(t *testing.T) {
 
 	for _, tc := range testCases {
 		tc := tc
-		t.Run(tc.description, func(t *testing.T) {
+		countAllSignatures := false
+		f := func(t *testing.T) {
 			err := vset.VerifyCommit(tc.chainID, tc.blockID, tc.height, tc.commit)
 			if tc.expErr {
 				if assert.Error(t, err, "VerifyCommit") {
@@ -735,15 +737,22 @@ func TestValidatorSet_VerifyCommit_All(t *testing.T) {
 				assert.NoError(t, err, "VerifyCommit")
 			}
 
-			err = vset.VerifyCommitLight(tc.chainID, tc.blockID, tc.height, tc.commit)
+			if countAllSignatures {
+				err = vset.VerifyCommitLightAllSignatures(tc.chainID, tc.blockID, tc.height, tc.commit)
+			} else {
+				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")
 			}
-		})
+		}
+		t.Run(tc.description+"/"+strconv.FormatBool(countAllSignatures), f)
+		countAllSignatures = true
+		t.Run(tc.description+"/"+strconv.FormatBool(countAllSignatures), f)
 	}
 }
 
@@ -772,7 +781,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)
@@ -783,6 +792,9 @@ func TestValidatorSet_VerifyCommitLight_ReturnsAsSoonAsMajorityOfVotingPowerSign
 	commit, err := MakeCommit(blockID, h, 0, voteSet, vals, time.Now())
 	require.NoError(t, err)
 
+	err = valSet.VerifyCommitLightAllSignatures(chainID, blockID, h, commit)
+	assert.NoError(t, err)
+
 	// malleate 4th signature (3 signatures are enough for 2/3+)
 	vote := voteSet.GetByIndex(3)
 	v := vote.ToProto()
@@ -793,9 +805,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)
@@ -806,6 +820,13 @@ func TestValidatorSet_VerifyCommitLightTrusting_ReturnsAsSoonAsTrustLevelOfVotin
 	commit, err := MakeCommit(blockID, h, 0, voteSet, vals, time.Now())
 	require.NoError(t, err)
 
+	err = valSet.VerifyCommitLightTrustingAllSignatures(
+		chainID,
+		commit,
+		cmtmath.Fraction{Numerator: 1, Denominator: 3},
+	)
+	assert.NoError(t, err)
+
 	// malleate 3rd signature (2 signatures are enough for 1/3+ trust level)
 	vote := voteSet.GetByIndex(2)
 	v := vote.ToProto()
@@ -816,6 +837,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 TestEmptySet(t *testing.T) {