ConnectionOpenTry: double-check check_client_consensus_height impl

[plafer]

Specifically this line. Our check is:

if claimed_height > ctx.host_current_height() { fail }

whereas ibc-go does

if consensusHeight.GTE(selfHeight) { fail }

That is, > vs >=. However, we shouldn't necessarily blindly change it to >=. See this discussion for the subtleties involved in this decision.

[plafer]

Additionally, note that we also check the lower bound in check_client_consensus_height(), as mandated by ics-24. ibc-go doesn't, and neither does the spec.

Maybe this check is redundant with the consensus state proof verification? i.e. if the host doesn't have the consensus state stored at that height, then the consensus state proof verification will fail.

[hu55a1n1]

Copying the TLDR; from Anca's comment on cosmos/ibc-go#1732 ->

The only thing i can think of is to have a different check for simulation (>) vs deliverTx (>=). But not sure it is possible. We can fix it at relayer side by waiting for the chain to advance to next height.

I wonder if the new ADR05 validation/execution separation can help solve this problem.

[hu55a1n1]

Maybe this check is redundant with the consensus state proof verification? i.e. if the host doesn't have the consensus state stored at that height, then the consensus state proof verification will fail.

I think this is correct and we could remove the redundant check for > oldest_height.

[Farhad-Shabani]

It is closed, as it is obsolete following the implementation of ADR05 and PR #257

[plafer]

We changed where the height check is (now here for conn_open_try), but we still need to understand the discrepancy (we do >, ibc-go does >=). This might have something to do with ibc-go's simulation; there was a bug in the past related to that check. It's low-priority since it is an edge case and the relayer can always just send the datagram a second time if the check fails; but we should still investigate and document our findings.

[Farhad-Shabani]

oh, Thanks for the update 👍🏻