feat: replace legacyInstamine option with instamine=eager|strict (#…

…2013)

src/chains/ethereum/ethereum/src/api.ts

@@ -305,13 +305,15 @@ export default class EthereumApi implements Api {
     arg?: number | { timestamp?: number; blocks?: number }
   ): Promise<"0x0"> {
     const blockchain = this.#blockchain;
-    const vmErrorsOnRPCResponse = this.#options.chain.vmErrorsOnRPCResponse;
+    const options = this.#options;
+    const vmErrorsOnRPCResponse = options.chain.vmErrorsOnRPCResponse;
     // Since `typeof null === "object"` we have to guard against that
     if (arg !== null && typeof arg === "object") {
       let { blocks, timestamp } = arg;
       if (blocks == null) {
         blocks = 1;
       }
+      const strictMiner = options.miner.instamine === "strict";
       // TODO(perf): add an option to mine a bunch of blocks in a batch so
       // we can save them all to the database in one go.
       // Developers like to move the blockchain forward by thousands of blocks
@@ -322,15 +324,20 @@ export default class EthereumApi implements Api {
           timestamp,
           true
         );
-        // wait until the blocks are fully saved before mining the next ones
-        await new Promise(resolve => {
-          const off = blockchain.on("block", block => {
-            if (block.header.number.toBuffer().equals(blockNumber)) {
-              off();
-              resolve(void 0);
-            }
+
+        if (strictMiner) {
+          // in strict mode we have to wait until the blocks are fully saved
+          // before mining the next ones, in eager mode they've already been
+          // saved
+          await new Promise(resolve => {
+            const off = blockchain.on("block", ({ header: { number } }) => {
+              if (number.toBuffer().equals(blockNumber)) {
+                off();
+                resolve(void 0);
+              }
+            });
           });
-        });
+        }
         if (vmErrorsOnRPCResponse) {
           assertExceptionalTransactions(transactions);
         }
@@ -594,7 +601,7 @@ export default class EthereumApi implements Api {
    */
   @assertArgLength(0, 1)
   async miner_start(threads: number = 1) {
-    if (this.#options.miner.legacyInstamine === true) {
+    if (this.#options.miner.instamine === "eager") {
       const resumption = await this.#blockchain.resume(threads);
       // resumption can be undefined if the blockchain isn't currently paused
       if (
@@ -1646,12 +1653,12 @@ export default class EthereumApi implements Api {
       return receipt.toJSON(block, transaction, common);
     }
 
-    // if we are performing non-legacy instamining, then check to see if the
-    // transaction is pending so as to warn about the v7 breaking change
+    // if we are performing "strict" instamining, then check to see if the
+    // transaction is pending so as to warn about the v7 instamine changes
     const options = this.#options;
     if (
       options.miner.blockTime <= 0 &&
-      options.miner.legacyInstamine !== true &&
+      options.miner.instamine === "strict" &&
       this.#blockchain.isStarted()
     ) {
       const tx = this.#blockchain.transactions.transactionPool.find(txHash);

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

@@ -115,7 +115,7 @@ export type BlockchainOptions = {
   coinbase: Account;
   chainId: number;
   common: Common;
-  legacyInstamine: boolean;
+  instamine: "eager" | "strict";
   vmErrorsOnRPCResponse: boolean;
   logger: Logger;
 };
@@ -215,36 +215,8 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {
 
     this.#options = options;
     this.fallback = fallback;
-
-    const instamine = (this.#instamine =
-      !options.miner.blockTime || options.miner.blockTime <= 0);
-    const legacyInstamine = options.miner.legacyInstamine;
-
-    {
-      // warnings and errors
-      if (legacyInstamine) {
-        console.info(
-          "Legacy instamining, where transactions are fully mined before the hash is returned, is deprecated and will be removed in the future."
-        );
-      }
-
-      if (!instamine) {
-        if (legacyInstamine) {
-          console.info(
-            "Setting `legacyInstamine` to `true` has no effect when blockTime is non-zero"
-          );
-        }
-
-        if (options.chain.vmErrorsOnRPCResponse) {
-          console.info(
-            "Setting `vmErrorsOnRPCResponse` to `true` has no effect on transactions when blockTime is non-zero"
-          );
-        }
-      }
-    }
-
     this.coinbase = coinbase;
-
+    this.#instamine = !options.miner.blockTime || options.miner.blockTime <= 0;
     this.#database = new Database(options.database, this);
   }
 
@@ -339,9 +311,8 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {
             options.miner.blockGasLimit,
             initialAccounts
           );
-          blocks.earliest = blocks.latest = await this.#blockBeingSavedPromise.then(
-            ({ block }) => block
-          );
+          blocks.earliest = blocks.latest =
+            await this.#blockBeingSavedPromise.then(({ block }) => block);
         }
       }
 
@@ -476,6 +447,9 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {
     });
   };
 
+  /**
+   * Emit the block now that everything has been fully saved to the database
+   */
   #emitNewBlock = async (blockInfo: {
     block: Block;
     blockLogs: BlockLogs;
@@ -484,15 +458,21 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {
     const options = this.#options;
     const { block, blockLogs, transactions } = blockInfo;
 
-    // emit the block once everything has been fully saved to the database
     transactions.forEach(transaction => {
       transaction.finalize("confirmed", transaction.execException);
     });
 
-    if (this.#instamine && options.miner.legacyInstamine) {
-      // in legacy instamine mode we must delay the broadcast of new blocks
+    if (this.#instamine && options.miner.instamine === "eager") {
+      // in eager instamine mode we must delay the broadcast of new blocks
       await new Promise(resolve => {
-        process.nextTick(async () => {
+        // we delay emitting blocks and blockLogs because we need to allow for:
+        // ```
+        //  await provider.request({"method": "eth_sendTransaction"...)
+        //  await provider.once("message") // <- should work
+        // ```
+        // If we don't have this delay here the messages will be sent before
+        // the call has a chance to listen to the event.
+        setImmediate(async () => {
           // emit block logs first so filters can pick them up before
           // block listeners are notified
           await Promise.all([
@@ -975,11 +955,11 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {
     if (this.#isPaused() || !this.#instamine) {
       return hash;
     } else {
-      if (this.#instamine && this.#options.miner.legacyInstamine) {
-        // in legacyInstamine mode we must wait for the transaction to be saved
+      if (this.#instamine && this.#options.miner.instamine === "eager") {
+        // 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");
-        // in legacyInstamine mode we must throw on all rejected transaction
+        // in eager instamine mode we must throw on all rejected transaction
         // errors. We must also throw on `confirmed` transactions when
         // vmErrorsOnRPCResponse is enabled.
         if (
@@ -1438,17 +1418,13 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {
     );
 
     // #3 - Rerun every transaction in block prior to and including the requested transaction
-    const {
-      gas,
-      structLogs,
-      returnValue,
-      storage
-    } = await this.#traceTransaction(
-      newBlock.transactions[transaction.index.toNumber()],
-      trie,
-      newBlock,
-      options
-    );
+    const { gas, structLogs, returnValue, storage } =
+      await this.#traceTransaction(
+        newBlock.transactions[transaction.index.toNumber()],
+        trie,
+        newBlock,
+        options
+      );
 
     // #4 - Send results back
     return { gas, structLogs, returnValue, storage };

src/chains/ethereum/ethereum/src/miner/miner.ts

@@ -223,7 +223,6 @@ export default class Miner extends Emittery<{
 
     let keepMining = true;
     const priced = this.#priced;
-    const legacyInstamine = this.#options.legacyInstamine;
     const storageKeys: StorageKeys = new Map();
     let blockTransactions: TypedTransaction[];
     do {
@@ -424,13 +423,7 @@ export default class Miner extends Emittery<{
         storageKeys
       );
       block = finalizedBlockData.block;
-      const emitBlockProm = this.emit("block", finalizedBlockData);
-      if (legacyInstamine === true) {
-        // we need to wait for each block to be done mining when in legacy
-        // mode because things like `mine` and `miner_start` must wait for the
-        // first mine operation to be fully complete.
-        await emitBlockProm;
-      }
+      this.emit("block", finalizedBlockData);
 
       if (onlyOneBlock) {
         this.#currentlyExecutingPrice = 0n;

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

@@ -1,7 +1,7 @@
 import { RPCQUANTITY_GWEI } from "@ganache/utils";
 import assert from "assert";
 import EthereumProvider from "../../../src/provider";
-import getProvider from "../../helpers/getProvider";
+import getProvider, { mnemonic } from "../../helpers/getProvider";
 
 function hex(length: number) {
   return `0x${Buffer.allocUnsafe(length).fill(0).toString("hex")}`;
@@ -13,7 +13,10 @@ describe("api", () => {
     let accounts: string[];
 
     beforeEach(async () => {
-      provider = await getProvider();
+      provider = await getProvider({
+        miner: { instamine: "strict" },
+        wallet: { mnemonic }
+      });
 
       accounts = await provider.send("eth_accounts");
     });

src/chains/ethereum/ethereum/tests/api/eth/getTransactionReceipt.test.ts

@@ -1,4 +1,4 @@
-import getProvider from "../../helpers/getProvider";
+import getProvider, { mnemonic } from "../../helpers/getProvider";
 import assert from "assert";
 import EthereumProvider from "../../../src/provider";
 
@@ -21,7 +21,10 @@ describe("api", () => {
             }
           }
         };
-        provider = await getProvider({ logging: { logger } });
+        provider = await getProvider({
+          logging: { logger },
+          wallet: { mnemonic }
+        });
         [from] = await provider.send("eth_accounts");
       });
 
@@ -61,16 +64,23 @@ describe("api", () => {
         );
       });
 
-      describe("legacy instamine detection and notice", () => {
+      describe("strict instamine detection and notice", () => {
         it("logs a warning if the transaction hasn't been mined yet", async () => {
-          const hash = await provider.send("eth_sendTransaction", [
-            { from, to: from }
-          ]);
+          const strictInstamineProvider = await getProvider({
+            logging: { logger },
+            miner: { instamine: "strict" },
+            wallet: { mnemonic }
+          });
+          const hash = await strictInstamineProvider.send(
+            "eth_sendTransaction",
+            [{ from, to: from }]
+          );
 
           // do not wait for the tx to be mined which will create a warning
-          const result = await provider.send("eth_getTransactionReceipt", [
-            hash
-          ]);
+          const result = await strictInstamineProvider.send(
+            "eth_getTransactionReceipt",
+            [hash]
+          );
 
           assert.strictEqual(result, null);
           assert(
@@ -125,26 +135,26 @@ describe("api", () => {
           );
         });
 
-        it("doesn't log if legacyInstamine is enabled", async () => {
-          const legacyInstamineProvider = await getProvider({
+        it("doesn't log if instamine is set to 'eager' (default)", async () => {
+          const eagerInstamineProvider = await getProvider({
             logging: { logger },
-            miner: { legacyInstamine: true }
+            miner: { instamine: "eager" }
           });
 
-          const [from] = await legacyInstamineProvider.send("eth_accounts");
+          const [from] = await eagerInstamineProvider.send("eth_accounts");
 
-          const hash = await legacyInstamineProvider.send(
+          const hash = await eagerInstamineProvider.send(
             "eth_sendTransaction",
             [{ from, to: from }]
           );
 
-          const result = await legacyInstamineProvider.send(
+          const result = await eagerInstamineProvider.send(
             "eth_getTransactionReceipt",
             [hash]
           );
 
           // the tx is mined before sending the tx hash back to the user
-          // if legacyInstamine is enabled - so they will get a receipt
+          // if instamine is set to 'eager' - so they will get a receipt
           assert(result);
           assert(
             !logger.loggedStuff.includes(

src/chains/ethereum/ethereum/tests/api/eth/legacyInstamining.test.ts → src/chains/ethereum/ethereum/tests/api/eth/greedyInstamining.test.ts

@@ -3,10 +3,10 @@ import assert from "assert";
 
 describe("api", () => {
   describe("eth", () => {
-    describe("legacy", () => {
-      it("when not in legacy mode, does not mine before returning the tx hash", async () => {
+    describe("eager", () => {
+      it("when in strict instamine mode, does not mine before returning the tx hash", async () => {
         const provider = await getProvider({
-          miner: { legacyInstamine: false }
+          miner: { instamine: "strict" }
         });
         const accounts = await provider.send("eth_accounts");
 
@@ -23,9 +23,9 @@ describe("api", () => {
         assert.strictEqual(receipt, null);
       });
 
-      it("when in legacy mode, mines before returns in the tx hash", async () => {
+      it("when in eager instamine mode, mines before returns in the tx hash", async () => {
         const provider = await getProvider({
-          miner: { legacyInstamine: true }
+          miner: { instamine: "eager" }
         });
         const accounts = await provider.send("eth_accounts");
 
@@ -44,7 +44,7 @@ describe("api", () => {
 
       it("handles transaction balance errors, callback style", done => {
         getProvider({
-          miner: { legacyInstamine: true },
+          miner: { instamine: "eager" },
           chain: { vmErrorsOnRPCResponse: true }
         }).then(async provider => {
           const [from, to] = await provider.send("eth_accounts");

src/chains/ethereum/ethereum/tests/api/eth/sendTransaction.test.ts

@@ -51,7 +51,7 @@ describe("api", () => {
         it("returns a VM error when the account has insufficient funds to transfer the value at runtime", async () => {
           const approximateGasCost = 99967968750001;
           const provider = await getProvider({
-            miner: { legacyInstamine: true },
+            miner: { instamine: "eager" },
             chain: { vmErrorsOnRPCResponse: true }
           });
           const accounts = await provider.send("eth_accounts");