diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index eb8a1117d..ac1f84c1b 100644 --- a/CHANGELOG_PENDING.md +++ b/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 diff --git a/lite2/client.go b/lite2/client.go index 8d93c2c58..8442adcbe 100644 --- a/lite2/client.go +++ b/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 diff --git a/lite2/verifier.go b/lite2/verifier.go index 1ef54677b..6acae5cd5 100644 --- a/lite2/verifier.go +++ b/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: diff --git a/types/evidence.go b/types/evidence.go index d63a13c5f..8fe7ab80e 100644 --- a/types/evidence.go +++ b/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") diff --git a/types/validator_set.go b/types/validator_set.go index 07b50f5ff..63573e90f 100644 --- a/types/validator_set.go +++ b/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 diff --git a/types/validator_set_test.go b/types/validator_set_test.go index 0e73e800a..f62a540ae 100644 --- a/types/validator_set_test.go +++ b/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")