fix: save evm_mine blocks before returning

[MicaiahReid]

Though it was rare, sometimes calling evm_mine would return before the mined block was actually saved. If you polled for the block that Ganache had claimed to mine directly after calling evm_mine, that block could not yet exist.

This change ensures that the block is saved before emitting the "newHeads" subscription message and before returning during an evm_mine. Fixes #3060.

Potential Side Effect

This change does have one potential side effect, though we really doubt anyone should be impacted, and we consider it a bug fix. Sometimes when a block is mined, Ganache waits until the next iteration of the event loop (using setImmediate) to emit the mined block. This is to ensure that the user has time to listen for the new block's "message" before Ganache emits it.

Previously, Ganache would delay emitting this event if --miner.instamine="eager" AND --miner.blockTime=0. These settings mean that transactions are saved before returning the transaction (as apposed to returning a hash as the transaction enters the transaction pool in "strict" mode), and the miner immediately starts mining transactions as they enter the transaction pool (as opposed to mining a block every blockTime seconds).*

Now, Ganache delays emitting this event when --miner.instamine="eager" regardless of how blockTime is set. The instamine mode affects when the transaction is returned to the user, so it should be the driving factor behind delaying the event. If in --miner.blockTime=0 mode (the default) you somehow relied on the event being emitted at a very specific event tick, this may cause issues for you. For most of us mere mortal developers, this won't make a difference.

*Note: We'll have a blog post or discussion providing a deeper dive into all of our mining modes soon that should help further clarify --miner.instamine, --miner.blockTime, and --chain.vmErrorsOnRPCResponse.

[davidmurdoch]

Thoughts on adding tests for this new timing behavior?

[MicaiahReid]

Thoughts on adding tests for this new timing behavior?

I mean our block is null; didn't correctly mine ${numberOfBlocksToMine} blocks test kind of tests this, right? 😄

Yeah, I'll get some tests added.

[MicaiahReid]

@davidmurdoch I've added a test!

I'm instantiating a provider where I specify the db using the {database: { db: ... }} flag, and I'm passing in memdown. I'm using patch-package to update memdown's _batch function to delay 20ms before saving. This _batch function is used by our blockchain to save blocks to the db.

I've created a branch that has these tests without the actual race condition fixes as a proof that the tests do what I say they should, and that my changes actually fix the bug.