sweep: preserve valid inputs after missing-input race

[vbrekher]

Fixes #10225.

When testmempoolaccept reports missing inputs before a historical spend notification has completed, verify the inputs with a blocking UTXO lookup instead of marking the whole batch fatal.

Specifically missing inputs are removed while the remaining inputs are retried through the existing unknown-spend path. If the blocking lookup cannot identify a missing input, the batch remains retryable rather than being dropped.

Tests:

• go test ./sweep -run 'TestHandleMissingInputsHistoricalLookup|TestHandleBumpEventTxUnknownSpendMissingInput' -count=1
• go test ./sweep -count=1
• go test . -run '^$' -count=1

[github-actions]

🔴 PR Severity: CRITICAL

gh pr view | 6 files | 255 lines changed

🔴 Critical (3 files)

• server.go - core server coordination logic
• sweep/fee_bumper.go - fee-bumping logic for sweep transactions
• sweep/sweeper.go - output sweeping / fund recovery logic

🟢 Low (3 files)

• docs/release-notes/release-notes-0.22.0.md - release notes entry
• sweep/fee_bumper_test.go - test-only changes
• sweep/sweeper_test.go - test-only changes

Analysis

This PR modifies sweep/fee_bumper.go and sweep/sweeper.go, which handle fee bumping and output sweeping (fund recovery) — both in the CRITICAL tier per the sweep/* rule. It also touches server.go, a core server coordination file, another CRITICAL-tier trigger. Two distinct critical packages are touched (server.go and sweep/*), which would normally bump severity up a level, but CRITICAL is already the highest tier. Non-test/non-generated line count (~152 lines across server.go, sweep/fee_bumper.go, sweep/sweeper.go, and the release note) is well under the 500-line bump threshold, and file count is well under 20. Recommend expert review given the fee-bumping and sweeping logic changes.

---

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

[Lrifton92]

Reviewed at 4c5691b.

The approach addresses the case from #10225: when no spend notification has arrived yet, handleMissingInputs (sweep/fee_bumper.go:740) now asks the backend per input and routes through createUnknownSpentBumpResult, and handleBumpEventTxUnknownSpend (sweep/sweeper.go:2000) fails only the listed outpoints while the rest go back as PublishFailed with Immediate set. The new path is only taken where the old code returned TxFatal for the whole set, so I don't see a regression against current behaviour.

I ran go test ./sweep -run 'TestHandleMissingInputs|Missing' -count=3 and go test ./sweep -count=1 at this commit, both pass; go vet ./sweep and go build . are clean.

Three things.

Inputs with an unconfirmed parent are reported as missing. On bitcoind and btcd, GetUtxo calls GetTxOut(&op.Hash, op.Index, false) (lnwallet/btcwallet/blockchain.go:76, :101), i.e. gettxout with include_mempool=false, and maps a nil result to ErrOutputSpent. An output created by a transaction that is still in the mempool therefore comes back as "not unspent", even though testmempoolaccept accepts it. The sweeper does receive such inputs: WalletKit.BumpFee only accepts outputs of unconfirmed transactions (lnrpc/walletrpc/walletkit_server.go:1515) and offers them via sweepNewInput, and these inputs carry no ExclusiveGroup, so BudgetAggregator.ClusterInputs can put them in the same set as a genuinely missing input. With this change the CPFP input is listed in MissingInputs and marked Fatal with ErrInputMissing, which is the same outcome this PR is meant to avoid for valid inputs. Flipping the flag to true is not a drop-in fix, since on the replacement path (handleReplacementTxError) the inputs are legitimately spent by our own in-mempool sweep. Checking whether the parent (op.Hash) is in the mempool before treating a nil gettxout as missing would cover it. I have not reproduced this against a live bitcoind; it follows from the RPC arguments above.

Spent and never-existed are collapsed. The closure in server.go:1309 maps both ErrOutputSpent and ErrOutputNotFound to "missing", and on bitcoind only ErrOutputSpent is ever returned. Inputs in MissingInputs skip handleUnknownSpendTx, so if the input was spent by one of our own earlier sweeps whose spend notification has not been delivered yet, it is failed with ErrInputMissing instead of being marked swept, and descendant records are not cleaned up. That matches the old behaviour for this branch, but the field comment at sweep/fee_bumper.go:284 ("confirmed no longer exist") suggests a stronger guarantee than the lookup gives. Worth either documenting or noting as a follow-up.

Test coverage of the new branches. TestHandleMissingInputsHistoricalLookup covers the mixed case with a stubbed callback. The two retry branches (len(missing) == 0 at :766, and the lookup error at :758), the IsInputUnspent == nil fallback, and the error mapping in the server.go closure are not exercised. A table-driven variant of the existing test would cover the first three cheaply. None of the tests go through handleInitialTxError or handleReplacementTxError, so the wiring from createAndCheckTx returning ErrInputMissing to the new result is only checked by reading.

Minor: lines 747, 765, 767 and 771 of sweep/fee_bumper.go are 82-83 columns with 8-wide tabs, which the ll linter (line-length: 80) will flag once CI runs. handleInitialBroadcast runs synchronously in processRecords, so the per-input RPC now blocks the publisher loop; cheap on bitcoind, but worth a comment since the callback is generic.

[vbrekher]

Thanks, I tightened the fallback without changing the existing GetUtxo semantics. If the notifier hasn’t delivered a spend yet, the publisher now checks the mempool watcher for a spender. When the chain lookup doesn’t find the output but the wallet knows the parent, the input is preserved only while that parent is still unconfirmed. I also adjusted the MissingInputs wording to match what the lookup can actually prove, pulled the server error mapping into a testable helper, and added coverage for the retry/error/nil-callback cases and both the initial and replacement error paths. The focused tests, full sweep package, root compile, vet and make lint-source all pass.

[Lrifton92]

Reviewed at 522cd68.

Thanks for working through the earlier points. The UTXO classifier is now a testable helper, the retry, error and nil-callback branches have tests, both the initial and replacement error paths are covered, and the MissingInputs doc comment no longer promises more than the lookup proves. go test ./sweep/ and go vet ./sweep/ pass at this commit, and no added line exceeds 80 columns.

One blocking issue, one I'd like addressed, and two small ones.

Blocking: our own unconfirmed sweep is now reported as TxConfirmed. The mempool fallback was added to getSpentInputs (sweep/fee_bumper.go:1568), but that function is not only reached from handleMissingInputs. processRecords calls it for every monitored record on every block (fee_bumper.go:1087). Once a sweep has been broadcast, LookupInputMempoolSpend returns that same sweep as the spender of its inputs. isUnknownSpent then sees the spender's txid equal r.tx and returns false, and the record goes to confirmedRecords (:1119) while the tx is still unconfirmed. handleTxConfirmed emits TxConfirmed, handleResult removes the record, and the sweeper's monitor exits and calls CancelRebroadcast on the tx (sweep/sweeper.go:1677-1690). Net effect: on bitcoind and btcd (where cc.MempoolNotifier is set, chainreg/chainregistry.go:380, :595) a published sweep is never fee-bumped and stops being rebroadcast from the next block on. For deadline-bound inputs like HTLCs, that is the path to missing the deadline.

I reproduced it with a unit test built on createTestPublisher: a record whose tx is set, no spend notification, and a MockMempoolWatcher returning that same tx for the input. processRecords() delivers Event=TxConfirmed. Before this commit there is no mempool branch there, so the record would go to feeBumpRecords. Previously the spend subscription only fired on a confirmed spend, which is why "spent by our tx" could safely mean "confirmed". Scoping the mempool lookup to handleMissingInputs (as its comment at :1559-1562 suggests was the intent), or not treating a mempool spend by r.tx as confirmation, would restore that. A regression test for the own-sweep-in-mempool case would pin it either way.

The wallet fallback returns an error whenever the wallet doesn't know the parent. In findMissingInputs (:719), Wallet.FetchTx is btcwallet.GetTransaction, which returns ErrNoTx ("can not find transaction") for a txid the wallet store doesn't hold, not (nil, nil). The err != nil return then turns the whole lookup into an error, and handleMissingInputs retries the full set. So an input whose parent is not a wallet transaction can never land in MissingInputs, and the partial-failure path this PR adds is only reachable for wallet-known parents. That includes the case of a parent that was double-spent and will never exist. The existing tests don't see this: TestHandleMissingInputsHistoricalLookup runs with Wallet nil, so the fallback is skipped, and TestFindMissingInputsWalletLookupError asserts that the error is surfaced. Treating errors.Is(err, base.ErrNoTx) as "not a wallet parent" and falling through to missing, with a test where the parent is unknown, would fix it. I haven't enumerated which contractcourt parents end up in the wallet store. Any that don't take this path.

Smaller:

• A wallet-known parent that was evicted or replaced in the mempool still reports NumConfirmations == 0, so its outputs are kept retryable indefinitely. Is there a bound on that, or is it acceptable because the input will eventually be resolved elsewhere?
• classifyInputUtxoLookup was inserted between newServer's doc comment and newServer, so its godoc now begins "newServer creates a new instance of the server…" and newServer has no doc comment (inline).

[Lrifton92]

getSpentInputs is also the per-block check in processRecords (:1087). Once r.tx is broadcast, this returns r.tx itself as the spender, isUnknownSpent is false, and the record goes to confirmedRecords: TxConfirmed is emitted for an unconfirmed tx, the record is dropped, and the sweeper cancels its rebroadcast. I reproduced it with createTestPublisher + MockMempoolWatcher returning r.tx: processRecords() yields Event=TxConfirmed. Scoping this lookup to the handleMissingInputs path would avoid it.

[Lrifton92]

With the real wallet, FetchTx on a txid the store doesn't hold returns btcwallet.ErrNoTx, not (nil, nil). So any input whose parent isn't a wallet tx makes the whole lookup fail and the set is retried, and it can never be classified as missing. Suggest treating errors.Is(err, base.ErrNoTx) as "unknown parent" and falling through to missing[op], with a test for it (the mixed-set test runs with Wallet nil, so it doesn't reach this).

[Lrifton92]

This landed between newServer's doc comment (:662-664) and newServer, so godoc attributes "newServer creates a new instance…" to classifyInputUtxoLookup, and newServer (now just //nolint:funlen) loses its comment. Moving the helper above line 662 fixes both.

[saubyk]

Thanks for the PR, @vbrekher.

Per the new contributor section of the contribution guidelines, we don't prioritize review of PRs from authors without a track record in the project. Given the current review load, I'm closing this rather than letting it sit.

Since #10225 already covers this, we'll handle it from that issue; if a fix is warranted a maintainer will pick it up.

For building a track record with the project, issue triage and reviewing open PRs are the best starting points. They demonstrate familiarity with the codebase far better than new code does, and they make future PRs from you much easier to prioritize.