Skip to content

feat(da): support fiber (not via c-node)#3244

Draft
julienrbrt wants to merge 9 commits intomainfrom
julien/fiber
Draft

feat(da): support fiber (not via c-node)#3244
julienrbrt wants to merge 9 commits intomainfrom
julien/fiber

Conversation

@julienrbrt
Copy link
Copy Markdown
Member

@julienrbrt julienrbrt commented Apr 13, 2026

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
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 13, 2026

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: e2d101db-0a34-4777-bf34-2f15c39aaaf5

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

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 13, 2026

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedApr 15, 2026, 5:41 PM

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 13, 2026

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


Code Review — feat(da): support fiber (not via c-node)

  • Gather context and read repository instructions
  • Read changed files
  • Analyze code quality, architecture, error handling, security, performance, testing, and documentation
  • Post comprehensive review

Summary

This PR adds a Fiber DA client (fiberDAClient) for direct validator communication, along with the fibremock package (moved from pkg/da/fibremock/block/internal/da/fibremock/), configuration flags, and minor SoloSequencer optimizations. The test suite is thorough. Several issues are worth addressing before this graduates from PoC.


Critical / High Priority

1. In-memory-only Retrieve — data not survives restarts

block/internal/da/fiber_client.go:233–291

Retrieve and RetrieveBlobs only consult the process-local c.index map. Any blob submitted before the current process started will return StatusNotFound. For light nodes or syncing nodes that didn't submit the original blobs, this means Fiber DA is completely unusable for retrieval. A production path needs to query the Fiber network (via Download + a Listen/BlobID lookup). This is likely an intentional PoC limitation, but must be clearly documented.

2. Local height counter ≠ DA chain height

block/internal/da/fiber_client.go:165–168 and 383–388

c.latestHeight++
h := c.latestHeight

latestHeight starts at 0 and is a process-local blob counter. GetLatestDAHeight() returns this counter. Any caller (syncer, sequencer, metrics) that interprets this as the actual Celestia chain height will get incorrect data. This needs to either be derived from the Fiber network or clearly documented as "local submission counter."

3. Subscribe silently ignores namespace

block/internal/da/fiber_client.go:329

func (c *fiberDAClient) Subscribe(ctx context.Context, _ []byte, _ bool) (<-chan datypes.SubscriptionEvent, error) {

The namespace argument is discarded. All subscriptions receive events for every blob regardless of namespace. This contradicts the Client interface contract and will cause callers that subscribe to a specific namespace to receive unexpected blobs.

Fix this →


Medium Priority

4. Multi-blob submissions create phantom heights

block/internal/da/fiber_client.go:163–175 and 209–231

When submitting N blobs, latestHeight is incremented once per blob, but indexUploaded stores all N blobs at submitHeight = uploaded[len(uploaded)-1].height (the last blob's height):

// heights 1, 2, 3 are allocated...
c.latestHeight++
h := c.latestHeight  // 1, then 2, then 3

// ...but all 3 blobs are indexed at height 3
submitHeight := uploaded[len(uploaded)-1].height  // 3
c.index[submitHeight] = append(...)

Retrieve(ctx, 1, ns) and Retrieve(ctx, 2, ns) return StatusNotFound, while Retrieve(ctx, 3, ns) returns all 3 blobs. Heights 1 and 2 are "phantom". This is deterministic but confusing. Consider allocating a single height per batch or documenting this contract explicitly.

5. bytes.Equal should replace manual byte comparisons

Three separate places use either a manual loop or string() casting to compare []byte, which is not idiomatic:

  • nsEqual at fiber_client.go:293–302: replace the entire function body with return bytes.Equal(a, b).
  • GetProofs at fiber_client.go:404: string(b.id) == string(id)bytes.Equal(b.id, id).
  • Validate at fiber_client.go:432: same pattern.

The string() conversion allocates and the semantic difference from bytes.Equal can surprise reviewers.

Fix this →

6. No validation when Fiber.Enabled = true

pkg/config/config.go:304–307

IsFiberEnabled() just checks the boolean. When Fiber is enabled, StateAddress, KeyName, and KeyringPath are all required for a real connection but are not validated anywhere. A ValidateFiber() method on FiberDAConfig would catch misconfiguration at startup rather than at connection time.

Fix this →


Low Priority / Style

7. Context is silently ignored in retrieve

block/internal/da/fiber_client.go:241

func (c *fiberDAClient) retrieve(_ context.Context, ...) datypes.ResultRetrieve {

While an in-memory lookup doesn't need a context today, the interface signature includes it for cancellation/tracing. Using _ hides that the implementation won't respect timeouts if the method ever does I/O.

8. Mock subscriber cleanup uses stale index

block/internal/da/fibremock/mock.go:171–183

The subscriber cleanup goroutine captures idx at subscription time. If two subscribers cancel concurrently, the swap-with-last approach can re-close a channel from the wrong subscriber or leave a dangling entry. Consider searching by channel identity (ch) instead of idx:

for i, s := range m.subscribers {
    if s.ch == ch {
        last := len(m.subscribers) - 1
        m.subscribers[i] = m.subscribers[last]
        m.subscribers = m.subscribers[:last]
        break
    }
}

9. faultInjector.SetError lacks synchronization

block/internal/da/fiber_client_test.go:582

func (f *faultInjector) SetError(err error) { f.err = err }
func (f *faultInjector) Upload(...) (...) {
    if f.err != nil { ... }

Concurrent reads/writes to f.err without a mutex will be flagged by the race detector. Use sync/atomic or a sync.Mutex.

10. Mock tests use stdlib t.Fatal vs testify in rest of suite

block/internal/da/fibremock/mock_test.go uses t.Fatal / raw if checks while fiber_client_test.go uses testify/require. Pick one style for consistency.


Sequencer Changes (pkg/sequencers/solo/sequencer.go)

The micro-optimization of pre-allocating static response objects and the in-place filter write-index pattern are correct improvements. The early-return on FilterTxs error is a semantic improvement (returns txs unfiltered rather than patching statuses), and removal of the now-dead FilterRemove case in the switch is correct since it's handled by the default fall-through.


Configuration Changes (pkg/config/config.go)

The FiberDAConfig struct, flags, and defaults are well-structured and consistent with the rest of the config package. The expectedFlagCount := 84 in config_test.go correctly accounts for the 6 new flags.


Overall Assessment

The code quality and test coverage are good for a PoC. The main blockers before production use are: (1) retrieval requires a real Fiber network query, not just an in-memory lookup; (2) the local height counter must not be confused with the DA chain height; and (3) namespace filtering in Subscribe needs to be implemented. These are all expected limitations given the "PoC" framing, but they should be tracked.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 13, 2026

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).

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 and others added 7 commits April 14, 2026 15:12
Adds a fibremock package with:
- DA interface (Upload/Download/Listen) matching the fibre gRPC service
- In-memory MockDA implementation with LRU eviction and configurable retention
- Tests covering all paths

Migrated from celestiaorg/x402-risotto#16 as-is for integration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants