sweep: isolate bad inputs on mempool rejection - #10842
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a diagnostic mechanism for the sweeper to identify and isolate problematic inputs when a transaction batch is rejected by the mempool. By performing no-broadcast probes on subsets of inputs, the system can now pinpoint specific inputs causing failures rather than failing the entire batch. This improves the robustness of the sweeping process by ensuring that only truly invalid inputs are treated as fatal, while valid inputs can continue to be processed and retried. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to isolate and diagnose bad inputs in a rejected sweep batch using no-broadcast mempool acceptance probes. When a multi-input batch is rejected with an unattributed non-fee mempool or script error, the TxPublisher performs a binary search probe of the input subsets to identify the specific inputs causing the failure. The UtxoSweeper then marks only those identified bad inputs as fatal, allowing the remaining valid inputs in the batch to be retried. A review comment suggests adding a defensive nil check for r.feeFunction in shouldDiagnoseBadInputs to prevent a potential nil pointer dereference panic.
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (2 files)
🟢 Low (2 files)
AnalysisThis PR modifies The bulk of changes (265 lines) are in Severity bump check: 4 non-test files (threshold: >20), 417 non-test lines (threshold: >500), single critical package — no bump applied. To override, add a |
87d1e3e to
75b9fe1
Compare
75b9fe1 to
4ef522b
Compare
|
LGTM ⚡ ( |
4ef522b to
b1443e9
Compare
|
LGTM 🦦 |
|
LGTM ✨ ( |
a5f51c3 to
24a2759
Compare
|
No issues found by |
|
No issues found by openai/gpt-5.5 ✅ |
24a2759 to
7ca790c
Compare
7ca790c to
7991909
Compare
7991909 to
e111613
Compare
Mark concrete mempool-check failures for later attribution. Treat btcd mempool conflicts as missing-input candidates. Spend attribution handles them instead of generic terminal logic.
Restrict subset probes to multi-input mempool rejections. Require a known fee function and no auxiliary sweeper.
Test input subsets without publishing them. Fund required-output probes only with normal inputs that pass an independent mempool check.
Recognize only script, witness, and input-standardness errors as attributable singleton failures.
Bisect rejected sets and fall back to singleton scans when a subset cannot be constructed. Stop on non-attributable policy errors.
Distinguish skipped, completed, aborted, and attributed diagnosis results so retry policy can depend on membership changes.
Advance the fee only when diagnosis aborts without attributing an input. Treat conclusive unattributed failures and fee advancement errors as fatal, while membership changes restart without a starting fee.
Apply singleton attribution and unchanged-set fee advancement to replacement failures exactly as initial publication does. Stop sweeper monitors after every terminal publisher result.
Match anchored btcd input-rejection forms, including Go's bracketed witness encoding, for the transaction under test. Require btcwallet's expected mapping before normalizing the error.
Track diagnosed wallet UTXOs for the sweeper lifetime and exclude them from future coin selection so an invalid wallet input cannot poison every regrouped sweep.
Mark only an attributed tracked input fatal, quarantine attributed wallet inputs, and restart surviving membership without carrying the aggregate transaction's fee rate.
Document no-broadcast diagnosis, funded required-output probes, survivor fee reset, unchanged-set advancement, fatal fee errors, and initial/replacement parity.
Record singleton bad-input isolation in the 0.22.0 release notes and add the contributor credit.
49f711b to
d6ff7b4
Compare
|
No issues found by |
| } | ||
|
|
||
| probeInputs := append([]input.Input(nil), inputs...) | ||
| for _, companion := range r.req.Inputs { |
There was a problem hiding this comment.
This companion scan turns diagnosis from O(log n) probes into roughly O(n^2) in the worst case, and it all runs inline on the blockbeat critical path against a hard 60s deadline.
Each companion costs a full createSweepTx (build + sign) plus a testmempoolaccept round trip at line 802-805, and then another createProbe for the grown set at line 810. Nothing is cached, so the same companion is re-verified from scratch on every subset that needs funding. findBadInputBySingleton (line 892) probes all n inputs individually, and each of those calls lands back here — with DefaultMaxInputsPerTx = 100 (sweep/txgenerator.go:22) that is up to ~100 x ~100 build+sign+RPC cycles for a single diagnosis.
The path is synchronous: processRecords calls handleInitialBroadcast inline at line 1352 — unlike handleTxConfirmed (1359) and handleFeeBumpTx (1368), which are dispatched as goroutines — and NotifyBlockProcessed only runs after processRecords returns (lines 1256-1259). chainio.DefaultProcessBlockTimeout is 60s (chainio/dispatcher.go:18), so on a remote or loaded bitcoind a large batch can blow the beat deadline outright.
A few options, in rough order of value: cache the "independently accepted" verdict per outpoint for the duration of one diagnosis so each companion is probed at most once; stop at the first companion that makes construction succeed rather than continuing to grow the set; cap the number of companions tried; and move the whole diagnosis off the beat path into a goroutine the way the other two handlers already are.
(claude-opus-5[1m])
|
|
||
| // normalizeMempoolAcceptError corrects exact raw btcd input rejections for the | ||
| // transaction being tested that btcwallet cannot distinguish. | ||
| func normalizeMempoolAcceptError(backend string, txHash chainhash.Hash, |
There was a problem hiding this comment.
This re-derives a dependency's error classification by regex-matching btcd's human-readable message text, and the result decides whether an input is permanently failed: both sentinels produced here feed isInputScriptFailure (sweep/fee_bumper.go:820), which drives BadInput and ultimately markInputFatal in the sweeper.
I checked the patterns against btcd v0.26.0-beta.rc1 and they are correct today — mempool/mempool.go:1636 produces transaction %v has a non-standard input: %v wrapping mempool/policy.go:167, and blockchain/scriptval.go:83,97 produce the failed to parse|validate input ... form, including the bracketed %x witness encoding the regex accounts for. So this works.
The concern is placement. These are plain fmt.Sprintf strings in btcd with no stability guarantee, and the mapping already belongs to btcwallet: chain/errors.go:517-518 maps both "non-standard script form" and "has a non-standard input" to ErrNonStandardScript, and line 551 carries a commented-out // "unmatched btcd error 2": ErrScriptVerifyFlag placeholder — the gap is known upstream and has a home there. Fixing it in btcwallet gives every consumer the correct sentinel and keeps the string knowledge in the one package that already owns it.
If a btcd reword silently stops the match the failure is safe (the original error passes through), but it also silently disables input isolation with no signal. Worth at least a warn log on the non-match path so the regression is visible rather than silent.
(claude-opus-5[1m])
| if errors.Is(err, chain.ErrMissingInputs) { | ||
| // sweepCtx and an error. Btcd reports an unconfirmed spend as a mempool | ||
| // conflict; both errors need spend attribution. | ||
| if errors.Is(err, chain.ErrMissingInputs) || |
There was a problem hiding this comment.
High: Treating chain.ErrMempoolConflict as ErrInputMissing makes an unconfirmed reversible conflict enter the confirmed-spend notifier path and can quarantine a still-valid input, so preserve a distinct retryable conflict state and consult the mempool watcher until eviction. (gpt-5.6-sol)
| probeInputs = append(probeInputs, companion) | ||
| sweepCtx, err = createProbe(probeInputs) | ||
| if err == nil { | ||
| return t.checkProbeTx(sweepCtx) |
There was a problem hiding this comment.
High: A companion can become spent after its singleton probe succeeds, so attributing a later combined-probe failure to r.request.Input can fatally quarantine the wrong input; revalidate the companion and treat ambiguous failures as retryable. (gpt-5.6-sol)
| // Establish that the companion is independently accepted before | ||
| // using it to fund a probe. Otherwise its own failure could be | ||
| // misattributed to the required-output input. | ||
| companionCtx, companionErr := createProbe( |
There was a problem hiding this comment.
High: Probing each funder alone and skipping funders that cannot independently produce an output misses cases where several low-value companions collectively fund the required output, so build and validate progressively funded groups before classifying the target as bad. (gpt-5.6-sol)
| return fn.Some(*bumpResult) | ||
| } | ||
|
|
||
| // Initial and replacement mempool rejections use the same diagnosis. |
There was a problem hiding this comment.
High: Reusing singleton bad-input diagnosis after a replacement is rejected can mark the request terminal even though oldTx is still accepted and spending the input, so retain monitoring of oldTx unless a valid replacement has actually displaced it. (gpt-5.6-sol)
| Err: err, | ||
| requestID: r.requestID, | ||
| } | ||
| if diagnosis.outcome == diagnosisAttributed { |
There was a problem hiding this comment.
Medium: This commit produces terminal BadInput results before the consumer and persistence behavior land in 6f71a1e6b, so fold the producer and consumer changes into one commit to keep the stack bisectable. (gpt-5.6-sol)
| "fee bump monitor", r.Event, | ||
| r.Tx.TxHash()) | ||
| switch r.Event { | ||
| case TxConfirmed, TxFailed, TxFatal, TxUnknownSpend: |
There was a problem hiding this comment.
Medium: Making TxUnknownSpend terminal here exposes replacement missing-input results from handleMissingInputs that contain no transaction or the rejected candidate instead of the accepted oldTx, so pass oldTx through that path before exiting and cancelling rebroadcast. (gpt-5.6-sol)
3 participants
Summary
Part of #10840.
Testing
go test ./sweep ./lnwallet/btcwalletgo test -race ./sweep ./lnwallet/btcwalletgo vet ./sweep ./lnwallet/btcwalletmake lint-sourcegit diff 2c3e6ffccebc440f4d1b561324ecf4702feac1da..HEAD --check