Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

07-tendermint: ignore misbehaviour if age is greater than consensus params #6422

Merged
merged 14 commits into from
Jun 18, 2020
Merged

07-tendermint: ignore misbehaviour if age is greater than consensus params #6422

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
587d82e
07-tendermint: ignore misbehaviour if age is greater than consensus p…
fedekunze Jun 12, 2020
e7af375
Merge branch 'master' into fedekunze/6316-evidence-height
fedekunze Jun 12, 2020
eac6e55
Merge branch 'master' into fedekunze/6316-evidence-height
fedekunze Jun 12, 2020
3a6961e
Merge branch 'master' into fedekunze/6316-evidence-height
fedekunze Jun 17, 2020
5e4ab58
update tests and errors
fedekunze Jun 17, 2020
f4606a3
Merge branch 'fedekunze/6316-evidence-height' of github.com:cosmos/co…
fedekunze Jun 17, 2020
39b855b
add comment on consensus params
fedekunze Jun 17, 2020
ede6aba
Update x/ibc/02-client/handler.go
fedekunze Jun 17, 2020
5c6e002
Merge branch 'master' into fedekunze/6316-evidence-height
fedekunze Jun 17, 2020
3109c3f
Merge branch 'master' of github.com:cosmos/cosmos-sdk into fedekunze/…
fedekunze Jun 17, 2020
eab42dc
update evidence GetTime to use Max
fedekunze Jun 17, 2020
b309bde
Merge branch 'fedekunze/6316-evidence-height' of github.com:cosmos/co…
fedekunze Jun 17, 2020
e025226
Merge branch 'master' into fedekunze/6316-evidence-height
fedekunze Jun 18, 2020
6fc65a1
Merge branch 'master' into fedekunze/6316-evidence-height
fedekunze Jun 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion x/ibc/02-client/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion x/ibc/02-client/keeper/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
)

default:
Expand Down
40 changes: 31 additions & 9 deletions x/ibc/07-tendermint/misbehaviour.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand All @@ -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())
Expand All @@ -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 &&
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
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()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand what this is checking? If we want to make sure that the evidence isn't at a height in the future shouldn't we check the opposite i.e that if height < evidence.GetHeight()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this height is the height from which we retrieve the old header to test from, so it must be before the two conflicting headers.

return sdkerrors.Wrapf(
Expand All @@ -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,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we were still using the default for misbehavior 👀

); 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)
}
Expand Down
131 changes: 124 additions & 7 deletions x/ibc/07-tendermint/misbehaviour_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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",
Expand All @@ -57,7 +62,9 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviour() {
ChainID: chainID,
ClientID: chainID,
},
simapp.DefaultConsensusParams,
height,
suite.now,
true,
},
{
Expand All @@ -70,7 +77,9 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviour() {
ChainID: chainID,
ClientID: chainID,
},
simapp.DefaultConsensusParams,
height - 1,
suite.now,
true,
},
{
Expand All @@ -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()),
Expand All @@ -96,7 +207,9 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviour() {
ChainID: chainID,
ClientID: chainID,
},
simapp.DefaultConsensusParams,
height,
suite.now,
false,
},
{
Expand All @@ -109,7 +222,9 @@ func (suite *TendermintTestSuite) TestCheckMisbehaviour() {
ChainID: chainID,
ClientID: chainID,
},
simapp.DefaultConsensusParams,
height,
suite.now,
false,
},
{
Expand All @@ -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)
Expand Down
9 changes: 9 additions & 0 deletions x/ibc/07-tendermint/types/evidence.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package types

import (
"math"
"time"

yaml "gopkg.in/yaml.v2"

Expand Down Expand Up @@ -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 {
Expand Down