fix: consider RLE blocks in zstd compatibility check

[noel2004]

Fix an issue that the new checkCompressedDataCompatibilityV7 do not consider RLE type block, which would be possible when we have switched to official zstd for data compression

After this change, checkCompressedDataCompatibilityV7 will serve as a sanity check, it will only return error if there is some issue with the compressed data. Examples include: Data is corrupted, data is truncated, compressed data uses unsupported features (e.g. dictionary). If the compression module is configured correctly, checkCompressedDataCompatibilityV7 will never fail.

See the spec for more details: https://datatracker.ietf.org/doc/html/rfc8878

Summary by CodeRabbit

• Bug Fixes
  • Improved validation of compressed data blocks to reduce decoding failures.
  • Correctly interprets block types, enforcing proper size for RLE blocks and rejecting reserved types with clear errors.
  • Provides more accurate data length checks and error reporting, improving reliability when processing V7-compressed inputs.

[coderabbitai]

Walkthrough

Adds blkType derivation in checkCompressedDataCompatibilityV7, branching handling for block types: forces blkSize=1 for RLE (type 1), errors on reserved (type 3), and otherwise computes blkSize as before. Control flow adjusts data-length validation and block advancement based on blkType.

Changes

Cohort / File(s)	Summary
Block-type handling in V7 compatibility check encoding/da.go	Derive blkType from header; special-case RLE (blkSize=1) and reserved type (error); retain last-block logic; adjust validation and advancement accordingly.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Caller
  participant F as checkCompressedDataCompatibilityV7
  participant H as Header Parser

  C->>F: validate compressed data (v7)
  F->>H: read header
  H-->>F: blkType, header fields

  alt blkType == 1 (RLE)
    note right of F: Force blkSize = 1
    F->>F: validate data length
    F->>F: advance to next block / check last-block
  else blkType == 3 (reserved)
    F-->>C: return error (reserved block type)
  else other types
    F->>F: compute blkSize from header
    F->>F: validate data length
    F->>F: advance / check last-block
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• fix: check v7 batch compressed data compatibility #52 — Also modifies checkCompressedDataCompatibilityV7 in encoding/da.go; this PR further refines block-type handling built atop those changes.

Suggested reviewers

• colinlyguo
• Thegaram
• jonastheis

Poem

I nibble bytes with tidy care,
RLE gets one—I’m well aware.
Type three? I thump: “Beware, verboten!”
The stream hops on, no bits forgotten.
With whiskered logic, swift and neat,
This bunny makes your headers sweet. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description explains the change and its rationale, but is missing required template sections including PR title with conventional commit type and breaking change label checkbox.	Add a PR title following conventional commits format (e.g., 'fix: handle RLE block type in checkCompressedDataCompatibilityV7') and indicate whether this is a breaking change by checking the appropriate checkbox.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title clearly and specifically describes the main change: adding RLE block handling to the zstd compatibility check function.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/da-compabilitycheck

---

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

encoding/da.go (1)

498-507: LGTM! RLE and reserved block handling added correctly.

The logic correctly handles RLE blocks (type 1) by setting blkSize = 1, and properly rejects reserved blocks (type 3). The block type extraction and branching logic are accurate.

However, consider this minor improvement to the error message at line 504:

-			return fmt.Errorf("encounter reserved block type at %v", data)
+			return fmt.Errorf("encounter reserved block type (3) at offset %d", len(originalData)-len(data))

This would show the position rather than dumping potentially large data arrays. To implement this, you'd need to track the original data length before the loop.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b4cce5c and 4e9428c.

📒 Files selected for processing (1)

• encoding/da.go (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2024-10-18T03:40:09.800Z

Learnt from: colinlyguo
PR: scroll-tech/da-codec#25
File: encoding/codecv1_types.go:105-116
Timestamp: 2024-10-18T03:40:09.800Z
Learning: The code in `encoding/codecv1_types.go`, specifically the `Encode` method in `daBatchV1`, has been updated. Previous comments regarding hardcoded byte offsets may be outdated.

Applied to files:

• encoding/da.go

⏰ 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). (1)

• GitHub Check: tests

🔇 Additional comments (1)

encoding/da.go (1)

500-507: Raw blocks (type 0) are valid per ZSTD v7 frame spec Raw (uncompressed) blocks are explicitly defined in the format and must be supported, so accepting type 0 here is correct.

[jonastheis]

I think this change should be relatively low risk as we decode batches before submitting them in the rollup relayer anyway.

As discussed in Slack, we should probably replace this check long-term with a decompression with the underlying library to avoid such compatibility issues and be better maintainable.