From 982de3b1011602256d9b4cf32940c164724bba4a Mon Sep 17 00:00:00 2001 From: Phillip Ho Date: Thu, 31 Oct 2024 10:36:55 +0800 Subject: [PATCH 1/6] fix: increase gas multiple on each resend attempt --- src/tests/math.test.ts | 24 ++++++++++++++++++++++- src/utils/math.ts | 5 +++++ src/worker/tasks/sendTransactionWorker.ts | 19 ++++++++++-------- 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/src/tests/math.test.ts b/src/tests/math.test.ts index 6303d6bed..dde3941bf 100644 --- a/src/tests/math.test.ts +++ b/src/tests/math.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { getPercentile } from "../utils/math"; +import { clamp, getPercentile } from "../utils/math"; describe("getPercentile", () => { it("should correctly calculate the p50 (median) of a sorted array", () => { @@ -27,3 +27,25 @@ describe("getPercentile", () => { expect(getPercentile(numbers, 50)).toBe(0); }); }); + +describe("clamp", () => { + it("clamps the value correctly below the minimum", () => { + expect(clamp(0, { min: 2, max: 10 })).toBe(2); + }); + + it("clamps the value correctly at the minimum", () => { + expect(clamp(1, { min: 2, max: 10 })).toBe(2); + }); + + it("returns the value when within bounds", () => { + expect(clamp(3, { min: 2, max: 10 })).toBe(6); + }); + + it("clamps the value correctly at the maximum", () => { + expect(clamp(5, { min: 2, max: 10 })).toBe(10); + }); + + it("clamps the value correctly above the maximum", () => { + expect(clamp(6, { min: 2, max: 10 })).toBe(10); + }); +}); diff --git a/src/utils/math.ts b/src/utils/math.ts index 2f693d661..d5af882dd 100644 --- a/src/utils/math.ts +++ b/src/utils/math.ts @@ -7,3 +7,8 @@ export const getPercentile = (arr: number[], percentile: number): number => { const index = Math.floor((percentile / 100) * (arr.length - 1)); return arr[index]; }; + +export const clamp = ( + val: number, + { min, max }: { min: number; max: number }, +) => Math.max(min, Math.min(val * 2, max)); diff --git a/src/worker/tasks/sendTransactionWorker.ts b/src/worker/tasks/sendTransactionWorker.ts index e3e2a514b..9bd8bcc8c 100644 --- a/src/worker/tasks/sendTransactionWorker.ts +++ b/src/worker/tasks/sendTransactionWorker.ts @@ -1,18 +1,18 @@ -import { type Job, type Processor, Worker } from "bullmq"; +import { Worker, type Job, type Processor } from "bullmq"; import assert from "node:assert"; import superjson from "superjson"; import { - type Hex, getAddress, getContract, readContract, toSerializableTransaction, + type Hex, } from "thirdweb"; import { stringify } from "thirdweb/utils"; import { - type UserOperation, bundleUserOp, createAndSignUserOp, + type UserOperation, } from "thirdweb/wallets/smart"; import { getContractAddress } from "viem"; import { TransactionDB } from "../../db/transactions/db"; @@ -33,6 +33,7 @@ import { prettifyError, prettifyTransactionError, } from "../../utils/error"; +import { clamp } from "../../utils/math"; import { getChecksumAddress } from "../../utils/primitiveTypes"; import { recordMetrics } from "../../utils/prometheus"; import { redis } from "../../utils/redis/redis"; @@ -48,8 +49,8 @@ import { reportUsage } from "../../utils/usage"; import { MineTransactionQueue } from "../queues/mineTransactionQueue"; import { logWorkerExceptions } from "../queues/queues"; import { - type SendTransactionData, SendTransactionQueue, + type SendTransactionData, } from "../queues/sendTransactionQueue"; /** @@ -400,18 +401,20 @@ const _resendTransaction = async ( }, }); - // Double gas fee settings if they were not provded in `overrides`. + // Double the gas fee settings each attempt up to 10x. + // Do not update gas if overrides were provided. + const gasMultiple = BigInt(clamp(job.attemptsMade * 2, { min: 2, max: 10 })); if (populatedTransaction.gasPrice) { - populatedTransaction.gasPrice *= 2n; + populatedTransaction.gasPrice *= gasMultiple; } if (populatedTransaction.maxFeePerGas && !overrides?.maxFeePerGas) { - populatedTransaction.maxFeePerGas *= 2n; + populatedTransaction.maxFeePerGas *= gasMultiple; } if ( populatedTransaction.maxPriorityFeePerGas && !overrides?.maxPriorityFeePerGas ) { - populatedTransaction.maxPriorityFeePerGas *= 2n; + populatedTransaction.maxPriorityFeePerGas *= gasMultiple; } job.log(`Populated transaction: ${stringify(populatedTransaction)}`); From b0ec018c3e02c64c4f04f2b562216433c142d466 Mon Sep 17 00:00:00 2001 From: Phillip Ho Date: Tue, 5 Nov 2024 07:16:18 +0800 Subject: [PATCH 2/6] fix clamp --- src/tests/math.test.ts | 8 ++++---- src/utils/math.ts | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/tests/math.test.ts b/src/tests/math.test.ts index dde3941bf..db3ce8be1 100644 --- a/src/tests/math.test.ts +++ b/src/tests/math.test.ts @@ -34,18 +34,18 @@ describe("clamp", () => { }); it("clamps the value correctly at the minimum", () => { - expect(clamp(1, { min: 2, max: 10 })).toBe(2); + expect(clamp(2, { min: 2, max: 10 })).toBe(2); }); it("returns the value when within bounds", () => { - expect(clamp(3, { min: 2, max: 10 })).toBe(6); + expect(clamp(3, { min: 2, max: 10 })).toBe(3); }); it("clamps the value correctly at the maximum", () => { - expect(clamp(5, { min: 2, max: 10 })).toBe(10); + expect(clamp(10, { min: 2, max: 10 })).toBe(10); }); it("clamps the value correctly above the maximum", () => { - expect(clamp(6, { min: 2, max: 10 })).toBe(10); + expect(clamp(12, { min: 2, max: 10 })).toBe(10); }); }); diff --git a/src/utils/math.ts b/src/utils/math.ts index d5af882dd..e8c948b87 100644 --- a/src/utils/math.ts +++ b/src/utils/math.ts @@ -11,4 +11,4 @@ export const getPercentile = (arr: number[], percentile: number): number => { export const clamp = ( val: number, { min, max }: { min: number; max: number }, -) => Math.max(min, Math.min(val * 2, max)); +) => Math.max(min, Math.min(val, max)); From 2faf3c8e651666cf366416fc1caa98bf9fa014a0 Mon Sep 17 00:00:00 2001 From: Phillip Ho Date: Fri, 8 Nov 2024 14:06:38 +0800 Subject: [PATCH 3/6] update gas retry logic --- src/tests/sendTransactionWorker.test.ts | 95 +++++++++++++++++++++++ src/worker/tasks/sendTransactionWorker.ts | 77 +++++++++++++----- 2 files changed, 152 insertions(+), 20 deletions(-) create mode 100644 src/tests/sendTransactionWorker.test.ts diff --git a/src/tests/sendTransactionWorker.test.ts b/src/tests/sendTransactionWorker.test.ts new file mode 100644 index 000000000..413f8125b --- /dev/null +++ b/src/tests/sendTransactionWorker.test.ts @@ -0,0 +1,95 @@ +import { Hex } from "thirdweb"; +import { describe, expect, it } from "vitest"; +import { _updateGasFees } from "../worker/tasks/sendTransactionWorker"; + +describe("_updateGasFees", () => { + const base = { + // Irrelevant values for testing. + chainId: 1, + data: "0x0" as Hex, + gas: 21000n, + to: undefined, + nonce: undefined, + accessList: undefined, + value: undefined, + }; + + // const legacyTransaction: PopulatedTransaction = { + // ...base, + // gasPrice: 35000000000n, + // }; + + // const eip1559Transaction: PopulatedTransaction = { + // ...base, + // maxPriorityFeePerGas: 500000000n, + // maxFeePerGas: 35000000000n, + // }; + + it("returns the original transaction on first send (resendCount = 0)", () => { + let result = _updateGasFees({ ...base, gasPrice: 100n }, 0, undefined); + expect(result.gasPrice).toEqual(100n); + + result = _updateGasFees( + { ...base, maxFeePerGas: 100n, maxPriorityFeePerGas: 10n }, + 0, + undefined, + ); + expect(result.maxFeePerGas).toEqual(100n); + expect(result.maxPriorityFeePerGas).toEqual(10n); + }); + + it("doubles gasPrice for legacy transactions", () => { + const result = _updateGasFees({ ...base, gasPrice: 100n }, 1, {}); + expect(result.gasPrice).toBe(200n); + }); + + it("caps gasPrice multiplier at 10x", () => { + const result = _updateGasFees({ ...base, gasPrice: 100n }, 10, {}); + expect(result.gasPrice).toBe(1000n); + }); + + it("updates maxPriorityFeePerGas and maxFeePerGas for EIP-1559 transactions", () => { + const result = _updateGasFees( + { ...base, maxFeePerGas: 100n, maxPriorityFeePerGas: 10n }, + 3, + {}, + ); + expect(result.maxPriorityFeePerGas).toBe(60n); + expect(result.maxFeePerGas).toBe(260n); + }); + + it("respects overrides for maxPriorityFeePerGas", () => { + const result = _updateGasFees( + { ...base, maxFeePerGas: 100n, maxPriorityFeePerGas: 10n }, + 3, + { maxPriorityFeePerGas: 10n }, + ); + expect(result.maxPriorityFeePerGas).toBe(10n); // matches override + expect(result.maxFeePerGas).toBe(210n); + }); + + it("respects overrides for maxFeePerGas", () => { + const result = _updateGasFees( + { ...base, maxFeePerGas: 100n, maxPriorityFeePerGas: 10n }, + 3, + { maxFeePerGas: 100n }, + ); + expect(result.maxPriorityFeePerGas).toBe(60n); + expect(result.maxFeePerGas).toBe(100n); // matches override + }); + + it("returns correct values when only maxPriorityFeePerGas is set", () => { + const result = _updateGasFees( + { ...base, maxPriorityFeePerGas: 10n }, + 3, + {}, + ); + expect(result.maxPriorityFeePerGas).toBe(60n); + expect(result.maxFeePerGas).toBeUndefined(); + }); + + it("returns correct values when only maxFeePerGas is set", () => { + const result = _updateGasFees({ ...base, maxFeePerGas: 80n }, 3, {}); + expect(result.maxFeePerGas).toBe(160n); + }); +}); diff --git a/src/worker/tasks/sendTransactionWorker.ts b/src/worker/tasks/sendTransactionWorker.ts index 7bd275bf3..a51c192e9 100644 --- a/src/worker/tasks/sendTransactionWorker.ts +++ b/src/worker/tasks/sendTransactionWorker.ts @@ -75,10 +75,15 @@ const handler: Processor = async (job: Job) => { return; } - // The transaction may be errored if it is manually retried. - // For example, the developer retried all failed transactions during an RPC outage. - // An errored queued transaction (resendCount = 0) is safe to retry: the transaction wasn't sent to RPC. - if (transaction.status === "errored" && resendCount === 0) { + // If the transaction is errored and has not yet been sent, + // reset it to a QueuedTransaction to try again. + // No transaction hashes means the transaction is not in-flight. + if ( + transaction.status === "errored" && + !transaction.isUserOp && + "sentTransactionHashes" in transaction && + transaction.sentTransactionHashes.length > 0 + ) { const { errorMessage, ...omitted } = transaction; transaction = { ...omitted, @@ -438,7 +443,7 @@ const _resendTransaction = async ( // Populate the transaction with double gas. const { chainId, from, overrides, sentTransactionHashes } = sentTransaction; - const populatedTransaction = await toSerializableTransaction({ + let populatedTransaction = await toSerializableTransaction({ from: getChecksumAddress(from), transaction: { client: thirdwebClient, @@ -452,21 +457,12 @@ const _resendTransaction = async ( }, }); - // Double the gas fee settings each attempt up to 10x. - // Do not update gas if overrides were provided. - const gasMultiple = BigInt(clamp(job.attemptsMade * 2, { min: 2, max: 10 })); - if (populatedTransaction.gasPrice) { - populatedTransaction.gasPrice *= gasMultiple; - } - if (populatedTransaction.maxFeePerGas && !overrides?.maxFeePerGas) { - populatedTransaction.maxFeePerGas *= gasMultiple; - } - if ( - populatedTransaction.maxPriorityFeePerGas && - !overrides?.maxPriorityFeePerGas - ) { - populatedTransaction.maxPriorityFeePerGas *= gasMultiple; - } + // Increase gas fees for this resend attempt. + populatedTransaction = _updateGasFees( + populatedTransaction, + sentTransaction.resendCount + 1, + sentTransaction.overrides, + ); job.log(`Populated transaction: ${stringify(populatedTransaction)}`); @@ -577,6 +573,47 @@ const _hasExceededTimeout = ( const _minutesFromNow = (minutes: number) => new Date(Date.now() + minutes * 60_000); +/** + * Computes the aggressive gas fees to use when resending a transaction. + * + * For legacy transactions (pre-EIP1559): + * - Set gas price to 2 * current attempt, capped at 10x. + * + * For transactions with maxFeePerGas + maxPriorityFeePerGas: + * - Set maxPriorityFeePerGas to 2x current attempt, capped at 10x. + * - Set maxFeePerGas to 2 * current max fee, plus the maxPriorityFeePerGas. + * + * @param populatedTransaction The transaction with estimated gas from RPC. + * @param resendCount The resend attempt #. Example: 2 = the transaction was initially sent, then resent once. This is the second resend attempt. + */ +export const _updateGasFees = ( + populatedTransaction: PopulatedTransaction, + resendCount: number, + overrides: SentTransaction["overrides"], +): PopulatedTransaction => { + if (resendCount === 0) { + return populatedTransaction; + } + + const multiplier = BigInt(clamp(resendCount * 2, { min: 2, max: 10 })); + + const updated = { ...populatedTransaction }; + if (updated.gasPrice) { + updated.gasPrice *= multiplier; + } + // Don't update gas fees that are explicitly overridden. + if (updated.maxPriorityFeePerGas && !overrides?.maxPriorityFeePerGas) { + updated.maxPriorityFeePerGas *= multiplier; + } + // Don't update gas fees that are explicitly overridden. + if (updated.maxFeePerGas && !overrides?.maxFeePerGas) { + updated.maxFeePerGas = + updated.maxFeePerGas * 2n + (updated.maxPriorityFeePerGas ?? 0n); + } + + return updated; +}; + // Must be explicitly called for the worker to run on this host. export const initSendTransactionWorker = () => { const _worker = new Worker(SendTransactionQueue.q.name, handler, { From 51b7c256ff78948829455d85760289f833fb753f Mon Sep 17 00:00:00 2001 From: Phillip Ho Date: Fri, 8 Nov 2024 14:08:22 +0800 Subject: [PATCH 4/6] remove clamp --- src/tests/math.test.ts | 24 +---------------------- src/utils/math.ts | 5 ----- src/worker/tasks/sendTransactionWorker.ts | 3 +-- 3 files changed, 2 insertions(+), 30 deletions(-) diff --git a/src/tests/math.test.ts b/src/tests/math.test.ts index db3ce8be1..6303d6bed 100644 --- a/src/tests/math.test.ts +++ b/src/tests/math.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { clamp, getPercentile } from "../utils/math"; +import { getPercentile } from "../utils/math"; describe("getPercentile", () => { it("should correctly calculate the p50 (median) of a sorted array", () => { @@ -27,25 +27,3 @@ describe("getPercentile", () => { expect(getPercentile(numbers, 50)).toBe(0); }); }); - -describe("clamp", () => { - it("clamps the value correctly below the minimum", () => { - expect(clamp(0, { min: 2, max: 10 })).toBe(2); - }); - - it("clamps the value correctly at the minimum", () => { - expect(clamp(2, { min: 2, max: 10 })).toBe(2); - }); - - it("returns the value when within bounds", () => { - expect(clamp(3, { min: 2, max: 10 })).toBe(3); - }); - - it("clamps the value correctly at the maximum", () => { - expect(clamp(10, { min: 2, max: 10 })).toBe(10); - }); - - it("clamps the value correctly above the maximum", () => { - expect(clamp(12, { min: 2, max: 10 })).toBe(10); - }); -}); diff --git a/src/utils/math.ts b/src/utils/math.ts index e8c948b87..2f693d661 100644 --- a/src/utils/math.ts +++ b/src/utils/math.ts @@ -7,8 +7,3 @@ export const getPercentile = (arr: number[], percentile: number): number => { const index = Math.floor((percentile / 100) * (arr.length - 1)); return arr[index]; }; - -export const clamp = ( - val: number, - { min, max }: { min: number; max: number }, -) => Math.max(min, Math.min(val, max)); diff --git a/src/worker/tasks/sendTransactionWorker.ts b/src/worker/tasks/sendTransactionWorker.ts index a51c192e9..3a8c02716 100644 --- a/src/worker/tasks/sendTransactionWorker.ts +++ b/src/worker/tasks/sendTransactionWorker.ts @@ -39,7 +39,6 @@ import { isReplacementGasFeeTooLow, wrapError, } from "../../utils/error"; -import { clamp } from "../../utils/math"; import { getChecksumAddress } from "../../utils/primitiveTypes"; import { recordMetrics } from "../../utils/prometheus"; import { redis } from "../../utils/redis/redis"; @@ -595,7 +594,7 @@ export const _updateGasFees = ( return populatedTransaction; } - const multiplier = BigInt(clamp(resendCount * 2, { min: 2, max: 10 })); + const multiplier = BigInt(Math.min(10, resendCount * 2)); const updated = { ...populatedTransaction }; if (updated.gasPrice) { From 5eeee1713950b4be00187be44e618ab91bc77e24 Mon Sep 17 00:00:00 2001 From: Phillip Ho Date: Fri, 8 Nov 2024 14:09:15 +0800 Subject: [PATCH 5/6] unused vars --- src/tests/sendTransactionWorker.test.ts | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/tests/sendTransactionWorker.test.ts b/src/tests/sendTransactionWorker.test.ts index 413f8125b..5fb0a5dcb 100644 --- a/src/tests/sendTransactionWorker.test.ts +++ b/src/tests/sendTransactionWorker.test.ts @@ -1,4 +1,4 @@ -import { Hex } from "thirdweb"; +import type { Hex } from "thirdweb"; import { describe, expect, it } from "vitest"; import { _updateGasFees } from "../worker/tasks/sendTransactionWorker"; @@ -14,17 +14,6 @@ describe("_updateGasFees", () => { value: undefined, }; - // const legacyTransaction: PopulatedTransaction = { - // ...base, - // gasPrice: 35000000000n, - // }; - - // const eip1559Transaction: PopulatedTransaction = { - // ...base, - // maxPriorityFeePerGas: 500000000n, - // maxFeePerGas: 35000000000n, - // }; - it("returns the original transaction on first send (resendCount = 0)", () => { let result = _updateGasFees({ ...base, gasPrice: 100n }, 0, undefined); expect(result.gasPrice).toEqual(100n); From 60ad3d8d38fe8eeebf24fcdee41afd147c084b62 Mon Sep 17 00:00:00 2001 From: Phillip Ho Date: Sat, 7 Dec 2024 10:02:16 +0800 Subject: [PATCH 6/6] update gasPrice --- src/worker/tasks/sendTransactionWorker.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/worker/tasks/sendTransactionWorker.ts b/src/worker/tasks/sendTransactionWorker.ts index c56713cde..c8d254581 100644 --- a/src/worker/tasks/sendTransactionWorker.ts +++ b/src/worker/tasks/sendTransactionWorker.ts @@ -553,14 +553,14 @@ const _minutesFromNow = (minutes: number) => new Date(Date.now() + minutes * 60_000); /** - * Computes the aggressive gas fees to use when resending a transaction. + * Computes aggressive gas fees when resending a transaction. * * For legacy transactions (pre-EIP1559): - * - Set gas price to 2 * current attempt, capped at 10x. + * - Gas price = (2 * attempt) * estimatedGasPrice, capped at 10x. * - * For transactions with maxFeePerGas + maxPriorityFeePerGas: - * - Set maxPriorityFeePerGas to 2x current attempt, capped at 10x. - * - Set maxFeePerGas to 2 * current max fee, plus the maxPriorityFeePerGas. + * For other transactions: + * - maxPriorityFeePerGas = (2 * attempt) * estimatedMaxPriorityFeePerGas, capped at 10x. + * - maxFeePerGas = (2 * estimatedMaxFeePerGas) + maxPriorityFeePerGas. * * @param populatedTransaction The transaction with estimated gas from RPC. * @param resendCount The resend attempt #. Example: 2 = the transaction was initially sent, then resent once. This is the second resend attempt. @@ -577,14 +577,15 @@ export const _updateGasFees = ( const multiplier = BigInt(Math.min(10, resendCount * 2)); const updated = { ...populatedTransaction }; - if (updated.gasPrice) { + + // Update gas fees (unless they were explicitly overridden). + + if (updated.gasPrice && !overrides?.gasPrice) { updated.gasPrice *= multiplier; } - // Don't update gas fees that are explicitly overridden. if (updated.maxPriorityFeePerGas && !overrides?.maxPriorityFeePerGas) { updated.maxPriorityFeePerGas *= multiplier; } - // Don't update gas fees that are explicitly overridden. if (updated.maxFeePerGas && !overrides?.maxFeePerGas) { updated.maxFeePerGas = updated.maxFeePerGas * 2n + (updated.maxPriorityFeePerGas ?? 0n);