[DNM] fix: remove unknown field protection thanks to da enveloppe

[julienrbrt]

Overview

[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: 9e7cb569-9056-4e69-b9ca-76b8cee513b3

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/remove

---

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

[claude]

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

---

Code Review

• Gathering context
• Analyzing changed files
• Providing review feedback

---

Summary

Small but targeted PR with three areas of change: logging improvements in the DA retriever, a type change for JSON height fields, and removal of an upper-bound DoS guard on protobuf unknown fields. The first two are straightforward; the third deserves a closer look.

---

block/internal/syncing/da_retriever.go — Logging improvements

No issues. Adding da_height to both the strict-mode rejection log and the "switching to STRICT MODE" event is a clean improvement for observability. These are exactly the fields an operator would want when investigating sync anomalies.

---

pkg/da/jsonrpc/header.go — string → uint64 for RawHeader.Height and Commit.Height

This change deserves attention. The top-level Header.Height field currently uses:

Height uint64 `json:"height,string,omitempty"`

The ,string tag tells encoding/json to expect the value as a quoted string in JSON (e.g., "height": "12345"). This is the standard behavior for Celestia/CometBFT JSON APIs which serialize int64/uint64 as strings to avoid JavaScript integer overflow.

The new RawHeader.Height and Commit.Height tags are:

Height uint64 `json:"height"`

Without ,string, Go's JSON decoder expects a bare number (e.g., "height": 12345). If the Celestia node returns these nested fields as quoted strings (which is typical for Tendermint-derived APIs), the decoder will silently set the field to 0 rather than returning an error.

Question: Have you verified against the actual Celestia node API response shape that header.header.height and commit.height are returned as JSON numbers and not quoted strings? If they are quoted strings, the fix would be to add ,string:

Height uint64 `json:"height,string"`

---

types/serialization.go — Removal of maxUnknownFieldSize (1 MB DoS guard)

The removed code was:

if len(unknown) > maxUnknownFieldSize {
    return nil, fmt.Errorf("unknown fields too large: %d bytes (max %d)", len(unknown), maxUnknownFieldSize)
}

The rationale in the PR title is that DA envelope validation now provides the trust boundary. This is reasonable at a high level, but worth considering explicitly:

1. Defense-in-depth: decodeLegacyHeaderFields is called during Header.FromProto which runs during binary unmarshaling — potentially before the DA envelope signature has been verified. A blob containing a header with ~500 KB of junk unknown fields (each individually under the 1 KB field-level limit) would still be iterated over in the loop.

2. DA blob size limits: Celestia caps individual blob sizes (typically ~2 MB), which does provide an implicit upper bound. Given that maxUnknownFieldSize was 1 MB, the removed check was largely redundant in practice.

3. Remaining protection: The per-field maxLegacyHashSize = 1024 check is still present and protects against individual oversized fields.

The removal is defensible given the DA-level blob size cap, but it would be good to add a brief comment explaining why the guard was removed (e.g., // DA blob size limits provide an upper bound; per-field size checks remain.) so future readers don't re-add it without context.

---

Minor observations

• The blank line removal between var legacy LegacyHeaderFields and the for loop in serialization.go is a trivial style nit with no functional impact.

[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	Mar 24, 2026, 1:33 PM

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.12%. Comparing base (459f3c9) to head (e147f41).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3194      +/-   ##
==========================================
- Coverage   61.14%   61.12%   -0.02%     
==========================================
  Files         117      117              
  Lines       12082    12080       -2     
==========================================
- Hits         7387     7384       -3     
- Misses       3868     3870       +2     
+ Partials      827      826       -1     

Flag	Coverage Δ
combined	61.12% <100.00%> (-0.02%)	⬇️

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]

This should not be necessary, strict mode work as intented, we are just getting attempting decoding data first sometimes.