IBC client - evidence module route #5305

fedekunze · 2019-11-12T17:27:04Z

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

… fedekuze/ibc-evidence

fedekunze · 2019-11-13T11:20:45Z

@cwgoes do we want to check every n blocks if there's existent client misbehavior stored in order to freeze the client without submitting a new transaction (i.e having a BeginBlock logic for that)?

cwgoes · 2019-11-13T12:38:38Z

@cwgoes do we want to check every n blocks if there's existent client misbehavior stored in order to freeze the client without submitting a new transaction (i.e having a BeginBlock logic for that)?

I'm a bit lost. If we ever detect misbehaviour (which requires submitting a transaction) we should freeze the client immediately. What are the two separate transactions involved in your scenario?

AdityaSripal · 2019-11-14T00:35:02Z

x/ibc/02-client/types/tendermint/misbehaviour.go

-	TotalPower     int64  `json:"total_power" yaml:"total_power"`
+// Misbehaviour contains an evidence that a
+type Misbehaviour struct {
+	*Evidence


Why does Misbehaviour contain ChainID when Evidence already has ChainID field?

Misbehaviour contains only the ClientID

AdityaSripal · 2019-11-14T00:37:20Z

x/ibc/02-client/types/tendermint/misbehaviour.go

-	}
-	if ev.ValidatorPower <= 0 {
-		return sdk.ConvertError(errors.ErrInvalidEvidence(errors.DefaultCodespace, fmt.Sprintf("Invalid Validator Power: %d", ev.ValidatorPower)))
+	if err := m.Evidence.ValidateBasic(); err != nil {


needs to also check m.ChainID == m.Evidence.ChainID

see comment above

x/ibc/02-client/types/tendermint/evidence_test.go

AdityaSripal · 2019-11-14T03:18:19Z

x/ibc/02-client/keeper/client.go

+//
+// NOTE: In the first implementation, only Tendermint misbehaviour evidence is
+// supported.
+func (k Keeper) CheckMisbehaviourAndUpdateState(ctx sdk.Context, evidence evidenceexported.Evidence) error {


This doesn't make sense to me. My understanding is that the client should be frozen if the entire chain breaks the consensus protocol. For example:

ChainA is running light client of ChainB

ChainB validator set commits 2 different blocks for the same height (violation of consensus protocol)

Evidence of this violation is submitted to ChainA

ChainA freezes light client of ChainB

The code here looks like it will freeze the client as soon as someone posts evidence of a single validator double signing. This doesn't make sense since it won't disrupt consensus, so long as validator power < 1/3 of total.

Thus, with code as is, a single slash event of a double-signing validator on chainB (which is still considered honest execution of Byzantine consensus protocol) will cause IBC client to freeze. Thus, a chainA no longer has any Byzantine tolerance for chainB; which doesn't seem like the intended behavior from looking at spec

@cwgoes

I believe the Evidence should be changed from embedding a single tendermint.DoubleVoteEvidence to including two different block headers that got committed by ChainB's validatorSet but are for same height

Haha, i think i was the one to commit this mistake to the code in the first place 😆. Will put up PR for proposed fix, once i get an ACK that the problem i point out is correct

https://github.com/cosmos/cosmos-sdk/blob/ibc-alpha/x/ibc/02-client/types/tendermint/misbehaviour.go#L22

Further thinking here after prototyping solution:

The Evidence interface defined in x/evidence isn't well suited to use here, since that is Evidence against a single malicious validator. Whereas I believe the Evidence that should freeze a ICS-02 client is evidence of a malicious chain. We should have some other primitive defined in x/ibc to deal with malicious chains.

The overloading of the term Evidence to describe evidence against validators and evidence against chains is confusing. I think we should have a different word to talk about malicious chains and the associated proofs, (Violation maybe)?

Proposals:

A) evidence module is adjusted to handle both malicious-validator evidence and malicious-chain evidence

B) malicious-chain evidence types and functions are implemented within x/ibc since that is the only place they will be used.

I'm partial to proposal B myself and removing any dependence of x/evidence from x/ibc for now, but interested to hear what others think. Note: validator-specific evidence isn't relevant in IBC for version 1.0.0, but will later be useful once we implement shared-security across chains.

I believe the Evidence should be changed from embedding a single tendermint.DoubleVoteEvidence to including two different block headers that got committed by ChainB's validatorSet but are for same height

yeah, that's how it is defined on the spec:

interface Evidence { h1: Header h2: Header }

The Evidence interface defined in x/evidence isn't well suited to use here, since that is Evidence against a single malicious validator. Whereas I believe the Evidence that should freeze a ICS-02 client is evidence of a malicious chain. We should have some other primitive defined in x/ibc to deal with malicious chains.

A) evidence module is adjusted to handle both malicious-validator evidence and malicious-chain evidence

I think we should implement a general Evidence interface that only contains the following methods:

// Evidence defines the contract which concrete evidence types of misbehavior // must implement. type Evidence interface { Route() string Type() string String() string Hash() cmn.HexBytes ValidateBasic() error }

Probably the byzantine validator related methods from the current Evidence interface should be abstracted into another interface:

// The consensus address of the malicious validator at time of infraction GetConsensusAddress() sdk.ConsAddress // Height at which the infraction occurred GetHeight() int64 // The total power of the malicious validator at time of infraction GetValidatorPower() int64 // The total validator set power at time of infraction GetTotalPower() int64

And ICS02 should implement the Evidence interface from the spec (2 headers) as well as the general interface

The overloading of the term Evidence to describe evidence against validators and evidence against chains is confusing. I think we should have a different word to talk about malicious chains and the associated proofs, (Violation maybe)?

Agree, ICS02 introduces the term of a client Misbehaviour, which is what I implemented in this PR

The Evidence interface defined in x/evidence isn't well suited to use here, since that is Evidence against a single malicious validator. Whereas I believe the Evidence that should freeze a ICS-02 client is evidence of a malicious chain. We should have some other primitive defined in x/ibc to deal with malicious chains.

A) evidence module is adjusted to handle both malicious-validator evidence and malicious-chain evidence

I think we should implement a general Evidence interface that only contains the following methods:

// Evidence defines the contract which concrete evidence types of misbehavior // must implement. type Evidence interface { Route() string Type() string String() string Hash() cmn.HexBytes ValidateBasic() error }

Probably the byzantine validator related methods from the current Evidence interface should be abstracted into another interface:

// The consensus address of the malicious validator at time of infraction GetConsensusAddress() sdk.ConsAddress // Height at which the infraction occurred GetHeight() int64 // The total power of the malicious validator at time of infraction GetValidatorPower() int64 // The total validator set power at time of infraction GetTotalPower() int64

And ICS02 should implement the Evidence interface from the spec (2 headers) as well as the general interface

This approach seems OK to me. Conceivably other modules might want to handle such misbehaviour as well (e.g. if someone had purchased insurance that pays out if a validator set breaks safety).

Conceivably other modules might want to handle such misbehaviour as well (e.g. if someone had purchased insurance that pays out if a validator set breaks safety).

afaik we haven't implemented slashing logic for this type of misbehaviour (if that's what you're referring with "handle such misbehaviour")

cwgoes

See responses to @AdityaSripal's comments

cwgoes · 2019-11-14T10:33:49Z

x/ibc/02-client/keeper/client.go

+//
+// NOTE: In the first implementation, only Tendermint misbehaviour evidence is
+// supported.
+func (k Keeper) CheckMisbehaviourAndUpdateState(ctx sdk.Context, evidence evidenceexported.Evidence) error {


This doesn't make sense to me. My understanding is that the client should be frozen if the entire chain breaks the consensus protocol. For example:

ChainA is running light client of ChainB

ChainB validator set commits 2 different blocks for the same height (violation of consensus protocol)

Evidence of this violation is submitted to ChainA

ChainA freezes light client of ChainB

The code here looks like it will freeze the client as soon as someone posts evidence of a single validator double signing. This doesn't make sense since it won't disrupt consensus, so long as validator power < 1/3 of total.

Thus, with code as is, a single slash event of a double-signing validator on chainB (which is still considered honest execution of Byzantine consensus protocol) will cause IBC client to freeze. Thus, a chainA no longer has any Byzantine tolerance for chainB; which doesn't seem like the intended behavior from looking at spec

@cwgoes

Yes, this is right. Although it is also possible to choose thresholds other than 1/3 (perhaps gaining some extra precautionary safety at the expense of liveness), we should initially freeze the client if and only if two valid headers were committed at the same height.

cwgoes · 2019-11-14T10:36:06Z

x/ibc/02-client/keeper/client.go

+//
+// NOTE: In the first implementation, only Tendermint misbehaviour evidence is
+// supported.
+func (k Keeper) CheckMisbehaviourAndUpdateState(ctx sdk.Context, evidence evidenceexported.Evidence) error {


Further thinking here after prototyping solution:

The Evidence interface defined in x/evidence isn't well suited to use here, since that is Evidence against a single malicious validator. Whereas I believe the Evidence that should freeze a ICS-02 client is evidence of a malicious chain. We should have some other primitive defined in x/ibc to deal with malicious chains.

The overloading of the term Evidence to describe evidence against validators and evidence against chains is confusing. I think we should have a different word to talk about malicious chains and the associated proofs, (Violation maybe)?

Proposals:

A) evidence module is adjusted to handle both malicious-validator evidence and malicious-chain evidence

B) malicious-chain evidence types and functions are implemented within x/ibc since that is the only place they will be used.

I'm partial to proposal B myself and removing any dependence of x/evidence from x/ibc for now, but interested to hear what others think. Note: validator-specific evidence isn't relevant in IBC for version 1.0.0, but will later be useful once we implement shared-security across chains.

A new term might be a helpful differentiator. Malicious-chain evidence (or violation) types will be used by both the IBC on-chain light clients and Tendermint light clients (e.g. if you run gaiacli in proper verification mode), so we should find a way to share the definitions between those two, ideally.

Co-Authored-By: Aditya <adityasripal@gmail.com>

fedekunze · 2019-11-14T15:17:15Z

Will put up PR for proposed fix, once i get an ACK that the problem i point out is correct

@AdityaSripal should we block this PR until your fix is merged to ibc-alpha? cc: @cwgoes

AdityaSripal · 2019-11-15T07:29:53Z

ill make the PR against this branch since it already incorporates the interface from x/evidence.

Don't think its a huge deal if this gets merged first. but would be slightly more convenient to block

… fedekuze/ibc-evidence

…mos-sdk into fedekuze/ibc-evidence

fedekunze · 2019-11-22T12:19:48Z

moved to #5321

ibc client evidence route

dbde0d1

fedekunze added x/ibc C:x/evidence labels Nov 12, 2019

fedekunze self-assigned this Nov 12, 2019

fedekunze changed the base branch from master to ibc-alpha November 12, 2019 17:27

fedekunze added 5 commits November 13, 2019 10:22

Merge branch 'ibc-alpha' of https://github.com/cosmos/cosmos-sdk into…

4b979b7

… fedekuze/ibc-evidence

split evidence from misbehaviour

a7e0588

clean up client events

33ae02e

test misbehaviour and evidence

94790bf

remove comments

f7db0fe

fedekunze added R4R and removed WIP labels Nov 13, 2019

fedekunze marked this pull request as ready for review November 13, 2019 11:09

fedekunze requested review from alessio, alexanderbez, jackzampolin and rigelrozanski as code owners November 13, 2019 11:09

fedekunze requested review from cwgoes and AdityaSripal November 13, 2019 11:09

cwgoes mentioned this pull request Nov 13, 2019

Post-MVP pre-launch IBC tasks #5199

Closed

7 tasks

remove frozen comments from demo

e32f304

AdityaSripal requested changes Nov 14, 2019

View reviewed changes

cwgoes suggested changes Nov 14, 2019

View reviewed changes

Update x/ibc/02-client/types/tendermint/evidence_test.go

0c5ee0a

Co-Authored-By: Aditya <adityasripal@gmail.com>

fedekunze added 2 commits November 20, 2019 22:29

Merge branch 'ibc-alpha' of https://github.com/cosmos/cosmos-sdk into…

6f7b571

… fedekuze/ibc-evidence

Merge branch 'fedekuze/ibc-evidence' of https://github.com/cosmos/cos…

d0c8232

…mos-sdk into fedekuze/ibc-evidence

alexanderbez mentioned this pull request Nov 21, 2019

IBC Implementation & Integration #5125

Closed

30 tasks

fedekunze mentioned this pull request Nov 22, 2019

ICS-2 Implement Misbehavior #5321

Merged

5 tasks

fedekunze closed this Nov 22, 2019

fedekunze deleted the fedekuze/ibc-evidence branch November 22, 2019 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBC client - evidence module route #5305

IBC client - evidence module route #5305

fedekunze commented Nov 12, 2019 •

edited

Loading

fedekunze commented Nov 13, 2019

cwgoes commented Nov 13, 2019

AdityaSripal Nov 14, 2019

fedekunze Nov 14, 2019

AdityaSripal Nov 14, 2019

fedekunze Nov 14, 2019

AdityaSripal Nov 14, 2019

AdityaSripal Nov 14, 2019

AdityaSripal Nov 14, 2019

AdityaSripal Nov 14, 2019

AdityaSripal Nov 14, 2019

fedekunze Nov 14, 2019

fedekunze Nov 14, 2019

fedekunze Nov 14, 2019

cwgoes Nov 14, 2019

fedekunze Nov 14, 2019

cwgoes left a comment

cwgoes Nov 14, 2019

cwgoes Nov 14, 2019

fedekunze commented Nov 14, 2019

AdityaSripal commented Nov 15, 2019 •

edited

Loading

fedekunze commented Nov 22, 2019

IBC client - evidence module route #5305

IBC client - evidence module route #5305

Conversation

fedekunze commented Nov 12, 2019 • edited Loading

fedekunze commented Nov 13, 2019

cwgoes commented Nov 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze commented Nov 14, 2019

AdityaSripal commented Nov 15, 2019 • edited Loading

fedekunze commented Nov 22, 2019

fedekunze commented Nov 12, 2019 •

edited

Loading

AdityaSripal commented Nov 15, 2019 •

edited

Loading