fix: calling evm_mine with a timestamp argument should reflect the change of time in subsequent blocks

[jeffsmale90]

Previously, calling evm_mine with a timestamp argument would result in a block with the specified timestamp, but subsequent blocks would have a timestamp that didn't reflect this change in time. This only occurred when miner.timestampIncrement is unspecified or clock.

const provider = ganache.provider();

await provider.request({ method: "evm_mine", params: ["0x1"] });
const { timestamp: firstTimestamp } = await provider.request({
  method: "eth_getBlockByNumber",
  params: ["latest", false],
});

// wait 1 second before mining the second block
await new Promise((resolve, reject) => setTimeout(resolve, 1000));

await provider.request({ method: "evm_mine", params: [] });
const { timestamp: secondTimestamp } = await provider.request({
  method: "eth_getBlockByNumber",
  params: ["latest", false],
});

console.log({
  firstTimestamp,
  secondTimestamp,
});

Will output something like:

{ firstTimestamp: '0x1', secondTimestamp: '0x5e0be0ff' }

Where secondTimestamp is the current time in seconds, but should be 0x2.

With this change, blocks mined after providing a timestamp parameter to evm_mine, will have timestamps that reflect the change in time.

Fixes: #3265

[MicaiahReid]

If CI is running slowly, could this break? I don't know what the solution is, and I don't feel it's worth holding up this PR for, but just wanted to throw it out there in case you have a solution on-hand.

[jeffsmale90]

It could do. Because we are specifying a timestamp in seconds, it would need to take an entire second to mine and fetch 2 blocks.

I feel like I'm building a lot of pressure on the time refactor - but it'll let us inject a now provider, which allow us to be much more resilient to slow build agents.

🤞

[davidmurdoch]

There are two time-related blocks of code in this function that both check this.#options.miner.timestampIncrement === "clock". Can these two blocks be joined together? If not, can you cache this.#options.miner.timestampIncrement === "clock" in a variable so we don't do it twice?

[jeffsmale90]

Good call!

[davidmurdoch]

Our describes are usually simple words or phrases like "api", "eth_chainId", "server". This is worded more like an it.

Can you rephrase the describe and its here to be more inline with our other tests?

We use describe to define structure to our tests, so maybe a bunch of these tests in this file should already be under describe("timestampIncrement", ...) (don't need to make a change like this in this same PR, of course), and then you could have another describe under that one like describe("evm_minewithtimestamp", ...) (just an example -- I didn't think it through very much).

[jeffsmale90]

I just flattened that describe down, and was more explicit in the test names. lmk what you think.

[davidmurdoch]

If you coerce these to numbers here it'd make the tests slightly simpler.

[jeffsmale90]

I intentionally didn't do this, thinking that we should be asserting the exact returned value. Otherwise, there's a potential issue that could slip through where the return type isn't what's expected, but coerces to what's expected.

Thinking about it in light of your comment, I think that we should be ensuring the above, but maybe in a specific test for that.

I've updated my tests to what you've suggested, keen to hear your thoughts on that.

[davidmurdoch]

Since this timestampIncrement is only used in this function is there a reason to define it in the describe?

[davidmurdoch]

Approved! (with one half-hearted suggestion/thought)

[davidmurdoch]

A decent alternative would be to change mineAll to (maxTransactions: Capacity, onlyOneBlock: boolean) =>

and then all use of mineAll would need to pass the values it needs, basically just:

txPool.on("drain", () => mineAll(Capacity.Single, false));

This might make this code a bit clearer. Or not. I don't think I care either way.

[jeffsmale90]

I left it as is because we set a default on the mine function, and don't feel that requiring it for mineAll, but not for mine feels nice - we might as well leverage the default that we have specified.