cosmos / cosmos/ibc

ICS04: some questions about function timeoutOnClose and timeoutPacket

Open
#968 7 comments 0 reactions 0 assignees View on GitHub
question tao
Dominant language
Go
Stars
1k
Forks
455
Avg merge
4d 5h
Merged PRs (30d)
29

Description

Question 1

As ICS03, the connection can't be closed.
>Once opened, connections cannot be closed and identifiers cannot be reallocated

But as ICS04, there are two comments in ```timeoutPacket``` and ```timeoutOnClose```.
>note: the connection may have been closed

It may be a mistake.

Question 2

I found a logical problem, but I'm not sure if it works in reality. (Some mistakes are possible but may not practical.)

As ICS04, A module can call ```sendPacket``` before a channel open.
```javascript
function sendPacket(
capability: CapabilityKey,
sourcePort: Identifier,
sourceChannel: Identifier,
timeoutHeight: Height,
timeoutTimestamp: uint64,
data: bytes): uint64 {
...
abortTransactionUnless(channel !== null)
abortTransactionUnless(channel.state !== CLOSED)
...
}
```
Assume the module successively calls ```chanOpenInit```, ```sendPacket``` and ```chanCloseInit```, which results that there is a in-flight packet and the closed channel.

The module can't call ```timeoutOnClose``` since there is no counterparty channel.
```javascript
function timeoutOnClose(
packet: Packet,
proof: CommitmentProof,
proofClosed: CommitmentProof,
proofHeight: Height,
nextSequenceRecv: Maybe,
relayer: string): Packet {
...
expected = ChannelEnd{CLOSED, channel.order, channel.portIdentifier,
channel.channelIdentifier, channel.connectionHops.reverse(), channel.version}
abortTransactionUnless(connection.verifyChannelState(
proofHeight,
proofClosed,
channel.counterpartyPortIdentifier,
channel.counterpartyChannelIdentifier,
expected
))
...
}
```
I think it does not match what the ```timeoutOnClose``` is designed for and
>Any in-flight packets can be timed-out as soon as a channel is closed

We might be able to use ```timeoutPacket``` to make the in-flight packet to be timeout.

It doesn't matter that it has some problems (As #965). It needs to call ```verifyNextSequenceRecv```, ```verifyPacketReceiptAbsence``` and ```verifyPacketReceipt``` anyway.
```javascript
function timeoutPacket(
packet: OpaquePacket,
proof: CommitmentProof,
proofHeight: Height,
nextSequenceRecv: Maybe,
relayer: string): Packet {
...
abortTransactionUnless(packet.destChannel === channel.counterpartyChannelIdentifier)
...
abortTransactionUnless(connection.verifyNextSequenceRecv(
proofHeight,
proof,
packet.destPort,
packet.destChannel,
nextSequenceRecv
))
...
abortTransactionUnless(connection.verifyPacketReceiptAbsence(
proofHeight,
proof,
packet.destPort,
packet.destChannel,
packet.sequence
))
...
abortTransactionUnless(connection.verifyPacketReceipt(
proofHeight,
proof,
packet.destPort,
packet.destChannel,
packet.sequence
TIMEOUT_RECEIPT,
))
}
```
I don't know if there functions work properly when ```channel.counterpartyChannelIdentifier``` is empty.

And in [ibc-go](https://github.com/michwqy/ibc-go/blob/main/modules/core/04-channel/keeper/timeout.go), ```TimeoutPacket``` can't be called when channel is closed.
```go
func (k Keeper) TimeoutPacket(
ctx sdk.Context,
packet exported.PacketI,
proof []byte,
proofHeight exported.Height,
nextSequenceRecv uint64,
) error {
if channel.State != types.OPEN {
return errorsmod.Wrapf(
types.ErrInvalidChannelState,
"channel state is not OPEN (got %s)", channel.State.String(),
)
}
}
```

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by comparing the ICS04 timeoutOnClose and timeoutPacket pseudocode in this issue with modules/core/04-channel/keeper/timeout.go in ibc-go, and review the related discussion in #965. Done means the specification and implementation have an agreed behavior for in-flight packets and closed channels, including the relevant verification calls.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
blockchain, distributed-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.