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 all 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
28 changes: 18 additions & 10 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,22 +592,29 @@ 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);

// if block time is incremental, adjustments should only apply once,
// otherwise they accumulate with each block.
if (this.#options.miner.timestampIncrement !== "clock") {
this.#timeAdjustment = 0;
}
const transactions = await this.#miner.mine(
nextBlock,
maxTransactions,
onlyOneBlock
);
await this.#blockBeingSavedPromise;

if (this.#options.miner.timestampIncrement !== "clock") {
// if block time is incremental, adjustments should only apply once,
// otherwise they accumulate with each block.
this.#timeAdjustment = 0;
} else if (timestamp !== undefined) {
// 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 +846,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]
}`
);
});
});
});
87 changes: 83 additions & 4 deletions src/chains/ethereum/ethereum/tests/provider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,86 @@ describe("provider", () => {
await provider.disconnect();
});

it("applies the adjustment only once when `timestampIncrement` is used", async () => {
async function mineBlocksForTimestamps(
provider: EthereumProvider
): Promise<number[]> {
// 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];
}

const timeArgumentSeconds = 100;

it("uses timestamp adjustment in subsequent blocks with `timestampIncrement` of `clock`", async () => {
const provider = await getProvider({
miner: { timestampIncrement: "clock" }
});

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

assert.strictEqual(
specifiedBlock,
timeArgumentSeconds,
"Unexpected timestamp for block mined with specified timestamp"
);

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

await provider.disconnect();
});

it("uses timestamp adjustment in subsequent blocks with numeric `timestampIncrement`", async () => {
const timestampIncrement = 5; // seconds

const provider = await getProvider({
miner: { timestampIncrement }
});

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

assert.strictEqual(
specifiedBlock,
timeArgumentSeconds,
"Unexpected timestamp for block mined with specified timestamp"
);

assert.strictEqual(
unspecifiedBlock,
timeArgumentSeconds + timestampIncrement,
"Unexpected timestamp for block mined without specified timestamp"
);

await provider.disconnect();
});

it("applies timestamp adjustment only once when `timestampIncrement` is used", async () => {
const time = new Date("2019-01-01T00:00:00.000Z");
const timestampIncrement = 5; // seconds
const fastForwardSeconds = 100;
Expand Down Expand Up @@ -185,9 +264,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