fix: evm_setTime doesn't work correctly with miner.timestampIncrement

[jeffsmale90]

When using miner.timestampIncrement, calling evm_setTime can result in unexpected values, and potentially with an error being thrown.

This is because the timeAdjustment is applied to the previous blocktime, so this offset is applied for every block that is mined.

With the following example, the initial block is mined correctly, as is the first block after setting the time, but subsequent blocks will have a descending block time, until the value becomes negative.

const p = await getProvider({
  miner: {timestampIncrement: 1},
  chain: {time : 10000}
});

const mineAndShowBlock = async () => {
  await p.send("evm_mine");
  const {number, timestamp} = await p.send("eth_getBlockByNumber", ["latest"]);

  console.log({number, timestamp: Quantity.toNumber(timestamp)});  
}
await mineAndShowBlock();

await p.send("evm_setTime", [5000]);

while(true) {
  await mineAndShowBlock();
}

{ number: '0x1', timestamp: 11 }
{ number: '0x2', timestamp: 6 }
{ number: '0x3', timestamp: 1 }

Error: Cannot wrap a negative value as a json-rpc type

Fixes: #3330

[jeffsmale90]

I have decided to close #3265 in favour of this approach. I still think there's an opportunity to improve the implementation, and to add validation, but I will open that in a separate PR.

[MicaiahReid]

I wonder if this would make more sense to do in the #adjustedTime function, right after we use it.
From:

#adjustedTime = (precedingTimestamp: Quantity) => {
  const timeAdjustment = this.#timeAdjustment;
  const timestampIncrement = this.#options.miner.timestampIncrement;
  if (timestampIncrement === "clock") {
    return Math.floor((Date.now() + timeAdjustment) / 1000);
  } else {
    return (
      precedingTimestamp.toNumber() +
      Math.floor(timeAdjustment / 1000) +
      timestampIncrement.toNumber()
    );
  }
};

to:

#adjustedTime = (precedingTimestamp: Quantity) => {
  let timeAdjustment = this.#timeAdjustment;
  const timestampIncrement = this.#options.miner.timestampIncrement;
  if (timestampIncrement === "clock") {
    return Math.floor((Date.now() + timeAdjustment) / 1000);
  } else {
    const time =
      precedingTimestamp.toNumber() +
      Math.floor(timeAdjustment / 1000) +
      timestampIncrement.toNumber();
    timeAdjustment = 0;
    return time;
  }
};

Just so we're keeping all of this time logic in one area rather than randomly having it in this mine block.

[jeffsmale90]

I didn't put it in there, because sometimes we call #adjustedTime, but don't persist the block - for instance the pending block changes that you've got in PR.

I agree that it's not super nice to have it thrown in the middle of here, but it needs to only happen if the block's being persisted for realsies.

I would like to tidy this up in a rewrite of the block-time stuff - those changes have been stressing me out, so I've put it on the backburner :P

[MicaiahReid]

That makes sense! I'm very much looking forward to your time management refactor - there seems to be a lot of tech-debt wrapped up in how we manage time. But let's get these bugs fixed now if we can!

[davidmurdoch]

Is there a specific issue for what this PR fixes? If so, can you update the PR description to indicate that this PR fixes it before you squash and merge? If not, squash and merge away!

I think this is the issue #3330.

Also, don't forget to update the PR title!