Verify Client on Connection Handshake #7057

AdityaSripal · 2020-08-14T22:55:51Z

Description

closes: #6524

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

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
Review Codecov Report in the comment section below once CI passes

codecov · 2020-08-14T23:01:16Z

Codecov Report

Merging #7057 into master will decrease coverage by 1.15%.
The diff coverage is 52.66%.

@@            Coverage Diff             @@
##           master    #7057      +/-   ##
==========================================
- Coverage   55.60%   54.44%   -1.16%     
==========================================
  Files         457      547      +90     
  Lines       27440    37659   +10219     
==========================================
+ Hits        15257    20503    +5246     
- Misses      11083    15469    +4386     
- Partials     1100     1687     +587

…ify-client

AdityaSripal · 2020-08-17T23:35:10Z

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

+			expectedUbdPeriod, tmClient.UnbondingPeriod)
+	}
+
+	if tmClient.UnbondingPeriod < tmClient.TrustingPeriod {


This is currently the only check I enforce on TrustingPeriod, should I add others? @cwgoes

I don't think so; although some values (almost unbonding period and very very low) are practically unworkable, that should be paid attention to by users or relayers when creating a client

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

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

x/ibc/03-connection/client/cli/tx.go

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

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

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

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

colin-axner

very quick pass, please update spec/docs as well

x/ibc/03-connection/client/utils/utils.go

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

x/ibc/03-connection/handler.go

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

Co-authored-by: Federico Kunze <[email protected]>

AdityaSripal · 2020-08-18T21:54:57Z

please update spec/docs as well

Do you mean in the ics repo @colin-axner ? I believe I've updated the docs of function in code, but I've missed any or they're insufficient please point them out and ill fix them

colin-axner

overall LGTM, left a lot of suggestions

proto/ibc/connection/connection.proto

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

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

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

colin-axner · 2020-08-19T11:23:19Z

Do you mean in the ics repo @colin-axner ? I believe I've updated the docs of function in code, but I've missed any or they're insufficient please point them out and ill fix them

No not ICS specs, x/ibc/spec, specifically 04-messages.md and it'd be lovely if you could add a section in 01-concepts.md on self client validation/consensus state (related to TM). We should probably begin adding sections on the Verify funcs but maybe we can add a wishlist of concept sections to an issue. I think docs/ibc/overview.md also needs to be updated on the handshake section

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

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

x/ibc/07-tendermint/types/client_state.go

fedekunze · 2020-08-19T14:53:28Z

x/ibc/09-localhost/types/client_state.go

+// VerifyClientConsensusState returns nil since a local host client does not store consensus
 // states.


this is actually not intuitive to me and is also not documented on the spec if it should return an error or not. cc @cwgoes

If this returns an error, then you can never open a connection with localhost client because the VerifyClientConsensusState will fail.

ditto on adding to localhost spec dir

It should not return an error; it should just pass.

fedekunze · 2020-08-19T14:54:42Z

x/ibc/09-localhost/types/client_state.go

 	_ []byte,
 	connectionID string,
 	connectionEnd connectionexported.ConnectionI,
 ) error {
-	path, err := commitmenttypes.ApplyPrefix(prefix, host.ConnectionPath(connectionID))


I think this requires a spec change too. @cwgoes

if not, document why the implementation differs from spec

We do not need a prefix because we are already being passed in the clientID prefixed store into our function.

So we shouldn't be prefixing at all with ibc. We really only need that prefix in the specific clients when we are construcing the proof path. It's not needed to get from the clientStore.

this is good info for when we add a spec dir for locahost since this differs from the ICS spec in implementation but not semantics

Aye, excellent use-case for a spec directory.

Co-authored-by: colin axnér <[email protected]> Co-authored-by: Federico Kunze <[email protected]>

…ify-client

…nto aditya/verify-client

AdityaSripal · 2020-08-19T20:43:59Z

@colin-axner updated the spec for messages. The docs/ibc/overview.md is a very cursory explanation of connections so it doesn't dive into the abstraction level where explaining the details of the handshake is necessary.

Although I do believe an in-depth explanation of the handshakes (which will explain proving self consensus states and clients for the connection handshake) in IBC is sorely needed so I will add it to an IBC docs wishlist issue to be tackled in separate PR.

Added that item to this issue: #6134 (comment)

fedekunze

ACK, pending minor test fixes, which can be incorporated in a follow-up if you want

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

fedekunze · 2020-08-20T07:15:47Z

x/ibc/09-localhost/types/client_state.go

@@ -91,13 +91,36 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState(
 	return nil, sdkerrors.Wrap(clienttypes.ErrInvalidEvidence, "cannot submit misbehaviour to localhost client")
 }

-// VerifyClientConsensusState returns an error since a local host client does not store consensus
+// VerifyClientState verifies that the localhost client state is stored locally
+func (cs ClientState) VerifyClientState(


pending test

fedekunze · 2020-08-20T07:17:49Z

x/ibc/09-localhost/types/client_state.go

 	if err != nil {
 		return err
 	}

-	if connectionEnd != &prevConnection {
+	if !reflect.DeepEqual(&prevConnection, connectionEnd) {


nit, this line is not covered by tests

fedekunze · 2020-08-20T07:17:56Z

x/ibc/09-localhost/types/client_state.go

 	if err != nil {
 		return err
 	}

-	if channel != &prevChannel {
+	if !reflect.DeepEqual(&prevChannel, channel) {


colin-axner · 2020-08-20T08:54:32Z

I opened an issue to begin tracking things that should be noted in the client specs #7119 . Feel free to add to the comment anything in particular that is important to note or is non-trivial to understand

colin-axner

LGTM, great work! agree with @fedekunze pending test request

cwgoes

ACK but see comments on ValidateSelfClient. I wonder if there is a way to write this function such that if we add a field to the client state that is unequal, it fails by default instead of passing by default.

cwgoes · 2020-08-20T10:37:13Z

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

+			expectedUbdPeriod, tmClient.UnbondingPeriod)
+	}
+
+	if tmClient.UnbondingPeriod < tmClient.TrustingPeriod {


I don't think so; although some values (almost unbonding period and very very low) are practically unworkable, that should be paid attention to by users or relayers when creating a client

cwgoes · 2020-08-20T10:47:48Z

x/ibc/09-localhost/types/client_state.go

+// VerifyClientConsensusState returns nil since a local host client does not store consensus
 // states.


It should not return an error; it should just pass.

cwgoes · 2020-08-20T10:48:24Z

x/ibc/09-localhost/types/client_state.go

 	_ []byte,
 	connectionID string,
 	connectionEnd connectionexported.ConnectionI,
 ) error {
-	path, err := commitmenttypes.ApplyPrefix(prefix, host.ConnectionPath(connectionID))


Aye, excellent use-case for a spec directory.

cwgoes · 2020-08-20T10:52:12Z

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

@@ -197,6 +200,48 @@ func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height uint64) (exported.
 	return consensusState, true
 }

+// ValidateSelfClient validates the client parameters for a client of the running chain
+// This function is only used to validate the client state the counterparty stores for this chain
+func (k Keeper) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error {


This doesn't check:

Max clock drift

Frozen height

Maximum clock drift can vary, so I think it's alright not to check that, but we should just make sure there aren't any values which will cause Tendermint to throw errors.

We should probably verify that the latter is 0 initially? Not required security-wise though.

Once we add an epoch number we'll need to check that.

Yes, I can add a frozen check, also epoch is implicitly checked by checking equality on chainID

…nto aditya/verify-client

* verify client state * add client state to msgs and retrieve in handler * fix connection msgs * fixed handshake tests * fix tests * fix sim tests * revert pb edit * add ValidateClient tests * Apply suggestions from code review Co-authored-by: Federico Kunze <[email protected]> * fix tests * fixed msgs test * use ibctesting for client state consts * Apply suggestions from code review Co-authored-by: colin axnér <[email protected]> Co-authored-by: Federico Kunze <[email protected]> * address rest of comments * rename to ValidateSelfClient and update spec * lint * Update x/ibc/02-client/keeper/keeper_test.go * Update x/ibc/02-client/keeper/keeper_test.go * complete rest of review * improve cov Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: colin axnér <[email protected]>

AdityaSripal added 2 commits August 14, 2020 18:06

verify client state

aa03892

add client state to msgs and retrieve in handler

a7e8646

AdityaSripal mentioned this pull request Aug 14, 2020

Verify ClientState on Connection Handshake #6556

Closed

9 tasks

AdityaSripal added the x/ibc label Aug 14, 2020

AdityaSripal added 7 commits August 17, 2020 18:24

fix connection msgs

c8fb33b

fixed handshake tests

fe85a62

fix tests

ec7edae

fix sim tests

56f9773

revert pb edit

fa09c9d

add ValidateClient tests

1d370e1

Merge branch 'master' of github.com:cosmos/cosmos-sdk into aditya/ver…

8c0a908

…ify-client

AdityaSripal commented Aug 17, 2020

View reviewed changes

AdityaSripal marked this pull request as ready for review August 17, 2020 23:39

AdityaSripal requested review from aaronc, alessio, alexanderbez, clevinson, fedekunze and jgimeno as code owners August 17, 2020 23:39

AdityaSripal requested review from cwgoes and colin-axner August 17, 2020 23:39

fedekunze reviewed Aug 18, 2020

View reviewed changes

x/ibc/02-client/keeper/keeper.go Outdated Show resolved Hide resolved

fedekunze reviewed Aug 18, 2020

View reviewed changes

colin-axner reviewed Aug 18, 2020

View reviewed changes

AdityaSripal and others added 3 commits August 18, 2020 16:09

Apply suggestions from code review

7fcf263

Co-authored-by: Federico Kunze <[email protected]>

fix tests

69846c4

remove rest files

8ffd01b

AdityaSripal requested a review from colin-axner August 18, 2020 22:17

AdityaSripal requested a review from fedekunze August 18, 2020 22:17

AdityaSripal added 2 commits August 18, 2020 18:35

fixed msgs test

73c1786

use ibctesting for client state consts

7d38197

colin-axner mentioned this pull request Aug 19, 2020

Restructure x/ibc layout #6945

Closed

4 tasks

colin-axner reviewed Aug 19, 2020

View reviewed changes

fedekunze reviewed Aug 19, 2020

View reviewed changes

AdityaSripal and others added 5 commits August 19, 2020 13:51

Apply suggestions from code review

9d3657b

Co-authored-by: colin axnér <[email protected]> Co-authored-by: Federico Kunze <[email protected]>

Merge branch 'master' of github.com:cosmos/cosmos-sdk into aditya/ver…

bdf83b5

…ify-client

address rest of comments

88ee0d6

Merge branch 'aditya/verify-client' of github.com:cosmos/cosmos-sdk i…

7b1ecd7

…nto aditya/verify-client

rename to ValidateSelfClient and update spec

b6f29a6

lint

4c105fa

fedekunze approved these changes Aug 20, 2020

View reviewed changes

fedekunze added 3 commits August 20, 2020 09:19

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

dcb0865

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

2395231

Merge branch 'master' into aditya/verify-client

b22a493

colin-axner approved these changes Aug 20, 2020

View reviewed changes

cwgoes approved these changes Aug 20, 2020

View reviewed changes

AdityaSripal added 4 commits August 20, 2020 17:46

complete rest of review

6b34ef6

Merge branch 'aditya/verify-client' of github.com:cosmos/cosmos-sdk i…

1e56cf3

…nto aditya/verify-client

improve cov

fd1a83a

fix merge

ba8e093

AdityaSripal added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 20, 2020

mergify bot merged commit 97df8b6 into master Aug 20, 2020

mergify bot deleted the aditya/verify-client branch August 20, 2020 22:05

colin-axner mentioned this pull request Mar 5, 2021

Add spec directory for each IBC client implementation cosmos/ibc-go#56

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify Client on Connection Handshake #7057

Verify Client on Connection Handshake #7057

AdityaSripal commented Aug 14, 2020 •

edited

Loading

codecov bot commented Aug 14, 2020 •

edited

Loading

AdityaSripal Aug 17, 2020

cwgoes Aug 20, 2020

colin-axner left a comment

AdityaSripal commented Aug 18, 2020

colin-axner left a comment

colin-axner commented Aug 19, 2020

fedekunze Aug 19, 2020

AdityaSripal Aug 19, 2020

colin-axner Aug 20, 2020

cwgoes Aug 20, 2020

fedekunze Aug 19, 2020

fedekunze Aug 19, 2020

AdityaSripal Aug 19, 2020

colin-axner Aug 20, 2020

cwgoes Aug 20, 2020

AdityaSripal commented Aug 19, 2020

fedekunze left a comment

fedekunze Aug 20, 2020

fedekunze Aug 20, 2020

fedekunze Aug 20, 2020

colin-axner commented Aug 20, 2020

colin-axner left a comment

cwgoes left a comment

cwgoes Aug 20, 2020

cwgoes Aug 20, 2020

cwgoes Aug 20, 2020

cwgoes Aug 20, 2020

AdityaSripal Aug 20, 2020

		// VerifyClientConsensusState returns nil since a local host client does not store consensus
		// states.

Verify Client on Connection Handshake #7057

Verify Client on Connection Handshake #7057

Conversation

AdityaSripal commented Aug 14, 2020 • edited Loading

Description

codecov bot commented Aug 14, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

AdityaSripal commented Aug 18, 2020

colin-axner left a comment

Choose a reason for hiding this comment

colin-axner commented Aug 19, 2020

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

AdityaSripal commented Aug 19, 2020

fedekunze left a 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

colin-axner commented Aug 20, 2020

colin-axner left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Aug 14, 2020 •

edited

Loading

codecov bot commented Aug 14, 2020 •

edited

Loading