Skip to content
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.

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

Upgrade Refactors #999

Merged
merged 21 commits into from
Jul 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
57 changes: 39 additions & 18 deletions spec/core/ics-004-channel-and-packet-semantics/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ interface ChannelEnd {
connectionHops: [Identifier]
version: string
upgradeSequence: uint64
flushStatus: FlushStatus
}
```

Expand All @@ -92,7 +91,7 @@ interface ChannelEnd {
- The `connectionHops` stores the list of connection identifiers, in order, along which packets sent on this channel will travel. At the moment this list must be of length 1. In the future multi-hop channels may be supported.
- The `version` string stores an opaque channel version, which is agreed upon during the handshake. This can determine module-level configuration such as which packet encoding is used for the channel. This version is not used by the core IBC protocol. If the version string contains structured metadata for the application to parse and interpret, then it is considered best practice to encode all metadata in a JSON struct and include the marshalled string in the version field.

See the [upgrade spec](./UPGRADES.md) for details on `upgradeSequence` and `flushStatus`.
See the [upgrade spec](./UPGRADES.md) for details on `upgradeSequence`.

Channel ends have a *state*:

Expand All @@ -102,6 +101,8 @@ enum ChannelState {
TRYOPEN,
OPEN,
CLOSED,
FLUSHING,
FLUSHINGCOMPLETE,
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
}
```

Expand All @@ -110,6 +111,9 @@ enum ChannelState {
- A channel end in `OPEN` state has completed the handshake and is ready to send and receive packets.
- A channel end in `CLOSED` state has been closed and can no longer be used to send or receive packets.

See the [upgrade spec](./UPGRADES.md) for details on `FLUSHING` and `FLUSHCOMPLETE`.


A `Packet`, in the interblockchain communication protocol, is a particular interface defined as follows:

```typescript
Expand Down Expand Up @@ -491,7 +495,6 @@ function chanCloseInit(
abortTransactionUnless(connection !== null)
abortTransactionUnless(connection.state === OPEN)
channel.state = CLOSED
channel.flushStatus = NOTINFLUSH
provableStore.set(channelPath(portIdentifier, channelIdentifier), channel)
}
```
Expand Down Expand Up @@ -532,7 +535,6 @@ function chanCloseConfirm(
expected
))
channel.state = CLOSED
channel.flushStatus = NOTINFLUSH
provableStore.set(channelPath(portIdentifier, channelIdentifier), channel)
}
```
Expand Down Expand Up @@ -593,9 +595,9 @@ function sendPacket(
data: bytes): uint64 {
channel = provableStore.get(channelPath(sourcePort, sourceChannel))

// check that the channel is not closed to send packets;
// check that the channel must be OPEN to send packets;
abortTransactionUnless(channel !== null)
abortTransactionUnless(channel.state !== CLOSED && channel.flushStatus === NOTINFLUSH)
abortTransactionUnless(channel.state === OPEN)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is disabling optimistic sends. Is this intentional? Maybe it should be in a separate pr? The previous behaviour before channel upgradability changes was channel.state !== CLOSED

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, its necessary now but wasn't done before. I suppose i could do it in a separate pr and then merge conflict

Copy link
Contributor

Choose a reason for hiding this comment

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

up to you

connection = provableStore.get(connectionPath(channel.connectionHops[0]))
abortTransactionUnless(connection !== null)

Expand Down Expand Up @@ -660,7 +662,7 @@ function recvPacket(
channel = provableStore.get(channelPath(packet.destPort, packet.destChannel))
abortTransactionUnless(channel !== null)
counterpartyLastPacketSent = privateStore.get(channelCounterpartyLastPacketSequencePath(packet.destPort, packet.destChannel)
abortTransactionUnless(channel.state === OPEN || (channel.flushStatus === FLUSHING && packet.sequence <= counterpartyLasPacketSent))
abortTransactionUnless(channel.state === OPEN || (channel.state === FLUSHING && packet.sequence <= counterpartyLastPacketSent))
abortTransactionUnless(authenticateCapability(channelCapabilityPath(packet.destPort, packet.destChannel), capability))
abortTransactionUnless(packet.sourcePort === channel.counterpartyPortIdentifier)
abortTransactionUnless(packet.sourceChannel === channel.counterpartyChannelIdentifier)
Expand Down Expand Up @@ -850,7 +852,7 @@ function acknowledgePacket(
// abort transaction unless that channel is open, calling module owns the associated port, and the packet fields match
channel = provableStore.get(channelPath(packet.sourcePort, packet.sourceChannel))
abortTransactionUnless(channel !== null)
abortTransactionUnless(channel.state === OPEN || channel.flushStatus === FLUSHING)
abortTransactionUnless(channel.state === OPEN || channel.state === FLUSHING)
abortTransactionUnless(authenticateCapability(channelCapabilityPath(packet.sourcePort, packet.sourceChannel), capability))
abortTransactionUnless(packet.destPort === channel.counterpartyPortIdentifier)
abortTransactionUnless(packet.destChannel === channel.counterpartyChannelIdentifier)
Expand Down Expand Up @@ -886,10 +888,20 @@ function acknowledgePacket(
// delete our commitment so we can't "acknowledge" again
provableStore.delete(packetCommitmentPath(packet.sourcePort, packet.sourceChannel, packet.sequence))

// if there are no in-flight packets on our end, we can automatically go to FLUSHCOMPLETE
if channel.flushStatus === FLUSHING && pendingInflightPackets(packet.sourcePort, packet.sourceChannel) == nil {
channel.flushStatus = FLUSHCOMPLETE
provableStore.set(channelPath(packet.sourcePort, packet.sourceChannel), channel)
if channel.state == FLUSHING {
upgradeTimeout = privateStore.get(counterpartyUpgradeTimeout(portIdentifier, channelIdentifier))
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
if upgradeTimeout != nil {
// counterparty-specified timeout must not have exceeded
// if it has, then restore the channel and abort upgrade handshake
if (upgradeTimeout.timeoutHeight != 0 && currentHeight() >= upgradeTimeout.timeoutHeight) ||
(upgradeTimeout.timeoutTimestamp != 0 && currentTimestamp() >= upgradeTimeout.timeoutTimestamp ) {
restoreChannel(portIdentifier, channelIdentifier)
} else if pendingInflightPackets(portIdentifier, channelIdentifier) == nil {
// if this was the last in-flight packet, then move channel state to FLUSHCOMPLETE
channel.state = FLUSHCOMPLETE
publicStore.set(channelPath(portIdentifier, channelIdentifier), channel)
}
}
}

// return transparent packet
Expand Down Expand Up @@ -1026,17 +1038,26 @@ function timeoutPacket(
// delete our commitment
provableStore.delete(packetCommitmentPath(packet.sourcePort, packet.sourceChannel, packet.sequence))

// if there are no in-flight packets on our end, we can automatically go to FLUSHCOMPLETE
if channel.flushStatus === FLUSHING && pendingInflightPackets(packet.sourcePort, packet.sourceChannel) == nil {
channel.flushStatus = FLUSHCOMPLETE
provableStore.set(channelPath(packet.sourcePort, packet.sourceChannel), channel)
if channel.state == FLUSHING {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
upgradeTimeout = privateStore.get(counterpartyUpgradeTimeout(portIdentifier, channelIdentifier))
if upgradeTimeout != nil {
// counterparty-specified timeout must not have exceeded
// if it has, then restore the channel and abort upgrade handshake
if (upgradeTimeout.timeoutHeight != 0 && currentHeight() >= upgradeTimeout.timeoutHeight) ||
(upgradeTimeout.timeoutTimestamp != 0 && currentTimestamp() >= upgradeTimeout.timeoutTimestamp ) {
restoreChannel(portIdentifier, channelIdentifier)
} else if pendingInflightPackets(portIdentifier, channelIdentifier) == nil {
// if this was the last in-flight packet, then move channel state to FLUSHCOMPLETE
channel.state = FLUSHCOMPLETE
publicStore.set(channelPath(portIdentifier, channelIdentifier), channel)
}
}
}

// only close on strictly ORDERED channels
if channel.order === ORDERED {
// ordered channel: close the channel and reset flushStatus
// ordered channel: close the channel
channel.state = CLOSED
channel.flushStatus = NOTINFLUSH
provableStore.set(channelPath(packet.sourcePort, packet.sourceChannel), channel)
}
// on ORDERED_ALLOW_TIMEOUT, increment NextSequenceAck so that next packet can be acknowledged after this packet timed out.
Expand Down
Loading