IBC ICS Spec versus IBC SDK/ relayer implementations

[ancazamfir]

A few observations regarding some inconsistencies among the connection/ channel handshake specs, relayer spec, SDK code.

ICS pdf

2011 1 function connOpenInit(
...
2017 7 abortTransactionUnless(validateConnectionIdentifier(identifier))

In SDK the message's ValidateBasic is executed before the handler is called so msg validation (including identifiers) is not performed in the handler. Similar in other handlers in the spec.

ICS:

2028 1 function connOpenTry(
...
2046 18 connection = ConnectionEnd{state , counterpartyConnectionIdentifier , counterpartyPrefix ,

Here in the spec line 2046/18 the state not initialized, in SDK is set to UNINITIALIZED which is not a valid state in ICS pdf.

Then, related to the allowed INIT -> TRYOPEN transition. It looks like both A and B can be in INIT state (they were independently initialized by different agents). And it is expected that a relayer will relay for this case but this is not covered in the pendingDatagrams in ICS-018.

Regardless, it is possible that connOpenTry is relayed to both A and B and both chains will transition to TRYOPEN.

Again, a correct relayer will not relay in this case as per ICS-018. If a relayer does relay a connOpenAck to A when both A and B are in TRYOPEN, the handler on A will be ok according to the ICS but fails in SDK:

ICS pdf:
2071 1 function connOpenAck(
...
2080 10 abortTransactionUnless(connection.state === INIT || connection.state === TRYOPEN)

SDK:

cosmos-sdk/x/ibc/03-connection/keeper/handshake.go

Line 145 in d456271

if connection.State != exported.INIT {

Assuming we go with the ICS we are now in the case where both A and B are in OPEN state which is nice as it finishes the handshake in the presence of multiple correct relayers.

Similar observations for channel handshake except the the chanOpenAck in SDK correctly allows for INIT and TRYOPEN states.

[ancazamfir]

Connection and channel state machine diagrams also need updates.

[cwgoes]

Thanks @ancazamfir! Looks like some differences need rectifying here.

[cwgoes]

The ValidateBasic usage is an SDK-specific design pattern; as long as it ends up being equivalent to checking everything in the handler it doesn't seem like a huge issue (to me), though perhaps this should be noted in the SDK implementation docs as a slight difference from the spec.

The rest of the differences are substantive and should be fixed - thanks.

[ancazamfir]

re: ValidateBasic - this SDK function performs a lot of (useful) validation and I believe not all are shown in the IBC spec. If the spec would include full validation in the handlers then they would be less readable. So I was in fact thinking to make a note in the IBC spec that the message fields should be validated. But indeed it is just a small thing.

[fedekunze]

@cwgoes can you check tomorrow if 3-5 still apply? Sounds like a small fix

[cwgoes]

(4) is still the case in the SDK; this line should be changed.

[cwgoes]

@ancazamfir Is the goal for the default relayer to relay everything which can be possibly be relayed? I'm not sure if you would necessarily want to relay if it seemed like another relayer had started the handshake too.

[cwgoes]

Here in the spec line 2046/18 the state not initialized, in SDK is set to UNINITIALIZED which is not a valid state in ICS pdf.

Fixed spec-side in cosmos/ibc@00531ee.

[cwgoes]

The spec-side ICS 18 work can be tracked in cosmos/ibc#402.

(ICS 18 was not really ever intended to be comprehensive, I can update it though)

[ancazamfir]

@ancazamfir Is the goal for the default relayer to relay everything which can be possibly be relayed? I'm not sure if you would necessarily want to relay if it seemed like another relayer had started the handshake too.

But then both relayers may back off.
I believe the relayer should send msg to dest if:
OpenInit -> if dest.state in {"UNINITIALIZED"}
OpentTry -> if src.state = "INIT" and dest.state in {"UNINITIALIZED", "INIT"}
OpenAck -> if src.state = "TRYOPEN" and dest.state in {"INIT", "TRYOPEN")
OpenConfirm -> if src.state = "OPEN" and dest.state in {"TRYOPEN"}

And SDK handler on dest should check local state as above.
Handshake diagrams also need updates in ICS-es