walletrpc: retain input leases through spend confirmation - #11125
walletrpc: retain input leases through spend confirmation#11125bhandras wants to merge 5 commits into
Conversation
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (3 files)
🟠 High (5 files)
🟡 Medium (2 files)
🟢 Low (3 files)
AnalysisThe highest-severity files touched are in To override, add a |
8f28756 to
34267bd
Compare
d276ae4 to
f543cac
Compare
|
/gateway review Review target: #11125, base Purpose and invariant: a caller may request that wallet inputs remain leased until their confirmed spend reaches a chosen depth. A shallow reorganization must not make those inputs selectable for a different transaction. Zero must preserve the existing wall-clock lease behavior. A non-zero request must fail closed if the wallet cannot install the stronger lease. Verify these concrete paths:
Risk discipline: for every finding, give exact preconditions, an execution trace, consequence, realistic likelihood, existing guards or recovery, whether this PR introduces or worsens it, the smallest failing regression, and the lowest-risk fix. Classify it as blocker, follow-up, or non-actionable. Do not block on speculative states, pre-existing debt, comments, or broad redesigns without a reachable material failure. Validation already passed locally: the focused WalletKit test suite, Stop when the intended invariant is proved and no concrete PR-introduced blocker remains. In the summary, state one final assessment: |
f543cac to
6e51c49
Compare
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gateway review — 4 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 4 Minor · 🔵 0 Nit
Summary
The change threads an opt-in release_after_spend_confs from two RPC entry points down to btcwallet's persisted lease option, and the plumbing is sound: the depth reaches every input FundPsbt selects, both entry points reject a non-zero request before touching the legacy lease method, and LeaseOutputOptions / OutputLeaserWithOptions keep other WalletController implementations source-compatible. The partial-lock rollback fix is correct and is a strict improvement — when no custom lock ID is supplied, locks[i].LockID is chanfunding.LndInternalLockID, so the default path is unchanged while the custom-ID path now releases with the ID that acquired the lease.
Wire compatibility holds. All four new fields take previously unused tag numbers (LeaseOutputRequest 4, LeaseOutputResponse 2, FundPsbtRequest 15, UtxoLease 6), are uint32, and default to zero, which preserves the wall-clock path. The generated .pb.go raw descriptor and the Swagger definitions match the .proto source, including the size-prefix updates on the four affected messages. Both direct requirements resolve to pseudo-versions carrying 5c2f9a351a0a, and go.mod has no btcwallet replace left.
Nothing here is a blocker. What I found is one stale lint exemption that contradicts the PR's own dependency claim, a response-echo that does not deliver the fail-closed signal the description promises, and two boundary details worth tightening. Two of the findings turn on btcwallet internals that are not in the reviewed context; I have flagged those as uncertainty rather than asserting a defect.
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)
| - github.com/lightningnetwork/lnd/sqldb | ||
| - github.com/lightningnetwork/lightning-onion | ||
| # Temporary pins for configurable persisted output leases. | ||
| - github.com/btcsuite/btcwallet |
There was a problem hiding this comment.
🟡 F1 (Minor) — Lint exemption permits btcwallet replaces the PR just removed · .golangci.yml:176
This PR removes the temporary fork replacements and go.mod contains no replace for github.com/btcsuite/btcwallet or its wtxmgr submodule, yet these two entries are added to gomoddirectives' replace-allow-list with a comment describing them as temporary pins. They are dead configuration today and permanently exempt any future btcwallet replace from the lint that would otherwise surface it for review.
There was a problem hiding this comment.
Fixed. The temporary btcwallet and wtxmgr allowlist entries are removed with the fork replacements. The current branch has no .golangci.yml diff.
|
|
||
| // Convert the lock leases to the RPC format. | ||
| rpcLocks := marshallLeases(locks) | ||
| for _, lock := range rpcLocks { |
There was a problem hiding this comment.
🟡 F2 (Minor) — Echoed depth reflects the request, not the installed lease · lnrpc/walletrpc/walletkit_server.go:2313
This loop overwrites every UtxoLease.ReleaseAfterSpendConfs that marshallLeases derived from the wallet's ListLeasedOutputResult, and LeaseOutput at lnrpc/walletrpc/walletkit_server.go:547 returns req.ReleaseAfterSpendConfs directly, so both responses are tautological echoes of the input. The proto documents these as the "accepted" and "applied" depth, so a caller cannot use them to fail closed if the wallet installed something other than what was asked for — the value is equal to the request whatever the wallet did.
There was a problem hiding this comment.
Clarified the contract and RPC docs. OutputLeaserWithOptions implementations must apply every non-zero option exactly or return an error. The response reports the depth only after that call succeeds. The docs now describe the non-zero echo as server capability confirmation, not an independent backend readback.
| func (b *BtcWallet) LeaseOutput(id wtxmgr.LockID, op wire.OutPoint, | ||
| duration time.Duration) (time.Time, error) { | ||
|
|
||
| return b.LeaseOutputWithOptions( |
There was a problem hiding this comment.
🟡 F3 (Minor) — Every existing lease caller is rerouted to a new wallet method · lnwallet/btcwallet/btcwallet.go:1147
BtcWallet.LeaseOutput now routes all pre-existing lease callers — channel funding, the sweeper, the plain LeaseOutput RPC — through b.wallet.LeaseOutputWithOptions with an empty option slice rather than b.wallet.LeaseOutput. I cannot tell from this diff whether those two btcwallet entry points are equivalent for zero options, since btcwallet is not in the reviewed context; keeping the no-option path on b.wallet.LeaseOutput and calling the WithOptions variant only when opts.ReleaseAfterSpendConfs > 0 would remove the question without changing the new behavior.
There was a problem hiding this comment.
No code change needed. I verified the merged btcwallet implementation: Wallet.LeaseOutput delegates directly to LeaseOutputWithOptions with no options. The existing zero-option path is therefore identical, and the LND adapter preserves that delegation.
| op wire.OutPoint, duration time.Duration, | ||
| opts LeaseOutputOptions) (time.Time, error) { | ||
|
|
||
| leaser, ok := l.WalletController.(OutputLeaserWithOptions) |
There was a problem hiding this comment.
🟡 F4 (Minor) — Capability probe always succeeds for LightningWallet · lnwallet/wallet.go:645
Defining this method on the wrapper makes *LightningWallet satisfy OutputLeaserWithOptions unconditionally, whether or not the embedded WalletController supports options, so the w.(lnwallet.OutputLeaserWithOptions) probe used by both new RPC call sites is not a reliable capability test for that type — rejection falls through to this forwarder's separate error string instead. Behavior stays fail-closed either way, but both new rejection tests pass a bare controller rather than a LightningWallet, so whichever of the two gates production actually hits is untested.
There was a problem hiding this comment.
Fixed. Added direct LightningWallet wrapper coverage. It verifies that supported controllers receive the exact options and that unsupported concrete controllers fail closed through the production forwarding path.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
6e51c49 to
ce6eddc
Compare
|
/gateway re-review Head: ce6eddc All four prior findings have individual replies. The stale lint allowlist is gone. The option contract and RPC wording are explicit. The zero-option btcwallet delegation was verified against the merged implementation. Direct LightningWallet forwarding and fail-closed tests were added. Please re-check only concrete defects introduced or worsened by this PR. Classify each remaining item as blocker, follow-up, or not worth changing. |
There was a problem hiding this comment.
Gateway review — 9 findings
🔴 0 Blocker · 🟠 1 Major · 🟡 8 Minor · 🔵 0 Nit
Summary
The four prior findings are all answered in substance: the stale lint allowlist is gone, the option contract and RPC field wording are now explicit, the zero-option btcwallet delegation was verified upstream, and direct *LightningWallet forwarding coverage was added for both the supported and unsupported cases. The partial-lock rollback fix and its regression test are correct — releasing with each lease's acquiring LockID is right for both the custom and internal ID cases, and the i < idx bound cannot touch an unassigned slot.
One new blocker-adjacent gap remains, and it is the same invariant the PR sets out to establish. Both the capability probe and the option application live inside lockInputs' per-outpoint loop, so a FundPsbt request carrying a non-zero depth that ends up leasing no new inputs is neither honoured nor rejected: capability detection becomes a function of how many inputs coin selection happened to add, not of what the wallet supports. Everything else new is minor — a pre-existing rollback hole in the function this PR rewrites, an untested zero-depth branch, an unbounded depth with no unit in the field name, and documentation that only describes the option at field level.
Two prior findings stay open at minor because the code did not change: the depth reported to clients is still request-derived rather than read back from the lease record, and the RPC-level capability probe is still vacuous for the type lnd actually passes in production, which means the error string the new RPC test asserts is not the one a real client sees.
Status of prior findings
- F1 addressed: The
.golangci.ymldiff is gone at this head — the file is not in the changed-file set, so the deadreplace-allow-listentries forbtcwallet/wtxmgrno longer exist alongside ago.modthat carries no matchingreplace.
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)
| ) | ||
| var expiration time.Time | ||
| if releaseAfterSpendConfs > 0 { | ||
| leaser, ok := w.(lnwallet.OutputLeaserWithOptions) |
There was a problem hiding this comment.
🟠 F5 (Major) — FundPsbt accepts a lease depth it never applies · lnrpc/walletrpc/psbt.go:80
A FundPsbt request with a non-zero input_release_after_spend_confs returns success without installing any release-after-spend lease, and without the unsupported-wallet error, whenever the call leases no new inputs. A caller that wired the field once and checks only err == nil believes spend-maturity leases are in force when none exist, and the same request against the same wallet succeeds or fails on capability depending only on whether coin selection had to add an input.
Why this matters
Both enforcement points are scoped to for idx := range outpoints: the w.(lnwallet.OutputLeaserWithOptions) assertion on this line and the LeaseOutputWithOptions call below it. fundPsbtCoinSelect calls lockAndCreateFundingResponse(packet, nil, changeIndex, ...) on the branch taken when the template already carries enough inputs, so outpoints is nil, the loop body never executes, and lockInputs returns nil, nil. The overwrite loop in lockAndCreateFundingResponse then has an empty rpcLocks to stamp, so the response carries an empty locked_utxos list — which is also the one response shape where a new client cannot distinguish a new server from an old server that ignored unknown field 15. A wallet with no OutputLeaserWithOptions support is likewise never probed on this path, so the fail-closed rule stated in the OutputLeaserWithOptions doc comment is not honoured for it. Hoisting the capability probe and the releaseAfterSpendConfs > 0 decision above the loop — or validating once at the FundPsbt entry point before template dispatch — makes the outcome depend on the wallet rather than on the input count. Note the defect is the absent error, not a forged lease: no weaker lease is installed on this path.
There was a problem hiding this comment.
Fixed. Capability resolution now happens before the outpoint loop. A non-zero request against an unsupported concrete controller fails even when outpoints is empty. Added TestLockInputsRejectsUnsupportedOptionsWithoutInputs for this exact path.
| @@ -74,9 +75,29 @@ func lockInputs(w lnwallet.WalletController, outpoints []wire.OutPoint, | |||
| return nil, fmt.Errorf("fetch outpoint info: %w", err) | |||
There was a problem hiding this comment.
🚫 Dismissed by @bhandras
🟡 F6 (Minor) — Lease rollback skipped on the outpoint-info error path · lnrpc/walletrpc/psbt.go:75
lockInputs returns on a FetchOutpointInfo failure without running the compensating release loop that sits a few statements below it, so at idx > 0 the leases already taken stay in place while the caller receives an error and no locks slice telling it which outpoints to release. The leak predates this PR, but the fourth commit rewrites the sibling rollback in this same function, and a non-zero depth may extend how long the orphaned leases survive — I cannot tell from the diff whether the bumped wtxmgr still honours duration as an upper bound for a spend-gated lease, which is why this is minor rather than major.
There was a problem hiding this comment.
Not worth changing in this PR. The fetch-error rollback gap predates this change. With no funded transaction returned, the new spend-gated state is not activated, so the already acquired lease still clears at its normal wall-clock expiration. This PR does not worsen the reachable outcome.
|
|
||
| // TestLockInputsForwardsReleaseAfterSpend verifies that FundPsbt's lease helper | ||
| // passes the requested confirmation depth to every selected input. | ||
| func TestLockInputsForwardsReleaseAfterSpend(t *testing.T) { |
There was a problem hiding this comment.
🟡 F7 (Minor) — Zero-depth legacy lease path is untested · lnrpc/walletrpc/psbt_test.go:72
The new if releaseAfterSpendConfs > 0 / else branch is only tested on the option-bearing side; nothing asserts that a zero depth against an options-capable wallet still routes to w.LeaseOutput instead of installing an options lease, which is the opt-in guarantee the PR leads with. leaseOptionsWallet already records leaseCalls, so a third case calling lockInputs(wallet, outpoints, &lockID, time.Hour, 0) and asserting require.Empty(t, wallet.leaseCalls) closes it.
There was a problem hiding this comment.
Fixed. Added TestLockInputsUsesLegacyPathForZeroDepth. It verifies one legacy LeaseOutput call and zero option-bearing calls against an options-capable wallet.
|
|
||
| // Keep each acquired input lease until its spending transaction reaches | ||
| // this confirmation count. A reorganization resets maturity progress. | ||
| uint32 input_release_after_spend_confs = 15; |
There was a problem hiding this comment.
🟡 F8 (Minor) — New lease option is undocumented at the RPC level · lnrpc/walletrpc/walletkit.proto:1664
The LeaseOutput, FundPsbt and ReleaseOutput service comments are unchanged, so the option is discoverable only from the field comments, and no method-level text states that a non-zero depth against an incapable wallet fails the whole call rather than degrading. Two interactions are also unstated: the precedence between expiration_seconds and the new depth, though the server applies both to the same lease, and whether ReleaseOutput — documented to unlock an output "if it remains unspent" — can reclaim a lease whose spending transaction has already been broadcast, which is exactly the question FundPsbt's closing NOTE about releasing locked UTXOs now raises.
There was a problem hiding this comment.
Fixed. Added method-level docs for LeaseOutput, FundPsbt, and ReleaseOutput. They now state capability failure, wall-clock versus confirmed-spend precedence, the no-new-input capability check, and explicit release of a retained spent-output lease.
| if req.ExpirationSeconds != 0 { | ||
| duration = time.Duration(req.ExpirationSeconds) * time.Second | ||
| } | ||
| releaseAfterSpendConfs := req.ReleaseAfterSpendConfs |
There was a problem hiding this comment.
🚫 Dismissed by @bhandras
🟡 F9 (Minor) — Lease depth accepted with no ceiling or unit · lnrpc/walletrpc/walletkit_server.go:513
The requested depth is forwarded with no upper bound, and neither the field name nor the type carries a unit, directly beneath expiration_seconds: transposing the two (expiration_seconds: 6, release_after_spend_confs: 600) marshals cleanly and yields a 600-block hold where a ten-minute lock was intended. An unbounded lease lifetime is already reachable today through expiration_seconds, so this is not a new class of exposure, but a sanity ceiling rejected before the coin-select lock is taken would keep the transposition diagnosable rather than silently persistent.
There was a problem hiding this comment.
Not worth changing. The depth is caller-controlled policy, just like the existing duration. A universal ceiling would be arbitrary and could reject legitimate long-lived protocols. The field and method docs now state that the value is a confirmation count.
| ) | ||
| } | ||
|
|
||
| // LeaseOutputWithOptions locks an output and applies optional persisted lease |
There was a problem hiding this comment.
🟡 F10 (Minor) — New lease method drops its locking and error contract · lnwallet/btcwallet/btcwallet.go:1152
BtcWallet.LeaseOutputWithOptions is now the method that actually takes the lease and returns wtxmgr.ErrOutputAlreadyLocked, but its doc comment restates neither the global-coin-selection-lock requirement nor the wtxmgr.ErrUnknownOutput / wtxmgr.ErrOutputAlreadyLocked sentinels that BtcWallet.LeaseOutput and WalletController.LeaseOutput both document. Both current callers do hold the lock, so this is a documentation gap rather than a live race, but OutputLeaserWithOptions is a new public capability and the next implementor has nothing in scope stating either obligation.
There was a problem hiding this comment.
Fixed. The optional interface and btcwallet implementation now document the global coin-selection-lock requirement and the same unknown-output and already-locked sentinel behavior as the legacy method.
|
|
||
| // Convert the lock leases to the RPC format. | ||
| rpcLocks := marshallLeases(locks) | ||
| for _, lock := range rpcLocks { |
There was a problem hiding this comment.
🚫 Dismissed by @bhandras
🟡 F2 (Minor) · lnrpc/walletrpc/walletkit_server.go:2313 · partially_addressed
The contract and field wording now match what the code actually guarantees — OutputLeaserWithOptions requires implementations to apply every non-zero option exactly or error, so a successful return does imply the depth was applied, and the echo is documented as capability confirmation rather than a backend readback. What remains is the field's split provenance: marshallLeases now reads lock.ReleaseAfterSpendConfs from the wallet's lease record, but lockInputs never sets it on the results it fabricates, so on the FundPsbt path the marshalled value is always zero and this loop patches the request value back over it. One struct field is therefore fed from two different sources depending on which RPC produced it, and a later refactor that drops this loop turns the FundPsbt echo silently into zero. Setting lock.ReleaseAfterSpendConfs = releaseAfterSpendConfs inside lockInputs next to lock.Expiration, and deleting this loop, makes the single assignment in marshallLeases meaningful for both callers.
There was a problem hiding this comment.
Not worth changing. The response is a capability acknowledgement after a successful call, not an independent integrity readback. The owning interface now requires every non-zero option to be applied exactly or return an error. A second query path would add another source of truth without a reachable in-tree implementation that can silently install a different value.
| func (b *BtcWallet) LeaseOutput(id wtxmgr.LockID, op wire.OutPoint, | ||
| duration time.Duration) (time.Time, error) { | ||
|
|
||
| return b.LeaseOutputWithOptions( |
There was a problem hiding this comment.
🟡 F3 (Minor) · lnwallet/btcwallet/btcwallet.go:1147 · unresolved
Still present as written: BtcWallet.LeaseOutput remains a thin delegation, so every pre-existing zero-option caller — channel funding, the sweeper, the plain LeaseOutput RPC — now reaches b.wallet.LeaseOutputWithOptions with an empty option slice instead of b.wallet.LeaseOutput. Your verification that upstream Wallet.LeaseOutput itself delegates to LeaseOutputWithOptions is recorded and I have no evidence against it, but it is not checkable from the review context (the module cache is unreadable here and btcwallet.go's blame came back fetch_failed), and nothing in this tree pins that equivalence against future upstream drift on a path the feature was never meant to touch. Keeping the no-option call on b.wallet.LeaseOutput and routing only opts.ReleaseAfterSpendConfs > 0 through the new method removes the question at no cost to the new behavior.
There was a problem hiding this comment.
Fixed. The legacy LND adapter path now calls b.wallet.LeaseOutput directly. Only the option-bearing path calls LeaseOutputWithOptions, so this no longer depends on proving the two upstream entry points equivalent.
| func (l *LightningWallet) LeaseOutputWithOptions(id wtxmgr.LockID, | ||
| op wire.OutPoint, duration time.Duration, | ||
| opts LeaseOutputOptions) (time.Time, error) { | ||
|
|
There was a problem hiding this comment.
🟡 F4 (Minor) · lnwallet/wallet.go:645 · partially_addressed
TestLightningWalletLeaseOutputWithOptions closes the coverage gap — the unsupported subtest now exercises the real production forwarding path and confirms it fails closed. The structural half is unchanged: because lnd passes *LightningWallet as the walletrpc Config.Wallet, and this method exists unconditionally, the w.(lnwallet.OutputLeaserWithOptions) probes in both RPC call sites always succeed in production, so the fail-fast-before-any-lease check is unreachable and the message a real client sees is this method's "wallet controller does not support output lease options" — not the "wallet does not support release-after-spend output leases" string that TestLeaseOutputRejectsUnsupportedOptions asserts against a bare legacyLeaseWallet. In lockInputs that error is additionally wrapped as "could not lease a lock on UTXO", attributing a wallet-capability problem to a specific outpoint. Collapsing the two messages into one shared sentinel would make the tested string and the production string the same one.
There was a problem hiding this comment.
Fixed. ResolveOutputLeaser unwraps LightningWallet and tests the concrete controller. Both RPC paths use it. The rejection tests now pass the production wrapper shape, so unsupported controllers fail at the same gate clients use.
4cc4d19 to
c6262a6
Compare
|
/gateway dismiss F2 The response is intentionally a capability acknowledgement after the owning interface succeeds. Every non-zero option must be applied exactly or return an error. An independent readback would expand the interface and create a second source of truth without a concrete silent-mismatch path. |
|
🚫 Dismissed F2 (minor) by @bhandras — no reason given Open findings on this PR: 🟡 F3 (minor) · 🟡 F4 (minor) · 🟠 F5 (major) · 🟡 F6 (minor) · 🟡 F7 (minor) · 🟡 F8 (minor) · 🟡 F9 (minor) · 🟡 F10 (minor) |
|
/gateway dismiss F6 This is pre-existing. A fetch failure returns no funded transaction, so the spend-gated state is not activated and prior acquisitions still clear at their ordinary wall-clock expiry. The PR does not worsen the reachable outcome. |
|
🚫 Dismissed F6 (minor) by @bhandras — no reason given Open findings on this PR: 🟡 F3 (minor) · 🟡 F4 (minor) · 🟠 F5 (major) · 🟡 F7 (minor) · 🟡 F8 (minor) · 🟡 F9 (minor) · 🟡 F10 (minor) |
|
/gateway dismiss F9 The confirmation depth is caller-controlled lease policy, like the existing duration. A universal ceiling would be arbitrary and reject valid long-lived protocols. The field and method docs identify the unit and lifecycle. |
|
🚫 Dismissed F9 (minor) by @bhandras — no reason given Open findings on this PR: 🟡 F3 (minor) · 🟡 F4 (minor) · 🟠 F5 (major) · 🟡 F7 (minor) · 🟡 F8 (minor) · 🟡 F10 (minor) |
|
/gateway re-review Head: c6262a6 Review only the changes since ce6eddc. F5 is fixed by resolving concrete-controller capability before the outpoint loop, with a no-input regression. F3 now keeps the exact legacy adapter call. F4 uses the same concrete-controller resolver in both production RPC paths and wrapper-shaped tests. F7, F8, and F10 have tests or docs. F2, F6, and F9 were dismissed with reachability and scope reasons. Stop if no concrete correctness, funds-safety, security, or severe reliability defect introduced or worsened by this PR remains. Final assessment: BLOCKED, READY WITH FOLLOW-UPS, or READY. |
|
❌ gateway re-review failed: code-review skill failed across all auth modes: You've hit your session limit · resets 4:50pm (UTC) See workflow logs for details. |
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 2 findings
🔴 0 Blocker · 🟠 1 Major · 🟡 1 Minor · 🔵 0 Nit
Summary
All seven live prior findings are addressed at c6262a64. The .golangci.yml allowlist is gone, the capability probe in lockInputs is hoisted above the outpoint loop with a no-input regression test, ResolveOutputLeaser gives both RPC entry points a real concrete-controller gate that the rejection tests now exercise through the production LightningWallet shape, the zero-depth legacy route has a test, and the proto method docs and LeaseOutputWithOptions contract now state the fail-closed rule, the wall-clock/confirmed-spend precedence and the lock/sentinel obligations. F2, F6 and F9 were dismissed and are not restated.
The F3 remediation introduced one regression: splitting the option-bearing path out of BtcWallet.LeaseOutput left the in-memory double-lock guard behind in the new method, so the legacy path every pre-existing lease caller uses — channel funding, the plain LeaseOutput RPC, the zero-depth FundPsbt route — no longer performs it. Restoring it is a three-line change and does not touch the new behavior. Separately, the wall-clock expiry still governs a retained lease until its spend confirms, so a caller that extends the lease while waiting may need to repeat the depth; that interaction is undocumented.
Final assessment: BLOCKED on F11. F12 is a follow-up.
Status of prior findings
- F1 addressed: No
.golangci.ymlinfiles[]at this head and noreplacefor btcwallet or wtxmgr ingo.mod— the stale allowlist entries and the fork replacements are both gone. - F3 addressed:
lnwallet/btcwallet/btcwallet.go:1147— the no-option path now callsb.wallet.LeaseOutput(id, op, duration)directly, so the change no longer depends on the two upstream entry points being equivalent. See F11 for what the split left behind. - F4 addressed: The unconditional forwarder is replaced by
ResolveOutputLeaser(lnwallet/wallet.go:640), which unwraps*LightningWalletand asserts on the concrete controller; both RPC call sites use it, andTestResolveOutputLeaser,TestLockInputsRejectsUnsupportedLeaseOptionsandTestLeaseOutputRejectsUnsupportedOptionsnow drive the production wrapper shape. - F5 addressed:
lnrpc/walletrpc/psbt.go:51-62resolves the capability beforefor idx := range outpoints, so a non-zero depth against an incapable wallet now errors even when coin selection adds no input;TestLockInputsRejectsUnsupportedOptionsWithoutInputscovers exactly that call (lockInputs(wallet, nil, nil, time.Hour, 6)). - F7 addressed:
TestLockInputsUsesLegacyPathForZeroDepthassertslegacyCalls == 1andrequire.Empty(wallet.leaseCalls)against an options-capable wallet, pinning the opt-in guarantee. - F8 addressed:
lnrpc/walletrpc/walletkit.protonow carries method-level text onLeaseOutput,ReleaseOutputandFundPsbtstating the capability failure, the wall-clock-until-confirmed-spend precedence, the no-new-input capability check, and that a retained lease can still be released after its spend is observed; the generated_grpc.pb.goand Swagger copies match. - F10 addressed:
lnwallet/btcwallet/btcwallet.go:1150-1155documentswtxmgr.ErrUnknownOutput,wtxmgr.ErrOutputAlreadyLockedand the global coin-selection-lock requirement, andOutputLeaserWithOptionsinlnwallet/interface.go:232repeats both obligations for future implementors.
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)
| func (b *BtcWallet) LeaseOutput(id wtxmgr.LockID, op wire.OutPoint, | ||
| duration time.Duration) (time.Time, error) { | ||
|
|
||
| return b.wallet.LeaseOutput(id, op, duration) |
There was a problem hiding this comment.
🟠 F11 (Major) — LeaseOutput no longer rejects in-memory-locked outputs · lnwallet/btcwallet/btcwallet.go:1147
BtcWallet.LeaseOutput no longer refuses to lease an outpoint that is locked by btcwallet's in-memory locker, so every pre-existing caller — channel funding through chanfunding's CoinLeaser, the plain LeaseOutput RPC, the zero-depth lockInputs route — now receives a successful persisted lease where the pre-PR code returned wtxmgr.ErrOutputAlreadyLocked. Two subsystems can then each believe they hold that input.
Why this matters
The guard was not deleted, it was relocated: the diff's context lines show if b.wallet.LockedOutpoint(op) { return time.Time{}, wtxmgr.ErrOutputAlreadyLocked } sitting inside LeaseOutput at the base SHA, and after the split it belongs to LeaseOutputWithOptions while LeaseOutput is reduced to a bare return b.wallet.LeaseOutput(id, op, duration). This was not part of the F3 remediation as described ("the legacy LND adapter path now calls b.wallet.LeaseOutput directly") — routing zero-option calls past LeaseOutputWithOptions did not require dropping the check, and the option-bearing path is unaffected either way.
How bad it is depends on whether anything sets the in-memory lock on this wallet: nothing in the loaded btcwallet.go or lnwallet/wallet.go calls LockOutpoint (LightningWallet.LockedOutpoints reads lnd's own lockedOutPoints map, a separate mechanism), and I cannot see the btcwallet dependency or the remote-signing wrapper from this context. If no caller sets it today, this is a silently disarmed guard rather than a live conflict — but the comment states the invariant it was protecting, and LeaseOutput is high-reach, so the next in-memory locker inherits the bug. Restoring the three guard lines at the top of LeaseOutput (or having LeaseOutput delegate to a shared helper that keeps them) returns the exact pre-PR behavior. A regression test asserting ErrOutputAlreadyLocked from LeaseOutput when the mock reports LockedOutpoint(op) == true would pin it.
There was a problem hiding this comment.
Fixed and folded into eeb239f. Restored the LockedOutpoint guard in the legacy BtcWallet.LeaseOutput path before calling btcwallet. Added TestLeaseOutputRejectsInMemoryLock, which asserts wtxmgr.ErrOutputAlreadyLocked and verifies the persisted lease call is not reached. The full lnwallet/btcwallet package passes.
| lockID, *op, duration, leaseOpts, | ||
| ) | ||
| } else { | ||
| expiration, err = w.cfg.Wallet.LeaseOutput( |
There was a problem hiding this comment.
🟡 F12 (Minor) — Extending a lease can drop its spend-maturity depth · lnrpc/walletrpc/walletkit_server.go:533
Because the wall-clock expiration still governs a retained lease until its spend confirms, a caller holding an input across a long broadcast-to-confirmation window must re-invoke LeaseOutput to extend it, and an extension that omits release_after_spend_confs takes this zero-depth branch and re-locks through the option-less path — I cannot tell from the reviewed context whether wtxmgr's re-lock preserves or clears the previously persisted release-after-spend flag, and if it clears it the extension silently reverts the input to ordinary wall-clock semantics, which is the exact invariant the PR exists to protect (this would be a major in that case). The cheap fix is a sentence on the LeaseOutput method doc stating that successive invocations must repeat the depth, next to the existing "expiration of the lock can be extended" text.
There was a problem hiding this comment.
Addressed in 27feb02. The merged wtxmgr behavior already preserves the existing confirmation depth and spend height for a same-owner renewal when the option is omitted (releaseAfterSpendConfsSet is false). I updated the LeaseOutput RPC documentation to state that same-ID renewal with zero depth preserves an existing non-zero depth, then regenerated the RPC outputs. Callers do not need to repeat the depth.
c6262a6 to
cd3310b
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review
No findings.
Status of prior findings
- F1 addressed: The stale
gomoddirectivesallowlist entries are gone —.golangci.ymlis no longer in the changed-file set, andgo.modcontains noreplacefor btcwallet or itswtxmgrsubmodule. - F3 addressed: Fixed at
lnwallet/btcwallet/btcwallet.go:1153— the legacy path is now a directreturn b.wallet.LeaseOutput(id, op, duration), so no claim about upstream zero-option equivalence is load-bearing. - F4 addressed:
ResolveOutputLeaseratlnwallet/wallet.go:637unwrapsLightningWalletand probes the concrete controller, replacing the unconditional wrapper forwarder; both rejection tests now pass the production&lnwallet.LightningWallet{WalletController: ...}shape, so the gate under test is the one clients hit. - F5 addressed: The capability probe and the
releaseAfterSpendConfs > 0decision are hoisted abovefor idx := range outpointsinlnrpc/walletrpc/psbt.go:51, so the outcome depends on the wallet rather than the input count.TestLockInputsRejectsUnsupportedOptionsWithoutInputspins the nil-outpoints path thatfundPsbtCoinSelectreaches vialockAndCreateFundingResponse(packet, nil, ...). - F7 addressed:
TestLockInputsUsesLegacyPathForZeroDepthasserts onelegacyCallsandrequire.Empty(t, wallet.leaseCalls)against an options-capable wallet, pinning the opt-in guarantee. - F8 addressed:
lnrpc/walletrpc/walletkit.protonow carries method-level text onLeaseOutput,FundPsbtandReleaseOutputcovering fail-closed behavior, wall-clock-until-confirmed-spend precedence, the no-new-input capability check, and release of a retained spent-output lease. - F10 addressed: Both
OutputLeaserWithOptionsinlnwallet/interface.goandBtcWallet.LeaseOutputWithOptionsnow restate the global coin-selection-lock requirement and theErrUnknownOutput/ErrOutputAlreadyLockedsentinels. - F11 addressed: The guard is restored:
lnwallet/btcwallet/btcwallet.go:1150retainsif b.wallet.LockedOutpoint(op) { return time.Time{}, wtxmgr.ErrOutputAlreadyLocked }ahead of the direct delegation, andLeaseOutputWithOptionscarries the same check, so no pre-existing lease caller had its double-lock refusal disarmed.
TestLeaseOutputRejectsInMemoryLock pins it from both directions — require.ErrorIs(t, err, wtxmgr.ErrOutputAlreadyLocked) plus require.False(t, backend.leaseCalled) proving the persisted lease call is never reached.
- F12 addressed:
lnrpc/walletrpc/walletkit.protonow states "Renewing the same lease with the same ID and a zero depth preserves an existing non-zero depth," regenerated consistently intowalletkit.pb.go,walletkit_grpc.pb.goand the swaggerdescription. That resolves the ambiguity as documentation, which is what the finding asked for; note the underlyingreleaseAfterSpendConfsSetbehavior is inwtxmgrand is not verifiable from the reviewed context, so this rests on the author's stated check against the merged implementation.
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 approve |
There was a problem hiding this comment.
✅ Approved — all prior findings addressed.
Findings recap
Findings:
F1(minor) — addressed: This PR removes the temporary fork replacements and go.mod contains noreplaceforgithub.com/btcsuite/btcwalletor itswtxmgrsubmodu…F2(minor) — partially_addressed: This loop overwrites everyUtxoLease.ReleaseAfterSpendConfsthatmarshallLeasesderived from the wallet'sListLeasedOutputResult, and …F3(minor) — addressed:BtcWallet.LeaseOutputnow routes all pre-existing lease callers — channel funding, the sweeper, the plainLeaseOutputRPC — through `b.w…F4(minor) — addressed: Defining this method on the wrapper makes*LightningWalletsatisfyOutputLeaserWithOptionsunconditionally, whether or not the embedded …F5(major) — addressed: AFundPsbtrequest with a non-zeroinput_release_after_spend_confsreturns success without installing any release-after-spend lease, and…F6(minor) — unresolved:lockInputsreturns on aFetchOutpointInfofailure without running the compensating release loop that sits a few statements below it, so …F7(minor) — addressed: The newif releaseAfterSpendConfs > 0 / elsebranch is only tested on the option-bearing side; nothing asserts that a zero depth against a…F8(minor) — addressed: TheLeaseOutput,FundPsbtandReleaseOutputservice comments are unchanged, so the option is discoverable only from the field comments…F9(minor) — unresolved: The requested depth is forwarded with no upper bound, and neither the field name nor the type carries a unit, directly beneath `expiration_s…F10(minor) — addressed:BtcWallet.LeaseOutputWithOptionsis now the method that actually takes the lease and returnswtxmgr.ErrOutputAlreadyLocked, but its doc …F11(major) — addressed:BtcWallet.LeaseOutputno longer refuses to lease an outpoint that is locked by btcwallet's in-memory locker, so every pre-existing caller …F12(minor) — addressed: Because the wall-clock expiration still governs a retained lease until its spend confirms, a caller holding an input across a long broadcast…
Dismissed:
Approved by @bhandras via /gateway approve. Last reviewed at cd3310b. Skill v0.5.0, model claude-opus-5.
|
Repinned to merged btcwallet, folded the review fixes, and replied to each finding. Gateway has approved the final head and CI is green after rerunning transient failures. Ready for maintainer re-review. |
cd3310b to
cfaf1e8
Compare
Pin btcwallet and wtxmgr to the upstream merge. This provides persisted release-after-spend lease behavior. Temporary fork replacements are no longer needed.
Define an optional output-leaser interface for persisted lease behavior beyond the existing WalletController contract. Forward it through LightningWallet and translate release-after-spend confirmation depth into btcwallet's lock option. Callers that use the existing LeaseOutput method keep the current behavior.
Add optional confirmation-depth fields to LeaseOutput and FundPsbt. Forward requests to configurable wallets and echo the accepted depth. Zero keeps existing behavior. Unsupported options fail closed.
Release each acquired lease with its actual owner ID. This fixes rollback for caller-provided lock IDs.
Document the new LeaseOutput and FundPsbt confirmation-depth option in the 0.22.0 release notes.
cfaf1e8 to
f4787f0
Compare
ziggie1984
added
the
backport-v0.21.x-branch
|
Should we also bound At the moment, this PR accepts any non-zero For this API, The two mechanisms do not necessarily need the same technical maximum, but if leases intentionally support a larger depth, it would be useful to document the reason for that policy divergence. |
|
As a follow-up, could we consider using this reorg-aware lease policy for internal wallet-input consumers as well, especially regular and batch channel funding? Today the behavior remains effectively external opt-in: batch channel funding calls This likely does not need to expand the scope of this PR, since internal adoption needs an explicit lifecycle policy:
A focused follow-up issue/PR for regular and batch channel funding would help prevent the RPC path and internal subsystems from evolving different reorg-protection designs. |
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
LGTM (left some non-blocking comments, feel free to addres in a follow-up)
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| type lockedOutpointWallet struct { |
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| type leaseOptionsController struct { |
| @@ -45,9 +45,22 @@ func verifyInputsUnspent(inputs []*wire.TxIn, utxos []*lnwallet.Utxo) error { | |||
| // (the passed outpoints), using either the optional custom lock ID and duration | |||
There was a problem hiding this comment.
Nit: can you create a more detailed commit message it is hard to digest
|
|
||
| ## RPC Additions | ||
|
|
||
| * WalletKit output leases can now [remain active until their spending |
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| const unsupportedLeaseOptionsErr = "wallet does not support " + |
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
Looks good, main req is not to hard code 6 confs, instead expose it as an option to the caller.
| []*base.ListLeasedOutputResult, error) { | ||
|
|
||
| var leaser lnwallet.OutputLeaserWithOptions | ||
| if releaseAfterSpendConfs > 0 { |
There was a problem hiding this comment.
Rather than this bool, with if we did something like releaseAfter fn.Option[uint8]? That way we don't hard code 6 confs everywhere, the caller picks, and in the future if we want to extend (eg: 1k blocks or w/e on testnet, we can do so).
There was a problem hiding this comment.
Totally misread this, doesn't hard code the confs, just uses -1 as a sentinel value. Ignore the comment.
| // ResolveOutputLeaser returns the optional output lease capability implemented | ||
| // by a wallet controller. It unwraps LightningWallet so the result reflects the | ||
| // concrete controller rather than the wrapper's method set. | ||
| func ResolveOutputLeaser(wallet WalletController) ( |
There was a problem hiding this comment.
So as is, this can't be used with a remote signer instance.
Eg:
ResolveOutputLeaser(&LightningWallet{WalletController: &RPCKeyRing{WalletController: &BtcWallet{}}})(remote signer embeds WalletController not the raw lnd btcwallet pointer)
There was a problem hiding this comment.
Should add a test that uses the rpc key ring.
| Expiration: uint64(lock.Expiration.Unix()), | ||
| PkScript: lock.PkScript, | ||
| Value: uint64(lock.Value), | ||
| ReleaseAfterSpendConfs: lock.ReleaseAfterSpendConfs, |
There was a problem hiding this comment.
Would adding a confirmation height be useful here?
|
Could also use a basic itest. |
3 participants
Change Description
Add opt-in release-after-spend semantics to WalletKit output leases and PSBT
funding.
Some transaction protocols need a selected input to remain unavailable until a
previously broadcast spend is buried by a configured number of blocks. A normal
wall-clock lease prevents concurrent selection while a transaction is assembled,
but it ends too early for that invariant: the wallet removes the lease when the
spend first confirms. If that block is disconnected, the input can become
available for a different transaction.
This PR exposes the wallet behavior added by
btcwallet#1351 without changing
existing callers.
RPC surface
LeaseOutputRequest.release_after_spend_confsapplies the behavior to anexplicitly leased output.
FundPsbtRequest.input_release_after_spend_confsapplies it to every inputselected and leased by
FundPsbt.LeaseOutputResponseandUtxoLeaseecho the requested depth after the walletsuccessfully applies it. A non-zero echo lets callers detect servers that do not
understand the option.
A zero value preserves the current wall-clock lease behavior.
Wallet boundary
lnwallet.OutputLeaserWithOptionsis an optional capability. It avoids wideningthe existing
WalletController.LeaseOutputmethod and keeps other walletimplementations source-compatible.
ResolveOutputLeaserunwrapsLightningWalletto inspect its concretecontroller, and the btcwallet controller maps the request to the persisted
btcwallet option.
When a non-zero depth is requested from a wallet that does not implement the
capability, the RPC returns an error. Silently falling back to an ordinary lease
would violate the caller's input-reuse invariant.
Partial-lock rollback fix
The fourth commit fixes an existing rollback bug exposed by the new test seam.
If leasing several inputs failed after a caller supplied a custom lock ID,
lockInputsattempted to release earlier inputs with LND's internal lock ID.Those releases could not match the acquired leases. The rollback now uses each
lease's actual
LockID.Dependency state
The root module pins the canonical upstream pseudo-versions at btcwallet merge
5c2f9a351a0a56cd4ac9de2cf1224b0cbfbe8add. The temporary fork replacementsare gone. The pins can move to tagged btcwallet and wtxmgr releases once those
are published.
Steps to Test
Local result: all five commands pass.
TestLockInputsForwardsReleaseAfterSpendverifies the requested depth reachesevery selected input.
TestLockInputsRejectsUnsupportedLeaseOptions,TestLockInputsRejectsUnsupportedOptionsWithoutInputs, andTestLeaseOutputRejectsUnsupportedOptionsprove both RPC entry points failbefore calling the legacy lease method, including when
FundPsbtacquires nonew input.
TestLockInputsRollbackUsesActualLockIDcovers the custom lock ID rollback regression.
Pull Request Checklist
Testing
Code Style and Documentation
make rpc.lnclicommands were added.