ICS07 Tendermint Client implementation #5485

fedekunze · 2020-01-06T17:06:10Z

Closes: #5312
Closes: #5476

NOTE: this PR is ready for review, pending tests fixes

Please review against each individual ICS specification.

TODO:

fix x/ibc/03-connection/keeper/handshake_test.go
fix x/ibc/03-connection/keeper/verify_test.go
fix x/ibc/04-channel/keeper/handshake_test.go
add x/ibc/04-channel/keeper/packet_test.go (nice-to-have)
add x/ibc/04-channel/keeper/timeout_test.go (nice-to-have)
~~fix x/ibc/07-tendermint/client_state_test.go (uncomment test cases)~~ Blocked by proof verification

Description

Implements ICS07 within x/ibc/07-tendermint/. Previous tendermint client types (x/ibc/02-client/types/tendermint/) are now in the former package
Moves client errors package (x/ibc/02-client/types/errors/) to client types (x/ibc/02-client/types/errors.go)
Standardize calls to clients using switch statement
Remove unused queriers
Refactor types and implement ChannelI and ConnectionI interfaces to x/ibc/XX-ICS/exported/ to prevent circle depencencies
Remove client MsgSubmitMisbehaviour and replace it for evidence's module MsgSubmitEvidence
Update implementation to latest IBC spec (rc5)
Comment port authentication-related functions until [R4R] ADR 003: Dynamic Capability Store #5397 is implemented

For contributor use:

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.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
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, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

lgtm-com · 2020-01-06T22:24:39Z

This pull request introduces 1 alert when merging 7705818 into 682c8ad - view on LGTM.com

new alerts:

1 for Useless assignment to field

lgtm-com · 2020-01-08T13:30:56Z

This pull request introduces 1 alert when merging 062b748 into 682c8ad - view on LGTM.com

new alerts:

1 for Useless assignment to field

… fedekunze/5476-ics-07-tendermint

cwgoes · 2020-01-14T16:16:03Z

Let's use the Tendermint verify function if possible - https://github.com/tendermint/tendermint/blob/master/lite2/verifier.go#L28

… fedekunze/5476-ics-07-tendermint

jackzampolin · 2020-01-16T18:40:16Z

Update to lite2 verifier blocked on #5527

jackzampolin

Just one quick comment rn

x/ibc/02-client/client/cli/tx.go

cwgoes · 2020-01-20T10:59:05Z

Update to lite2 verifier blocked on #5527

Should be unblocked now?

fedekunze · 2020-01-20T13:17:51Z

x/ibc/02-client/types/msgs.go

@@ -119,53 +120,3 @@ func (msg MsgUpdateClient) GetSignBytes() []byte {
 func (msg MsgUpdateClient) GetSigners() []sdk.AccAddress {
 	return []sdk.AccAddress{msg.Signer}
 }
-
-// MsgSubmitMisbehaviour defines a message to update an IBC client
-type MsgSubmitMisbehaviour struct {


replaced by evicence module's MsgSubmitEvidence

x/ibc/02-client/client/cli/query.go

x/ibc/02-client/client/cli/tx.go

jackzampolin · 2020-01-22T18:29:37Z

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

+
+	// State verification functions
+
+	VerifyClientConsensusState(


Are these verification functions the ones needed to complete the handshakes? Might be helpful to annotate the methods with the different places they are anticipated to be used.

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

jackzampolin · 2020-01-22T18:34:43Z

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

 	"github.com/cosmos/cosmos-sdk/x/ibc/02-client/exported"
 	"github.com/cosmos/cosmos-sdk/x/ibc/02-client/types"
-	"github.com/cosmos/cosmos-sdk/x/ibc/02-client/types/errors"
-	"github.com/cosmos/cosmos-sdk/x/ibc/02-client/types/tendermint"
+	tendermint "github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint"
 )

 // CreateClient creates a new client state and populates it with a given consensus
 // state as defined in https://github.com/cosmos/ics/tree/master/spec/ics-002-client-semantics#create
 func (k Keeper) CreateClient(


I think this function needs some comments in it. It is quite long...

x/ibc/03-connection/keeper/handshake.go

jackzampolin

This PR is pretty massive. Added a few minor comments.

x/ibc/03-connection/keeper/handshake_test.go

x/ibc/03-connection/types/keys.go

x/ibc/04-channel/keeper/handshake.go

x/ibc/04-channel/keeper/timeout.go

jackzampolin · 2020-01-22T19:22:24Z

x/ibc/04-channel/types/expected_keepers.go

-		ctx sdk.Context, connection connection.ConnectionEnd, height uint64,
-		proof commitment.ProofI, path string,
-	) bool
+	GetConnection(ctx sdk.Context, connectionID string) (connectiontypes.ConnectionEnd, bool)


I feel like more comments would be very helpful here.

* Update ICS 7 tests * Fix one problem * Also set consensus state for height 1 * Apply same fixes to ICS 4 tests * Remove unnecessary change * Fix ante tests; remove printfs * Remove printf * Update x/ibc/ante/ante_test.go Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

* Add skeleton for ValidateBasic tests * Move tests; basic one passes * Add more ValidateBasic tests * Add more ValidateBasic testcases * Fix minor typo * Add `ValidateBasic` tests for Packet * Move to packet_test.go

…/cosmos/cosmos-sdk into fedekunze/5476-ics-07-tendermint

AdityaSripal

I believe with the removal of the Committer type we lost some correctness properties. Think there are some issues to be resolved here that may need to be reflected up to the spec.

Still need a way to retrieve the validatorSet that approved updates to client state. This can be done by persisting in the IBC store (perhaps in ConsensusState) or by passing it in as part of the message (like with fromValidatorSet) and verifying against the hash saved in ConsensusState
Bisection algorithm needs to be enforced for the update functions. This requires access to lastValidatorSet
Misbehavior may not be on a height that was explicitly passed in as an update. This can be accounted for by loading last consensus state saved before misbehavior
I believe there needs to be a way to submit misbehavior to a frozen connection if it lowers the frozen height. Suppose height is frozen at 15, but there's evidence of misbehavior at 10. User should still be allowed to submit this evidence to lower the frozen height even further.
ClientState should have an UnbondingPeriod field so that chains have some notion of what the unbonding period is for the connecting chain.

This can then be used to prevent updates that fastforward height beyond last_height + unbonding_period even if the header can be verified by the last ValidatorSet
It can also be used to prevent misbehavior from being submitted at height below last_height - unbonding_period since the keys of validator set that early may be compromised and can be used to create fake misbehavior and allow malicious user to freeze a connection. Ex: Chain is at block 1000000, validator set keys at block 1 are no longer used and compromised. User creates fake commit at height 1 with compromised keys and freezes chain all the way back at height 1.

I can work on implementing these changes once i get agreement that the problems i raised are real and consensus on the solutions i propose

AdityaSripal · 2020-01-23T21:00:08Z

x/ibc/07-tendermint/update.go

+	if err := header.ValidateBasic(chainID); err != nil {
+		return err
+	}
+


Seems like with removing the Committer interface an important check was removed.

I believe we still need to make sure that this Header will be verified by the last update's ValidatorSet.

To do this, we need a way to retrieve the last ValidatorSet that caused an update on this connection and then call

lastValidatorSet.VerifyFutureCommit(header.ChainID, header.Commit.BlockID, header.Height, header.Commit)

Not sure how to do this with the current datastructures now that Committer is removed. If the full ValidatorSet gets saved in ConsensusState rather than just the hash then this is possible

The code as is seems like it will allow arbitrary updates to a connection from an arbitrary validatorset.

Need to implement the bisection algorithm for correct and safe updating

If the full ValidatorSet gets saved in ConsensusState rather than just the hash then this is possible

@cwgoes can we update the spec to include the validator set instead of the hash?

Sure. cosmos/ibc#366. I'll work on this now.

AdityaSripal · 2020-01-23T21:13:53Z

x/ibc/07-tendermint/misbehaviour.go

+	clientState clientexported.ClientState,
+	consensusState clientexported.ConsensusState,
+	misbehaviour clientexported.Misbehaviour,
+	height uint64, // height at which the consensus state was loaded


Doesn't look like it is:
https://github.com/cosmos/cosmos-sdk/pull/5485/files#diff-be64020aa203f351d8bd639c630aa097R124

Since the height passed in is the height of the misbehaviour not the height of the consensusState. While this PR makes them the same height, i believe they may not be in general

AdityaSripal · 2020-01-23T21:26:45Z

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

 	}

-	committer, found := k.GetCommitter(ctx, misbehaviour.ClientID, uint64(misbehaviour.GetHeight()))
+	consensusState, found := k.GetConsensusState(ctx, misbehaviour.GetClientID(), uint64(misbehaviour.GetHeight()))


The consensusState may not exist for the height where the misbehavior is found.

For example, if there were client state updates at height 10 and height 20, then there will be consensus states in the store at height 10 and 20

However, a misbehavior may have first happened at height 15 forwhich there is no consensus state stored. In this case, the fromValidatorSet should be the validatorSet from height 10.

And thus the ConsensusState passed in should also be from height 10

ok, so change it tomin(clientState.GetLatestHeight(), misbehaviour.GetHeight()) = min(20, 15) but that won't work since it'll be still 15... there won't be a consensus state stored at that height.

We'd need to iterate over the keys to get the latest height at which the consensus state was stored that satisfies height <= min(clientState.GetLatestHeight(), misbehaviour.GetHeight()) and min(clientState.GetLatestHeight(), misbehaviour.GetHeight()) - height <= trusting period

Do you mean for the case where there was misbehaviour at height 15 but we are already at height 20? If so, there should be an intermediary set of two conflicting headers, and if we have a consensus state before height 15 stored the evidence should be valid (since both will be valid updates to it). I don't think this requires changes (at least to the spec). This line needs to be changed though.

We'd need to iterate over the keys to get the latest height at which the consensus state

Yes this is true. This is what the old GetConsensusState function did under the hood. You could reimplement this (either in GetConsensusState or in another helper function)

Or you could use the iavlStore.ReverseIterator and just grab the first element. This should be faster, but i couldn't find how to get this working on my first try hence issue #5365

if we have a consensus state before height 15 stored the evidence should be valid

Yup the evidence will still be valid because the update at 20 must have been valid. However, we still need to iterate through the store to find this past consensus state

Yup the evidence will still be valid because the update at 20 must have been valid. However, we still need to iterate through the store to find this past consensus state

Why do we need to iterate? We know what the old height was (from the Evidence).

cwgoes · 2020-01-24T07:12:23Z

Still need a way to retrieve the validatorSet that approved updates to client state. This can be done by persisting in the IBC store (perhaps in ConsensusState) or by passing it in as part of the message (like with fromValidatorSet) and verifying against the hash saved in ConsensusState

Yes, I suggest the latter approach since it saves state space, but we could also store & prune the validator sets, that would be fine as well.

Bisection algorithm needs to be enforced for the update functions. This requires access to lastValidatorSet

This should be imported from Tendermint with the Tendermint lite client "Verify" function. The lastValidatorSet can be checked against the known hash, or we can store it.

Misbehavior may not be on a height that was explicitly passed in as an update. This can be accounted for by loading last consensus state saved before misbehavior

Misbehaviour should be able to load a particular past consensus state, as per the spec. Looking at your comment above, this is not quite what the implementation does - it should be.

I believe there needs to be a way to submit misbehavior to a frozen connection if it lowers the frozen height. Suppose height is frozen at 15, but there's evidence of misbehavior at 10. User should still be allowed to submit this evidence to lower the frozen height even further.

I agree (see above comment about "min" for frozen height).

ClientState should have an UnbondingPeriod field so that chains have some notion of what the unbonding period is for the connecting chain.

That's probably the right place to put it. Any Tendermint lite client parameters also need to be in the ClientState, I suppose. The spec should be updated for this - cosmos/ibc#365.

This can then be used to prevent updates that fastforward height beyond last_height + unbonding_period even if the header can be verified by the last ValidatorSet

This should not be necessary to separately check if the Tendermint light client's Verify function is used, as Verify (with appropriate parameters) performs bisection.

It can also be used to prevent misbehavior from being submitted at height below last_height - unbonding_period since the keys of validator set that early may be compromised and can be used to create fake misbehavior and allow malicious user to freeze a connection. Ex: Chain is at block 1000000, validator set keys at block 1 are no longer used and compromised. User creates fake commit at height 1 with compromised keys and freezes chain all the way back at height 1.

Ah yes, I agree with this, it should also be changed in the spec - cosmos/ibc#364.

* ICS 07 Debugging * WIP Debugging * Foo bar baz, baz bar foo, 🤷‍♂️ * cleanup print statements * cleanup verification test * add return err for proof verification * cleanup; start handshake testing * connection handshake tests * scopelint * WIP Test refactor * fix ICS03 tests * fix ICS04 tests * nolint Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

x/ibc/03-connection/keeper/keeper_test.go

addressed

x/ibc/07-tendermint/consensus_state_test.go

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

x/ibc/23-commitment/merkle_test.go

golangcibot · 2020-01-27T15:36:54Z

x/ibc/03-connection/keeper/handshake_test.go

+		suite.Run(fmt.Sprintf("Case %s", tc.msg), func() {
+			suite.SetupTest() // reset
+
+			tc.malleate()


Using the variable on range scope tc in function literal (from scopelint)

x/ibc/03-connection/keeper/handshake_test.go

Will address in a follow up PR

melekes · 2020-01-28T06:33:20Z

x/ibc/03-connection/keeper/handshake.go

+	// }
+
+	previousConnection, found := k.GetConnection(ctx, connectionID)
+	if found && !(previousConnection.State == exported.INIT &&


maybe a good idea to extract this into a function previousConnection.????(exported, counterparty, version)

melekes · 2020-01-28T06:40:50Z

x/ibc/07-tendermint/header.go

+	}
+	if h.ValidatorSet == nil {
+		return sdkerrors.Wrap(clienttypes.ErrInvalidHeader, "validator set is nil")
+	}


don't you wanna check SignedHeader.ValidatorsHash == ValidatorSet.Hash() here?

I'm surprised that the check is not already done on the SignedHeader.ValidateBasic() @marbar3778 @tessr

implement ICS07

a32d4b9

fedekunze added x/ibc labels Jan 6, 2020

fedekunze added 2 commits January 6, 2020 15:38

build

c2e66b8

update tests and cleanup

7705818

x/ibc/02-client/types: remove misbehaviour in favor of evidence

6404caa

cwgoes mentioned this pull request Jan 7, 2020

The one true master IBC implementation tracking issue cosmos/gaia#201

Closed

34 tasks

remove root query, update queriers, implement verification funcs

062b748

remove committer; cleanup

2126cc8

fedekunze mentioned this pull request Jan 10, 2020

Cleanup ICS 2 implementation #5510

Closed

fedekunze added 3 commits January 14, 2020 14:38

move keys to ibc/types

1b17735

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

4a53fc1

… fedekunze/5476-ics-07-tendermint

fix paths

9c15f5d

fedekunze added 7 commits January 15, 2020 11:57

update ICS03 connection verification

9a4a93d

move order and states to exported pkg

4db2bdf

update ICS04 to latest spec

3fc142f

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

592a1ff

… fedekunze/5476-ics-07-tendermint

fix build

32160b9

move ics02 types/errors package to /types

944fec8

update a few tests

3d48b9b

jackzampolin reviewed Jan 16, 2020

View reviewed changes

x/ibc/02-client/client/cli/tx.go Show resolved Hide resolved

update tests; fix codec registration

c1de08b

merge ibc-alpha

bbd9826

fedekunze commented Jan 20, 2020

View reviewed changes

minor changes from code review

959a4f5

fedekunze added 3 commits January 21, 2020 19:18

x/ibc/07-tendermint: add tests for consensus state, header and update

c9fadfa

ibc/07-tendermint: fix verification panic and add tests

8e18839

ibc/07-tendermint: add failure test cases

32189d1

jackzampolin reviewed Jan 22, 2020

View reviewed changes

jackzampolin approved these changes Jan 22, 2020

View reviewed changes

fedekunze and others added 8 commits January 23, 2020 12:39

x/ibc/03-connection/keeper: add verification tests for failing cases

9a06451

remove unused queriers

c889f3d

add TODOs for ADR 03 dynamic store

d8e237d

add tests for msgs and packet (#5559)

2256db7

* Add skeleton for ValidateBasic tests * Move tests; basic one passes * Add more ValidateBasic tests * Add more ValidateBasic testcases * Fix minor typo * Add `ValidateBasic` tests for Packet * Move to packet_test.go

use app.Commit for tests

5a3f3fc

Merge branch 'fedekunze/5476-ics-07-tendermint' of https://github.com…

aa5c3d9

…/cosmos/cosmos-sdk into fedekunze/5476-ics-07-tendermint

update verify.go

0eccd5e

AdityaSripal previously requested changes Jan 23, 2020

View reviewed changes

cwgoes mentioned this pull request Jan 24, 2020

Store full validator set instead of validator set hash in ICS 7 cosmos/ibc#366

Closed

golangcibot reviewed Jan 27, 2020

View reviewed changes

x/ibc/03-connection/keeper/keeper_test.go Show resolved Hide resolved

cleanup comments and add a few tests

0f1b1d7

golangcibot reviewed Jan 27, 2020

View reviewed changes

typo

ddf6a25

fedekunze merged commit 3df016e into ibc-alpha Jan 27, 2020

fedekunze deleted the fedekunze/5476-ics-07-tendermint branch January 27, 2020 17:00

This was referenced Jan 27, 2020

Update IBC to rc5 #5312

Closed

Implement ICS 007: Tendermint Client #5476

Closed

melekes reviewed Jan 28, 2020

View reviewed changes

fedekunze mentioned this pull request Feb 3, 2020

ICS07 follow up changes #5606

Merged

11 tasks

AdityaSripal mentioned this pull request Feb 10, 2020

ICS07 follow ups #5631

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICS07 Tendermint Client implementation #5485

ICS07 Tendermint Client implementation #5485

fedekunze commented Jan 6, 2020 •

edited

Loading

lgtm-com bot commented Jan 6, 2020

lgtm-com bot commented Jan 8, 2020

cwgoes commented Jan 14, 2020

jackzampolin commented Jan 16, 2020

jackzampolin left a comment

cwgoes commented Jan 20, 2020

fedekunze Jan 20, 2020

jackzampolin Jan 22, 2020

jackzampolin Jan 22, 2020

jackzampolin left a comment

jackzampolin Jan 22, 2020

AdityaSripal left a comment •

edited

Loading

AdityaSripal Jan 23, 2020

fedekunze Jan 24, 2020

cwgoes Jan 24, 2020

AdityaSripal Jan 23, 2020

AdityaSripal Jan 23, 2020

fedekunze Jan 24, 2020

cwgoes Jan 24, 2020 •

edited

Loading

AdityaSripal Jan 24, 2020 •

edited

Loading

cwgoes Jan 24, 2020

cwgoes commented Jan 24, 2020 •

edited

Loading

golangcibot Jan 27, 2020

melekes Jan 28, 2020

melekes Jan 28, 2020

fedekunze Jan 29, 2020

ICS07 Tendermint Client implementation #5485

ICS07 Tendermint Client implementation #5485

Conversation

fedekunze commented Jan 6, 2020 • edited Loading

Description

lgtm-com bot commented Jan 6, 2020

lgtm-com bot commented Jan 8, 2020

cwgoes commented Jan 14, 2020

jackzampolin commented Jan 16, 2020

jackzampolin left a comment

Choose a reason for hiding this comment

cwgoes commented Jan 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackzampolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment • edited Loading

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 Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

AdityaSripal Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Jan 24, 2020 • edited Loading

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

fedekunze commented Jan 6, 2020 •

edited

Loading

AdityaSripal left a comment •

edited

Loading

cwgoes Jan 24, 2020 •

edited

Loading

AdityaSripal Jan 24, 2020 •

edited

Loading

cwgoes commented Jan 24, 2020 •

edited

Loading