Add verifiable upgrade type and validate basic functions #3451

chatton · 2023-04-13T09:53:11Z

Description

closes: #3445

This PR also introduces a helper function to check if a connection is open. Other checks can be refactored in a follow up.

Follow up Items after this PR is merged:

Add ValidateBasic tests for Upgrade Types
Determine the correct usage of latest_sequence_send field.
Implement this suggestion

Commit Message / Changelog Entry

N/A part of feature branch

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 and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

…unction

modules/core/04-channel/keeper/keeper_test.go

DimitrisJim · 2023-04-13T12:50:46Z

modules/core/04-channel/types/upgrade.go

+		return errorsmod.Wrap(ErrInvalidUpgrade, "upgrade timeout cannot be empty")
+	}
+
+	// TODO: determine if last packet sequence sent can be 0?


leave this todo in? I remember we talked about it being sensible if its zero (i.e channel which has just opened) but maybe it makes sense to leave it around for another pair of eyes to check?

Yes I think 0 should technically be a valid value (no packets sent yet), maybe @colin-axner / @AdityaSripal could confirm if we should let 0 be an accepted value here?

Packet sequences start at 1. But iirc from yesterday this value can be determined within the state machine in WriteChanUpgradeInit() as opposed to expecting a user provided value

validate basic of the message should then check for >= 1 right? Even if it is determined within the state machine it should still be validated here if possible I think.

Maybe we should remove this function? I think we should have MsgUpgradeInit and MsgUpgradeTry use the UpgradeFields and UpgradeTimeout directly rather than wrapping with Upgrade. I suggest this because it seems unnecessary to expose a field populated by the state machine to the relayer. This would render this ValidateBasic unnecessary as the msg should do basic validation on the fields and timeout and then the state machine would construct the upgrade type with the fields, timeout and retrieved packet sequence send. I guess we could keep if for the situation where the entire counterparty upgrade type is provided in the TRY and ACK, in this situation the sequence should not equal 0.

My vote is to modify the init/try field for self upgrade to use the fields/timeout directly in the msg type and add a if sequence != 0 check to be used to verify the fully formed counterparty upgrade

sounds good to me

Yeah I'm happy enough with that approach 👍

Ah, just realising the message modification will cause a bigger diff with the OnChanInit keeper func which is being addressed in a follow up. @colin-axner are you happy if the message structure gets updated (and this validate basic removed) in a follow up?

Absolutely!

…e-upgrade-type-and-validate-basic-functions

damiannolan

Looks great. I left some suggestions, mostly around naming

modules/core/04-channel/types/upgrade.go

damiannolan · 2023-04-13T13:40:29Z

modules/core/04-channel/types/upgrade.go

+		return errorsmod.Wrap(ErrInvalidUpgrade, "upgrade timeout cannot be empty")
+	}
+
+	// TODO: determine if last packet sequence sent can be 0?


Packet sequences start at 1. But iirc from yesterday this value can be determined within the state machine in WriteChanUpgradeInit() as opposed to expecting a user provided value

proto/ibc/core/channel/v1/upgrade.proto

damiannolan · 2023-04-13T13:47:39Z

modules/core/04-channel/keeper/keeper.go

+		return errorsmod.Wrap(types.ErrInvalidChannelOrdering, "channel ordering must be a subset of the new ordering")
+	}
+
+	return k.connectionKeeper.CheckIsOpen(ctx, proposedUpgrade.ConnectionHops[0])


Personally I prefer just doing the check explicitly in here rather than adding another expected keeper interface method but happy to keep this as is if there's consensus on adding this instead.
My vote would be for doing the check directly here.

The idea is that this could be used in many other places, it also covers the not found case. Quickly looking it looks like we have around 7 other duplicate checks.

My preference would be to re-use this helper

I agree with @damiannolan. I think also the CheckIsOpen function should anyway return a boolean (unless we rename it to CheckExistsAndIsOpen, since it is actually checking two things). In my opinion I would just do the explicit check here.

yeah there was a bit of bike-shedding over the name/scope of this function between us. Initially we had it as IsOpen with a single boolean return but that didn't make it as self contained as we'd like. Returning the error allowed it to basically also check for any required error conditions and return them, simplifying the validation, hence the rename to CheckIsOpen.

Cian did say this function might not fly with everyone though 😄

I'm happy to just remove the function entirely and do the explicit checking where needed! 👍

modules/core/04-channel/keeper/keeper.go

modules/core/04-channel/types/expected_keepers.go

proto/ibc/core/channel/v1/upgrade.proto

charleenfei

so much easier to review 🚀

crodriguezvega

Thank you, @chatton. I have left a bunch of suggestions.

modules/core/03-connection/keeper/keeper.go

crodriguezvega · 2023-04-14T07:39:23Z

modules/core/04-channel/keeper/keeper.go

+		return errorsmod.Wrap(types.ErrInvalidChannelOrdering, "channel ordering must be a subset of the new ordering")
+	}
+
+	return k.connectionKeeper.CheckIsOpen(ctx, proposedUpgrade.ConnectionHops[0])


I agree with @damiannolan. I think also the CheckIsOpen function should anyway return a boolean (unless we rename it to CheckExistsAndIsOpen, since it is actually checking two things). In my opinion I would just do the explicit check here.

crodriguezvega · 2023-04-14T07:43:12Z

modules/core/04-channel/types/upgrade.go

+	}
+
+	if strings.TrimSpace(muf.Version) == "" {
+		return errorsmod.Wrap(ErrInvalidUpgrade, "proposed upgrade version cannot be empty")


Suggested change

return errorsmod.Wrap(ErrInvalidUpgrade, "proposed upgrade version cannot be empty")

return errorsmod.Wrap(ErrInvalidVersion, "version cannot be empty")

crodriguezvega · 2023-04-14T07:48:36Z

proto/ibc/core/channel/v1/upgrade.proto

+message Upgrade {
+  UpgradeFields  upgrade_fields   = 1 [(gogoproto.nullable) = false];
+  UpgradeTimeout timeout          = 2 [(gogoproto.nullable) = false];
+  uint64         last_packet_sent = 3;


nit: last implies that there will be no more, while latest is the most recent but there will be more.

Suggested change

uint64 last_packet_sent = 3;

uint64 latest_packet_sent = 3;

We could probably also make this work as next_sequence_send or without the offset prev_sequence_send / latest_sequence_send, which may be more conformant to the existing 04-channel primitives

I like latest_sequence_send

crodriguezvega · 2023-04-14T07:48:54Z

proto/ibc/core/channel/v1/upgrade.proto

+  uint64         last_packet_sent = 3;
+}
+
+// ModifiableUpgradeFields are the fields in a channel end which may be changed


Suggested change

// ModifiableUpgradeFields are the fields in a channel end which may be changed

// UpgradeFields are the fields in a channel end which may be changed

crodriguezvega · 2023-04-14T07:59:11Z

modules/core/04-channel/keeper/keeper_test.go

+			expPass:  false,
+		},
+		{
+			name: "attempt upgrade when connection is not set",


Suggested change

name: "attempt upgrade when connection is not set",

name: "fails when connection is not set",

crodriguezvega · 2023-04-14T07:59:19Z

modules/core/04-channel/keeper/keeper_test.go

+				path.EndpointA.SetConnection(connection)
+			},
+			expPass: false,
+		},


I guess we should add test cases for empty channel version, packet sent and the upgrade timeout.

I think these should be tested in a separate test that tests validate basic. This PR is already quite large, are you happy if we create a follow up PR to add tests for ValidateBasic of UpgradeFields and Upgrade?

crodriguezvega · 2023-04-14T07:59:39Z

modules/core/04-channel/keeper/keeper_test.go

+			expPass: false,
+		},
+		{
+			name: "attempt upgrade when connection is not open",


Suggested change

name: "attempt upgrade when connection is not open",

name: "fails when connection is not open",

crodriguezvega · 2023-04-14T08:03:51Z

modules/core/04-channel/types/upgrade.go

+}
+
+// ValidateBasic performs a basic validation of the proposed upgrade fields
+func (muf UpgradeFields) ValidateBasic() error {


Suggested change

func (muf UpgradeFields) ValidateBasic() error {

func (uf UpgradeFields) ValidateBasic() error {

crodriguezvega · 2023-04-14T08:04:37Z

modules/core/04-channel/keeper/keeper.go

+// - the proposed version is non-empty (checked in ModifiableUpgradeFields.ValidateBasic())
+// - the proposed connection hops are not open
+func (k Keeper) ValidateUpgradeFields(ctx sdk.Context, proposedUpgrade types.UpgradeFields, existingChannel types.Channel) error {
+	currentFields := types.UpgradeFields{


Should we have maybe a helper function that creates an UpgradeFields object from a channel? Something like func From(channel types.Channel): UpgradeFields

crodriguezvega · 2023-04-17T08:58:33Z

proto/ibc/core/channel/v1/upgrade.proto

+message Upgrade {
+  UpgradeFields  fields               = 1 [(gogoproto.nullable) = false];
+  UpgradeTimeout timeout              = 2 [(gogoproto.nullable) = false];
+  uint64         latest_sequence_send = 3;


Just trying to understand how the flushing of in-flight packets will work, so excuses if I am missing something.

If we have 2 chains (A and B) and A initiates the upgrade (i.e. executes MsgChannelUpgradeInit) this latest_sequence_send in the message can be used by chain A to know that it needs to block further sending of packets. But how can it be used by chain B to block sends? Doesn't chain B keep track of its own next_sequence_send that does not necessarily match the next_sequence_send on chain A?

chatton · 2023-04-17T11:36:15Z

e2e/go.mod

@@ -7,7 +7,7 @@ require (
 	github.com/cosmos/cosmos-sdk v0.47.0
 	github.com/cosmos/gogoproto v1.4.6
 	github.com/cosmos/ibc-go/v7 v7.0.0
-	github.com/cosmos/interchain-accounts v0.5.0


there was an error I'm not quite sure I can explain happening suddenly

On branch cian/issue#3445-add-verifiable-upgrade-type-and-validate-basic-functions downloaded: h1:b9P3j2ZPziqYUYzb7zHVucZg84EfiGY+T+fPgszVxfI= [135/1987] sum.golang.org: h1:0Pzv5xHq+TVY7ExRIsrzJ33+t22TUoJ1k8UgSxvv1X4= SECURITY ERROR This download does NOT match the one reported by the checksum server. The bits may have been replaced on the origin server, or an attacker may have intercepted the download attempt. For more information, see 'go help module-auth'. github.com/cosmos/ibc-go/e2e/testsuite imports github.com/strangelove-ventures/interchaintest/v7/chain/cosmos imports github.com/cosmos/cosmos-sdk/x/upgrade imports github.com/cosmos/cosmos-sdk/x/upgrade/client/cli imports github.com/cosmos/cosmos-sdk/x/upgrade/plan imports github.com/hashicorp/go-getter imports cloud.google.com/go/storage tested by cloud.google.com/go/storage.test imports cloud.google.com/go/httpreplay imports cloud.google.com/go/httpreplay/internal/proxy imports github.com/google/martian/v3/mitm: github.com/cosmos/interchain-accounts@v0.5.0: verifying go.mod: checksum mismatch downloaded: h1:b9P3j2ZPziqYUYzb7zHVucZg84EfiGY+T+fPgszVxfI= sum.golang.org: h1:0Pzv5xHq+TVY7ExRIsrzJ33+t22TUoJ1k8UgSxvv1X4= SECURITY ERROR This download does NOT match the one reported by the checksum server. The bits may have been replaced on the origin server, or an attacker may have intercepted the download attempt. For more information, see 'go help module-auth'. github.com/cosmos/ibc-go/e2e/testsuite imports github.com/strangelove-ventures/interchaintest/v7/chain/cosmos imports github.com/cosmos/cosmos-sdk/x/upgrade imports github.com/cosmos/cosmos-sdk/x/upgrade/client/cli imports github.com/cosmos/cosmos-sdk/x/upgrade/plan imports github.com/hashicorp/go-getter imports github.com/aws/aws-sdk-go/service/s3 imports github.com/aws/aws-sdk-go/aws/awsutil imports github.com/jmespath/go-jmespath tested by github.com/jmespath/go-jmespath.test imports github.com/jmespath/go-jmespath/internal/testify/assert: github.com/cosmos/interchain-accounts@v0.5.0: verifying go.mod: check sum mismatch downloaded: h1:b9P3j2ZPziqYUYzb7zHVucZg84EfiGY+T+fPgszVxfI= sum.golang.org: h1:0Pzv5xHq+TVY7ExRIsrzJ33+t22TUoJ1k8UgSxvv1X4= SECURITY ERROR This download does NOT match the one reported by the checksum server. The bits may have been replaced on the origin server, or an attacker may have intercepted the download attempt. For more information, see 'go help module-auth'.

It looks like some sort of mismatch in v0.5.0 in the one we've imported and the one that is imported in interchaintest.

This error became reproducible locally after I cleaned my local modcache

modules/core/04-channel/keeper/keeper.go

colin-axner

LGTM! See relevant comments. Thanks for the quick changes! :)

colin-axner · 2023-04-17T15:09:13Z

modules/core/04-channel/types/upgrade.go

+
+// ValidateBasic performs a basic validation of the proposed upgrade fields
+func (uf UpgradeFields) ValidateBasic() error {
+	if !collections.Contains(uf.Ordering, []Order{ORDERED, UNORDERED}) {


I wonder if there is a variable we can use for []Order{ORDERED, UNORDERED}? Such as SupportedOrderings? Can be done in followup

modules/core/04-channel/keeper/keeper.go

damiannolan · 2023-04-17T14:54:54Z

modules/core/04-channel/keeper/keeper.go

+// - the proposed connection hops do not exist
+// - the proposed version is non-empty (checked in UpgradeFields.ValidateBasic())
+// - the proposed connection hops are not open
+func (k Keeper) ValidateUpgradeFields(ctx sdk.Context, proposedUpgrade types.UpgradeFields, existingChannel types.Channel) error {


nit; wondering if existingChannel -> currentChannel would be better readability, maybe/maybe not

damiannolan · 2023-04-17T15:17:50Z

modules/core/04-channel/types/upgrade.go

+		return errorsmod.Wrap(ErrInvalidUpgrade, "upgrade timeout cannot be empty")
+	}
+
+	// TODO: determine if last packet sequence sent can be 0?


sounds good to me

modules/core/04-channel/keeper/keeper.go

damiannolan · 2023-04-18T08:29:01Z

proto/ibc/core/channel/v1/upgrade.proto

+// to allow the counterparty to block sends after the upgrade has started.
+message Upgrade {
+  UpgradeFields  fields               = 1 [(gogoproto.nullable) = false];
+  UpgradeTimeout timeout              = 2 [(gogoproto.nullable) = false];


When accessing the fields of timeout we use e.g. upgrade.Timeout.TimeoutHeight and upgrade.Timeout.TimeoutTimestamp.

We could considering dropping timeout_ prefix in the UpgradeTimeout type so that fields are upgrade.Timeout.Height and upgrade.Timeout.Timestamp.

This is out of scope of this PR tho, we can address this later if needs be.

damiannolan

LGTM, lets remove Upgrade.ValidateBasic() and update msgs to include UpgradeFields and UpgradeTimeout in follow ups.

colin-axner and others added 5 commits April 13, 2023 10:50

added validateProposedUpgradeFields function and CheckIsOpen helper f…

995c1f1

…unction

added unit test for CheckIsOpen

f8f6ad4

adding unit tests for ValidateProposedUpgradeFields

808ec70

fix linter

8dd0c44

update to use errorsmod instead of sdk errors

64878ae

chatton marked this pull request as ready for review April 13, 2023 11:05

chatton requested review from colin-axner, AdityaSripal and damiannolan as code owners April 13, 2023 11:05

DimitrisJim reviewed Apr 13, 2023

View reviewed changes

modules/core/04-channel/keeper/keeper_test.go Outdated Show resolved Hide resolved

DimitrisJim reviewed Apr 13, 2023

View reviewed changes

DimitrisJim approved these changes Apr 13, 2023

View reviewed changes

chatton and others added 2 commits April 13, 2023 14:33

Merge branch '04-channel-upgrades' into cian/issue#3445-add-verifiabl…

837e3c9

…e-upgrade-type-and-validate-basic-functions

re-arranged order of test functions

5f9ad67

damiannolan reviewed Apr 13, 2023

View reviewed changes

charleenfei approved these changes Apr 13, 2023

View reviewed changes

addressing PR feedback

04dd0af

crodriguezvega reviewed Apr 14, 2023

View reviewed changes

addressing PR feedback

d66fe9d

crodriguezvega reviewed Apr 17, 2023

View reviewed changes

chatton added 2 commits April 17, 2023 11:12

addressing PR feedback

18dbffe

bump interchain account demo to v0.5.1

a3da022

chatton commented Apr 17, 2023

View reviewed changes

chatton mentioned this pull request Apr 17, 2023

Add ValidateBasic tests for Channel Upgrade Types #3464

Closed

3 tasks

DimitrisJim mentioned this pull request Apr 17, 2023

Add helper for checking if a connection exists and is open #3466

Closed

3 tasks

remove CheckIsOpen

9b71740

damiannolan reviewed Apr 17, 2023

View reviewed changes

modules/core/04-channel/keeper/keeper.go Outdated Show resolved Hide resolved

updated godoc

7215e84

colin-axner reviewed Apr 17, 2023

View reviewed changes

damiannolan reviewed Apr 17, 2023

View reviewed changes

damiannolan mentioned this pull request Apr 18, 2023

Adapt channel order SubsetOf method #3469

Closed

3 tasks

damiannolan reviewed Apr 18, 2023

View reviewed changes

damiannolan approved these changes Apr 18, 2023

View reviewed changes

chatton merged commit 34e800c into 04-channel-upgrades Apr 18, 2023

chatton deleted the cian/issue#3445-add-verifiable-upgrade-type-and-validate-basic-functions branch April 18, 2023 08:35

chatton mentioned this pull request Apr 18, 2023

Refactor ChanUpgradeInit to use new upgrade type #3456

Merged

9 tasks

colin-axner mentioned this pull request May 29, 2023

Upgradability fixes cosmos/ibc#967

Merged

	return errorsmod.Wrap(ErrInvalidUpgrade, "proposed upgrade version cannot be empty")
	return errorsmod.Wrap(ErrInvalidVersion, "version cannot be empty")

	// ModifiableUpgradeFields are the fields in a channel end which may be changed
	// UpgradeFields are the fields in a channel end which may be changed

	name: "attempt upgrade when connection is not set",
	name: "fails when connection is not set",

	name: "attempt upgrade when connection is not open",
	name: "fails when connection is not open",

	func (muf UpgradeFields) ValidateBasic() error {
	func (uf UpgradeFields) ValidateBasic() error {

Add verifiable upgrade type and validate basic functions #3451

Add verifiable upgrade type and validate basic functions #3451

Conversation

chatton commented Apr 13, 2023 • edited Loading

Description

Commit Message / Changelog Entry

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

damiannolan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan Apr 13, 2023 • 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

Choose a reason for hiding this comment

charleenfei left a comment

Choose a reason for hiding this comment

crodriguezvega 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

damiannolan Apr 14, 2023 • 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

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

crodriguezvega Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

chatton Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

colin-axner left a comment • edited Loading

Choose a reason for hiding this comment

colin-axner Apr 17, 2023 • 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

damiannolan left a comment

Choose a reason for hiding this comment

chatton commented Apr 13, 2023 •

edited

Loading

damiannolan Apr 13, 2023 •

edited

Loading

damiannolan Apr 14, 2023 •

edited

Loading

crodriguezvega Apr 17, 2023 •

edited

Loading

chatton Apr 17, 2023 •

edited

Loading

colin-axner left a comment •

edited

Loading

colin-axner Apr 17, 2023 •

edited

Loading