Support for priority mempool chains

[ancazamfir]

Summary

Starting with tm v0.34.20 we may see cosmos-sdk chains using priority enabled mempool. We need to make sure we support these chains.

Problem Definition

Here is my understanding about how prio mempool will be used from hermes pov.

Let’s say hermes submits 2 Tx-es, tx1 and tx2, with incrementing nonces, using broadcast_tx_sync. The application runs checkTx for each of them and returns an abci.ResponseCheckTx that includes a priority. The prio mempool then stores this and when it’s time to create a block it picks Tx-es in priority order.
In sdk chains, during checkTx, the priority is set by a TxFeeChecker and the default is defined in the auth module: https://github.com/cosmos/cosmos-sdk/blob/main/x/auth/ante/validator_tx_fee.go.

In this case the priority is computed based on the Tx fee. Different apps/chains will have the ability to overwrite this handler.
But staying with the default behavior for now, it is possible that tx2 will be included in a block before tx1, so deliverTx(tx2) will be executed first and will fail with account sequence mismatch. tx1 will be successful.

If we try to use multiple wallets to go around the account sequence mismatch we will still see issues, e.g.:

• tx2 may be rejected if tx1 includes a client update needed for msgs in tx2, or
• the msgs in the tx-es are for ordered channels.

While we push for a solution in tendermint or SDK (e.g. to not reorder Tx-es from same account or allow out of order execution during deliverTx), we should remove our assumption that Tx-es are executed in order, figure out all the cases where we are exposed to this and how to handle them.
For the examples above, include update client in every tx (see also #2191), don’t send multiple Tx-es for ordered channels in same block (i.e. wait for tx1 hash to show up in a block before sending tx2), etc.

Acceptance Criteria

hermes works with prio mempool chains

• depends on Update tendermint-rs to v0.23.8 #2455
• update compatibility.rs to mark compat w/ sdk 0.46
  • https://github.com/informalsystems/ibc-rs/blob/5df35478607452baa8db004efb1d7aa38dfd8d89/relayer/src/chain/cosmos/compatibility.rs#L8-L23

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate milestone (priority) applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[adizere]

An update on this:

I've released tendermint-rs v0.23.8-pre.1 to cater for the backported prioritized mempool released in Tendermint v0.34.20-rc0: https://github.com/informalsystems/tendermint-rs/releases/tag/v0.23.8-pre.1 (basically just adds 3 fields to the CheckTx result that are only relevant if the prioritized mempool is enabled). See informalsystems/tendermint-rs#1148 for more details.

The Tendermint team's working on fixing a bug in the prioritized mempool at the moment: tendermint/tendermint#8775
Once that's resolved, they'll cut another RC, do some more extensive testing with the SDK, and then finally cut a v0.34.20 release. Once that's out, the SDK team is keen to roll that out ASAP.

[mzabaluev]

Notes from today's meeting:

Issues raised

• There is a size overflow problem in cosmos/batch.rs: the batch is allowed to grow past the limit by the length of one message. Not related to prio mempool, but needs to be fixed just the same, as this causes the tx to fail due to fee miscalculation (Osmosis tx fee calculation issues #2422) (Hermes may break the max_tx_size configuration param #2477)

• Moving the batching logic to the worker:

  • Max size checking also needs to be moved, in addition to the solution in Insert ClientUpdate message into every transaction when splitting a message batch #2191 (comment)
  • Need to do this with the full encoding of SignedTx (when batching for Cosmos chains), can't with just Vec<Any>.

Solutions proposed

• Add method to the chain trait to evaluate tx size from the Vec<Any>. The worker will use this method in the batching logic.
• The worker should be solely responsible for forming tx batches and inserting client updates.

[soareschen]

Here is a summary from recent discussions:

Interaction with account sequence in Cosmos SDK

• At the moment it is unclear how the prioritized mempool would interact with the nonce mechanism in Cosmos SDK.
• For example:
  • Given tx1 and tx2 are in the mempool at the same time with account sequences N and N+1, with the current nonce being N.
  • If tx2 has higher priority than tx1, then tx2 will get processed first. But tx2 will fail because of nonce mismatch.
    • It is unclear if tx1 will subsequently be processed successfully with matching nonce?
  • A possible fix is for Cosmos SDK to allow for the use of any nonce greater than or equal to the current nonce.
  • But if tx2 is accepted, then tx1 would still fail with account sequence mismatch error because of the updated nonce.
• As a result, it is never safe to have two transactions by the same sender in the mempool at any time.
• A possible workaround is to require ABCI applications like Cosmos SDK to always give higher priority to transactions with lower account sequence.
  • But it is uncertain how this can work in a stateless manner, unless a fully application-specific mempool is implemented.

Sender-specific limitation on Tendermint

• Tendermint v0.34.20 will allow only one transaction per sender in the mempool at any time.
• The sender is determined by a Tendermint-specific sender field inside the transaction.
• However, Cosmos SDK do not currently set the sender field to any value, i.e. the sender is always "".
• Tendermint handles a special case of allowing unlimited transactions in the mempool when the sender value is "".
• Therefore for SDK chains, there is currently no limit of how many transactions per wallet can be in the mempool at the same time.
• However, it is unclear whether Cosmos SDK will set the sender field in the future.
• It is also unclear if there is any other non-SDK ABCI applications that make use of the sender field.
• We need more documentation that describe in detail how the sender field works in Tendermint.

Strategies for IBC Relayers

• With the introduction of prioritized mempool, it does not seem to be safe for the relayer to submit more than one transactions in parallel to the mempool.
  • Having priority set in the wrong ordering will result in account sequence mismatch error for one or both transactions.
  • Even if both transactions are somehow accepted in different orders, tx2 would still fail if it depends on the UpdateClient message from tx1 to be committed first.
  • Parallel transactions will get outright rejected for non-SDK chains that make use of the sender field in Tendermint.
• The best course of action in the short term is to submit one transaction at a time to the mempool, and only submit the next transaction when the current transaction is committed.
  • This can be done with minimal changes in the ibc-rs code base by modifying the batching algorithm inside send_messages_and_wait_commit to submit batch transactions one by one, instead of submitting them all at once and then waiting for the DeliverTX result.
  • Depending on the UX decision, we may want to make this configurable so that relayer operators can still make use of parallel transactions if applicable.
• This is not the most optimal strategy, but we can come out with a better strategy after we confirm that the simple strategy fixes the issue and merge the changes.

Testing Strategy

• Tendermint v0.34.20 has been used in ibc-go v5.0.0-beta1. So we can test the relayer against the new behavior by running the integration test against the corresponding simd.
• Cosmos.nix support for simd from ibc-go v5.0.0-beta1 has been merged in Update core Cosmos packages cosmos.nix#90.
• Integration with ibc-go v5.0.0-beta1 is being done in Update Nix and CI dependencies #2523. However there are other integration tests that are failing.
  • The failed tests are: test_clear_packet_recovery and test_supervisor.
  • The failures might or might not be related to prioritized mempool.
  • Further investigation is needed to triage the failures.
• After the initial integration is done, we should also write more integration tests to test for the behavior of prioritized mempool. Some rough ideas:
  • Set the chain block time in test to 5s.
  • Try and send multiple transactions by manually setting the account sequences and then directly submitting them to the chain without going through ChainHandle.
    • This can be done by spawning multiple async tasks and calling simple_send_tx in parallel.
  • Attempt to manipulate tx2 to have higher priority than tx1. It is unclear how this can be done, but we can try a few strategies
    • Make tx2 contain more messages than tx1.
    • Set a higher fee in tx2.
  • Find ways to determine if tx2 is committed before tx1.
    • We can probably find this by querying the transaction history for a wallet?
  • Determine if tx2 is committed before tx1, would the anticipated errors be raised?
    • Will we get account sequence mismatch errors?
    • Or will tx2 be rejected from the mempool?
  • Run the same integration test against earlier versions, e.g. Gaia v6, and see if the test produce different results.
  • Write test assertions based on the observed behavior, so that CI can detect if the behavior ever changes.

[soareschen]

Here is a summary of recent discussions with the Tendermint and Cosmos SDK teams on this issue:

• Priority Mempool has a per-node configuration.
• The default configuration is to have priority mempool disabled.
• The effect of whether priority mempool is used depends on whether the configuration is enabled on the proposer validator node in each block.
• If transactions are reordered by priority, the transaction with mismatching nonce will be rejected by Cosmos SDK.
• Tendermint has a sender field that limits one transaction per sender in the mempool, but Cosmos SDK do not set the sender field.
• Cosmos SDK sets all transaction priorities to 0 by default.
• Priority mempool will be superceded by ABCI++.
  • However similar issues will be present with applications using ABCI++.
• It is not possible to peek into message content and disable priority for IBC messages.
  • Because priority can be overridden by applications on the transaction level.
• There is currently no clear use cases on which chain would likely activate priority mempool.
  • It might be useful in the future for features like oracles
• Priority mempool is likely not needed by major chains like Cosmos Hub and Osmosis.

Short Term Recommendation

• To keep the Hermes relayer working smoothly with Tendermint v0.34.20, it is highly recommended to disable priority mempool for all full nodes, in particular validator nodes for chains supporting IBC.
  • Since priority mempool is disabled by default, there may be no explicit action needed on this step.
• This should not have any major impact, unless there is a compelling case for a chain to enable priority mempool.
  • If a chain wants to make use of priority mempool and also use Hermes for IBC relaying, note that IBC relaying may break in unexpected ways. The developers for such chain are strongly advised to contact the Hermes team to explore for alternative solutions.

Long Term Plans

• Supporting priority mempool and ABCI++ will require major architecture overhaul for the Hermes relayer.
• The best course of action is to use multiple wallets for parallel transactions being submitted to the chain.
  • Each wallet should have at most one transaction in the mempool at a time.
  • To support multiple wallets, data structures such as Any will need to be modified to allow the signer fields to be updated.
  • The ChainHandle trait will need to support the use of multiple wallets, as compared to using a globally configured wallet that cannot be changed.
• To provide smooth experience for relayer operators, the relayer can automatically create multiple burner wallets and use the fee grant feature to allow the burner wallets to spend fees from the main wallet.
• On the analytics side, we will need to investigate ways associate the burner wallets with relayer operators.
• Submitting parallel transactions using multiple wallets will require different strategies for submitting UpdateClient messages.
  • It is not possible to require an IBC transaction to depend on another IBC transaction containing the UpdateClient message submitted by a different wallet.
  • The UpdateClient message will have to either:
    • Be committed before the transactions that depend on it, which increases latency, or
    • Be included in all transactions that depend on it, which increases cost.
  • This will likely require significant redesign of how the packet worker operates.
• In the worst case, if we ever need to support priority mempool with the current relayer architecture, we can expose a configuration that make send_messages_and_wait_commit send batched messages in serial instead of in parallel.
  • This may cause significant performance degration, but it is better than having IBC relaying broken on chains that want to disregard our recommendation and enable priority mempool.

[adizere]

Update: It seems Umee and Cronos are two networks that confirmed they intend to enable priority mempool.

• https://github.com/cosmos/chain-registry/blob/master/umee/chain.json
• https://github.com/cosmos/chain-registry/tree/master/cronos

The cronos network has no entry on mintscan or IBC assets tagged in the chain-registry. The crypto.org team is managing everything relaying-related themselves it appears.

I had a look through the mintscan IBC data for Umee, the relevant part is the channel Umee <> Osmosis https://www.mintscan.io/umee/relayers/channel-0 which has high traffic & will likely be impacted.

After speaking with Robert Z. from Umee team, we decided on the following:

1. the Umee team will disable tx prioritzation for non-oracle-transactions, which is possible to do at the application level. Validators & node operators on Umee will enable prio mempool, which is important for oracle price feeds, and those transactions will have top priority; all other transactions will retain their default ordering (will not be prioritized by their fees).
2. once the Umee 0.46-based testnet enables IBC, they will give us access to a full node and a wallet and we'll conduct some basic tests to confirm IBC relaying with Hermes still works.

[yihuang]

Apparently, the v1 mempool keeps FIFO behavior when the priorities are the same, so one short-term solution could be to make sure the relayer transactions end up with a static priority value, for example:

1. With a custom TxFeeChecker on app side, one can assign a static priority to the tx which includes IBC messages.
2. relayer carefully set gas prices so the resulting priority is static
   1. the SDK default mechanism compute priority based on fee amount (I think it's more reasonable to based on gas price), one can set a constant fee amount to have a constant tx priority.
      Actually if the SDK compute priority based on gas price, then problem solved, because relayer already uses a static gas price which will translate to a static priority.
   2. in ethermint and cronos, we applied EIP-1559 like feemarket to cosmos tx, where one can set the max_priority_price to a fixed value and set gas_price to maxint, the resulting priority would also be a fixed value, the effective gas price would be base_fee + max_priority_price. This will require some customization to the tx building in the relayer.

[tomtau]

include extension option ExtensionOptionDynamicFeeTx and set the max_priority_price to a constant value, set the gas_price to maxint, so it'll end up with a constant priority value.

@adizere @soareschen can this workaround be added to Hermes in the short term?

[yihuang]

include extension option ExtensionOptionDynamicFeeTx and set the max_priority_price to a constant value, set the gas_price to maxint, so it'll end up with a constant priority value.

@adizere @soareschen can this workaround be added to Hermes in the short term?

opened an issue which includes a simple proposal. #2566

[ancazamfir]

Apparently, the v1 mempool keeps FIFO behavior when the priorities are the same, so one short-term solution could be to make sure the relayer transactions end up with a static priority value, for example:

1. With a custom TxFeeChecker on app side, one can assign a static priority to the tx which includes IBC messages.

Not sure how easy this is as the checker needs to decode the Tx and figure out the type of messages included. What should be the prio if there are also non IBC messages?

2. relayer carefully set gas prices so the resulting priority is static

   1. the SDK default mechanism compute priority based on fee amount (I think it's more reasonable to based on gas price), one can set a constant fee amount to have a constant tx priority.

The problem is that this fee needs to be paid and should be enough to cover the actual Tx handling cost. And this constant fee needs to cover the cost of every Tx, big or small, that the relayer submits. It will result in high fees for relayers.

Actually if the SDK compute priority based on gas price, then problem solved, because relayer already uses a static gas price which will translate to a static priority.

We should wait and see if SDK goes this way, I thought the idea is to give higher prio to higher fee Tx-es.

2. in ethermint and cronos, we applied EIP-1559 like feemarket to cosmos tx, where one can set the max_priority_price to a fixed value and set gas_price to maxint, the resulting priority would also be a fixed value, the effective gas price would be base_fee + max_priority_price. This will require some customization to the tx building in the relayer.

Could you please clarify? what is base_fee? And is base_fee + max_priority_price same as gas_price? Do you plan to use gas price to set the Tx the priority, as suggested in cosmos/cosmos-sdk#12932?

[yihuang]

Not sure how easy this is as the checker needs to decode the Tx and figure out the type of messages included

It's pretty easy to do in TxFeeChecker, where it gets passed a sdk.Tx, it just needs to iterate the tx.GetMsgs().

What should be the prio if there are also non IBC messages?

We can single out the txs which only contains IBC messages, which I assume is the case with relayers.
For the other custom clients who also relay IBC messages, they'll need to be aware of the priority thing, and manage it with gas price.

The problem is that this fee needs to be paid and should be enough to cover the actual Tx handling cost. And this constant fee needs to cover the cost of every Tx, big or small, that the relayer submits. It will result in high fees for relayers.

There was discussions about should we set priority based on msg types or gas price/fee, there was no consensus on that, so the default implementation built in the SDK is only a minimal one.
Fortunately, it should be straightforward enough to customize the TxFeeChecker.
If it's important to prioritize the IBC message without requiring a higher fee, and it also doesn't introduce DoS vulnerability, one can do that with a custom TxFeeChecker.

We should wait and see if SDK goes this way, I thought the idea is to give higher prio to higher fee Tx-es.

Yeah, I think the default implementation in SDK is only like an MVP, the chains should think more about their own feemarket design, for example, the ethermint will use the ethereum-like design conveniently. But this brings us to the issue of a flexible way to build the tx in the clients.

Could you please clarify? what is base_fee? And is base_fee + max_priority_price same as gas_price? Do you plan to use gas price to set the Tx the priority, as suggested in cosmos/cosmos-sdk#12932?

it follows the design of EIP-1559 of ethereum, the base-fee is an in-protocol gas price that is constantly adjusted based on the gas usage of the previous block.
And user supplies two fields, a) feeCap which is the maximum gas price to pay, b) prioFeeCap which is the maximum priority gas price to pay, the relationship between them is like this:

require(feeCap >= baseFee)
effectiveGasPrice = min(baseFee + prioFeeCap, feeCap)
priority = effectiveGasPrice - baseFee
effectiveFee = effectiveGasPrice * gas limit  # the actual fee to pay

When apply to cosmos-sdk tx, we reuse the existing fees / gas limit as the feeCap, and define an extension option for the user to input the prioFeeCap.
So if client want to have a constant priority, it can set prioFeeCap to a constant value, and feeCap to a maximum value, then the priority will always be prioFeeCap, and gas price to pay will be baseFee + prioFeeCap, which is adjusted based on block gas usage.

[ancazamfir]

Thanks for the pointer and notes!

There was discussions about should we set priority based on msg types or gas price/fee, there was no consensus on that, so the default implementation built in the SDK is only a minimal one.
Fortunately, it should be straightforward enough to customize the TxFeeChecker.

So a solution involving the TxFeeChecker (*) is to require the default and ANY custom/ app specific implementations to result in relayer Tx-es having same priority. You propose doing this by computing the priority based on the "effective" gas price and not the gas limit or amount. This of course would work for the relayer and any actor trying to submit multiple Tx-es from same account.
But many operators also don't want to pay more than what they pay today (which is already quite a lot). This would mean allowing setting prioFeeCap to zero (i.e. no need for the extra hermes configuration) or something close to zero, with the understanding that IBC Tx-es will have low (can be zero) priority and therefore may be delayed.

Do you plan to enable priority mempool for ethermint? If yes, why (what are the usecases)? And have a custom impl of TxFeeCheckeralong the lines you describe above (ala EIP-1559)?

(*) I still feel we are trying a convoluted solution for a very basic SDK chain UX requirement, the ability to submit multiple Tx-es from same account, with incrementing nonces, to be included in the same block (i.e. never to be reordered regardless of the way priority is computed).

[yihuang]

But many operators also don't want to pay more than what they pay today (which is already quite a lot). This would mean allowing setting prioFeeCap to zero (i.e. no need for the extra hermes configuration) or something close to zero, with the understanding that IBC Tx-es will have low (can be zero) priority and therefore may be delayed.

I think this is a different issue, possibly an orthogonal one, that the fee is too costly for relayer operators, I can see several options:

• lower the gas fee requirement for all tx types
• using ibc incentivization feature to move the cost to the end user
• hardcode in the chain to make IBC messages enjoy higher priority without paying a higher fee, this has the risk that IBC messages become the lowest barrier for DoS attack.

Do you plan to enable priority mempool for ethermint? If yes, why (what are the usecases)? And have a custom impl of TxFeeCheckeralong the lines you describe above (ala EIP-1559)?

yeah, that's the plan, together with the EIP-1559.

what are the usecases

I think the main use case for prioritized mempool is when the chain is congested, the users who are willing to pay higher price can get through faster.
And the use case for EIP-1559 is it maintains a base-fee which is adjusted according to block usage, so if the chain usage is low, the fee will drop automatically.

[yihuang]

(*) I still feel we are trying a convoluted solution for a very basic SDK chain UX requirement, the ability to submit multiple Tx-es from same account, with incrementing nonces, to be included in the same block (i.e. never to be reordered regardless of the way priority is computed).

I've proposed a tiered-price-system before, where client specify the priority(tier of service) explicitly, different tier has different gas price requirement, and within the same tier, it's FIFO. I still think this design has some merits, and we might use it on our main chain (which don't use ethermint). For ethermint, it makes sense to keep it compatible with ethereum.

[yihuang]

We should wait and see if SDK goes this way, I thought the idea is to give higher prio to higher fee Tx-es.

cosmos/cosmos-sdk#12953

[soareschen]

#2543 is merged, but haven't fully resolved the workaround. This is because the relay_from_operational_data function is using the AsyncSender, which calls send_messages_and_wait_check_tx instead of send_messages_and_wait_commit. We would need to also modify the RelayPath to switch to use SyncSender if sequential_batch_tx is set to true.

I have also filed cosmos/cosmos-sdk#13009 to ask for the possibility to introduce parallel nonces to Cosmos SDK. If that option is available, it would be much easier for us to support concurrent transactions.

[yihuang]

We should wait and see if SDK goes this way, I thought the idea is to give higher prio to higher fee Tx-es.

cosmos/cosmos-sdk#12953

This PR is merged, may I know if it fixes the relayer issues with prioritized mempool now?

[adizere]

This PR is merged, may I know if it fixes the relayer issues with prioritized mempool now?

I think the prio mempool issue is fixed as a temporary workaround in #2543. We plan to extend that solution with an approach that is more resilient to errors or corner-cases (eg #2565 and the comment above #2350 (comment)).

[yihuang]

This PR is merged, may I know if it fixes the relayer issues with prioritized mempool now?

I think the prio mempool issue is fixed as a temporary workaround in #2543. We plan to extend that solution with an approach that is more resilient to errors or corner-cases (eg #2565 and the comment above #2350 (comment)).

I thought the issue with prioritized mempool is relayer txs get reordered, so if it keep the FIFO behavior with relayer txs, it's work just like before? And in recent sdk when using static gas price, the tx will have static priority, which will have get a FIFO treatment.

[adizere]

I thought the issue with prioritized mempool is relayer txs get reordered, so if it keep the FIFO behavior with relayer txs, it's work just like before? And in recent sdk when using static gas price, the tx will have static priority, which will have get a FIFO treatment.

This is correct. Networks that guarantee FIFO ordering do not need the fix we implemented in #2543. Things should work as usual for those networks as long as mempool FIFO ordering is retained.

In order for Hermes to cope (or allow) cases when FIFO mempool is not guaranteed, we added #2543 (introducing an additional configuration parameter that adjusts Hermes behavior to submit transactions serially); in this case, the fix is partial and temporary. This is what my earlier comment refers to #2350 (comment). Hope this clarifies things.