Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

broadcast multiple tx per block from the same wallet #1084

Merged
merged 19 commits into from
Feb 21, 2023

Conversation

agouin
Copy link
Member

@agouin agouin commented Feb 5, 2023

Currently, a transaction error when there is a batch of multiple messages means we don't always know exactly which specific message caused the error. This also means some messages could be given up on due to being grouped with messages that have errors.

The current retry logic for broadcasting messages in SendMessages does not pair well with the retry logic in the PathProcessor. It is redundant retrying, and The new SendMessagesToMempool does not have retry logic, allowing the PathProcessors to manage retries in tune with block production.

This also means that PathProcessors will not be blocked waiting for transactions to be included in blocks. They will only be blocked until the transaction enters the mempool (or fails making it there).

This splits up messages to be two per transaction (update client plus specific IBC tx) and overrides broadcast logic to pack multiple transactions into a single block signed by a single account.

This ensures that messages are error handled independently by wrapping them each in their own transactions. It copies in logic from lens to use sync broadcast mode, but not blocking on block inclusion. It uses the nextAccountSeq to make assumptions on the account sequence numbers that should be used for transactions in the same block.

Trade-off: this provides a much higher guarantee that valid messages will be delivered in the midst of error scenarios, but costs more in gas fees as the relayer is not batching messages.

Question - for broadcast-mode: batch, why not simulate txs with messages individually first to identify valid messages, then batch the valid ones?
This means potentially making many simulate API calls before batching messages. We can assume that for the high majority, all messages within the batch will be valid. By simulating the batched message first, if it errors, it will break out to individual simulate calls after that. This reduces overall API calls for typical operation under broadcast-mode: batch, and just means one extra API call in the case the batch has an error.

TODO:

  • Make this behavior configurable
  • [] Wire in with fee-grant work (handle as follow up)
  • Hybrid mode where messages can be retried individually in the case the batched transaction fails
  • [] Cleanup by pulling the new broadcast logic into lens (or vise versa?) (handle as follow up)

Example showing multiple txs for the same relayer wallets in the Stride ICQ interchaintest case during ICA channel handshake (4 at once):
Screenshot 2023-02-05 at 12 30 49 AM

@agouin agouin marked this pull request as ready for review February 8, 2023 00:49
Copy link
Member

@jtieri jtieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything here looks correct!

I had a few comments, most of which could probably be addressed in follow up PRs but I'll block on approving for now until you are back in office and respond

relayer/chains/cosmos/tx.go Show resolved Hide resolved
relayer/processor/path_processor_internal.go Outdated Show resolved Hide resolved
relayer/processor/path_processor_internal.go Outdated Show resolved Hide resolved
relayer/processor/path_processor_internal.go Outdated Show resolved Hide resolved
relayer/processor/path_processor_internal.go Outdated Show resolved Hide resolved
Copy link
Member

@jtieri jtieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! You did a great job on the refactor.

I had the one comment here but I don't think it's worth blocking approval on. It could be argued that this is a premature optimization anyways as it seems like we spend more time writing than reading so I will leave it up to you whether to address my feedback or not.

relayer/processor/message_processor.go Show resolved Hide resolved
@agouin agouin merged commit b07fd2a into main Feb 21, 2023
@agouin agouin deleted the andrew/broadcast_sync_multiple_per_block branch February 21, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants