[ZZiigguurraatt]

cleanStore prunes the switch's network result store at startup, keeping only the results of still in-flight attempts. It iterated the bucket with ForEach and deleted each stale key with its own Delete. On the native SQL kvdb backend every ForEach step and every Delete is a separate database round-trip (ForEach issues one SELECT ... LIMIT 1 per row; Delete issues a SELECT bucket-check plus a DELETE), so pruning a store with N entries costs on the order of N sequential round-trips. The store is only pruned here at startup, so it grows with payment and forwarding volume during uptime; on a busy node against a networked Postgres this made startup spend hours doing round-trip-bound work. The whole prune also runs in a single transaction, so a node restarted before it finished rolled the deletes back and made no progress, leaving the store oversized for the next startup to repeat.

Since the keep set is small, read the keepers with one streaming query via kvdb.ForAll, drop the whole bucket in one cascading DeleteTopLevelBucket, recreate it, and re-insert the keepers. This turns the O(N) read-and-delete loop into one read, one drop, one recreate, and O(keep) inserts, independent of how many stale entries have accumulated, so the prune commits in well under a second regardless of store size. On bbolt kvdb.ForAll falls back to ForEach and the drop/recreate are ordinary operations, so behavior there is unchanged.

Fixes #11353.

[github-actions]

🔴 PR Severity: CRITICAL

gh pr view | 1 file | 66 lines changed

🔴 Critical (1 file)

• htlcswitch/payment_result.go - HTLC forwarding/payment routing state machine package; this PR changes how the switch's network result store is pruned at startup (bucket drop/recreate vs per-row delete), touching persistence logic for in-flight payment attempt results.

Analysis

The only changed file, htlcswitch/payment_result.go, falls under htlcswitch/*, which handles HTLC forwarding and payment routing state — a critical-path package per the classification rules. The change itself replaces a per-row ForEach/Delete pruning loop with a bucket drop/recreate plus re-insert of keepers, which is a meaningful change to how in-flight payment attempt state is persisted and recovered across restarts (including transaction/rollback semantics). This warrants expert review given the correctness implications for payment state durability, even though the diff is small (53 additions, 13 deletions).

---

_{To override, add a severity-override-{critical,high,medium,low} label.}

[ziggie1984]

The implementation approach and single-commit scope look good. Before merge, I would only suggest:

• Add a table-driven cleanStore test using kvdb.GetTestBackend, covering a partial keep set, an empty keep set, and keeping all entries. The current TestNetworkResultStore uses channeldb.OpenForTesting, which always opens bbolt, so it does not exercise this path on PostgreSQL or SQLite.
• Add the required release-note entry, resolve the lint failure, and let the full CI matrix complete.
• Tighten the description: this store grows with locally initiated HTLC attempt volume, not ordinary forwarded HTLC traffic. Also describe the improvement as removing O(N) client/server round-trips; PostgreSQL still streams and deletes O(N) rows internally.

The implementation itself and the one-commit PR structure can otherwise remain unchanged.

[ziggie1984]

Suggested change

	key := make([]byte, len(k))
	copy(key, k)
	value := make([]byte, len(v))
	copy(value, v)
	kept = append(kept, keptResult{key: key, value: value})
	kept = append(kept, keptResult{
	key: bytes.Clone(k),
	value: bytes.Clone(v),
	})

[ZZiigguurraatt]

commit amended but I have not yet got this backported into my environment with the node that was struggling to test.

[ZZiigguurraatt]

fixed a new issue in the test that the linter didn't like

[ZZiigguurraatt]

/gateway review

[lightninglabs-gateway]

Sorry, /gateway review requires maintainer permission.

[saubyk]

/gateway review

[lightninglabs-gateway]

Gateway review — 5 findings

🔴 0 Blocker · 🟠 0 Major · 🟡 4 Minor · 🔵 1 Nit

Summary

This PR replaces the per-key ForEach/Delete prune in cleanStore with three steps: one kvdb.ForAll read that snapshots the entries to keep, one DeleteTopLevelBucket, then a recreate and re-insert of the kept entries. On the SQL backends this removes the O(N) client/server round-trips that stalled startup. The rewrite is correct. Kept keys and values are cloned before the bucket is dropped, the removed == 0 early return skips needless churn, errors still propagate, and everything stays in one transaction, so a failure rolls back cleanly. The new table-driven test runs the SQL path when built with the postgres or sqlite tag.

What remains is minor. The release note promises a speed it doesn't measure. The test can't detect a bad value copy. There is no empty-store case. The decode at startup still panics on a malformed key, as it did before this PR.

Lower-severity findings (5)

🟡 F1 (Minor) — Release note overstates the speedup · docs/release-notes/unreleased/11408.md:11

Nothing measures "commits in well under a second", and the reviewer specifically asked for this to be framed as removing O(N) client/server round-trips, since Postgres still scans and cascade-deletes O(N) rows internally. End the sentence at "…instead of a round-trip per entry" and drop the timing claim.

🟡 F2 (Minor) — Malformed key panics node at startup · htlcswitch/payment_result.go:287

binary.BigEndian.Uint64(k) panics on any key shorter than 8 bytes, so one corrupt key in network-result-store-bucket would crash startup instead of returning an error. The removed ForEach code had the same unchecked decode, so this is hardening, not a regression. Checking len(k) != 8 and returning a wrapped error would let startup fail cleanly.

🟡 F3 (Minor) — No test case for an empty store · htlcswitch/payment_result_test.go:234

The table never calls cleanStore on an empty bucket, which is the first-startup path through the removed == 0 early return at payment_result.go:307. Add a case that stores nothing, calls cleanStore with a non-empty keep set, and asserts that it returns no error.

🟡 F4 (Minor) — Identical payloads can't catch value-copy bugs · htlcswitch/payment_result_test.go:252

All five stored results are the same &lnwire.UpdateAddHTLC{}, and the kept-ID check only asserts NoError. If the bytes.Clone(v) at payment_result.go:297 regressed and a backend reused its buffer, every kept entry would still decode and the test would pass. Store a distinct payload per ID (for example, ID: i) and assert that each kept result matches what was stored.

🔵 F5 (Nit) — Small-keep-set assumption is not checked · htlcswitch/payment_result.go:307

The drop-and-reinsert path relies on "the keep set is small" (comment at lines 270–277), but only removed == 0 is special-cased. When few entries are stale and many are kept, it does more work than a per-key delete would. Either state the assumption as a precondition, or fall back to per-key Delete when removed < len(kept).

Reviewed at medium effort.

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)

[lightninglabs-gateway]

🤖 gateway audit metadata for this PR — auto-generated, please don't edit.

[saubyk]

Two comments from my end

[saubyk]

Was the hours-long stall actually observed on SQLite, or is that inferred from the shared code path? SQLite round-trips are in-process, so the note may be overstating who is affected.

Also, should this read "per HTLC attempt" rather than "per payment"? MPP payments add several entries each, which matches the code comment and the correction in the thread above.

[ZZiigguurraatt]

Was the hours-long stall actually observed on SQLite, or is that inferred from the shared code path? SQLite round-trips are in-process, so the note may be overstating who is affected.

Did not investigate yet on a comparable node.

[ZZiigguurraatt]

Also, should this read "per HTLC attempt" rather than "per payment"? MPP payments add several entries each, which matches the code comment and the correction in the thread above.

Not sure, was using "per payment" to differentiate from receives and forwards. Will have to investigate that and get back.

[ziggie1984]

I benchmarked the old implementation against the implementation in #11413 using the same PostgreSQL benchmark and three iterations:

Store contents	Old	New	Speedup
1,000 stale	103.3 ms	3.4 ms	30.3x
10,000 stale	1.057 s	37.9 ms	27.9x
10,000, keep 10	1.048 s	42.3 ms	24.8x

This is approximately a 25-30x improvement on embedded PostgreSQL over localhost. A networked PostgreSQL deployment may see a larger improvement because the old implementation performs multiple client/server round trips per stale entry.

PR #11413 includes BenchmarkPostgresNetworkResultStoreCleanStore, so the comparison can be reproduced against both revisions.

[ziggie1984]

I compared this PR at b696ddbec5 with #11413 at 09286107f5.

The production cleanStore implementation is behaviorally identical in both PRs; the only production-file delta is comment wording. The meaningful differences are in validation and documentation:

• htlcswitch: avoid per-row queries when pruning network result store #11413 starts embedded PostgreSQL before calling kvdb.GetTestBackend. This PR's kvdb_postgres CI job currently fails in TestNetworkResultStoreCleanStore with 127.0.0.1:9876: connect: connection refused because no PostgreSQL server is started.
• htlcswitch: avoid per-row queries when pruning network result store #11413 adds the missing initially-empty-store case.
• htlcswitch: avoid per-row queries when pruning network result store #11413 stores distinct HTLC IDs and asserts retained payloads, so it can catch incorrect key/value cloning.
• htlcswitch: avoid per-row queries when pruning network result store #11413 adds BenchmarkPostgresNetworkResultStoreCleanStore for 1k stale, 10k stale, and 10k/keep-10. Using that benchmark, old vs. new measured about 25-30x faster on embedded PostgreSQL over localhost.
• htlcswitch: avoid per-row queries when pruning network result store #11413's release note removes the unmeasured “well under a second” guarantee and describes the improvement as eliminating per-entry client/server round trips.

So these PRs should not both merge. I recommend treating #11413 as the superseding version, or porting its validation/benchmark commit into this PR if this one is preferred. I found no correctness or security difference in the production implementation itself.

[ziggie1984]

Here are the concrete changes needed to bring this PR to parity with #11413:

1. Start PostgreSQL in TestNetworkResultStoreCleanStore. The current kvdb_postgres CI failure is caused by GetTestBackend connecting to port 9876 without first starting the embedded server:

func TestNetworkResultStoreCleanStore(t *testing.T) {
	if kvdb.PostgresBackend {
		stop, err := kvdb.StartEmbeddedPostgres()
		require.NoError(t, err)

		t.Cleanup(func() {
			require.NoError(t, stop())
		})
	} else {
		t.Parallel()
	}

	// ...
}

2. Add the initially empty store case to exercise the first-startup/no-results path:

{
	name:       "empty store",
	numResults: 0,
	keep:       map[uint64]struct{}{0: {}},
},

This requires making numResults a field of each table entry instead of a single constant.

3. Use distinct stored payloads and verify retained values. This makes the test detect an incorrect/missing bytes.Clone:

msg := &lnwire.UpdateAddHTLC{ID: i}
n := &networkResult{
	msg:          msg,
	unencrypted:  true,
	isResolution: true,
}

Then, for each retained result:

result, err := store.getResult(i)
require.NoError(t, err)
add, ok := result.msg.(*lnwire.UpdateAddHTLC)
require.True(t, ok)
require.Equal(t, i, add.ID)

4. Add the PostgreSQL benchmark from htlcswitch: avoid per-row queries when pruning network result store #11413 (BenchmarkPostgresNetworkResultStoreCleanStore) with these cases:

• 1k_all_stale
• 10k_all_stale
• 10k_keep_10

Keep fixture population outside the timer. This is the benchmark used for the old/new comparison (~25-30x locally).

5. Tighten the release note. Remove “so it commits in well under a second,” since the benchmark does not establish that as a universal guarantee. Ending with “one cascading delete instead of a round-trip per entry” accurately describes the improvement.

The production cleanStore logic itself does not need to change; it matches #11413 aside from comment wording.

[ZZiigguurraatt]

I benchmarked the old implementation against the implementation in #11413 using the same PostgreSQL benchmark and three iterations:
Store contents Old New Speedup
1,000 stale 103.3 ms 3.4 ms 30.3x
10,000 stale 1.057 s 37.9 ms 27.9x
10,000, keep 10 1.048 s 42.3 ms 24.8x

This is approximately a 25-30x improvement on embedded PostgreSQL over localhost. A networked PostgreSQL deployment may see a larger improvement because the old implementation performs multiple client/server round trips per stale entry.

PR #11413 includes BenchmarkPostgresNetworkResultStoreCleanStore, so the comparison can be reproduced against both revisions.

@saubyk is there any value in testing with SQLite to satisfy #11408 (comment) , or just forget about that comment?

[saubyk]

@ZZiigguurraatt No need for a SQLite benchmark. My comment was about the release-note wording. The release note in #11413 already resolves this: "SQL-backed nodes" with the hours attributed to remote Postgres, and "per locally initiated outbound HTLC attempt" instead of "per payment". If #11413 supersedes this PR, I consider the thread resolved.

Regardless SQLite ran the same per-entry statements, so it benefits from the fix too.

[ziggie1984]

closing in favour of #11413