fiber refactor block package for performance and simplified sync

[julienrbrt]

Overview

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

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: e03606b0-f0fa-4fbc-b403-90aa5382e039

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-optz

---

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 12, 2026, 8:48 AM

[claude]

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

---

Code Review: fiber refactor block package for performance and simplified sync

Tasks:

• Read AGENTS.md for repo guidelines
• Analyze changed files and git diff
• Review block/internal/submitting changes
• Review block/internal/syncing changes
• Review block/internal/executing changes
• Review block/internal/common changes
• Review DIVERGENCE.md and architecture docs
• Review test coverage changes
• Compile comprehensive review feedback

---

Overview

This PR is a breaking, wire-incompatible refactor with clear goals documented in DIVERGENCE.md. The core changes are:

1. Combined header+data into a single DA blob (halves round trips)
2. P2P sync completely removed — DA is the single source of truth
3. Unified SubmitBlocks replaces SubmitHeaders/SubmitData
4. Custom binary blob format replaces separate protobuf envelope types

The simplification is substantial and well-motivated. Below are issues found, from most to least critical.

---

🔴 Critical

1. Signature verification uses re-marshalled bytes instead of raw blob bytes

In block/internal/syncing/da_retriever.go, the envelope signature is verified against a re-marshalled header rather than the original bytes from the blob:

// processBlobs: after UnmarshalBlockBlob returns *types.SignedHeader
payload, err := header.MarshalBinary()
if valid, err := header.Signer.PubKey.Verify(payload, envelopeSig); ...

But the signature was produced over header.MarshalBinary() before it was stored into the blob (in da_submitter.go). The retriever's UnmarshalBlockBlob parses the raw bytes into a SignedHeader struct, then re-marshals it. If MarshalBinary(UnmarshalBinary(x)) != x for any valid header (proto normalisation, unknown-field stripping, map-key ordering, etc.), verification silently drops every block.

The fix is to have UnmarshalBlockBlob return the raw header bytes alongside the parsed struct — so the verifier can check Verify(rawHeaderBz, envelopeSig) directly, avoiding the round-trip risk:

func UnmarshalBlockBlob(bz []byte) (*types.SignedHeader, []byte /*rawHeaderBz*/, *types.Data, []byte, error)

Fix this →

2. Batching strategy size estimate excludes data — combined blobs will be larger than expected

In block/internal/submitting/submitter.go the ShouldSubmit call only sums marshalled header bytes to estimate blob size:

totalSize := uint64(0)
for _, marshalled := range marshalledHeaders {
    totalSize += uint64(len(marshalled))
}
shouldSubmit := s.batchingStrategy.ShouldSubmit(uint64(len(headers)), totalSize, ...)

Blobs now contain header + data + signature. For blocks with large transaction payloads the actual blob can be many times larger than totalSize. The strategy may approve a batch that exceeds DefaultMaxBlobSize at the DA layer, hitting the ErrOversizedItem fatal-error path. This is a silent regression from the previous code that sized headers and data separately.

Fix this →

---

🟡 Medium

3. Double-marshalling of header per block in SubmitBlocks

da_submitter.go:SubmitBlocks marshals each header twice: once to sign it, then MarshalBlockBlob marshals it again internally:

// da_submitter.go
headerBz, err := header.MarshalBinary()    // call 1 — for signing
envelopeSig, err := signer.Sign(ctx, headerBz)
blob, err := common.MarshalBlockBlob(header, data, envelopeSig) // calls header.MarshalBinary() again inside

// blob.go:MarshalBlockBlob
headerBz, err := header.MarshalBinary()    // call 2 — identical bytes

Pass headerBz into MarshalBlockBlob to avoid the redundant serialisation:

func MarshalBlockBlob(headerBz []byte, data *types.Data, envelopeSig []byte) ([]byte, error)

This also resolves issue #1 naturally — the submitter already has headerBz and that's the authoritative byte slice for both storage and signing.

Fix this →

4. DataNamespace in DAFollowerConfig still subscribes to the old data namespace

The da_follower.go subscriber is still configured with both namespaces:

dataNs := cfg.DataNamespace   // still used!
Namespaces: [][]byte{cfg.Namespace, dataNs},

Since all blobs now go to HeaderNamespace only, subscribing to DataNamespace wastes DA bandwidth and RPC calls. When HeaderNamespace == DataNamespace this is harmless, but for deployments with separate namespaces it means double-fetching empty data.

5. lastSubmittedHeight and DefaultEnvelopeCacheSize are dead code

In da_submitter.go:

// lastSubmittedHeight tracks the last successfully submitted height for lazy cache invalidation
lastSubmittedHeight atomic.Uint64  // never written in new code

DefaultEnvelopeCacheSize = 10_000  // LRU cache was removed

Both constants/fields should be removed to avoid misleading future maintainers.

6. Tracing: SubmitBlocks span lost total_bytes attribute

da_submitter_tracing.go no longer reports byte counts. The span only has block.count, block.start_height, block.end_height. Adding block.total_bytes would restore production observability for monitoring oversized blobs and DA throughput.

---

🟢 Low / Nit

7. processBlobs signature accepts context.Context but ignores it

DARetriever.ProcessBlobs accepts a context but _ context.Context in the implementation. If context is truly unnecessary, the interface should drop the parameter. If it may be needed (e.g., for future cache calls), keep it but document.

8. Silent error in UnmarshalBlockBlob signature field

sig, _, sigErr := readBlobField(bz, off, "signature")
if sigErr == nil {
    envelopeSig = sig
}

A parse failure on the signature field is silently swallowed (returns empty sig). This means a blob with a truncated-but-present sig prefix decodes as "no signature" and falls through to "signature absent" handling rather than being rejected. Consider returning the error or logging it at debug level.

9. initializeState: cache.DaHeight() now always consulted

Old code only used cache.DaHeight() when P2P had advanced past the current state. New code unconditionally takes max(daHeight, s.cache.DaHeight()). On a fresh node start where the cache has no stored height this is fine (returns 0), but a stale/corrupt cache could push daHeight forward inappropriately. This is a minor behaviour change worth a brief comment.

10. Test coverage reduced significantly

The PR deletes da_submitter_test.go almost entirely (478 → 0 lines), removes da_retriever_strict_test.go (97 → 0), and strips most of syncer_test.go (491 → 27). Some deleted tests (TestDASubmitter_SubmitHeaders_Success, strict-mode tests, cross-DA-height matching tests) covered behaviour that still exists in mutated form. New unit tests for SubmitBlocks end-to-end and UnmarshalBlockBlob round-trips (including signature verification) would restore confidence.

---

Architecture Notes

• Wire incompatibility is well-documented in DIVERGENCE.md — good.
• Single namespace submission removes the pendingHeaders/pendingData matching complexity from daRetriever — a real win for correctness and simplicity.
• P2P removal is a significant scope reduction. The SourceP2P event path and all DA-hint machinery are cleanly excised.
• wg.Go() in syncer.go is correct for Go 1.25.7 (confirmed in go.mod).
• The submitter.go inline data fetch (store.GetBlockData inside the goroutine) is a behaviour change from the previous separate pending-data queue. It's sound since data is committed before headers are pended, but worth a comment documenting the ordering invariant.