07-tendermint: ignore misbehaviour if age is greater than consensus p…

…arams (cosmos#6422) * 07-tendermint: ignore misbehaviour if age is greater than consensus params * update tests and errors * add comment on consensus params * Update x/ibc/02-client/handler.go Co-authored-by: Anton Kaliaev <[email protected]> * update evidence GetTime to use Max Co-authored-by: Anton Kaliaev <[email protected]>
cabemo · Jun 18, 2020 · fe9356d · fe9356d
1 parent 257354d
commit fe9356d
Show file tree

Hide file tree

Showing 5 changed files with 168 additions and 18 deletions.
diff --git a/x/ibc/02-client/handler.go b/x/ibc/02-client/handler.go
@@ -86,7 +86,9 @@ func HandlerClientMisbehaviour(k Keeper) evidencetypes.Handler {
 	return func(ctx sdk.Context, evidence evidenceexported.Evidence) error {
 		misbehaviour, ok := evidence.(exported.Misbehaviour)
 		if !ok {
-			return types.ErrInvalidEvidence
+			return sdkerrors.Wrapf(types.ErrInvalidEvidence,
+				"expected evidence to implement client Misbehaviour interface, got %T", evidence,
+			)
 		}
 
 		return k.CheckMisbehaviourAndUpdateState(ctx, misbehaviour)

diff --git a/x/ibc/02-client/keeper/client.go b/x/ibc/02-client/keeper/client.go
@@ -140,7 +140,7 @@ func (k Keeper) CheckMisbehaviourAndUpdateState(ctx sdk.Context, misbehaviour ex
 	switch e := misbehaviour.(type) {
 	case ibctmtypes.Evidence:
 		clientState, err = tendermint.CheckMisbehaviourAndUpdateState(
-			clientState, consensusState, misbehaviour, consensusState.GetHeight(), ctx.BlockTime(),
+			clientState, consensusState, misbehaviour, consensusState.GetHeight(), ctx.BlockTime(), ctx.ConsensusParams(),
 		)
 
 	default:

diff --git a/x/ibc/07-tendermint/misbehaviour.go b/x/ibc/07-tendermint/misbehaviour.go
@@ -3,7 +3,7 @@ package tendermint
 import (
 	"time"
 
-	lite "github.com/tendermint/tendermint/lite2"
+	abci "github.com/tendermint/tendermint/abci/types"
 
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	clientexported "github.com/cosmos/cosmos-sdk/x/ibc/02-client/exported"
@@ -22,6 +22,7 @@ func CheckMisbehaviourAndUpdateState(
 	misbehaviour clientexported.Misbehaviour,
 	height uint64, // height at which the consensus state was loaded
 	currentTimestamp time.Time,
+	consensusParams *abci.ConsensusParams,
 ) (clientexported.ClientState, error) {
 
 	// cast the interface to specific types before checking for misbehaviour
@@ -47,9 +48,9 @@ func CheckMisbehaviourAndUpdateState(
 	}
 
 	if err := checkMisbehaviour(
-		tmClientState, tmConsensusState, tmEvidence, height, currentTimestamp,
+		tmClientState, tmConsensusState, tmEvidence, height, currentTimestamp, consensusParams,
 	); err != nil {
-		return nil, sdkerrors.Wrap(clienttypes.ErrInvalidEvidence, err.Error())
+		return nil, err
 	}
 
 	tmClientState.FrozenHeight = uint64(tmEvidence.GetHeight())
@@ -59,8 +60,32 @@ func CheckMisbehaviourAndUpdateState(
 // checkMisbehaviour checks if the evidence provided is a valid light client misbehaviour
 func checkMisbehaviour(
 	clientState types.ClientState, consensusState types.ConsensusState, evidence types.Evidence,
-	height uint64, currentTimestamp time.Time,
+	height uint64, currentTimestamp time.Time, consensusParams *abci.ConsensusParams,
 ) error {
+	// calculate the age of the misbehaviour evidence
+	infractionHeight := evidence.GetHeight()
+	infractionTime := evidence.GetTime()
+	ageDuration := currentTimestamp.Sub(infractionTime)
+	ageBlocks := height - uint64(infractionHeight)
+
+	// Reject misbehaviour if the age is too old. Evidence is considered stale
+	// if the difference in time and number of blocks is greater than the allowed
+	// parameters defined.
+	//
+	// NOTE: The first condition is a safety check as the consensus params cannot
+	// be nil since the previous param values will be used in case they can't be
+	// retreived. If they are not set during initialization, Tendermint will always
+	// use the default values.
+	if consensusParams != nil &&
+		consensusParams.Evidence != nil &&
+		ageDuration > consensusParams.Evidence.MaxAgeDuration &&
+		ageBlocks > uint64(consensusParams.Evidence.MaxAgeNumBlocks) {
+		return sdkerrors.Wrapf(clienttypes.ErrInvalidEvidence,
+			"age duration (%s) and age blocks (%d) are greater than max consensus params for duration (%s) and block (%d)",
+			ageDuration, ageBlocks, consensusParams.Evidence.MaxAgeDuration, consensusParams.Evidence.MaxAgeNumBlocks,
+		)
+	}
+
 	// check if provided height matches the headers' height
 	if height > uint64(evidence.GetHeight()) {
 		return sdkerrors.Wrapf(
@@ -81,21 +106,18 @@ func checkMisbehaviour(
 		)
 	}
 
-	// TODO: Evidence must be within trusting period
-	// Blocked on https://github.com/cosmos/ics/issues/379
-
 	// - ValidatorSet must have 2/3 similarity with trusted FromValidatorSet
 	// - ValidatorSets on both headers are valid given the last trusted ValidatorSet
 	if err := consensusState.ValidatorSet.VerifyCommitTrusting(
 		evidence.ChainID, evidence.Header1.Commit.BlockID, evidence.Header1.Height,
-		evidence.Header1.Commit, lite.DefaultTrustLevel,
+		evidence.Header1.Commit, clientState.TrustLevel,
 	); err != nil {
 		return sdkerrors.Wrapf(clienttypes.ErrInvalidEvidence, "validator set in header 1 has too much change from last known validator set: %v", err)
 	}
 
 	if err := consensusState.ValidatorSet.VerifyCommitTrusting(
 		evidence.ChainID, evidence.Header2.Commit.BlockID, evidence.Header2.Height,
-		evidence.Header2.Commit, lite.DefaultTrustLevel,
+		evidence.Header2.Commit, clientState.TrustLevel,
 	); err != nil {
 		return sdkerrors.Wrapf(clienttypes.ErrInvalidEvidence, "validator set in header 2 has too much change from last known validator set: %v", err)
 	}

diff --git a/x/ibc/07-tendermint/misbehaviour_test.go b/x/ibc/07-tendermint/misbehaviour_test.go
@@ -4,10 +4,13 @@ import (
 	"bytes"
 	"time"
 
+	abci "github.com/tendermint/tendermint/abci/types"
 	"github.com/tendermint/tendermint/crypto/tmhash"
 	lite "github.com/tendermint/tendermint/lite2"
 	tmtypes "github.com/tendermint/tendermint/types"
 
+	"github.com/cosmos/cosmos-sdk/simapp"
+	clientexported "github.com/cosmos/cosmos-sdk/x/ibc/02-client/exported"
 	tendermint "github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint"
 	ibctmtypes "github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint/types"
 	commitmenttypes "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment/types"
@@ -40,12 +43,14 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviour() {
 	altSigners := []tmtypes.PrivValidator{altPrivVal}
 
 	testCases := []struct {
-		name           string
-		clientState    ibctmtypes.ClientState
-		consensusState ibctmtypes.ConsensusState
-		evidence       ibctmtypes.Evidence
-		height         uint64
-		expPass        bool
+		name            string
+		clientState     clientexported.ClientState
+		consensusState  clientexported.ConsensusState
+		evidence        clientexported.Misbehaviour
+		consensusParams *abci.ConsensusParams
+		height          uint64
+		timestamp       time.Time
+		expPass         bool
 	}{
 		{
 			"valid misbehavior evidence",
@@ -57,7 +62,9 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviour() {
 				ChainID:  chainID,
 				ClientID: chainID,
 			},
+			simapp.DefaultConsensusParams,
 			height,
+			suite.now,
 			true,
 		},
 		{
@@ -70,7 +77,9 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviour() {
 				ChainID:  chainID,
 				ClientID: chainID,
 			},
+			simapp.DefaultConsensusParams,
 			height - 1,
+			suite.now,
 			true,
 		},
 		{
@@ -83,9 +92,111 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviour() {
 				ChainID:  chainID,
 				ClientID: chainID,
 			},
+			simapp.DefaultConsensusParams,
 			height - 1,
+			suite.now,
 			true,
 		},
+		{
+			"invalid tendermint client state",
+			nil,
+			ibctmtypes.ConsensusState{Timestamp: suite.now, Root: commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), ValidatorSet: bothValSet},
+			ibctmtypes.Evidence{
+				Header1:  ibctmtypes.CreateTestHeader(chainID, height, suite.now, altValSet, altSigners),
+				Header2:  ibctmtypes.CreateTestHeader(chainID, height, suite.now.Add(time.Minute), bothValSet, bothSigners),
+				ChainID:  chainID,
+				ClientID: chainID,
+			},
+			simapp.DefaultConsensusParams,
+			height,
+			suite.now,
+			false,
+		},
+		{
+			"already frozen client state",
+			ibctmtypes.ClientState{FrozenHeight: 1},
+			ibctmtypes.ConsensusState{Timestamp: suite.now, Root: commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), ValidatorSet: bothValSet},
+			ibctmtypes.Evidence{
+				Header1:  ibctmtypes.CreateTestHeader(chainID, height, suite.now, bothValSet, bothSigners),
+				Header2:  ibctmtypes.CreateTestHeader(chainID, height, suite.now.Add(time.Minute), bothValSet, bothSigners),
+				ChainID:  chainID,
+				ClientID: chainID,
+			},
+			simapp.DefaultConsensusParams,
+			height,
+			suite.now,
+			false,
+		},
+		{
+			"invalid tendermint consensus state",
+			ibctmtypes.NewClientState(chainID, lite.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, suite.header, commitmenttypes.GetSDKSpecs()),
+			nil,
+			ibctmtypes.Evidence{
+				Header1:  ibctmtypes.CreateTestHeader(chainID, height, suite.now, altValSet, altSigners),
+				Header2:  ibctmtypes.CreateTestHeader(chainID, height, suite.now.Add(time.Minute), bothValSet, bothSigners),
+				ChainID:  chainID,
+				ClientID: chainID,
+			},
+			simapp.DefaultConsensusParams,
+			height,
+			suite.now,
+			false,
+		},
+		{
+			"invalid tendermint misbehaviour evidence",
+			ibctmtypes.NewClientState(chainID, lite.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, suite.header, commitmenttypes.GetSDKSpecs()),
+			ibctmtypes.ConsensusState{Timestamp: suite.now, Root: commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), ValidatorSet: bothValSet},
+			nil,
+			simapp.DefaultConsensusParams,
+			height,
+			suite.now,
+			false,
+		},
+		{
+			"rejected misbehaviour due to expired age",
+			ibctmtypes.NewClientState(chainID, lite.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, suite.header, commitmenttypes.GetSDKSpecs()),
+			ibctmtypes.ConsensusState{Timestamp: suite.now, Root: commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), ValidatorSet: bothValSet},
+			ibctmtypes.Evidence{
+				Header1:  ibctmtypes.CreateTestHeader(chainID, height, suite.now, bothValSet, bothSigners),
+				Header2:  ibctmtypes.CreateTestHeader(chainID, height, suite.now.Add(time.Minute), bothValSet, bothSigners),
+				ChainID:  chainID,
+				ClientID: chainID,
+			},
+			simapp.DefaultConsensusParams,
+			2*height + uint64(simapp.DefaultConsensusParams.Evidence.MaxAgeNumBlocks),
+			suite.now.Add(2 * time.Minute).Add(simapp.DefaultConsensusParams.Evidence.MaxAgeDuration),
+			false,
+		},
+		{
+			"provided height ≠ header height",
+			ibctmtypes.NewClientState(chainID, lite.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, suite.header, commitmenttypes.GetSDKSpecs()),
+			ibctmtypes.ConsensusState{Timestamp: suite.now, Root: commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), ValidatorSet: bothValSet},
+			ibctmtypes.Evidence{
+				Header1:  ibctmtypes.CreateTestHeader(chainID, height, suite.now, bothValSet, bothSigners),
+				Header2:  ibctmtypes.CreateTestHeader(chainID, height, suite.now.Add(time.Minute), bothValSet, bothSigners),
+				ChainID:  chainID,
+				ClientID: chainID,
+			},
+			simapp.DefaultConsensusParams,
+			height + 10,
+			suite.now,
+			false,
+		},
+		{
+			"unbonding period expired",
+			ibctmtypes.NewClientState(chainID, lite.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, suite.header, commitmenttypes.GetSDKSpecs()),
+			ibctmtypes.ConsensusState{Timestamp: time.Time{}, Root: commitmenttypes.NewMerkleRoot(tmhash.Sum([]byte("app_hash"))), ValidatorSet: bothValSet},
+			ibctmtypes.Evidence{
+				Header1:  ibctmtypes.CreateTestHeader(chainID, height, suite.now, bothValSet, bothSigners),
+				Header2:  ibctmtypes.CreateTestHeader(chainID, height, suite.now.Add(time.Minute), bothValSet, bothSigners),
+				ChainID:  chainID,
+				ClientID: chainID,
+			},
+			simapp.DefaultConsensusParams,
+			height,
+			suite.now,
+			false,
+		},
 		{
 			"first valset has too much change",
 			ibctmtypes.NewClientState(chainID, lite.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, suite.header, commitmenttypes.GetSDKSpecs()),
@@ -96,7 +207,9 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviour() {
 				ChainID:  chainID,
 				ClientID: chainID,
 			},
+			simapp.DefaultConsensusParams,
 			height,
+			suite.now,
 			false,
 		},
 		{
@@ -109,7 +222,9 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviour() {
 				ChainID:  chainID,
 				ClientID: chainID,
 			},
+			simapp.DefaultConsensusParams,
 			height,
+			suite.now,
 			false,
 		},
 		{
@@ -122,15 +237,17 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviour() {
 				ChainID:  chainID,
 				ClientID: chainID,
 			},
+			simapp.DefaultConsensusParams,
 			height,
+			suite.now,
 			false,
 		},
 	}
 
 	for i, tc := range testCases {
 		tc := tc
 
-		clientState, err := tendermint.CheckMisbehaviourAndUpdateState(tc.clientState, tc.consensusState, tc.evidence, tc.height, suite.now)
+		clientState, err := tendermint.CheckMisbehaviourAndUpdateState(tc.clientState, tc.consensusState, tc.evidence, tc.height, tc.timestamp, tc.consensusParams)
 
 		if tc.expPass {
 			suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)

diff --git a/x/ibc/07-tendermint/types/evidence.go b/x/ibc/07-tendermint/types/evidence.go
@@ -2,6 +2,7 @@ package types
 
 import (
 	"math"
+	"time"
 
 	yaml "gopkg.in/yaml.v2"
 
@@ -73,6 +74,14 @@ func (ev Evidence) GetHeight() int64 {
 	return int64(math.Min(float64(ev.Header1.Height), float64(ev.Header2.Height)))
 }
 
+// GetTime returns the timestamp at which misbehaviour occurred. It uses the
+// maximum value from both headers to prevent producing an invalid header outside
+// of the evidence age range.
+func (ev Evidence) GetTime() time.Time {
+	minTime := int64(math.Max(float64(ev.Header1.Time.UnixNano()), float64(ev.Header2.Time.UnixNano())))
+	return time.Unix(0, minTime)
+}
+
 // ValidateBasic implements Evidence interface
 func (ev Evidence) ValidateBasic() error {
 	if err := host.ClientIdentifierValidator(ev.ClientID); err != nil {