Skip to content

Homestead: reduce redundant int() casts in calculate_block_difficulty (#1415) #1460

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

Homestead: reduce redundant int() casts in calculate_block_difficulty (#1415) #1460

wants to merge 5 commits into from

Conversation

@omegajudith
Copy link

Description

Refactors calculate_block_difficulty in the Homestead fork to avoid repeated int() casts while keeping behavior identical.

  • Convert once to Python int for the (signed) offset math.
  • Compute base/offset, apply difficulty bomb, clamp at MINIMUM_DIFFICULTY.
  • Return type remains Uint.

No behavioral change intended; this is a readability/micro-perf cleanup.

Related issue

Refs #1415.

Rationale

  • Fewer casts → clearer intent, slightly cheaper arithmetic in tight loops.
  • Matches repo guidance to tighten math while preserving protocol correctness.

Test plan

  • Static suite locally:
    • codespell: ✅
    • ruff check + ruff format --check: ✅
    • mypy: ✅
    • (Heads-up) ethereum-spec-lint shows a GlacierForksHygiene message locally even with only Homestead touched; opening as Draft to confirm CI behavior and get guidance.
  • Runtime checks:
    • Fast, scoped run: tox -e py3 -- -k 'homestead and difficulty' ✅
    • Full filler run (project default): tox -e py3 → 61013 passed, 473 skipped on my box ✅
    • Equivalence sanity: compared new vs old implementation over edge cases + random inputs; no mismatches.

Notes

Per maintainer advice, this PR limits changes to Homestead. I’m happy to follow up with the remaining forks after review feedback on this pattern.

@SamWilsn , Homestead-only refactor for #1415. Static is green locally; full tox -e py3 also green. Open as Draft for feedback.

@omegajudith omegajudith mentioned this pull request Oct 8, 2025
Open
@omegajudith omegajudith marked this pull request as ready for review October 8, 2025 15:28
SamWilsn
SamWilsn reviewed Oct 10, 2025
View reviewed changes
Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

Quick initial pass. This review basically amounts to: try not to destroy the surrounding code/docstring/comments.

src/ethereum/forks/homestead/fork.py Outdated
can't be less difficult than the genesis block, therefore each block's
difficulty is set to the maximum value between the calculated
difficulty and the ``GENESIS_DIFFICULTY``.
"""Homestead difficulty: same behavior, fewer int() casts."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably keep the docstring.

src/ethereum/forks/homestead/fork.py Outdated
Comment on lines 745 to 746
pd_int = int(parent_difficulty)
dt_int = int(block_timestamp) - int(parent_timestamp)
Copy link
Contributor

Choose a reason for hiding this comment

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

We try to avoid opaque variable names, and instead prefer full words whenever possible.

src/ethereum/forks/homestead/fork.py
Comment on lines 786 to 790
# Historical Note: The difficulty bomb was not present in Ethereum at the
# start of Frontier, but was added shortly after launch. However since the
# bomb has no effect prior to block 200000 we pretend it existed from
# genesis.
# See https://github.com/ethereum/go-ethereum/pull/1588
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep the comments.

src/ethereum/forks/homestead/fork.py Outdated
num_bomb_periods = (int(block_number) // 100000) - 2
if num_bomb_periods >= 0:
difficulty += 2**num_bomb_periods
diff_int += 1 << num_bomb_periods
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this to a shift?

@omegajudith
Homestead: avoid redundant int() in calculate_block_difficulty (ether…
83ac7fa 
…eum#1415)

- Preserve docstrings and historical comments
- Keep function identical across early PoW forks (Frontier→Spurious Dragon)
Carsons-Eels pushed a commit to Carsons-Eels/execution-specs that referenced this pull request Oct 16, 2025
@danceratopz
chore(tooling): configure markdownlint ul indent to 4 (ethereum#1460)
0aaef4d
danceratopz added a commit to danceratopz/execution-specs that referenced this pull request Oct 22, 2025
@danceratopz
chore(tooling): configure markdownlint ul indent to 4 (ethereum#1460)
84d28db
@omegajudith
Copy link
Author

Hey @SamWilsn -
This PR is now updated to address all your previous review comments.

Specifically:
✅ Docstrings/Comments: The full docstring and historical comments have been preserved.
✅ Variable Names: Opaque names like pd_int and dt_int are now full words (parent_difficulty_int, time_delta).
✅ Bit Shift: The code has been reverted to use difficulty += 2**num_bomb_periods (avoiding the shift change you questioned).
✅ Casting: All casts have been hoisted to the start of the function.

The branch is passing all relevant checks locally (ruff check/format passed, and the scoped test run tox -e py3 -- -k 'homestead and difficulty' is green).

Ready for a second review!

@bomanaps
Copy link
Contributor

Hey @SamWilsn - This PR is now updated to address all your previous review comments.

Specifically: ✅ Docstrings/Comments: The full docstring and historical comments have been preserved. ✅ Variable Names: Opaque names like pd_int and dt_int are now full words (parent_difficulty_int, time_delta). ✅ Bit Shift: The code has been reverted to use difficulty += 2**num_bomb_periods (avoiding the shift change you questioned). ✅ Casting: All casts have been hoisted to the start of the function.

The branch is passing all relevant checks locally (ruff check/format passed, and the scoped test run tox -e py3 -- -k 'homestead and difficulty' is green).

Ready for a second review!

Some of the changes you made are unnecessary, especially in the inline comments and the task is till unsolved the init is still there.

@omegajudith
Copy link
Author

@SamWilsn , wish u could check and we have time to resolve

bomanaps
bomanaps reviewed Oct 24, 2025
View reviewed changes
src/ethereum/forks/frontier/fork.py
"""
Computes difficulty of a block using its header and parent header.
The difficulty of a block is determined by the time the block was created
Copy link
Contributor

Choose a reason for hiding this comment

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

This for example

@omegajudith omegajudith reopened this Oct 26, 2025
@omegajudith
Copy link
Author

iam closing this pr, i have created another one at
#1684
@bomanaps
@SamWilsn

@omegajudith omegajudith deleted the calculate_block_difficulty_1415 branch October 26, 2025 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SamWilsn SamWilsn SamWilsn left review comments

+1 more reviewer

@bomanaps bomanaps bomanaps left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@omegajudith @bomanaps @SamWilsn