Skip to content

ICS4: Channel Upgradability Spec #677

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AdityaSripal merged 13 commits into from
Mar 23, 2022
Merged

ICS4: Channel Upgradability Spec #677

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d5408ab
copy over connection upgrade spec and replace connection with channel
AdityaSripal Mar 7, 2022
c76e1f3
do core changes, TODO: app changes and callbacks
AdityaSripal Mar 7, 2022
7f5a03e
fix typos
AdityaSripal Mar 14, 2022
d6d3a67
call app upgrade callbacks
AdityaSripal Mar 14, 2022
1f7d1c9
fix restoreChannelUnless usage
AdityaSripal Mar 14, 2022
0d5574a
fix state conditional checks on TRY
AdityaSripal Mar 14, 2022
795a0a1
Apply suggestions from code review
AdityaSripal Mar 22, 2022
0a00e31
fix partial review of colin, left out crossing hello remarks
AdityaSripal Mar 22, 2022
55d64a1
Apply suggestions from code review
AdityaSripal Mar 23, 2022
b831b23
fix verify functions
AdityaSripal Mar 23, 2022
da970b1
fix merge
AdityaSripal Mar 23, 2022
d24fd7b
consistent msg names
AdityaSripal Mar 23, 2022
0c8e753
add explicit erroring
AdityaSripal Mar 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix restoreChannelUnless usage
  • Loading branch information
@AdityaSripal
AdityaSripal committed Mar 14, 2022
commit 1f7d1c90333847471576043bc4b4b64310e9a546
147 changes: 88 additions & 59 deletions spec/core/ics-004-channel-and-packet-semantics/UPGRADES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,34 @@ function verifyUpgradeTimeout(

The Channel Upgrade process consists of three sub-protocols: `UpgradeChannelHandshake`, `CancelChannelUpgrade`, and `TimeoutChannelUpgrade`. In the case where both chains approve of the proposed upgrade, the upgrade handshake protocol should complete successfully and the ChannelEnd should upgrade successfully.

### Utility Functions

`restoreConnectionUnless()` is a utility function that allows a chain to abort an upgrade handshake in progress, and return the `channelEnd` to its original pre-upgrade state while also setting the `errorReceipt`. A relayer can then send a `cancelUpgradeMsg` to the counterparty so that it can restore its `channelEnd` to its pre-upgrade state as well. Once both channel ends are back to the pre-upgrade state, packet processing will resume with the original channel and application parameters.

```typescript
function restoreChannel() {
// cancel upgrade
// write an error receipt into the error path
// and restore original channel
errorReceipt = []byte{1}
provableStore.set(errorPath(portIdentifier, channelIdentifier), errorReceipt)
originalChannel = privateStore.get(restorePath(portIdentifier, channelIdentifier))
provableStore.set(channelPath(portIdentifier, channelIdentifier), originalChannel)
provableStore.delete(timeoutPath(portIdentifier, channelIdentifier))
privateStore.delete(restorePath(portIdentifier, channelIdentifier))

// call modules onChanUpgradeRestore callback
module = lookupModule(portIdentifier)
// restore callback must not return error and it must successfully restore
// application to its pre-upgrade state
module.onChanUpgradeRestore(
portIdentifier,
channelIdentifier
)
// caller should return as well
}
```

### Upgrade Handshake

The upgrade handshake defines four datagrams: *ChanUpgradeInit*, *ChanUpgradeTry*, *ChanUpgradeAck*, and *ChanUpgradeConfirm*
Expand Down Expand Up @@ -286,19 +314,6 @@ function chanUpgradeTry(
currentChannel = provableStore.get(channelPath(portIdentifier, channelIdentifier))
abortTransactionUnless(currentChannel.state == OPEN || currentChannel.state == UPGRADE_INIT)

if currentChannel.state == UPGRADE_INIT {
// if there is a crossing hello, ie an UpgradeInit has been called on both channelEnds,
// then we must ensure that the proposedUpgrade by the counterparty is the same as the currentChannel
// except for the channel state (upgrade channel will be in UPGRADE_TRY and current channel will be in UPGRADE_INIT)
// if the proposed upgrades on either side are incompatible, then we will restore the channel and cancel the upgrade.
currentChannel.state = UPGRADE_TRY
restoreChannelUnless(currentChannel.IsEqual(proposedUpgradeChannel)
} else {
// this is first message in upgrade handshake on this chain so we must store original channel in restore path
// in case we need to restore channel later.
privateStore.set(restorePath(portIdentifier, channelIdentifier), currentChannel)
}

// abort transaction if an unmodifiable field is modified
// upgraded channel state must be in `UPGRADE_TRY`
// NOTE: Any added fields are by default modifiable.
Expand All @@ -314,27 +329,56 @@ function chanUpgradeTry(
abortTransactionUnless(
currentChannel.ordering.subsetOf(proposedUpgradeChannel.ordering)
)

// verify proofs of counterparty state
abortTransactionUnless(verifyChannelState(currentChannel, proofHeight, proofChannel, currentChannel.counterpartyChannelIdentifier, counterpartyChannel))
abortTransactionUnless(verifyUpgradeTimeout(currentChannel, proofHeight, proofUpgradeTimeout, currentChannel.counterpartyChannelIdentifier, upgradeTimeout))

if currentChannel.state == UPGRADE_INIT {
// if there is a crossing hello, ie an UpgradeInit has been called on both channelEnds,
// then we must ensure that the proposedUpgrade by the counterparty is the same as the currentChannel
// except for the channel state (upgrade channel will be in UPGRADE_TRY and current channel will be in UPGRADE_INIT)
// if the proposed upgrades on either side are incompatible, then we will restore the channel and cancel the upgrade.
Comment on lines +329 to +332
Copy link
Copy Markdown
Contributor

@colin-axner colin-axner Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find timeout handling in the face of crossing hellos to be subtle. My understanding is that so long as one chain reaches TRY, the handshake can finish regardless of if one of the timeouts set has passed. That is:

chainA, UPGRADE_INIT, timeout 5
chainB, UPGRADE_INIT, timeout 10

at time 7, both chains attempt to go to TRY. chainA succeeds and chainB fails. So we have:
chainA, UPGRADE_TRY
chainB, UPGRADE_INIT

chainB should be able to go to open without a relayer timing it out (which I believe it does)

Copy link
Copy Markdown
Contributor Author

@AdityaSripal AdityaSripal Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed this is a complicated case, and we could design it to handle this such that chainB goes to OPEN. But I think that is quite difficult to engineer and evaluate for correctness. It is better for the first implementation to just revert the channel in this case. This is an inconvenience when we reach this edge case, since we have to restart the handshake but I believe that is fine.

So we will start with:

chainA, UPGRADE_INIT, timeout 5
chainB, UPGRADE_INIT, timeout 10

at time 7, both chains attempt to go to TRY. chainA succeeds and chainB fails. So we have:

chainA, UPGRADE_TRY
chainB, OPEN (reverted and error receipt written. See pseudocode below where we restore channel if timeout has passed)

Then at a time < 10 relayer can send a CancelUpgradeMsg to chainA:

chainA, OPEN (reverted)
chainB, OPEN (reverted and error receipt written)

currentChannel.state = UPGRADE_TRY
if !currentChannel.IsEqual(proposedUpgradeChannel) {
restoreChannel()
return
}
} else {
// this is first message in upgrade handshake on this chain so we must store original channel in restore path
// in case we need to restore channel later.
privateStore.set(restorePath(portIdentifier, channelIdentifier), currentChannel)
}



// either timeout height or timestamp must be non-zero
// if the upgrade feature is implemented on the TRY chain, then a relayer may submit a TRY transaction after the timeout.
// this will restore the channel on the executing chain and allow counterparty to use the CancelUpgradeMsg to restore their channel.
restoreChannelUnless(timeoutHeight != 0 || timeoutTimestamp != 0)
if timeoutHeight == 0 && timeoutTimestamp == 0 {
restoreChannel()
return
}
upgradeTimeout = UpgradeTimeout{
timeoutHeight: timeoutHeight,
timeoutTimestamp: timeoutTimestamp,
}

// counterparty-specified timeout must not have exceeded
restoreChannelUnless(
timeoutHeight < currentHeight() &&
timeoutTimestamp < currentTimestamp()
)
if (currentHeight() > timeoutHeight && timeoutHeight != 0) ||
(currentTimestamp() > timeoutTimestamp && timeoutTimestamp != 0) {
restoreChannel()
return
}

// both channel ends must be mutually compatible.
// this function has been left unspecified since it will depend on the specific structure of the new channel.
// It is the responsibility of implementations to make sure that verification that the proposed new channels
// on either side are correctly constructed according to the new version selected.
restoreChannelUnless(IsCompatible(counterpartyChannel, proposedUpgrade.Channel))
if !IsCompatible(counterpartyChannel, proposedUpgradeChannel) {
restoreChannel()
return
}

Comment on lines +367 to +371
Copy link
Copy Markdown
Contributor

@colin-axner colin-axner Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like leftover code from crossing hellos case handled above?

Copy link
Copy Markdown
Contributor

@mpoke mpoke Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. counterpartyChannel is not defined.

Copy link
Copy Markdown
Contributor Author

@AdityaSripal AdityaSripal Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry my mistake. Needed to define and prove counterpartyChannel earlier. counterpartyChannel is the channel that initiator upgraded to.

proposedUpgradeChannel is the channel that TRY is going to upgrade to.

Similar to the connection upgrade channel there needs to be some check on compatibility that will not be specified here since it is upgrade-specific.

// call modules onChanUpgradeTry callback
module = lookupModule(portIdentifier)
Expand All @@ -348,15 +392,14 @@ function chanUpgradeTry(
proposedUpgradeChannel.version
)
// restore channel if callback returned error
restoreChannelUnless(err != nil)
if err != nil {
restoreChannel()
return
}

// replace channel version with the version returned by application
// in case it was modified
proposedUpgradeChannel.version = version

// verify proofs of counterparty state
abortTransactionUnless(verifyChannelState(currentChannel, proofHeight, proofChannel, currentChannel.counterpartyChannelIdentifier, counterpartyChannel))
abortTransactionUnless(verifyUpgradeTimeout(currentChannel, proofHeight, proofUpgradeTimeout, currentChannel.counterpartyChannelIdentifier, upgradeTimeout))

provableStore.set(channelPath(portIdentifier, channelIdentifier), proposedUpgradeChannel)
}
Expand All @@ -377,15 +420,24 @@ function chanUpgradeAck(
currentChannel = provableStore.get(channelPath(portIdentifier, channelIdentifier))
abortTransactionUnless(currentChannel.state == UPGRADE_INIT || currentChannel.state == UPGRADE_TRY)

// verify proofs of counterparty state
abortTransactionUnless(verifyChannelState(currentChannel, proofHeight, proofChannel, currentChannel.counterpartyChannelIdentifier, counterpartyChannel))

// counterparty must be in TRY state
restoreChannelUnless(counterpartyChannel.State == UPGRADE_TRY)
if counterpartyChannel.State != UPGRADE_TRY {
restoreChannel()
return
}

// verify channels are mutually compatible
// this will also check counterparty chosen version is valid
// this function has been left unspecified since it will depend on the specific structure of the new channel.
// It is the responsibility of implementations to make sure that verification that the proposed new channels
// on either side are correctly constructed according to the new version selected.
restoreChannelUnless(IsCompatible(counterpartyChannel, channel))
if !IsCompatible(counterpartyChannel, channel) {
restoreChannel()
return
}

// call modules onChanUpgradeAck callback
module = lookupModule(portIdentifier)
Expand All @@ -396,10 +448,10 @@ function chanUpgradeAck(
counterpartyChannel.version
)
// restore channel if callback returned error
restoreChannelUnless(err != nil)

// verify proofs of counterparty state
abortTransactionUnless(verifyChannelState(currentChannel, proofHeight, proofChannel, currentChannel.counterpartyChannelIdentifier, counterpartyChannel))
if err != nil {
restoreChannel()
return
}

// upgrade is complete
// set channel to OPEN and remove unnecessary state
Expand Down Expand Up @@ -445,34 +497,6 @@ function chanUpgradeConfirm(
}
```

```typescript
function restoreChannelUnless(condition: bool) {
if !condition {
// cancel upgrade
// write an error receipt into the error path
// and restore original channel
errorReceipt = []byte{1}
provableStore.set(errorPath(portIdentifier, channelIdentifier), errorReceipt)
originalChannel = privateStore.get(restorePath(portIdentifier, channelIdentifier))
provableStore.set(channelPath(portIdentifier, channelIdentifier), originalChannel)
provableStore.delete(timeoutPath(portIdentifier, channelIdentifier))
privateStore.delete(restorePath(portIdentifier, channelIdentifier))

// call modules onChanUpgradeRestore callback
module = lookupModule(portIdentifier)
// restore callback must not return error and it must successfully restore
// application to its pre-upgrade state
module.onChanUpgradeRestore(
portIdentifier,
channelIdentifier
)

// caller should return as well
} else {
// caller should continue execution
}
}
```

### Cancel Upgrade Process

Expand Down Expand Up @@ -541,7 +565,12 @@ function timeoutChannelUpgrade(
abortTransactionUnless(channel.client.verifyChannelState(proofHeight, proofChannel, counterpartyChannel))

// we must restore the channel since the timeout verification has passed
restoreChannelUnless(false)
originalChannel = privateStore.get(restorePath(portIdentifier, channelIdentifier))
provableStore.set(channelPath(portIdentifier, channelIdentifier), originalChannel)

// delete auxilliary upgrade state
provableStore.delete(timeoutPath(portIdentifier, channelIdentifier))
privateStore.delete(restorePath(portIdentifier, channelIdentifier))
}
```

Expand Down