fix(node): verify blob content in beacon fallback before accepting a response - #1031
Conversation
…response The fallback beacon client accepted any 200 response carrying enough sidecars, so a beacon serving the right count with corrupted blob bytes (or sidecars missing the requested versioned hash) was accepted, failed verification downstream, and derivation retried the same bad endpoint forever without ever switching. Move content authentication into the fallback loop: each candidate response must contain every requested versioned hash and each matched blob must pass a local KZG commitment round-trip, otherwise the endpoint is recorded as failed and the next one is tried. BlockByNumber failures now abort the attempt instead of degrading to an unfiltered, unverifiable all-blobs query, so the fallback always has hashes to authenticate against. Co-authored-by: Cursor <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBlob retrieval now passes optional indexed hash hints to verified beacon retrieval. The fallback client validates sidecar content, commitments, sizes, and requested hashes. Invalid responses trigger endpoint fallback. L1 block lookup failures no longer stop sidecar retrieval. ChangesBlob retrieval validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant fetchRollupDataByTxHash
participant FallbackBeaconClient
participant blobsFromSidecars
fetchRollupDataByTxHash->>FallbackBeaconClient: request sidecars with optional hash hints
FallbackBeaconClient->>blobsFromSidecars: validate beacon sidecars
blobsFromSidecars-->>FallbackBeaconClient: verified blobs and commitments
FallbackBeaconClient-->>fetchRollupDataByTxHash: return verified sidecars
Possibly related PRs
Suggested labels: 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 (1)
node/derivation/base_client_test.go (1)
215-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the per-endpoint failure metric.
This test creates
*Metrics, but it only checks endpoint hits. Assert oneBeaconRequestFailureincrement for both endpoints. This protects the requirement that corrupt-content failures count as beacon failures.🤖 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_test.go` around lines 215 - 230, Update TestFallbackBeacon_AllFailReturnsError to assert that the real Metrics instance records one BeaconRequestFailure increment for each endpoint after both beacon attempts fail. Keep the existing error, nil sidecars, and endpoint-hit assertions, and verify the corrupt-content fallback contributes equally to the failure metric.
🤖 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 293-297: Update verifySidecars to detect and reject nil entries in
the sidecars slice before accessing sidecar.KZGCommitment, returning the
existing verification error path so fallback endpoint handling can continue.
Keep normal non-nil sidecar hash mapping unchanged.
---
Nitpick comments:
In `@node/derivation/base_client_test.go`:
- Around line 215-230: Update TestFallbackBeacon_AllFailReturnsError to assert
that the real Metrics instance records one BeaconRequestFailure increment for
each endpoint after both beacon attempts fail. Keep the existing error, nil
sidecars, and endpoint-hit assertions, and verify the corrupt-content fallback
contributes equally to the failure metric.
🪄 Autofix
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 Plus
Run ID: a0871d63-e9a9-44b0-ab9c-2e7f3c39bade
📒 Files selected for processing (3)
node/derivation/base_client_test.gonode/derivation/beacon.gonode/derivation/derivation.go
Co-authored-by: Cursor <[email protected]>
![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.
Fold the duplicated match/decode/verify logic into the fallback client: GetVerifiedBlobs now returns the assembled, content-verified sidecar in tx order, and fetchRollupDataByTxHash just uses it. Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
…etch-all self-heal Verification is always against the tx's versioned blob hashes, which need no block body; blob indices are only a beacon query filter. BlockByNumber failure now just drops the hint and fetches the whole slot instead of wedging derivation on a block whose body never loads (#745). Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
node/derivation/base_client_test.go (1)
100-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for multi-blob ordering.
Every test requests a single hash, so no test proves that
blobsFromSidecarsreturns blobs inwantHashesorder. Order is a documented correctness requirement, because batches are decoded by concatenating blob bodies. Add a case with two distinct blobs served in reverse order and assert that the returnedBlobsfollow the requested order.🤖 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_test.go` around lines 100 - 118, Add a multi-blob test covering blobsFromSidecars through the existing FallbackBeaconClient test helpers: serve two distinct sidecars in reverse order, request their hashes in the intended order, and assert the returned Blobs match that requested order. Keep the existing single-blob tests unchanged.
🤖 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 261-266: Update FallbackBeaconClient.GetVerifiedBlobSidecar to
detect an empty c.clients configuration when wantHashes is non-empty and return
an explicit error instead of falling through with a nil error. Preserve the
existing immediate empty-result behavior when wantHashes is empty and the normal
client iteration for configured clients.
---
Nitpick comments:
In `@node/derivation/base_client_test.go`:
- Around line 100-118: Add a multi-blob test covering blobsFromSidecars through
the existing FallbackBeaconClient test helpers: serve two distinct sidecars in
reverse order, request their hashes in the intended order, and assert the
returned Blobs match that requested order. Keep the existing single-blob tests
unchanged.
🪄 Autofix
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 Plus
Run ID: fea5d42e-db53-4fb0-9976-51df6e02bef7
📒 Files selected for processing (3)
node/derivation/base_client_test.gonode/derivation/beacon.gonode/derivation/derivation.go
Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
…t the fetch site Co-authored-by: Cursor <[email protected]>
…e original Co-authored-by: Cursor <[email protected]>
…otation Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
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.
2 participants
Summary
Liveness fix for the beacon fallback path: if a beacon returns the correct number of sidecars but the blob content is wrong, the fallback never switched to the next endpoint.
The fallback client accepted any 200 response with enough sidecars. Content verification only happened downstream in
fetchRollupDataByTxHash, whose error aborts the poll round — the next round starts from the same primary beacon, so derivation stalls on a bad endpoint forever. Safety was never at risk (downstream verification always rejected bad bytes); the bug is that the node gets stuck instead of rotating, with no metric to alert on.Changes
beacon.go: the fallback client now owns blob verification, in one place.GetVerifiedBlobSidecar(ctx, ref, wantHashes, indexHints)tries each beacon in order and returns the assembledBlobTxSidecar(blobs + commitments, in tx order). Verification is purely hash-based: every hash inwantHashes(the L1 tx's versioned blob hashes) must be matched via the commitment-derived hash and pass a local KZG commitment round-trip (verifyBlob). Any failure — error, missing hash, bad bytes, nil/null sidecar entries — counts as an endpoint failure (beacon_request_failure_total) and rotates to the next beacon.beacon.goscope note: fallback only covers per-endpoint data faults; safety comes from hash verification itself, and EL/CL fork mismatches near the head are addressed byconfirmations=finalized, not by trying more beacons.derivation.go:fetchRollupDataByTxHashdrops its own match/decode/verify loop and uses the verified result, so verification runs exactly once.wantHashes(security input, fromtx.BlobHashes()) is deliberately separated fromindexHints(query optimization, needs the full block body): aBlockByNumberfailure just drops the hint and fetches every sidecar at the slot, keeping the Get all blobs corresponding to this timestamp when filter failed #745 self-heal — verification strength is identical with or without hints.base_client_test.go: stubs serve real KZG data (zero blob + its actual commitment); added the failure-mode tests below.New test coverage
TestFallbackBeacon_FallsBackOnCorruptBlobContent: 200, right count, right commitment, corrupted blob bytes → must switch (the original scenario).TestFallbackBeacon_FallsBackOnMissingRequestedHash: 200, right count, but none of the sidecars carries the requested hash (e.g. another fork's sidecars at the same slot) → must switch.TestFallbackBeacon_FallsBackOnNullSidecar: JSONnullin the sidecar list → verification failure and rotation, not a panic.TestFallbackBeacon_VerifiesWithoutIndexHints/TestFallbackBeacon_FallsBackWithoutIndexHintsOnCorruptContent: the no-hint fetch-all path stays fully verified and still rotates on bad content.Test plan
go test ./derivation/ -count=1passes (all existing + new tests)go build ./...innode/Summary by CodeRabbit