fix: save evm_mine blocks before returning (#3016)

* await _saving_ block before returning evm_mine

* delay emitting block til next event loop in eager blocktime mode

* remove manual awaitBlockSaving

* await `blockBeingSavedPromise` after mining

* add back newline

* add memdown and patch-package as dev dep

* add patch to memdown to delay _batch calls by 20ms

* add test for mining race condition

* remove patch package; monkey patch instead

* remove patch-package dependency

* assert memdown patch works

* add clarifying comment

* remove unneeded return

src/chains/ethereum/ethereum/package.json

@@ -106,6 +106,7 @@
     "fast-check": "2.17.0",
     "fs-extra": "9.0.1",
     "local-web-server": "4.2.1",
+    "memdown": "6.1.1",
     "mocha": "9.1.3",
     "nyc": "15.1.0",
     "solc": "0.8.11",

src/chains/ethereum/ethereum/src/blockchain.ts

@@ -469,7 +469,7 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {
       transaction.finalize("confirmed", transaction.execException);
     });
 
-    if (this.#instamine && options.miner.instamine === "eager") {
+    if (options.miner.instamine === "eager") {
       // in eager instamine mode we must delay the broadcast of new blocks
       await new Promise(resolve => {
         // we delay emitting blocks and blockLogs because we need to allow for:
@@ -535,12 +535,15 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {
     storageKeys: StorageKeys;
     transactions: TypedTransaction[];
   }) => {
-    this.#blockBeingSavedPromise = this.#blockBeingSavedPromise
-      .then(() => this.#saveNewBlock(blockData))
-      .then(this.#emitNewBlock);
+    this.#blockBeingSavedPromise = this.#blockBeingSavedPromise.then(() => {
+      const saveBlockProm = this.#saveNewBlock(blockData);
+      saveBlockProm.then(this.#emitNewBlock);
+      // blockBeingSavedPromise should await the block being _saved_, but doesn't
+      // need to await the block being emitted.
+      return saveBlockProm;
+    });
 
     await this.#blockBeingSavedPromise;
-    return;
   };
 
   coinbase: Address;
@@ -570,14 +573,15 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {
     timestamp?: number,
     onlyOneBlock: boolean = false
   ) => {
-    await this.#blockBeingSavedPromise;
     const nextBlock = this.#readyNextBlock(this.blocks.latest, timestamp);
+    const transactions = await this.#miner.mine(
+      nextBlock,
+      maxTransactions,
+      onlyOneBlock
+    );
+    await this.#blockBeingSavedPromise;
     return {
-      transactions: await this.#miner.mine(
-        nextBlock,
-        maxTransactions,
-        onlyOneBlock
-      ),
+      transactions,
       blockNumber: nextBlock.header.number.toBuffer()
     };
   };

src/chains/ethereum/ethereum/tests/api/evm/evm.test.ts

@@ -3,6 +3,7 @@ import assert from "assert";
 import { Data, Quantity } from "@ganache/utils";
 import { EthereumProvider } from "../../../src/provider";
 import { Transaction } from "@ganache/ethereum-transaction";
+import memdown from "memdown";
 
 function between(x: number, min: number, max: number) {
   return x >= min && x <= max;
@@ -132,6 +133,37 @@ describe("api", () => {
         const currentBlock = parseInt(await provider.send("eth_blockNumber"));
         assert.strictEqual(currentBlock, initialBlock + 1);
       });
+
+      it("should save the block before returning", async () => {
+        // slow down memdown's _batch function to consistently reproduce a past
+        // race condition where a block was mined and returned by evm_mine
+        // before it actually saved to the database. for history, the race
+        // condition issue is documented here:
+        // https://github.com/trufflesuite/ganache/issues/3060
+        const db = memdown();
+        let customBatchCalled = false;
+        db._batch = (...args) => {
+          setTimeout(() => {
+            customBatchCalled = true;
+            Reflect.apply(memdown.prototype._batch, db, args);
+          }, 20);
+        };
+
+        const options = { database: { db } };
+        const provider = await getProvider(options);
+        await provider.request({ method: "evm_mine", params: [{}] });
+
+        const block = await provider.request({
+          method: "eth_getBlockByNumber",
+          params: [`0x1`]
+        });
+        assert(
+          block,
+          `the block doesn't exist. evm_mine returned before saving the block`
+        );
+        // make sure our patch works
+        assert(customBatchCalled);
+      });
     });
 
     describe("evm_setAccountNonce", () => {