Skip to content

Conversation

@Thegaram
Copy link

@Thegaram Thegaram commented Dec 4, 2025

1. Purpose or design rationale of this PR

Consider the special hard fork state transitions (typically upgrading the L1GasPriceOracle predeploy) during tracing operations like debug_traceTransaction. See also this thread.

2. PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • fix: A bug fix

3. Deployment tag versioning

Has the version in params/version.go been updated?

  • This PR doesn't involve a new deployment, git tag, docker image tag, and it doesn't affect traces
  • Yes

4. Breaking change label

Does this PR have the breaking-change label?

  • This PR is not a breaking change
  • Yes

Summary by CodeRabbit

  • Chores

    • Version bumped to patch 19.
  • Refactor

    • Centralized hard-fork state transitions into a single unified application step across execution, block production, state processing, mining, and tracing.
    • Tracing and state-preparation flows now propagate parent-block context to ensure fork transitions are applied consistently before transaction/tracing execution.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 4, 2025

Walkthrough

This PR centralizes Scroll hard-fork state transition logic by introducing misc.ApplyForkStateTransitions and replacing per-fork conditional Apply...HardFork calls across execution, chain making, state processing, tracing, miner, and state accessor code. It also renames per-fork helpers to unexported forms and bumps VersionPatch from 18 to 19.

Changes

Cohort / File(s) Summary
Hard-fork consolidation
cmd/evm/internal/t8ntool/execution.go, core/chain_makers.go, core/state_processor.go, miner/scroll_worker.go
Replaces scattered per-fork checks/Apply...HardFork calls with a single call to misc.ApplyForkStateTransitions(config, statedb, blockNumber, blockTimestamp, parentTimestamp) at the appropriate point in each control flow.
Centralized fork logic
consensus/misc/forks.go
Adds exported ApplyForkStateTransitions(config *params.ChainConfig, statedb *state.StateDB, blockNumber, blockTimestamp, parentTimestamp uint64) which conditionally invokes the per-fork apply functions based on block number/timestamps.
Per-fork visibility changes
consensus/misc/curie.go, consensus/misc/feynman.go, consensus/misc/galileoV2.go
Renames exported helpers to unexported forms (ApplyCurieHardForkapplyCurieHardFork, ApplyFeynmanHardForkapplyFeynmanHardFork, ApplyGalileoV2HardForkapplyGalileoV2HardFork) without changing behavior.
Tracing integration
eth/tracers/api.go
Adds parentBlock *types.Block to blockTraceTask, wires parent propagation through tracing paths, and invokes misc.ApplyForkStateTransitions at tracing entry points before EIP-2935/parent-hash processing.
State accessor
eth/state_accessor.go
Calls misc.ApplyForkStateTransitions after loading parent state in stateAtTransaction to prepare statedb for transaction execution.
Version bump
params/version.go
Increments VersionPatch from 18 to 19.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Areas needing extra attention:
    • eth/tracers/api.go: ensure parentBlock is consistently propagated and ApplyForkStateTransitions is invoked in the correct tracing paths without duplication.
    • consensus/misc/forks.go: verify conditional logic mirrors previous per-location checks and that timestamp vs block-number decisions are correct.
    • core/state_processor.go and core/chain_makers.go: confirm the unified call maintains ordering relative to EIP-2935 and EVM setup.
    • Confirm renaming of exported helpers to unexported does not break other packages that previously depended on the exported symbols.

Possibly related PRs

Suggested labels

bump-version

Suggested reviewers

  • georgehao
  • jonastheis
  • zimpha

Poem

🐰 In code where forks once hopped apart,
I stitched their steps into one heart.
Curie, Feynman, Galileo too,
Joined in sequence, tidy and true—
Patch nineteen hops in with a start. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'fix: apply hard forks during tracing' clearly describes the main change—applying hard fork state transitions during tracing operations.
Description check ✅ Passed The PR description follows the repository template with all required sections completed: purpose stated, conventional commit type selected, version updated, and breaking change status specified.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat-apply-hard-forks-during-tracing

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cc27ba3 and ed61edb.

📒 Files selected for processing (10)
  • cmd/evm/internal/t8ntool/execution.go (1 hunks)
  • consensus/misc/curie.go (1 hunks)
  • consensus/misc/feynman.go (1 hunks)
  • consensus/misc/forks.go (2 hunks)
  • consensus/misc/galileoV2.go (1 hunks)
  • core/chain_makers.go (1 hunks)
  • core/state_processor.go (1 hunks)
  • eth/state_accessor.go (2 hunks)
  • eth/tracers/api.go (7 hunks)
  • miner/scroll_worker.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • core/chain_makers.go
  • eth/state_accessor.go
  • core/state_processor.go
  • miner/scroll_worker.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/evm/internal/t8ntool/execution.go (1)
consensus/misc/forks.go (1)
  • ApplyForkStateTransitions (49-62)
eth/tracers/api.go (1)
consensus/misc/forks.go (1)
  • ApplyForkStateTransitions (49-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: test
  • GitHub Check: Analyze (go)
🔇 Additional comments (10)
consensus/misc/feynman.go (1)

10-22: LGTM! Clean refactoring to unexported function.

The visibility change from exported to unexported is appropriate for the centralized fork-state transition approach. The function logic remains intact and will be invoked via ApplyForkStateTransitions.

cmd/evm/internal/t8ntool/execution.go (1)

151-153: LGTM! Excellent consolidation of fork-state transitions.

The centralized call to misc.ApplyForkStateTransitions correctly replaces the previous per-fork conditional blocks. Parameter order is correct (blockNumber, blockTimestamp, parentTimestamp), and the placement before EIP-2935 handling ensures proper state sequencing.

consensus/misc/galileoV2.go (1)

10-20: LGTM! Consistent refactoring pattern.

The visibility change aligns with the centralized fork-state transition architecture. The function will be invoked via ApplyForkStateTransitions when GalileoV2 transition conditions are met.

consensus/misc/curie.go (1)

10-23: LGTM! Completes the trilogy of fork function refactorings.

The visibility change is consistent with the Feynman and GalileoV2 refactorings. All three per-fork helpers are now unexported and invoked through the centralized dispatcher.

consensus/misc/forks.go (1)

47-62: LGTM! Well-designed centralized fork dispatcher.

The function correctly handles different fork activation mechanisms:

  • Curie: block-number-based with proper nil check
  • Feynman & GalileoV2: timestamp-based transition detection using parent timestamp

This consolidation improves maintainability by providing a single entry point for fork-state transitions across execution, tracing, and mining flows.

eth/tracers/api.go (5)

206-211: LGTM! Essential addition for fork-state transition support.

Adding parentBlock to blockTraceTask enables proper timestamp-based fork detection during tracing. The parent block's timestamp is required to determine if a block is a transition block for time-activated forks (Feynman, GalileoV2).


283-290: LGTM! Correct fork transition application in concurrent tracing.

The implementation correctly:

  • Applies fork transitions before EIP-2935 parent hash processing
  • Uses task.block.NumberU64() and task.block.Time() for the block being traced
  • Uses task.parentBlock.Time() for parent timestamp to detect transition blocks

The logic at line 420 correctly assigns parentBlock: block where block is the parent of next, ensuring accurate fork detection.


554-562: LGTM! Consistent fork transition application in IntermediateRoots.

Fork transitions are correctly applied with proper parameters before EIP-2935 processing, maintaining consistency with other tracing paths.


638-646: LGTM! Consistent fork transition application in traceBlock.

Fork transitions are correctly applied before EIP-2935 processing, using the parent block's timestamp for transition detection.


783-790: LGTM! Consistent fork transition application in standardTraceBlockToFile.

Fork transitions are correctly applied before EIP-2935 processing, completing the comprehensive tracing coverage.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

yiweichi
yiweichi previously approved these changes Dec 4, 2025
jonastheis
jonastheis previously approved these changes Dec 5, 2025
@Thegaram
Copy link
Author

Thegaram commented Dec 5, 2025

Will test on our EKS rpc nodes before merging.

@Thegaram Thegaram dismissed stale reviews from jonastheis and yiweichi via ed61edb December 5, 2025 12:12
@Thegaram Thegaram merged commit ad45959 into develop Dec 5, 2025
14 checks passed
@Thegaram Thegaram deleted the feat-apply-hard-forks-during-tracing branch December 5, 2025 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants