UpgradeClient Followup #1 #7457

AdityaSripal · 2020-10-05T16:04:59Z

Description

The current UpgradeClient followup logic does not take into account at what point the upgrade is going to occur on the old chain. This is an issue because multiple upgrade plans might be voted on and stored, with the latest one taking precedence. This becomes a problem since outdated plans are still provable by IBC light clients. Clients need some way to know what the latest planned upgrade is.

In order to accomplish this, I set the upgraded client at the height the planned upgrade will occur. This upgrade height must then be passed into the MsgUpgradeClient struct. The upgrade client logic will use this upgrade height as the proof height to ensure that the passed in upgrade client is the latest agreed upon client at the time of the upgrade height.

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-10-05T16:10:31Z

Codecov Report

Merging #7457 into master will increase coverage by 0.02%.
The diff coverage is 85.33%.

@@            Coverage Diff             @@
##           master    #7457      +/-   ##
==========================================
+ Coverage   55.25%   55.28%   +0.02%     
==========================================
  Files         591      591              
  Lines       36951    36998      +47     
==========================================
+ Hits        20416    20453      +37     
- Misses      14427    14432       +5     
- Partials     2108     2113       +5

proto/ibc/core/client/v1/client.proto

x/upgrade/keeper/keeper.go

x/ibc/light-clients/07-tendermint/types/upgrade.go

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

fedekunze · 2020-10-05T17:22:06Z

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

+		keys := strings.Split(cs.UpgradePath, "/")
+		for _, k := range keys {
+			if strings.TrimSpace(k) == "" {
+				return sdkerrors.Wrapf(ErrInvalidUpgradePath, "upgrade path invalid: %s", cs.UpgradePath)
+			}


ditto, can we move this to a ValidateUpgradePath function?

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

cwgoes · 2020-10-06T08:49:13Z

In order to accomplish this, I set the upgraded client at the height the planned upgrade will occur. This upgrade height must then be passed into the MsgUpgradeClient struct. The upgrade client logic will use this upgrade height as the proof height to ensure that the passed in upgrade client is the latest agreed upon client at the time of the upgrade height.

In principle this seems alright but we need to be careful that off-by-one errors won't cause problems here - in particular, transactions processed in the last block need an extra block to be committed-to and proved - will that work here?

x/ibc/core/02-client/types/msgs_test.go

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

x/upgrade/keeper/keeper.go

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

x/ibc/light-clients/07-tendermint/types/upgrade.go

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

…dk into aditya/upgrade-followups

AdityaSripal · 2020-10-06T16:52:20Z

In principle this seems alright but we need to be careful that off-by-one errors won't cause problems here - in particular, transactions processed in the last block need an extra block to be committed-to and proved - will that work here?

Yes this is an excellent point. This will work because it isn't as if the upgrade client is committed at the last block. It should be committed much earlier. It's just that we use the last block height to prove it since it the latest upgraded client is what will still remain in store by that point.

Although I think it is crucial that we see a live test of this feature in stargate testnets

cc: @cwgoes

…in store

lgtm-com · 2020-10-06T17:47:27Z

This pull request introduces 1 alert when merging 85817e5 into 3e6089d - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

x/ibc/light-clients/07-tendermint/types/upgrade.go

cwgoes · 2020-10-07T08:15:07Z

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

@@ -112,6 +112,14 @@ func (cs ClientState) Validate() error {
 			return sdkerrors.Wrap(ErrInvalidProofSpecs, "proof spec cannot be nil")
 		}
 	}
+	if cs.UpgradePath != "" {
+		keys := strings.Split(cs.UpgradePath, "/")


should we move this to a utility function or is there one we could already use?

colin-axner

I'm still uncertain about the handling on Get/SetUpgradeClient. It feels like something someone will try to rewrite. Can you add sufficient godoc as to why it is constructed the way it is

x/ibc/core/02-client/types/msgs_test.go

x/ibc/light-clients/07-tendermint/types/upgrade.go

x/upgrade/keeper/keeper.go

x/upgrade/keeper/keeper_test.go

x/upgrade/keeper/keeper.go

fedekunze

Changes LGTM. Pending previous comments from reviews

x/upgrade/keeper/keeper.go

…dk into aditya/upgrade-followups

colin-axner

ACK for most recent changes

cwgoes

utACK

require old chain halts before upgrade

fb9cd44

AdityaSripal requested review from cwgoes, colin-axner and fedekunze October 5, 2020 16:04

AdityaSripal requested review from aaronc, alessio and alexanderbez as code owners October 5, 2020 16:04

AdityaSripal mentioned this pull request Oct 5, 2020

UpgradeClient Followup #7445

Closed

9 tasks

AdityaSripal added the x/ibc label Oct 5, 2020

fedekunze reviewed Oct 5, 2020

View reviewed changes

Update proto/ibc/core/client/v1/client.proto

7dda223

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

AdityaSripal added this to the IBC 1.0 [Implementation] milestone Oct 5, 2020

start address reviews

37a20af

colin-axner reviewed Oct 6, 2020

View reviewed changes

AdityaSripal and others added 3 commits October 6, 2020 09:59

Apply suggestions from code review

7ad8d73

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

Merge branch 'aditya/upgrade-followups' of github.com:cosmos/cosmos-s…

ebaad0c

…dk into aditya/upgrade-followups

address reviews

a39437b

AdityaSripal added 2 commits October 6, 2020 12:59

fix merge

fef7f93

rework upgrade to ensure there is never more than one upgrade client …

85817e5

…in store

AdityaSripal added 4 commits October 6, 2020 14:09

fix tests

8d59a8f

fix conditional

25847e4

Merge branch 'master' into aditya/upgrade-followups

8e6268a

make proto-gen

8ba3d1f

cwgoes reviewed Oct 7, 2020

View reviewed changes

colin-axner reviewed Oct 7, 2020

View reviewed changes

Merge branch 'master' into aditya/upgrade-followups

690dfcc

remove if statement skipping tests in upgrade keeper test

2671765

colin-axner approved these changes Oct 7, 2020

View reviewed changes

fedekunze approved these changes Oct 7, 2020

View reviewed changes

x/upgrade/keeper/keeper.go Show resolved Hide resolved

x/upgrade/keeper/keeper.go Show resolved Hide resolved

colin-axner and others added 4 commits October 7, 2020 15:17

Merge branch 'master' into aditya/upgrade-followups

2d4c5f2

address reviews

f656313

Merge branch 'aditya/upgrade-followups' of github.com:cosmos/cosmos-s…

9c1e043

…dk into aditya/upgrade-followups

correctly escape and unescape merkle keys

86e5d30

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

add small conditional check

cb1556d

colin-axner approved these changes Oct 7, 2020

View reviewed changes

cwgoes approved these changes Oct 7, 2020

View reviewed changes

mergify bot merged commit 31ab35a into master Oct 8, 2020

mergify bot deleted the aditya/upgrade-followups branch October 8, 2020 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UpgradeClient Followup #1 #7457

UpgradeClient Followup #1 #7457

AdityaSripal commented Oct 5, 2020 •

edited

Loading

codecov bot commented Oct 5, 2020 •

edited

Loading

fedekunze Oct 5, 2020

cwgoes commented Oct 6, 2020

AdityaSripal commented Oct 6, 2020 •

edited

Loading

lgtm-com bot commented Oct 6, 2020

cwgoes Oct 7, 2020

colin-axner left a comment

fedekunze left a comment

colin-axner left a comment

cwgoes left a comment

UpgradeClient Followup #1 #7457

UpgradeClient Followup #1 #7457

Conversation

AdityaSripal commented Oct 5, 2020 • edited Loading

Description

codecov bot commented Oct 5, 2020 • edited Loading

Codecov Report

fedekunze Oct 5, 2020

Choose a reason for hiding this comment

cwgoes commented Oct 6, 2020

AdityaSripal commented Oct 6, 2020 • edited Loading

lgtm-com bot commented Oct 6, 2020

cwgoes Oct 7, 2020

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

AdityaSripal commented Oct 5, 2020 •

edited

Loading

codecov bot commented Oct 5, 2020 •

edited

Loading

AdityaSripal commented Oct 6, 2020 •

edited

Loading