lnwire: remove legacy zlib encoding for channel queries - #10980
lnwire: remove legacy zlib encoding for channel queries#10980TechLateef wants to merge 3 commits into
Conversation
🔴 PR Severity: CRITICAL
🔴 Critical (3 files)
🟠 High (1 file)
🟢 Low (4 files)
AnalysisThe highest-severity files are in No severity bump applied: only 5 non-test files and ~189 non-test lines changed (well under the 20-file/500-line thresholds), and only one critical package ( To override, add a |
saubyk
left a comment
There was a problem hiding this comment.
Please correct the file for the release note entry
0982ef4 to
7611d8f
Compare
|
/gateway review |
There was a problem hiding this comment.
Gateway review — 2 findings
🔴 0 Blocker · 🟠 1 Major · 🟡 1 Minor · 🔵 0 Nit
Summary
This PR removes the deprecated EncodingSortedZlib (type 1) gossip-query encoding, matching its removal from BOLT 7. The bulk of the change is clean deletion: the zlib encode block, the maxZlibBufSize/zlibDecodeMtx memory guards, the maxQueryChanRangeRepliesZlibFactor reply multiplier, and the corresponding test fixtures. The processChanRangeReply and encode-side removals are effectively dead-code cleanup — both key off g.cfg.encodingType, which only ever holds EncodingSortedPlain (encodingTypeToChunkSize has no other entry).
The one change that alters behavior on untrusted input is the decode path: decodeShortChanIDs now returns a hard error for type 1 instead of decompressing, and this function backs both QueryShortChanIDs.Decode and ReplyChannelRange.Decode. That behavior change, and its effect on peers that still emit zlib, is the main thing to confirm before merge. A secondary API-hygiene issue remains around the retained-but-unencodable exported constant.
Bot commands
/gateway re-review— re-run after pushing changes/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
7611d8f to
d0a3fa5
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 2 findings
🔴 0 Blocker · 🟠 1 Major · 🟡 1 Minor · 🔵 0 Nit
Summary
Since the last review the author addressed F2: encodeShortChanIDs now has an explicit case EncodingSortedZlib returning ErrZlibNotSupported instead of falling through to the misleading ErrUnknownShortChanIDEncoding, and the EncodingSortedZlib doc in encoding.go now states the constant is "retained for detection on the decode side only — it must never be used for encoding."
F1 remains unresolved. The decode side still hard-rejects a peer-controlled type-1 encoding byte with a wire-decode error. The specialist pass sharpens, but does not clear, the concern: the guard removal itself is safe within lnwire (the maxZlibBufSize limit and zlibDecodeMtx only ever protected the now-deleted inflate loop, and both were unexported so channeldb/migration could never have referenced them), so the live risk is the interop/propagation behavior against not-yet-upgraded peers, which still cannot be confirmed from the loaded context.
One genuinely-new minor finding: the new encode-side error branch has no test.
Status of prior findings
- F2 addressed: Fixed in
lnwire/query_short_chan_ids.go—encodeShortChanIDsnow has an explicitcase EncodingSortedZlibreturningErrZlibNotSupported(no longer falling through toErrUnknownShortChanIDEncoding), andlnwire/encoding.gotightens theDeprecateddoc to state the constant is "retained for detection on the decode side only — it must never be used for encoding."
Bot commands
/gateway re-review— re-run after pushing changes/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
d0a3fa5 to
7b27191
Compare
|
/gateway dismiss F1 per #10980 (comment) |
|
🚫 Dismissed F1 (major) by @saubyk — per #10980 (comment) Open findings on this PR: 🟡 F3 (minor) |
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review
No findings.
Status of prior findings
- F2 addressed: Addressed —
encodeShortChanIDscarries an explicitcase EncodingSortedZlibreturningErrZlibNotSupported(lnwire/query_short_chan_ids.go), and theDeprecated:doc comment in encoding.go now states the constant is decode/detection-only. - F3 addressed: Addressed —
TestEncodeShortChanIDsZlibRejection(lnwire/query_short_chan_ids_test.go) assertsencodeShortChanIDs(&b, EncodingSortedZlib, nil)returnsErrZlibNotSupportedviarequire.ErrorIs, covering the encode branch that was previously untested.
Bot commands
/gateway re-review— re-run after pushing changes/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
|
/gateway approve |
There was a problem hiding this comment.
✅ Approved — all prior findings addressed.
Findings recap
Findings:
F1(major) — unresolved:decodeShortChanIDsnow returnsfmt.Errorf("zlib encoding (type %d) is no longer supported", ...)for a type-1 payload instead of decodin…F2(minor) — addressed:EncodingSortedZlibremains an exported constant, but theEncodingSortedZlibcase was fully removed fromencodeShortChanIDs, so it now …F3(minor) — addressed: The newEncodingSortedZlibbranch inencodeShortChanIDsreturnsErrZlibNotSupported, but only the decode-side rejection is covered by …
Dismissed:
F1by @saubyk — per #10980 (comment)
Approved by @saubyk via /gateway approve. Last reviewed at 7b27191. Skill v0.3.0, model claude-opus-4-8.
Abdulkbk
left a comment
There was a problem hiding this comment.
First pass with a few feedback items:
- Consider splitting into logical commits, e.g.,
lnwire: remove legacy zlib encoding for channel queries,discovery: drop zlib reply chunking factor, anddocs: add release note for zlib removal. - Ensure the linter is happy (cause is here).
- Look at how release note entries are added. For example, the author is missing from the release note, and some unrelated formatting was added.
5907b81 to
cc1432a
Compare
Thanks for the review! |
ziggie1984
left a comment
There was a problem hiding this comment.
Went through each commit and traced the remaining zlib paths — the removal itself is correct and safe. Recording the verification here since none of it is in the PR description, then some cleanup nits below.
Why this is safe (worth folding into the PR/commit bodies)
- lnd never sent zlib: all three construction sites are plain (
discovery/syncer.go:888hardcoded,:1210/:1283viacfg.encodingType, whichsync_manager.go:663pins toEncodingSortedPlain). The new error inencodeShortChanIDsis therefore unreachable in production. - lnd never echoes a peer's encoding type back into replies —
replyChanRangeQueryusesg.cfg.encodingType, notquery.EncodingType, andpeer/brontide.go:2826only logs it. If we did echo it, a zlib query would have made our ownEncodefail. - Nobody on the network sends type 1: eclair removed sending in ACINQ/eclair#2244 and decoding in ACINQ/eclair#3263; lnd was the last decoder standing (per #10694).
- The
timestampsTLV already rejected everything but plain (lnwire/timestamps.go:92), and lnd doesn't implementquery_flags, so no otherencoding_typebyte is left behind. - The empty-list case is a non-issue: eclair's codec always encoded empty lists as uncompressed "for compatibility with other implementations", and lnd never sent zlib, so nothing on the wire produces an empty type-1 body. The removed
len(queryBody) == 0tolerance isn't needed. - The chunking-factor removal in 8f01ba8 is a semantic no-op: that switch is on
g.cfg.encodingType(ours, always plain), never the reply's encoding, so the zlib arm was already dead code. - Security win beyond the decompression bomb: this also deletes
zlibDecodeMtx, a package-global mutex that serialized zlib decodes across all peers. - No new DoS surface. A peer could already force a fatal decode with encoding type 2, an unsorted SCID list, or a bad body length; type 1 just joins that existing class, which is already rate-limited by
nextPeerBackoff(server.go:4069, decaying toMaxBackoff). .golangci.yml:195disables-SA1019, so the newDeprecated:marker won't trip lint on the internal references to the constant.- Verified both code commits build standalone and that
./lnwire/...and./discovery/...pass at each.
Cleanup items
Dead fuzz targets left behind. lnwire/fuzz_test.go:315 FuzzZlibQueryShortChanIDs and :349 FuzzZlibReplyChannelRange both hardcode the encoding byte to \x01, so ReadMessage now always errors and the harness returns early at fuzz_test.go:45 — every assertion after that is unreachable. I ran one for 3s: 131k execs, and the only coverage it finds is stdlib zlib compression inside the harness itself. Worth deleting both, along with the then-unused compress/zlib import.
Commit messages. Body lines run 130 and 149 chars in 9413a7f and 154 in 8f01ba8; the commit guidelines ask for 72. Both would also read better as prose than as bullet lists. More substantively, neither commit nor the PR body states why the removal is safe, or that receiving type 1 now disconnects the peer — that's a reviewer's first question, and the points above would cover it.
Inline comments follow. None of them are correctness problems.
8c243b3 to
3ba646b
Compare
Thanks for the review! I've addressed all the feedback in the latest push: Updated the release notes with the PR link and the operator context. Cleaned up the two stale comments regarding the encoding types. |
68cb210 to
d5f55de
Compare
|
@saubyk: review reminder |
d5f55de to
667cab5
Compare
This commit removes support for the deprecated EncodingSortedZlib format (type 1) which was dropped from the BOLT 7 specification. All zlib compression and decompression logic has been removed from lnwire messages. The EncodingSortedZlib constant is now marked as Deprecated to retain protocol documentation. The decode path now explicitly returns an error when a peer attempts to send zlib-encoded data, rather than a generic unknown encoding error. All tests that previously verified zlib scenarios have been cleaned up or deleted. This removal is safe because lnd has never sent zlib-encoded data, and nobody on the network sends type 1. Additionally, dropping this encoding removes the package-global zlibDecodeMtx, improving performance and security without introducing new DoS vectors, as receiving type 1 now simply triggers an instant disconnect.
This commit removes zlib-specific chunking logic from the discovery syncer. Since LND no longer supports zlib compression for gossip queries, the corresponding chunking factors and parsing cases in the syncer are no longer needed.
667cab5 to
eccbb81
Compare
Change Description
This PR removes support for the deprecated
EncodingSortedZlibformat (type 1) which was officially dropped from the BOLT 7 specification in lightning/bolts#981.Changes:
lnwiremessages.EncodingSortedZlibconstant asDeprecatedto retain protocol documentation.maxQueryChanRangeRepliesZlibFactor) from thediscoverysyncer.message_test.go,reply_channel_range_test.go,query_short_chan_ids_test.go) that previously verified zlib scenarios.channeldb/migrationfiles untouched to ensure historical snapshots remain intact.Fixes #10694
Steps to Test
make unitto ensure alllnwireanddiscoverytests pass.EncodingSortedZlib.Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.