feat(hyp-ethereum): gas escalator middleware #3852
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3852 +/- ##
==========================================
- Coverage 60.56% 0.00% -60.57%
==========================================
Files 129 1 -128
Lines 1656 14 -1642
Branches 176 0 -176
==========================================
- Hits 1003 0 -1003
+ Misses 626 14 -612
+ Partials 27 0 -27
|
There was a problem hiding this comment.
After diving a bit deeper, imo like many things in ethers, it feels like not many people actually use these middlewares in production / long-running services, and that it may need some work to meet our bar
The worst thing is that it doesn't seem to have EIP-1559 support at all (see #3852 (comment)), and it returns errors if you try to use this middleware with 1559 txs. This feels like a dealbreaker imo in its current state - either we'd need to change it to just ignore 1559 txs but still allow them, or we'd need to change it to accommodate 1559 txs.
Though pls correct me if I'm wrong here because I would've expected e2e to fail if this were the case? Maybe the transactions submitted by the relayer on e2e are all legacy transactions?
Some other things that make me nervous:
- In our fork, it holds the lock across potentially a lot of async operations, which feels questionable: https://github.com/hyperlane-xyz/ethers-rs/blob/master/ethers-middleware/src/gas_escalator/mod.rs#L177
In upstream this seems to have been fixed fwiw https://github.com/gakonst/ethers-rs/blob/master/ethers-middleware/src/gas_escalator/mod.rs
-
The task can silently error https://github.com/hyperlane-xyz/ethers-rs/blob/master/ethers-middleware/src/gas_escalator/mod.rs#L223 (I think it's still possible to hit this edge case Investigate ethers-rs middleware for "stuck" txs #2959 (comment)). Upstream at least it indicates that's happening https://github.com/gakonst/ethers-rs/blob/master/ethers-middleware/src/gas_escalator/mod.rs#L343
-
We'd probably want some way to test things to certainly hit the code path that results in a gas escalation - not sure if you did this, but being able to test this in e2e or at least ad-hoc would be nice to derisk things
rust/chains/hyperlane-ethereum/src/rpc_clients/trait_builder.rs
Outdated
Show resolved
Hide resolved
rust/chains/hyperlane-ethereum/src/rpc_clients/trait_builder.rs
Outdated
Show resolved
Hide resolved
rust/chains/hyperlane-ethereum/src/rpc_clients/trait_builder.rs
Outdated
Show resolved
Hide resolved
|
E2E fails because validators can't send the announce txs. The error is This means that the chain ID in the txData is different from the chain ID in the signature for some reason. Should look into why |
…yz/hyperlane-monorepo into dan/gas-escalator-middleware
Fixes some issues @tkporter brought up in hyperlane-xyz/hyperlane-monorepo#3852 (review): 1. Holding mutex lock across gas escalations (fixed upstream, I just merged the changes) 2. Silently ignoring nonce related errors: added a `warn` log. These error are only expected to occur if the original tx landed onchain though 3. I ran the unit test that comes with this middleware, and similar to what Trevor pointed out, sending a tx returns a `PendingTransaction` whose tx hash will not reflect the hash of the tx that lands onchain. However I manually checked by polling anvil and emitting logs that the gas is escalated and the final tx is finalized
rust/chains/hyperlane-ethereum/src/rpc_clients/trait_builder.rs
Outdated
Show resolved
Hide resolved
rust/chains/hyperlane-ethereum/src/rpc_clients/trait_builder.rs
Outdated
Show resolved
Hide resolved
This reverts commit c3c002a.
…4098) This reverts commit c3c002a. ### Description The gas escalator took up to 28gb of memory, and even though it was only ever released on RC, it must have taken up too many cluster resources, ending up starving the `hyperlane` relayer. Reverting for now until we have a better idea ### Drive-by changes <!-- Are there any minor or drive-by changes also included? --> ### Related issues <!-- - Fixes #[issue number here] --> ### Backward compatibility <!-- Are these changes backward compatible? Are there any infrastructure implications, e.g. changes that would prohibit deploying older commits using this infra tooling? Yes/No --> ### Testing <!-- What kind of testing have these changes undergone? None/Manual/Unit Tests -->
### Description Follow up to #3852 and #4098. This PR brings major fixes to the escalator: - A revamped escalator implementation, that was forked from ethers-rs and had various fixes applied to it. The most notable bug caused "infinite" gas price escalations to occur within a very short timespan, until the max configured gas price was reached. Full diff here: hyperlane-xyz/ethers-rs@950dd34...edf703a - Support for escalating EIP-1559 txs, in addition to the existing Legacy tx type support - Memory leak fix. Since each escalator instance spawns a thread and we create transient providers for each message, this created a memory leak of escalator tasks that would build up over time - on RC, memory usage had reached 27gb before we noticed. This PR merges in the changes from #4208 to stop the leak. Profiling results: - before the fix (62mb peak usage):  - after the fix (16mb peak usage):  ### Drive-by changes <!-- Are there any minor or drive-by changes also included? --> ### Related issues - Fixes #4108 ### Backward compatibility Yes ### Testing Manual testing using the unit test setup from ethers-rs, and using it on RC to observe it in the wild. Prom chain returns a `could not replace existing tx` error when a tx is resubmitted so the escalator won't be effective there, but this can be fixed later --------- Co-authored-by: Trevor Porter <tkporter4@gmail.com>
### Description Follow up to hyperlane-xyz#3852 and hyperlane-xyz#4098. This PR brings major fixes to the escalator: - A revamped escalator implementation, that was forked from ethers-rs and had various fixes applied to it. The most notable bug caused "infinite" gas price escalations to occur within a very short timespan, until the max configured gas price was reached. Full diff here: hyperlane-xyz/ethers-rs@950dd34...edf703a - Support for escalating EIP-1559 txs, in addition to the existing Legacy tx type support - Memory leak fix. Since each escalator instance spawns a thread and we create transient providers for each message, this created a memory leak of escalator tasks that would build up over time - on RC, memory usage had reached 27gb before we noticed. This PR merges in the changes from hyperlane-xyz#4208 to stop the leak. Profiling results: - before the fix (62mb peak usage):  - after the fix (16mb peak usage):  ### Drive-by changes <!-- Are there any minor or drive-by changes also included? --> ### Related issues - Fixes hyperlane-xyz#4108 ### Backward compatibility Yes ### Testing Manual testing using the unit test setup from ethers-rs, and using it on RC to observe it in the wild. Prom chain returns a `could not replace existing tx` error when a tx is resubmitted so the escalator won't be effective there, but this can be fixed later --------- Co-authored-by: Trevor Porter <tkporter4@gmail.com>
### Description Follow up to hyperlane-xyz#3852 and hyperlane-xyz#4098. This PR brings major fixes to the escalator: - A revamped escalator implementation, that was forked from ethers-rs and had various fixes applied to it. The most notable bug caused "infinite" gas price escalations to occur within a very short timespan, until the max configured gas price was reached. Full diff here: hyperlane-xyz/ethers-rs@950dd34...edf703a - Support for escalating EIP-1559 txs, in addition to the existing Legacy tx type support - Memory leak fix. Since each escalator instance spawns a thread and we create transient providers for each message, this created a memory leak of escalator tasks that would build up over time - on RC, memory usage had reached 27gb before we noticed. This PR merges in the changes from hyperlane-xyz#4208 to stop the leak. Profiling results: - before the fix (62mb peak usage):  - after the fix (16mb peak usage):  ### Drive-by changes <!-- Are there any minor or drive-by changes also included? --> ### Related issues - Fixes hyperlane-xyz#4108 ### Backward compatibility Yes ### Testing Manual testing using the unit test setup from ethers-rs, and using it on RC to observe it in the wild. Prom chain returns a `could not replace existing tx` error when a tx is resubmitted so the escalator won't be effective there, but this can be fixed later --------- Co-authored-by: Trevor Porter <tkporter4@gmail.com>
Description
Wraps evm providers in a gas escalator middleware as part of the
build_with_signerstep, so signed txs can have their gas price bumped if stuck in the mempool.Gas price is increased every 60s by 12.5%, and is capped at the
max_fee_per_gasset in tx overrides.Drive-by changes
ethers:M: Clonebound, since it's not actually required and conflicts with the middleware we useescalatedlog in the middleware todebug(so we can see it working in prod without enablingtrace)Related issues
Backward compatibility
Yes
Testing
e2e