Remove Last Header from ClientState #6873

AdityaSripal · 2020-07-29T02:12:39Z

Description

Removes LastHeader from ClientState
Allow updates to past heights

closes: #6736 and https://github.com/cosmos/ics/issues/420

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-07-29T02:18:41Z

Codecov Report

Merging #6873 into master will increase coverage by 5.84%.
The diff coverage is 55.73%.

@@            Coverage Diff             @@
##           master    #6873      +/-   ##
==========================================
+ Coverage   55.60%   61.44%   +5.84%     
==========================================
  Files         457      517      +60     
  Lines       27440    32103    +4663     
==========================================
+ Hits        15257    19726    +4469     
+ Misses      11083    10804     -279     
- Partials     1100     1573     +473

fedekunze

looks good. Minor comments

fedekunze · 2020-07-29T08:28:44Z

x/ibc/testing/chain.go

-		Time: chain.CurrentHeader.Time,
+		Time:               chain.CurrentHeader.Time,
+		ValidatorsHash:     chain.Vals.Hash(),
+		NextValidatorsHash: chain.Vals.Hash(),


I guess it's ok for now but it would be useful to test validator changes

could we open up an issue for this?

add to #6509 testing pkg wishlist

fedekunze · 2020-07-29T08:29:34Z

x/ibc/ibc_test.go

@@ -31,6 +33,8 @@ const (
 	channelOrder   = channeltypes.ORDERED
 	channelVersion = "1.0"

+	height = 10


nit, can we use the ibc testing package here? 🙏

this is just used by genesis_test.go, fine for now. handler_test.go uses ibc testing pkg

x/ibc/07-tendermint/update.go

fedekunze · 2020-07-29T08:34:42Z

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

-	ErrTrustingPeriodExpired  = sdkerrors.Register(SubModuleName, 6, "time since latest trusted state has passed the trusting period")
-	ErrUnbondingPeriodExpired = sdkerrors.Register(SubModuleName, 7, "time since latest trusted state has passed the unbonding period")
-	ErrInvalidProofSpecs      = sdkerrors.Register(SubModuleName, 8, "invalid proof specs")
+	ErrInvalidChainID         = sdkerrors.Register(SubModuleName, 2, "invalid chain-id")


I think this should live on the sdkerrors package since it's very general

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

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

…sdk into aditya/remove-last-header

AdityaSripal · 2020-07-30T21:07:56Z

Note: This PR currently does not support updating past heights once a client has already been frozen.

Thus, in an unfrozen client: It is possible for the latest height of client to be at height 10, and update to fill in a consensus state height at 7

In a frozen client with frozen height 10, it is not possible to update and fill in a consensus state at height < frozenheight.

Enabling this would require adding a method on ClientState interface FrozenHeight() uint64

If this addition is worth it, I can implement it in this PR; it is a fairly trivial addition from an implementation standpoint

cc: @cwgoes

cwgoes · 2020-07-31T08:54:17Z

Note: This PR currently does not support updating past heights once a client has already been frozen.

Thus, in an unfrozen client: It is possible for the latest height of client to be at height 10, and update to fill in a consensus state height at 7

In a frozen client with frozen height 10, it is not possible to update and fill in a consensus state at height < frozenheight.

Enabling this would require adding a method on ClientState interface FrozenHeight() uint64

If this addition is worth it, I can implement it in this PR; it is a fairly trivial addition from an implementation standpoint

cc: @cwgoes

Hmm, good question, per our current definition of frozenHeight this would be technically correct to allow, I think, although it doesn't matter too much - but if it's easy, let's do it.

cwgoes

Let's not store the validator set but rather the hash. Otherwise looks fine.

cwgoes · 2020-07-31T09:01:05Z

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

+	Root               commitmentexported.Root `json:"root" yaml:"root"`
+	Height             uint64                  `json:"height" yaml:"height"`
+	NextValidatorsHash tmbytes.HexBytes        `json:"next_validators_hash"` // validators hash for the next block
+	ValidatorSet       *tmtypes.ValidatorSet   `json:"validator_set" yaml:"validator_set"`


I don't think we want to store the whole validator set in each consensus state - too large - can we store the hash and require that the validator set be passed in update messages instead?

Hmm yea that's fine. Note this makes relayer flow for updating more complicated since the relayer needs to know the LatestHeight of the client in order to query the last trusted valset.

Though since this is true, we can just query for the ValidatorSet at LatestHeight+1 so that we can use NextValidators of last trusted header in lite.Verify

Ref issue here: https://github.com/cosmos/ics/issues/456#issuecomment-665118922

This requires significant changes with tradeoffs, I will implement this in a separate PR so people can see the diffs in isolation.

opened an issue to track this #6930

Agreed, this can be done in a follow-up.

colin-axner

Implementation looks good to me. This is a great add on, nice work!

The godoc and error messages seem to be have been neglected and need some love in being updated to convey the new functionality. I think it'd be fantastic if you could update the ibc spec on this logic as well. Maybe add a section in concepts.

The testing pkg was also relying on the client state needing the full signed header, which means there is now a lot of redundant info being tracked that can be discarded. Could you update the changes this pr does in relation to those fields?

I think it'd also be very nice to add some testing with the testing pkg. Just adding a func to update at a specific height would be nice and then testing filling consensus states for previous height. This could be done in a followup issue though

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

colin-axner · 2020-07-31T14:37:35Z

x/ibc/testing/chain.go

-		Time: chain.CurrentHeader.Time,
+		Time:               chain.CurrentHeader.Time,
+		ValidatorsHash:     chain.Vals.Hash(),
+		NextValidatorsHash: chain.Vals.Hash(),


could we open up an issue for this?

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

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

x/ibc/07-tendermint/update.go

x/ibc/07-tendermint/update_test.go

Co-authored-by: colin axner <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

colin-axner

left some more nits and suggestions

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

x/ibc/07-tendermint/update.go

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

x/ibc/07-tendermint/update.go

colin-axner · 2020-08-03T16:10:46Z

Following merge of this pr I'm pretty sure we can migrate client state entirely, consensusState and headers will have to wait for tendemint update. No fields in client state rely on tendermint except Fraction, but an initial migration already showed we have to define our own Fraction anyways

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

…sdk into aditya/remove-last-header

colin-axner

LGTM - amazing work!

colin-axner · 2020-08-04T08:08:43Z

I think the following changes should still be done

updating spec
removing unnecessary header parts from ibc-testing and the tm utils testing in the 07-tendermint/types
adding tests to check update functionality using testing pkg

But since this pr is now the blocking factor for client state migration, they should be done in followup pr's

cwgoes

ACK, one minor query, agreed on follow-ups

cwgoes · 2020-08-04T08:57:58Z

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

+	Root               commitmentexported.Root `json:"root" yaml:"root"`
+	Height             uint64                  `json:"height" yaml:"height"`
+	NextValidatorsHash tmbytes.HexBytes        `json:"next_validators_hash"` // validators hash for the next block
+	ValidatorSet       *tmtypes.ValidatorSet   `json:"validator_set" yaml:"validator_set"`


Agreed, this can be done in a follow-up.

cwgoes · 2020-08-04T08:59:54Z

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

@@ -71,9 +71,16 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H

 	switch clientType {
 	case exported.Tendermint:
+		trustedConsState, found := k.GetClientConsensusStateLTE(ctx, clientID, header.GetHeight())


This is probably alright, although I wonder if we want to allow the transaction to provide an explicit past height to update from - though in most cases the most recent height will be best, as we have here

fedekunze

LGTM

@colin-axner

* implement features, fix tendermint tests * allow test headers to pass in custom nextvalidatorset * allow updates to previous heights and test * remove unnecessary testing feature * fix client tests * fix ibc tests, and updating consensus state * fix ibc-transfer tests * appease linter * Apply suggestions from code review Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> * add chainID test case * fix expPass value * Apply suggestions from code review Co-authored-by: colin axner <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> * address rest of @colin-axner review * fix bug and errors * Apply suggestions from code review Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * address rest of @colin-axner review * implement updating before frozen height Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-authored-by: colin axner <25233464+colin-axner@users.noreply.github.com>

AdityaSripal added 7 commits July 28, 2020 17:12

implement features, fix tendermint tests

db20c91

allow test headers to pass in custom nextvalidatorset

d7ea75a

allow updates to previous heights and test

c5e9756

remove unnecessary testing feature

1583104

fix client tests

09ada3f

fix ibc tests, and updating consensus state

c5d0d52

fix ibc-transfer tests

5fbfca3

AdityaSripal requested review from aaronc, alessio, alexanderbez, clevinson, fedekunze and jgimeno as code owners July 29, 2020 02:12

fix conflicts

8a74ba3

appease linter

1180ed7

fedekunze reviewed Jul 29, 2020

View reviewed changes

AdityaSripal and others added 5 commits July 29, 2020 11:53

Apply suggestions from code review

6a246a9

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

add chainID test case

26b92bc

Merge branch 'aditya/remove-last-header' of github.com:cosmos/cosmos-…

21610a9

…sdk into aditya/remove-last-header

fix expPass value

acc6b19

Merge branch 'master' into aditya/remove-last-header

544bece

AdityaSripal requested review from cwgoes and colin-axner July 30, 2020 16:02

cwgoes suggested changes Jul 31, 2020

View reviewed changes

colin-axner requested changes Jul 31, 2020

View reviewed changes

AdityaSripal and others added 2 commits July 31, 2020 13:21

Apply suggestions from code review

8a2bbff

Co-authored-by: colin axner <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

address rest of @colin-axner review

f3e5fdd

AdityaSripal added 2 commits July 31, 2020 13:54

fix bug and errors

94aff30

Merge branch 'master' into aditya/remove-last-header

333b4c0

colin-axner reviewed Aug 3, 2020

View reviewed changes

AdityaSripal and others added 5 commits August 3, 2020 21:21

Apply suggestions from code review

f892220

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

address rest of @colin-axner review

b366db6

Merge branch 'aditya/remove-last-header' of github.com:cosmos/cosmos-…

c91bcb3

…sdk into aditya/remove-last-header

implement updating before frozen height

ec36c80

Merge branch 'master' into aditya/remove-last-header

7027989

colin-axner approved these changes Aug 4, 2020

View reviewed changes

colin-axner mentioned this pull request Aug 4, 2020

Store Validator Set Hash in tendermint consensus state #6930

Closed

4 tasks

colin-axner requested review from cwgoes and fedekunze August 4, 2020 07:55

cwgoes approved these changes Aug 4, 2020

View reviewed changes

fedekunze approved these changes Aug 4, 2020

View reviewed changes

fedekunze merged commit 9b61e09 into master Aug 4, 2020

fedekunze deleted the aditya/remove-last-header branch August 4, 2020 09:05

AdityaSripal mentioned this pull request Aug 14, 2020

Determine what in ClientState needs to be verified by counterparty #6524

Closed

colin-axner mentioned this pull request Mar 5, 2021

Reflect recent 02-client/tendermint changes in specs and testing cosmos/ibc-go#55

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Last Header from ClientState #6873

Remove Last Header from ClientState #6873

AdityaSripal commented Jul 29, 2020

codecov bot commented Jul 29, 2020 •

edited

Loading

fedekunze left a comment

fedekunze Jul 29, 2020

colin-axner Jul 31, 2020

colin-axner Aug 4, 2020

fedekunze Jul 29, 2020

colin-axner Aug 4, 2020

fedekunze Jul 29, 2020

AdityaSripal commented Jul 30, 2020

cwgoes commented Jul 31, 2020

cwgoes left a comment

cwgoes Jul 31, 2020

AdityaSripal Aug 4, 2020

AdityaSripal Aug 4, 2020

colin-axner Aug 4, 2020

cwgoes Aug 4, 2020

colin-axner left a comment

colin-axner Jul 31, 2020

colin-axner left a comment

colin-axner commented Aug 3, 2020

colin-axner left a comment

colin-axner commented Aug 4, 2020

cwgoes left a comment

cwgoes Aug 4, 2020

cwgoes Aug 4, 2020

fedekunze left a comment

Remove Last Header from ClientState #6873

Remove Last Header from ClientState #6873

Conversation

AdityaSripal commented Jul 29, 2020

Description

codecov bot commented Jul 29, 2020 • edited Loading

Codecov Report

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

AdityaSripal commented Jul 30, 2020

cwgoes commented Jul 31, 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

colin-axner left a comment

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

colin-axner commented Aug 3, 2020

colin-axner left a comment

Choose a reason for hiding this comment

colin-axner commented Aug 4, 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

fedekunze left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2020 •

edited

Loading