Interchain Accounts: consider using smaller timeoutTimestamp

[andrey-kuprianov]

Surfaced from @informalsystems audit of ICS27 InterchainAccounts at hash b8bc1a8

In the current implementation of 27-interchain-accounts/keeper/relay.go, when tx packets are sent, the timeout is set to max uint64, thus making the packets effectively never expiring:

	// timeoutTimestamp is set to be a max number here so that we never recieve a timeout
	// ics-27-1 uses ordered channels which can close upon recieving a timeout, which is an undesired effect
	const timeoutTimestamp = ^uint64(0) >> 1 // Shift the unsigned bit to satisfy hermes relayer timestamp conversion

	packet := channeltypes.NewPacket(
		icaPacketData.GetBytes(),
		sequence,
		sourcePort,
		sourceChannel,
		destinationPort,
		destinationChannel,
		clienttypes.ZeroHeight(),
		timeoutTimestamp,
	)

The problem I see with this approach is that it gives the application control away completely: the app (IA) doesn't have a chance to react in case a packet got stuck for some reason. And there are such cases, e.g. when a LightClient is not updated, and the trusting period passes, so it can't ever be updated again (besides using a special governance proposal), as outlined e.g. in the ICS02:

If a client can no longer be updated (if, for example, the trusting period has passed), it will no longer be possible to send any packets over connections & channels associated with that client, or timeout any packets in-flight (since the height & timestamp on the destination chain can no longer be verified). Manual intervention must take place to reset the client state or migrate the connections & channels to another client. This cannot safely be done completely automatically, but chains implementing IBC could elect to allow governance mechanisms to perform these actions (perhaps even per-client/connection/channel in a multi-sig or contract).

I understand the motivation for choosing the high timeout, such that the channel never gets closed because of a timed out packet. But I believe the real architectural choice here is different. Comparing the two cases:

1. with unclosed but unusable channel, with stuck packets
2. received notification of expired timeout, and a closed channel

The second one I think is much better, because you can react somehow. In the first one your app is stuck completely, and you loose any control over what happens next.

CC @seantking

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[seantking]

Great points. I agree with this 👍

I guess the question is how long do we make the timeouts? Do we pass the decision along to the application developers who will be building modules on the controller side that use ICA or just pick a reasonable time like 2-3 days?

[andrey-kuprianov]

As this is not a single transaction, but a sequence of transactions, I would do it like that:

• use relative timeouts for time & height; let's call them timeoutTimestampDelay and timeoutHeightDelay, with the semantics that for an outgoing packet the ICA module sets timeoutTimestamp and timeoutHeight to the current timestamp (resp. height) + the corresponding delay.
• give reasonable defaults to this values in the ICA itself (e.g. 2 days for timestamp delay, and unlimited delay for heights)
• export the corresponding variables to the users of ICA, i.e. allow them to change the defaults if needed.

Also, because any timeout will close the channel, this implies the necessity of some retry mechanism which would reopen the channels when needed and/or notify the user application of the failures.

[colin-axner]

thanks @andrey-kuprianov! The concern with packets that can never be timed out makes sense to me.

One concern that comes to mind with respect to the current proposed solutions is that predicting appropriate timeouts is difficult since we are trying to predict the time/height for a counterparty blockchain which we only have past information for. In fact we recently faced this issue on the UX side in #508. Using time.Now() is not an option since it could lead to chain halts due to unsynchronized validator clocks. We could use our own block time, but I'm unaware of how synchronized blockchains currently are with respect to timestamp

We could allow this to be inputted by the controller module (and thus likely a user inputted value). The benefit is that a user inputted timestamp has the benefit of knowing the actual time on the counterparty (assuming it has access to the counterparties blockchain), but the downside is that timeouts due to user error are probably pretty likely. Thoughts?

[colin-axner]

We have decided to use an ICA authentication module provided timestamp value. This increases the likelihood of channels becoming closed due to the timeout being capable of being reached. Since usage of interchain accounts is so widely varied, it is impossible to predict what value a timeout timestamp would provide. Thus allowing flexibility in the decision makes sense.

Implementation of this issue should make it very clear that an appropriate timeout timestamp value is required. The TrySendTx interface will be modified with this extra argument