Wait for tx hash to succeed regardless of unrecognized IbcEvents

[soareschen]

Closes: #2314

Description

Extracted from #2227 to make the waiting of send_tx to succeed if all TxHash are committed successfully. Previously, send_tx would timeout and fail if unsupported transactions are submitted with events unrecognized by IbcEvent, even if the transaction were successful.

---

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/).

Reviewer checklist:

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

[ancazamfir]

Is there an issue we can link here that describes the usecase of "unsuported" transactions? If not, can we open one and provide some context?

[ancazamfir]

The original intent for send_batched_messages_and_wait_commit() was:

• send N messages to the chain
• get back exactly N IbcEvent-s such that result i is for message i

Not sure this still holds. For non-IBC messages we could get IbcEvent::Empty

[soareschen]

I don't understand the rationale to expect the number of IbcEvents to be exactly the same as the number of messages sent. In theory one message can result in any number of chain events, which can be translated into any number of IbcEvents.

I think a more practical approach is to use find through the result list of IbcEvents to find the event of interest, instead of looking at a specific position.

Also the change does not affect the semantics of existing transactions sent. So if the existing code produce messages that result in exact number of IbcEvents, then they would still get that. It would only affect future changes, such as when the code is updated to send non-IBC messages mixed with IBC messages.

[ancazamfir]

I don't understand the rationale to expect the number of IbcEvents to be exactly the same as the number of messages sent. In theory one message can result in any number of chain events, which can be translated into any number of IbcEvents.

It's always been the case in practice that an IBC message results in one and only one IBC event. While the events have not been documented in the IBC spec yet, some documentation exists here

I think a more practical approach is to use find through the result list of IbcEvents to find the event of interest, instead of looking at a specific position.

You still need to figure out the original message that caused a specific IbcEvent. But sure there are other ways of coding this. Still trying to understand the usecase that breaks the current design.

Also the change does not affect the semantics of existing transactions sent. So if the existing code produce messages that result in exact number of IbcEvents, then they would still get that.

Yes I was asking in general not necessarily pointing to your changes in this PR.

It would only affect future changes, such as when the code is updated to send non-IBC messages mixed with IBC messages.

Yeah, i still want to understand this. I think hermes should only deal with IBC messages and never send messages that have nothing to do with IBC. Probably we should start with what "IBC message" means? And what is a non-IBC message?

[ancazamfir]

The original intent for send_batched_messages_and_wait_commit() was:

• send N messages to the chain
• get back exactly N IbcEvent-s such that result i is for message i

Not sure this still holds. For non-IBC messages we could get IbcEvent::Empty

i checked master and it is broken. When a Tx has N messages, if the Tx fails then all messages should be marked as failed and get back N error events. I have a quick fix in branch anca/err_for_all_msgs_in_tx. Also added and corrected some comments.
I haven't checked/ fixed the packet worker (async sender), at first look it doesn't handle error events here: (https://github.com/informalsystems/ibc-rs/blob/194a1714cf9dc807dd7b4e33726319293848dc58/relayer/src/link/relay_path.rs#L609-L615).
For the sync sender cases the code looks better.

[soareschen]

Is there an issue we can link here that describes the usecase of "unsuported" transactions? If not, can we open one and provide some context?

Filed #2314.

[seanchen1991]

What is the purpose of this try_collect? Is it attempting to resolve the Futures in the previous collect?

[mzabaluev]

@seanchen1991 I believe the first collect populates the FuturesOrdered with tasks to complete, and try_collect is a futures method to drive the stream and collect the yielded values, failing fast on error.

[soareschen]

You can see from Rust Analyzer the types for each step:

let responses = tx_hashes
  // Iterator of `Hash`
  .iter()
  // Iterator of future of `Result<TxSearchResponse, Error>`
  .map(|tx_hash| wait_tx_hash(rpc_client, rpc_address, timeout, tx_hash))
  // Future of iterator of `Result<TxSearchResponse, Error>`
  .collect::<FuturesOrdered<_>>()
  // Future of `Result<Vec<TxSearchResponse>, Error>`
  .try_collect()
  .await?;

• Basically we have a collection that in simple form is something like Vec<Future<Result<TxSearchResponse, Error>>>.
• But we want to return Result<Vec<TxSearchResponse>, Error>.
• So we first turn it into Future<Vec<Result<TxSearchResponse, Error>>> using .collect::<FuturesOrdered<_>>().
• Next we turn it into a Future<Result<Vec<TxSearchResponse>, Error>> using .try_collect().
• Finaly we turn it into Result<Vec<TxSearchResponse>, Error> using .await.

[adizere]

Probably we should start with what "IBC message" means? And what is a non-IBC message?

I think SDK upgrade proposals are one example of non-IBC transactions. I doubt there will be others.

@mzabaluev had experience with parsing events from non-IBC tx-es, specifically while working with upgrade proposals in #1909; we closed the PR because it required too many changes: #1909 (comment). We decide in that PR that Hermes will not wait on tx events after submitting the upgrade proposal, and instead it just prints the tx hash and exits (solution is #1979).

[mzabaluev]

Probably we should start with what "IBC message" means? And what is a non-IBC message?

As far as I understand, the IbcEvent enum is our strictly typed way to represent various chain events.
It is defined in ibc. I don't know if it is used anywhere outside the relayer, but this has been put forward as a justification for keeping it "pure IBC", for example, in #1909.

A discussion on how to systematize chain events should go hand in hand with broader extensibility, which looks like a sane way to accommodate non-Cosmos chains: #2284 (comment)

[ancazamfir]

I think SDK upgrade proposals are one example of non-IBC transactions. I doubt there will be others.

Yes but part of that message is this blob defined in IBC TAO (client) implementation.
There is also an IBC handler in the IBC core client impl in go. So it seems that this operation should be applicable, in theory at least, to any chain type.

And imo if it is defined in the IBC TAO proto, implemented (even partially) by an IBC core (client in this case) handler then it is a transaction that should generate and IBC event.
(more thoughts in #2285 (comment))

[adizere]

I agree with the approach of digging into the particular use-cases of where we need to support non-IBC events. What are those cases in detail?

• The chain-upgrade use-case is currently handled by using the AsyncSender interface which skips waiting on events, and this use-case in particular should in fact generate IBC-specific events (which will do, ref: IBC Client event for client UpgradeProposal cosmos/ibc-go#1558).
• The most important is from new Gaiad-Rho upgrade, do we have details there?
• Another one is for testing, what is the rationale there, for submitting arbitrary transactions?

[adizere]

I will close this PR because it is a distraction from doing a stable v1 release.

Summary

The discussion here & in the associated issue can be summarised as follows: It is unclear what benefits or problem we're solving by allowing non-IBC events to percolate through the Hermes pipeline.

Further details

There are 2 cases where non-IBC events could have been potentially interesting for Hermes:

• for chain upgrade proposals, which did not emit any IBC event, but fixed with IBC Client event for client UpgradeProposal cosmos/ibc-go#1558
• for MsgRegisterCounterpartyPayee part of the ICS29 implementation flow, fixed with emit event on RegisterPayee and RegisterCounterpartyPayee rpc handlers cosmos/ibc-go#1566

For the above two cases, it makes sense to have IBC events associated with them, and the IBC-go team agreed to fix that. Other cases which will demand non-IBC events might occur in the future, and in that case we could reconsider the relayer architecture. Quoted the issue #2314:

Generally, the same limitation probably also applies to all non-IBC messages, such as local token transfer, staking tokens, token swaps, calling smart contracts, etc. Although Hermes does not currently perform any of such transactions, it is an overly specialized limitation that should not be there.

Hermes is indeed specialized to IBC transactions. That could change in the future to become a more general off-chain orchestrator, but not clear at this point. Until concrete cases such as the above appear & inform our architectural decisions, I don't see a reason to generalize the relayer beyond IBC.

There is another implicit specialization (or assumption) that Hermes makes when submits transactions: it expects each message to result in one IBC event by filtering the TxSyncResult data it obtains from the chain. A suggestion by Soares is that Hermes should return a Vec<Vec<IbcEvent>> instead of Vec<IbcEvent>, allowing a vector of responses (likely with 1 element each) for each message.