feat: configurable batch block header retrieval via RPCClientConfig

[joanestebanr]

🔄 Changes Summary

• Moves BlockHeadersResult, ErrNotFound and IsErrNotFound from etherman to the types package
• Adds RetrieveBlockHeaders(ctx, blockNumbers, maxConcurrency) to the CustomEthereumClienter interface, implemented by DefaultEthClient
• Adds BatchBlockHeaderRetrieval bool (default true) to RPCClientConfig — operators can disable batch RPC requests for nodes that do not support them
• Simplifies multidownloader call sites: the three etherman.RetrieveBlockHeaders(ctx, log, ethClient, rpcClient, ...) calls collapse to dh.ethClient.RetrieveBlockHeaders(ctx, ...)
• Documents RPCClientConfig in docs/common_config.md

⚠️ Breaking Changes

N/A

📋 Config Updates

• 🧾 New optional field in [L1NetworkConfig.RPC] and [Common.L2RPC]:

BatchBlockHeaderRetrieval = true  # default, disable for nodes without batch support

✅ Testing

• 🤖 Automatic: existing unit tests updated (TestEVMMultidownloader_MoveUnsafeToSafeIfPossible, TestEVMMultidownloader_StepSafe); TestLoadDefaultConfig asserts the new default
• 🖱️ Manual: build passes (go build ./...), unit tests pass (go test ./types/... ./etherman/... ./multidownloader/... ./config/...)

📝 Notes

• etherman.BlockHeadersResult and etherman.NewBlockHeadersResult are kept as type aliases for backward compatibility with existing callers
• etherman.ErrNotFound and etherman.IsErrNotFound are kept as wrappers delegating to types
• The standalone etherman.RetrieveBlockHeaders, RetrieveBlockHeadersBatch, RetrieveBlockHeadersLegacy functions remain for direct use in tests and other callers outside the multidownloader

[claude]

Claude finished @joanestebanr's task in 1m 53s —— View job

---

PR Review Complete

---

🚀 What's New

This PR makes batch block header retrieval configurable via RPCClientConfig, allowing operators to disable batch RPC requests for nodes that don't support them. The change moves core types to the types package for better code organization and adds RetrieveBlockHeaders to the CustomEthereumClienter interface.

🐛 Bug Fixes

None

📋 Config Updates

New optional field in [L1NetworkConfig.RPC] and [Common.L2RPC]:

BatchBlockHeaderRetrieval = true  # default value; disable for nodes without batch support

⚠️ Breaking Changes

• API: The CustomEthereumClienter interface gains a new method RetrieveBlockHeaders(ctx, blockNumbers, maxConcurrency) (*BlockHeadersResult, error) — any custom implementations must add this method

---

Review Feedback

🔴 Required Issues

1. Missing nil check in DefaultEthClient.RetrieveBlockHeaders (etherman/default_eth_client.go:175)

Severity: High (potential panic)

The method calls RetrieveBlockHeadersBatch or RetrieveBlockHeadersLegacy without checking if c.logger is nil, but both functions expect a non-nil logger.

func (c *DefaultEthClient) RetrieveBlockHeaders(ctx context.Context, blockNumbers []uint64, maxConcurrency int) (*aggkittypes.BlockHeadersResult, error) {
	if c.batchBlockHeaders {
		return RetrieveBlockHeadersBatch(ctx, c.logger, c.RPCClienter, blockNumbers, maxConcurrency)
	}
	return RetrieveBlockHeadersLegacy(ctx, c.logger, c, blockNumbers, maxConcurrency)
}

While NewDefaultEthClientWithLogger sets a nil logger to log.NewLoggerNil() (line 75), this protection only applies when cfg is nil but rpcClient is also nil. If someone constructs a DefaultEthClient directly or if there's an edge case, this could panic.

Recommendation: Add a defensive nil check at the start of the method or ensure all construction paths validate logger is non-nil.

2. Inconsistent error checking in IsErrNotFound (types/errors.go:12-27)

Severity: Medium (incorrect behavior)

The IsErrNotFound function has overlapping and redundant checks:

func IsErrNotFound(err error) bool {
	if err == nil {
		return false
	}
	if errors.Is(err, ErrNotFound) {
		return true
	}
	if err.Error() == ErrNotFound.Error() {  // Redundant after errors.Is
		return true
	}
	// If error contains "not found" (case sensitive) is an ErrNotFound
	if strings.Contains(err.Error(), "not found") {
		return true
	}
	return false
}

Issues:

• Line 19-21: The err.Error() == ErrNotFound.Error() check is redundant because errors.Is already handles this case
• Line 23-25: The strings.Contains check is too broad and could match unrelated errors (e.g., "configuration not found in database" should not necessarily be treated as a "not found" error)

Recommendation: Remove the redundant check and document the string matching behavior. Consider making the string matching more specific or removing it entirely.

---

🟡 Suggested Improvements

3. Error handling in BlockHeadersResult.ComposeError (types/block_headers_result.go:88-99)

Severity: Low (code quality)

The ComposeError method only composes errors for "not found" blocks but ignores other errors:

func (r *BlockHeadersResult) ComposeError() error {
	if len(r.Errors) == 0 {
		return nil
	}
	errResult := fmt.Errorf("RetrieveBlockHeaders errors")
	errBlockNumbers := r.ListBlocksNumberNotFound()  // Only gets NotFound errors!
	for _, bn := range errBlockNumbers {
		errResult = fmt.Errorf("%w\nBlock %d: %w", errResult, bn, r.Errors[bn])
	}
	return errResult
}

Issue: If there are non-"not found" errors (e.g., RPC timeout, network error), they are silently ignored in the composed error message.

Recommendation: Either rename to ComposeNotFoundError() to be explicit, or iterate over all errors in r.Errors map.

4. Missing validation for maxConcurrency parameter

Severity: Low (potential DoS)

The RetrieveBlockHeaders, RetrieveBlockHeadersBatch, and RetrieveBlockHeadersLegacy functions accept maxConcurrency int without validating it. If maxConcurrency is 0 or negative, errgroup.SetLimit() could behave unexpectedly.

Recommendation: Add validation: if maxConcurrency <= 0 { maxConcurrency = 1 } or document the behavior.

5. Documentation clarity for BatchBlockHeaderRetrieval default

Severity: Low (documentation)

The documentation in docs/common_config.md:146 states:

When true, uses JSON-RPC batch requests to fetch block headers in bulk (faster). Disable if the node does not support batch calls

While correct, it might be helpful to mention specific node types that don't support batch requests (e.g., certain Optimism nodes, custom RPC endpoints) for operator guidance.

---

🟢 Optional / Nice to Have

6. Test coverage for error scenarios

The test TestEVMMultidownloader_MoveUnsafeToSafeIfPossible and TestEVMMultidownloader_StepSafe were updated, but there's no direct test for:

• DefaultEthClient.RetrieveBlockHeaders with batchBlockHeaders=false
• Partial failures in batch block header retrieval

Recommendation: Add tests to verify behavior when BatchBlockHeaderRetrieval is disabled.

7. Backward compatibility type aliases

The PR keeps backward compatibility with:

type BlockHeadersResult = aggkittypes.BlockHeadersResult
var ErrNotFound = aggkittypes.ErrNotFound

This is good! However, consider adding deprecation comments to guide users toward the new location:

// Deprecated: Use aggkittypes.BlockHeadersResult instead.
type BlockHeadersResult = aggkittypes.BlockHeadersResult

---

Security Analysis

✅ No security vulnerabilities identified. The changes are primarily refactoring and configuration additions.

---

Performance Considerations

✅ The changes maintain existing performance characteristics. The new configuration allows operators to improve performance by disabling batch requests on nodes where they cause issues, falling back to parallel individual requests.

---

Code Quality

• ✅ Well-structured refactoring with minimal code duplication
• ✅ Comprehensive test coverage updates
• ✅ Good use of type aliases for backward compatibility
• ⚠️ Minor issues with error handling logic (see Required Issues Split L1 / L2 responsibilities for lastgersync #2)

---

Documentation

✅ Excellent documentation added to docs/common_config.md with clear examples and default values.

---

Summary

This is a well-designed change that improves configurability and code organization. The main concerns are:

1. Potential nil logger panic (should be addressed)
2. Overly broad IsErrNotFound logic (could cause false positives)
3. ComposeError silently ignoring non-"not found" errors

After addressing the required issues, this PR will be ready to merge.

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 05b2e25af4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

Adds operator-configurable block header batch retrieval and consolidates header retrieval logic behind the Ethereum client interface, while relocating shared header/error result types into the types package.

Changes:

• Introduces BatchBlockHeaderRetrieval in RPCClientConfig (default true) and wires it into DefaultEthClient’s header retrieval behavior.
• Adds RetrieveBlockHeaders(ctx, blockNumbers, maxConcurrency) to CustomEthereumClienter and updates multidownloader call sites/tests to use it.
• Moves BlockHeadersResult and not-found error helpers into types, keeping etherman aliases/wrappers for compatibility.

Reviewed changes

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

Show a summary per file

File	Description
types/eth_client.go	Extends client interface with RetrieveBlockHeaders.
types/errors.go	Defines ErrNotFound and IsErrNotFound in types.
types/block_headers_result.go	Adds BlockHeadersResult implementation in types.
etherman/default_eth_client.go	Implements RetrieveBlockHeaders with batch/legacy selection based on config.
etherman/config/network.go	Adds BatchBlockHeaderRetrieval to RPCClientConfig and default config.
etherman/batch_requests.go	Keeps backward-compatible aliases to types.BlockHeadersResult.
etherman/errors.go	Delegates ErrNotFound / IsErrNotFound to types.
multidownloader/evm_multidownloader.go	Replaces standalone helper calls with ethClient.RetrieveBlockHeaders.
multidownloader/evm_multidownloader_test.go	Updates tests to mock RetrieveBlockHeaders.
multidownloader/reorg_processor_port.go	Switches not-found check to types.IsErrNotFound.
types/mocks/*	Regenerates mocks to include RetrieveBlockHeaders.
test/helpers/simulated.go	Adds RetrieveBlockHeaders passthrough on TestClient.
docs/common_config.md	Documents RPCClientConfig, including the new batch flag.
config/default.go	Adds BatchBlockHeaderRetrieval = true to rendered defaults.
config/config_test.go	Asserts new default behavior for the config field.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[Copilot]

Pull request overview

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