discovery: bound channel range reply memory usage - #10992
Conversation
65e517e to
551bdec
Compare
|
/gateway review |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gateway review — 4 findings
🔴 0 Blocker · 🟠 2 Major · 🟡 2 Minor · 🔵 0 Nit
Summary
This PR replaces a coarse 67MB zlib byte-cap with a precise 100k short-channel-ID count cap, adds an aggregate per-query SCID budget, resets accumulated reply state on any error, and counts the reply budget from each reply's own encoding. The intent — making the channel-range-sync working set predictable against a hostile peer — is sound, and the memory bound itself is now correctly enforced (buffered ≤ received ≤ maxChanRangeReplySCIDs), with good test coverage.
Two concerns should be resolved before merge. First, processChanRangeReply indexes msg.Timestamps[i] over msg.ShortChanIDs without establishing that the two slices are the same length; if the lnwire.ReplyChannelRange decoder does not enforce parity, a crafted reply panics the gossip goroutine — a DoS on the very path this PR hardens. Second, the zlib decoder now rejects io.ErrUnexpectedEOF, which the code being replaced (last touched deliberately in PR #4391) accepted as a clean terminator; this needs confirmation that no conforming peer relies on the old behavior. Two smaller issues round out the review.
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)
| g.numChanRangeReplySCIDsRcvd += numReplySCIDs | ||
| g.prevReplyChannelRange = msg | ||
|
|
||
| for i, scid := range msg.ShortChanIDs { |
There was a problem hiding this comment.
🟠 F1 (Major) — Mismatched Timestamps length can panic the gossip syncer
A remote peer can crash the gossip-syncer goroutine with a ReplyChannelRange whose Timestamps field is non-empty but shorter than ShortChanIDs: the buffer loop does msg.Timestamps[i] for i ranging over msg.ShortChanIDs whenever len(msg.Timestamps) != 0, with no check that the two lengths match. That is an index-out-of-range panic driven entirely by untrusted peer input.
Why this matters
msg is peer-supplied and Timestamps is a length-delimited wire field decoded independently of ShortChanIDs, so the parity len(msg.Timestamps) == len(msg.ShortChanIDs) must be guaranteed by lnwire.ReplyChannelRange.Decode — which is not in the provided context, so I cannot confirm it. If the decoder does not reject the mismatch, this is a blocker (reachable panic on a hostile path); it is reported as major pending that confirmation. The loop predates this PR, but the enclosing block is being relocated here and this PR is specifically about bounding hostile channel-range input, so it is the right place to close it: either add if len(msg.Timestamps) != 0 && len(msg.Timestamps) != len(msg.ShortChanIDs) { return fmt.Errorf(...) } before the loop, or confirm the decoder enforces parity.
There was a problem hiding this comment.
This is already checked during wire parsing and in several other layers.
| var cid ShortChannelID | ||
| err := ReadElements(r, &cid) | ||
|
|
||
| switch { |
There was a problem hiding this comment.
🟠 F2 (Major) — Zlib decoder now rejects io.ErrUnexpectedEOF that #4391 accepted
decodeCompressedShortChanIDs treats only io.EOF as clean stream termination, whereas the loop it replaces treated both io.EOF and io.ErrUnexpectedEOF as clean and returned the SCIDs read so far. Any zlib reply whose final ReadElements returns io.ErrUnexpectedEOF is now rejected outright and terminates the range sync, where it was previously accepted.
Why this matters
Blame shows the EOF/ErrUnexpectedEOF handling was last written deliberately by matheusd in PR #4391 (2020), on top of Roasbeef's original #1399 (2018) — precisely the branch this diff drops. There is a strong argument this is correct hardening rather than a regression: a BOLT-conforming zlib stream decodes to a whole number of 8-byte SCIDs, and the old ErrUnexpectedEOF acceptance was tied to the now-removed io.LimitedReader, which could truncate a valid stream mid-element at maxZlibBufSize. With the LimitedReader gone, ErrUnexpectedEOF should only arise from genuinely malformed/truncated input, which is correct to reject. The remaining question is empirical: confirm no real-world peer implementation emits a stream that decodes with a trailing partial element under normal conditions before landing. Reach is low (lnwire decode paths plus processChanRangeReply), so the blast radius is a per-peer sync break, not a local crash. Restore the io.ErrUnexpectedEOF case if the historical behavior turns out to be load-bearing.
| } | ||
|
|
||
| var replyCount uint32 | ||
| switch msg.EncodingType { |
There was a problem hiding this comment.
🟡 F3 (Minor) — Reply-message budget keyed on peer-supplied encoding type
The reply-count weight now derives from msg.EncodingType (peer-controlled) instead of g.cfg.encodingType (what we requested). A peer answering with EncodingSortedPlain gets replyCount = 1 and can send up to maxReplies (500) ReplyChannelRange messages, versus the ~125 that the maxQueryChanRangeRepliesZlibFactor = 4 weighting enforced when zlib was in play — a ~4x amplification of accepted messages and their per-message validation/buffering passes. Total buffered memory is still hard-bounded by the independent maxChanRangeReplySCIDs = 100_000 cap, so this is message-count amplification, not a memory blow-up. Consider keying replyCount off the requested encoding, or counting the reply budget in SCIDs rather than messages.
| // compute the number of bytes encoded based on the size of the | ||
| // query body. | ||
| numShortChanIDs := len(queryBody) / 8 | ||
| if numShortChanIDs > maxDecodedShortChanIDs { |
There was a problem hiding this comment.
🟡 F4 (Minor) — maxDecodedShortChanIDs comment and plain-path guard mislead
The maxDecodedShortChanIDs comment describes it as the max IDs "decoded from a single compressed message" that "bounds decompression," but the guard is also applied to the uncompressed EncodingSortedPlain branch, and on that branch it is effectively unreachable: numShortChanIDs = len(queryBody)/8 is bounded by the ~64KB wire message to roughly 8191, far below the 100_000 cap. The real cross-message plain-encoding bound lives in syncer.go via numChanRangeReplySCIDsRcvd. Reword the comment to cover both encodings and note that the per-message plain guard is defense-in-depth, so a reader doesn't mistake it for where the limit actually bites.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
551bdec to
ce56fef
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 3 findings
🔴 0 Blocker · 🟠 2 Major · 🟡 1 Minor · 🔵 0 Nit
Summary
This PR bounds the working set during channel-range sync, and the memory-bounding logic itself is sound: the new per-message and per-query SCID caps are enforced incrementally, the error-path resetChanRangeReplyState() correctly releases accumulated state, and the aggregate-count check is free of integer underflow. One prior finding (F4) was cleanly addressed by rewording the maxDecodedShortChanIDs comment.
Three prior findings remain unresolved. The most important is F1: the peer-controlled msg.Timestamps[i] access in the buffering loop is still guarded only by a non-empty check, not a length-parity check — three independent specialists flagged this as a remote-reachable panic. The maintainer's inline reply states parity is already enforced during wire parsing; that is plausible but cannot be confirmed from the review context (the decoder is not in scope and the repo is not checked out here), so the concern stands pending an explicit guard or a decoder-invariant citation. F2 (dropping io.ErrUnexpectedEOF acceptance) and F3 (reply budget keyed off peer-supplied encoding) are also still present; F2 now appears intentional given the new corrupt-zlib test, though the interop question it raises is unanswered.
No new commits introduced new defects: the removed io.LimitedReader byte-bound does not re-open a zlib-bomb vector because the 100k count cap short-circuits the decode loop per iteration.
Status of prior findings
- F4 addressed: Fixed in
lnwire/query_short_chan_ids.go:14— themaxDecodedShortChanIDscomment was rewritten to "the maximum number of short channel IDs accepted from a single message. The plain encoding is also bounded by the wire size, so its check is defense in depth," which now covers both encodings and explicitly labels the plain-branch guard as defense-in-depth.
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 dismiss F1 |
|
/gateway dismiss F2 |
|
/gateway dismiss F3 |
|
🚫 Dismissed F1 (major) by @Roasbeef — no reason given Open findings on this PR: 🟠 F2 (major) · 🟡 F3 (minor) |
|
🚫 Dismissed F2 (major) by @Roasbeef — no reason given Open findings on this PR: 🟡 F3 (minor) |
|
🚫 Dismissed F3 (minor) by @Roasbeef — no reason given All findings on this PR are addressed or dismissed. |
There was a problem hiding this comment.
The aggregate SCID bound and cleanup are solid, but I think two correctness/lifecycle issues should block merging:
- A terminal channel-range processing error clears the accumulated fields but does not establish a terminal or recoverable lifecycle outcome for the syncer.
- Exhausting the local reply budget before receiving the protocol-final reply currently falls through to processing a partial response as successful completion.
Please also cover the first issue with a lifecycle-level test through the public message path. I left inline comments at the relevant locations.
Non-blocking design notes: the fixed 100k cap is an effective immediate guard but also a future graph-size compatibility boundary, so bounded range queries/incremental processing would be preferable long term. The master-versus-backport policy for zlib should be decided explicitly, and more granular commits would make that policy split and backporting easier.
Linter is failing currently as well
| numReplySCIDs > maxChanRangeReplySCIDs- | ||
| g.numChanRangeReplySCIDsRcvd { | ||
|
|
||
| return fmt.Errorf("channel range reply exceeds maximum "+ |
There was a problem hiding this comment.
Must fix: returning this error clears the range-reply fields, but the state-machine handler exits without an explicit lifecycle transition for the syncer. Please make terminal errors retire/cancel the syncer, or transition it to a defined recoverable state, so its externally visible state remains consistent with its active handlers.
There was a problem hiding this comment.
So this function already returns errors that would effectively tear down the state machine on any unexpected behavior. So recover or hard bail here would be new behavior, expanding the scope of this PR a bit. One thin gto watch out for is entering a loop with a bad peer.
| maxChanRangeReplySCIDs) | ||
| } | ||
|
|
||
| g.numChanRangeRepliesRcvd += replyCount |
There was a problem hiding this comment.
Must fix: maxReplies is a local safety budget, not evidence that the protocol response completed. When this counter reaches the budget before the final reply, the later completion logic falls through and processes the partial result as success. Please track protocol completion separately and return an incomplete-response error when the local budget is exhausted first.
There was a problem hiding this comment.
return an incomplete-response error when the local budget is exhausted first.
As in send a warning to the peer?
We have an existing test that exercises the scenario I think you're describing: TestGossipSyncerMaxChannelRangeReplies.
| reply.ShortChanIDs = []lnwire.ShortChannelID{ | ||
| lnwire.NewShortChanIDFromInt(uint64(len(scids))), | ||
| } | ||
| err = syncer.processChanRangeReply(ctx, reply) |
There was a problem hiding this comment.
This verifies function-level cleanup, but it bypasses the syncer lifecycle. Please add a regression that starts the state-machine handler, delivers the failing reply through ProcessQueryMsg, and asserts the chosen terminal or recovery outcome. That will cover the behavior the direct call cannot observe.
ziggie1984
added
backport-v0.20.x-branch
What do you mean by this? The shift here is basically lazy decode for zlib. With normal encoding you can only encode 8186 scids. Also the querier is meant to chunk their queries anyway. We are missing a machine readable error code there though. |
| // Any error terminates the range sync. Release the accumulated reply | ||
| // state so the peer cannot pin it after forcing an error. | ||
| defer func() { | ||
| if err != nil { | ||
| g.resetChanRangeReplyState() | ||
| } | ||
| }() |
There was a problem hiding this comment.
I'm not sure this is could also hurt normal syncers, as I think the !isLegacyReplyChannelRange checks are not 100% spec compliant (especially, what happens if somebody sends us block-gapped replies?). Wanted just to avoid to not be able to sync anymore.
There was a problem hiding this comment.
block-gapped replies
Can you expand on this?
The reset here is intended to stop a syncer that hit an error from having us keep around anything that we've buffered.
As mentioned above, right now (before this PR), we don't actually recover from any syncer failures, resulting in gossip syncing silently halting for that peer if we encounter an error. The state will then sit there. Only after we go to disconnect will the syncer state be freed up.
There was a problem hiding this comment.
Ah great, I haven't realized that it was aborting the goroutine already before this change. So this is preexisting and could be addressed in a separate PR.
The checks concerning the block heights are stricter than the spec demands:
successive reply_channel_range message:
MUST have first_blocknum equal or greater than the previous first_blocknum.
Which doesn't say anything about the previous reply message's last block height, so it would be allowed by the spec to have gaps in the previous last and this message's first block height.
ce56fef to
20e1c5a
Compare
In this commit, we cap each decompressed short channel ID set at 100,000 entries, matching the aggregate range reply budget. The old zlib reader bounded compressed input rather than decoded output, so the two working-set limits could drift apart. We retain compatibility with protocol-valid compressed replies, reject truncated or corrupt zlib streams, and close the reader on every exit. Boundary, compatibility, corruption, and property tests cover the decoder.
20e1c5a to
e4c1b74
Compare
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
LGTM 🎉 (the nil check could be good to have)
| // If we're not communicating with a legacy node, we'll apply some | ||
| // further constraints on their reply to ensure it satisfies our query. | ||
| if !isLegacyReplyChannelRange(g.curQueryRangeMsg, msg) { |
There was a problem hiding this comment.
We could do a defensive nil check for g.curQueryRangeMsg, just to make sure?
| g.numChanRangeRepliesRcvd += replyCount | ||
| g.numChanRangeReplySCIDsRcvd += numReplySCIDs | ||
| g.prevReplyChannelRange = msg | ||
|
|
There was a problem hiding this comment.
At this point we could use slices.Grow to pre-allocate the nil g.bufferedChanRangeReplies slice with the number of scids to add, which would make it a bit more efficient.
There was a problem hiding this comment.
Good idea, will tack this on
| // Any error terminates the range sync. Release the accumulated reply | ||
| // state so the peer cannot pin it after forcing an error. | ||
| defer func() { | ||
| if err != nil { | ||
| g.resetChanRangeReplyState() | ||
| } | ||
| }() |
There was a problem hiding this comment.
Ah great, I haven't realized that it was aborting the goroutine already before this change. So this is preexisting and could be addressed in a separate PR.
The checks concerning the block heights are stricter than the spec demands:
successive reply_channel_range message:
MUST have first_blocknum equal or greater than the previous first_blocknum.
Which doesn't say anything about the previous reply message's last block height, so it would be allowed by the spec to have gaps in the previous last and this message's first block height.
In this commit, we cap each QueryChannelRange response at 100,000 SCIDs across all streamed replies. The existing reply-count limit did not track the aggregate decoded working set, so memory use varied with the encoding and composition of the reply stream. We count raw SCIDs before timestamp filtering, charge replies using the received encoding type, and release all accumulated range state on any error. This bounds both memory and CPU work while still leaving headroom above the current graph.
e4c1b74 to
ceff94f
Compare
4 participants
In this PR, we bound the memory used while decoding and buffering short
channel IDs during channel range synchronization. Compressed replies can
carry more IDs than plain replies, and a query can be answered across many
messages, so the temporary working set can grow well beyond the graph data
we actually need to process.
We now cap decoded ID sets and the aggregate IDs accepted for a single range
query. We also count reply budgets from the encoding carried by each reply,
clear accumulated range state on errors, and reject incomplete compressed
streams. The limits leave headroom above the current graph while keeping the
working set predictable.
See each commit message for a detailed description w.r.t. the incremental
changes.
Testing
go test -count=1 ./lnwire ./discoverygo test -race -count=1 ./lnwire ./discoveryQueryShortChanIDsandReplyChannelRange