Abstract Height in IBC Clients #6686

AdityaSripal · 2020-07-10T22:31:57Z

Description

Adds height interface to 02-client, and concrete types in 07-tendermint and 09-localhost along with appropriate update/misbehaviour handling

Important Context For Reviewers:

This PR does not implement all of the abstract height functionality necessary for upgrades. Since this is already a very large change this will be done in stages. This particular PR implements the following:

All reference to height (except for 23-commitment which will continue using uint64) will now use the exported.Height instead of uint64 in all of ibc and ibc-transfer
Updates are supported within a single epoch
Counterparty chains are allowed to initialize at whatever epoch-number they choose (and can continue updating EpochHeight). The self-chain, however, only supports EpochNumber: 0 for now.
Updates to the next EpochNumber are not supported, this will be done in future PR
Misbehaviour that freezes a height at an earlier epoch, will cause any updates in a future epoch to fail.
Packets will timeout if clientstate is at epoch number heigher than timeout height

Changes to be done in Future PR:

Extract EpochNumber from clientID so that validators commit to an epoch number.
Enable Updating a client to a new EpochNumber

closes: #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

colin-axner · 2020-07-23T10:08:49Z

what's the status of this? is it blocked?

AdityaSripal · 2020-07-23T16:46:10Z

Just need to fix the many merge conflicts 😱

cwgoes

Thanks; good start, see comments.

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

cwgoes · 2020-07-26T10:41:33Z

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

+// zero     if a = b
+// positive if a > b
+//
+// Decrement will return a decremented height from the given height. If this is not possible,


Move comment to below function?

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

cwgoes · 2020-07-26T10:43:38Z

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

-	histInfo, found := k.stakingKeeper.GetHistoricalInfo(ctx, int64(height))
+func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, bool) {
+	// Only support retrieving HistoricalInfo from the current epoch for now
+	// TODO: check EpochNumber matches expected


This should work across epochs, I think, so that handshakes will work cleanly across upgrades

(we should use both the epoch number & the epoch height in the key)

cwgoes · 2020-07-26T10:44:08Z

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

+	height := maxHeight
+	var success bool
+	for height.Valid() {
+		found := k.HasClientConsensusState(ctx, clientID, height)


Hmm, this isn't very efficient. Can we iterate on the key?

I can look into this on a separate PR. Should be able to use iavl.ReverseIterator

cwgoes · 2020-07-26T10:45:35Z

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

@@ -63,6 +64,9 @@ func (p Packet) GetDestChannel() string { return p.DestinationChannel }
 // GetData implements PacketI interface
 func (p Packet) GetData() []byte { return p.Data }

+// GetTimeoutEpoch implements PacketI interface
+func (p Packet) GetTimeoutEpoch() uint64 { return p.TimeoutEpoch }


Hmm, why this two-function strategy? Better to have a GetTimeoutHeight() Height function instead, no?

I think this has to do with proto. Since Height is not proto defined, it cannot be referenced in the .proto files (or maybe it can?) so the work around is to define the 2 uints and then construct them when needed to compare, decrement etc.

I'm not very convinced this solution is any better than just dealing with Any. I think it will lead to more technical debt/possible bugs

Currently this would introduce an import cycle between 02-client/exported and 04-client/exported

fedekunze

First pass, looks great! It'd be nice if we could add LT(), Equal(), GT() util funcs for Height (i.e basically wrappers for compare).

proto/ibc/connection/connection.proto

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

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

fedekunze · 2020-07-27T10:41:05Z

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

+	EpochNumber uint64 `json:"epoch_number" yaml:"epoch_number"`
+	EpochHeight uint64 `json:"epoch_height" yaml:"epoch_height"`


maybe for a third party, it's not so clear what an epoch means/represents. Can you add a comment + example about that on the Height godoc and also add comments on each of the fields in order to distinguish them?

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

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

colin-axner

Thanks for doing this work!!!

Did a high level scan, the general approach looks good. I have some serious concerns about the current work around to just using Any. I'd be willing to lend a hand in using Any because the current approach of separating epoch number and epoch height makes me question why you would even need a Height struct at all?

proto/ibc/connection/connection.proto

colin-axner · 2020-07-27T10:26:35Z

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

+// Decrement will return a decremented height from the given height. If this is not possible,
+// an error is returned
+// Valid returns true if height is valid, false otherwise
+type Height struct {


I don't think we should regularly rely on concretely defining objects in exported.go. It is poor code hygiene (exported does not imply type implementation). Also, I think Height needs to be a protobuf object once we migrate 02-client?

This is outside the scope of this pr, but something we should address in the future.

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

x/ibc/03-connection/handler.go

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

colin-axner · 2020-07-27T10:56:08Z

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

@@ -63,6 +64,9 @@ func (p Packet) GetDestChannel() string { return p.DestinationChannel }
 // GetData implements PacketI interface
 func (p Packet) GetData() []byte { return p.Data }

+// GetTimeoutEpoch implements PacketI interface
+func (p Packet) GetTimeoutEpoch() uint64 { return p.TimeoutEpoch }


I think this has to do with proto. Since Height is not proto defined, it cannot be referenced in the .proto files (or maybe it can?) so the work around is to define the 2 uints and then construct them when needed to compare, decrement etc.

I'm not very convinced this solution is any better than just dealing with Any. I think it will lead to more technical debt/possible bugs

Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: colin axner <[email protected]> Co-authored-by: Christopher Goes <[email protected]>

fedekunze

LGTM

fedekunze · 2020-07-30T08:09:25Z

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

+// we can call a.Compare(b) which will return
+// -1 if a < b
+// 0  if a = b
+// 1  if a > b


Suggested change

// we can call a.Compare(b) which will return

// -1 if a < b

// 0 if a = b

// 1 if a > b

// we can call a.Compare(b) which will return:

//

// - -1 if a < b

// - 0 if a = b

// - 1 if a > b

fedekunze · 2020-07-30T08:11:16Z

x/ibc/02-client/handler.go

@@ -38,8 +38,9 @@ func HandleMsgCreateClient(ctx sdk.Context, k keeper.Keeper, msg exported.MsgCre
 		consensusHeight = msg.GetConsensusState().GetHeight()
 	case exported.Localhost:
 		// msg client id is always "localhost"
+		// Localhost epoch is always 0 for now


@cwgoes I think this needs a note on the ICS09 spec

hmm, localhost clients shouldn't even have an epoch

nor should solo machine clients

fedekunze · 2020-07-30T08:12:49Z

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

-	histInfo, found := k.stakingKeeper.GetHistoricalInfo(ctx, int64(height))
+func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, bool) {
+	// Only support retrieving HistoricalInfo from the current epoch for now
+	// TODO: check EpochNumber matches expected


do we need to address this TODO on this PR?

I think it shouldn't be hard? We just need to store historical info with an epoch as well as a height

fedekunze · 2020-07-30T08:14:27Z

x/ibc/03-connection/handler.go

@@ -65,9 +70,12 @@ func HandleMsgConnectionOpenTry(ctx sdk.Context, k keeper.Keeper, msg *types.Msg

 // HandleMsgConnectionOpenAck defines the sdk.Handler for MsgConnectionOpenAck
 func HandleMsgConnectionOpenAck(ctx sdk.Context, k keeper.Keeper, msg *types.MsgConnectionOpenAck) (*sdk.Result, error) {
+	// For now, convert uint64 heights to clientexported.Height


ditto issue reference

fedekunze · 2020-07-30T08:14:40Z

x/ibc/03-connection/handler.go

@@ -95,8 +103,10 @@ func HandleMsgConnectionOpenAck(ctx sdk.Context, k keeper.Keeper, msg *types.Msg

 // HandleMsgConnectionOpenConfirm defines the sdk.Handler for MsgConnectionOpenConfirm
 func HandleMsgConnectionOpenConfirm(ctx sdk.Context, k keeper.Keeper, msg *types.MsgConnectionOpenConfirm) (*sdk.Result, error) {
+	// For now, convert uint64 heights to clientexported.Height


ditto issue reference

fedekunze · 2020-07-30T08:17:11Z

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

@@ -334,14 +338,15 @@ func (msg MsgChannelCloseConfirm) GetSigners() []sdk.AccAddress {

 var _ sdk.Msg = &MsgRecvPacket{}

-// NewMsgRecvPacket constructs new MsgRecvPacket
+// NewMsgPacket constructs new MsgPacket


Suggested change

// NewMsgPacket constructs new MsgPacket

// NewMsgRecvPacket constructs new MsgRecvPacket

fedekunze · 2020-07-30T08:21:15Z

x/ibc/testing/chain.go

@@ -409,12 +411,16 @@ func (chain *TestChain) ConnectionOpenTry(

 	proofConsensus, consensusHeight := counterparty.QueryConsensusStateProof(counterpartyConnection.ClientID)

+	// For now, assume our own epoch numebr is 0, while retrieving epoch number for counterparty


Suggested change

// For now, assume our own epoch numebr is 0, while retrieving epoch number for counterparty

// For now, assume our own epoch number is 0, while retrieving epoch number for counterparty

fedekunze · 2020-07-30T08:21:28Z

x/ibc/testing/chain.go

@@ -430,10 +436,14 @@ func (chain *TestChain) ConnectionOpenAck(

 	proofConsensus, consensusHeight := counterparty.QueryConsensusStateProof(counterpartyConnection.ClientID)

+	// For now, assume our own epoch numebr is 0, while retrieving epoch number for counterparty


Suggested change

// For now, assume our own epoch numebr is 0, while retrieving epoch number for counterparty

// For now, assume our own epoch number is 0, while retrieving epoch number for counterparty

cwgoes

See outstanding comment on fetching past self consensus states by epoch & height.

cwgoes · 2020-07-31T08:50:29Z

x/ibc/02-client/handler.go

@@ -38,8 +38,9 @@ func HandleMsgCreateClient(ctx sdk.Context, k keeper.Keeper, msg exported.MsgCre
 		consensusHeight = msg.GetConsensusState().GetHeight()
 	case exported.Localhost:
 		// msg client id is always "localhost"
+		// Localhost epoch is always 0 for now


hmm, localhost clients shouldn't even have an epoch

cwgoes · 2020-07-31T08:50:42Z

x/ibc/02-client/handler.go

@@ -38,8 +38,9 @@ func HandleMsgCreateClient(ctx sdk.Context, k keeper.Keeper, msg exported.MsgCre
 		consensusHeight = msg.GetConsensusState().GetHeight()
 	case exported.Localhost:
 		// msg client id is always "localhost"
+		// Localhost epoch is always 0 for now


nor should solo machine clients

cwgoes · 2020-07-31T08:51:14Z

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

-	histInfo, found := k.stakingKeeper.GetHistoricalInfo(ctx, int64(height))
+func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, bool) {
+	// Only support retrieving HistoricalInfo from the current epoch for now
+	// TODO: check EpochNumber matches expected


I think it shouldn't be hard? We just need to store historical info with an epoch as well as a height

lgtm-com · 2020-08-25T22:34:00Z

This pull request introduces 4 alerts when merging 541737c into d9fd4d2 - view on LGTM.com

new alerts:

4 for Useless assignment to local variable

lgtm-com · 2020-08-25T23:16:54Z

This pull request introduces 4 alerts when merging cde1aea into d9fd4d2 - view on LGTM.com

new alerts:

4 for Useless assignment to local variable

lgtm-com · 2020-08-26T23:25:35Z

This pull request introduces 2 alerts when merging 43ec7bf into 9fc0dbb - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

AdityaSripal · 2020-09-15T19:38:21Z

Closed by #7184 and #7211

AdityaSripal added 4 commits July 7, 2020 19:03

add abstract height method

e3ee983

convert 07-tendermint to abstract height

6233c94

make localhost build pass

c51bab1

make client keeper changes

ee5edcc

AdityaSripal requested review from aaronc, alessio, alexanderbez, clevinson, fedekunze and jgimeno as code owners July 10, 2020 22:31

AdityaSripal added the x/ibc label Jul 10, 2020

AdityaSripal marked this pull request as draft July 10, 2020 22:34

AdityaSripal added 15 commits July 14, 2020 15:25

fix connection and update proto

488d232

update all proto files correctly

10c3a3f

fix all non-test builds

8fa0e2f

fix clients

b405b9e

fix ibc testing package

5adc5bc

fix tmtypes test

6c494bb

fix tendermint package tests

faafff7

fix localhost tests

660b0ff

fix client tests

04cb265

fix connection tests

179943d

fix channel tests

452a66f

fix all x/ibc tests

ac32c79

fix all tests

7dfe1c9

fmt proto files

2bdf31f

add tendermint abstract height handling tests

fcefa34

move frozen test case to appropriate pkg

ff89df0

AdityaSripal requested review from cwgoes and colin-axner July 23, 2020 21:28

AdityaSripal marked this pull request as ready for review July 23, 2020 21:29

fix lint bugs

c2e0cd7

cwgoes suggested changes Jul 26, 2020

View reviewed changes

fedekunze reviewed Jul 27, 2020

View reviewed changes

colin-axner reviewed Jul 27, 2020

View reviewed changes

AdityaSripal and others added 4 commits July 28, 2020 21:59

Apply suggestions from code review

aa0a342

Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: colin axner <[email protected]> Co-authored-by: Christopher Goes <[email protected]>

make Compare more idiomatic

c74bec9

fix godoc

b612ac7

address @colin-axner comments

363287c

fedekunze approved these changes Jul 30, 2020

View reviewed changes

AdityaSripal mentioned this pull request Jul 30, 2020

Replace clientexported.Height with proto Height #6888

Closed

fedekunze requested a review from cwgoes July 30, 2020 19:20

cwgoes reviewed Jul 31, 2020

View reviewed changes

AdityaSripal added 4 commits August 25, 2020 17:32

fix merge in x/ibc

28585e9

fix rest of merges

aa9d1e6

refix some builds

3f77210

cleanup last merge issues

541737c

AdityaSripal added 2 commits August 25, 2020 18:44

start migrating fields to use proto Height

898c363

start using Height in 02-client

cde1aea

AdityaSripal marked this pull request as draft August 25, 2020 23:04

AdityaSripal added 2 commits August 26, 2020 18:19

start updating clients with proto Height

3a7910b

fix client types

43ec7bf

AdityaSripal closed this Sep 15, 2020

AdityaSripal deleted the aditya/abstract-height branch September 15, 2020 19:38

		EpochNumber uint64 `json:"epoch_number" yaml:"epoch_number"`
		EpochHeight uint64 `json:"epoch_height" yaml:"epoch_height"`

	// NewMsgPacket constructs new MsgPacket
	// NewMsgRecvPacket constructs new MsgRecvPacket

		@@ -409,12 +411,16 @@ func (chain *TestChain) ConnectionOpenTry(

		proofConsensus, consensusHeight := counterparty.QueryConsensusStateProof(counterpartyConnection.ClientID)

		// For now, assume our own epoch numebr is 0, while retrieving epoch number for counterparty

	// For now, assume our own epoch numebr is 0, while retrieving epoch number for counterparty
	// For now, assume our own epoch number is 0, while retrieving epoch number for counterparty

		@@ -430,10 +436,14 @@ func (chain *TestChain) ConnectionOpenAck(

		proofConsensus, consensusHeight := counterparty.QueryConsensusStateProof(counterpartyConnection.ClientID)

		// For now, assume our own epoch numebr is 0, while retrieving epoch number for counterparty

Abstract Height in IBC Clients #6686

Abstract Height in IBC Clients #6686

Conversation

AdityaSripal commented Jul 10, 2020 • edited Loading

Description

colin-axner commented Jul 23, 2020

AdityaSripal commented Jul 23, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze left a comment • edited Loading

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

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

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

Choose a reason for hiding this comment

lgtm-com bot commented Aug 25, 2020

lgtm-com bot commented Aug 25, 2020

lgtm-com bot commented Aug 26, 2020

AdityaSripal commented Sep 15, 2020

AdityaSripal commented Jul 10, 2020 •

edited

Loading

fedekunze left a comment •

edited

Loading