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

Relayer double applies a gas buffer for txs, and uses post-buffer gas amounts when enforcing IGP gas amounts #5061

Open
Tracked by #3321
tkporter opened this issue Dec 20, 2024 · 0 comments
Assignees
Labels

Comments

@tkporter
Copy link
Collaborator

tkporter commented Dec 20, 2024

Problem

See discord thread https://discord.com/channels/935678348330434570/1319115367167033399, juicy bits copied:

Problem 1 - double applying a gas buffer for txs

Found what seems like a bug where we double apply the tx buffer when submitting a tx. We still only apply the buffer once when using the amount to compare for gas payment enforcement, but it does mean we always end up paying 150k gas more than the estimate, which doesn't feel intentional?

Looking at this message https://explorer.hyperlane.xyz/message/0x49ab3d0831517ee53c998f59c5732dd1e44f937eb746bb152959e475069e5368

Logs around here https://cloudlogging.app.goo.gl/nYfKhujwm1smLSL26

When preparing:

  1. We do an eth_estimateGas and get back 295432 (while this isn't the true source, that number can be seen in the zks_estimateFee here https://cloudlogging.app.goo.gl/ehvYD149nEwwGXhW8)
  2. We add the 75k buffer due to apply_gas_estimate_buffer and set the gas limit for the transaction to 370432 (https://cloudlogging.app.goo.gl/ee9yJsz72n9tKJmX7)
  3. We save this gas limit https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/main/rust/main/agents/relayer/src/msg/pending_message.rs#L334
  4. We now have prepared the msg and send it to the submit queue

Now when submitting:

  1. First we do another estimate, just to make sure that it still won't revert. Here we again get a gas limit of 370432 (https://cloudlogging.app.goo.gl/iupm4erLr4mETgvT7, https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/main/rust/main/agents/relayer/src/msg/pending_message.rs#L358)
  2. Now we pass in the saved gas limit of 370432 when calling mailbox.process (https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/main/rust/main/agents/relayer/src/msg/pending_message.rs#L371)
  3. We again add 75k to this saved gas limit of 370432, giving us 445432 due to the apply_gas_estimate_buffer call in tx.rs fill_tx_gas_params (https://cloudlogging.app.goo.gl/fWrGnyNUHBA589x77, https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/main/rust/main/chains/hyperlane-ethereum/src/tx.rs#L129)

Problem 2 - using post-buffer gas amounts when enforcing IGP gas amounts

We compare an IGP payment's gas amount to the gas limit of a tx after a buffer has been applied.

I notice that we apply the buffer (https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/main/rust/main/chains/hyperlane-ethereum/src/tx.rs#L36) to the gas estimate in process_estimate_costs

Call path goes process_estimate_costs -> add_gas_overrides -> fill_tx_gas_params -> apply_gas_estimate_buffer

imo the relayer's agreement is "i'll require users to pay for a gas amount that is used on the destination" not "i'll require users to pay for a gas amount that is what the destination chains estimate is, even if the estimate is inflated"

Solution

  • Fix both these issues, copying some suggestions from that thread:

For the double payment, I suggested:

Not 100% sure on the fix, maybe if tx.tx.gas() is Some in fill_tx_gas_params we don't apply the buffer?

Dan's suggestion:

I think our inferred / desired behavior is that we only fill_gas_params before submitting but keep the estimate
so in this function we can add a boolean flag based on which add_gas_overrides is called or not
so an even shorter-term fix is to remove that prematurely added buffer as you suggested, by requiring process_estimate_costs to take in an additional apply_gas_overrides boolean that it passes to process_contract_call

Whatever solution we go with, it should fix both these issues and be easy to understand - it was confusing to track this flow down

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Sprint
Development

No branches or pull requests

1 participant