ICS04: some questions about function timeoutOnClose and timeoutPacket

[michwqy]

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.

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.

function timeoutOnClose(
  packet: Packet,
  proof: CommitmentProof,
  proofClosed: CommitmentProof,
  proofHeight: Height,
  nextSequenceRecv: Maybe<uint64>,
  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.

function timeoutPacket(
  packet: OpaquePacket,
  proof: CommitmentProof,
  proofHeight: Height,
  nextSequenceRecv: Maybe<uint64>,
  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, TimeoutPacket can't be called when channel is closed.

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(),
		)
	}    
}

[crodriguezvega]

Hi @michwqy. Thanks for bringing up these questions!

Question 1

note: the connection may have been closed

I think the comment is not correct and can be removed, CLOSED is not a valid state for the connection.

Question 2

Optimistic sends (i.e. sending packets before the channel is fully opened) was disabled in ibc-go because it was not working properly. Maybe the spec should be updated as well (so that packets are not allowed to be sent before the channel end is opened) until we specify a fully-working design of optimistic sends sometime in the future. But even if the situation that you describe could theoretically happen according to the spec, it doesn't in practice since ibc-go will allow packets to be sent only when the counterparty has also opened the channel.

[michwqy]

Thanks @crodriguezvega . I find several others that look likely according to spec but I don't know whether they are feasible in reality.

Question 3

Acknowledging packets is not required

Though it seems that the sent packet may not require an ack, I think that acknowledgePacket, timeoutPacket and timeoutOnClose should make every packet sent eventually be ack or timeout in any case (at least at the logical level).

Assume that module a on chain A sends a packet to module b on chain B (by calling sendPacket and assume that the channel is already open in both chains), and module b receives the packet successfully (by calling recvPacket), then module b closes the channel (by calling chanCloseInit). Now module a has two datagrams on the way and two function acknowledgePacket and chanCloseConfirm to call. If chanCloseConfirm is called first due to the concurrency between relayers or incorrect behavior of relay (the relay is not trusted), acknowledgePacket can not be called bacause the channel is closed.

function acknowledgePacket(
  packet: OpaquePacket,
  acknowledgement: bytes,
  proof: CommitmentProof,
  proofHeight: Height,
  relayer: string): Packet {
    ...
    channel = provableStore.get(channelPath(packet.sourcePort, packet.sourceChannel))
    abortTransactionUnless(channel !== null)
    abortTransactionUnless(channel.state === OPEN)
    ...
  }

And the packet can't be ack or timeout (since it has been received successfully).

Question 4

For ORDERED_ALLOW_TIMEOUT channel, timeoutPacket will increase nextSequenceAck.

function timeoutPacket(
  packet: OpaquePacket,
  proof: CommitmentProof,
  proofHeight: Height,
  nextSequenceRecv: Maybe<uint64>,
  relayer: string): Packet {
    ...
    if channel.order === ORDERED_ALLOW_TIMEOUT {
      nextSequenceAck = nextSequenceAck + 1
      provableStore.set(nextSequenceAckPath(packet.srcPort, packet.srcChannel), nextSequenceAck)
    }
    ...
  }

Assume that module A sent two packets (e.g. sequences are 1,2 and the init nextSequenceAck == 1) and module B received No. 1 packet successfully. But No. 2 packet was timeout and module A called timeoutPacket for it, which caused the nextSequenceAck == 2. And now module A can't call acknowledgePacket for No.1 packet, because acknowledgePacket requires packet.sequence === nextSequenceAck.

function acknowledgePacket(
  packet: OpaquePacket,
  acknowledgement: bytes,
  proof: CommitmentProof,
  proofHeight: Height,
  relayer: string): Packet {
    ...
    if (channel.order === ORDERED || channel.order == ORDERED_ALLOW_TIMEOUT) {
      nextSequenceAck = provableStore.get(nextSequenceAckPath(packet.sourcePort, packet.sourceChannel))
      abortTransactionUnless(packet.sequence === nextSequenceAck)
      ...
    }
    ...
  }

[crodriguezvega]

Question 3

Now module a has two datagrams on the way and two function acknowledgePacket and chanCloseConfirm to call. If chanCloseConfirm is called first due to the concurrency between relayers or incorrect behavior of relay (the relay is not trusted), acknowledgePacket can not be called bacause the channel is closed.

I think this is something that the application callback can take care of and it doesn't necessarily need to be handled at the TAO layer. For example, if packets sent from chain A have been received on chain B AND chain B has closed its side of the channel AND a relayer tries to execute chanCloseConfirm on chain A before the acknowledgements have been processed, then the application callback onChanCloseConfirm can block closing the channel until the remaining packets have been acknowledged (and it can figure that out if there are still packet commitments for those remaining packets).

Question 4

Assume that module A sent two packets (e.g. sequences are 1,2 and the init nextSequenceAck == 1) and module B received No. 1 packet successfully. But No. 2 packet was timeout and module A called timeoutPacket for it, which caused the nextSequenceAck == 2. And now module A can't call acknowledgePacket for No.1 packet, because acknowledgePacket requires packet.sequence === nextSequenceAck.

I am not sure if the condition should be relaxed here actually... If the condition changes toabortTransactionUnless(packet.sequence <= nextSequenceAck) in acknowledgePacket for ORDERED_ALLOW_TIMEOUT channels only, then it would be possible to acknowledge packets in an order different to what they were sent, which I am not sure if it is the desired behaviour... However, I think that if there's a use case that requires to an application to process all acknowledgements, then this situation could be handled again in the application callback: in the onTimeoutPacket callback the application can check if there are still packet commitments for the sequences before the one that is being timed out, and if there are, then reject the timeout and wait for a relayer to finish the lifecycle of the previous packets and then re-try timeout.

[crodriguezvega]

Closing this issue for now. @michwqy I hope my answers above helped, otherwise, please feel free to reopen.

[michwqy]

I hope it is not too late to further discuss these questions. @crodriguezvega

Question 2

Of course, if there is no optimistic send, there is no problem. But I want to further discuss the problem brought by optimistic sends.

In reality, optimistic sends are disabled due to a transaction that cannot be timed out. The public issue behind this event and my example is that the two functions timeoutOnClose and timeoutPacket fail to cover all situations.

If optimistic sends are allowed, the local channel end may be in a state of no counterparty, tryopen, closed, or open after the packet sent. In spec, timeoutOnClose requires the counterparty channel end is closed, and timeoutPacket requires at least the counterparty channel end to exist. In ibc-go, timeoutOnClose requires the counterparty channel end is closed, and timeoutPacket requires the local channel end is open. They all missed some situations, which is why there are some special scenarios where packets cannot be timed out.

Question 3

I agree that this issue is more suitable for handling in application callbacks.

Question 4

If all packets are required to be received and acknowledged in an order they were sent, the correct conditions should be as follows:

function timeoutPacket(
  packet: OpaquePacket,
  proof: CommitmentProof,
  proofHeight: Height,
  nextSequenceRecv: Maybe<uint64>,
  relayer: string): Packet {
    ...
    if channel.order === ORDERED_ALLOW_TIMEOUT {
      abortTransactionUnless(packet.sequence === nextSequenceAck)
      nextSequenceAck = nextSequenceAck + 1
      provableStore.set(nextSequenceAckPath(packet.srcPort, packet.srcChannel), nextSequenceAck)
    }
    ...
  }

Subsequent packets should wait for the previous packet to be acknowledged or timed out before being acknowledged or timed out.

In addition, I believe that although some special examples can be targeted by the application, application developers may not necessarily be aware of the need for special handling. And some special examples may reflect negligence in protocol design.

I would be grateful if there could be more discussion.

[michwqy]

@crodriguezvega I can't reopen this issue, shall I open a new one?

[crodriguezvega]

@michwqy Sorry I missed your messages within the rest of all GitHub notifications. Yes, happy to continue the discussion; I will go over your previous comment and reply.