feat(node): add beacon RPC fallback for blob sidecar fetching#1021
feat(node): add beacon RPC fallback for blob sidecar fetching#1021curryxbo wants to merge 6 commits into
Conversation
The primary beacon node occasionally fails to serve blob sidecars in production (transport errors or 5xx/404 responses), stalling derivation. Support configuring multiple beacon endpoints so derivation can fall back to another beacon when one temporarily fails. Add FallbackHTTPClient, which implements the existing HTTP interface and tries each endpoint in order, transparently falling through on a transport error or non-200 response. Because it sits at the HTTP layer, the fallback covers all beacon calls (genesis, spec, blob sidecars) uniformly and requires no change to L1BeaconClient. Endpoints are supplied as a comma-separated --l1.beaconrpc value (L1_ETH_BEACON_RPC env), tried primary-first. A single URL yields a one-element list, so existing configs keep working unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBeacon RPC configuration now accepts ordered endpoint lists. Derivation uses sequential blob-sidecar fallback with endpoint failure metrics and tests for successful, empty, HTTP-error, transport-error, and all-failed responses. An unrelated quiescence helper signature is also simplified. ChangesBeacon RPC fallback
Reactor quiescence call cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Derivation
participant FallbackBeaconClient
participant PrimaryBeaconEndpoint
participant FallbackBeaconEndpoint
Derivation->>FallbackBeaconClient: GetBlobSidecarsEnhanced
FallbackBeaconClient->>PrimaryBeaconEndpoint: Request blob sidecars
PrimaryBeaconEndpoint-->>FallbackBeaconClient: Blob sidecars or failure
FallbackBeaconClient->>FallbackBeaconEndpoint: Retry after failure
FallbackBeaconEndpoint-->>FallbackBeaconClient: Blob sidecars or failure
FallbackBeaconClient-->>Derivation: Sidecars or final error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
node/derivation/base_client.go (2)
87-92: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePrioritize context errors on cancellation.
When the context is cancelled, the loop breaks and retains
lastErr. If a previous endpoint encountered a transport error,lastErrwill contain that transport error instead of the context cancellation error. This might mislead callers into treating a cancellation event as a network failure.Consider unconditionally overwriting
lastErrwith the context error when breaking the loop.♻️ Proposed refactor
if err := ctx.Err(); err != nil { - if lastErr == nil { - lastErr = err - } + lastErr = err break }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@node/derivation/base_client.go` around lines 87 - 92, When ctx.Err() is detected in the retry loop, unconditionally assign that context error to lastErr before breaking, replacing any earlier transport error. Update the surrounding error-handling logic in the relevant client method while preserving the existing loop termination behavior.
79-84: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd defensive check for empty endpoint list.
While
Config.SetCliContextguarantees that at least one endpoint is provided,FallbackHTTPClientwould returnnil, nilif instantiated directly with an empty slice. This could lead to a nil-pointer dereference in a downstream caller.Consider explicitly returning an error if
cl.clientsis empty to make this component robust and safer to use in isolation.🛡️ Proposed fix to handle empty clients
var ( lastResp *http.Response lastErr error ) + if len(cl.clients) == 0 { + return nil, errors.New("no beacon endpoints available") + } for i, client := range cl.clients {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@node/derivation/base_client.go` around lines 79 - 84, Update FallbackHTTPClient.Get to detect an empty cl.clients slice before entering the iteration and return a non-nil descriptive error with no response. Preserve the existing fallback behavior for configurations containing one or more clients.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@node/derivation/base_client.go`:
- Around line 102-104: Update the successful-response branch in the endpoint
request flow to close lastResp.Body before returning the new 200 OK response,
but only when lastResp is non-nil. Preserve the existing return of the
successful resp and avoid closing the response being returned.
---
Nitpick comments:
In `@node/derivation/base_client.go`:
- Around line 87-92: When ctx.Err() is detected in the retry loop,
unconditionally assign that context error to lastErr before breaking, replacing
any earlier transport error. Update the surrounding error-handling logic in the
relevant client method while preserving the existing loop termination behavior.
- Around line 79-84: Update FallbackHTTPClient.Get to detect an empty cl.clients
slice before entering the iteration and return a non-nil descriptive error with
no response. Preserve the existing fallback behavior for configurations
containing one or more clients.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2beea3ee-1a74-4b23-8a1c-fc1fb6d8e9f3
📒 Files selected for processing (6)
node/derivation/base_client.gonode/derivation/base_client_test.gonode/derivation/config.gonode/derivation/config_test.gonode/derivation/derivation.gonode/flags/flags.go
Simplify FallbackHTTPClient.Get to a plain in-order loop: return the first endpoint that answers 200, otherwise return the last error. Drop the response-retention bookkeeping. Record each failing endpoint in the new beacon_request_failure_total counter (labelled by endpoint) so a flaky beacon is visible on dashboards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The HTTP-layer FallbackHTTPClient only fell back on transport errors and non-200 responses. But the common production failure is a beacon that answers 200 with an empty or partial sidecar list (slot pruned or not yet indexed) -- that surfaces as an application-level "insufficient sidecars" error *after* the HTTP call returns, so the transport-level fallback never triggered. Replace it with FallbackBeaconClient, which wraps one L1BeaconClient per endpoint and iterates GetBlobSidecarsEnhanced: a beacon is skipped when it errors, is unreachable, or returns fewer sidecars than requested, and the next configured beacon is tried. This covers all failure modes uniformly, including the empty/partial-result case. Each failing endpoint is recorded in beacon_request_failure_total. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
node/derivation/derivation.go (1)
849-864: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse bounded contexts to prevent indefinite hangs.
Hardcoding
context.Background()on network RPCs exposes the node to permanent hangs if the L2 endpoint is unresponsive.
- Pre-write check: Use the component lifecycle context (
d.ctx) so the call cancels cleanly during a node shutdown.- Defer block: While avoiding the parent context here is correct (to ensure reactors restart even if the parent was canceled), the network call must still have a timeout to prevent an indefinite hang.
🛡️ Proposed fix
// pick a sensible resume height. - preWrite, err := d.l2Client.BlockNumber(context.Background()) + preWrite, err := d.l2Client.BlockNumber(d.ctx) if err != nil { d.logger.Error("pre-write BlockNumber read failed; skipping reorg, will retry next poll", "batchIndex", batchIndex, "err", err) return err } if err := d.node.StopReactorsBeforeReorg(); err != nil { d.logger.Error("StopReactorsBeforeReorg failed; skipping reorg, will retry next poll", "batchIndex", batchIndex, "err", err) return err } defer func() { // Use background context so a canceled parent ctx doesn't // prevent reactor restart. height := preWrite - if cur, readErr := d.l2Client.BlockNumber(context.Background()); readErr == nil { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if cur, readErr := d.l2Client.BlockNumber(ctx); readErr == nil { height = cur } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@node/derivation/derivation.go` around lines 849 - 864, Update the pre-write BlockNumber call in the reorg flow to use the component lifecycle context d.ctx instead of context.Background(). In the deferred reactor-restart logic, retain a non-parent context so restart still occurs after cancellation, but wrap the BlockNumber call with a bounded timeout context and ensure its cancellation is released.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@node/derivation/beacon.go`:
- Around line 246-254: Update the client loop around GetBlobSidecarsEnhanced to
detect context cancellation or deadline expiry immediately after the call and
return the context error without recording a beacon failure, logging it, or
trying additional clients. Preserve the existing sidecar validation and retry
behavior for non-context errors.
In `@node/derivation/metrics.go`:
- Around line 131-138: Bind the base labels when constructing
BeaconRequestFailure so calls that increment it include the required values.
Update the prometheus.NewCounterFrom setup for BeaconRequestFailure to apply
labelsAndValues via the existing metric binding pattern, while preserving the
per-increment endpoint label binding in IncBeaconRequestFailure.
---
Outside diff comments:
In `@node/derivation/derivation.go`:
- Around line 849-864: Update the pre-write BlockNumber call in the reorg flow
to use the component lifecycle context d.ctx instead of context.Background(). In
the deferred reactor-restart logic, retain a non-parent context so restart still
occurs after cancellation, but wrap the BlockNumber call with a bounded timeout
context and ensure its cancellation is released.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 219370fa-5710-43e8-be08-89b899d4f8e4
📒 Files selected for processing (4)
node/derivation/base_client_test.gonode/derivation/beacon.gonode/derivation/derivation.gonode/derivation/metrics.go
| var lastErr error | ||
| for i, cl := range c.clients { | ||
| sidecars, err := cl.GetBlobSidecarsEnhanced(ctx, ref, hashes) | ||
| if err == nil && len(sidecars) > 0 && len(sidecars) >= len(hashes) { | ||
| return sidecars, nil | ||
| } | ||
| if err == nil { | ||
| err = fmt.Errorf("beacon returned %d sidecars, want at least %d", len(sidecars), len(hashes)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail fast on context cancellation.
If ctx is canceled (e.g., during node shutdown or a request timeout), the current loop treats the resulting context.Canceled as a beacon failure, increments the metric, logs an error, and proceeds to the next endpoint (which will also immediately fail).
Abort early to avoid spamming logs and artificially skewing the failure metric.
💡 Proposed fix
var lastErr error
for i, cl := range c.clients {
sidecars, err := cl.GetBlobSidecarsEnhanced(ctx, ref, hashes)
if err == nil && len(sidecars) > 0 && len(sidecars) >= len(hashes) {
return sidecars, nil
}
+ if ctx.Err() != nil {
+ return nil, ctx.Err()
+ }
if err == nil {
err = fmt.Errorf("beacon returned %d sidecars, want at least %d", len(sidecars), len(hashes))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var lastErr error | |
| for i, cl := range c.clients { | |
| sidecars, err := cl.GetBlobSidecarsEnhanced(ctx, ref, hashes) | |
| if err == nil && len(sidecars) > 0 && len(sidecars) >= len(hashes) { | |
| return sidecars, nil | |
| } | |
| if err == nil { | |
| err = fmt.Errorf("beacon returned %d sidecars, want at least %d", len(sidecars), len(hashes)) | |
| } | |
| var lastErr error | |
| for i, cl := range c.clients { | |
| sidecars, err := cl.GetBlobSidecarsEnhanced(ctx, ref, hashes) | |
| if err == nil && len(sidecars) > 0 && len(sidecars) >= len(hashes) { | |
| return sidecars, nil | |
| } | |
| if ctx.Err() != nil { | |
| return nil, ctx.Err() | |
| } | |
| if err == nil { | |
| err = fmt.Errorf("beacon returned %d sidecars, want at least %d", len(sidecars), len(hashes)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@node/derivation/beacon.go` around lines 246 - 254, Update the client loop
around GetBlobSidecarsEnhanced to detect context cancellation or deadline expiry
immediately after the call and return the context error without recording a
beacon failure, logging it, or trying additional clients. Preserve the existing
sidecar validation and retry behavior for non-context errors.
Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Why
In production the connected beacon occasionally fails to serve blob sidecars, stalling derivation.
--l1.beaconrpcaccepts only one endpoint, so a single flaky beacon has no backup.Crucially, the failure often isn't a transport error or
5xx— the beacon answers200with an empty or partial sidecar list (slot pruned, or not yet indexed). A transport-level fallback would treat that200as success and never retry another node.What
Add
FallbackBeaconClient(node/derivation/beacon.go): it wraps oneL1BeaconClientper configured endpoint and, inGetBlobSidecarsEnhanced, tries each beacon in order. A beacon is skipped and the next one tried when it:The first beacon that actually carries the requested blobs wins; if all fail, the last error is returned. Because the check sits at the blob-fetch layer, it covers every failure mode uniformly — including the
200-but-incomplete case the earlier HTTP-layer approach missed.derivation_beacon_request_failure_totalcounter, labelled byendpoint, incremented for each beacon that fails to serve the blob before fallback — so the flaky node is visible on dashboards.--l1.beaconrpc(L1_ETH_BEACON_RPC) now accepts a comma-separated list, tried primary-first.Config.BeaconRpcList()splits/trims/drops empties.Compatibility
Fully backward compatible: a single URL yields a one-element list and behaves like a bare
L1BeaconClient. No new flags, no config migration.Testing
TestFallbackBeacon_*(stub beacon serving genesis/spec/blob_sidecars): primary serves blob (fallback untouched), falls back on empty result, falls back on 5xx, falls back on transport error, all-fail returns error + records per-endpoint metric.TestBeaconRpcList— single / multiple / trims / drops empties / empty / separators-only.go test ./derivation/passes (two unrelatedTestParseBatch*failures come from separate in-progress zstd changes, not this PR).🤖 Generated with Claude Code
Summary by CodeRabbit