Delay period on a per-connection basis

[cwgoes]

Replaces #500 per discussion.

[AdityaSripal]

ACK, same approach taken in PR

[milosevic]

My understanding is that delayPeriod is a parameter defined by the chain to provide security to its users. Shouldn't it be reasonable to give each side of the connection possibility to choose delayPeriod? Also what happen in crossing hello case when different delayPeriod is chosen by counter parties?

[colin-axner]

I don't think chains are defining the delay period in this implementation. As you say, each side of the conenction has the possibility to choose the delayPeriod.

In the case of crossing hello's, the connection handshake will succeed with different delay periods. It can be viewed the same as if the crossing hellos selected different client identifiers (they cannot connect with each other because they have different parameters)

[AdityaSripal]

In the case of crossing hello's, the connection handshake will succeed with different delay periods.

I think you mean to say it will fail.

---

We want the initiator of the handshake to define the delayPeriod on both ends. Consider that I want to create a connection with ChainA and chainB, and I want to use the delayPeriod feature to protect packets going across this connection from misbehaviour on either side.

I submit a ConnOpenInit to chainA with a delayPeriod=60. Now, I intend to also set a delayPeriod on chainB, however another relayer has submitted a ConnOpenTry before me on chainB with delayPeriod=0. This tx passes, and I now have a connection with delayPeriod on chainA and no delayPeriod on chainB.

This is clearly not what I wanted as the initiator. This is why we fix the delay period at the INIT step to ensure that the final connection, regardless of which relayer completes it, has the delay period that was specified by the initiating relayer. In OpenTry, we enforce that this connectionEnd also has a delay period specified by the initiating relayer.
As @colin-axner said, in case of crossing hello's that don't agree on delay period, we error just as we do in other cases where parameters do not match.

Now, it is possible that in the INIT step, we could define different delay periods for each chain. So ConnOpenInit might specify delayPeriod AND counterpartyDelayPeriod.
I could see this being potentially useful in cases where a highly secure chain is connecting to a more risky chain, the initiating relayer may specify no delay period for the cosmos-hub client but a high delay period for the risky-chain client.

However, this is a (slight) complication that doesn't seem all that useful imo, and unnecessary for IBC 1.0

[milosevic]

It seems that giving possibility for each party to choose a delayPeriod would conflict with optimistic sends, so it seems like good tradeoff to have it the same on both ends.

[cwgoes]

Aye, we discussed this a bit internally, I guess it should be summarised somewhere public. Great summary @AdityaSripal!

[milosevic]

I have few more high level questions:

1. What are the benefits of having delayPeriod as part of ConnectionEnd and not ClientState? The potential benefits of
   having it as part of ClientState are the fact that as soon as client is created, we know what delay it will ensure. So from higher level protocols and abstractions as soon as they are using right client id it is safe. Second, at the moment verifyClientConsensusState and other verifyX functions do not have packetDelay as a parameter. Are we sure who is using those functions and aren't those functions also potentially targets for the same attack? Having packetDelay as part of ClientState could potentially make also these functions secured the same way (if there is a need for this).
2. What happens with optimistic channel handshake and packet sends? I guess from the channel perspective, at the point channelOpenInit is executed, user should be aware of what delayPeriod it will ensure, and the way to configure this is by choosing connectionId. My understanding is that in this proposal we assume that delayPeriod will be defined at the connection level, so the fact that connection with the given id is created gives user possibility to inspect the connectionEnd and see if its delayPeriod field satisfies its security needs. This seems fine if we assume that delayPeriod is agreed out of band, so there is no need to negotiate the value during the handshake.
3. Does it make sense adding minDelayPeriod also as a parameter to a chanOpenInit and chanOpenTry so IBC can ensure that the chosen connection end will indeed ensure that the delayPeriod will be greater than minDelayPeriod. Having minDelayPeriod as a field in ChannelEnd structure might simplify figuring out which channel to use in case application wants to have flexibility in choosing what delay to ensure depending on the packet data. Also in multi-hop scenarios, we might want to ensure that some minDelayPeriod is ensured across all path, but it does not need necessarily being the same.
4. Adding delayPeriod provides an additional security only if there is a correct relayer monitoring client state updates. It will be probably good idea adding this requirement to the ICS18. More generally, we should probably make this more obvious to the IBC users. I don't know what are the plans regarding updates to ICS20 with respect to these changes, but that could potentially be a way to also signal to the user how to benefit from this mechanism.

[colin-axner]

I have few more high level questions:

I think I can take a shot at answering some of these questions.

1. The benefit of having the delayPeriod in the connection instead of the clientState is that you don't have to create new clients just to have different delay periods, you could simply create a new connection. This would result in having to update less clients overall which can be costly and would also encourage maintaining less clients for a chain, but still allow flexibility in the security of each connection.

I think the delay period is left out of the other verify functions since there is not really any risk involved until a packet is sent (at which point we verify the delay period)

I guess from the channel perspective, at the point channelOpenInit is executed, user should be aware of what delayPeriod it will ensure, and the way to configure this is by choosing connectionId.

correct.

3. Will leave this to @cwgoes and @AdityaSripal but this sounds to me like a UI improvement as opposed to something necessary for spec correctness. Since delayPeriod is set and finalized upon initialization of a new connection, the delay period to be associated with a channel can always be queried for.

[AdityaSripal]

The benefit of having the delayPeriod in the connection instead of the clientState is that you don't have to create new clients just to have different delay periods, you could simply create a new connection. This would result in having to update less clients overall which can be costly and would also encourage maintaining less clients for a chain, but still allow flexibility in the security of each connection.
I think the delay period is left out of the other verify functions since there is not really any risk involved until a packet is sent (at which point we verify the delay period)

1. Correct. In the non-packet verify function, the order of verification success and misbehaviour submission doesn't really matter. Given it doesn't matter, it makes sense to prioritize speed here.
   ie. A channel handshake message may fails because client is frozen, or it may pass and the client is frozen afterwards. The end result is the same. Once packets start getting sent, they will all fail to be received.

However, the order of packet submission and misbehaviour submission really does matter. Which is why delay period gives a chance for misbehaviour to be submitted first.

3. It could make sense to do this. And certainly this check should happen somewhere such that users are always using channels that have underlying connections that suit the user's needs.
   However, I would prefer we don't add this check to the core state-machine. Specifying a connection-id in channel handshake, is an implicit acknowledgement of all the connection parameters.
   We should implement this using connection queries on the relayer-side and make the relayer responsible for using connections that match user's needs when creating a channel.

4. Yes it is true, we rely on correct relayer. However, this is true of misbehaviour in general. All of the misbehaviour and freezing logic we have are only useful if there is a correct relayer monitoring the client state updates. We should communicate this clearly to Hub and to users

[milosevic]

Thanks a lot for the answers, it makes sense. Looks good to me. Just a note that we will put some effort in adding this change to the IBC tla+ spec and try to run it through model checker so there are no surprises :)