Repository navigation
multi: count forwarding packages without loading them - #11327
Conversation
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟠 High (3 files)
🟡 Medium (1 file)
🟢 Low (3 files)
AnalysisThis PR makes forwarding package counts optional in an RPC response, which requires a change to To override, add a |
|
Why do we have |
|
To clarify: only The two fields cannot be combined because they travel in opposite directions: |
|
Just tested and now I'm getting which is much better than what is reported in #11289 (comment) |
|
So this PR only fixes part 1 of #11289 (comment) and not part 2? |
Yes I am hesitant to fix part 2 with a patch because it will be fixed with the SQL implemenation once it is deployed. Let me know how your testing goes and whether we need to improve some things still to give you a temporary working version for your test-setup. |
| return nil, 0, err | ||
| } | ||
|
|
||
| numFwdPkgs := int64(len(fwdPkgs)) |
There was a problem hiding this comment.
nit: numFwdPkgs is a single-use intermediate variable here since channel is already declared above; we can just assign directly.
channel.NumForwardingPackages = int64(len(fwdPkgs))
TechLateef
left a comment
There was a problem hiding this comment.
LGTM! Tested and verified that tests and proto generation pass locally. Left one minor nit inline.
Also, don't forget to add your name to the docs/release-notes/release-notes-0.22.0.md
|
/gateway review |
There was a problem hiding this comment.
Gateway review — 2 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 2 Minor · 🔵 0 Nit
Summary
This PR stops the PendingChannels RPC from loading forwarding packages for waiting-close and pending-force-close channels unless the caller sets the new include_num_forwarding_packages field. It is a reasonable mitigation for the SQL-kvdb latency reported in #11289, and the requester's timing confirms the improvement. The server-side change is mechanical. When the flag is set, the old LoadFwdPkgs call and its error path are kept exactly. When it is not set, the field is left at its zero value. Release notes and the lncli flag are in place.
There are two issues. First, num_forwarding_packages = 0 now means both "the channel has no forwarding packages" and "the count was not requested", and the response gives callers no way to tell which. Second, the new default-off assertion only covers waiting-close channels. The test harness now always sets the flag, so nothing checks the default path for pending-force-close channels.
Findings (could not anchor inline)
🟡 F1 (Minor) — Zero forwarding packages now also means not requested · lnrpc/lightning.proto:773
Existing callers that don't set the new flag now get num_forwarding_packages = 0 for every pending channel. That is the same value as a real count of zero, so a tool that reads 0 as "forwarding packages have been wiped" now gets a false positive with no error. lnd's own lnd_wipe_fwdpkgs_test reasons this way, and it keeps working only because the harness now always sets the flag.
The response field is a plain proto3 scalar, so it has no field presence. Updating the doc comment and adding a release-notes entry does not reach automated consumers: they keep compiling and quietly read a different meaning. If the ambiguity matters, the field could carry presence. One option is to declare it optional int64 num_forwarding_packages = 10;, which is wire-compatible but changes the Go field to *int64, a compile-time break for Go clients. Another is to add a sibling bool that says whether the count was populated. Either way, REST/JSON would omit the value instead of returning "0". If the team decides the count is diagnostic-only and the release note is enough, this can be left as is. The trade-off should be a deliberate choice.
Lower-severity findings (1)
🟡 F2 (Minor) — Default-off path untested for pending force-close channels · itest/lnd_wipe_fwdpkgs_test.go:63
The new assertion only covers the default-off branch of fetchWaitingCloseChannels. Nothing tests the new if includeNumForwardingPackages guard in fetchPendingForceCloseChannels. Because lntest/rpc/lnd.go now sets IncludeNumForwardingPackages: true on every harness call, no itest calls PendingForceClosingChannels without the flag. A matching PendingChannelsRequest{} check after the channel moves to pending force close would cover it.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/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. |
There was a problem hiding this comment.
Nice, I think this strategy makes sense, but I think a cheaper count can keep the response unchanged and make the new flag unnecessary, so perhaps it's worth changing the approach. Details are in the inline comment.
Edit: the request is non-blocking, we can also go on with your proposal, just let me know what you think of this
| "packages for channel:%s, %v", | ||
| historical.ShortChannelID, err) | ||
| return nil, 0, err | ||
| if includeNumForwardingPackages { |
There was a problem hiding this comment.
The flag exists only to avoid the cost of LoadFwdPkgs, but the RPC needs only the count, which is the number of package heights in the channel's bucket. A count-only read keeps num_forwarding_packages populated, so existing clients do not silently get 0 and no breaking change is necessary. What do you think about adding a CountFwdPkgs method to the forwarding package store and the chanstate interface that reads only the heights with kvdb.ForAll, use it at both call sites, and drop the request flag? In a store-level benchmark with 500 packages of 50 adds per channel, this goes from 244 ms to 263 µs on sqlite, and from 30003 queries to 3. The motivation behind this is that I'd like to avoid adding external knobs that make lnd more complicated.
505ed9c to
6f58a2a
Compare
6f58a2a to
ad12d2c
Compare
|
@bitromortac updated to your new design proposal |
yyforyongyu
left a comment
There was a problem hiding this comment.
Can we address the two count/error-handling cases below before merging?
| } | ||
|
|
||
| var count int | ||
| err := kvdb.ForAll(sourceBkt, func(key, value []byte) error { |
There was a problem hiding this comment.
Can we return rows.Err() from SQL ForAll before using it here? It currently returns nil when rows.Next() stops on an error. I reproduced this through CountFwdPkgs on SQLite: with 3 valid packages, a read failure after 0 or 1 rows returns 0, nil or 1, nil. That gives PendingChannels a successful but incorrect count. Can we add an interrupted-iteration regression test too?
| // Each direct child is a forwarding package bucket identified | ||
| // by its eight-byte commitment height. Reject malformed leaf | ||
| // entries instead of counting them as packages. | ||
| if len(key) != 8 || value != nil { |
There was a problem hiding this comment.
Can we preserve the SQL NULL/empty-BLOB distinction in ForAll and add an empty-leaf case on SQLite? The iterator exposes an empty BLOB as nil, so this check treats it as a bucket. With 1 valid package plus an empty leaf, CountFwdPkgs returns 2, nil, while LoadFwdPkgs returns ErrCorruptedFwdPkg. This misses the stated guarantee that stray leaf values are rejected.
Add a count-only forwarding package store method that iterates package height buckets without materializing their HTLC contents. Use the new method for waiting-close and pending-force-close channels so PendingChannels keeps populating the existing response field without a new request flag. Make SQL ForAll preserve empty leaf values and propagate row iteration errors, with regression coverage for both cases. Add forwarding package correctness coverage and a PostgreSQL benchmark for the count-only path.
ad12d2c to
63a17fc
Compare
63a17fc to
62467bb
Compare
OK, so looks like your later force pushes removed |
|
Please rebase so we can get e69112e . |
|
I'm now running 62467bb . Will report back on Monday if it is still stable like the previous version was I was testing earlier in the week. |
bitromortac
left a comment
There was a problem hiding this comment.
Thanks for the updates! LGTM 🎉 I left one non-blocking nit on the commit layout.
| rows, cancel, err := b.tx.Query( | ||
| "SELECT key, value FROM " + b.table + " WHERE " + | ||
| parentSelector(b.id) + " ORDER BY key", | ||
| "SELECT key, value, " + |
There was a problem hiding this comment.
nit: The multi: commit mixes a ForAll bug fix in the kvdb module with the new store method and the RPC change. kvdb is its own Go module and other stores share ForAll, so a separate commit is easier to review, bisect and backport. A better split would be three commits that each build: kvdb/sqlbase: for the ForAll fix with its sqlite tests, chanstate: for CountFwdPkgs with its test, the interface, channeldb and the benchmark, and rpcserver: for the PendingChannels change (non-blocking)
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-11327-to-v0.20.x-branch
git worktree add --checkout .worktree/backport-11327-to-v0.20.x-branch backport-11327-to-v0.20.x-branch
cd .worktree/backport-11327-to-v0.20.x-branch
git reset --hard HEAD^
git cherry-pick -x 65b2a54b6a59b009ef056b256b3497898dcb2249 62467bb0cf3788bd6436f8e2038e25216c497000
git push --force-with-lease |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-11327-to-v0.21.x-branch
git worktree add --checkout .worktree/backport-11327-to-v0.21.x-branch backport-11327-to-v0.21.x-branch
cd .worktree/backport-11327-to-v0.21.x-branch
git reset --hard HEAD^
git cherry-pick -x 65b2a54b6a59b009ef056b256b3497898dcb2249 62467bb0cf3788bd6436f8e2038e25216c497000
git push --force-with-lease |
…20.x-branch [v0.20.x-branch] Backport #11327: multi: count forwarding packages without loading them
…21.x-branch [v0.21.x-branch] Backport #11327: multi: count forwarding packages without loading them
The bug fix entry of lightningnetwork#11327 was folded from a fragment without a link to its pull request. Link it, like every other entry in the release notes.
The bug fix entry of lightningnetwork#11327 was folded from a fragment without a link to its pull request. Link it, like every other entry in the release notes.
Change
CountFwdPkgsmethod to the forwarding package store(
FwdPackager,OpenChannelFwdPkgStore,ChannelStateDB,OpenChannel)that iterates the per-channel height buckets via
kvdb.ForAllwithoutloading package contents.
PendingChannelsfor waiting-close and pending-force-closechannels to populate
num_forwarding_packages.ForAllimplementation so it returns row-iteration errorsand preserves the distinction between nested buckets (
NULL) and emptyleaf values.
No RPC or CLI changes:
num_forwarding_packagesis still always populated.Motivation
PendingChannelspreviously materialized every forwarding package, includingall of its HTLCs, only to return
len(fwdPkgs). On the SQL-backed KV storethis produced a large number of database round trips per channel. Counting the
immediate height buckets with
ForAllis a single query per channel.The SQL iterator must not turn a read interrupted after a subset of rows into
a successful partial count. It must also keep an empty SQLite BLOB distinct
from the SQL
NULLused to represent a nested bucket, otherwise a malformedempty leaf can be counted as a forwarding package.
Counting is more lenient than loading package contents: a height bucket with
missing sub-buckets or filters is still counted. Malformed keys and stray leaf
values in the channel's source bucket return
ErrCorruptedFwdPkg.Testing
TestCountFwdPkgscovers add, remove, wipe, non-empty corrupted leaves andempty corrupted leaves.
iteration.
BenchmarkPostgresFwdPkgCount(-tags kvdb_postgres) comparesLoadFwdPkgsagainstCountFwdPkgs.itest/lnd_wipe_fwdpkgs_test.goexercisesnum_forwarding_packagesend to end.