ethereum-optimism / ethereum-optimism/optimism
kona/protocol: brotli decoder (brotli-decompressor) is more lenient than op-node (andybalholm) — accepts malformed channels op-node rejects
- Dominant language
- Go
- Stars
- 6.5k
- Forks
- 4k
- Avg merge
- 2d 15h
- Merged PRs (30d)
- 145
Description
## Summary
Differential testing of the two brotli decoders used in OP Stack derivation — `brotli-decompressor` 5.0.1 (kona, [`decompress_brotli`](https://github.com/ethereum-optimism/optimism/blob/44e40ba521c9ec2ad02f3ed7db3e79055afa9877/rust/kona/crates/protocol/protocol/src/brotli.rs#L19-L90)) vs `andybalholm/brotli` v1.1.0 (op-node, [`BatchReader`](https://github.com/ethereum-optimism/optimism/blob/44e40ba521c9ec2ad02f3ed7db3e79055afa9877/op-node/rollup/derive/channel.go#L174-L219)) — shows the two disagree on brotli **validity**. The consensus-relevant direction: **kona's decoder accepts malformed channels that op-node rejects.** kona would then RLP-decode batches op-node discards. If any such batch is valid, kona advances the safe chain where op-node does not → consensus split. This is the mirror image of #20666 (which worried about the opposite direction — that one turns out to be structurally impossible for the current eager decoder).
This is the *residual* decoder gap behind [protocol-team#84 (strict channel decompression)](https://github.com/ethereum-optimism/protocol-team/issues/84): strict semantics remove the unbounded partial-output surface but leave this validity-boundary disagreement, which needs decoder conformance work.
## Evidence
Over 70,526 adversarial streams (corruptions + truncations + fuzz), classifying each by whether the decoder reaches a *clean* end-of-stream:
| | count |
|---|---|
| both decoders accept | 58,695 |
| …of which **byte-length mismatch** | **0** |
| both reject | 5,941 |
| **kona accepts, op-node rejects** (kona too lenient) | **242** |
| kona rejects, op-node accepts (op-node too lenient) | 5,648 |
Key facts:
- **Accepted streams never diverge** (0 length mismatches) — valid brotli is deterministic. All divergence is at the accept/reject boundary.
- The 242 kona-too-lenient cases collapse to two root causes: `BLOCK_LENGTH_1/2` (98) and `excessive input` (144).
## Root cause 1 — `BLOCK_LENGTH` (confirmed, fix in hand)
`brotli-decompressor`'s `BROTLI_STATE_METABLOCK_DONE` handler is **missing the `meta_block_remaining_len < 0` check** that andybalholm ([decode.go#L2464-L2468](https://github.com/andybalholm/brotli/blob/v1.1.0/decode.go#L2464-L2468)) and the C reference enforce. When a metablock's insert-copy commands overshoot the declared MLEN, `meta_block_remaining_len` goes negative; the command loop breaks to `METABLOCK_DONE`, which runs cleanup and proceeds to `SUCCESS` without validating. andybalholm rejects with `BLOCK_LENGTH_2`.
Confirmed by red/green in an isolated copy of the crate: unpatched accepts **98/98** of the malformed streams; with a 5-line check added it accepts **0/98** (matching andybalholm), and all valid streams still decode:
```diff
BrotliRunningState::BROTLI_STATE_METABLOCK_DONE => {
+ if (s.meta_block_remaining_len < 0) {
+ result = BrotliDecoderErrorCode::BROTLI_DECODER_ERROR_FORMAT_BLOCK_LENGTH_2;
+ break;
+ }
s.BrotliStateCleanupAfterMetablock();
```
This is the **same class** as the PADDING_2 fix ([dropbox/rust-brotli-decompressor#44](https://github.com/dropbox/rust-brotli-decompressor/pull/44)) — a missing end-of-metablock validation in the very same state handler. It needs an **upstream fix in `brotli-decompressor`**, then a version bump here (like #21505 did for PADDING_2).
## Root cause 2 — `excessive input` (trailing bytes)
andybalholm rejects a stream with bytes remaining after the final metablock (`excessive input`); `brotli-decompressor` stops at the ISLAST metablock and returns success, ignoring the trailing bytes. **Fixable at the kona wrapper** by requiring the input reader to be fully consumed — the `decompress_brotli_strict` "reader fully consumed" check floated in #20666 (the one sound idea there). Under strict semantics this is the natural rule: a channel with trailing garbage is invalid.
## Reverse direction (for completeness)
The 5,648 "op-node accepts, kona rejects" cases are **89% truncations**: andybalholm treats a truncated stream as clean (returns partial output, nil error), while `brotli-decompressor` correctly rejects it (RFC 7932: a truncated stream is invalid). Here **andybalholm** is the lenient/non-conformant side. This direction also causes divergence (op-node ahead) and needs an op-node-side decision, since op-node is the incumbent.
## Relationship to strict decompression (protocol-team#84)
Strict semantics are the right foundation and should land regardless — they guarantee accepted channels produce identical output and delete the entire best-effort partial-output surface. But strict is **necessary, not sufficient**: it reduces the problem from "identical partial bytes on every malformed stream" (implementation-defined, ~impossible) to "agree on validity" (a spec-defined boolean) — and the decoders still disagree on that boolean. Closing it is the bounded conformance work below.
## Proposed work
- [ ] Upstream `brotli-decompressor`: add the `BLOCK_LENGTH_2` check to `METABLOCK_DONE` (patch above; sibling to #44) — PR up at dropbox/rust-brotli-decompressor#47; then bump the pin here once it releases.
- [ ] kona wrapper: require the input reader fully consumed → rejects `excessive input` — **#84-gated; do NOT land standalone.** Under today's best-effort rule op-node already keeps the valid-portion batches from an `excessive input` channel (its "excessive input" error routes through the same `NextChannel` + keep-partial path as `io.EOF`), and kona agrees (it ignores the trailing bytes and decodes the same batches). Adding this check now would make kona drop those batches (0 for the whole channel, via `decompress_brotli(...)?`) → a *new* kona-too-strict divergence. It reaches parity only once #84 makes op-node reject the whole channel too. (Same #84-gating applies to every "make the decoder stricter" step here, incl. the dropbox#47 `BLOCK_LENGTH` bump: today both clients keep partial batches and agree; tightening one side alone breaks that.)
- [x] op-node/andybalholm truncation direction — bumped op-node to andybalholm v1.2.2 (#21799, **merged**). The decoder now rejects truncated streams per RFC 7932 §9.3, matching the C reference and kona. The bump is behaviorally inert on its own (verified byte-identical batch output v1.1.0 vs v1.2.2); it provides the `io.ErrUnexpectedEOF` signal that the strict-decompression rule handling (below) acts on.
- [ ] Land protocol-team#84 (strict decompression) as the foundational rule change — includes the op-node rule handling that acts on the new `ErrUnexpectedEOF` signal to reject truncated channels, plus kona going strict.
- [ ] Gate a Go↔Rust differential *validity* fuzzer in CI (now feasible — it compares a boolean, not byte sequences).
## Methodology
Differential harness: kona's `brotli-decompressor` decode status vs `andybalholm/brotli` `io.ReadAll` over a shared corpus (corruptions of valid streams, truncations, random fuzz). Root cause confirmed by red/green against an isolated copy of `brotli-decompressor` 5.0.1. Harnesses and the 98 `BLOCK_LENGTH` reproducers are available on request.
## References
- #20666 — proposed the opposite-direction fix (refuted; the current eager decoder can't lag op-node)
- #19333 / #20598 / #20004 — original brotli divergence investigation and Bug 1/Bug 2 fixes
- #21505 — the PADDING_2 bump (brotli-decompressor 5.0.1)
- [dropbox/rust-brotli-decompressor#44](https://github.com/dropbox/rust-brotli-decompressor/pull/44) — PADDING_2 fix (same bug class)
- [protocol-team#84](https://github.com/ethereum-optimism/protocol-team/issues/84) — strict channel decompression
🤖 *Generated by Claude Opus 4.8*
Contributor guide
Research direction
Start with rust/kona/crates/protocol/protocol/src/brotli.rs and the upstream brotli-decompressor METABLOCK_DONE handler, then run the differential harness against the 98 BLOCK_LENGTH reproducers. Done requires the upstream release and Kona version bump, with wrapper strictness gated on protocol-team#84; parity should be checked through the shared validity corpus.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, rust
- Domain
- distributed-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100