feat: hash from rpc instead of calculated v2 1389 #1397

joanestebanr · 2025-12-12T14:06:34Z

🔄 Changes Summary

The eth client could use Hash field as blockHash in rpc Json response instead of calculating it from fields (default behaviour for go-ethereum package)

Internal changes

Add a new function to `EthClienter` interface:

CustomHeaderByNumber(ctx context.Context, number *BlockNumberFinality) (*BlockHeader, error)

It requires a new name because can change HeaderByNumber because it returns a specific type of go-ethereum and Hash() function is not possible to override
The new function use internal types, for blockNumber and the block header struct

Refactor `BlockNumberFinality`

It supports a number (so it can be used to retrieve a named block or a specific one)
Previous type BlockNumber have been renamed to BlockName to avoid confusions
Removed BlockNumberFinality.BlockHeader now it's supported directly by CustomHeaderByNumber

Removed object `NoopRPCClient`:

If the RPC direct call is not availablee is handle by DefaultEthClient
If you call to any direct call not supported returns an error (to avoid silently by pass errors)

Unified eth clients configuration

The configuration struct L2RPCClientConfig have extended to L1, also. To keep the previous behaviour L1 client can on be mode basic

Always create L1 client and use it for `RollupDataQuerier`

The object RollupDataQuerier is always created (not related to components) so the L1 client need to be created always

⚠️ Breaking Changes

N/A

📋 Config Updates

🛠️ Config: Add config field HashFromJSON for rpc clients:

[L2RPC] 
	HashFromJSON = true

	[L1NetworkConfig.RPC]
		HashFromJSON = true

If the param HashFromJSON is true use a direct the Hash field instead of calculating, this require to pass a valid RPCClient with support for directs Calls (so the internals e2e that use simulated environment can't use this feature)

✅ Testing

🤖 Automatic: e2e, unittest
Manually: run a bridgesyncer against network: bali-63-outpost

🐞 Issues

Closes bug: some L2 doesn't match hash with calculated #1389

Pending works

Remove specific calls to rpc.Call, instead incorporate it to ethClienter:
- bridgesyncer: eth_getTransactionByHash
- bridgesyncer: debug_traceTransaction
Rename multidownloader HeaderByNumber to CustomHeaderByNumber

etherman/default_eth_client.go

types/block_finality.go

Copilot

Pull request overview

This PR refactors how block headers and hashes are retrieved from Ethereum RPC nodes, transitioning from using calculated hashes (via go-ethereum's RLP hashing) to hashes directly from JSON-RPC responses. The changes introduce support for constant block numbers alongside the existing finality keywords (latest, safe, finalized, pending).

Key changes:

Introduced BlockName type (renamed from BlockNumber) and added support for constant/specific block numbers via BlockNumberFinality.Specific field
Added CustomHeaderByNumber method to retrieve block headers with hash from JSON-RPC
Refactored EthClienter interface hierarchy and created DefaultEthClient implementation
Updated LessFinalThan to return an error when comparing incompatible block types
Updated all usage sites to use new type signatures and error handling

Reviewed changes

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

Show a summary per file

File	Description
types/block_finality.go	Core refactoring: added Constant block type, renamed BlockNumber to BlockName, updated LessFinalThan signature to return error
types/multidownloader.go	Updated HeaderByNumber signature to use BlockNumberFinality instead of *big.Int
types/eth_client.go	Refactored interface hierarchy, removed DialWithRetry (moved to etherman package)
types/block_header.go	Added RequestedBlock field to track original block request
etherman/default_eth_client.go	New default implementation with HashFromJSON flag for RPC-based hash retrieval
etherman/rpcnoopclient.go	New noop RPC client implementation
etherman/batch_requests.go	Defines blockRawEth struct for JSON-RPC responses
multidownloader/evm_multidownloader_syncers.go	Updated to use BlockNumberFinality and handle only constant blocks
multidownloader/types/syncer_config.go	Added error handling for LessFinalThan comparison
test/helpers/e2e.go	Changed default RPC clients from NoopRPCClient to nil
Multiple test files	Updated mocks and tests to use new signatures

types/block_finality.go

multidownloader/types/syncer_config.go

multidownloader/evm_multidownloader_syncers.go

etherman/config/network_test.go

etherman/default_eth_client.go

types/block_finality.go

Copilot

Pull request overview

Copilot reviewed 52 out of 52 changed files in this pull request and generated no new comments.

temaniarpit27 · 2025-12-18T15:14:50Z

cmd/run.go

-		return nil, fmt.Errorf("failed to create Ethereum client for L1 using URL: %s. Err: %w", cfg.RPC.URL, err)
-	}
-	return ethermanquierier.NewRollupDataQuerier(ctx, cfg, ethClient,
+	return ethermanquierier.NewRollupDataQuerier(ctx, cfg, l1Client,


nit: ethermanquerier

sorry, I can't see the difference! 😞

ok, I understand, you think that is better to remove checking the error. I did in that way to it's more easy to find the source of the error when you see in logs

temaniarpit27 · 2025-12-18T15:33:08Z

log/log_nil.go

@@ -0,0 +1,50 @@
+package log


where are we using this?

If the log is nil then create a new instance of LoggerNil here

in what cases, the logger will be nil?

yes, you pass to the constructor function a nil as logger and internally it's filled with a instance of LoggerNil

vcastellm

Still reviewing but I'm not completely sure about this solution, the differences on the hash calculation are because of op-geth differences in block header, etc. so it looks like the proper client should be used instead of coming up with our own trickery here. See https://github.com/ethereum-optimism/op-geth/tree/optimism/ethclient

Let me think a bit more on this

vcastellm

Looks like we should select the op-stack ethClient for L2 syncers based on op

sonarqubecloud · 2025-12-18T16:43:06Z

Quality Gate passed

Issues
20 New issues
0 Accepted issues

Measures
0 Security Hotspots
80.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

vcastellm · 2025-12-18T17:26:58Z

etherman/default_eth_client.go

+	}
+	var result *aggkittypes.BlockHeader
+	if c.HashFromJSON {
+		result, err = c.rpcGetBlockByNumber(ctx, numberBigInt)


This is called here too https://github.com/agglayer/aggkit/pull/1397/changes#diff-b1f974dfe4dd0deb115cab293014c56d3ceac09854aa5a5adeca534426c5bfd6R129 so we can save a call to the rpc if we collapse the two calls, there are several ways to do it

which call do you refer? rpcGetBlockByNumber? can you share again the link, it's not clear to the code that you mean

joanestebanr · 2025-12-19T13:12:40Z

Looks like we should select the op-stack ethClient for L2 syncers based on op

I suggest that after applying this PR that abstract the client we can do another PR that allow to choose a new mode = 'op-stack' and use the more convinient client, WDYT?

Copilot

Pull request overview

Copilot reviewed 52 out of 52 changed files in this pull request and generated no new comments.

joanestebanr self-assigned this Dec 12, 2025

github-advanced-security bot found potential problems Dec 12, 2025

View reviewed changes

etherman/default_eth_client.go Fixed Show fixed Hide fixed

types/block_finality.go Fixed Show fixed Hide fixed

joanestebanr requested a review from Copilot December 15, 2025 09:22

Copilot started reviewing on behalf of joanestebanr December 15, 2025 09:23 View session

Copilot AI reviewed Dec 15, 2025

View reviewed changes

joanestebanr force-pushed the feat/hash_from_rpc_instead_of_calculated_v2-1389 branch from b1822e4 to 897d073 Compare December 15, 2025 09:49

joanestebanr requested a review from Copilot December 16, 2025 08:58

Copilot started reviewing on behalf of joanestebanr December 16, 2025 08:58 View session

Copilot AI reviewed Dec 16, 2025

View reviewed changes

joanestebanr marked this pull request as ready for review December 16, 2025 10:58

joanestebanr force-pushed the feat/hash_from_rpc_instead_of_calculated_v2-1389 branch 3 times, most recently from c99fd19 to a23d8ea Compare December 17, 2025 16:26

temaniarpit27 approved these changes Dec 18, 2025

View reviewed changes

vcastellm requested changes Dec 18, 2025

View reviewed changes

joanestebanr added 15 commits December 18, 2025 17:28

feat: wip

792aa3e

feat: wip

2de9669

feat: wip

4c973b4

fix: ut, lint

6328178

fix: copilot comments

4cc0f28

fix: ut

6a32c7f

fix: missing file

4a62d15

fix: fix ConvertStringToNumber

8201d24

feat: refactor client config

12e987c

feat: refactor client config

f5c7224

fix: ut

fbe5091

feat: add logger to ethClient

b745263

feat: removed blockFinality.BlockHeader

7cd3445

fix: ut

af4deb8

fix: startup errors

e6e53ae

joanestebanr added 13 commits December 18, 2025 17:28

fix: ut

0fc2264

fix: lint

4eec23a

fix: ut

1466484

fix: ut

e653c08

fix: ut

6800cf8

fix: ut

6d41963

fix: removed rpcnoopclient.go

b46d980

fix: removed rpcnoopclient.go

bfff19b

fix: coverage

f13b3fd

fix: coverage

e254bcd

fix: coverage

c332db2

fix: coverage

d73da4d

fix: coverage

e2cabeb

joanestebanr force-pushed the feat/hash_from_rpc_instead_of_calculated_v2-1389 branch from a23d8ea to e2cabeb Compare December 18, 2025 16:31

vcastellm reviewed Dec 18, 2025

View reviewed changes

joanestebanr requested review from Copilot, temaniarpit27 and vcastellm December 19, 2025 13:13

Copilot started reviewing on behalf of joanestebanr December 19, 2025 13:14 View session

Copilot AI reviewed Dec 19, 2025

View reviewed changes

temaniarpit27 approved these changes Dec 19, 2025

View reviewed changes

feat: hash from rpc instead of calculated v2 1389 #1397

Are you sure you want to change the base?

feat: hash from rpc instead of calculated v2 1389 #1397

Uh oh!

Conversation

joanestebanr commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔄 Changes Summary

Internal changes

Add a new function to EthClienter interface:

Refactor BlockNumberFinality

Removed object NoopRPCClient:

Unified eth clients configuration

Always create L1 client and use it for RollupDataQuerier

⚠️ Breaking Changes

📋 Config Updates

✅ Testing

🐞 Issues

Pending works

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joanestebanr Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vcastellm left a comment

Choose a reason for hiding this comment

Uh oh!

vcastellm left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Dec 18, 2025

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joanestebanr commented Dec 19, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

joanestebanr commented Dec 12, 2025 •

edited

Loading

Add a new function to `EthClienter` interface:

Refactor `BlockNumberFinality`

Removed object `NoopRPCClient`:

Always create L1 client and use it for `RollupDataQuerier`

joanestebanr Dec 19, 2025 •

edited

Loading