ICS3: Add Connection Upgrade Spec by AdityaSripal · Pull Request #621 · cosmos/ibc

AdityaSripal · 2021-12-01T15:48:25Z

This is a rough draft proposal for the ability to upgrade a connection once it has already been established

AdityaSripal · 2021-12-01T15:54:25Z

Some notes:

Current idea is that upgrading a connection will require bilateral agreement. i.e. Both sides must explicitly allow the initial parts of handshake (INIT and TRY). Ack and Confirm can be completed by trustless relayer.
There is currently a timeout specified on INIT and TRY, so that the connection does not hang indefinitely in a closed state. Not fully specified yet.
The details of how to handle crossing hellos is not fully specified yet, so ignore that possibility on initial review.

colin-axner

Excellent!! Concept makes sense to me. Architecturally not difficult to introduce into existing ibc implementations

spec/core/ics-003-connection-semantics/UPGRADES.md

colin-axner · 2021-12-03T11:26:02Z

spec/core/ics-003-connection-semantics/UPGRADES.md

+    }
+
+    provableStore.set(statusPath(identifier), upgradeStatus)
+    provableStore.set(connectionPath(identifier), proposedUpgrade.connection)


Interesting, I'd think you'd want to leave the connection unchanged until the upgrade process successfully completes. Otherwise you could potentially disrupt packet flow if the connection is in UPGRADE_INIT instead of OPEN?

I'd expect you to store the proposed upgrade connection under a proposed upgrade connection path. Use that for proofs and then if successful update the connection on ack/confirm?

I don't want the case where chainA has its connectionEnd OPEN with the old parameters, and chainB has its connectionEnd OPEN with the new parameters as that may cause unexpected consequences.

If a connection is OPEN on both ends it should be a valid connection always.

So either both ends are OPEN running the old parameters, or both ends are OPEN running the new parameters.

The only way to accomplish this is to temporarily close the connection on both sides (INIT and TRY), then reopen after the upgrade is complete on either end (ACK and CONFIRM).

This does mean packet flow is temporarily halted but i believe this is fine. Once the connection is back up, the relayers can either receive or timeout the pending packets

This is also why I specify a timeout, because I don't want the "temporary pause" to last arbitrarily long

yeah, this makes sense to me

cwgoes

Concept ACK; most changes ACK (modulo open question of crossing hellos); see a few minor comments.

I will also note that I would like to see re-verification of IBC safety / liveness with this logic incorporated.

spec/core/ics-003-connection-semantics/UPGRADES.md

cwgoes · 2021-12-06T08:12:14Z

spec/core/ics-003-connection-semantics/UPGRADES.md

+    }
+
+    provableStore.set(statusPath(identifier), upgradeStatus)
+    provableStore.set(connectionPath(identifier), proposedUpgrade.connection)


yeah, this makes sense to me

spec/core/ics-003-connection-semantics/UPGRADES.md

Co-authored-by: Christopher Goes <cwgoes@pluranimity.org>

…ya/path-upgrade

mpoke

Concept ACK. I agree with Christopher that we need to make sure that the IBC safety and liveness are preserved with this logic incorporated. See comments below.

spec/core/ics-003-connection-semantics/UPGRADES.md

Co-authored-by: Marius Poke <marius.poke@posteo.de>

mpoke · 2022-03-08T18:33:00Z

spec/core/ics-003-connection-semantics/UPGRADES.md

+
+```typescript
+// Client VerifyUpgradeError
+function verifyConnectionUpgradeError(


Suggested change

function verifyConnectionUpgradeError(

function VerifyUpgradeError(

I've added Connection here to the name because there are identical functions for channel upgradability.

Yeah, but the comment above is // Client VerifyUpgradeError and in the above function, i.e.,

// Connection VerifyConnectionUpgradeError method function verifyConnectionUpgradeError(

you call client.verifyUpgradeError

mpoke · 2022-03-08T18:40:15Z

spec/core/ics-003-connection-semantics/UPGRADES.md

+
+```typescript
+// Client VerifyUpgradeTimeout
+function verifyConnectionUpgradeTimeout(


Suggested change

function verifyConnectionUpgradeTimeout(

function VerifyUpgradeTimeout(

spec/core/ics-003-connection-semantics/UPGRADES.md

mpoke · 2022-03-08T21:14:08Z

spec/core/ics-003-connection-semantics/UPGRADES.md

+```typescript
+// Client VerifyUpgradeError
+function verifyConnectionUpgradeError(
+    clientState: ClientState,


clientState is not passed when calling client.verifyUpgradeError()

This is the same pattern used in ICS3 and ICS2. I believe it is the standard for Typescript.

https://github.com/cosmos/ibc/blob/master/spec/core/ics-003-connection-semantics/README.md#helper-functions

https://github.com/cosmos/ibc/blob/master/spec/core/ics-002-client-semantics/README.md#required-functions

Yeah, I've seen that. I find it very confusing. Until now I thought it's an inconsistency. Still not convinced that it's not :)

mpoke · 2022-03-08T21:14:56Z

spec/core/ics-003-connection-semantics/UPGRADES.md

+```typescript
+// Client VerifyUpgradeTimeout
+function verifyConnectionUpgradeTimeout(
+    clientState: ClientState,


clientState is not passed when calling client.verifyUpgradeTimeout()

mpoke · 2022-03-08T21:17:43Z

spec/core/ics-003-connection-semantics/UPGRADES.md

+    path = applyPrefix(prefix, timeoutPath(counterpartyConnectionIdentifier))
+    abortTransactionUnless(!clientState.frozen)
+    timeoutBytes = protobuf.marshal(upgradeTimeout)
+    return clientState.verifiedRoots[height].verifyMembership(path, timeoutBytes, proof)


This seems very Tendermint specific, i.e., https://github.com/cosmos/ibc/blob/master/spec/client/ics-007-tendermint-client/README.md#state-verification-functions

This does not assume anything about Tendermint, but just says we will verify membership against a consensus state at provided height. This is taken from ICS2.

https://github.com/cosmos/ibc/blob/master/spec/core/ics-002-client-semantics/README.md#example-implementation

spec/core/ics-003-connection-semantics/UPGRADES.md

mpoke

Thanks for the changes. The protocol seems correct. I left some comments, most of them minor.

spec/core/ics-003-connection-semantics/UPGRADES.md

Co-authored-by: Marius Poke <marius.poke@posteo.de>

…ya/path-upgrade

colin-axner

Only reviewed up to the Sub-Protocols section. LGTM so far

spec/core/ics-003-connection-semantics/UPGRADES.md

colin-axner · 2022-03-17T15:54:45Z

spec/core/ics-003-connection-semantics/UPGRADES.md

+    if counterpartyConnection.State != UPGRADE_TRY {
+        restoreConnection()
+        return
+    }


we have to be careful to not use relayer provided information unless verified (especially if we are restoring/cancelling an upgrade)

mpoke

Looks great. I have just one concern regarding the transition from UPGRADE_TRY to OPEN (i.e., connUpgradeConfirm). The rest of the comments are just nits.

spec/core/ics-003-connection-semantics/UPGRADES.md

mpoke · 2022-03-21T15:53:47Z

spec/core/ics-003-connection-semantics/UPGRADES.md

+}
+```
+
+The UpgradeError MUST have an associated verification function added to the connection and client interfaces so that a counterparty may verify that chain has stored an error in the UpgradeError path.


A verification function is no longer needed for the client interface.

mpoke · 2022-03-21T15:56:42Z

spec/core/ics-003-connection-semantics/UPGRADES.md

+}
+```
+
+The timeout path MUST have associated verification methods on the connection and client interfaces in order for a counterparty to prove that a chain stored a particular `UpgradeTimeout`.


mpoke · 2022-03-21T15:58:27Z

spec/core/ics-003-connection-semantics/UPGRADES.md

+    // construct CommitmentPath
+    path = applyPrefix(connection.counterpartyPrefix, connectionTimeoutPath(connection.counterpartyConnectionIdentifier))
+    // marshal upgradeTimeout into bytes with standardized protobuf codec
+    timeoutBytes = protobuf.marshal(upgradeTimeout)


I find this a detail of implementation. Not sure it's needed in the spec.

No because both sides must agree on an encoding, otherwise they won't be able to verify correctly. So it must be part of spec. I think this is a downside in the rest of spec, that it doesn't explicitly define the encoding you need to use to speak with other IBC chains

spec/core/ics-003-connection-semantics/UPGRADES.md

mpoke · 2022-03-21T16:14:02Z

spec/core/ics-003-connection-semantics/UPGRADES.md

+        restoreConnection()
+        return
+    }
+    upgradeTimeout = UpgradeTimeout{


upgradeTimeout is not used.

Ahh oops, it gets used above in proof verification. Will reorder

mpoke · 2022-03-21T16:22:49Z

spec/core/ics-003-connection-semantics/UPGRADES.md

+
+    // verify proofs of counterparty state
+    abortTransactionUnless(verifyConnectionState(currentConnection, proofHeight, proofConnection, currentConnection.counterpartyConnectionIdentifier, counterpartyConnection))
+


Before changing the local connection state to OPEN, shouldn't we check again that the remote connection is compatible with the local one? During connUpgradeAck, the remote side could restore the connection, which means moving to a connection with state OPEN. Just verifying that the remote side is OPEN may not be enough.

connUpgradeConfirm should also contain a proof that UpgradeError was not set by the remote side.

Ahh thank you, nice catch

spec/core/ics-003-connection-semantics/UPGRADES.md

Co-authored-by: Marius Poke <marius.poke@posteo.de>

AdityaSripal added 2 commits November 30, 2021 19:24

connection upgrade progress

e0fa2e1

initial writeup

cb0b5c5

AdityaSripal requested review from cezarad and cwgoes as code owners December 1, 2021 15:48

add notes on init and try access control

067956e

AdityaSripal requested a review from colin-axner December 1, 2021 15:54

colin-axner reviewed Dec 3, 2021

View reviewed changes

address colin suggestions

dc00b01

cwgoes suggested changes Dec 6, 2021

View reviewed changes

Apply suggestions from code review

82f7286

Co-authored-by: Christopher Goes <cwgoes@pluranimity.org>

AdityaSripal marked this pull request as draft December 6, 2021 10:06

AdityaSripal added 2 commits December 6, 2021 11:06

address chris initial review

26169bd

Merge branch 'aditya/path-upgrade' of github.com:cosmos/ibc into adit…

b7ab84d

…ya/path-upgrade

colin-axner mentioned this pull request Dec 15, 2021

ICS27: Controller PortID Format should not contain version #626

Closed

AdityaSripal requested a review from ethanfrey January 14, 2022 15:17

AdityaSripal added 3 commits February 10, 2022 12:43

add timeout logic

d71f150

crossing hellos logic

e398dbb

migrations and crossing hello cleanup

b92cced

AdityaSripal marked this pull request as ready for review February 11, 2022 14:54

AdityaSripal requested a review from mpoke as a code owner February 11, 2022 14:54

AdityaSripal requested review from colin-axner and cwgoes February 11, 2022 14:55

mpoke suggested changes Feb 21, 2022

View reviewed changes

AdityaSripal and others added 6 commits February 22, 2022 18:22

Apply suggestions from code review

e7ff7bf

Co-authored-by: Marius Poke <marius.poke@posteo.de>

continue addressing @mpoke suggestions

bd795ae

clarify handshake pseudocode

da255ad

remove unnecessary structs and simplify protocol

d5588c8

clarify TRY actor reasoning

3d30b28

TRY timeout clarification

37916cd

cleanup pseudocode

f483d1f

AdityaSripal requested a review from mpoke March 2, 2022 16:59

AdityaSripal added 2 commits March 7, 2022 18:32

fix mistakes after readthrough

7239327

chan->conn

6f7f572

AdityaSripal changed the title ~~Add Connection Upgrade Spec~~ ICS3: Add Connection Upgrade Spec Mar 7, 2022

mpoke reviewed Mar 8, 2022

View reviewed changes

mpoke suggested changes Mar 8, 2022

View reviewed changes

AdityaSripal and others added 4 commits March 9, 2022 15:45

Apply suggestions from code review

758e5ab

Co-authored-by: Marius Poke <marius.poke@posteo.de>

complete fixing minor issues

67f2a03

Merge branch 'aditya/path-upgrade' of github.com:cosmos/ibc into adit…

c40ab29

…ya/path-upgrade

reorder utility function and fix timeout conditional bug

a152627

AdityaSripal requested a review from mpoke March 9, 2022 15:09

colin-axner reviewed Mar 9, 2022

View reviewed changes

spec/core/ics-003-connection-semantics/UPGRADES.md Outdated Show resolved Hide resolved

spec/core/ics-003-connection-semantics/UPGRADES.md Show resolved Hide resolved

spec/core/ics-003-connection-semantics/UPGRADES.md Show resolved Hide resolved

colin-axner reviewed Mar 10, 2022

View reviewed changes

AdityaSripal mentioned this pull request Mar 10, 2022

ICS2: Replace specific VerifyFunctions with generic VerifyMembership and VerifyNonMembership #684

Closed

AdityaSripal added 3 commits March 14, 2022 19:01

fix restore connection logic

6a59321

use 02-client generic verify methods

a4c9fdc

address colin comments

7359894

AdityaSripal requested a review from colin-axner March 15, 2022 17:06

colin-axner reviewed Mar 17, 2022

View reviewed changes

mpoke suggested changes Mar 21, 2022

View reviewed changes

AdityaSripal and others added 2 commits March 22, 2022 11:58

Apply suggestions from code review

849059a

Co-authored-by: Marius Poke <marius.poke@posteo.de>

address rest of reviews

c261dd3

mpoke approved these changes Mar 22, 2022

View reviewed changes

use connection method

1948a82

AdityaSripal merged commit d275905 into master Mar 22, 2022

AdityaSripal deleted the aditya/path-upgrade branch March 22, 2022 14:33

AdityaSripal mentioned this pull request May 2, 2022

ICS004 Channel Upgrades - In flight packets #725

Closed

	function verifyConnectionUpgradeError(
	function VerifyUpgradeError(

	function verifyConnectionUpgradeTimeout(
	function VerifyUpgradeTimeout(


		// verify proofs of counterparty state
		abortTransactionUnless(verifyConnectionState(currentConnection, proofHeight, proofConnection, currentConnection.counterpartyConnectionIdentifier, counterpartyConnection))

Conversation

AdityaSripal commented Dec 1, 2021

Uh oh!

AdityaSripal commented Dec 1, 2021

Uh oh!

colin-axner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwgoes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mpoke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpoke left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cwgoes left a comment •

edited

Loading

mpoke left a comment •

edited

Loading