hyperlane-xyz / hyperlane-monorepo

The home for Hyperlane core contracts, sdk packages, and other infrastructure
https://hyperlane.xyz
Other
295 stars 312 forks source link

On chain fee quoting: relayers cap the destination tx gas amount at what a message has paid for #1535

Closed tkporter closed 1 year ago

tkporter commented 1 year ago

At the moment, the agents will only index the message_id and payment for InterchainGasPayments: https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/main/rust/hyperlane-core/src/types/mod.rs#L23

However, in v2, we also emit the gasAmount (see here https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/370e3e768a9d2c8d91459e6489ce41674c6165d9/solidity/contracts/InterchainGasPaymaster.sol#L25). This is an important bit of on chain fee quoting, where the gasAmount emitted in that InterchainGasPayment event is the amount of gas on the destination that has been paid for

So concretely, we should:

  1. Start indexing the gas_amount by adding it to the InterchainGasPayment struct and the Ethereum contract_sync. If we observe 2 interchain gas payments for the same message, just add the gas_amount to the previously saved amount
  2. Add a new GasPaymentEnforcementPolicy to do with on chain fee quoting. Maybe even OnChainFeeQuoting, or GasAmount or something? Idk about the name, just something to illustrate that it pertains to on chain fee quoting, or that it relates to observing the gas amount that a message has paid for and using that as an upper bound on how much gas to use in the process tx
  3. For this new policy, we want to ensure that the process transaction wouldn't use more than the gas_amount from the db's InterchainGasPayment for the message. I'll dive into some weird bits below...

The naive way of doing this would be a simple bool tx_cost_estimate.gas_limit <= interchain_gas_payment_from_db.gas_amount. But because the tx cost estimate is just an estimate, it's possible for that gas limit to be estimated as much higher than the amount of gas that the tx actually requires. Here's an example:

In this example, it's estimating the gas to just send a super basic ETH transfer (albeit on Celo, but it's just sending 0xff from a random address I found to itself). This should only cost 21,000 gas, but eth_estimateGas gives us 27300 gas, which is 30% higher than 21k:

$ cast rpc eth_estimateGas '[{"from": "0xe37f80f22f43579edce961563f793031d00bfa9f", "to": "0xe37f80f22f43579edce961563f793031d00bfa9f", "value": "0xff"}, "latest"]' --raw --rpc-url https://forno.celo.org
"0x6aa4"
$ cast td 0x6aa4
27300

And we can see that we don't get an error if we eth_call with 21,000 gas:

$ cast th 21000
0x5208
$ cast rpc eth_call '[{"from": "0xe37f80f22f43579edce961563f793031d00bfa9f", "to": "0xe37f80f22f43579edce961563f793031d00bfa9f", "value": "0xff", "gas": "0x5208"}, "latest"]' --raw --rpc-url https://forno.celo.org
"0x

And as a sanity check, if we try 20,999 gas we do get an error:

$ cast rpc eth_call '[{"from": "0xe37f80f22f43579edce961563f793031d00bfa9f", "to": "0xe37f80f22f43579edce961563f793031d00bfa9f", "value": "0xff", "gas": "0x5207"}, "latest"]' --raw --rpc-url https://forno.celo.org
Error:
(code: -32000, message: err: intrinsic gas too low: have 20999, want 21000 (supplied gas 20999), data: None)

I think the most "correct" solution is something like:

  1. Plumb things in a way such that from the GasPaymentPolicy we can make an eth_call with the exact same params as what was originally used for the eth_estimateGas, but with the gas limit as gas_amount from the db's InterchainGasPayment for the message
  2. If that's successful, change the code such that the GasPaymentPolicy can return an Option<U256> or something indicating a special gas limit to use on the process tx

But there's an annoying issue here -- the RetryingProvider will retry the eth_call errors in the case where the gas amount is too low -- we have no way of indicating that we are totally happy to get errors from that eth_call :(

So I think this leaves us with a couple options...

  1. Some greater effort around refactoring things such that we can make eth_calls in a clean way without retrying
  2. Edit the RetryingProvider such that if the request is an eth_call with a gas limit specified, we don't retry (I believe in every other situation where we make eth_calls, we don't set a gas limit). This feels like a lAyEr ViOlAtIoN though and potentially fragile...
  3. Go with an 80/20 solution for now, which can be something like: as long as the gas limit from the eth_estimateGas is within 30% of the amount of gas paid for from InterchainGasPayments, consider everything as fine. And then we can figure out how to do this more elegantly

I can't think of a way to do 1 without being super inelegant or being a bit of a rabbithole rn. 2 feels the most hacky but also the most "correct" in terms of enforcing gas amounts. 3 is the easiest, but might set bad expectations if we end up processing messages that actually aren't correctly paying for gas, and once we get a more accurate solution their messages may no longer be processed, causing dev confusion

tkporter commented 1 year ago

For the record I think we should do option 2 or 3