Connection handshake incomplete verification logic

[adizere]

Crate

ibc-relayer

Summary

There is a low-priority TODO in one of the methods for doing the connection handshake steps.

https://github.com/informalsystems/ibc-rs/blob/b30d7637aa54bc55db3236cdcdfd95315eb112ad/relayer/src/connection.rs#L1128

Acceptance Criteria

• Fixed todo, by adding two additional checks, on the ConnectionEnd version fields and the commitment prefix fields.
• Follow-up on @ancazamfir's comment below

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate milestone (priority) applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[seanchen1991]

on the ConnectionEnd version fields and the commitment prefix fields

What criteria needs to be checked on these?

[adizere]

What criteria needs to be checked on these?

I think this should be it:

• the version field on existing_connection must match the version field of expected_connection.
• similarly, the counterparty.prefix field on existing_connection must match the field with the same name of expected_connection.

[seanchen1991]

the version field on existing_connection must match the version field of expected_connection.

It looks like the versions field on the ConnectionEnd type is a vector, but the set_versions method always sets self.versions to be a new 1-element vector for any given new version. Is this vector always expected to have only one element? What should happen if the vector is empty?

[adizere]

but the set_versions method always sets self.versions to be a new 1-element vector for any given new version. Is this vector always expected to have only one element?

I think we use set_versions from the IBC modules library only (and that may evolve to support a vector, but not sure). On the relayer side, when Hermes queries for a ConnectionEnd, it may receive as reply a connection end that has multiple entries in the version field (ref).

In general, an empty version is a valid choice (ref):

The version field is an opaque string which can be utilised to determine encodings or protocols for channels or packets utilising this connection. If not specified, a default version of "" should be used.

So then we'd need to check that both connection ends match (even with empty version).

In practice, this is how a typical version looks like:

 $ ./target/debug/hermes query connection end ibc-0 connection-0
2022-02-14T16:16:03.826508Z  INFO ThreadId(01) using default configuration from '/Users/adi/.hermes/config.toml'
2022-02-14T16:16:03.831423Z DEBUG ThreadId(01) Options: QueryConnectionEndCmd { chain_id: ChainId { id: "ibc-0", version: 0 }, connection_id: ConnectionId("connection-0"), height: None }
Success: ConnectionEnd {
    state: Open,
    client_id: ClientId(
        "07-tendermint-0",
    ),
    counterparty: Counterparty {
        client_id: ClientId(
            "07-tendermint-0",
        ),
        connection_id: Some(
            ConnectionId(
                "connection-0",
            ),
        ),
        prefix: ibc,
    },
    versions: [
        Version {
            identifier: "1",
            features: [
                "ORDER_ORDERED",
                "ORDER_UNORDERED",
            ],
        },
    ],
    delay_period: 0ns,
}

[seanchen1991]

This is very much tangential to this issue, but I noticed that the ConnectionEnd::versions method clones the versions vector. It looks like it would be pretty straightforward to change the method to return a slice, as well as convert any downstream calls to the method to accept a &'a [Version] instead. Some call sites will require a .to_vec to get back an owned vector, but I think it makes more sense to do that than to have the versions getter method return an owned vector.

I could include those changes in this PR or create a new issue and PR for it. What do folks think? 🙂