From 6514c0b3f13d26f8b5bcf266cc7ea21755359b33 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Tue, 2 Aug 2022 15:27:10 -0400 Subject: [PATCH 01/21] add test to expose nonce generation race condition --- .../ethereum/tests/transaction-pool.test.ts | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts b/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts index d542e873bf..05d149d13e 100644 --- a/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts +++ b/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts @@ -512,4 +512,33 @@ describe("transaction pool", async () => { transaction.serialized.toString() ); }); + + it("generates a nonce based off of the account's latest executed transaction", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const transaction = TransactionFactory.fromRpc(rpcTx, common); + const { pending, inProgress } = txPool.executables; + + await txPool.prepareTransaction(transaction, secretKey); + const drainPromise = txPool.on("drain", () => { + // when a transaction is run, the miner removes the transaction from the + // pending queue and adds it to the inProgress pool. There is a lag + // between running the transaction and saving the block, which can cause + // a race condition for nonce generation. we will make this lag infinite + // here, because we never save the block. If the account's nonce is looked + // up, it will not have changed, so the pool will have to rely on the + // inProgress transactions to set the nonce of the next transaction + const pendingOrigin = pending.get(from); + inProgress.add(transaction); + pendingOrigin.removeBest(); + }); + txPool.drain(); + await drainPromise; + + const anotherTransaction = TransactionFactory.fromRpc(rpcTx, common); + // our transaction doesn't have a nonce up front. + assert(anotherTransaction.nonce.isNull()); + await txPool.prepareTransaction(anotherTransaction, secretKey); + // after it's prepared by the txPool, an appropriate nonce for the account is set + assert.strictEqual(anotherTransaction.nonce.toBigInt(), 1n); + }); }); From f2dcffe54b2667a1e500acc5434f2820e0041fa5 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Tue, 2 Aug 2022 15:44:11 -0400 Subject: [PATCH 02/21] rename to pending cause wtf were we thinking? --- src/chains/ethereum/ethereum/src/transaction-pool.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/transaction-pool.ts b/src/chains/ethereum/ethereum/src/transaction-pool.ts index 035e9dc140..490dc821f6 100644 --- a/src/chains/ethereum/ethereum/src/transaction-pool.ts +++ b/src/chains/ethereum/ethereum/src/transaction-pool.ts @@ -231,8 +231,8 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { const queuedOriginTransactions = origins.get(origin); let transactionPlacement = TriageOption.FutureQueue; - const executables = this.executables.pending; - let executableOriginTransactions = executables.get(origin); + const pending = this.executables.pending; + let executableOriginTransactions = pending.get(origin); const priceBump = this.#priceBump; let length: number; @@ -352,7 +352,7 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { } else { // if we don't yet have an executables queue for this origin make one now executableOriginTransactions = Heap.from(transaction, byNonce); - executables.set(origin, executableOriginTransactions); + pending.set(origin, executableOriginTransactions); } // Now we need to drain any queued transactions that were previously From f9c43f2fe18c8c24b79186b493bc6f5adb9796fb Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Tue, 2 Aug 2022 15:45:06 -0400 Subject: [PATCH 03/21] check inProgress queue for next nonce --- .../ethereum/ethereum/src/transaction-pool.ts | 35 ++++++++++++++++--- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/transaction-pool.ts b/src/chains/ethereum/ethereum/src/transaction-pool.ts index 490dc821f6..c74a7319ad 100644 --- a/src/chains/ethereum/ethereum/src/transaction-pool.ts +++ b/src/chains/ethereum/ethereum/src/transaction-pool.ts @@ -76,6 +76,19 @@ function shouldReplace( return true; } } +function findHighestNonceByOrigin(set: Set, origin: string) { + let highestNonce: bigint = null; + for (const transaction of set) { + if ( + (transaction.from.toString() === origin && + transaction.nonce.toBigInt() > highestNonce) || + highestNonce === null + ) { + highestNonce = transaction.nonce.toBigInt(); + } + } + return highestNonce; +} function byNonce(values: TypedTransaction[], a: number, b: number) { return ( @@ -231,7 +244,7 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { const queuedOriginTransactions = origins.get(origin); let transactionPlacement = TriageOption.FutureQueue; - const pending = this.executables.pending; + const { pending, inProgress } = this.executables; let executableOriginTransactions = pending.get(origin); const priceBump = this.#priceBump; @@ -284,19 +297,33 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { // since we don't have any executable transactions at the moment, we need // to find our nonce from the account itself... const transactorNonce = transactor.nonce.toBigInt(); + + const highestNonceFromOrigin = findHighestNonceByOrigin( + inProgress, + origin + ); + + const useHighestNonce = + highestNonceFromOrigin !== null && + highestNonceFromOrigin >= transactorNonce; + + const effectiveNonce = useHighestNonce + ? highestNonceFromOrigin + 1n + : transactorNonce || 0n; + if (txNonce === void 0) { // if we don't have a transactionNonce, just use the account's next // nonce and mark as executable - txNonce = transactorNonce ? transactorNonce : 0n; + txNonce = effectiveNonce; transaction.nonce = Quantity.from(txNonce); transactionPlacement = TriageOption.Executable; - } else if (txNonce < transactorNonce) { + } else if (txNonce < effectiveNonce) { // it's an error if the transaction's nonce is <= the persisted nonce throw new CodedError( `the tx doesn't have the correct nonce. account has nonce of: ${transactorNonce} tx has nonce of: ${txNonce}`, JsonRpcErrorCode.INVALID_INPUT ); - } else if (txNonce === transactorNonce) { + } else if (txNonce === effectiveNonce) { transactionPlacement = TriageOption.Executable; } } From 05a8b305c86f2479aab0b866f49bf8b1608378ef Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Thu, 4 Aug 2022 15:08:07 -0400 Subject: [PATCH 04/21] fix error message --- src/chains/ethereum/ethereum/src/transaction-pool.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/chains/ethereum/ethereum/src/transaction-pool.ts b/src/chains/ethereum/ethereum/src/transaction-pool.ts index c74a7319ad..79f2f11652 100644 --- a/src/chains/ethereum/ethereum/src/transaction-pool.ts +++ b/src/chains/ethereum/ethereum/src/transaction-pool.ts @@ -320,7 +320,7 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { } else if (txNonce < effectiveNonce) { // it's an error if the transaction's nonce is <= the persisted nonce throw new CodedError( - `the tx doesn't have the correct nonce. account has nonce of: ${transactorNonce} tx has nonce of: ${txNonce}`, + `the tx doesn't have the correct nonce. account has nonce of: ${effectiveNonce} tx has nonce of: ${txNonce}`, JsonRpcErrorCode.INVALID_INPUT ); } else if (txNonce === effectiveNonce) { From 0a347080730f63ce15fa5c560dcab56085210947 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Thu, 4 Aug 2022 15:10:31 -0400 Subject: [PATCH 05/21] rearrange tests a bit --- .../ethereum/tests/transaction-pool.test.ts | 806 +++++++++--------- 1 file changed, 404 insertions(+), 402 deletions(-) diff --git a/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts b/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts index 05d149d13e..e39e759c7c 100644 --- a/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts +++ b/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts @@ -41,7 +41,8 @@ describe("transaction pool", async () => { }; const options = EthereumOptionsConfig.normalize(optionsJson); let futureNonceRpc, executableRpc: Transaction; - before(function () { + + const beforeEachSetup = () => { const wallet = new Wallet(options.wallet); [from] = wallet.addresses; secretKey = wallet.unlockedAccounts.get(from); @@ -95,422 +96,423 @@ describe("transaction pool", async () => { latest: { header: { baseFeePerGas: Quantity.from(875000000) } } } }; - }); - beforeEach(async function () { - // for each test, we'll need a fresh set of origins + origins = new Map(); - }); + }; - it("rejects transactions whose gasLimit is greater than the block gas limit", async () => { - // for this tx pool, we'll have the block gas limit low - const optionsJson = { miner: { blockGasLimit: "0xff" } }; - const options = EthereumOptionsConfig.normalize(optionsJson); - const txPool = new TransactionPool(options.miner, blockchain, origins); - const executableTx = TransactionFactory.fromRpc(executableRpc, common); - await assert.rejects( - txPool.prepareTransaction(executableTx, secretKey), - { - code: -32000, - message: "exceeds block gas limit" - }, - "transaction with gas limit higher than block gas limit should have been rejected" - ); - }); - it("rejects transactions whose gasLimit is not enough to run the transaction", async () => { - const txPool = new TransactionPool(options.miner, blockchain); - // the tx should have a very low gas limit to be rejected - const lowGasRpc: Transaction = { - from: from, - type: "0x2", - maxFeePerGas: "0xffffffff", - gasLimit: "0xff" - }; - const lowGasTx = TransactionFactory.fromRpc(lowGasRpc, common); - await assert.rejects( - txPool.prepareTransaction(lowGasTx, secretKey), - { - code: -32000, - message: "intrinsic gas too low" - }, - "transaction with gas limit that is too low to run the transaction should have been rejected" - ); - }); + describe("rejections", async () => { + beforeEach(beforeEachSetup); + it("rejects transactions whose gasLimit is greater than the block gas limit", async () => { + // for this tx pool, we'll have the block gas limit low + const optionsJson = { miner: { blockGasLimit: "0xff" } }; + const options = EthereumOptionsConfig.normalize(optionsJson); + const txPool = new TransactionPool(options.miner, blockchain, origins); + const executableTx = TransactionFactory.fromRpc(executableRpc, common); + await assert.rejects( + txPool.prepareTransaction(executableTx, secretKey), + { + code: -32000, + message: "exceeds block gas limit" + }, + "transaction with gas limit higher than block gas limit should have been rejected" + ); + }); + it("rejects transactions whose gasLimit is not enough to run the transaction", async () => { + const txPool = new TransactionPool(options.miner, blockchain); + // the tx should have a very low gas limit to be rejected + const lowGasRpc: Transaction = { + from: from, + type: "0x2", + maxFeePerGas: "0xffffffff", + gasLimit: "0xff" + }; + const lowGasTx = TransactionFactory.fromRpc(lowGasRpc, common); + await assert.rejects( + txPool.prepareTransaction(lowGasTx, secretKey), + { + code: -32000, + message: "intrinsic gas too low" + }, + "transaction with gas limit that is too low to run the transaction should have been rejected" + ); + }); - it("rejects transactions whose nonce is lower than the account nonce", async () => { - const options = EthereumOptionsConfig.normalize({}); - // when the tx pool requests a nonce for an account, we'll always respond 1 - // so if we send a tx with nonce 0, it should reject - const fakeNonceChain = { - accounts: { - getNonceAndBalance: async () => { - return { nonce: Quantity.from(1), balance: Quantity.from(1e15) }; + it("rejects transactions whose nonce is lower than the account nonce", async () => { + const options = EthereumOptionsConfig.normalize({}); + // when the tx pool requests a nonce for an account, we'll always respond 1 + // so if we send a tx with nonce 0, it should reject + const fakeNonceChain = { + accounts: { + getNonceAndBalance: async () => { + return { nonce: Quantity.from(1), balance: Quantity.from(1e15) }; + } + }, + common, + blocks: { + latest: { header: { baseFeePerGas: Quantity.from(875000000) } } } - }, - common, - blocks: { - latest: { header: { baseFeePerGas: Quantity.from(875000000) } } - } - } as any; - const txPool = new TransactionPool(options.miner, fakeNonceChain, origins); - const executableTx = TransactionFactory.fromRpc(executableRpc, common); - await assert.rejects( - txPool.prepareTransaction(executableTx, secretKey), - { - message: `the tx doesn't have the correct nonce. account has nonce of: 1 tx has nonce of: ${executableTx.nonce.toBigInt()}` - }, - "transaction with nonce lower than account nonce should have been rejected" - ); - }); - - it("rejects executable replacement transactions whose gas price isn't sufficiently high", async () => { - const txPool = new TransactionPool(options.miner, blockchain, origins); - const executableTx = TransactionFactory.fromRpc(executableRpc, common); - const isExecutable = await txPool.prepareTransaction( - executableTx, - secretKey - ); - assert(isExecutable); // our first transaction is executable - const { pending } = txPool.executables; - // our executable transaction should be found in the pending queue - const found = findIn(executableTx.hash.toBuffer(), pending); - assert.strictEqual( - found.serialized.toString(), - executableTx.serialized.toString() - ); - - const replacementRpc = JSON.parse(JSON.stringify(executableRpc)); - replacementRpc.maxPriorityFeePerGas = "0xffff"; - const replacementTx1 = TransactionFactory.fromRpc(replacementRpc, common); - // even if the tip is high enough, the max fee isn't enough to replace, so it'll throw - await assert.rejects( - txPool.prepareTransaction(replacementTx1, secretKey), - { - code: -32003, - message: "transaction underpriced" - }, - "replacement transaction with insufficient gas price to replace should have been rejected" - ); - - replacementRpc.maxPriorityFeePerGas = executableRpc.maxPriorityFeePerGas; - replacementRpc.maxFeePerGas = "0xffffffffff"; - const replacementTx2 = TransactionFactory.fromRpc(replacementRpc, common); - // even if the maxFee is high enough, the tip isn't enough to replace, so it'll throw - await assert.rejects( - txPool.prepareTransaction(replacementTx2, secretKey), - { - code: -32003, - message: "transaction underpriced" - }, - "replacement transaction with insufficient gas price to replace should have been rejected" - ); - - const legacyReplacementRpc: Transaction = { - from: from, - type: "0x0", - gasPrice: "0xffffffff", - gasLimit: "0xffff", - nonce: "0x0" - }; - const replacementTx3 = TransactionFactory.fromRpc( - legacyReplacementRpc, - common - ); - // the gasPrice is higher than the tip but lower than the maxFee, which isn't enough, so it'll throw - await assert.rejects( - txPool.prepareTransaction(replacementTx3, secretKey), - { - code: -32003, - message: "transaction underpriced" - }, - "replacement transaction with insufficient gas price to replace should have been rejected" - ); - }); - - it("rejects future nonce replacement transactions whose gas price isn't sufficiently high", async () => { - const txPool = new TransactionPool(options.miner, blockchain, origins); - const futureNonceTx = TransactionFactory.fromRpc(futureNonceRpc, common); - const isExecutable = await txPool.prepareTransaction( - futureNonceTx, - secretKey - ); - assert(!isExecutable); // our transaction is not executable - // our non executable transaction should be found in the origins queue - const found = findIn(futureNonceTx.hash.toBuffer(), origins); - assert.strictEqual( - found.serialized.toString(), - futureNonceTx.serialized.toString() - ); - // now, if we resend the same transaction, since the gas price isn't higher, - // it should be rejected - await assert.rejects( - txPool.prepareTransaction(futureNonceTx, secretKey), - { - code: -32003, - message: "transaction underpriced" - }, - "replacement transaction with insufficient gas price to replace should have been rejected" - ); - }); - - it("rejects transactions whose potential cost is more than the account's balance", async () => { - const expensiveRpc: EIP1559FeeMarketRpcTransaction = { - from, - type: "0x2", - value: "0xfffffffffffffffffff", - maxFeePerGas: "0xffffff", - maxPriorityFeePerGas: "0xff", - gasLimit: "0xffff" - }; - const txPool = new TransactionPool(options.miner, blockchain, origins); - const expensiveTx = TransactionFactory.fromRpc(expensiveRpc, common); - await assert.rejects( - txPool.prepareTransaction(expensiveTx, secretKey), - { - code: -32003, - message: "insufficient funds for gas * price + value" - }, - "transaction whose potential cost is more than the account's balance should have been rejected" - ); - }); - - it("adds immediately executable transactions to the pending queue", async () => { - const txPool = new TransactionPool(options.miner, blockchain, origins); - const executableTx = TransactionFactory.fromRpc(executableRpc, common); - const isExecutable = await txPool.prepareTransaction( - executableTx, - secretKey - ); - assert(isExecutable); // our first transaction is executable - const { pending } = txPool.executables; - // our executable transaction should be found in the pending queue - const found = findIn(executableTx.hash.toBuffer(), pending); - assert.strictEqual( - found.serialized.toString(), - executableTx.serialized.toString() - ); - }); - - it("adds future nonce transactions to the future queue", async () => { - const txPool = new TransactionPool(options.miner, blockchain, origins); - const futureNonceTx = TransactionFactory.fromRpc(futureNonceRpc, common); - const isExecutable = await txPool.prepareTransaction( - futureNonceTx, - secretKey - ); - assert(!isExecutable); // our transaction is not executable - // our non executable transaction should be found in the origins queue - const found = findIn(futureNonceTx.hash.toBuffer(), origins); - assert.strictEqual( - found.serialized.toString(), - futureNonceTx.serialized.toString() - ); - }); - - it("replaces immediately executable transactions in the pending queue", async () => { - const txPool = new TransactionPool(options.miner, blockchain, origins); - const executableTx = TransactionFactory.fromRpc(executableRpc, common); - const isExecutable = await txPool.prepareTransaction( - executableTx, - secretKey - ); - assert(isExecutable); // our first transaction is executable - const { pending } = txPool.executables; - // our executable transaction should be found in the pending queue - const found = findIn(executableTx.hash.toBuffer(), pending); - assert.strictEqual( - found.serialized.toString(), - executableTx.serialized.toString() - ); - - // raise our replacement transaction's prices by exactly the price bump amount - const originalMaxFee = Quantity.toBigInt(executableRpc.maxFeePerGas); - const originalTip = Quantity.from( - executableRpc.maxPriorityFeePerGas - ).toBigInt(); - const maxFeePremium = originalMaxFee + (originalMaxFee * priceBump) / 100n; - const tipPremium = originalTip + (originalTip * priceBump) / 100n; - // our replacement transaction needs to have a sufficiently higher gasPrice - const replacementRpc: Transaction = { - from: from, - type: "0x2", - maxFeePerGas: Quantity.toString(maxFeePremium), - maxPriorityFeePerGas: Quantity.toString(tipPremium), - gasLimit: "0xffff", - nonce: "0x0" - }; - const replacementTx = TransactionFactory.fromRpc(replacementRpc, common); - const replacementIsExecutable = await txPool.prepareTransaction( - replacementTx, - secretKey - ); - assert(replacementIsExecutable); // our replacement transaction is executable - // our replacement transaction should be found in the pending queue - const replacementFound = findIn(replacementTx.hash.toBuffer(), pending); - assert.strictEqual( - replacementFound.serialized.toString(), - replacementTx.serialized.toString() - ); - - // our replaced transaction should not be found anywhere in the pool - const originalFound = txPool.find(executableTx.hash.toBuffer()); - assert.strictEqual(originalFound, null); - }); - - it("replaces future nonce transactions in the future queue", async () => { - const txPool = new TransactionPool(options.miner, blockchain, origins); - const futureNonceTx = TransactionFactory.fromRpc(futureNonceRpc, common); - const isExecutable = await txPool.prepareTransaction( - futureNonceTx, - secretKey - ); - assert(!isExecutable); // our transaction is not executable - // our non executable transaction should be found in the origins queue - const found = findIn(futureNonceTx.hash.toBuffer(), origins); - assert.strictEqual( - found.serialized.toString(), - futureNonceTx.serialized.toString() - ); - - // our replacement transaction needs to have a sufficiently higher gasPrice - const replacementRpc: Transaction = { - from: from, - type: "0x2", - maxFeePerGas: "0xffffffffff", - maxPriorityFeePerGas: "0xffff", - gasLimit: "0xffff", - nonce: "0x2" - }; - const replacementTx = TransactionFactory.fromRpc(replacementRpc, common); - const replacementIsExecutable = await txPool.prepareTransaction( - replacementTx, - secretKey - ); - assert(!replacementIsExecutable); // our replacement transaction is also not executable - // our replacement transaction should be found in the origins queue - const replacementFound = findIn(replacementTx.hash.toBuffer(), origins); - assert.strictEqual( - replacementFound.serialized.toString(), - replacementTx.serialized.toString() - ); - - // our replaced transaction should not be found anywhere in the pool - const originalFound = txPool.find(futureNonceTx.hash.toBuffer()); - assert.strictEqual(originalFound, null); - }); - - it("executes future transactions when the nonce gap is filled", async () => { - const txPool = new TransactionPool(options.miner, blockchain, origins); - const futureNonceTx = TransactionFactory.fromRpc(futureNonceRpc, common); - const futureIsExecutable = await txPool.prepareTransaction( - futureNonceTx, - secretKey - ); - assert(!futureIsExecutable); // our transaction is not executable - // our non executable transaction should be found in the origins queue - const foundInOrigins = findIn(futureNonceTx.hash.toBuffer(), origins); - assert.strictEqual( - foundInOrigins.serialized.toString(), - futureNonceTx.serialized.toString() - ); - - // since the "future nonce" is 0x2, we need a 0x0 and a 0x1 nonce transaction - // from this origin. queue up the 0x0 one now - const executableTx = TransactionFactory.fromRpc(executableRpc, common); - const isExecutable = await txPool.prepareTransaction( - executableTx, - secretKey - ); - assert(isExecutable); // our first transaction is executable - const { pending } = txPool.executables; - // our executable transaction should be found in the pending queue - const found = findIn(executableTx.hash.toBuffer(), pending); - assert.strictEqual( - found.serialized.toString(), - executableTx.serialized.toString() - ); + } as any; + const txPool = new TransactionPool( + options.miner, + fakeNonceChain, + origins + ); + const executableTx = TransactionFactory.fromRpc(executableRpc, common); + await assert.rejects( + txPool.prepareTransaction(executableTx, secretKey), + { + message: `the tx doesn't have the correct nonce. account has nonce of: 1 tx has nonce of: ${executableTx.nonce.toBigInt()}` + }, + "transaction with nonce lower than account nonce should have been rejected" + ); + }); - // now we'll send in transaction that will fill the gap between the queued - // tx and the account's nonce - // note, we're sending a transaction that doesn't have a nonce just for - // code coverage, to hit the lines where there 1. is an executable tx already - // and 2. a no-nonce tx is sent so the next highest nonce needs to be used - const tx = TransactionFactory.fromRpc(rpcTx, common); - const txIsExecutable = await txPool.prepareTransaction(tx, secretKey); - assert(txIsExecutable); // our next transaction is executable - // our executable transaction should be found in the pending queue - const txFound = findIn(tx.hash.toBuffer(), pending); - assert.strictEqual(txFound.serialized.toString(), tx.serialized.toString()); - - // now, the tx pool should have automatically marked our previously "future" - // tx as executable and moved it out of the origins queue. - const futureInPending = findIn(futureNonceTx.hash.toBuffer(), pending); - assert.strictEqual( - futureInPending.serialized.toString(), - futureNonceTx.serialized.toString() - ); - const futureInOrigin = findIn(futureNonceTx.hash.toBuffer(), origins); - assert.strictEqual(futureInOrigin, undefined); - }); + it("rejects executable replacement transactions whose gas price isn't sufficiently high", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const executableTx = TransactionFactory.fromRpc(executableRpc, common); + const isExecutable = await txPool.prepareTransaction( + executableTx, + secretKey + ); + assert(isExecutable); // our first transaction is executable + const { pending } = txPool.executables; + // our executable transaction should be found in the pending queue + const found = findIn(executableTx.hash.toBuffer(), pending); + assert.strictEqual( + found.serialized.toString(), + executableTx.serialized.toString() + ); + + const replacementRpc = JSON.parse(JSON.stringify(executableRpc)); + replacementRpc.maxPriorityFeePerGas = "0xffff"; + const replacementTx1 = TransactionFactory.fromRpc(replacementRpc, common); + // even if the tip is high enough, the max fee isn't enough to replace, so it'll throw + await assert.rejects( + txPool.prepareTransaction(replacementTx1, secretKey), + { + code: -32003, + message: "transaction underpriced" + }, + "replacement transaction with insufficient gas price to replace should have been rejected" + ); + + replacementRpc.maxPriorityFeePerGas = executableRpc.maxPriorityFeePerGas; + replacementRpc.maxFeePerGas = "0xffffffffff"; + const replacementTx2 = TransactionFactory.fromRpc(replacementRpc, common); + // even if the maxFee is high enough, the tip isn't enough to replace, so it'll throw + await assert.rejects( + txPool.prepareTransaction(replacementTx2, secretKey), + { + code: -32003, + message: "transaction underpriced" + }, + "replacement transaction with insufficient gas price to replace should have been rejected" + ); + + const legacyReplacementRpc: Transaction = { + from: from, + type: "0x0", + gasPrice: "0xffffffff", + gasLimit: "0xffff", + nonce: "0x0" + }; + const replacementTx3 = TransactionFactory.fromRpc( + legacyReplacementRpc, + common + ); + // the gasPrice is higher than the tip but lower than the maxFee, which isn't enough, so it'll throw + await assert.rejects( + txPool.prepareTransaction(replacementTx3, secretKey), + { + code: -32003, + message: "transaction underpriced" + }, + "replacement transaction with insufficient gas price to replace should have been rejected" + ); + }); - it("sets the transactions nonce appropriately if omitted from the transaction", async () => { - const txPool = new TransactionPool(options.miner, blockchain, origins); - const transaction = TransactionFactory.fromRpc(rpcTx, common); + it("rejects future nonce replacement transactions whose gas price isn't sufficiently high", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const futureNonceTx = TransactionFactory.fromRpc(futureNonceRpc, common); + const isExecutable = await txPool.prepareTransaction( + futureNonceTx, + secretKey + ); + assert(!isExecutable); // our transaction is not executable + // our non executable transaction should be found in the origins queue + const found = findIn(futureNonceTx.hash.toBuffer(), origins); + assert.strictEqual( + found.serialized.toString(), + futureNonceTx.serialized.toString() + ); + // now, if we resend the same transaction, since the gas price isn't higher, + // it should be rejected + await assert.rejects( + txPool.prepareTransaction(futureNonceTx, secretKey), + { + code: -32003, + message: "transaction underpriced" + }, + "replacement transaction with insufficient gas price to replace should have been rejected" + ); + }); - // our transaction doesn't have a nonce up front. - assert(transaction.nonce.isNull()); - await txPool.prepareTransaction(transaction, secretKey); - // after it's prepared by the txPool, an appropriate nonce for the account is set - assert.strictEqual(transaction.nonce.valueOf(), Quantity.from(0).valueOf()); + it("rejects transactions whose potential cost is more than the account's balance", async () => { + const expensiveRpc: EIP1559FeeMarketRpcTransaction = { + from, + type: "0x2", + value: "0xfffffffffffffffffff", + maxFeePerGas: "0xffffff", + maxPriorityFeePerGas: "0xff", + gasLimit: "0xffff" + }; + const txPool = new TransactionPool(options.miner, blockchain, origins); + const expensiveTx = TransactionFactory.fromRpc(expensiveRpc, common); + await assert.rejects( + txPool.prepareTransaction(expensiveTx, secretKey), + { + code: -32003, + message: "insufficient funds for gas * price + value" + }, + "transaction whose potential cost is more than the account's balance should have been rejected" + ); + }); }); - it("can be cleared/emptied", async () => { - const txPool = new TransactionPool(options.miner, blockchain, origins); - const transaction = TransactionFactory.fromRpc(rpcTx, common); + describe("regular operation", async () => { + beforeEach(beforeEachSetup); + it("adds immediately executable transactions to the pending queue", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const executableTx = TransactionFactory.fromRpc(executableRpc, common); + const isExecutable = await txPool.prepareTransaction( + executableTx, + secretKey + ); + assert(isExecutable); // our first transaction is executable + const { pending } = txPool.executables; + // our executable transaction should be found in the pending queue + const found = findIn(executableTx.hash.toBuffer(), pending); + assert.strictEqual( + found.serialized.toString(), + executableTx.serialized.toString() + ); + }); - await txPool.prepareTransaction(transaction, secretKey); - const { pending } = txPool.executables; - // our executable transaction should be found in the pending queue - const found = findIn(transaction.hash.toBuffer(), pending); - assert.strictEqual( - found.serialized.toString(), - transaction.serialized.toString() - ); + it("adds future nonce transactions to the future queue", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const futureNonceTx = TransactionFactory.fromRpc(futureNonceRpc, common); + const isExecutable = await txPool.prepareTransaction( + futureNonceTx, + secretKey + ); + assert(!isExecutable); // our transaction is not executable + // our non executable transaction should be found in the origins queue + const found = findIn(futureNonceTx.hash.toBuffer(), origins); + assert.strictEqual( + found.serialized.toString(), + futureNonceTx.serialized.toString() + ); + }); - const futureNonceTx = TransactionFactory.fromRpc(futureNonceRpc, common); - const futureIsExecutable = await txPool.prepareTransaction( - futureNonceTx, - secretKey - ); - assert(!futureIsExecutable); // our transaction is not executable - // our non executable transaction should be found in the origins queue - const foundInOrigins = findIn(futureNonceTx.hash.toBuffer(), origins); - assert.strictEqual( - foundInOrigins.serialized.toString(), - futureNonceTx.serialized.toString() - ); + it("replaces immediately executable transactions in the pending queue", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const executableTx = TransactionFactory.fromRpc(executableRpc, common); + const isExecutable = await txPool.prepareTransaction( + executableTx, + secretKey + ); + assert(isExecutable); // our first transaction is executable + const { pending } = txPool.executables; + // our executable transaction should be found in the pending queue + const found = findIn(executableTx.hash.toBuffer(), pending); + assert.strictEqual( + found.serialized.toString(), + executableTx.serialized.toString() + ); + + // raise our replacement transaction's prices by exactly the price bump amount + const originalMaxFee = Quantity.toBigInt(executableRpc.maxFeePerGas); + const originalTip = Quantity.from( + executableRpc.maxPriorityFeePerGas + ).toBigInt(); + const maxFeePremium = + originalMaxFee + (originalMaxFee * priceBump) / 100n; + const tipPremium = originalTip + (originalTip * priceBump) / 100n; + // our replacement transaction needs to have a sufficiently higher gasPrice + const replacementRpc: Transaction = { + from: from, + type: "0x2", + maxFeePerGas: Quantity.toString(maxFeePremium), + maxPriorityFeePerGas: Quantity.toString(tipPremium), + gasLimit: "0xffff", + nonce: "0x0" + }; + const replacementTx = TransactionFactory.fromRpc(replacementRpc, common); + const replacementIsExecutable = await txPool.prepareTransaction( + replacementTx, + secretKey + ); + assert(replacementIsExecutable); // our replacement transaction is executable + // our replacement transaction should be found in the pending queue + const replacementFound = findIn(replacementTx.hash.toBuffer(), pending); + assert.strictEqual( + replacementFound.serialized.toString(), + replacementTx.serialized.toString() + ); + + // our replaced transaction should not be found anywhere in the pool + const originalFound = txPool.find(executableTx.hash.toBuffer()); + assert.strictEqual(originalFound, null); + }); - txPool.clear(); - // both queues should be empty - assert.strictEqual(pending.values.length, 0); - assert.strictEqual(origins.values.length, 0); - }); + it("replaces future nonce transactions in the future queue", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const futureNonceTx = TransactionFactory.fromRpc(futureNonceRpc, common); + const isExecutable = await txPool.prepareTransaction( + futureNonceTx, + secretKey + ); + assert(!isExecutable); // our transaction is not executable + // our non executable transaction should be found in the origins queue + const found = findIn(futureNonceTx.hash.toBuffer(), origins); + assert.strictEqual( + found.serialized.toString(), + futureNonceTx.serialized.toString() + ); + + // our replacement transaction needs to have a sufficiently higher gasPrice + const replacementRpc: Transaction = { + from: from, + type: "0x2", + maxFeePerGas: "0xffffffffff", + maxPriorityFeePerGas: "0xffff", + gasLimit: "0xffff", + nonce: "0x2" + }; + const replacementTx = TransactionFactory.fromRpc(replacementRpc, common); + const replacementIsExecutable = await txPool.prepareTransaction( + replacementTx, + secretKey + ); + assert(!replacementIsExecutable); // our replacement transaction is also not executable + // our replacement transaction should be found in the origins queue + const replacementFound = findIn(replacementTx.hash.toBuffer(), origins); + assert.strictEqual( + replacementFound.serialized.toString(), + replacementTx.serialized.toString() + ); + + // our replaced transaction should not be found anywhere in the pool + const originalFound = txPool.find(futureNonceTx.hash.toBuffer()); + assert.strictEqual(originalFound, null); + }); - it("emits an event when a transaction is ready to be mined", async () => { - const txPool = new TransactionPool(options.miner, blockchain, origins); - const transaction = TransactionFactory.fromRpc(rpcTx, common); + it("executes future transactions when the nonce gap is filled", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const futureNonceTx = TransactionFactory.fromRpc(futureNonceRpc, common); + const futureIsExecutable = await txPool.prepareTransaction( + futureNonceTx, + secretKey + ); + assert(!futureIsExecutable); // our transaction is not executable + // our non executable transaction should be found in the origins queue + const foundInOrigins = findIn(futureNonceTx.hash.toBuffer(), origins); + assert.strictEqual( + foundInOrigins.serialized.toString(), + futureNonceTx.serialized.toString() + ); + + // since the "future nonce" is 0x2, we need a 0x0 and a 0x1 nonce transaction + // from this origin. queue up the 0x0 one now + const executableTx = TransactionFactory.fromRpc(executableRpc, common); + const isExecutable = await txPool.prepareTransaction( + executableTx, + secretKey + ); + assert(isExecutable); // our first transaction is executable + const { pending } = txPool.executables; + // our executable transaction should be found in the pending queue + const found = findIn(executableTx.hash.toBuffer(), pending); + assert.strictEqual( + found.serialized.toString(), + executableTx.serialized.toString() + ); + + // now we'll send in transaction that will fill the gap between the queued + // tx and the account's nonce + // note, we're sending a transaction that doesn't have a nonce just for + // code coverage, to hit the lines where there 1. is an executable tx already + // and 2. a no-nonce tx is sent so the next highest nonce needs to be used + const tx = TransactionFactory.fromRpc(rpcTx, common); + const txIsExecutable = await txPool.prepareTransaction(tx, secretKey); + assert(txIsExecutable); // our next transaction is executable + // our executable transaction should be found in the pending queue + const txFound = findIn(tx.hash.toBuffer(), pending); + assert.strictEqual( + txFound.serialized.toString(), + tx.serialized.toString() + ); + + // now, the tx pool should have automatically marked our previously "future" + // tx as executable and moved it out of the origins queue. + const futureInPending = findIn(futureNonceTx.hash.toBuffer(), pending); + assert.strictEqual( + futureInPending.serialized.toString(), + futureNonceTx.serialized.toString() + ); + const futureInOrigin = findIn(futureNonceTx.hash.toBuffer(), origins); + assert.strictEqual(futureInOrigin, undefined); + }); - await txPool.prepareTransaction(transaction, secretKey); - const drainPromise = txPool.once("drain"); - txPool.drain(); - await drainPromise; + it("can be cleared/emptied", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const transaction = TransactionFactory.fromRpc(rpcTx, common); + + await txPool.prepareTransaction(transaction, secretKey); + const { pending } = txPool.executables; + // our executable transaction should be found in the pending queue + const found = findIn(transaction.hash.toBuffer(), pending); + assert.strictEqual( + found.serialized.toString(), + transaction.serialized.toString() + ); + + const futureNonceTx = TransactionFactory.fromRpc(futureNonceRpc, common); + const futureIsExecutable = await txPool.prepareTransaction( + futureNonceTx, + secretKey + ); + assert(!futureIsExecutable); // our transaction is not executable + // our non executable transaction should be found in the origins queue + const foundInOrigins = findIn(futureNonceTx.hash.toBuffer(), origins); + assert.strictEqual( + foundInOrigins.serialized.toString(), + futureNonceTx.serialized.toString() + ); + + txPool.clear(); + // both queues should be empty + assert.strictEqual(pending.values.length, 0); + assert.strictEqual(origins.values.length, 0); + }); - const { pending } = txPool.executables; - // our executable transaction should be found in the pending queue after the drain event - const found = findIn(transaction.hash.toBuffer(), pending); - assert.strictEqual( - found.serialized.toString(), - transaction.serialized.toString() - ); + it("emits an event when a transaction is ready to be mined", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const transaction = TransactionFactory.fromRpc(rpcTx, common); + + await txPool.prepareTransaction(transaction, secretKey); + const drainPromise = txPool.once("drain"); + txPool.drain(); + await drainPromise; + + const { pending } = txPool.executables; + // our executable transaction should be found in the pending queue after the drain event + const found = findIn(transaction.hash.toBuffer(), pending); + assert.strictEqual( + found.serialized.toString(), + transaction.serialized.toString() + ); + }); }); it("generates a nonce based off of the account's latest executed transaction", async () => { From 43025cee9437ce0bafaa5838d2b6c536305a9787 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Thu, 4 Aug 2022 15:10:43 -0400 Subject: [PATCH 06/21] add tests for nonce generation --- .../ethereum/tests/transaction-pool.test.ts | 315 ++++++++++++++++-- 1 file changed, 289 insertions(+), 26 deletions(-) diff --git a/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts b/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts index e39e759c7c..7e7fa74a36 100644 --- a/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts +++ b/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts @@ -515,32 +515,295 @@ describe("transaction pool", async () => { }); }); - it("generates a nonce based off of the account's latest executed transaction", async () => { - const txPool = new TransactionPool(options.miner, blockchain, origins); - const transaction = TransactionFactory.fromRpc(rpcTx, common); - const { pending, inProgress } = txPool.executables; - - await txPool.prepareTransaction(transaction, secretKey); - const drainPromise = txPool.on("drain", () => { - // when a transaction is run, the miner removes the transaction from the - // pending queue and adds it to the inProgress pool. There is a lag - // between running the transaction and saving the block, which can cause - // a race condition for nonce generation. we will make this lag infinite - // here, because we never save the block. If the account's nonce is looked - // up, it will not have changed, so the pool will have to rely on the - // inProgress transactions to set the nonce of the next transaction - const pendingOrigin = pending.get(from); - inProgress.add(transaction); - pendingOrigin.removeBest(); + describe("nonce generation and validation", async () => { + beforeEach(beforeEachSetup); + + /** + * Adds transaction to pool. If location is "pending", verifies that the + * transaction is executable and is in the pending executables. If "queued", + * verifies that the transaction is not executable and is in the queued pool + * @param txPool + * @param transaction + * @param secretKey + * @param location + */ + const addTxToPoolAndVerify = async ( + txPool: TransactionPool, + transaction: TypedTransaction, + secretKey: Data, + location: "pending" | "queued" + ) => { + const isExecutable = await txPool.prepareTransaction( + transaction, + secretKey + ); + let found: TypedTransaction; + if (location === "pending") { + assert(isExecutable); + // our transaction should be executable and found in the pending queue + const { pending } = txPool.executables; + found = findIn(transaction.hash.toBuffer(), pending); + } else { + // our transaction should not be executable and found in the queued pool + const origins = txPool.origins; + assert(!isExecutable); + found = findIn(transaction.hash.toBuffer(), origins); + } + assert.strictEqual( + found.serialized.toString(), + transaction.serialized.toString() + ); + }; + + /** + * Adds a transaction to the pending pool and "drains" the pool, causing + * the transaction to move from pending to inProgress. + * @param txPool + * @returns + */ + const addTxToInProgress = async (txPool: TransactionPool) => { + const transaction = TransactionFactory.fromRpc(rpcTx, common); + const { pending, inProgress } = txPool.executables; + + const isExecutable = await txPool.prepareTransaction( + transaction, + secretKey + ); + assert(isExecutable); // this tx will need to be executable to be moved to inProgress + const drainPromise = txPool.on("drain", () => { + // when a transaction is run, the miner removes the transaction from the + // pending queue and adds it to the inProgress pool. There is a lag + // between running the transaction and saving the block, which can cause + // a race condition for nonce generation. we will make this lag infinite + // here, because we never save the block. If the account's nonce is looked + // up, it will not have changed, so the pool will have to rely on the + // inProgress transactions to set the nonce of the next transaction + const pendingOrigin = pending.get(from); + inProgress.add(transaction); + pendingOrigin.removeBest(); + }); + txPool.drain(); + await drainPromise; + return transaction.nonce; + }; + + describe("with no pending/inProgress transactions from the account", async () => { + const accountNonce = 1; + beforeEach(() => { + blockchain.accounts.getNonceAndBalance = async () => { + return { + // have different starting nonce for these tests + nonce: Quantity.from(accountNonce), + // 1000 ether + balance: Quantity.from("0x3635c9adc5dea00000") + }; + }; + }); + + it("generates a nonce equal to the account's transaction count", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const transaction = TransactionFactory.fromRpc(rpcTx, common); + assert(transaction.nonce.isNull()); + + await txPool.prepareTransaction(transaction, secretKey); + + assert.deepStrictEqual(transaction.nonce.toNumber(), accountNonce); + }); + + it("allows a transaction with nonce equal to the account's transaction count", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const transaction = TransactionFactory.fromRpc( + { ...rpcTx, nonce: Quantity.toString(accountNonce) }, + common + ); + assert.deepStrictEqual(transaction.nonce.toNumber(), accountNonce); + + await addTxToPoolAndVerify(txPool, transaction, secretKey, "pending"); + }); + + it("allows a transaction with nonce greater than the account's transaction count", async () => { + const futureNonce = accountNonce + 1; + const txPool = new TransactionPool(options.miner, blockchain, origins); + const transaction = TransactionFactory.fromRpc( + { ...rpcTx, nonce: Quantity.toString(futureNonce) }, + common + ); + assert.deepStrictEqual(transaction.nonce.toNumber(), futureNonce); + + await addTxToPoolAndVerify(txPool, transaction, secretKey, "queued"); + }); + + it("rejects a transaction with nonce less than the account's transaction count", async () => { + const nonce = "0x0"; + const txPool = new TransactionPool(options.miner, blockchain, origins); + const transaction = TransactionFactory.fromRpc( + { ...rpcTx, nonce }, + common + ); + assert.deepStrictEqual(transaction.nonce.toString(), nonce); + + await assert.rejects( + txPool.prepareTransaction(transaction, secretKey), + { + code: -32000, + message: + "the tx doesn't have the correct nonce. account has nonce of: 1 tx has nonce of: 0" + } + ); + }); + }); + + describe("with inProgress transactions from the account and no pending transactions from the account", async () => { + it("generates a nonce equal to the highest nonce of inProgress transactions from the account plus 1", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const inProgressTxNonce = await addTxToInProgress(txPool); + + const transaction = TransactionFactory.fromRpc(rpcTx, common); + assert(transaction.nonce.isNull()); + + await txPool.prepareTransaction(transaction, secretKey); + assert.strictEqual( + transaction.nonce.toNumber(), + inProgressTxNonce.toNumber() + 1 + ); + }); + + it("allows a transaction with nonce equal to the highest nonce of inProgress transactions from the account plus 1", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const inProgressTxNonce = await addTxToInProgress(txPool); + + const nonce = Quantity.toString(inProgressTxNonce.toNumber() + 1); + const transaction = TransactionFactory.fromRpc( + { ...rpcTx, nonce }, + common + ); + + await addTxToPoolAndVerify(txPool, transaction, secretKey, "pending"); + assert.strictEqual( + transaction.nonce.toNumber(), + inProgressTxNonce.toNumber() + 1 + ); + }); + + it("allows a transaction with nonce greater than the highest nonce of inProgress transactions from the account plus 1", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const inProgressTxNonce = await addTxToInProgress(txPool); + + const nonce = Quantity.toString(inProgressTxNonce.toNumber() + 2); + const transaction = TransactionFactory.fromRpc( + { ...rpcTx, nonce }, + common + ); + + await addTxToPoolAndVerify(txPool, transaction, secretKey, "queued"); + assert.strictEqual( + transaction.nonce.toNumber(), + inProgressTxNonce.toNumber() + 2 + ); + }); + + it("rejects a transaction with nonce less than the highest nonce of inProgress transactions from the account plus 1", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const inProgressTxNonce = await addTxToInProgress(txPool); + + const transaction = TransactionFactory.fromRpc( + { ...rpcTx, nonce: inProgressTxNonce.toString() }, + common + ); + + await assert.rejects( + txPool.prepareTransaction(transaction, secretKey), + { + code: -32000, + message: + "the tx doesn't have the correct nonce. account has nonce of: 1 tx has nonce of: 0" + } + ); + }); + }); + + describe("with pending transactions from the account", async () => { + it("generates a nonce equal to the highest nonce of pending transactions from the account plus 1", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const inProgressTxNonce = await addTxToInProgress(txPool); + + const nonce = Quantity.toString(inProgressTxNonce.toNumber() + 1); + const transaction = TransactionFactory.fromRpc( + { ...rpcTx, nonce }, + common + ); + + await addTxToPoolAndVerify(txPool, transaction, secretKey, "pending"); + assert.strictEqual( + transaction.nonce.toNumber(), + inProgressTxNonce.toNumber() + 1 + ); + + const transaction2 = TransactionFactory.fromRpc(rpcTx, common); + assert(transaction2.nonce.isNull()); + + await addTxToPoolAndVerify(txPool, transaction2, secretKey, "pending"); + assert.strictEqual( + transaction2.nonce.toString(), + Quantity.toString(transaction.nonce.toNumber() + 1) + ); + }); + + it("allows a transaction with nonce equal to the highest nonce of pending transactions from the account plus 1", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const inProgressTxNonce = await addTxToInProgress(txPool); + + const nonce = Quantity.toString(inProgressTxNonce.toNumber() + 1); + const transaction = TransactionFactory.fromRpc( + { ...rpcTx, nonce }, + common + ); + + await addTxToPoolAndVerify(txPool, transaction, secretKey, "pending"); + assert.strictEqual( + transaction.nonce.toNumber(), + inProgressTxNonce.toNumber() + 1 + ); + + const transaction2Nonce = Quantity.toString( + transaction.nonce.toNumber() + 1 + ); + const transaction2 = TransactionFactory.fromRpc( + { ...rpcTx, nonce: transaction2Nonce }, + common + ); + + await addTxToPoolAndVerify(txPool, transaction2, secretKey, "pending"); + assert.strictEqual(transaction2.nonce.toString(), transaction2Nonce); + }); + + it("allows a transaction with nonce greater than the highest nonce of pending transactions from the account plus 1", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const inProgressTxNonce = await addTxToInProgress(txPool); + + const nonce = Quantity.toString(inProgressTxNonce.toNumber() + 1); + const transaction = TransactionFactory.fromRpc( + { ...rpcTx, nonce }, + common + ); + + await addTxToPoolAndVerify(txPool, transaction, secretKey, "pending"); + assert.strictEqual( + transaction.nonce.toNumber(), + inProgressTxNonce.toNumber() + 1 + ); + + const transaction2Nonce = Quantity.toString( + transaction.nonce.toNumber() + 2 + ); + const transaction2 = TransactionFactory.fromRpc( + { ...rpcTx, nonce: transaction2Nonce }, + common + ); + + await addTxToPoolAndVerify(txPool, transaction2, secretKey, "queued"); + assert.strictEqual(transaction2.nonce.toString(), transaction2Nonce); + }); }); - txPool.drain(); - await drainPromise; - - const anotherTransaction = TransactionFactory.fromRpc(rpcTx, common); - // our transaction doesn't have a nonce up front. - assert(anotherTransaction.nonce.isNull()); - await txPool.prepareTransaction(anotherTransaction, secretKey); - // after it's prepared by the txPool, an appropriate nonce for the account is set - assert.strictEqual(anotherTransaction.nonce.toBigInt(), 1n); }); }); From c2551d5b8d25a549ec67746277ea90729cabbac7 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Fri, 5 Aug 2022 11:23:21 -0400 Subject: [PATCH 07/21] fix logic for finding highest nonce by origin --- src/chains/ethereum/ethereum/src/transaction-pool.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/transaction-pool.ts b/src/chains/ethereum/ethereum/src/transaction-pool.ts index 79f2f11652..3999e72da1 100644 --- a/src/chains/ethereum/ethereum/src/transaction-pool.ts +++ b/src/chains/ethereum/ethereum/src/transaction-pool.ts @@ -80,9 +80,8 @@ function findHighestNonceByOrigin(set: Set, origin: string) { let highestNonce: bigint = null; for (const transaction of set) { if ( - (transaction.from.toString() === origin && - transaction.nonce.toBigInt() > highestNonce) || - highestNonce === null + transaction.from.toString() === origin && + (highestNonce === null || transaction.nonce.toBigInt() > highestNonce) ) { highestNonce = transaction.nonce.toBigInt(); } From e7547ad89fa9da74c2fde62583e5934b81bfdbe9 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Fri, 5 Aug 2022 11:24:39 -0400 Subject: [PATCH 08/21] queue transactions using semaphore --- .../ethereum/ethereum/src/transaction-pool.ts | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/src/chains/ethereum/ethereum/src/transaction-pool.ts b/src/chains/ethereum/ethereum/src/transaction-pool.ts index 3999e72da1..ca654e1ae2 100644 --- a/src/chains/ethereum/ethereum/src/transaction-pool.ts +++ b/src/chains/ethereum/ethereum/src/transaction-pool.ts @@ -13,6 +13,7 @@ import { } from "@ganache/ethereum-utils"; import { EthereumInternalOptions } from "@ganache/ethereum-options"; import { Executables } from "./miner/executables"; +import Semaphore from "semaphore"; import { TypedTransaction } from "@ganache/ethereum-transaction"; /** @@ -130,6 +131,7 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { #priceBump: bigint; #blockchain: Blockchain; + #originsQueue: Map = new Map(); constructor( options: EthereumInternalOptions["miner"], blockchain: Blockchain, @@ -151,6 +153,31 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { Promise<{ balance: Quantity; nonce: Quantity }> >(); + public async prepareTransaction( + transaction: TypedTransaction, + secretKey?: Data + ) { + const origin = transaction.from.toString(); + console.log(`processing origin ${origin}`); + let queueForOrigin: Semaphore; + if (!(queueForOrigin = this.#originsQueue.get(origin))) { + console.log("making new semaphore"); + queueForOrigin = Semaphore(1); + this.#originsQueue.set(origin, queueForOrigin); + } else { + console.log(`semaphore already existed`); + } + await new Promise(resolve => queueForOrigin.take(resolve)); + try { + // check if transaction is in origins, if not add it + // if so, use semaphore to call prepareTransaction + // if not, just call prepare transaction + return await this._prepareTransaction(transaction, secretKey); + } finally { + queueForOrigin.leave(); + } + } + /** * Inserts a transaction into the pending queue, if executable, or future pool * if not. @@ -159,7 +186,7 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { * @param secretKey - * @returns data that can be used to drain the queue */ - public async prepareTransaction( + public async _prepareTransaction( transaction: TypedTransaction, secretKey?: Data ) { From be6b64672dec6689a06fdead78fdde7639bce821 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Fri, 5 Aug 2022 11:27:23 -0400 Subject: [PATCH 09/21] remove logging --- src/chains/ethereum/ethereum/src/transaction-pool.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/transaction-pool.ts b/src/chains/ethereum/ethereum/src/transaction-pool.ts index ca654e1ae2..a745f6b68e 100644 --- a/src/chains/ethereum/ethereum/src/transaction-pool.ts +++ b/src/chains/ethereum/ethereum/src/transaction-pool.ts @@ -158,14 +158,10 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { secretKey?: Data ) { const origin = transaction.from.toString(); - console.log(`processing origin ${origin}`); let queueForOrigin: Semaphore; if (!(queueForOrigin = this.#originsQueue.get(origin))) { - console.log("making new semaphore"); queueForOrigin = Semaphore(1); this.#originsQueue.set(origin, queueForOrigin); - } else { - console.log(`semaphore already existed`); } await new Promise(resolve => queueForOrigin.take(resolve)); try { From ab14e24660773a1491111a74310e76c0baf3aa82 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Fri, 5 Aug 2022 12:05:53 -0400 Subject: [PATCH 10/21] only reference private variable once --- src/chains/ethereum/ethereum/src/transaction-pool.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/transaction-pool.ts b/src/chains/ethereum/ethereum/src/transaction-pool.ts index a745f6b68e..022f2d6e8a 100644 --- a/src/chains/ethereum/ethereum/src/transaction-pool.ts +++ b/src/chains/ethereum/ethereum/src/transaction-pool.ts @@ -158,10 +158,11 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { secretKey?: Data ) { const origin = transaction.from.toString(); + const originsQueue = this.#originsQueue; let queueForOrigin: Semaphore; - if (!(queueForOrigin = this.#originsQueue.get(origin))) { + if (!(queueForOrigin = originsQueue.get(origin))) { queueForOrigin = Semaphore(1); - this.#originsQueue.set(origin, queueForOrigin); + originsQueue.set(origin, queueForOrigin); } await new Promise(resolve => queueForOrigin.take(resolve)); try { From a9b2de4b014ac1ce9108ad4d488111f521457a8a Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Fri, 5 Aug 2022 12:50:44 -0400 Subject: [PATCH 11/21] add tests for some race conditions --- .../ethereum/tests/transaction-pool.test.ts | 76 ++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-) diff --git a/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts b/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts index 7e7fa74a36..940b13f5a8 100644 --- a/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts +++ b/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts @@ -30,6 +30,7 @@ function findIn( describe("transaction pool", async () => { let rpcTx: Transaction; let from: string; + let addresses: string[] = []; let secretKey: Data; let common: Common; let blockchain: any; @@ -44,7 +45,8 @@ describe("transaction pool", async () => { const beforeEachSetup = () => { const wallet = new Wallet(options.wallet); - [from] = wallet.addresses; + addresses = wallet.addresses; + [from] = addresses; secretKey = wallet.unlockedAccounts.get(from); rpcTx = { from: from, @@ -513,6 +515,78 @@ describe("transaction pool", async () => { transaction.serialized.toString() ); }); + + /** + * The plan is to queue up some transactions (from the same origin) without + * awaiting them. We'll loop over each of the subsequent promises and await + * them in the order they were sent. Once one is awaited, confirm that it is + * now in the pending queue, and that those promises which we _haven't_ yet + * awaited are _not_ yet in the queue (we actually check if they've been + * hashed yet, because this happens right before they're added to the queue) + */ + it("adds same-origin transactions in series", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const { pending } = txPool.executables; + const promises: Map> = new Map(); + const notInPool: TypedTransaction[] = []; + + for (let i = 0; i < 10; i++) { + const transaction = TransactionFactory.fromRpc(rpcTx, common); + promises.set( + transaction, + txPool.prepareTransaction(transaction, secretKey) + ); + notInPool.push(transaction); + } + + notInPool.reverse(); + for (const [transaction, promise] of promises.entries()) { + const isExecutable = await promise; + assert(isExecutable); + const found = findIn(transaction.hash.toBuffer(), pending); + assert.strictEqual(found.hash, transaction.hash); + notInPool.pop(); + for (const unpooled of notInPool) { + // the transaction hasn't been signed and hashed yet cause it hasn't + // been through the pool + assert.strictEqual(unpooled.hash, undefined); + } + } + }); + + /** + * The plan is to queue up some transactions (from unique origins) and wait + * for _any one_ of them to be done. We'll then loop (backwards, so the most + * recently sent transaction is checked first) over each of the transactions + * and confirm that they are in the pending executables pool, even though we + * haven't technically awaited them. Because they are queued in parallel, w + * hen one is done, they should all be done. + * + * Note: Obviously this reasoning breaks down eventually, but the test timed + * out from too many transactions before it failed. + * + */ + it("adds unique-origin transactions in parallel", async () => { + const txPool = new TransactionPool(options.miner, blockchain, origins); + const { pending } = txPool.executables; + const promises = []; + const transactions: TypedTransaction[] = []; + + for (let i = 0; i < 10; i++) { + const uniqueOrigin = addresses[i]; + const transaction = TransactionFactory.fromRpc( + { ...rpcTx, from: uniqueOrigin }, + common + ); + promises.push(txPool.prepareTransaction(transaction, secretKey)); + } + await Promise.race(promises); + for (let i = transactions.length - 1; i >= 0; i--) { + const transaction = transactions[i]; + const found = findIn(transaction.hash.toBuffer(), pending); + assert.strictEqual(found.hash, transaction.hash); + } + }); }); describe("nonce generation and validation", async () => { From 38d6d029218103eae1954b7ffb5308870e4118f1 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Mon, 8 Aug 2022 16:02:41 -0400 Subject: [PATCH 12/21] fix typo in comment --- src/chains/ethereum/ethereum/src/transaction-pool.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/chains/ethereum/ethereum/src/transaction-pool.ts b/src/chains/ethereum/ethereum/src/transaction-pool.ts index 022f2d6e8a..72f645e709 100644 --- a/src/chains/ethereum/ethereum/src/transaction-pool.ts +++ b/src/chains/ethereum/ethereum/src/transaction-pool.ts @@ -40,7 +40,7 @@ function shouldReplace( } // if the transaction being replaced is in the middle of being mined, we can't - // replpace it so let's back out early + // replace it so let's back out early if (replacee.locked) { throw new CodedError( TRANSACTION_LOCKED, From eb0eed42a7732aa61ca5fe71f141d202777c1f52 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Mon, 8 Aug 2022 16:31:10 -0400 Subject: [PATCH 13/21] cache account balance with inProgress txs --- .../ethereum/src/miner/executables.ts | 10 +- .../ethereum/ethereum/src/miner/miner.ts | 21 ++- .../ethereum/ethereum/src/transaction-pool.ts | 131 +++++++++--------- .../ethereum/tests/transaction-pool.test.ts | 11 +- 4 files changed, 105 insertions(+), 68 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/miner/executables.ts b/src/chains/ethereum/ethereum/src/miner/executables.ts index 2d9430d5f0..ed48f57946 100644 --- a/src/chains/ethereum/ethereum/src/miner/executables.ts +++ b/src/chains/ethereum/ethereum/src/miner/executables.ts @@ -1,7 +1,13 @@ import { TypedTransaction } from "@ganache/ethereum-transaction"; -import { Heap } from "@ganache/utils"; +import { Heap, Quantity } from "@ganache/utils"; +export type InProgressData = { + transaction: TypedTransaction; + originBalance: Quantity; +}; + +export type InProgress = Map>; export type Executables = { - inProgress: Set; + inProgress: InProgress; pending: Map>; }; diff --git a/src/chains/ethereum/ethereum/src/miner/miner.ts b/src/chains/ethereum/ethereum/src/miner/miner.ts index 49b9a9cb57..2e731129cd 100644 --- a/src/chains/ethereum/ethereum/src/miner/miner.ts +++ b/src/chains/ethereum/ethereum/src/miner/miner.ts @@ -343,11 +343,28 @@ export default class Miner extends Emittery<{ numTransactions++; const pendingOrigin = pending.get(origin); - inProgress.add(best); + const inProgressOrigin = inProgress.get(origin); + const { balance } = await vm.stateManager.getAccount({ + buf: Quantity.toBuffer(origin) + } as any); + const inProgressData = { + transaction: best, + originBalance: Quantity.from(balance.toBuffer()) + }; + + if (inProgressOrigin) { + inProgressOrigin.add(inProgressData); + } else { + inProgress.set(origin, new Set([inProgressData])); + } best.once("finalized").then(() => { // it is in the database (or thrown out) so delete it from the // `inProgress` Set - inProgress.delete(best); + const inProgressOrigin = inProgress.get(origin); + inProgressOrigin.delete(inProgressData); + if (inProgressOrigin.size === 0) { + inProgress.delete(origin); + } }); // since this transaction was successful, remove it from the "pending" diff --git a/src/chains/ethereum/ethereum/src/transaction-pool.ts b/src/chains/ethereum/ethereum/src/transaction-pool.ts index 72f645e709..a138796628 100644 --- a/src/chains/ethereum/ethereum/src/transaction-pool.ts +++ b/src/chains/ethereum/ethereum/src/transaction-pool.ts @@ -12,7 +12,7 @@ import { INSUFFICIENT_FUNDS } from "@ganache/ethereum-utils"; import { EthereumInternalOptions } from "@ganache/ethereum-options"; -import { Executables } from "./miner/executables"; +import { Executables, InProgressData } from "./miner/executables"; import Semaphore from "semaphore"; import { TypedTransaction } from "@ganache/ethereum-transaction"; @@ -77,17 +77,14 @@ function shouldReplace( return true; } } -function findHighestNonceByOrigin(set: Set, origin: string) { - let highestNonce: bigint = null; - for (const transaction of set) { - if ( - transaction.from.toString() === origin && - (highestNonce === null || transaction.nonce.toBigInt() > highestNonce) - ) { - highestNonce = transaction.nonce.toBigInt(); - } + +function validateSufficientFunds(cost: bigint, balance: bigint) { + if (balance < cost) { + throw new CodedError( + INSUFFICIENT_FUNDS, + JsonRpcErrorCode.TRANSACTION_REJECTED + ); } - return highestNonce; } function byNonce(values: TypedTransaction[], a: number, b: number) { @@ -144,14 +141,10 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { this.#priceBump = options.priceBump; } public readonly executables: Executables = { - inProgress: new Set(), + inProgress: new Map(), pending: new Map() }; public readonly origins: Map>; - readonly #accountPromises = new Map< - string, - Promise<{ balance: Quantity; nonce: Quantity }> - >(); public async prepareTransaction( transaction: TypedTransaction, @@ -202,18 +195,6 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { const origin = from.toString(); - // We await the `transactorNoncePromise` async request to ensure we process - // transactions in FIFO order *by account*. We look up accounts because - // ganache fills in missing nonces automatically, and we need to do it in - // order. - // The trick here is that we might actually get the next nonce from the - // account's pending executable transactions, not the account... - // But another transaction might currently be getting the nonce from the - // account, if it is, we need to wait for it to be done doing that. Hence: - let transactorPromise = this.#accountPromises.get(origin); - if (transactorPromise) { - await transactorPromise; - } // if the user called sendTransaction or sendRawTransaction, effectiveGasPrice // hasn't been set yet on the tx. calculating the effectiveGasPrice requires // the block context, so we need to set it outside of the transaction. this @@ -244,30 +225,15 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { // `executableOriginTransactions` map, always taking the highest of the two. let highestNonce = 0n; - if (!transactorPromise) { - transactorPromise = this.#blockchain.accounts.getNonceAndBalance(from); - this.#accountPromises.set(origin, transactorPromise); - transactorPromise.then(() => { - this.#accountPromises.delete(origin); - }); - } - const transactor = await transactorPromise; - - const cost = + const transactionCost = transaction.gas.toBigInt() * transaction.maxGasPrice().toBigInt() + transaction.value.toBigInt(); - if (transactor.balance.toBigInt() < cost) { - throw new CodedError( - INSUFFICIENT_FUNDS, - JsonRpcErrorCode.TRANSACTION_REJECTED - ); - } const origins = this.origins; const queuedOriginTransactions = origins.get(origin); let transactionPlacement = TriageOption.FutureQueue; - const { pending, inProgress } = this.executables; + const { pending } = this.executables; let executableOriginTransactions = pending.get(origin); const priceBump = this.#priceBump; @@ -316,23 +282,38 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { // origin's heap we are executable. transactionPlacement = TriageOption.Executable; } + // we've gotten the account's nonce the synchronous way (by looking at + // the pending queue), but we still need to look up the account's balance + const { balance } = await this.#blockchain.accounts.getNonceAndBalance( + from + ); + validateSufficientFunds(transactionCost, balance.toBigInt()); } else { // since we don't have any executable transactions at the moment, we need // to find our nonce from the account itself... - const transactorNonce = transactor.nonce.toBigInt(); - const highestNonceFromOrigin = findHighestNonceByOrigin( - inProgress, - origin - ); - - const useHighestNonce = - highestNonceFromOrigin !== null && - highestNonceFromOrigin >= transactorNonce; + const { + transaction: latestInProgressTransaction, + originBalance: balanceAfterLatestInProgressTransaction + } = this.#getLatestInProgressFromOrigin(origin); + + const hasInProgressFromOrigin = latestInProgressTransaction !== null; + + let balance: bigint; + let effectiveNonce: bigint; + if (hasInProgressFromOrigin) { + const highestInProgressNonce = latestInProgressTransaction.nonce; + effectiveNonce = highestInProgressNonce.toBigInt() + 1n; + balance = balanceAfterLatestInProgressTransaction.toBigInt(); + } else { + const transactor = await this.#blockchain.accounts.getNonceAndBalance( + from + ); + effectiveNonce = transactor.nonce.toBigInt(); + balance = transactor.balance.toBigInt(); + } - const effectiveNonce = useHighestNonce - ? highestNonceFromOrigin + 1n - : transactorNonce || 0n; + validateSufficientFunds(transactionCost, balance); if (txNonce === void 0) { // if we don't have a transactionNonce, just use the account's next @@ -393,7 +374,7 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { if (secretKey) { transaction.signAndHash(secretKey.toBuffer()); } - + console.log(`txNonce ${txNonce}, origin: ${origin}`); switch (transactionPlacement) { case TriageOption.Executable: // if it is executable add it to the executables queue @@ -455,7 +436,6 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { public clear() { this.origins.clear(); - this.#accountPromises.clear(); this.executables.pending.clear(); } @@ -495,11 +475,14 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { } // and finally transactions that have just been processed, but not yet saved - for (let tx of inProgress) { - if (tx.hash.toBuffer().equals(transactionHash)) { - return tx; + for (const [_, originData] of inProgress) + if (originData) { + for (let { transaction } of originData) { + if (transaction.hash.toBuffer().equals(transactionHash)) { + return transaction; + } + } } - } return null; } @@ -526,4 +509,26 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { return null; }; + + readonly #getLatestInProgressFromOrigin = (origin: string) => { + let highestNonceData: InProgressData = { + transaction: null, + originBalance: null + }; + let highestNonce: bigint = null; + const originData = this.executables.inProgress.get(origin); + if (originData) { + for (const inProgressData of originData) { + const { transaction } = inProgressData; + if ( + transaction.from.toString() === origin && + (highestNonce === null || transaction.nonce.toBigInt() > highestNonce) + ) { + highestNonce = transaction.nonce.toBigInt(); + highestNonceData = inProgressData; + } + } + } + return highestNonceData; + }; } diff --git a/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts b/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts index 940b13f5a8..7b21f80c4d 100644 --- a/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts +++ b/src/chains/ethereum/ethereum/tests/transaction-pool.test.ts @@ -653,7 +653,16 @@ describe("transaction pool", async () => { // up, it will not have changed, so the pool will have to rely on the // inProgress transactions to set the nonce of the next transaction const pendingOrigin = pending.get(from); - inProgress.add(transaction); + const inProgressOrigin = inProgress.get(from); + const data = { + transaction, + originBalance: Quantity.from("0x3635c9adc5dea00000") + }; + if (inProgressOrigin) { + inProgressOrigin.add(data); + } else { + inProgress.set(from, new Set([data])); + } pendingOrigin.removeBest(); }); txPool.drain(); From eb3752516359b7f018df8e1a2d6da1bad21bceb5 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Mon, 8 Aug 2022 16:37:32 -0400 Subject: [PATCH 14/21] add comments --- src/chains/ethereum/ethereum/src/transaction-pool.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/transaction-pool.ts b/src/chains/ethereum/ethereum/src/transaction-pool.ts index a138796628..aba8b88b41 100644 --- a/src/chains/ethereum/ethereum/src/transaction-pool.ts +++ b/src/chains/ethereum/ethereum/src/transaction-pool.ts @@ -289,9 +289,8 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { ); validateSufficientFunds(transactionCost, balance.toBigInt()); } else { - // since we don't have any executable transactions at the moment, we need - // to find our nonce from the account itself... - + // we don't have any pending executable transactions, but we could have + // some inProgress executable transactions const { transaction: latestInProgressTransaction, originBalance: balanceAfterLatestInProgressTransaction @@ -306,6 +305,8 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { effectiveNonce = highestInProgressNonce.toBigInt() + 1n; balance = balanceAfterLatestInProgressTransaction.toBigInt(); } else { + // if we don't have in progress transactions either, we'll need to find + // our nonce from the account itself const transactor = await this.#blockchain.accounts.getNonceAndBalance( from ); @@ -316,8 +317,6 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { validateSufficientFunds(transactionCost, balance); if (txNonce === void 0) { - // if we don't have a transactionNonce, just use the account's next - // nonce and mark as executable txNonce = effectiveNonce; transaction.nonce = Quantity.from(txNonce); transactionPlacement = TriageOption.Executable; From be16adf67a8fc61fd5fe632093bcfbedb5f69959 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Mon, 8 Aug 2022 16:40:26 -0400 Subject: [PATCH 15/21] jsdoc comments --- .../ethereum/ethereum/src/transaction-pool.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/chains/ethereum/ethereum/src/transaction-pool.ts b/src/chains/ethereum/ethereum/src/transaction-pool.ts index aba8b88b41..7507855ade 100644 --- a/src/chains/ethereum/ethereum/src/transaction-pool.ts +++ b/src/chains/ethereum/ethereum/src/transaction-pool.ts @@ -78,6 +78,11 @@ function shouldReplace( } } +/** + * Throws insufficient funds error if `balance` < `cost`. + * @param cost The transaction cost. + * @param balance The account's balance. + */ function validateSufficientFunds(cost: bigint, balance: bigint) { if (balance < cost) { throw new CodedError( @@ -509,6 +514,14 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { return null; }; + /** + * Searches all in progress transactions from the specified origin for the + * transaction with the highest nonce. + * @param origin The origin to search. + * @returns The in progress transaction with the highest nonce from the + * specified origin, along with the account's balance after running that + * transaction. + */ readonly #getLatestInProgressFromOrigin = (origin: string) => { let highestNonceData: InProgressData = { transaction: null, From 82bd7b51c1d997a77ad932ea97a88b4c0610e8c7 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Mon, 8 Aug 2022 16:44:59 -0400 Subject: [PATCH 16/21] rearranging --- .../ethereum/ethereum/src/transaction-pool.ts | 25 +++++++------------ 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/transaction-pool.ts b/src/chains/ethereum/ethereum/src/transaction-pool.ts index 7507855ade..fad18f940f 100644 --- a/src/chains/ethereum/ethereum/src/transaction-pool.ts +++ b/src/chains/ethereum/ethereum/src/transaction-pool.ts @@ -214,22 +214,6 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { transaction.updateEffectiveGasPrice(baseFeePerGas); } - // we should _probably_ cache `highestNonce`, but it's actually a really hard thing to cache as the current highest - // nonce might be invalidated (like if the sender doesn't have enough funds), so we'd have to go back to the previous - // highest nonce... but what if that previous highest nonce was also invalidated?! we have to go back to the... you - // get the picture. - // So... we currently do things sub-optimally: - // if we currently have txs in `executableOriginTransactions`, we iterate over them to find the highest nonce - // and use that. Otherwise, we just fetch it from the database. - // Beware! There might still be race conditions here: - // * if the highest tx executes, which causes it to be removed from the `executableOriginTransactions` heap, - // then a new tx comes in _before_ the block is persisted to the database, the nonce might be of the second - // tx would be too low. - // * rough idea for a fix: transactions have a `finalize` method that is called _after_ the tx is saved. Maybe - // when tx's are executed their nonce is moved to a `highNonceByOrigin` map? We'd check this map in addition to the - // `executableOriginTransactions` map, always taking the highest of the two. - let highestNonce = 0n; - const transactionCost = transaction.gas.toBigInt() * transaction.maxGasPrice().toBigInt() + transaction.value.toBigInt(); @@ -250,6 +234,15 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { // check if a transaction with the same nonce is in the origin's // executables queue already. Replace the matching transaction or throw this // new transaction away as necessary. + + // we should _probably_ cache `highestNonce`, but it's actually a really hard thing to cache as the current highest + // nonce might be invalidated (like if the sender doesn't have enough funds), so we'd have to go back to the previous + // highest nonce... but what if that previous highest nonce was also invalidated?! we have to go back to the... you + // get the picture. + // rough idea for a fix: transactions have a `finalize` method that is called _after_ the tx is saved. Maybe + // when tx's are executed their nonce is moved to a `highNonceByOrigin` map? We'd check this map in addition to the + // `executableOriginTransactions` map, always taking the highest of the two. + let highestNonce = 0n; const pendingArray = executableOriginTransactions.array; // Notice: we're iterating over the raw heap array, which isn't // necessarily sorted From 37af2379e0afacdac22cfc145c7e136b5b2a2cdd Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Tue, 9 Aug 2022 10:14:46 -0400 Subject: [PATCH 17/21] remove logging --- src/chains/ethereum/ethereum/src/transaction-pool.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/chains/ethereum/ethereum/src/transaction-pool.ts b/src/chains/ethereum/ethereum/src/transaction-pool.ts index fad18f940f..0f5bd80ebe 100644 --- a/src/chains/ethereum/ethereum/src/transaction-pool.ts +++ b/src/chains/ethereum/ethereum/src/transaction-pool.ts @@ -371,7 +371,7 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { if (secretKey) { transaction.signAndHash(secretKey.toBuffer()); } - console.log(`txNonce ${txNonce}, origin: ${origin}`); + switch (transactionPlacement) { case TriageOption.Executable: // if it is executable add it to the executables queue From 7127d3867dbe15a1d83274ea24dfdee170d4775c Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Tue, 9 Aug 2022 10:26:06 -0400 Subject: [PATCH 18/21] remove unneeded var --- src/chains/ethereum/ethereum/src/miner/executables.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/miner/executables.ts b/src/chains/ethereum/ethereum/src/miner/executables.ts index ed48f57946..458490d2f8 100644 --- a/src/chains/ethereum/ethereum/src/miner/executables.ts +++ b/src/chains/ethereum/ethereum/src/miner/executables.ts @@ -6,8 +6,7 @@ export type InProgressData = { originBalance: Quantity; }; -export type InProgress = Map>; export type Executables = { - inProgress: InProgress; + inProgress: Map>; pending: Map>; }; From 5c1167715365cc9bb1646bd14ed95d1bb6dfe05b Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Tue, 9 Aug 2022 10:28:13 -0400 Subject: [PATCH 19/21] add comment --- src/chains/ethereum/ethereum/src/miner/miner.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/chains/ethereum/ethereum/src/miner/miner.ts b/src/chains/ethereum/ethereum/src/miner/miner.ts index 2e731129cd..9d90fafc3b 100644 --- a/src/chains/ethereum/ethereum/src/miner/miner.ts +++ b/src/chains/ethereum/ethereum/src/miner/miner.ts @@ -344,6 +344,8 @@ export default class Miner extends Emittery<{ const pendingOrigin = pending.get(origin); const inProgressOrigin = inProgress.get(origin); + // we cache the account balance with the inProgress transaction for + // an optimization in the transaction pool, so fetch it here const { balance } = await vm.stateManager.getAccount({ buf: Quantity.toBuffer(origin) } as any); From 17fab067dff4a7d3eb905a5b66a62d326528b253 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Tue, 9 Aug 2022 10:28:59 -0400 Subject: [PATCH 20/21] add type --- src/chains/ethereum/ethereum/src/miner/miner.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/miner/miner.ts b/src/chains/ethereum/ethereum/src/miner/miner.ts index 9d90fafc3b..70a0c49690 100644 --- a/src/chains/ethereum/ethereum/src/miner/miner.ts +++ b/src/chains/ethereum/ethereum/src/miner/miner.ts @@ -21,7 +21,7 @@ import { EthereumInternalOptions } from "@ganache/ethereum-options"; import replaceFromHeap from "./replace-from-heap"; import { EVMResult } from "@ethereumjs/vm/dist/evm/evm"; import { Params, TypedTransaction } from "@ganache/ethereum-transaction"; -import { Executables } from "./executables"; +import { Executables, InProgressData } from "./executables"; import { Block, RuntimeBlock } from "@ganache/ethereum-block"; import { makeStepEvent, @@ -349,7 +349,7 @@ export default class Miner extends Emittery<{ const { balance } = await vm.stateManager.getAccount({ buf: Quantity.toBuffer(origin) } as any); - const inProgressData = { + const inProgressData: InProgressData = { transaction: best, originBalance: Quantity.from(balance.toBuffer()) }; From 0b5fa5bd9bf4cf63ac730b3a55a70cdd538d4d04 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Tue, 9 Aug 2022 10:39:04 -0400 Subject: [PATCH 21/21] add comments to prepareTransaction --- .../ethereum/ethereum/src/transaction-pool.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/chains/ethereum/ethereum/src/transaction-pool.ts b/src/chains/ethereum/ethereum/src/transaction-pool.ts index 0f5bd80ebe..6825903492 100644 --- a/src/chains/ethereum/ethereum/src/transaction-pool.ts +++ b/src/chains/ethereum/ethereum/src/transaction-pool.ts @@ -151,6 +151,14 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { }; public readonly origins: Map>; + /** + * Queues transactions to be inserted into the pool via `_prepareTransaction` + * such that unique-origin transactions are added to the pool concurrently and + * same-origin transactions are queued in series. + * @param transaction The transaction to be added to the transaction pool. + * @param secretKey The optional key to sign and hash the transaction. + * @returns Data that can be used to drain the queue. + */ public async prepareTransaction( transaction: TypedTransaction, secretKey?: Data @@ -158,10 +166,14 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { const origin = transaction.from.toString(); const originsQueue = this.#originsQueue; let queueForOrigin: Semaphore; + // each unique origin gets its own semaphore rather than them all sharing + // one because we only need transactions from the same origin to be added in + // series if (!(queueForOrigin = originsQueue.get(origin))) { queueForOrigin = Semaphore(1); originsQueue.set(origin, queueForOrigin); } + // the semaphore will hold on `take` until previous uses have resolved await new Promise(resolve => queueForOrigin.take(resolve)); try { // check if transaction is in origins, if not add it @@ -169,6 +181,7 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> { // if not, just call prepare transaction return await this._prepareTransaction(transaction, secretKey); } finally { + // free up the semaphore once we finish queueForOrigin.leave(); } }