Channel handshake failure with one-sided fee middleware on empty version string

[srdtrk]

Overview

This issue serves as a documentation point for a recurring unexpected behavior observed when initializing new channels with the MsgChannelOpenInit and when the version string is left empty and the counterparty chain lacks the fee middleware. Users encountering this issue are encouraged to refer to this discussion for more information.

Summary of Issue

When MsgChannelOpenInit is invoked with an empty version string to establish a new channel, and the counterparty chain does not implement the fee middleware, the channel opening process fails at the ChanOpenTry step.

Expected Behaviour

A channel initializes successfully without requiring the fee middleware.

Description and Context

• The fee middleware is designed to operate with channels that include a specifically formatted (wrapped) version string, such as '{"fee_version":"ics29-1","app_version":"ics20-1"}'.
• The issue arises because when an empty version string is passed to MsgChannelOpenInit, by default, the fee middleware wraps any application version within the specified format. When an empty version string is provided, it results in a mismatch if the counterparty chain does not recognize or support the wrapped version format, leading to rejection at the ChanOpenTry phase.
• To circumvent this issue, it is recommended to initialize channels using a filled version string such as 'ics20-1' for transfer channels.

Version

All current versions are affected by this issue.

Steps to Reproduce

1. Ensure the counterparty chain does not have the fee middleware installed.
2. Create a new channel with an empty version string.
3. Observe that the channel opening is unsuccessful, halting at ChanOpenTry.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[crodriguezvega]

Thanks for opening the issue, @srdtrk.

Just quickly sketching some changes to see if this would fix the issue (haven't thoroughly thought of all the consequences yet):

diff --git a/modules/apps/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go
index 4ac1e7dd9..5c839c8f2 100644
--- a/modules/apps/29-fee/ibc_middleware.go
+++ b/modules/apps/29-fee/ibc_middleware.go
@@ -114,8 +114,6 @@ func (im IBCMiddleware) OnChanOpenTry(
                return "", errorsmod.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, versionMetadata.FeeVersion)
        }

-       im.keeper.SetFeeEnabled(ctx, portID, channelID)
-
        // call underlying app's OnChanOpenTry callback with the app versions
        appVersion, err := im.app.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, versionMetadata.AppVersion)
        if err != nil {
@@ -139,18 +137,14 @@ func (im IBCMiddleware) OnChanOpenAck(
        counterpartyChannelID string,
        counterpartyVersion string,
 ) error {
-       // If handshake was initialized with fee enabled it must complete with fee enabled.
-       // If handshake was initialized with fee disabled it must complete with fee disabled.
-       if im.keeper.IsFeeEnabled(ctx, portID, channelID) {
-               versionMetadata, err := types.MetadataFromVersion(counterpartyVersion)
-               if err != nil {
-                       return errorsmod.Wrapf(err, "failed to unmarshal ICS29 counterparty version metadata: %s", counterpartyVersion)
-               }
-
+       versionMetadata, err := types.MetadataFromVersion(counterpartyVersion)
+       if err == nil {
                if versionMetadata.FeeVersion != types.Version {
                        return errorsmod.Wrapf(types.ErrInvalidVersion, "expected counterparty fee version: %s, got: %s", types.Version, versionMetadata.FeeVersion)
                }

+               im.keeper.SetFeeEnabled(ctx, portID, channelID)
+
                // call underlying app's OnChanOpenAck callback with the counterparty app version.
                return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, versionMetadata.AppVersion)
        }

Basically:

• Not calling SetFeeEnabled in OnChanOpenTry, but call it instead in OnChanOpenAck.
• If the counterparty version in OnChanOpenAck doesn't include fee version, then call underlying app.

Would this work? cc @AdityaSripal

[crodriguezvega]

We would also probably need to modify transfer's OnChanOpenTry, since if the counterparty doesn't have fee middleware wired up, then this check will fail.

[AdityaSripal]

So there's a very simple solution here.

We can simply set the default version to not have fee enabled, and only explicitly enable if the relayer passes in the correct version.

I'm slightly not in favor.

The point is that a module with fee middleware should optimistically try to pass a fee version. Of course if the counterparty doesn't have fee then this will fail.

But passing an empty version in the reverse direction, INIT from the chain without fee. Or just passing in an explicit version ics-20 will work just fine.

I understand it can be annoying in a world where many chains don't have fee middleware yet. But I still find it weird to have the default behavior of fee be disabled. In this case, unless a relayer explicitly desires it, a channel between two fee-enabled chains will not have fees if the relayer passes in blank versions. I think that will then be its own issue once chains catch up and then now we need relayers to pass in explicit fee versions (and by extension) explicit versions for everything else on the stack.

I understand the bit of inconvenience in this particular case. But failed handshakes because the initializing side was too ambitious with its proposed version is not such a big deal or necessarily unexpected behaviour. The information that TRY failed should tell relayers that they need to pass in an explicit version that both accept. Since handshakes are one-time per chain-chain connection, I don't think this is bad.

In principle, I don't see why TRY couldn't simply move forward with its own proposed version ics-20. Then, on ACK, we see the counterparty didn't accept our version and we downgrade and move to OPEN on ics-20. This I think would be the ideal solution.

This I think is what you are proposing Carlos.

However, the problem is that the counterparty that is executing TRY does not have fee middleware enabled. So the transfer module on TRY is seeing the full fee version and returning an error.

Rather than returning an error in transfer TRY, we can instead commit our default version ics-20. And then continue with the logic you propose Carlos.