From 89f5adfcb436df89d13cf9e9206825cc45356898 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Mon, 16 Jan 2023 21:03:45 -0500 Subject: [PATCH 1/6] only await transaction finalization if executable --- src/chains/ethereum/ethereum/src/blockchain.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/chains/ethereum/ethereum/src/blockchain.ts b/src/chains/ethereum/ethereum/src/blockchain.ts index 8614fc024c..d279fe40b3 100644 --- a/src/chains/ethereum/ethereum/src/blockchain.ts +++ b/src/chains/ethereum/ethereum/src/blockchain.ts @@ -1058,7 +1058,12 @@ export default class Blockchain extends Emittery { if (this.#isPaused() || !this.#instamine) { return hash; } else { - if (this.#instamine && this.#options.miner.instamine === "eager") { + // if the transaction is not executable, we just have to return the hash + if ( + this.#instamine && + this.#options.miner.instamine === "eager" && + isExecutable + ) { // in eager instamine mode we must wait for the transaction to be saved // before we can return the hash const { status, error } = await transaction.once("finalized"); From 44e1ee9cc001e0df408b8b5a2169f2b5e9b4487d Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Mon, 16 Jan 2023 21:04:21 -0500 Subject: [PATCH 2/6] rearrange test to make more sense --- .../tests/api/eth/greedyInstamining.test.ts | 161 +++++++++--------- 1 file changed, 84 insertions(+), 77 deletions(-) diff --git a/src/chains/ethereum/ethereum/tests/api/eth/greedyInstamining.test.ts b/src/chains/ethereum/ethereum/tests/api/eth/greedyInstamining.test.ts index d681ce7d93..81cffde842 100644 --- a/src/chains/ethereum/ethereum/tests/api/eth/greedyInstamining.test.ts +++ b/src/chains/ethereum/ethereum/tests/api/eth/greedyInstamining.test.ts @@ -3,96 +3,103 @@ import assert from "assert"; describe("api", () => { describe("eth", () => { - describe("eager", () => { - it("when in strict instamine mode, does not mine before returning the tx hash", async () => { - const provider = await getProvider({ - miner: { instamine: "strict" } - }); - const accounts = await provider.send("eth_accounts"); - - const hash = await provider.send("eth_sendTransaction", [ - { - from: accounts[0], - to: accounts[1], - value: "0x1" - } - ]); - const receipt = await provider.send("eth_getTransactionReceipt", [ - hash - ]); - assert.strictEqual(receipt, null); - }); + describe("instamine modes (eager/strict)", () => { + describe("strict", () => { + it("when in strict instamine mode, does not mine before returning the tx hash", async () => { + const provider = await getProvider({ + miner: { instamine: "strict" } + }); + const accounts = await provider.send("eth_accounts"); - it("when in eager instamine mode, mines before returns in the tx hash", async () => { - const provider = await getProvider({ - miner: { instamine: "eager" } + const hash = await provider.send("eth_sendTransaction", [ + { + from: accounts[0], + to: accounts[1], + value: "0x1" + } + ]); + const receipt = await provider.send("eth_getTransactionReceipt", [ + hash + ]); + assert.strictEqual(receipt, null); }); - const accounts = await provider.send("eth_accounts"); - - const hash = await provider.send("eth_sendTransaction", [ - { - from: accounts[0], - to: accounts[1], - value: "0x1" - } - ]); - const receipt = await provider.send("eth_getTransactionReceipt", [ - hash - ]); - assert.notStrictEqual(receipt, null); }); - it("handles transaction balance errors, callback style", done => { - getProvider({ - miner: { instamine: "eager" }, - chain: { vmErrorsOnRPCResponse: true } - }).then(async provider => { - const [from, to] = await provider.send("eth_accounts"); - const balance = parseInt( - await provider.send("eth_getBalance", [from]), - 16 - ); - const gasCost = 99967968750001; - // send a transaction that will spend some of the balance - provider.request({ - method: "eth_sendTransaction", - params: [ - { - from, - to - } - ] + describe("eager", () => { + it("mines before returning the tx hash", async () => { + const provider = await getProvider({ + miner: { instamine: "eager" } }); + const accounts = await provider.send("eth_accounts"); - // send another transaction while the previous transaction is still - // pending. this transaction appears to have enough balance to run, - // so the transaction pool will accept it, but when it runs in the VM - // it won't have enough balance to run. - provider.send( + const hash = await provider.send("eth_sendTransaction", [ { - jsonrpc: "2.0", - id: "1", + from: accounts[0], + to: accounts[1], + value: "0x1" + } + ]); + const receipt = await provider.send("eth_getTransactionReceipt", [ + hash + ]); + assert.notStrictEqual(receipt, null); + }); + + it("handles transaction balance errors, callback style", done => { + getProvider({ + miner: { instamine: "eager" }, + chain: { vmErrorsOnRPCResponse: true } + }).then(async provider => { + const [from, to] = await provider.send("eth_accounts"); + const balance = parseInt( + await provider.send("eth_getBalance", [from]), + 16 + ); + const gasCost = 99967968750001; + // send a transaction that will spend some of the balance + provider.request({ method: "eth_sendTransaction", params: [ { from, - to, - value: `0x${(balance - gasCost).toString(16)}` + to } ] - }, - (e, r) => { - assert( - e.message.includes( - "sender doesn't have enough funds to send tx" - ) - ); - assert.strictEqual(e.message, (r as any).error.message); - assert.strictEqual((r as any).error.code, -32000); - assert.strictEqual(typeof (r as any).error.data.result, "string"); - done(); - } - ); + }); + + // send another transaction while the previous transaction is still + // pending. this transaction appears to have enough balance to run, + // so the transaction pool will accept it, but when it runs in the VM + // it won't have enough balance to run. + provider.send( + { + jsonrpc: "2.0", + id: "1", + method: "eth_sendTransaction", + params: [ + { + from, + to, + value: `0x${(balance - gasCost).toString(16)}` + } + ] + }, + (e, r) => { + assert( + e.message.includes( + "sender doesn't have enough funds to send tx" + ) + ); + assert.strictEqual(e.message, (r as any).error.message); + assert.strictEqual((r as any).error.code, -32000); + assert.strictEqual( + typeof (r as any).error.data.result, + "string" + ); + done(); + } + ); + }); }); }); }); From ceccbfc7b8c78ea09cea7484d4e53f5f8121e2e5 Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Mon, 16 Jan 2023 21:08:52 -0500 Subject: [PATCH 3/6] add new test --- .../tests/api/eth/greedyInstamining.test.ts | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/chains/ethereum/ethereum/tests/api/eth/greedyInstamining.test.ts b/src/chains/ethereum/ethereum/tests/api/eth/greedyInstamining.test.ts index 81cffde842..0626116237 100644 --- a/src/chains/ethereum/ethereum/tests/api/eth/greedyInstamining.test.ts +++ b/src/chains/ethereum/ethereum/tests/api/eth/greedyInstamining.test.ts @@ -45,6 +45,37 @@ describe("api", () => { assert.notStrictEqual(receipt, null); }); + it("returns the tx hash before mining for future-nonce transactions", async () => { + const provider = await getProvider({ + miner: { instamine: "eager" }, + chain: { vmErrorsOnRPCResponse: true } + }); + const [from, to] = await provider.send("eth_accounts"); + const futureNonceTx = { from, to, nonce: "0x1" }; + const futureNonceTxHash = await provider.send("eth_sendTransaction", [ + futureNonceTx + ]); + assert.notEqual(futureNonceTxHash, null); + const nullReceipt = await provider.send("eth_getTransactionReceipt", [ + futureNonceTxHash + ]); + assert.strictEqual(nullReceipt, null); + await provider.send("eth_subscribe", ["newHeads"]); + // send a transaction to fill the nonce gap + await provider.send("eth_sendTransaction", [{ from, to }]); + // usually we don't have to wait for these messages in eager mode, + // but because instamine only mines one tx per block, we need to + // wait for our future-nonce tx to be mined before fetching the + // receipt + await provider.once("message"); + await provider.once("message"); + // now our nonce gap is filled so the original tx is mined + const receipt = await provider.send("eth_getTransactionReceipt", [ + futureNonceTxHash + ]); + assert.notStrictEqual(receipt, null); + }); + it("handles transaction balance errors, callback style", done => { getProvider({ miner: { instamine: "eager" }, From 9e5d396af5c7c71e089c741e16657d5b70426c3a Mon Sep 17 00:00:00 2001 From: MicaiahReid Date: Mon, 16 Jan 2023 21:09:00 -0500 Subject: [PATCH 4/6] rename test --- .../api/eth/{greedyInstamining.test.ts => instamine.test.ts} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/chains/ethereum/ethereum/tests/api/eth/{greedyInstamining.test.ts => instamine.test.ts} (100%) diff --git a/src/chains/ethereum/ethereum/tests/api/eth/greedyInstamining.test.ts b/src/chains/ethereum/ethereum/tests/api/eth/instamine.test.ts similarity index 100% rename from src/chains/ethereum/ethereum/tests/api/eth/greedyInstamining.test.ts rename to src/chains/ethereum/ethereum/tests/api/eth/instamine.test.ts From bc3325022e860fbbda78e83ade7080361cc2d434 Mon Sep 17 00:00:00 2001 From: David Murdoch Date: Fri, 17 Feb 2023 16:32:03 -0500 Subject: [PATCH 5/6] switch to a log instead --- .../ethereum/ethereum/src/blockchain.ts | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/blockchain.ts b/src/chains/ethereum/ethereum/src/blockchain.ts index d279fe40b3..e5904fa026 100644 --- a/src/chains/ethereum/ethereum/src/blockchain.ts +++ b/src/chains/ethereum/ethereum/src/blockchain.ts @@ -1054,16 +1054,22 @@ export default class Blockchain extends Emittery { process.nextTick(this.emit.bind(this), "pendingTransaction", transaction); } - const hash = transaction.hash; - if (this.#isPaused() || !this.#instamine) { + const { hash } = transaction; + const instamine = this.#instamine; + if (!instamine || this.#isPaused()) { return hash; } else { + const options = this.#options; // if the transaction is not executable, we just have to return the hash - if ( - this.#instamine && - this.#options.miner.instamine === "eager" && - isExecutable - ) { + if (instamine && options.miner.instamine === "eager") { + if (!isExecutable) { + // users have been confused about why ganache "hangs" when sending a + // transaction with a "too-high" nonce. This message should help them + // understand what's going on. + options.logging.logger.log( + `Transaction "${hash}" has a too-high nonce; this transaction has been added to the pool, and will be processed when its nonce is reached. See https://github.com/trufflesuite/ganache/issues/4165 for more information.` + ); + } // in eager instamine mode we must wait for the transaction to be saved // before we can return the hash const { status, error } = await transaction.once("finalized"); @@ -1072,7 +1078,7 @@ export default class Blockchain extends Emittery { // vmErrorsOnRPCResponse is enabled. if ( error && - (status === "rejected" || this.#options.chain.vmErrorsOnRPCResponse) + (status === "rejected" || options.chain.vmErrorsOnRPCResponse) ) throw error; } From c38acb9bdfc04ab41a931564938a194a263fa12a Mon Sep 17 00:00:00 2001 From: David Murdoch Date: Fri, 17 Feb 2023 18:14:15 -0500 Subject: [PATCH 6/6] update test --- .../ethereum/tests/api/eth/instamine.test.ts | 40 +++++++++++-------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/src/chains/ethereum/ethereum/tests/api/eth/instamine.test.ts b/src/chains/ethereum/ethereum/tests/api/eth/instamine.test.ts index 0626116237..bc3bf3a710 100644 --- a/src/chains/ethereum/ethereum/tests/api/eth/instamine.test.ts +++ b/src/chains/ethereum/ethereum/tests/api/eth/instamine.test.ts @@ -1,5 +1,6 @@ import getProvider from "../../helpers/getProvider"; import assert from "assert"; +import { Logger } from "@ganache/ethereum-options/typings/src/logging-options"; describe("api", () => { describe("eth", () => { @@ -45,33 +46,40 @@ describe("api", () => { assert.notStrictEqual(receipt, null); }); - it("returns the tx hash before mining for future-nonce transactions", async () => { + it("log a message about future-nonce transactions in eager mode", async () => { + let logger: Logger; + const logPromise = new Promise(resolve => { + logger = { + log: (msg: string) => { + const regex = + /Transaction "0x[a-zA-z0-9]{64}" has a too-high nonce; this transaction has been added to the pool, and will be processed when its nonce is reached\. See https:\/\/github.com\/trufflesuite\/ganache\/issues\/4165 for more information\./; + if (regex.test(msg)) resolve(true); + } + }; + }); + const provider = await getProvider({ + logging: { logger }, miner: { instamine: "eager" }, chain: { vmErrorsOnRPCResponse: true } }); const [from, to] = await provider.send("eth_accounts"); const futureNonceTx = { from, to, nonce: "0x1" }; - const futureNonceTxHash = await provider.send("eth_sendTransaction", [ + const futureNonceProm = provider.send("eth_sendTransaction", [ futureNonceTx ]); - assert.notEqual(futureNonceTxHash, null); - const nullReceipt = await provider.send("eth_getTransactionReceipt", [ - futureNonceTxHash - ]); - assert.strictEqual(nullReceipt, null); - await provider.send("eth_subscribe", ["newHeads"]); + // send a transaction to fill the nonce gap - await provider.send("eth_sendTransaction", [{ from, to }]); - // usually we don't have to wait for these messages in eager mode, - // but because instamine only mines one tx per block, we need to - // wait for our future-nonce tx to be mined before fetching the - // receipt - await provider.once("message"); - await provider.once("message"); + provider.send("eth_sendTransaction", [{ from, to }]); // we don't await this on purpose. + + const result = await Promise.race([futureNonceProm, logPromise]); + // `logPromise` should resolve before the the hash gets returned + // (logPromise returns true) + assert.strictEqual(result, true); + // now our nonce gap is filled so the original tx is mined const receipt = await provider.send("eth_getTransactionReceipt", [ - futureNonceTxHash + await futureNonceProm ]); assert.notStrictEqual(receipt, null); });