[DNM] feat(da): support fiber (not via c-node)

[julienrbrt]

Overview

Support Fiber client (based on https://github.com/celestiaorg/celestia-app/blob/63fbf31cca216fc4e067a9e1b3a3431115c7009b/fibre), but not via celestia node or apex for this PoC
celestiaorg/celestia-node#4892

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 815ab2f3-ee3d-41ad-b66c-d5dbd0195eca

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch julien/fiber

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	May 19, 2026, 7:11 PM

[claude]

Claude finished @julienrbrt's task in 3m 55s —— View job

---

PR Review: [DNM] feat(da): support fiber (not via c-node)

• Gather context and understand PR scope
• Read AGENTS.md and project guidelines
• Review core fiber client implementation
• Review DA submitter changes
• Review DA retriever/syncer changes
• Review config changes and tools
• Review security findings
• Compile and post final review

---

This is a large, architecturally significant PoC. The DIVERGENCE.md document is excellent — it clearly explains the intentional breaking changes. The overall direction (combined blobs, DA-only sync, simplified submitter) is sound. Most issues below are bugs in the new Fiber-specific code that could cause silent failures or correctness problems in production.

---

Critical Bugs

1. context.Background() used in upload — cancellation is broken

block/internal/da/fiber_client.go:102

result, err := c.fiber.Upload(context.Background(), nsID, flat)

The caller's ctx is passed into Submit but ignored for the actual upload. This means:

• Shutdowns / context cancellations will not abort in-flight uploads
• The downstream errors.Is(err, context.Canceled) check (line 114) can never be true, because context.Background() is never cancelled — so that code path is dead

Fix this →

2. Namespace slice panics if namespace is shorter than 10 bytes

block/internal/da/fiber_client.go:95, 202, 321

nsID := namespace[len(namespace)-10:]   // panics if len(namespace) < 10

There's no bounds check before slicing. If a caller passes a namespace shorter than 10 bytes (e.g. misconfiguration), this will panic the node. All three call sites (Submit, retrieve, Subscribe) have this issue.

Fix this →

3. splitBlobs count field not validated — potential OOM

block/internal/da/fiber_client.go:434

count := int(binary.BigEndian.Uint32(data))
blobs := make([][]byte, 0, count)  // count can be up to 4 billion

A corrupted or maliciously crafted blob can set count to math.MaxUint32, causing an immediate OOM allocation. Should cap count at len(data)/4 (minimum 4 bytes per blob entry) before allocating.

Fix this →

4. MockDA subscriber cleanup has an index-corruption bug

block/internal/da/fibremock/mock.go:224-236

The cleanup goroutine uses a captured idx to remove itself by swap-with-last, but idx becomes stale when earlier subscribers are removed first. If subscriber A (idx=0) is removed and swaps B (idx=1) to position 0, then when B's goroutine runs it still has idx=1 — it now removes whichever subscriber is at position 1 (not B itself), corrupting the subscriber list. This is reproducible with 3+ concurrent Listen calls. For a test mock the practical impact is limited, but it's worth fixing before the mock is used in integration tests.

---

Significant Issues

5. CodeQL: Integer overflow in MarshalBlockBlob

block/internal/common/blob.go:25

size := 4 + 4 + len(headerBz) + 4 + len(dataBz) + 4 + len(envelopeSig)

On 32-bit targets this overflows if the combined payload approaches 2 GiB. CodeQL correctly flags this. Cast to int64 (or uint64) before summing or validate individual lengths first.

6. GetProofs / Validate are silently unimplemented

block/internal/da/fiber_client.go:378-398

GetProofs always returns a non-nil error, and Validate always returns true. Callers that rely on either for security decisions (proof verification) will silently skip all validation. These stubs should at minimum return a clearly typed sentinel error (da.ErrNotSupported) so call sites can handle or skip them explicitly.

7. TODO DA height is always 0 in Submit result

block/internal/da/fiber_client.go:149

Height: 0, /* TODO */

The returned Height is always 0. The cache update in the submitter uses this value (cacheMgr.SetHeaderDAIncluded(hash, daHeight, ...)). If callers rely on DA height for sequencing or inclusion proof generation, this silently produces wrong state.

8. DefaultMaxBlobSize is wrong for non-Fiber nodes (admitted HACK)

block/internal/common/consts.go:13-19

The HACK comment acknowledges that this global 128 MiB default is incorrect for the legacy JSON-RPC blob client (bridge nodes reject >5 MiB). This means anyone running a non-Fiber node on this branch will fail DA submission silently. Given this is "DNM" / PoC, acceptable short-term, but the comment says "restructure when throughput-cleanup lands" — that cleanup should be a hard prerequisite before any production merge.

9. panic in NewFiberDAClient instead of returning error

block/public.go:103-104

if err != nil {
    panic(err)
}

Even though NewFiberClient currently only errors on nil client (already checked), panicking from startup initialization is an anti-pattern. Return the error instead.

Fix this →

10. Dual log lines on upload failure

block/internal/da/fiber_client.go:104-128

On upload failure both c.logger.Warn() (with timing/context) and then c.logger.Error() (with just the error) are called, producing two log events for the same failure. Consolidate into a single structured log at Error level with all fields.

---

Code Quality / Minor Issues

11. Typo: heigth variable name

block/internal/da/fiber_client.go:370

heigth, err := c.fiber.Head(headCtx)
return heigth, nil

Rename to height. Fix this →

12. DIVERGENCE.md refers to wrong branch name

block/DIVERGENCE.md:1

The document says branch perf/block-optimization but this is julien/fiber.

13. fiberSubscribeChanSize = 42 is unexplained magic

block/internal/da/fiber_client.go:309

The buffer size of 42 has no documented rationale. Either use a named constant with a comment explaining the sizing (e.g. "holds ~N seconds of blob events at max throughput") or derive it from config.

14. retrieve ignores its bool parameter entirely

block/internal/da/fiber_client.go:198

Both Retrieve and RetrieveBlobs call c.retrieve(ctx, height, namespace, true/false) but the bool parameter is named _ and ignored. If the parameter serves no purpose, remove it from the signature.

15. Missing test coverage for block/public.go (12 uncovered lines)

The codecov report shows block/public.go at 0% coverage for the new NewFiberDAClient and SetMaxBlobSize wrappers. These should have at least basic smoke tests.

---

Architecture Notes

The DIVERGENCE.md is a thoughtful write-up and the design rationale is solid. A few observations for when this moves toward production:

• Forced inclusion namespace: HasForcedInclusionNamespace() always returns false for the Fiber PoC, which disables censorship-resistance verification entirely. The design document should call this out explicitly as a known gap.

• tools/talis: ~2500 lines of new AWS/DigitalOcean infrastructure code with no tests. For a PoC benchmark tool this is acceptable, but the SSH key handling and cloud API calls in aws.go and deployment.go deserve at least a README section on credential management.

• P2P removal: Removing P2P sync from the block package is a major architectural change (Divergence §3). DA-only sync has latency implications (DA finality time vs gossip time). The tradeoff is intentional for the Fiber throughput experiment, but clients relying on P2P fast-sync will need updating.

• da.Subscriber: The Subscribe → da.Subscriber → da.NewSubscriber indirection is clean. The inline fast-path in DAFollower.HandleEvent (skip re-fetch if blobs already present in the subscription event) is a good optimization.

---

[codecov]

Codecov Report

❌ Patch coverage is 90.65657% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.16%. Comparing base (2865d6d) to head (4485d91).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
block/public.go	0.00%	12 Missing ⚠️
block/internal/da/fibremock/mock.go	90.90%	5 Missing and 5 partials ⚠️
block/internal/da/fiber_client.go	96.74%	5 Missing and 3 partials ⚠️
pkg/sequencers/solo/sequencer.go	61.53%	5 Missing ⚠️
pkg/config/config.go	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3244      +/-   ##
==========================================
+ Coverage   62.33%   63.16%   +0.82%     
==========================================
  Files         122      124       +2     
  Lines       12873    13258     +385     
==========================================
+ Hits         8024     8374     +350     
- Misses       3968     3995      +27     
- Partials      881      889       +8     

Flag	Coverage Δ
combined	63.16% <90.65%> (+0.82%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[julienrbrt]

ev-grpc for risotto isn't the solution chosen.