-
Notifications
You must be signed in to change notification settings - Fork 20.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
consensus/ethash: implement faster difficulty calculators #21976
Conversation
Tested this on bigendian too
Works fine |
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.
LGTM (I mainly looked at the fuzzers though)
x = x - 1 | ||
} | ||
z := new(uint256.Int).SetUint64(x) | ||
adjust.Mul(adjust, z) // adjust: (pdiff / 2048) * max((time - ptime) / 10 - 1, 99) |
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.
Just thinking out loud, if we moved the formula from max(1 - (block_timestamp - parent_timestamp) // 10, -99)
to max((time - ptime) / 10 - 1, 99)
this should become min right? e.g. min((time - ptime) / 10 - 1, 99)
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.
Right, so what we want is the absolute value to not exceed 99. So hence the if x>=100
above.
This PR is very safe, and doesn't actually do anything.
While I was investigating header import (https://github.com/holiman/headerimport/blob/main/README.md ) , I noticed that there is a lot of time spent (allocations spent) on calculating the difficulty.
This PR adds re-written difficulty calculators, which are based on
uint256
. It also adds a fuzzer + oss-fuzz integration for the new fuzzer. It does differential fuzzing between the new and old calculators.Note: this PR does not actually enable the new calculators.
Anyway, if travis looks green, we could merge this and have it fuzzed, and later decide whether about enabling it.
Oh, and there are some benchmarks too,
u256-xx
are the new ones,big-xx
are the old: