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

Jump to bottom

migrate tm evidence to proto #7145

Merged
merged 4 commits into from
Aug 24, 2020
Merged

migrate tm evidence to proto #7145

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8bce5a
migrate tm evidence to proto
colin-axner Aug 24, 2020
8e7e5fa
Merge branch 'master' into colin/migrate-tm-evidence
fedekunze Aug 24, 2020
a18020f
Update proto/ibc/tendermint/tendermint.proto
AdityaSripal Aug 24, 2020
45d2786
Merge branch 'master' into colin/migrate-tm-evidence
mergify[bot] Aug 24, 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
20 changes: 20 additions & 0 deletions proto/ibc/tendermint/tendermint.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,26 @@ message ConsensusState {
];
}

// Evidence is a wrapper over tendermint's DuplicateVoteEvidence
Copy link
Member

Choose a reason for hiding this comment

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

This is NOT a wrapper over DuplicateVoteEvidence. This is really a reimplementation of ConflictingHeadersEvidence

AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
// that implements Evidence interface expected by ICS-02
message Evidence {
Copy link
Member

Choose a reason for hiding this comment

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

how do you differentiate between different forms of evidence?

Copy link
Contributor Author

@colin-axner colin-axner Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

good question. we currently only have one form of evidence for tendermint. Whenever we add more we can rename this to be more specific, but that is for another pr. Are we planning to support more than DuplicateVoteEvidence for 1.0? @cwgoes

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes; we want to support all of the evidence types which Tendermint supports.

Ideally we should just import the proto files from Tendermint directly, if they exist?

cc @cmwaters

Copy link
Member

Choose a reason for hiding this comment

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

they are here: https://github.com/tendermint/tendermint/blob/master/proto/tendermint/types/evidence.proto.

I would recommend generating them yourself instead of importing them

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, they are located here although they are still undergoing changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opened #7146 . @AdityaSripal do you think you could look into doing this? I'm not sure if this would require added logic in misbehaviour

Copy link
Member

@AdityaSripal AdityaSripal Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@cwgoes I don't see why we would want to support all the evidence types that Tendermint supports in core IBC (we will want to do this later for apps like cross-chain staking). Most are irrelevant for core IBC since they involve individual validators misbehaving.

What is core IBC supposed to do upon receiving Evidence of a single validator misbehaving (suppose a DoubleVote or Amnesia or Lunatic Evidence)?
From my understanding, Nothing. It is incumbent on the chain to punish this validator accordingly upon processing this evidence, but the IBC client should operate as normal as this is entirely within the concerns of the counterparty chain (It is part of the security model assumptions of Tendermint blockchains that individual validators may become malicious without worrying about the lite-client state).

IBC clients only need to do something when the security assumptions of the counterparty chain are violated. I.E. There is a light or full fork of the counterparty chain. This case is already handled in IBC's misbehaviour logic, though we could look into reusing the Evidence type Tendermint uses for this, namely ConflictingHeadersEvidence

Ref further dicussion here: https://github.com/cosmos/cosmos-sdk/issues/7146#issuecomment-679305265

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I was confused by the comment above this message - https://github.com/cosmos/cosmos-sdk/issues/7146#issuecomment-679364939.

option (gogoproto.goproto_getters) = false;
option (gogoproto.goproto_stringer) = false;

string client_id = 1 [(gogoproto.moretags) = "yaml:\"client_id\""];
string chain_id = 2 [(gogoproto.moretags) = "yaml:\"chain_id\""];
Header header_1 = 3 [
(gogoproto.customname) = "Header1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc/ @amaurymartiny . I'm using customname here because otherwise the generated files will output "Header_1". We don't need gRPC calls for evidence because we don't store them in state (we don't even have gRPC calls for this sub-module). Just wanted to check this won't cause issues in relation to #7033 changes. I kept the "Id" naming for other fields in this struct for consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is not ok I can update to "HeaderOne"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine 👍

(gogoproto.nullable) = false,
(gogoproto.moretags) = "yaml:\"header_1\""
];
Header header_2 = 4 [
(gogoproto.customname) = "Header2",
(gogoproto.nullable) = false,
(gogoproto.moretags) = "yaml:\"header_2\""
];
}

// Header defines the Tendermint client consensus Header.
// It encapsulates all the information necessary to update from a trusted
// Tendermint ConsensusState. The inclusion of TrustedHeight and
Expand Down
52 changes: 26 additions & 26 deletions x/ibc/02-client/keeper/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,17 +267,17 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {

testCases := []struct {
name string
evidence ibctmtypes.Evidence
evidence *ibctmtypes.Evidence
malleate func() error
expPass bool
}{
{
"trusting period misbehavior should pass",
ibctmtypes.Evidence{
&ibctmtypes.Evidence{
Header1: ibctmtypes.CreateTestHeader(testChainID, testClientHeight, testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
Header2: ibctmtypes.CreateTestHeader(testChainID, testClientHeight, testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
ChainID: testChainID,
ClientID: testClientID,
ChainId: testChainID,
ClientId: testClientID,
},
func() error {
suite.consensusState.NextValidatorsHash = bothValsHash
Expand All @@ -290,11 +290,11 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
},
{
"misbehavior at later height should pass",
ibctmtypes.Evidence{
&ibctmtypes.Evidence{
Header1: ibctmtypes.CreateTestHeader(testChainID, testClientHeight+5, testClientHeight, altTime, bothValSet, valSet, bothSigners),
Header2: ibctmtypes.CreateTestHeader(testChainID, testClientHeight+5, testClientHeight, suite.ctx.BlockTime(), bothValSet, valSet, bothSigners),
ChainID: testChainID,
ClientID: testClientID,
ChainId: testChainID,
ClientId: testClientID,
},
func() error {
suite.consensusState.NextValidatorsHash = valsHash
Expand All @@ -318,11 +318,11 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
},
{
"misbehavior at later height with different trusted heights should pass",
ibctmtypes.Evidence{
&ibctmtypes.Evidence{
Header1: ibctmtypes.CreateTestHeader(testChainID, testClientHeight+5, testClientHeight, altTime, bothValSet, valSet, bothSigners),
Header2: ibctmtypes.CreateTestHeader(testChainID, testClientHeight+5, testClientHeight+3, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
ChainID: testChainID,
ClientID: testClientID,
ChainId: testChainID,
ClientId: testClientID,
},
func() error {
suite.consensusState.NextValidatorsHash = valsHash
Expand All @@ -346,11 +346,11 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
},
{
"misbehaviour fails validatebasic",
ibctmtypes.Evidence{
&ibctmtypes.Evidence{
Header1: ibctmtypes.CreateTestHeader(testChainID, testClientHeight+1, testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
Header2: ibctmtypes.CreateTestHeader(testChainID, testClientHeight, testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
ChainID: testChainID,
ClientID: testClientID,
ChainId: testChainID,
ClientId: testClientID,
},
func() error {
suite.consensusState.NextValidatorsHash = bothValsHash
Expand All @@ -363,11 +363,11 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
},
{
"trusted ConsensusState1 not found",
ibctmtypes.Evidence{
&ibctmtypes.Evidence{
Header1: ibctmtypes.CreateTestHeader(testChainID, testClientHeight+5, testClientHeight+3, altTime, bothValSet, bothValSet, bothSigners),
Header2: ibctmtypes.CreateTestHeader(testChainID, testClientHeight+5, testClientHeight, suite.ctx.BlockTime(), bothValSet, valSet, bothSigners),
ChainID: testChainID,
ClientID: testClientID,
ChainId: testChainID,
ClientId: testClientID,
},
func() error {
suite.consensusState.NextValidatorsHash = valsHash
Expand All @@ -380,11 +380,11 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
},
{
"trusted ConsensusState2 not found",
ibctmtypes.Evidence{
&ibctmtypes.Evidence{
Header1: ibctmtypes.CreateTestHeader(testChainID, testClientHeight+5, testClientHeight, altTime, bothValSet, valSet, bothSigners),
Header2: ibctmtypes.CreateTestHeader(testChainID, testClientHeight+5, testClientHeight+3, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
ChainID: testChainID,
ClientID: testClientID,
ChainId: testChainID,
ClientId: testClientID,
},
func() error {
suite.consensusState.NextValidatorsHash = valsHash
Expand All @@ -398,17 +398,17 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {

{
"client state not found",
ibctmtypes.Evidence{},
&ibctmtypes.Evidence{},
func() error { return nil },
false,
},
{
"client already frozen at earlier height",
ibctmtypes.Evidence{
&ibctmtypes.Evidence{
Header1: ibctmtypes.CreateTestHeader(testChainID, testClientHeight, testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
Header2: ibctmtypes.CreateTestHeader(testChainID, testClientHeight, testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
ChainID: testChainID,
ClientID: testClientID,
ChainId: testChainID,
ClientId: testClientID,
},
func() error {
suite.consensusState.NextValidatorsHash = bothValsHash
Expand All @@ -425,11 +425,11 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {

{
"misbehaviour check failed",
ibctmtypes.Evidence{
&ibctmtypes.Evidence{
Header1: ibctmtypes.CreateTestHeader(testChainID, testClientHeight, testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
Header2: ibctmtypes.CreateTestHeader(testChainID, testClientHeight, testClientHeight, suite.ctx.BlockTime(), altValSet, bothValSet, altSigners),
ChainID: testChainID,
ClientID: testClientID,
ChainId: testChainID,
ClientId: testClientID,
},
func() error {
clientState := ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs())
Expand Down
5 changes: 5 additions & 0 deletions x/ibc/07-tendermint/types/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
evidenceexported "github.com/cosmos/cosmos-sdk/x/evidence/exported"
clientexported "github.com/cosmos/cosmos-sdk/x/ibc/02-client/exported"
)

Expand All @@ -30,6 +31,10 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
(*clientexported.ConsensusState)(nil),
&ConsensusState{},
)
registry.RegisterImplementations(
(*evidenceexported.Evidence)(nil),
&Evidence{},
)
}

var (
Expand Down
31 changes: 16 additions & 15 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 @@ -23,13 +23,15 @@ var (
_ clientexported.Misbehaviour = Evidence{}
)

// Evidence is a wrapper over tendermint's DuplicateVoteEvidence
// that implements Evidence interface expected by ICS-02
type Evidence struct {
ClientID string `json:"client_id" yaml:"client_id"`
Header1 Header `json:"header1" yaml:"header1"`
Header2 Header `json:"header2" yaml:"header2"`
ChainID string `json:"chain_id" yaml:"chain_id"`
// NewEvidence creates a new Evidence instance.
func NewEvidence(clientID, chainID string, header1, header2 Header) *Evidence {
return &Evidence{
ClientId: clientID,
ChainId: chainID,
Header1: header1,
Header2: header2,
}

}

// ClientType is Tendermint light client
Expand All @@ -39,7 +41,7 @@ func (ev Evidence) ClientType() clientexported.ClientType {

// GetClientID returns the ID of the client that committed a misbehaviour.
func (ev Evidence) GetClientID() string {
return ev.ClientID
return ev.ClientId
}

// Route implements Evidence interface
Expand All @@ -64,8 +66,7 @@ func (ev Evidence) String() string {

// Hash implements Evidence interface
func (ev Evidence) Hash() tmbytes.HexBytes {
// TODO use submodule cdc
bz := amino.MustMarshalBinaryBare(ev)
bz := SubModuleCdc.MustMarshalBinaryBare(&ev)
return tmhash.Sum(bz)
}

Expand Down Expand Up @@ -99,18 +100,18 @@ func (ev Evidence) ValidateBasic() error {
return sdkerrors.Wrap(ErrInvalidValidatorSet, "trusted validator set in Header2 cannot be empty")
}

if err := host.ClientIdentifierValidator(ev.ClientID); err != nil {
if err := host.ClientIdentifierValidator(ev.ClientId); err != nil {
return sdkerrors.Wrap(err, "evidence client ID is invalid")
}

// ValidateBasic on both validators
if err := ev.Header1.ValidateBasic(ev.ChainID); err != nil {
if err := ev.Header1.ValidateBasic(ev.ChainId); err != nil {
return sdkerrors.Wrap(
clienttypes.ErrInvalidEvidence,
sdkerrors.Wrap(err, "header 1 failed validation").Error(),
)
}
if err := ev.Header2.ValidateBasic(ev.ChainID); err != nil {
if err := ev.Header2.ValidateBasic(ev.ChainId); err != nil {
return sdkerrors.Wrap(
clienttypes.ErrInvalidEvidence,
sdkerrors.Wrap(err, "header 2 failed validation").Error(),
Expand All @@ -134,10 +135,10 @@ func (ev Evidence) ValidateBasic() error {
if blockID1.Equals(*blockID2) {
return sdkerrors.Wrap(clienttypes.ErrInvalidEvidence, "headers blockIDs are not equal")
}
if err := ValidCommit(ev.ChainID, ev.Header1.Commit, ev.Header1.ValidatorSet); err != nil {
if err := ValidCommit(ev.ChainId, ev.Header1.Commit, ev.Header1.ValidatorSet); err != nil {
return err
}
if err := ValidCommit(ev.ChainID, ev.Header2.Commit, ev.Header2.ValidatorSet); err != nil {
if err := ValidCommit(ev.ChainId, ev.Header2.Commit, ev.Header2.ValidatorSet); err != nil {
return err
}
return nil
Expand Down
Loading