fix: if a timestamp is passed to `evm_mine`, subsequent blocks reflect the change in time #3317

jeffsmale90 · 2022-06-30T02:13:39Z

This fixes a regression introduced in Ganche v7, where providing a timestamp in a call to evm_mine would result in a block with the correct timestamp, but the internal time-offset would not be updated, so subsequent blocks would not reflect the change in time.

Fixes: #3265 #3271

MicaiahReid

Just two small changes!

BTW, I've installed this version and tested against the repro I had set up from #3265, and it fixes the issue as expected!

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

MicaiahReid · 2022-07-01T12:58:16Z

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

+ if (numBlocks == null) {
+ numBlocks = 1;
+ }
+ if (timestamp) {


Suggested change

if (timestamp) {

if (timestamp !== null) {

I know this is a silly edge case, but what if they set the timestamp to 0? This would get skipped.

I fixed it :)

And added an explicit test case

…equent blocks reflect the change in time

jeffsmale90 · 2022-07-13T23:15:33Z

src/chains/ethereum/ethereum/src/block-time.ts

@@ -0,0 +1,97 @@
+export interface BlockTime {


I've extracted the logic for block timestamp into BlockTime, which allows us to more clearly describe the behaviour, and test more thoroughly.

By settings this as a dependency, it will allow us to decouple our tests from the system clock (without deferring to timestampIncrement), for the likes of #3271 (which I'll pick up after this).

I opened PR #3389 to address #3271 but have also fixed the issue in this PR, and was able to test the issue in this PR (at least on the underlying provider, not so easy at the higher level integration test).

I figured I would leave both PRs open, so that we can get a quick fix in with #3389, but am happy to close that PR if this one goes through quickly.

jeffsmale90 · 2022-07-14T02:49:30Z

I've standardised all timestamp units to milliseconds, with the exceptions:

A block timestamp is obviously seconds.
Some API parameters are in seconds.

I ensure that all interactions below api.ts (with the exception of a block's timestamp) are ms.

…and IncrementBasedBlockTime.

…epency between start time and the value returned from reference clock causing non-zero offset. fix: #3271

jeffsmale90 · 2022-07-20T07:58:13Z

src/chains/ethereum/ethereum/src/block-time.ts

+ }
+ }
+
+ constructor(getReferenceClockTime: () => number | Date, startTime: number | Date | undefined) {


I made the constructor explicitly accept a startTime of value undefined, and switched the argument order to make it more intuitive. I didn't make the argument optional, because the only time that we don't pass it (except for providing a variable of value undefined) is in tests.

This allows us to instantiate a ClockBasedBlockTime with no offset, without jumping through hoops.

jeffsmale90 · 2022-07-20T10:41:57Z

src/chains/ethereum/ethereum/src/block-time.ts

+}
+
+export class ClockBasedBlockTime implements BlockTime {
+ private _getReferenceClockTime: () => number | Date;


I changed all of the # prefixed private class fields to _ prefixed typescript privates. The former have performance implications, and I don't see the benefit until we can resolve this.

#3240 addresses this more generally.

davidmurdoch

quick partial review

davidmurdoch · 2022-07-19T15:30:54Z

src/chains/ethereum/ethereum/src/block-time.ts

+ }
+ }
+
+ constructor(startTime: number | Date, getReferenceClockTime: () => number | Date) {


Use number here (and elsewhere) instead of number | Date. The Date object isn't used internally and we are casting everywhere we used the referenced clock time. Date.now() is about a bajillion or so times faster than new Date()

davidmurdoch · 2022-07-19T15:37:16Z

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

+ const timestampMilliseconds =
+ timestamp == undefined ? undefined : timestamp * 1000;


We shouldn't compute this in the loop; add this logic to the timestamp != undefined condition above.

Yeah definitely!

davidmurdoch · 2022-07-19T15:39:39Z

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

@@ -297,33 +297,36 @@ export default class EthereumApi implements Api {
 const blockchain = this.#blockchain;
 const options = this.#options;
 const vmErrorsOnRPCResponse = options.chain.vmErrorsOnRPCResponse;
+
+ let numBlocks, timestamp;


These need types.

davidmurdoch · 2022-07-19T15:40:40Z

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

+ if (numBlocks == undefined) {
+ numBlocks = 1;
+ }


Any objection to checking this condition in the if above?

numBlocks = arg.blocks == null ? 1 : arg.blocks;

and then in the else assign numBlocks to 1.

Not at all - I think that makes sense. It splits out the two use cases (args vs timestamp) nicely.

davidmurdoch · 2022-07-19T15:46:10Z

src/chains/ethereum/ethereum/src/block-time.ts

+
+ private static validateTimestamp(timestamp: number | Date) {
+ if (timestamp < 0) {
+ throw new Error(`Invalid timestamp: ${timestamp}. Value must be positive.`);


Why must the timestamp be positive? This breaks ganache --time, and evm_setTime.

As discussed separately, although a negative timestamp is valid, the block timestamp property must be a positive integer value (it is of type Quantity). Presently Ganache will accept a negative value (programatically, but not via cli), but will result in undesired block time.

I've added a comment explaining why we are validating. I don't think this should be considered a breaking change, as it's aligning Ganache with the expected behaviour, but it could cause problems for our users, so I'm happy to push that change out to milestone 8 if desired.

davidmurdoch · 2022-07-19T16:27:31Z

src/chains/ethereum/ethereum/src/provider.ts

+
+ const startTime = this.#options.chain.time || new Date();
+ const blockTime =
+ this.#options.miner.timestampIncrement === "clock"


Suggested change

this.#options.miner.timestampIncrement === "clock"

providerOptions.miner.timestampIncrement === "clock"

davidmurdoch · 2022-07-19T16:27:44Z

src/chains/ethereum/ethereum/src/provider.ts

+ const startTime = this.#options.chain.time || new Date();
+ const blockTime =
+ this.#options.miner.timestampIncrement === "clock"
+ ? new ClockBasedBlockTime(startTime, () => new Date())


Suggested change

? new ClockBasedBlockTime(startTime, () => new Date())

? new ClockBasedBlockTime(startTime, () => Date.now())

davidmurdoch · 2022-07-19T16:30:27Z

src/chains/ethereum/ethereum/src/provider.ts

+ ? new ClockBasedBlockTime(startTime, () => new Date())
+ : new IncrementBasedBlockTime(
+ startTime,
+ this.#options.miner.timestampIncrement.toNumber() * 1000


Suggested change

this.#options.miner.timestampIncrement.toNumber() * 1000

providerOptions.miner.timestampIncrement.toNumber() * 1000

davidmurdoch · 2022-07-19T16:44:13Z

src/chains/ethereum/ethereum/src/forking/fork.ts

+ // whereas in incrementBasedBlockTime we set the time of the _next_ block
+ // maybe when we call setTime, we increment the value passed in.
+ const blockTimestampMs = this.block.header.timestamp.toNumber() * 1000;
+ this.block.header.timestamp = Quantity.from(


chainOptions.time is no longer set and should be. The CLI startup output uses this updated time value when it outputs its startup details.

You can check this by running ganache --fork mainnet --fork.blockNumber 15073145 --miner.timestampIncrement 1

ganache 7.3.2:

Forked Chain ================== Location: Ethereum Mainnet, via 丕Infura Block: 15073145 Network ID: 1 Time: Sun Jul 03 2022 21:41:07 GMT-0400 (Eastern Daylight Time)

this PR:

Forked Chain ================== Location: Ethereum Mainnet, via 丕Infura Block: 15073145 Network ID: 1 Time: Tue Jul 19 2022 12:41:59 GMT-0400 (Eastern Daylight Time)

We really should add tests for the CLI output one day 😅

Ah. Good to know.

I've set that value, and validated (note the timezone difference!):

Forked Chain ================== Location: Ethereum Mainnet, via 丕Infura Block: 15073145 Network ID: 1 Time: Mon Jul 04 2022 13:41:06 GMT+1200 (New Zealand Standard Time) Chain Id ================== 1337

Except the timestamp is now incorrect if ganache is using "clock" mode (since it doesn't use the clock for the first block after the fork block).

Note: I don't particularly like that this is ganache's behavior in "clock" mode.

I've fixed and added some explicit tests around this behaviour to forking.test.ts

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

…w Date()). Set chainOptions.time in case where time comes from forked chain.

davidmurdoch · 2022-07-26T14:34:11Z

src/chains/ethereum/block/tests/index.test.ts

+ blockchain = new Blockchain(
+ options,
+ fromAddress,
+ new ClockBasedBlockTime(Date.now, undefined)


No need to pass undefined here.

That's intentional - #3317 (comment)

I wanted to make it explicit. In real usage in provider.ts we explicitly pass a variable with value undefined. I wanted the tests to explicitly match this, so I didn't make the argument optional.

I'm open to changing that if you feel strongly about it.

As per #3317

I added fromSystemClock() static constructor method which means the only time we call this constructor directly is in tests, so I've done as you suggested.

davidmurdoch · 2022-07-26T15:29:55Z

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

+
+ let numBlocks: number, timestamp: number;
 // 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;
- }
- // 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
- // at a time and doing this would make it way faster
- for (let i = 0; i < blocks; i++) {
- const { transactions } = await blockchain.mine(
- Capacity.FillBlock,
- timestamp,
- true
- );
-
- if (vmErrorsOnRPCResponse) {
- assertExceptionalTransactions(transactions);
- }
- }
+ numBlocks = arg.blocks == null ? 1 : arg.blocks;
+ timestamp = arg.timestamp;
 } else {
+ numBlocks = 1;
+ timestamp = arg as number | null;
+ }
+
+ let timestampMilliseconds: number;
+ if (timestamp != undefined) {
+ timestampMilliseconds = timestamp * 1000;
+ this.#blockchain.setTime(timestampMilliseconds);
+ }


I don't think we need to setTime here because blockchain.mine already does this if a timestamp is passed in. I think this can be simplified further into something like:

let numBlocks: number, timestampMilliseconds: number; if (arg == null) { numBlocks = 1; } else if (typeof arg === "object") { numBlocks = typeof arg.blocks === "number" ? arg.blocks : 1; if (typeof arg.timestamp === "number") { timestampMilliseconds = arg.timestamp * 1000; } } else { numBlocks = 1; timestampMilliseconds = arg * 1000; }

That said, we are converting our time from seconds to milliseconds here, just to convert it back to seconds in readyNextBlock. Maybe this is fine and is the best we can do because the precision of our options in various places is all over the place (some in seconds, some milliseconds).

Yeah, you are correct. I've simplified as you suggested.

davidmurdoch · 2022-07-26T15:49:40Z

src/chains/ethereum/ethereum/tests/provider.test.ts

@@ -107,8 +108,7 @@ describe("provider", () => {
 method: "eth_getBlockByNumber",
 params: ["latest", false]
 });
- const expectedTime =
- Math.floor((fastForward + +time) / 1000) + timestampIncrement;
+ const expectedTime = Math.floor((fastForward + +time) / 1000) + timestampIncrement;


Looks like this may need to be prettieried.

davidmurdoch · 2022-07-26T15:53:23Z

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

@@ -214,6 +216,7 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {
 constructor(
 options: EthereumInternalOptions,
 coinbase: Address,
+ blockTime: BlockTime,


Should we give this a default? Something like blockTime: BlockTime = new ClockBasedBlockTime(Date.now),. I think it could simplify a bunch of tests.

I prefer to not specify a default in cases such as this, as it encourages more implicit code which is prone to miss-understandings. I also feel that the implementation should be optimised for the "real" use case (while obviously being testable).

I've added a fromSystemClock() static constructor. I intentionally didn't just give getReferenceClockTime argument a default, because I wanted to make it explicit that it's using the system clock.

I think this gives a nice balance, and also tidies up the real world consumption - where we can just call fromSystemClock(startTime) and have the Date.now part abstracted away.

Hm, there is a default, you just put it in a different place. :-)

I'm fine with it... but I can foresee someone creating a test helper that applies actual defaults in the future (like the getProvider() helper does already).

src/chains/ethereum/ethereum/src/block-time.ts

davidmurdoch · 2022-07-26T15:57:37Z

src/chains/ethereum/ethereum/src/block-time.ts

+/*
+ A BlockTime implementation that increments it's reference time by the duration specified by incrementMilliseconds,
+ every time createBlockTimestampInSeconds() is called. The timestamp returned will be impacted by the timestamp offset,
+ so can be moved forward and backwards by calling .setOffset() independent of the start time, and will increment when


Can you check this comment for accuracy/consistency? I'm having a hard time following it.

Yeah, it's a bit mucky, I've made it more concise and easier to follow. 🤞

davidmurdoch · 2022-07-26T16:03:42Z

src/chains/ethereum/ethereum/src/block-time.ts

+ private readonly _tickReferenceClock: () => void;
+
+ constructor(startTime: number, incrementMilliseconds: number) {
+ let referenceTime = startTime;


I don't think this referenceTime is reset on evm_revert, but I think it should be.

That's a really good point. I rethought the approach and went back and forward a couple of times. I've settled on a slight modification, which I think makes sense. The IncrementBasedBlockTime now holds the reference clock at startTime and increments it's offset rather than reference clock.

Offset wasn't working correctly with increment based clocks prior to this change, (as blocktime = previousBlocktime + increment + offset will result in compounding the offset). This change fixes that.

It does mean that the meaning of "offset" wrt increment based clocks is now strictly the elapsed duration (in block time) since the "start time" (plus / minus any modifications made by calls to evm_setTime / evm_increaseTime).

Happy to hear opinions about the above, or we can set up a call to beat out the details.

dBlockTime

…e sub and supers of each other. Modify IncrementBasedBlockTime to increment the offset, so that checkpointing and reverting work. Improve comments, and tests.

davidmurdoch

since you told me to hold off on the rest of this review I thought I'd just send off the two comments I have so far

davidmurdoch · 2022-08-04T18:55:51Z

src/chains/ethereum/ethereum/src/provider.ts

+
+ const startTime =
+ providerOptions.chain.time === undefined
+ ? Date.now()
+ : +providerOptions.chain.time;
+ // if startTime is undefined, BlockTime will determine 0 offset from the reference time
+ const blockTime =
+ providerOptions.miner.timestampIncrement === "clock"
+ ? BlockTime.fromSystemClock(startTime)
+ : new IncrementBasedBlockTime(
+ startTime,
+ providerOptions.miner.timestampIncrement.toNumber() * 1000
+ );


A BlockTime static that abstracts this logic away would really help make the BlockTime logic more cohesive. I'm not a fan of adding all this additional time logic to the provider constructor.

davidmurdoch · 2022-08-04T19:00:12Z

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

@@ -214,6 +216,7 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {
 constructor(
 options: EthereumInternalOptions,
 coinbase: Address,
+ blockTime: BlockTime,


Hm, there is a default, you just put it in a different place. :-)

I'm fine with it... but I can foresee someone creating a test helper that applies actual defaults in the future (like the getProvider() helper does already).

jeffsmale90 · 2022-08-08T02:27:56Z

This PR is now redundant

jeffsmale90 changed the title ~~fix: if a timestamp is passed to evm_mine, subsequent blocks reflect the change in time. fix:#3265~~ fix: if a timestamp is passed to evm_mine, subsequent blocks reflect the change in time. fix: #3265 Jun 30, 2022

jeffsmale90 force-pushed the fix/evm-mine-set-time branch from 8542ba7 to 3f51c9f Compare June 30, 2022 04:04

jeffsmale90 requested review from MicaiahReid, davidmurdoch and tenthirtyone June 30, 2022 04:05

jeffsmale90 marked this pull request as ready for review June 30, 2022 04:05

davidmurdoch changed the title ~~fix: if a timestamp is passed to evm_mine, subsequent blocks reflect the change in time. fix: #3265~~ fix: if a timestamp is passed to evm_mine, subsequent blocks reflect the change in time Jun 30, 2022

MicaiahReid suggested changes Jul 1, 2022

View reviewed changes

jeffsmale90 linked an issue Jul 5, 2022 that may be closed by this pull request

evm_setTime doesn't work correctly with miner.timestampIncrement #3330

Closed

jeffsmale90 mentioned this pull request Jul 11, 2022

Reconsider evm_setTime behavior wrt timestampIncrement #3346

Open

jeffsmale90 force-pushed the fix/evm-mine-set-time branch from 221b482 to 8e0f535 Compare July 13, 2022 23:06

jeffsmale90 added 3 commits July 14, 2022 11:08

if a timestamp is passed to evm_mine, set the timeoffset so that subs…

88ab6cb

…equent blocks reflect the change in time

integration test to cover the fix

0d126b0

Prettier changes

57abff6

jeffsmale90 force-pushed the fix/evm-mine-set-time branch 3 times, most recently from 13943c9 to 3ba0cfc Compare July 13, 2022 23:12

jeffsmale90 commented Jul 13, 2022

View reviewed changes

jeffsmale90 force-pushed the fix/evm-mine-set-time branch from 3ba0cfc to e7d4124 Compare July 13, 2022 23:16

jeffsmale90 requested a review from MicaiahReid July 13, 2022 23:20

jeffsmale90 force-pushed the fix/evm-mine-set-time branch from e7d4124 to 19f3f25 Compare July 13, 2022 23:23

jeffsmale90 marked this pull request as draft July 13, 2022 23:55

jeffsmale90 force-pushed the fix/evm-mine-set-time branch from 19f3f25 to 8d7d5bd Compare July 14, 2022 02:40

jeffsmale90 force-pushed the fix/evm-mine-set-time branch from 8d7d5bd to 194571b Compare July 14, 2022 02:54

Implement decoupled BlockTime, covered tests for ClockBasedBlockTime …

58e3a6e

…and IncrementBasedBlockTime.

jeffsmale90 force-pushed the fix/evm-mine-set-time branch from 194571b to 58e3a6e Compare July 14, 2022 03:58

Add test case for setting time to zero with

9df8402

jeffsmale90 marked this pull request as ready for review July 14, 2022 08:08

jeffsmale90 mentioned this pull request Jul 20, 2022

fix: use a consistent "now" time when setting up blockchain #3389

Merged

jeffsmale90 linked an issue Jul 20, 2022 that may be closed by this pull request

"should return the timeAdjustment value via evm_increaseTime when provided as a number" test is flaky #3271

Closed

set internal offset to 0 if no start time is provided. resolves discr…

8d56eae

…epency between start time and the value returned from reference clock causing non-zero offset. fix: #3271

jeffsmale90 force-pushed the fix/evm-mine-set-time branch from 92048a5 to 8d56eae Compare July 20, 2022 03:24

jeffsmale90 commented Jul 20, 2022

View reviewed changes

Convert private class fields to typescript privates

b26acdd

jeffsmale90 commented Jul 20, 2022

View reviewed changes

davidmurdoch suggested changes Jul 22, 2022

View reviewed changes

Only accept number values for timestamps. use Date.now (instead of ne…

28232f9

…w Date()). Set chainOptions.time in case where time comes from forked chain.

davidmurdoch suggested changes Jul 26, 2022

View reviewed changes

jeffsmale90 added 6 commits July 28, 2022 15:18

Fix forking timestamp behaviour, tidy up BlockTime

95bc12c

Use offset to represent incremental time for IncrementBase

050e19e

dBlockTime

Remove unnecessary BlockTime interface (as the two implementations ar…

1491988

…e sub and supers of each other. Modify IncrementBasedBlockTime to increment the offset, so that checkpointing and reverting work. Improve comments, and tests.

Missed reference to ClockBasedBlockTime

bca82f2

Tidy up api level evm_mine implementation

7b29ea0

Add BlockTime.fromSystemClock() helper method.

b8a2e85

jeffsmale90 force-pushed the fix/evm-mine-set-time branch from c348ba6 to b8a2e85 Compare July 31, 2022 21:48

Fix TSDoc comment

a535819

jeffsmale90 requested a review from davidmurdoch July 31, 2022 21:52

Merge branch 'develop' into fix/evm-mine-set-time

895ca65

jeffsmale90 mentioned this pull request Aug 2, 2022

feat: build a real pending block #3232

Open

davidmurdoch reviewed Aug 4, 2022

View reviewed changes

experiments with PreviousBlockBasedBlockTime

5c47443

jeffsmale90 closed this Aug 8, 2022

		const timestampMilliseconds =
		timestamp == undefined ? undefined : timestamp * 1000;

	this.#options.miner.timestampIncrement === "clock"
	providerOptions.miner.timestampIncrement === "clock"

	? new ClockBasedBlockTime(startTime, () => new Date())
	? new ClockBasedBlockTime(startTime, () => Date.now())

	this.#options.miner.timestampIncrement.toNumber() * 1000
	providerOptions.miner.timestampIncrement.toNumber() * 1000

fix: if a timestamp is passed to evm_mine, subsequent blocks reflect the change in time #3317

fix: if a timestamp is passed to evm_mine, subsequent blocks reflect the change in time #3317

Conversation

jeffsmale90 commented Jun 30, 2022 • edited Loading

MicaiahReid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffsmale90 Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffsmale90 commented Jul 14, 2022

jeffsmale90 Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmurdoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffsmale90 Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmurdoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffsmale90 commented Aug 8, 2022

fix: if a timestamp is passed to `evm_mine`, subsequent blocks reflect the change in time #3317

fix: if a timestamp is passed to `evm_mine`, subsequent blocks reflect the change in time #3317

jeffsmale90 commented Jun 30, 2022 •

edited

Loading

jeffsmale90 Jul 13, 2022 •

edited

Loading

jeffsmale90 Jul 20, 2022 •

edited

Loading

jeffsmale90 Jul 28, 2022 •

edited

Loading