Skip to content

Conversation

@flcl42
Copy link
Contributor

@flcl42 flcl42 commented Dec 1, 2025

Getting list of all forks is convenient sometimes.

It's not according to an EIP, just an extension for better debugging

Changes

  • Return all forks if needed

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

@flcl42 flcl42 marked this pull request as ready for review December 1, 2025 11:15
@flcl42 flcl42 requested a review from Marchhill December 1, 2025 11:17
@benaadams benaadams requested a review from Copilot December 1, 2025 21:19
Copilot finished reviewing on behalf of benaadams December 1, 2025 21:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a new debugging feature to the eth_config RPC method, allowing it to return the complete fork schedule for a chain when requested with a boolean parameter. The implementation introduces a static cache to avoid recomputing fork configurations on every request.

Key Changes:

  • Added GetAllForks() method to IForkInfo interface to retrieve the complete fork schedule
  • Extended eth_config RPC method with optional showAllForks parameter to return all known forks
  • Implemented static caching of fork configurations using FrozenDictionary for performance optimization

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/Nethermind/Nethermind.Network/IForkInfo.cs Adds GetAllForks() method to interface and imports System namespace for ReadOnlySpan<T>
src/Nethermind/Nethermind.Network/ForkInfo.cs Implements GetAllForks() returning the internal forks array; updates copyright year
src/Nethermind/Nethermind.JsonRpc/Modules/Eth/IEthRpcModule.cs Updates eth_config signature to accept optional showAllForks parameter; removes unused System import
src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.cs Implements fork configuration caching and conditional return of all forks; refactors fork config building into static helper method
src/Nethermind/Nethermind.JsonRpc/Data/ForkConfigSummary.cs Adds All property to hold the complete list of fork configurations when requested
src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.cs Adds two tests verifying the behavior with and without the showAllForks parameter

@flcl42 flcl42 requested a review from rubo as a code owner December 2, 2025 13:34
@deffrian
Copy link
Contributor

deffrian commented Dec 2, 2025

Maybe we should add new method for that? We already have a lot of compatibility issues when different clients have extra features on the same methods

@flcl42
Copy link
Contributor Author

flcl42 commented Dec 2, 2025

Maybe we should add new method for that? We already have a lot of compatibility issues when different clients have extra features on the same methods

When called in standard way it reveals no difference. What can happen?

@deffrian
Copy link
Contributor

deffrian commented Dec 2, 2025

Maybe we should add new method for that? We already have a lot of compatibility issues when different clients have extra features on the same methods

When called in standard way it reveals no difference. What can happen?

I mean someone could use this feature with nethermind, but when they try to call geth it will fail. We probably shouldn't add extra fields that don't appear in the spec. Maybe it makes sense to move this to debug_ namespace?

@flcl42
Copy link
Contributor Author

flcl42 commented Dec 2, 2025

Maybe we should add new method for that? We already have a lot of compatibility issues when different clients have extra features on the same methods

When called in standard way it reveals no difference. What can happen?

I mean someone could use this feature with nethermind, but when they try to call geth it will fail. We probably shouldn't add extra fields that don't appear in the spec. Maybe it makes sense to move this to debug_ namespace?

According to the same logic user may find it useful in debug_ namespace, and will not find it in geth. And I don't really want to add nethermind_ namespace

@deffrian
Copy link
Contributor

deffrian commented Dec 2, 2025

Maybe we should add new method for that? We already have a lot of compatibility issues when different clients have extra features on the same methods

When called in standard way it reveals no difference. What can happen?

I mean someone could use this feature with nethermind, but when they try to call geth it will fail. We probably shouldn't add extra fields that don't appear in the spec. Maybe it makes sense to move this to debug_ namespace?

According to the same logic user may find it useful in debug_ namespace, and will not find it in geth. And I don't really want to add nethermind_ namespace

Yes, but debug_ is for devs and experimental features, so it's much better to add it there. Imo eth_ namespace should behave exactly the same way between clients, because it used by users.

@flcl42 flcl42 force-pushed the extend-eth-config branch from 5d0b168 to 4e3961e Compare December 2, 2025 18:21
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