Support IBC version negotiation in contracts

[alpe]

We need to support the protocol version negotiation. An implementation example from ICS20:

// NegotiateAppVersion implements the IBCModule interface
func (am AppModule) NegotiateAppVersion(
	ctx sdk.Context,
	order channeltypes.Order,
	connectionID string,
	portID string,
	counterparty channeltypes.Counterparty,
	proposedVersion string,
) (string, error) {
	if proposedVersion != types.Version {
		return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "failed to negotiate app version: expected %s, got %s", types.Version, proposedVersion)
	}

	return types.Version, nil
}

See https://github.com/cosmos/ibc-go/blob/v2.0.1/modules/apps/transfer/module.go#L439

[ethanfrey]

Are there more detailed docs / specs on how this works?

There is version negotiation already implicit in the Channel Handshake, but they put the onus on the relayer to do so. I would love to see a high level explanation of how this works now.

Nevermind, I will ask the ibc team directly.

[alpe]

I have only found this: https://github.com/cosmos/ibc-go/blob/v2.0.1/docs/migrations/ibc-migration-v100.md#ibc-apps

[faddat]

It is not super clear at all

[ethanfrey]

From Carlos at Interchain GmbH:

We have some documentation here https://ibc.cosmos.network/main/ibc/apps.html in the Channel Handshake Version Negotiation section. Indeed relayers don't support this yet officially but we're working on hermes and go relayer to add support for this because it's needed for Interchain Accounts. If this is not enough information, please let us know.

That does explain the call stack some. I assume for a first version, we can just stub this function that it always returns the proposed version, and the contracts will work the same as ibc v1. I would consider adding an optional entry point to allow a contract to negotiate, but that is definitely an "after the upgrade" issue. For now, let's just have all contracts working the same in 0.44 as in 0.42. Later add backwards-compatible enhancements

[ethanfrey]

BTW, this seems to be a separate query: https://github.com/cosmos/ibc-go/blob/7a0fedddf6a038fdda8b6911a345b7c6ab8a564e/modules/core/05-port/keeper/grpc_query.go#L17-L45

I actually disapprove of this design.

[ethanfrey]

Here is the flow in ChannelOpenTry:

• In k.ChannelKeeper.ChanOpenTry:
  • Create the new channel with "version", which is taken from whatever the relayer sets
  • Ensure the counterpartyVersion is set properly via proofs.
• In cbs.OnChanOpenTry we just take these as fixed values.

BUT the version has never been committed to yet.

A better version, IMO would be to pass in counterpartyVersion in OnChanOpenTry (is there any use for the separate version field? if so, then both) and then allow it to return (version string, err error). We could then update the channel struct in storage before committing it in the same message.

The communication back to the initiator is only done by the channel objects which is stored in state and committed to. Which contains the desired version.

The benefits here are:

• The negotiation happens only when a channel is really being created (in the same call), allowing the IBC Handler to save some state about what was requested and what was granted.
• No changes needed to the relayer to work with this
• No changes needed to logic structure of programs (easier opt-in for CosmWasm for example)

[ethanfrey]

BTW, discussing this with Chris ages ago: CosmWasm/cosmwasm#147 (comment)

Really wish the IBC team consulted me... CosmWasm/cosmwasm#147 (comment)

Just like this bug which was dismissed until Informal mentioned it in an audit. Sigh

[ethanfrey]

To give some more context, reposting some of my Discord comments:

Init - chain A accepts version

Try - chain B receives counterparty version, chooses what version it supports (may be the same, may be nothing compatible)

Ack - A sees what B committed to, if they is unacceptable, can abort

Confirm - we are all good.

Both sides know once Ack/Confirm passes the channel is solid
I had longer discussions on this. Chris said the relayers must set the versions
There is no way for A to be sure of the version B wants until it gets Ack with that proof

What I said above is how I understood version negotiation and what I was asking summer 2020 when integrating in cosmwasm

[ethanfrey]

Also, what are versions???

I figured it would be like

A: ics20-2
B: ics20-1 (it only supports older version, attempts to downgrade)
A: can accept downgrade or abort
Another example. Maybe there are multiple feature flags on a protocol.B doesn't support ics27 governance votes, but does support other parts of the protocol.

A: ics27:bank,staking,gov
B: ics27:bank,staking
A: decides if it really needs gov or can live with subset

Is that currently what they are? Protocol versions?
Same way I can connect via HTTP/1.1 and request and upgrade to HTTP/2.0 (version strings should carry such meaning I believe)

[chipshort]

It looks to me like this is no longer needed: https://github.com/cosmos/ibc-go/blob/10175b91b6b8c2c3ecdcb22d6d844748f9fcc516/docs/docs/05-migrations/04-v2-to-v3.md?plain=1#L148