fix: harden autoclaim L1 detection against batch limits and failing RPCs (#1889)

[arnaubennassar]

🔄 Changes Summary

• Item 1, header batching and multidownloader back-off: JSON-RPC header batch size is now adaptive, with a configurable start size BatchRequestMaxSize. It parses the provider limit from errors like too many batch requests, max is N, otherwise halves, and shrinks only on recognised batch-limit errors. The multidownloader Start loop now uses exponential back-off (100 ms to 10 s, with jitter) and throttled error logging instead of a 1 ms retry loop.
• Item 2, autoclaim per-destination isolation: the L1ToL2 and L2ToLx detectors isolate failing destinations and apply capped exponential back-off (poll period doubling up to 2 min, with jitter), with one Warn per back-off window. New metrics: autoclaim_l1_to_l2_stalled_destinations, autoclaim_l2_to_lx_stalled_destinations, autoclaim_detector_destination_errors_total, autoclaim_detector_skipped_already_claimed_total.
• Item 3, legacy sync downloader: eth_getLogs omissions confirmed by arbitration now splice the recovered logs instead of retrying the whole range (cap of 8 omitted blocks, then fallback to the previous behaviour). New metrics: sync_logs_omission_confirmed_total and sync_logs_omission_range_retries_total. Logs are quieter.
• Item 4, already-claimed trace: bridges skipped as already claimed now leave a Debug log line and a metric.

⚠️ Breaking Changes

• 🛠️ Config: None (one new optional config key with a backward-compatible default).
• 🔌 API/CLI: None
• 🗑️ Deprecated Features: None

📋 Config Updates

• 🧾 Diff/Config snippet: new optional key in the RPC client config ([L1NetworkConfig.RPC] and [Common.L2RPC]), next to BatchBlockHeaderRetrieval:

BatchBlockHeaderRetrieval = true
BatchRequestMaxSize = 1000

A missing key or 0 means 1000; a negative value is a validation error.

🔌 API Updates

🔌 Bridge service API

• 🔌 Bridge service API Update: None

🔌 Proxy API

• 🔌 AProxy API Update: None

🔌 Others API

• 🔌 Others API Update: None

✅ Testing

• 🤖 Automatic:
  • Unit tests per item: batch_size_adaptive tests (etherman), back-off tests in common and multidownloader, L1ToL2/L2ToLx isolation and back-off tests including cursor invariant checks, sync splice tests, and already-claimed trace tests.
  • Component e2e TestE2E_BatchLimitedProvider (go test -run TestE2E_BatchLimitedProvider ./multidownloader/).
  • Docker e2e TestAutoClaimL1ToL2FailingDestinationIsolated (added to the CI autoclaim matrix) plus the existing autoclaim e2e tests, all passing locally.
  • make lint and make test-unit are green on the rebased history.
• 🖱️ Manual: AGGKIT_E2E_ENV=anvil-2chains make test-e2e TEST_RUN='^TestAutoClaimL1ToL2FailingDestinationIsolated$'. Note: e2e metrics output and quoted back-off log lines were not captured (the e2e tests use assertions only); metrics are verified at unit level.

🐞 Issues

• Closes autoclaim: L1 detection robustness (batch-limit error loop w/o back-off, one destination's RPC stalls all, incomplete eth_getLogs) #1889

🔗 Related PRs

• None

📝 Notes

• Item 3 design (option A): when arbitration confirms that eth_getLogs omitted logs for a few blocks, we fetch the missing blocks' logs and splice them into the result rather than retrying the whole range. This is safe because the arbitration result is authoritative for those blocks and the spliced set is checked for completeness as before. If more than K=8 blocks are omitted, we fall back to the previous whole-range retry.
• Known limitations:
  • Retry-After is not honoured because go-ethereum's RPC error drops response headers.
  • While a destination stays failed, L2ToLx holds its shared block cursor at that source's verify row, so verify rows more than BlockWindow blocks later are not reached until it recovers (accepted design, documented in docs/autoclaim.md).
  • The multidownloader omission retry is now spaced by the new back-off.
• Follow-ups (not filed, for the author to decide):
  • Bisect the range on omission.
  • Secondary RPC URL for arbitration.
  • Splice parity in the multidownloader.
  • Persisted skipped_already_claimed request state.
  • Stop failed destinations from holding the L2ToLx block cursor (limit or per-destination cursor).
  • Extract the duplicated back-off bookkeeping from both detectors.
  • Honour Retry-After if a header-preserving transport is added.

🤖 Generated with Claude Code

https://claude.ai/code/session_017eyBoQ7G2hoShKEzAQvZWM

[arnaubennassar]

@claude

[claude]

Claude finished @arnaubennassar's task in 1m 25s —— View job

---

Review of PR #1890: harden autoclaim L1 detection against batch limits and failing RPCs

I read the full diff for the core logic: common (backoff, repeated-error logger, batch-limit parsing), etherman batching, the multidownloader Start loop, the sync completeness splice and both autoclaim detectors. I did not run the tests or lint here, so I'm relying on your note that they are green. Tests and docs were only skimmed.

Overall the design is sound and the isolation, cursor-hold and back-off invariants are well reasoned and documented. I found no 🔴 blockers.

---

🚀 What's New

• Adaptive JSON-RPC header batching. DefaultEthClient now holds a persistent batchSizeLimiter (start size from the new BatchRequestMaxSize). On a recognised batch-limit error it shrinks to the provider-reported limit, or to half the sent size if none is reported. Only the rejected block numbers are retried and headers already fetched are kept.
• Multidownloader Start back-off. Non-reorg step errors now use jittered exponential back-off (100 ms to 10 s) with throttled logging (RepeatedErrorLogger), replacing the 1 ms retry loop.
• Autoclaim L1ToL2 and L2ToLx isolation. A failing destination no longer fails the whole poll. Its cursor is held, the other destinations keep advancing, and it is paused by per-destination capped back-off (poll period doubling up to 2 min, ±20% jitter). New metrics cover stalled destinations, per-destination errors and already-claimed skips.
• Legacy sync downloader. eth_getLogs omissions confirmed by block-hash arbitration are spliced into the result (up to 8 blocks) instead of retrying the whole range. Beyond the cap, or when a block is unverifiable, it falls back to the old range retry. New metrics: sync_logs_omission_confirmed_total and sync_logs_omission_range_retries_total.
• Already-claimed trace. Bridges skipped as already claimed now leave a Debug log and a metric.

🐛 Bug Fixes

• Header retrieval no longer fails persistently against providers with batch caps (too many batch requests, max is N).
• The multidownloader no longer hot-loops and floods logs when a step keeps failing.
• One unreachable or rate-limited destination RPC no longer blocks claim detection for the other destinations (with the L2ToLx limitation below).
• Omitted eth_getLogs results no longer trigger repeated whole-range re-downloads for a single missing block.

📋 Config Updates

New optional key in the RPC client config ([L1NetworkConfig.RPC] and [Common.L2RPC]):

BatchBlockHeaderRetrieval = true
BatchRequestMaxSize = 1000   # default 1000; 0 or missing = 1000; negative = validation error

⚠️ Breaking Changes

None.

---

🟡 Suggested

1. The batch limiter only ever shrinks and never recovers (etherman/batch_size_limiter.go:10, etherman/batch_requests.go:130)
The limiter lives for the process lifetime. A single misclassified or transient error, such as a 413 caused by response size, or a rate-limit message that matches the loose batch (request )?limit keyword in common/errors.go:31, permanently halves the batch size. Repeated occurrences ratchet it down to 1, which means a large increase in RPC round trips until restart. Options:

• Make the keyword regexes more specific.
• Log a clearly visible Warn when it reaches very low values.
• Optionally add a slow probe-up, for example doubling after N consecutive successful batches, capped at the configured BatchRequestMaxSize.

2. CodeQL "incorrect integer conversion" alert at etherman/batch_size_limiter.go:40
This is a false positive in practice. parsed < uint64(sentSize) already bounds the value to a valid int range, but CodeQL does not follow that comparison. To clear the alert without a suppression, bound the value explicitly against math.MaxInt32:

if parsed, ok := aggkitcommon.ParseMaxBatchSizeFromError(err.Error()); ok &&
    parsed >= 1 && parsed < uint64(sentSize) && parsed <= math.MaxInt32 {
    target = int64(parsed)
}

Alternatively, dismiss the alert with a short justification.

3. L2ToLx: a persistently failing destination still holds the shared block-window cursor (autoclaim/bridgedetector/l2_to_lx.go, PollOnce)
This is documented as accepted, and it is no worse than before, since a hard error also left the cursor untouched. It does mean the "isolation" is partial. With a permanently dead destination, verify rows more than BlockWindow blocks past the failing source's row are never reached for any destination. Detection for healthy destinations then silently lags, and the only signal is the stalled-destinations gauge. I'd prioritise the follow-up ("per-destination cursor or limit") and add an alert recommendation on autoclaim_l2_to_lx_stalled_destinations > 0 for N minutes to docs/autoclaim.md.

4. The first back-off window is effectively a no-op (autoclaim/bridgedetector/destination_backoff.go)
The initial window equals the poll period with ±20% jitter. On the first failure the destination is therefore retried on the very next poll (or one poll later), so the 2-minute cap is reached only after about 7 consecutive failures. This is fine as designed and documented, but starting the window at 2x the poll period would make the first failure back off meaningfully.

5. Duplicated back-off bookkeeping in both detectors
updateBackoff, the 429 detection and the Warn formatting are nearly identical in l1_to_l2.go and l2_to_lx.go. You already list this as a follow-up. A small shared helper such as destinationBackoff.logFailure(logger, detector, dest, err) would remove it cheaply, and is worth doing in this PR if the diff stays small.

6. Splice path costs more arbitration queries for suspicious blocks (sync/logs_completeness.go, checkLogsCompleteness)
Previously the first confirmed omission returned immediately. Now the loop continues through the remaining suspicious blocks, up to 8 confirmed, each with up to 2 by-hash queries. The cost is bounded and documented, so this is only a note. The omittedBlocks metric semantics ("a block re-confirmed after a range retry counts again") are also clearly stated in the metric help.

🟢 Optional

• l2_to_lx.go: in the back-off loop at the start of PollOnce, the dup check on outcomes.backedOff is redundant if destinationNetworks is already de-duplicated. Either drop it or leave a short comment saying why it is there.
• RetrieveBlockHeadersBatch (exported) now builds a fresh limiter per call, so the learned limit is not shared across calls. DefaultEthClient is the only production caller and does keep one, so this is fine, but a one-line doc note would avoid surprise for future callers.
• common/repeated_error_logger.go: normalising messages with \d+|0x[0-9a-f]+ means errors that differ only in numbers, such as HTTP 429 vs 500, collapse into one key. This matches the intent, but status codes in messages are now invisible to the "new distinct error" Warn. Consider excluding HTTP status codes from normalisation if distinguishing those matters.

✅ Things I checked and found correct

• The adaptive batch loop terminates. Each rejection of a chunk with more than 1 element lowers the limit below the chunk size (or another goroutine already did), and single-element rejections surface as errors.
• Per-element "batch too large" errors from go-ethereum are lifted into a batch-level rejection only when len(batch) > 1, so a 1-element batch cannot loop forever.
• The multidownloader returns without a Warn on shutdown (runCtx.Err() != nil), and back-off plus logger are reset on success.
• L1ToL2 processBridge checks state.err before updating nextCursor, so deferred bridges neither advance nor mark seen. saveCursors skips failed destinations, so the next poll re-evaluates from the persisted cursor.
• L2ToLx: the failed destination's pair cursor is excluded from advanceLERCursors, the other group members advance, and errors.Join returns after the block-window cursor is handled. The CursorAdvanced plus non-nil error combination is documented.
• The splice keeps address-only unfiltered semantics (arbitration uses the same address filter), validates both hash and block number, and sorts stably by block number, so range-sourced logs are never reordered.
• rpc.HTTPError is matched by value with errors.As, which works because it implements error with a value receiver.

[arnaubennassar]

This PR also appears to fix #1883 (one failing destination withholding the L1→L2 cursor for the whole group) and #1884 (no reactive back-off on 429s): l1_to_l2.go now isolates failures per destination, skips only the failed destination's cursor save, and applies capped per-destination back-off. Could you add Closes #1883 and Closes #1884 to the description so they are closed on merge?

[joanestebanr]

This feautre is already implemented using common/retry_backoff_config.go

Retries policies could be defined as specific delays of a exponential backoff way. Using RetryPolicyGenericConfig.

I suggest to use the already existing mechanism