Add raw transaction hex to pendingsweeps response#10670
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 enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 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 counter productive. 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 new RawTxHex field to the PendingSweep RPC message, allowing users to retrieve the hex-encoded raw sweep transaction. This involves updating the protobuf definition, modifying the RPC server to serialize the sweep transaction, and adjusting the internal sweeper logic to store and expose this transaction. Integration tests have been updated to validate the presence and correctness of this new field. Additionally, a new informational message is printed to the console after a successful bumpFee command. A suggestion was made to refactor duplicated validation logic in the integration tests into a helper function for improved maintainability.
saubyk
force-pushed
the
return-raw-hex-on-bumpfee
branch
from
20558ca to
172c7f9
Compare
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟠 High (3 files)
🟡 Medium (1 file)
⚪ Excluded from counts (3 files)
AnalysisThis PR touches The remaining non-excluded, non-test changes are in No severity bump was triggered: only 5 non-excluded files (threshold: >20) and ~52 non-excluded lines changed (threshold: >500), and only one critical package ( To override, add a |
saubyk
force-pushed
the
return-raw-hex-on-bumpfee
branch
from
172c7f9 to
7ce7535
Compare
saubyk
force-pushed
the
return-raw-hex-on-bumpfee
branch
2 times, most recently
from
d79b6fa to
db8573b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a raw_tx_hex field to the PendingSweep response in walletrpc.PendingSweeps, allowing callers to inspect or rebroadcast in-flight sweep transactions. Feedback on the changes includes addressing a potential data race caused by passing a mutable transaction pointer across channel boundaries, resolving potential integration test flakiness by wrapping a RawTxHex assertion in a retry loop, and improving unit test coverage by asserting that the sweep transaction is correctly stored on published inputs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
saubyk
force-pushed
the
return-raw-hex-on-bumpfee
branch
from
db8573b to
1cf7e44
Compare
github-actions
Bot
added
severity-critical
saubyk
force-pushed
the
return-raw-hex-on-bumpfee
branch
from
1cf7e44 to
aaa701c
Compare
CI fix:
|
saubyk
force-pushed
the
return-raw-hex-on-bumpfee
branch
from
aaa701c to
8231f83
Compare
|
/gateway review |
|
✅ Review posted: #10670 (review) 6 finding(s); 6 inline, 0 in body. 🔁 Need a re-review after pushing changes? Reply with |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds a raw_tx_hex field to walletrpc.PendingSweeps (and the matching lncli output), backed by a new sweepTx stored on each SweeperInput, so callers can inspect or rebroadcast an in-flight sweep without scraping the mempool. The plumbing (proto, generated code, RPC server, CLI wrapper, itests) is coherent, the previously-raised cross-goroutine concern is correctly handled by deep-copying in handlePendingSweepsReq, and the RBF-on-Published staleness fix in markInputsPublished is sound and tested.
The core problem is that the new sweepTx field has an incomplete lifecycle. It is set only on the publish-success path and is never cleared on publish failure or unknown-spend retry, and it is never populated when an input is re-registered after a restart with its sweep already in the mempool. As a result raw_tx_hex can be stale, point at an RBF-replaced or rejected transaction, or be empty for a sweep that is in fact in flight. Combined with a proto doc comment that promises the tx "currently spends this input" and a stated rebroadcast use-case, this can lead an operator to rebroadcast a transaction that is no longer the active sweep.
Separately, the bumpfee CLI now writes a human hint to stdout after the JSON response, breaking programmatic consumers that parse stdout. These issues echo unresolved review threads from the maintainer that have no author response.
Findings: 🔴 0 Blocker · 🟠 4 Major · 🟡 2 Minor · 🔵 0 Nit
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
saubyk
force-pushed
the
return-raw-hex-on-bumpfee
branch
from
8231f83 to
0d7e91e
Compare
|
/gateway re-review |
|
✅ Re-review posted: #10670 (review) Prior findings: 6 addressed, 0 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
All six prior findings are addressed. F1 dropped the stdout print and moved the hint into bumpfee's help, so stdout stays JSON-only. F2's proto/swagger/release-note wording now says "most recently broadcast," documents the RBF lag and the empty-state semantics. F3 clears pi.sweepTx in markInputsPublishFailed; F4 threads the recovered mempool tx out of decideRBFInfo into handleNewInput; F5 logs-and-continues on a per-input serialize failure; F6 adds the spentByTx filter plus an inputNotInTx negative case. Test coverage backs each.
Two new minor concerns remain, both about subtle edges of the widened markInputsPublished guard and the rebroadcast guidance — neither has a demonstrated reachable defect, so they're flagged for confirmation rather than as blockers. No new major issues.
Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 2 Minor · 🔵 0 Nit
Status of prior findings
- F1 addressed: Fixed in
cmd/commands/walletrpc_active.go—bumpFee()now ends withprintRespJSON(resp)only; thependingsweepspointer lives in the commandDescription, so stdout stays machine-readable. - F2 addressed: Reworded in
lnrpc/walletrpc/walletkit.proto:1239(mirrored inwalletkit.swagger.jsonand the release notes): "most recently broadcast," documents the brief RBF lag and that empty means no tracked sweep. The overstatement ("currently spends") is gone. (A narrower residual on the rebroadcast recipe is raised fresh as F7.) - F3 addressed: Fixed in
sweep/sweeper.go—markInputsPublishFailedsetspi.sweepTx = nil, so the unknown-spend retry path no longer surfaces a stale/invalidated tx. Locked in byTestMarkInputsPublishFailed(require.Nil(pi.sweepTx)on both transitioned inputs). - F4 addressed: Fixed in
sweep/sweeper.go—decideRBFInfonow returns(fn.Option[RBFInfo], *wire.MsgTx)andhandleNewInputstores the recovered mempool tx assweepTx, so an already-published sweep surfacesraw_tx_hexright after restart. Covered byTestDecideRBFInfo(require.Equal(&tx, sweepTx)). - F5 addressed: Fixed in
lnrpc/walletrpc/walletkit_server.go— a per-inputSerializefailure now logs and leavesraw_tx_hexempty for that input instead of returning an error that aborts the whole listing. - F6 addressed: Fixed in
sweep/sweeper.go—markInputsPublishedbuildsspentByTxfromtx.TxInand only storessweepTxfor inputs actually spent by the tx. The negative case is locked in byTestMarkInputsPublished'sinputNotInTx(require.Nil(...sweepTx)).
Store the most recent sweep transaction on each SweeperInput when it is published or replaced via RBF. This allows callers of PendingInputs to access the current in-flight sweep transaction for each input. Update PendingInputResponse to expose the sweep tx externally.
Add a raw_tx_hex field to the PendingSweep proto message so users can obtain the hex-encoded sweep transaction for manual rebroadcasting. The field is populated by serializing the sweep tx in the PendingSweeps RPC handler when available. Closes lightningnetwork#8470
Print a message after a successful bumpfee call directing users to use pendingsweeps to monitor the sweep transaction and obtain the raw transaction hex if needed.
Add assertions to the bump fee integration tests verifying that the raw_tx_hex field in the PendingSweeps response is populated and refers to the actual sweep transaction for the pending input. Rather than requiring raw_tx_hex to be a byte-for-byte match of the sweep tx currently in the mempool, the assertion decodes raw_tx_hex and checks that it spends the input being swept. An exact match is unreliable under RBF: publishAttempts (reported as BroadcastAttempts) is incremented in markInputsPendingPublish and the replacement tx is broadcast to the mempool before sweepTx is recorded in markInputsPublished. The two stages run on the sweeper's goroutine in sequence, so there is a window where the mempool already holds the new RBF version while the input still tracks the previous one. testBumpFeeLowBudget samples raw_tx_hex immediately after AssertTxNotInMempool confirms the replacement reached the mempool, i.e. inside that window, with no BroadcastAttempts barrier to gate on (unlike runBumpFee). It therefore reads the mempool tx and the RPC's tracked tx one RBF version apart and the exact match never holds, failing deterministically across all backends. Every RBF version spends the same input, so asserting that invariant is both correct and free of the timing dependency. runBumpFee is hardened the same way since its exact match only passed by virtue of its BroadcastAttempts gate.
saubyk
force-pushed
the
return-raw-hex-on-bumpfee
branch
from
0d7e91e to
f7595b7
Compare
|
/gateway re-review |
|
✅ Re-review posted: #10670 (review) Prior findings: 7 addressed, 1 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
Seven of the eight prior findings remain fully addressed in the current head, including F7 (proto doc reworded to advisory/diagnostic with an explicit in-flight-RBF conflict warning) and F8 (the if tr.FeeRate != 0 guard). The one regression: between the last-reviewed SHA and this head, markInputsPublishFailed was changed from clearing pi.sweepTx (the mechanism that addressed F3) to retaining it. Retaining is correct and an improvement for the common budget-failure RBF case — but it re-opens F3's original concern for the unknown-spend retry path, where a survivor keeps a non-confirmable batched tx. The two new items are minor: a CLI-help/proto-doc consistency gap and a confirm-the-invariant on the widened publish-state guard. Nothing here blocks; harm is bounded by the field's documented advisory semantics.
Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 3 Minor · 🔵 0 Nit
Status of prior findings
- F1 addressed: The
bumpfeehint now lives in the commandDescriptionand thebumpFeehandler'sprintRespJSON(resp)path is unchanged, so stdout stays pure JSON. - F2 addressed: The proto field doc (mirrored in swagger) was reworded to "most recently broadcast … advisory and intended for diagnostics only," with explicit RBF-staleness and re-injection-conflict warnings.
- F4 addressed:
decideRBFInfonow returns the recovered mempool tx andhandleNewInputstores it assweepTx;TestDecideRBFInfoasserts the returned tx (require.Equal(&tx, sweepTx)). - F5 addressed:
PendingSweepslogs a per-inputSerializeerror and leavesraw_tx_hexempty rather than aborting the whole listing. - F6 addressed:
TestMarkInputsPublishedadds theinputNotInTxnegative case, locking in thatsweepTxis stored only for inputs the published tx actually spends. - F7 addressed: The reworded proto doc now frames
raw_tx_hexas diagnostic/advisory and states plainly that re-injecting it can conflict with lnd's in-flight RBF — it no longer implies a txid-presence check makes rebroadcast safe. - F8 addressed:
pi.lastFeeRate = chainfee.SatPerKWeight(tr.FeeRate)is now guarded byif tr.FeeRate != 0, so a zero rate never clobbers a valid one (and the RPC'ssat_per_vbyte).
| parent's fee. This can be done by specifying an outpoint within the low | ||
| fee transaction that is under the control of the wallet. | ||
|
|
||
| After bumping, use 'lncli wallet pendingsweeps' to monitor the sweep and |
There was a problem hiding this comment.
🟡 Minor F9: bumpfee help omits the advisory caveat the proto doc carries
The bumpfee help text — "use 'lncli wallet pendingsweeps' to monitor the sweep and obtain its raw transaction hex if needed" — is the surface a CLI user actually reads, and it omits the advisory/no-blind-rebroadcast caveat that the proto and swagger docs spend their entire body on. A user who follows the help to "obtain its raw transaction hex" never sees the warning that re-injecting it can conflict with lnd's in-flight RBF. Add a short caveat to the help (e.g. "the raw hex shown there is advisory/diagnostic only and may be stale during fee-bumping"), so the user-facing guidance is consistent with the field's documented semantics.
| if pi.state != PendingPublish { | ||
| // We may get a Published if this is a replacement tx. | ||
| // Validate that the input is in an expected state. We may | ||
| // get a Published input if this is a replacement tx, in |
There was a problem hiding this comment.
🟡 Minor F10: Widened publish guard re-enters and overwrites sweepTx on already-Published inputs
The markInputsPublished guard was widened from PendingPublish-only to {PendingPublish, Published} (intended for RBF-on-Published, covered by TestHandleBumpEventTxReplacedOnPublished). A side effect distinct from prior F6/F8: an already-Published input now re-enters the mutation block on every publish/replace event and unconditionally executes pi.sweepTx = tx. The per-set event stream is serial, but an input can be regrouped into a new set while a TxReplaced for the old set is still queued on bumpRespChan; under the old guard that stale event hit continue, but it now passes and could regress sweepTx to an older tx. Worst case is a stale advisory field, so this is minor — but confirm the publish-event stream cannot deliver an out-of-order event for an input that has left a set, or gate the sweepTx overwrite on tx recency. (The monitorFeeBumpResult/sweepPendingInputs lifecycle is not in the loaded context, so I cannot construct the scenario concretely; this is a confirm-the-invariant note.)
| // Update the input's state. | ||
| pi.state = PublishFailed | ||
|
|
||
| // We intentionally keep pi.sweepTx here. It's only ever set to a |
There was a problem hiding this comment.
🟡 Minor F3 · partially_addressed
The clear-on-PublishFailed that addressed F3 at the last-reviewed SHA has been reverted: markInputsPublishFailed now retains pi.sweepTx ("We intentionally keep pi.sweepTx here … still the live sweep in the mempool"), and TestMarkInputsPublishFailed asserts the retention. The justification holds for the budget-failure RBF case (no new tx broadcast, the prior one is genuinely still live) and is a real improvement over clearing — clearing would empty raw_tx_hex while a live sweep exists. But the original F3 concern was the unknown-spend retry path: handleBumpEventTxUnknownSpend marks surviving inputs PublishFailed, so a survivor of a batched sweep retains the batched tx that also spent the now-third-party-spent sibling — a tx that can never confirm — and surfaces it via raw_tx_hex until its next re-publish. The retain comment's "still the live sweep in the mempool" rationale does not hold on that path. Bounded window + advisory framing keep this minor; consider a conditional clear on the unknown-spend path or a note that the retained tx may be non-confirmable. (I lack handleBumpEventTxUnknownSpend's body in the loaded context; this rests on the prior marker plus TestHandleBumpEventTxUnknownSpendWithRetry showing survivors → PublishFailed.)
2 participants
Fixes #8470
Change Description
Add raw transaction hex to pendingsweeps response
Steps to Test
lncli wallet bumpfee --sat_per_vbyte 10 --immediate <outpoint>lncli wallet pendingsweepsbitcoin-cli decoderawtransaction <raw_tx_hex>Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.