Remove assumption that ICS-04 packet data is a valid UTF-8 string

[romac]

Closes: #3770

Description

This PR removes the assumption that the ICS-04 packet data is valid UTF-8, by using the packet_data_hex attribute instead of the deprecated packet_data attribute.

Relying on the packet_data attribute enforces a UTF-8 encoded payload (eg. JSON), disallowing Protobuf-encoded payloads which we are starting to see in the wild.

The packet_data atttribute has been deprecated in favor of packet_data_hex since IBC-Go v1.0.0.

---

PR author checklist:

• Added changelog entry, using unclog.
• Added tests: integration (for Hermes) or unit/mock tests (for modules).
• Linked to GitHub issue.
• Updated code comments and documentation (e.g., docs/).
• Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

• Reviewed Files changed in the GitHub PR explorer.
• Manually tested (in case integration/unit/mock tests are absent).

[romac]

We should figure out how to test this, preferably against a chain with a module which uses Protobuf-encoded ICS 04 packet data.

[romac]

Here is how I tested this PR:

1. I modified ibc-gotransfer module to change the type of the ICS-20 memo from string to []byte.

2. I used a modified version of Hermes based on this branch to submit a ICS-20 transfer message with a non-UTF-8 memo, and got the following error:

estimate_gas: failed to simulate tx. propagating error to caller: gRPC call `send_tx_simulate` failed with status: status: Internal, message: "failed to decode Protobuf message: EventAttribute.value: Event.attributes: Result.events: SimulateResponse.result: invalid string value: data is not UTF-8 encoded

This error comes from somewhere in Comet/SDK, where they assume that an event attribute value is a valid UTF-8 encoded string, and there is nothing we can do about that.

3. Then I modified ibc-go again to not emit an event attribute for the memo and the deprecated packet_data attribute, and then error went away, my packet was successfully relayed.

---

So to summarize, while this PR indeed enables Hermes to relay packets with non-UTF-8 packet data, in practice we will never see those because:

1. ibc-go emits the whole packet data as a string in the deprecated packet_data event attribute, without ensuring it's actually valid UTF-8.
2. There is an assumption somewhere in the Cosmos SDK or in CometBFT that event attributes must be valid UTF-8 data, leading to the error above.

[ancazamfir]

Great work! Thanks @romac!

[romac]

There is an assumption somewhere in the Cosmos SDK or in CometBFT that event attributes must be valid UTF-8 data, leading to the error above

Actually I was wrong. There is indeed an assumption in Comet v0.37+ that event attributes are valid UTF-8 strings but it is not Comet that emits the error above.

In fact, this error is yielded not by the chain but within Hermes, specifically by prost when it tries to decode the SimulateResponse it gets from the chain.

This SimulateResponse proto has a field result which itself contains an events field of type Vec<tendermint_proto::abci::Event>.

This tendermint_proto::abci::Event is an alias for tendermint_proto::v0_38::abci::Event which contains a vector of EventAttribute, defined as:

pub struct EventAttribute {
    #[prost(string, tag = "1")]
    pub key: ::prost::alloc::string::String,
    #[prost(string, tag = "2")]
    pub value: ::prost::alloc::string::String,
    /// nondeterministic
    #[prost(bool, tag = "3")]
    pub index: bool,
}

where key and value are of type String, which enforces valid UTF-8.

Hence why decoding the simulation response fails.

I modified ibc-proto-rs to instead use tendermint_proto::v0_34::abci::EventAttribute, whose key and value are Vec<u8> and was able to successfully relay a packet with non UTF-8 memo, even in the presence of the packet_data event attribute in the simulation response.

[romac]

Until ibc-go stops emitting the packet_data and packet_ack attributes, the best we can do is to modify the proto types for EventAttribute to use the CometBFT v0.34 definition, which has those specified as bytes instead of string. WIP PR for ibc-proto-rs here.

[maurolacy]

Hi,
I can confirm this solves the issue we were having with using Protobuf in the IBC channel.

Any plans to merge this?

Thanks,

[romac]

Glad to hear! We'll include this in the next Hermes release.