Fix GetBlobSidecarsEnhanced #825
Conversation
WalkthroughReplaces strict per-index sidecar validation with a minimum-count check, removes single-index retrieval/verification helpers and hex/KZG logic, and adds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (1)node/derivation/beacon.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (3)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
node/derivation/beacon.go (1)
250-278: Inconsistent validation in fallback path.When the fallback path at line 274 returns
blobResp.Data, it skips the validation thatGetBlobSidecarsperforms (ensuring at leastlen(hashes)sidecars are returned). This could return fewer sidecars than needed, causing downstream failures in callers that expect all requested hashes to be present.Additionally, shadowing
errinside the if-block (line 257) loses the original error fromGetBlobSidecars, making debugging harder.func (cl *L1BeaconClient) GetBlobSidecarsEnhanced(ctx context.Context, ref L1BlockRef, hashes []IndexedBlobHash) ([]*BlobSidecar, error) { // First try using the original GetBlobSidecars method blobSidecars, err := cl.GetBlobSidecars(ctx, ref, hashes) - if err != nil || len(blobSidecars) == 0 { + if err == nil && len(blobSidecars) >= len(hashes) { + return blobSidecars, nil + } + + // If failed or insufficient blobs retrieved, try the second method + slotFn, slotFnErr := cl.GetTimeToSlotFn(ctx) + if slotFnErr != nil { + return nil, fmt.Errorf("failed to get timeToSlotFn: %w", slotFnErr) + } - // If failed or no blobs retrieved, try the second method - slotFn, err := cl.GetTimeToSlotFn(ctx) - if err != nil { - return nil, fmt.Errorf("failed to get timeToSlotFn: %w", err) - } + slot, slotErr := slotFn(ref.Time) + if slotErr != nil { + return nil, fmt.Errorf("failed to calculate slot: %w", slotErr) + } - slot, err := slotFn(ref.Time) - if err != nil { - return nil, fmt.Errorf("failed to calculate slot: %w", err) - } + // Build request URL and use apiReq method directly + method := fmt.Sprintf("%s%d", sidecarsMethodPrefix, slot) + var blobResp APIGetBlobSidecarsResponse + if reqErr := cl.apiReq(ctx, &blobResp, method); reqErr != nil { + return nil, fmt.Errorf("failed to request blob sidecars: %w", reqErr) + } - // Build request URL and use apiReq method directly - method := fmt.Sprintf("%s%d", sidecarsMethodPrefix, slot) - var blobResp APIGetBlobSidecarsResponse - if err := cl.apiReq(ctx, &blobResp, method); err != nil { - return nil, fmt.Errorf("failed to request blob sidecars: %w", err) - } - - return blobResp.Data, nil + if len(blobResp.Data) < len(hashes) { + return nil, fmt.Errorf("expected at least %v sidecars but got %v", len(hashes), len(blobResp.Data)) } - return blobSidecars, nil + return blobResp.Data, nil }
🧹 Nitpick comments (1)
node/derivation/beacon_test.go (1)
17-21: Consider adding test skip annotation for clarity.The test silently returns when
BLOB_URLis not set. Consider usingt.Skip()to make test output clearer in CI environments.func TestGetBlob(t *testing.T) { url := os.Getenv("BLOB_URL") if url == "" { - return + t.Skip("BLOB_URL environment variable not set, skipping integration test") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
node/derivation/beacon.go(1 hunks)node/derivation/beacon_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
node/derivation/beacon.go (1)
ops/l2-genesis/eth/types.go (1)
Data(123-123)
node/derivation/beacon_test.go (2)
node/derivation/blobs.go (1)
BlobSidecar(15-22)node/derivation/beacon.go (1)
L1BlockRef(96-101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
node/derivation/beacon.go (1)
134-139: LGTM! Relaxed validation aligns with Beacon API behavior.The change from exact equality to
>=check is correct since some Beacon nodes return all sidecars for a slot regardless of theindicesparameter. ThesidecarFromSidecarsfunction properly filters by index and verifies blob hashes downstream.node/derivation/beacon_test.go (1)
44-51: Test correctly updated for new API.The test properly uses
GetBlobSidecarsEnhancedand handles the slice return type. Note thatlen(bts)now counts the number of sidecar entries returned, which differs from the previouslen(bts.Blobs)semantics, but this is correct for the new API.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
node/derivation/beacon.go (1)
201-229: Several issues in the fallback logic.
Original error discarded: When
GetBlobSidecarsfails, the error is silently discarded. Consider logging or wrapping it for debugging.Empty
hashestriggers unnecessary fallback: Ifhashesis empty,GetBlobSidecarsreturns an empty slice (line 106-108), which triggers the fallback to fetch ALL sidecars for the slot unnecessarily.Fallback lacks validation:
GetBlobSidecarsvalidateslen(resp.Data) < len(hashes), but the fallback path doesn't perform this check, potentially returning insufficient data.Variable shadowing: Line 208 redeclares
errwith:=, shadowing the outererrvariable.func (cl *L1BeaconClient) GetBlobSidecarsEnhanced(ctx context.Context, ref L1BlockRef, hashes []IndexedBlobHash) ([]*BlobSidecar, error) { + if len(hashes) == 0 { + return []*BlobSidecar{}, nil + } + // First try using the original GetBlobSidecars method - blobSidecars, err := cl.GetBlobSidecars(ctx, ref, hashes) - if err != nil || len(blobSidecars) == 0 { + blobSidecars, firstErr := cl.GetBlobSidecars(ctx, ref, hashes) + if firstErr != nil || len(blobSidecars) == 0 { // If failed or no blobs retrieved, try the second method slotFn, err := cl.GetTimeToSlotFn(ctx) if err != nil { - return nil, fmt.Errorf("failed to get timeToSlotFn: %w", err) + return nil, fmt.Errorf("failed to get timeToSlotFn (original error: %v): %w", firstErr, err) } slot, err := slotFn(ref.Time) if err != nil { - return nil, fmt.Errorf("failed to calculate slot: %w", err) + return nil, fmt.Errorf("failed to calculate slot (original error: %v): %w", firstErr, err) } // Build request URL and use apiReq method directly method := fmt.Sprintf("%s%d", sidecarsMethodPrefix, slot) var blobResp APIGetBlobSidecarsResponse if err := cl.apiReq(ctx, &blobResp, method); err != nil { - return nil, fmt.Errorf("failed to request blob sidecars: %w", err) + return nil, fmt.Errorf("failed to request blob sidecars (original error: %v): %w", firstErr, err) + } + + if len(blobResp.Data) < len(hashes) { + return nil, fmt.Errorf("expected at least %v sidecars but got %v", len(hashes), len(blobResp.Data)) } return blobResp.Data, nil } return blobSidecars, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
node/derivation/beacon.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
node/derivation/beacon.go (1)
ops/l2-genesis/eth/types.go (1)
Data(123-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
Summary by CodeRabbit
Bug Fixes
API Changes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.