-
Notifications
You must be signed in to change notification settings - Fork 425
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
fix(evm): only create signer provider when needed #4208
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4208 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 1 1
Lines 14 14
=====================================
Misses 14 14
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanna make sure batching still works fine? I assume so as we create the multicall here with the Mailbox provider https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/main/rust/chains/hyperlane-ethereum/src/contracts/mailbox.rs#L502
@tkporter batches are still successfully sent in e2e: 2024-07-26T16:39:09.2561535Z <RLY> 2024-07-26T16:39:08.757633Z INFO submit_task: relayer::msg::op_submitter: Submitted transaction batch batch_size=3 outcome=TxOutcome { transaction_id: 0x00000000000000000000000000000000000000000000000000000000000000004d141a1cfa0ba55bc68d066bce86718d35817f3a19530f04de19f88474353f6f, executed: true, gas_used: 386210, gas_price: FixedPointNumber(BigDecimal("1500000008")) } batch=[PendingMessage { num_retries: 0, since_last_attempt_s: 5, next_attempt_after_s: 0, message: HyperlaneMessage { id: 0x0b272ed8392c57e889ca16c5d4a473b4f57d096202ba70f389d1a44a3442223a, version: 3, nonce: 0, origin: test3, sender: 0xb82008565fdc7e44609fa118a4a681e92581e680, destination: test1, recipient: 0xb82008565fdc7e44609fa118a4a681e92581e680, body: 0x1234 }, status: ReadyToSubmit, app_context: None }, PendingMessage { num_retries: 0, since_last_attempt_s: 4, next_attempt_after_s: 0, message: HyperlaneMessage { id: 0xa4f8613651ed250cc0f399c38917122e4b18ddc697abc126908596d90f16c01c, version: 3, nonce: 0, origin: test2, sender: 0xb82008565fdc7e44609fa118a4a681e92581e680, destination: test1, recipient: 0xb82008565fdc7e44609fa118a4a681e92581e680, body: 0x1234 }, status: ReadyToSubmit, app_context: None }, PendingMessage { num_retries: 0, since_last_attempt_s: 6, next_attempt_after_s: 0, message: HyperlaneMessage { id: 0x5305762c0f2dae98812fad3ee21bbe05cf020c1bec362fc2c2c9849799a9e764, version: 3, nonce: 4, origin: test2, sender: 0x870526b7973b56163a6997bb7c886f5e4ea53638, destination: test1, recipient: 0x870526b7973b56163a6997bb7c886f5e4ea53638, body: 0x1234 }, status: ReadyToSubmit, app_context: None }] excluded_ops=[] domain=test1 |
actually a good takeaway from your comment is to add an invariant so the count of |
### 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): ![Screenshot_2024-07-29_at_12 19 26](https://github.com/user-attachments/assets/5b52fecb-6733-481c-96fb-10f505b9bf8a) - after the fix (16mb peak usage): ![Screenshot_2024-07-29_at_12 19 53](https://github.com/user-attachments/assets/a78b9452-78cb-4b5b-9aa6-fe7f3a6ac683) ### 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): ![Screenshot_2024-07-29_at_12 19 26](https://github.com/user-attachments/assets/5b52fecb-6733-481c-96fb-10f505b9bf8a) - after the fix (16mb peak usage): ![Screenshot_2024-07-29_at_12 19 53](https://github.com/user-attachments/assets/a78b9452-78cb-4b5b-9aa6-fe7f3a6ac683) ### 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): ![Screenshot_2024-07-29_at_12 19 26](https://github.com/user-attachments/assets/5b52fecb-6733-481c-96fb-10f505b9bf8a) - after the fix (16mb peak usage): ![Screenshot_2024-07-29_at_12 19 53](https://github.com/user-attachments/assets/a78b9452-78cb-4b5b-9aa6-fe7f3a6ac683) ### 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
Adds an associated const to all ethereum provider builders to specify whether a signer is needed. For an e2e run with 20 messages, we now have 12 providers built with signers at the beginning (3 of which are validator announce), and 708 providers built without a signer. This should hopefully fix the gas escalator memory leak.
Drive-by changes
Related issues
Backward compatibility
Testing