ICS04: some questions about function timeoutOnClose and timeoutPacket
- 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