Skip to content

feat: Limit aggsender eth_getLogs block ranges#1594

Open
arnaubennassar wants to merge 1 commit into
developfrom
fix/aggsender-max-log-block-range
Open

feat: Limit aggsender eth_getLogs block ranges#1594
arnaubennassar wants to merge 1 commit into
developfrom
fix/aggsender-max-log-block-range

Conversation

@arnaubennassar
Copy link
Copy Markdown
Collaborator

@arnaubennassar arnaubennassar commented Apr 24, 2026

🔄 Changes Summary

  • Recognize the RPC error text query exceeds max block range <n> in the shared max-range parser.
  • Replace AggSender.UnsetClaimsMaxLogBlockRange / Validator.UnsetClaimsMaxLogBlockRange with MaxLogBlockRange.
  • Apply MaxLogBlockRange to aggsender certificate-building eth_getLogs queries, including unset claims and injected/removed GERs.
  • Keep reactive chunking for recognized provider max-range errors and add proactive chunking for injected/removed GER readers.
  • Update default config and aggsender docs for the new parameter.

⚠️ Breaking Changes

  • 🛠️ Config: UnsetClaimsMaxLogBlockRange has been replaced by MaxLogBlockRange under both [AggSender] and [Validator].
  • 🔌 API/CLI: N/A.
  • 🗑️ Deprecated Features: Old UnsetClaimsMaxLogBlockRange config keys are no longer loaded as active config fields; deprecated-field detection reports a migration hint to use MaxLogBlockRange.

📋 Config Updates

  • 🧾 Diff/Config snippet:
[AggSender]
# Max block range per eth_getLogs call issued by AggSender. 0 disables proactive chunking.
MaxLogBlockRange = 0

[Validator]
# Max block range per eth_getLogs call issued by the validator. 0 disables proactive chunking.
MaxLogBlockRange = {{AggSender.MaxLogBlockRange}}

✅ Testing

  • 🤖 Automatic:
    • go test ./aggsender/...
    • go test ./common -run TestParseMaxRangeFromError
    • go test ./l2gersync -run TestL2EVMGERReader_Get(Injected|Removed)GERsForRange
    • go test ./config -run TestLoadDefaultConfig
    • go test ./claimsync -run TestAgglayerBridgeL2Reader_GetUnsetClaimsForBlockRange_ProactiveChunkingByConfig|TestAgglayerBridgeL2Reader_GetUnsetClaimsForBlockRange_WithMockedClient|TestGetUnsetClaimsForBlockRange_ChunkingIntegration
  • 🖱️ Manual: N/A.

🐞 Issues

  • N/A.

🔗 Related PRs

  • N/A.

📝 Notes

  • This addresses the staging failure where injected GER proof generation queried a block range larger than the RPC provider limit and received query exceeds max block range 100000.
  • An earlier broad run of go test ./common ./claimsync ./l2gersync ./config ./aggsender/config ./aggsender/validator ./aggsender/flows hit existing integration instability: claimsync local geth on :8545 reset the connection, and l2gersync TestL2GERSync_IndexLegacyGERManagerSC observed 2 GERs instead of 3.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Claude finished @arnaubennassar's task in 2m 9s —— View job


PR Review: Limit aggsender eth_getLogs block ranges


🚀 What's New

This PR introduces comprehensive block range limiting for eth_getLogs queries across aggsender and validator components. It replaces the narrow UnsetClaimsMaxLogBlockRange config with a broader MaxLogBlockRange config that applies to all certificate-building queries (unset claims, injected GERs, removed GERs). The implementation adds proactive chunking based on the configured max range while maintaining reactive chunking when RPC providers return max-range errors.

🐛 Bug Fixes

  • Fixed error pattern matching to recognize "query exceeds max block range " error format from RPC providers

📋 Config Updates

Breaking Change: Configuration field renamed

  • Old: AggSender.UnsetClaimsMaxLogBlockRange and Validator.UnsetClaimsMaxLogBlockRange
  • New: AggSender.MaxLogBlockRange and Validator.MaxLogBlockRange
  • Default: 0 (disables proactive chunking, reactive chunking still works)
  • Description: Proactive max block range for eth_getLogs calls issued while building/validating certificates. When set to a non-zero value, queries are proactively split into chunks. When 0, queries are attempted without chunking but will still chunk reactively if RPC returns a max-range error.

⚠️ Breaking Changes

  • Configuration field renamed from UnsetClaimsMaxLogBlockRange to MaxLogBlockRange (deprecation warning added in config/config.go:244-250)
  • The new field has broader scope - applies to unset claims, injected GERs, and removed GERs (not just unset claims)

Code Review Feedback

🟢 Strengths

  1. Well-designed error parsing (common/errors.go:18): The new regex pattern for "query exceeds max block range" follows the existing pattern and includes comma handling for formatted numbers.

  2. Comprehensive test coverage: Added tests for both proactive and reactive chunking scenarios in l2gersync/l2_evm_ger_reader_test.go:149-171,237-258.

  3. Consistent implementation: The chunking pattern is consistently applied across claimsync and l2gersync using the shared ChunkedRangeQuery utility.

  4. Proper deprecation handling: The old config field is properly deprecated with clear migration hints (config/config.go:244-250).

  5. Clean factory pattern: Using l2GERReaderFactory (aggsender/flows/builder_flow_factory.go:28) makes the code testable.

🟡 Suggestions

  1. Potential off-by-one issue in chunk boundary (l2gersync/l2_evm_ger_reader.go:79,187):

    if e.maxLogBlockRange > 0 && toBlock-fromBlock >= e.maxLogBlockRange {

    Should this be > instead of >=? If maxLogBlockRange = 1000, a range from block 0 to 1000 is 1001 blocks (inclusive), which exceeds the max range. The current >= condition would chunk a range of exactly 1000 blocks, which might be unnecessary.

    Example: If maxLogBlockRange = 1000, fromBlock = 0, toBlock = 999, then toBlock - fromBlock = 999, which is < 1000, so no chunking occurs. But the range is actually 1000 blocks (0 to 999 inclusive). Consider using toBlock - fromBlock + 1 > maxLogBlockRange for clarity.

  2. Nested chunking concern (claimsync/agglayer_bridge_l2_reader.go:106):

    func (r *AgglayerBridgeL2Reader) getUnsetClaimsInChunks(ctx context.Context,
        fromBlock, toBlock, maxRange uint64) ([]claimsynctypes.Unclaim, error) {
        log.Debugf("block range too large, splitting into chunks of max %d blocks", maxRange)
        return aggkitcommon.ChunkedRangeQuery(
            ctx, fromBlock, toBlock, maxRange,
            r.fetchUnsetClaimsWithFallbackChunking,  // ⚠️ Calls another chunking function
            ...

    getUnsetClaimsInChunks calls fetchUnsetClaimsWithFallbackChunking, which itself can trigger chunking again if it encounters a different max range error. While this should work correctly, it could lead to nested chunking with different chunk sizes. Consider whether this is the intended behavior or if getUnsetClaimsInChunks should call fetchUnsetClaims directly to avoid this complexity.

  3. Missing validation (aggsender/config/config.go:144, aggsender/validator/config.go:52):
    The MaxLogBlockRange field has no validation in the Validate() methods. While 0 is a valid value (disables proactive chunking), should there be a maximum reasonable value? Very small values (e.g., 1) could cause excessive chunking and poor performance.

  4. Documentation clarity: The docs mention "recognized RPC max-range errors" but don't specify which error patterns are recognized. Users might benefit from knowing the exact error formats that trigger reactive chunking.

🔴 Required

None - no critical issues found.

Additional Notes

  1. Test instability acknowledgment: The PR description mentions existing integration test instability (claimsync connection reset, l2gersync incorrect GER count). These appear to be pre-existing issues unrelated to this PR's changes.

  2. Backward compatibility: The deprecation handling is well done. Users will get clear warnings about the renamed field with migration instructions.

  3. Performance consideration: The implementation wisely uses proactive chunking first (if configured) before attempting the full range, which should reduce unnecessary failed RPC calls.


Summary

This is a well-executed PR that expands block range limiting to all certificate-building queries. The implementation is clean, well-tested, and follows existing patterns in the codebase. The main suggestions are minor improvements around boundary conditions and preventing potential edge cases. The breaking config change is properly documented and deprecated.

Recommendation: Approve with minor suggestions to consider.


@sonarqubecloud
Copy link
Copy Markdown

@arnaubennassar arnaubennassar self-assigned this Apr 24, 2026
@arnaubennassar arnaubennassar changed the title Limit aggsender eth_getLogs block ranges feat: Limit aggsender eth_getLogs block ranges Apr 27, 2026
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.

1 participant