Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

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

Merged
merged 6 commits into from
Sep 20, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/chains/ethereum/ethereum/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,9 @@ export default class EthereumApi implements Api {
async evm_mine(options: Ethereum.MineOptions): Promise<"0x0">;
@assertArgLength(0, 1)
async evm_mine(arg?: number | Ethereum.MineOptions): Promise<"0x0"> {
// `MINE_ONLY_ONE_BLOCK` refers to the number of blocks mined per call to `blockchain.mine()`
const MINE_ONLY_ONE_BLOCK = true;

const blockchain = this.#blockchain;
const options = this.#options;
const vmErrorsOnRPCResponse = options.chain.vmErrorsOnRPCResponse;
Expand All @@ -310,19 +313,20 @@ export default class EthereumApi implements Api {
for (let i = 0; i < blocks; i++) {
const { transactions } = await blockchain.mine(
Capacity.FillBlock,
timestamp,
true
MINE_ONLY_ONE_BLOCK,
timestamp
);

if (vmErrorsOnRPCResponse) {
assertExceptionalTransactions(transactions);
}
}
} else {
const timestamp = arg as number | null;
const { transactions } = await blockchain.mine(
Capacity.FillBlock,
arg as number | null,
true
MINE_ONLY_ONE_BLOCK,
timestamp
);
if (vmErrorsOnRPCResponse) {
assertExceptionalTransactions(transactions);
Expand Down
20 changes: 15 additions & 5 deletions src/chains/ethereum/ethereum/src/blockchain.ts
Original file line number Diff line number Diff line change
Expand Up @@ -357,10 +357,10 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {

//#region automatic mining
const nullResolved = Promise.resolve(null);
const mineAll = (maxTransactions: Capacity, onlyOneBlock = false) =>
const mineAll = (maxTransactions: Capacity, onlyOneBlock?: boolean) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.#isPaused()
? nullResolved
: this.mine(maxTransactions, null, onlyOneBlock);
: this.mine(maxTransactions, onlyOneBlock);
if (instamine) {
// insta mining
// whenever the transaction pool is drained mine the txs into blocks
Expand Down Expand Up @@ -592,8 +592,8 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {

mine = async (
maxTransactions: number | Capacity,
timestamp?: number,
onlyOneBlock: boolean = false
onlyOneBlock: boolean = false,
timestamp?: number
) => {
const nextBlock = this.#readyNextBlock(this.blocks.latest, timestamp);

Expand All @@ -608,6 +608,15 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {
onlyOneBlock
);
await this.#blockBeingSavedPromise;

if (
timestamp !== undefined &&
this.#options.miner.timestampIncrement === "clock"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call!

) {
// when miner.timestampIncrement is a number, the previous block timestamp is used as a reference
// for the next block, so this call is not required.
this.setTimeDiff(timestamp * 1000);
}
return {
transactions,
blockNumber: nextBlock.header.number.toArrayLike(Buffer)
Expand Down Expand Up @@ -839,7 +848,8 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {
}

/**
* @param newTime - the number of milliseconds to adjust the time by. Can be negative.
davidmurdoch marked this conversation as resolved.
Show resolved Hide resolved
* Adjusts the internal time adjustment such that the provided time is considered the "current" time.
* @param newTime - the time (in milliseconds) that will be considered the "current" time
* @returns the total time offset *in milliseconds*
*/
public setTimeDiff(newTime: number) {
Expand Down
7 changes: 6 additions & 1 deletion src/chains/ethereum/ethereum/tests/blockchain.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,12 @@ describe("blockchain", async () => {
// assert that second block's timestamp is at least `blockTime` greater
// than the first block's. meaning, these blocks weren't mined one after
// the other
assert(timestamps[1] >= timestamps[0] + blockTime);
assert(
timestamps[1] >= timestamps[0] + blockTime,
`Unexpected timestamp - expected >= ${timestamps[0] + blockTime}, got ${
davidmurdoch marked this conversation as resolved.
Show resolved Hide resolved
timestamps[1]
}`
);
});
});
});
83 changes: 80 additions & 3 deletions src/chains/ethereum/ethereum/tests/provider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,83 @@ describe("provider", () => {
await provider.disconnect();
});

describe("uses timestamp adjustment in subsequent blocks after calling `evm_mine` with a `timestamp` argument", () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const timestampIncrement = 5; // seconds
const timeArgumentSeconds = 100;

async function mineBlocksForTimestamps(
provider: EthereumProvider
): Promise<string[]> {
// mine a block with a specified timestamp
await provider.request({
method: "evm_mine",
params: [timeArgumentSeconds]
});
const specifiedBlock = await provider.request({
method: "eth_getBlockByNumber",
params: ["latest", false]
});
// mine a block without a specified timestamp
await provider.request({
method: "evm_mine",
params: []
});
const unspecifiedBlock = await provider.request({
method: "eth_getBlockByNumber",
params: ["latest", false]
});

return [specifiedBlock.timestamp, unspecifiedBlock.timestamp];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
it("should work with `timestampIncrement` of `clock`", async () => {
const provider = await getProvider({
miner: { timestampIncrement: "clock" }
});

const [specifiedBlock, unspecifiedBlock] =
await mineBlocksForTimestamps(provider);

assert.strictEqual(
specifiedBlock,
`0x${timeArgumentSeconds.toString(16)}`,
"Unexpected timestamp for block mined with specified timestamp"
);

assert(
+unspecifiedBlock >= timeArgumentSeconds &&
+unspecifiedBlock <= timeArgumentSeconds + 1000,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

🤞

`Unexpected timestamp for block mined without specified timestamp - expected a value between ${timeArgumentSeconds} and ${
timeArgumentSeconds + 1000
}, got ${+unspecifiedBlock}`
);

await provider.disconnect();
});

it("should work with a numeric `timestampIncrement`", async () => {
const provider = await getProvider({
miner: { timestampIncrement }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

const [specifiedBlock, unspecifiedBlock] =
await mineBlocksForTimestamps(provider);

assert.strictEqual(
specifiedBlock,
`0x${timeArgumentSeconds.toString(16)}`,
"Unexpected timestamp for block mined with specified timestamp"
);

assert.strictEqual(
unspecifiedBlock,
`0x${(timeArgumentSeconds + timestampIncrement).toString(16)}`,
"Unexpected timestamp for block mined without specified timestamp"
);

await provider.disconnect();
});
});

it("applies the adjustment only once when `timestampIncrement` is used", async () => {
const time = new Date("2019-01-01T00:00:00.000Z");
const timestampIncrement = 5; // seconds
Expand Down Expand Up @@ -185,9 +262,9 @@ describe("provider", () => {
"unexpected timestamp for the second block mined"
);
await mineAndAssertTimestamp(
startTimeSeconds + fastForwardSeconds + timestampIncrement * 3
),
"unexpected timestamp for the third block mined";
startTimeSeconds + fastForwardSeconds + timestampIncrement * 3,
"unexpected timestamp for the third block mined"
);
});

it("uses the `timestampIncrement` for the first block when forking", async () => {
Expand Down