-
Notifications
You must be signed in to change notification settings - Fork 332
fix: block difficulty calc frontier #1409
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
fix: block difficulty calc frontier #1409
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.
Huh, is this broken all the way up to the merge?
@@ -768,7 +768,7 @@ def calculate_block_difficulty( | |||
# See https://github.com/ethereum/go-ethereum/pull/1588 | |||
num_bomb_periods = (int(block_number) // 100000) - 2 | |||
if num_bomb_periods >= 0: | |||
difficulty += 2**num_bomb_periods | |||
difficulty += Uint(2**num_bomb_periods) |
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.
num_bomb_periods = (block_number // Uint(100000)) - Uint(2)
if num_bomb_periods >= 0:
difficulty += Uint(2) ** num_bomb_periods
Kinda just thinking out loud, but would it be better in your opinion to do the math as Uint
, or do the conversion at the end?
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.
yeah was thinking the same! that would make it consistent with the other forks. Also do you think in that case converting difficulty to int for doing the calculations is better to be consistent?
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.
difficulty = int(difficulty)
num_bomb_periods = (int(block_number) // 100000) - 2
if num_bomb_periods >= 0:
difficulty += 2**num_bomb_periods
return max(Uint(difficulty), MINIMUM_DIFFICULTY)
is this better?
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.
Does Frontier have a minimum 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.
the other forks seems to be okay with the calculation - looks like only a problem with frontier |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## forks/osaka #1409 +/- ##
============================================
Coverage 94.94% 94.94%
============================================
Files 583 583
Lines 34665 34665
Branches 3070 3070
============================================
Hits 32914 32914
Misses 1198 1198
Partials 553 553
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
What you had is fine, now that I've looked a bit more deeply at it. Post-Frontier forks convert everything to |
What was wrong?
Frontier block difficulty calculation was broken
How was it fixed?
Type conversion
Cute Animal Picture