types: simplify VerifyCommitTrusting

Closes #4783 

It looks like we're validating Commit twice. Also, height and blockID params were coming from the commit, so no need to pass them separately.

CHANGELOG_PENDING.md

@@ -23,6 +23,7 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi
 
   - [crypto] [\#4721](https://github.com/tendermint/tendermint/pull/4721) Remove `SimpleHashFromMap()` and `SimpleProofsFromMap()` (@erikgrinaker)
   - [privval] [\#4744](https://github.com/tendermint/tendermint/pull/4744) Remove deprecated `OldFilePV` (@melekes)
+  - [types] \#4798 Simplify `VerifyCommitTrusting` func + remove extra validation (@melekes)
 
 - Blockchain Protocol
 

lite2/client.go

@@ -954,8 +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,
-					altH.Height, altH.Commit, c.trustLevel); err != nil {
+				if err = c.latestTrustedVals.VerifyCommitTrusting(c.chainID, altH.Commit, c.trustLevel); err != nil {
 					c.logger.Error("Witness sent us incorrect header", "err", err, "witness", witness)
 					witnessesToRemove = append(witnessesToRemove, i)
 					continue

lite2/verifier.go

@@ -57,8 +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,
-		untrustedHeader.Commit, trustLevel)
+	err := trustedVals.VerifyCommitTrusting(chainID, untrustedHeader.Commit, trustLevel)
 	if err != nil {
 		switch e := err.(type) {
 		case types.ErrNotEnoughVotingPowerSigned:

types/evidence.go

@@ -509,8 +509,6 @@ func (ev ConflictingHeadersEvidence) VerifyComposite(committedHeader *Header, va
 	// trusted validator set.
 	if err := valSet.VerifyCommitTrusting(
 		alternativeHeader.ChainID,
-		alternativeHeader.Commit.BlockID,
-		alternativeHeader.Height,
 		alternativeHeader.Commit,
 		tmmath.Fraction{Numerator: 1, Denominator: 3}); err != nil {
 		return errors.Wrap(err, "alt header does not have 1/3+ of voting power of our validator set")

types/validator_set.go

@@ -641,9 +641,17 @@ func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID,
 	if vals.Size() != len(commit.Signatures) {
 		return NewErrInvalidCommitSignatures(vals.Size(), len(commit.Signatures))
 	}
-	if err := verifyCommitBasic(commit, height, blockID); err != nil {
+
+	if err := commit.ValidateBasic(); err != nil {
 		return err
 	}
+	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
@@ -756,24 +764,15 @@ func (vals *ValidatorSet) VerifyFutureCommit(newSet *ValidatorSet, chainID strin
 	return nil
 }
 
-// VerifyCommitTrusting verifies that trustLevel ([1/3, 1]) of the validator
-// set signed this commit.
+// VerifyCommitTrusting 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.
-//
-// Panics if trustLevel is invalid.
-func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, blockID BlockID,
-	height int64, commit *Commit, trustLevel tmmath.Fraction) error {
-
+func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, commit *Commit, trustLevel tmmath.Fraction) error {
 	// sanity check
-	if trustLevel.Numerator*3 < trustLevel.Denominator || // < 1/3
-		trustLevel.Numerator > trustLevel.Denominator { // > 1
-		panic(fmt.Sprintf("trustLevel must be within [1/3, 1], given %v", trustLevel))
-	}
-
-	if err := verifyCommitBasic(commit, height, blockID); err != nil {
-		return err
+	if trustLevel.Denominator == 0 {
+		return errors.New("trustLevel has zero Denominator")
 	}
 
 	var (
@@ -812,12 +811,12 @@ func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, blockID BlockID,
 			}
 
 			// Good!
-			if blockID.Equals(commitSig.BlockID(commit.BlockID)) {
+			if commitSig.ForBlock() {
 				talliedVotingPower += val.VotingPower
 			}
 			// else {
-			// It's OK that the BlockID doesn't match.  We include stray
-			// signatures (~votes for nil) to measure validator availability.
+			// It's OK. We include stray signatures (~votes for nil) to measure
+			// validator availability.
 			// }
 
 			if talliedVotingPower > votingPowerNeeded {
@@ -829,20 +828,6 @@ func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, blockID BlockID,
 	return ErrNotEnoughVotingPowerSigned{Got: talliedVotingPower, Needed: votingPowerNeeded}
 }
 
-func verifyCommitBasic(commit *Commit, height int64, blockID BlockID) error {
-	if err := commit.ValidateBasic(); err != nil {
-		return err
-	}
-	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)
-	}
-	return nil
-}
-
 //-----------------
 
 // IsErrNotEnoughVotingPowerSigned returns true if err is

types/validator_set_test.go

@@ -1357,7 +1357,7 @@ func TestVerifyCommitTrusting(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		err = tc.valSet.VerifyCommitTrusting("test_chain_id", blockID, commit.Height, commit,
+		err = tc.valSet.VerifyCommitTrusting("test_chain_id", commit,
 			tmmath.Fraction{Numerator: 1, Denominator: 3})
 		if tc.err {
 			assert.Error(t, err)
@@ -1375,7 +1375,7 @@ func TestVerifyCommitTrustingErrorsOnOverflow(t *testing.T) {
 	)
 	require.NoError(t, err)
 
-	err = valSet.VerifyCommitTrusting("test_chain_id", blockID, commit.Height, commit,
+	err = valSet.VerifyCommitTrusting("test_chain_id", commit,
 		tmmath.Fraction{Numerator: 25, Denominator: 55})
 	if assert.Error(t, err) {
 		assert.Contains(t, err.Error(), "int64 overflow")