-
Notifications
You must be signed in to change notification settings - Fork 678
fix: set difficulty default to 1 and add totalDifficulty
#771
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really Awesome work. I committed a change, if you wouldn't mind reviewing it.
The gist of the change I made was because:
totalDifficultyshouldn't be included in block size calculationstotalDifficultyshouldn't be included in the block hash (which it previously was, because it was included in therawarray)
It seems to work now, and as a bonus, I figured out why our block size calculations were always off a little (PR for that coming soon!)
I also requested some more tests :-D
Thanks
| Quantity.from(timestamp) | ||
| Quantity.from(timestamp), | ||
| this.#options.miner.difficulty, | ||
| Quantity.from(0) // we start the totalDifficulty at 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
| Quantity.from(0) // we start the totalDifficulty at 0 | |
| RPCQUANTITY_ZERO // we start the totalDifficulty at 0 |
| await provider.send("eth_subscribe", ["newHeads"]); | ||
|
|
||
| await provider.send("eth_sendTransaction", [{ from, to: from }]); | ||
|
|
||
| await provider.once("message"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't need the transaction itself, you can do await provider.send("evm_mine") instead.
Oh, actually... now that #793 is merged (as of yesterday), you can do await provider.send("evm_mine", [{blocks: number}]), no (backwards 😄) loop needed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh cool! I'll merge in develop and update this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like deleting code.
| const block = await provider.send("eth_getBlockByNumber", ["0x0"]); | ||
| assert.strictEqual( | ||
| block.totalDifficulty, | ||
| `0x${DEFAULT_DIFFICULTY}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If DEFAULT_DIFFICULTY is ever greater than 9 in the future this test will fail.
Other ways of solving this:
assert.strictEqual(
block.totalDifficulty,
Quantity.from(DEFAULT_DIFFICULTY).toString()
);
or
assert.strictEqual(
parseInt(block.totalDifficulty, 16),
DEFAULT_DIFFICULTY
);
or
assert.strictEqual(
BigInt(block.totalDifficulty),
DEFAULT_DIFFICULTY // change DEFAULT_DIFFICULTY to a BigInt
);
and lots of others, I'm sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also update other places that use the 0x{number} pattern in these tests? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant to update these but must have overlooked them. Updated!
f27b613 to
609c25c
Compare
… current block's difficulty
I had to store totalDifficulty in a different way so it wasn't included in the `raw` fields (which messes with the hash).
Co-authored-by: David Murdoch <[email protected]>
Co-authored-by: David Murdoch <[email protected]>
609c25c to
1c90485
Compare
davidmurdoch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more little "suggestions" and it's good to go!
src/chains/ethereum/ethereum/tests/api/eth/getBlockByNumber.test.ts
Outdated
Show resolved
Hide resolved
src/chains/ethereum/ethereum/tests/api/eth/getBlockByNumber.test.ts
Outdated
Show resolved
Hide resolved
difficulty and totalDifficultydifficulty default to 1 and add totalDifficulty
…ys-archive#771) Co-authored-by: David Murdoch <[email protected]>
Currently Ganache sets all block headers to have
difficultyandtotalDifficultyhard-coded to 0. This PR causes Ganache to set the difficulty of each block (by default) to 1 (the user can specify their own block difficulty on the command line). ThetotalDifficultyis also now calculated based on the previous block'stotalDifficultyand the block difficulty.