Implement Height interface in core IBC #7211

AdityaSripal · 2020-08-31T21:07:53Z

Description

Part 2 of #6531

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

jackzampolin · 2020-09-01T20:53:45Z

This PR is blocking continued progress on the relayer

AdityaSripal · 2020-09-01T22:33:41Z

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

@@ -72,13 +77,16 @@ to the counterparty channel. Any timeout set to 0 is disabled.`),
 			// if the timeouts are not absolute, retrieve latest block height and block timestamp
 			// for the consensus state connected to the destination port/channel
 			if !absoluteTimeouts {
-				consensusState, _, err := channelutils.QueryCounterpartyConsensusState(clientCtx, srcPort, srcChannel, uint64(clientCtx.Height))
+				consensusState, _, err := channelutils.QueryLatestConsensusState(clientCtx, srcPort, srcChannel)


This was incorrect, we cannot query consensus state with a consensus height using our own block height

AdityaSripal · 2020-09-01T22:41:58Z

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

 			if err != nil {
 				return err
 			}

-			proofHeight := uint64(clientCtx.Height)


This was incorrect, cannot retrieve proofHeight/consensusHeight from our own context

thanks for fixing these, most of the cli code is broken and we can address anything else in a final refactor of it

AdityaSripal · 2020-09-01T22:42:19Z

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

 			if err != nil {
 				return err
 			}

-			proofHeight := uint64(clientCtx.Height)


x/ibc/04-channel/exported/exported.go

jackzampolin · 2020-09-02T00:39:11Z

Quick Q on the height interface... How do I get the height number out of it?

colin-axner · 2020-09-02T08:26:06Z

Quick Q on the height interface... How do I get the height number out of it?

cast to the height concrete type and use height.EpochHeight. The interface is just there because we use exported.go files, we plan to enforce usage of the concrete type only by not using Any.

…k into aditya/height-interface

colin-axner

Fantastic work !!! Glad to see this is finally ready to be merged! 🎉

left some small nits, but nothing needs to be addressed now. In a followup pr or at some point in the future is fine

proto/ibc/channel/query.proto

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

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

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

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

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

colin-axner · 2020-09-03T10:15:09Z

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

 			version.AppName, host.ModuleName, types.SubModuleName,
 		),
-		Args: cobra.ExactArgs(10),
+		Args: cobra.ExactArgs(12),


lmao the usability of entering 12 arguments by command line 👀 My hot take is IBC should drop support for doing handshakes by CLI. I'd rather just leave this to relayers

hehe, this is probably reasonable, although it has been convenient for testing

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

fedekunze

Still have 40+ files left to review

proto/ibc/connection/query.proto

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

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

fedekunze · 2020-09-03T08:56:37Z

x/ibc/light-clients/solomachine/types/client_state_test.go

 	testConnectionID             = "connectionid"
 	testChannelID                = "testchannelid"
 	testPortID                   = "testportid"
 )

 var (
-	prefix = commitmenttypes.NewMerklePrefix([]byte("ibc"))
+	prefix          = commitmenttypes.NewMerklePrefix([]byte("ibc"))


can we define this in ibctesting?

I think we can just use chain.GetPrefix()

x/ibc/spec/01_concepts.md

x/ibc/light-clients/solomachine/types/client_state.go

x/ibc/24-host/keys.go

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

fedekunze · 2020-09-03T10:45:57Z

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

@@ -152,7 +151,7 @@ func checkValidity(

 // update the consensus state from a new header
 func update(clientState *ClientState, header *Header) (*ClientState, *ConsensusState) {
-	height := clienttypes.NewHeight(0, header.GetHeight())
+	height := header.GetHeight().(clienttypes.Height)


is this validated somewhere? otherwise, it will panic

if this is a Tendermint HeaderI would prefer to return the concrete type instead from the fields

fedekunze

final review comments. Great work!

x/ibc/04-channel/client/utils/utils.go

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

x/ibc/04-channel/client/utils/utils.go

fedekunze · 2020-09-03T10:58:14Z

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

@@ -731,3 +732,7 @@ func (suite *KeeperTestSuite) TestChanCloseConfirm() {
 		})
 	}
 }
+
+func malleateHeight(height exported.Height, diff uint64) exported.Height {


Should we move this to ibctesting since it's reused on the handshakes?

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

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

colin-axner · 2020-09-03T13:11:45Z

it might be worthwhile to add a ZeroHeight() func to clientypes similar to sdk.ZeroInt()

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

…k into aditya/height-interface

AdityaSripal · 2020-09-03T19:25:07Z

All the RPC request types must have the EpochNumber and EpochHeight split. This is because you cannot put a message type into the rpc get url. Thus, they need to be split up so that the rpc url can be constructed like this example:

https://github.com/cosmos/cosmos-sdk/blob/aditya/height-interface/proto/ibc/client/query.proto#L26

fedekunze

ACK 👍

cwgoes

I think we need to think a bit about the misbehaviour handling logic (may also require minor spec updates). Otherwise great work.

cwgoes · 2020-09-03T20:24:31Z

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

 			version.AppName, host.ModuleName, types.SubModuleName,
 		),
-		Args: cobra.ExactArgs(10),
+		Args: cobra.ExactArgs(12),


hehe, this is probably reasonable, although it has been convenient for testing

cwgoes · 2020-09-03T20:26:59Z

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

 	}

 	// calculate the age of the misbehaviour
-	infractionHeight := tmMisbehaviour.GetHeight()
+	infractionHeight := tmMisbehaviour.GetHeight().(clienttypes.Height).EpochHeight


Hmm, I'm a little worried about this. Is it possible we could treat previously invalid evidence as valid again after an upgrade? I think we need to parse the epoch from the chain ID of the misbehaviour and check using the height comparator.

cwgoes · 2020-09-03T20:27:56Z

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

 	}

 	// calculate the age of the misbehaviour
-	infractionHeight := tmMisbehaviour.GetHeight()
+	infractionHeight := tmMisbehaviour.GetHeight().(clienttypes.Height).EpochHeight
 	infractionTime := tmMisbehaviour.GetTime()
 	ageDuration := ctx.BlockTime().Sub(infractionTime)
 	ageBlocks := int64(cs.LatestHeight.EpochHeight - infractionHeight)


ageBlocks can still be calculated by summing the differences across epochs

This would require knowing the last epoch height of each epoch, and having them easily retrievable from state

Aye, yes, never mind

AdityaSripal · 2020-09-03T20:59:43Z

Good point @cwgoes . We want to have the ability to submit misbehaviour from a previous epoch without storing the last epoch height of each epoch and summing them up to find out an absolute difference in block heights.

Note that in the expiration check we have both a time check and a block check. We can retain both checks in the case where the misbehaviour epoch number and the client's current epoch are the same.

In cases where the misbehaviour epoch is less than the client epoch, we can rely solely on the time check which will still work across epochs even as the epoch heights reset.

This allows us to submit previous epoch misbehaviour without adding any more state in the client store to track last epoch-heights for each epoch

AdityaSripal added 6 commits August 31, 2020 15:01

change interfaces

6e695f1

fix tendermint client interfaces

48f8119

fix other clients interfaces

8f884d3

fix queries

0f669d8

fix tendermint build

afb615b

fix builds of clients

6d23423

AdityaSripal requested review from aaronc, alessio and alexanderbez as code owners August 31, 2020 21:07

AdityaSripal marked this pull request as draft August 31, 2020 21:07

AdityaSripal changed the title ~~Start implementing Height interface in core IBC~~ Implement Height interface in core IBC Aug 31, 2020

AdityaSripal added the x/ibc label Aug 31, 2020

AdityaSripal added 3 commits September 1, 2020 13:56

fix 02-build

20380f2

start fixing connection and make queries non-nullable

9cc8450

fix connection keepers and queries

a74862f

fix connection build

e0c1692

jackzampolin marked this pull request as ready for review September 1, 2020 21:21

fix channel build

b9471a9

AdityaSripal marked this pull request as draft September 1, 2020 21:44

AdityaSripal requested a review from jackzampolin September 1, 2020 21:44

AdityaSripal added 2 commits September 1, 2020 18:22

fix all non-test files

bad9d59

fix testing build

7d17d80

AdityaSripal commented Sep 1, 2020

View reviewed changes

AdityaSripal added 3 commits September 1, 2020 18:51

cleanup

be79eaa

lint

0f5c6a3

fix merge

2d1a2b2

jackzampolin mentioned this pull request Sep 1, 2020

Migrate to Gaia v3.0 cosmos/relayer#278

Merged

AdityaSripal and others added 7 commits September 2, 2020 18:53

fix client tests

2543a7b

Merge branch 'master' into aditya/height-interface

e3cee11

fix channel tests

5f6ef15

fix all ibc tests

9ce03df

Merge branch 'aditya/height-interface' of github.com:cosmos/cosmos-sd…

7602b5a

…k into aditya/height-interface

fix transfer tests:

9c0276a

docs

eaaad0f

AdityaSripal marked this pull request as ready for review September 2, 2020 23:48

AdityaSripal requested review from colin-axner, fedekunze and cwgoes September 2, 2020 23:48

fix connection query

eb56e9a

colin-axner approved these changes Sep 3, 2020

View reviewed changes

Merge branch 'master' into aditya/height-interface

8bd956f

fedekunze suggested changes Sep 3, 2020

View reviewed changes

fedekunze reviewed Sep 3, 2020

View reviewed changes

AdityaSripal and others added 4 commits September 3, 2020 14:55

Apply suggestions from code review

f953ac2

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

wrap-up review

d9ed6be

Merge branch 'aditya/height-interface' of github.com:cosmos/cosmos-sd…

2df72d9

…k into aditya/height-interface

Merge branch 'master' into aditya/height-interface

2d62a86

fedekunze approved these changes Sep 3, 2020

View reviewed changes

fedekunze merged commit 4f9e31e into master Sep 3, 2020

fedekunze deleted the aditya/height-interface branch September 3, 2020 20:23

fedekunze mentioned this pull request Sep 3, 2020

Replace clientexported.Height with proto Height #6888

Closed

cwgoes suggested changes Sep 3, 2020

View reviewed changes

AdityaSripal mentioned this pull request Sep 3, 2020

7211 followup #7235

Merged

9 tasks

AdityaSripal mentioned this pull request Sep 15, 2020

Abstract Height in IBC Clients #6686

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Height interface in core IBC #7211

Implement Height interface in core IBC #7211

AdityaSripal commented Aug 31, 2020 •

edited by colin-axner

Loading

jackzampolin commented Sep 1, 2020 •

edited

Loading

AdityaSripal Sep 1, 2020

AdityaSripal Sep 1, 2020

colin-axner Sep 2, 2020

AdityaSripal Sep 1, 2020

jackzampolin commented Sep 2, 2020

colin-axner commented Sep 2, 2020

colin-axner left a comment

colin-axner Sep 3, 2020

cwgoes Sep 3, 2020

fedekunze left a comment

fedekunze Sep 3, 2020

colin-axner Sep 3, 2020

fedekunze Sep 3, 2020

fedekunze Sep 3, 2020

fedekunze left a comment

fedekunze Sep 3, 2020

colin-axner commented Sep 3, 2020

AdityaSripal commented Sep 3, 2020

fedekunze left a comment

cwgoes left a comment

cwgoes Sep 3, 2020

cwgoes Sep 3, 2020

cwgoes Sep 3, 2020

AdityaSripal Sep 3, 2020

cwgoes Sep 4, 2020

AdityaSripal commented Sep 3, 2020

Implement Height interface in core IBC #7211

Implement Height interface in core IBC #7211

Conversation

AdityaSripal commented Aug 31, 2020 • edited by colin-axner Loading

Description

jackzampolin commented Sep 1, 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

jackzampolin commented Sep 2, 2020

colin-axner commented Sep 2, 2020

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

fedekunze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner commented Sep 3, 2020

AdityaSripal commented Sep 3, 2020

fedekunze 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 Sep 3, 2020

AdityaSripal commented Aug 31, 2020 •

edited by colin-axner

Loading

jackzampolin commented Sep 1, 2020 •

edited

Loading