lnwallet/rpcwallet: keep zero-value prev outputs in remote-sign PSBT#10815
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 addresses a bug in the remote signing flow where zero-value previous outputs were being silently ignored. This behavior caused failures in signing flows that require zero-value inputs, such as BIP-322 message signing. By removing the incorrect value check and refactoring the logic into a testable helper, the wallet now correctly includes these outputs in the PSBT, ensuring downstream compatibility with taproot sighash requirements. 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 PR refactors the PSBT preparation logic for remote signers into a helper function, populateNonSignedInputWitnessUtxos, and enables support for zero-value UTXO entries required for BIP-322 signing flows. The changes include comprehensive unit tests for the new helper. Feedback highlights a placeholder PR number in the release notes that needs updating and recommends a nil check on the UTXO info to prevent potential runtime panics.
| [clarifies](https://github.com/lightningnetwork/lnd/issues/10568) the ZMQ | ||
| port-mismatch warnings so they no longer suggest that the connection failed. | ||
|
|
||
| * The [remote-signer PSBT prep](https://github.com/lightningnetwork/lnd/pull/PRNUM) |
There was a problem hiding this comment.
The link contains a placeholder PRNUM. Please update this with the actual pull request number once it is assigned.
| } | ||
|
|
||
| txIn := tx.TxIn[idx] | ||
| info, err := fetchInfo(&txIn.PreviousOutPoint) |
There was a problem hiding this comment.
While FetchOutpointInfo is generally expected to return a non-nil Utxo when no error occurs, it is safer to perform a nil check on info before accessing its fields to prevent potential panics.
info, err := fetchInfo(&txIn.PreviousOutPoint)
if err == nil && info != nil {
packet.Inputs[idx].WitnessUtxo = &wire.TxOut{
Value: int64(info.Value),
PkScript: info.PkScript,
}
continue
}
// The wallet doesn't know about this outpoint. Fall
// back to the caller-supplied PrevOutputFetcher.
Roasbeef
force-pushed
the
bip322-fix
branch
2 times, most recently
from
5e50e65 to
c5d5fcf
Compare
github-actions
Bot
added
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟢 Low (2 files — not counted toward severity)
AnalysisThis PR modifies The change is focused (67 additions / 36 deletions in the non-test source file) and is accompanied by a new test file (228 lines) that provides coverage for the modified logic. No severity bump was triggered: only 2 non-test/non-generated files changed (<20 threshold) and 114 non-test lines changed (<500 threshold). Expert review is warranted given the wallet/signing context. To override, add a |
| func populateNonSignedInputWitnessUtxos(packet *psbt.Packet, tx *wire.MsgTx, | ||
| signDesc *input.SignDescriptor, fetchInfo fetchOutpointInfoFn) { | ||
|
|
||
| for idx := range packet.Inputs { |
There was a problem hiding this comment.
Don't you think adding a quick length alignment check at the top makes sense here? It would make the loop completely defensive against out-of-bounds panics if a malformed packet ever slips through.
if len(packet.Inputs) != len(tx.TxIn) {
return
}
There was a problem hiding this comment.
The invariant len(packet.Inputs) == len(tx.TxIn) is guaranteed by psbt.NewFromUnsignedTx(tx) at the only call site two lines above. Validating an invariant the caller already establishes is noise, and if it ever did diverge, that's a programmer bug — panicking is the correct response, not a silent skip. The real smell is the dual-arg signature; a runtime check papers over it instead of fixing it
There was a problem hiding this comment.
Ah, got it! That makes total sense. Thanks for the thorough explanation!
|
LGTM. Solid fix for the BIP-322 edge case, and the unit tests are very thorough. I just left a quick suggestion on the input loop regarding a defensive length check. Aside from that, logic looks great! |
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
LGTM nice bugfix, pending Linter CI
|
Thx for the review, will fix CI then land. |
…ign prep Before forwarding a SignOutputRaw request to the remote signer instance, remoteSign rebuilds a PSBT from the unsigned transaction and annotates every input with a WitnessUtxo (so the downstream walletkit.SignPsbt call accepts it — taproot sighash computation requires the prev output of every input, not just the one being signed). For non-signed inputs the prep stage first asks the watch-only wallet about the outpoint via FetchOutpointInfo, then — when the wallet does not own or track the outpoint — falls back to the sign descriptor's PrevOutputFetcher. The fallback previously required `utxo.Value != 0`, which silently dropped legitimate zero-value entries on the floor and left the corresponding PSBT input bare. The walletkit.SignPsbt entry point on the remote signer then rejected the PSBT with "input (index=N) doesn't specify any UTXO info" because input N had neither a WitnessUtxo nor a NonWitnessUtxo annotation. BIP-322 (signing virtual transactions for message attestation) is the canonical hitter: its to_spend output is mandated by the BIP to be exactly value=0 with the message commitment as pk_script, and that output is referenced as input 0 of every BIP-322 to_sign transaction. Any caller that drives a BIP-322 sign through a remote-signer LND deployment was failing for this reason. The validation we actually want is that the fetched prev output is representable as a usable WitnessUtxo: non-nil and with a non-empty pk_script. Drop the Value check; the zero-value case is well-formed and the resulting PSBT input will serialize cleanly. The fetched-but- empty-pk_script case continues to be rejected (a WitnessUtxo with empty PkScript is malformed at PSBT serialization), and the warning log when no fallback resolves the outpoint is preserved verbatim. Lift the WitnessUtxo-population loop out of remoteSign into a package-level helper so the resolution policy is unit-testable without spinning up a real wallet + remote signer pair. The helper takes a fetchOutpointInfoFn callback that mirrors lnwallet.WalletController.FetchOutpointInfo. No behavior change for the wallet-owns-it path or the no-fallback path.
Cover the four resolution branches plus the BIP-322 regression case:
- wallet-owns-it: FetchOutpointInfo returns a Utxo, helper writes
the matching WitnessUtxo into the PSBT input.
- external-fallback: wallet returns ErrNotMine, helper writes the
WitnessUtxo from the sign descriptor's PrevOutputFetcher.
- zero-value-fallback: same as above with the fetched entry's Value
set to zero. This is the BIP-322 to_spend shape (input 0 of every
BIP-322 to_sign references a virtual prev whose Value is mandated
to be zero); the helper must populate the WitnessUtxo rather than
silently skip it.
- no-fallback: wallet returns ErrNotMine and no PrevOutputFetcher
is provided; the helper leaves the input bare and the warning log
fires (asserted only by absence of a populated WitnessUtxo).
- empty-pk_script-fallback: the fetcher returns a non-nil entry
with an empty PkScript; the helper rejects it as unusable (the
PSBT WitnessUtxo serializer requires a non-empty script) and
leaves the input bare.
The signed input (signDesc.InputIndex) is intentionally left untouched
by the helper — that input is the one the caller's main path will
populate later — and the tests cross-check that invariant on the
wallet-owns-it case.
Add the bug-fix entry under 0.21.0 with a link to the PR.
Roasbeef
added
the
backport-v0.21.x-branch
|
Successfully created backport PR for |
…21.x-branch [v0.21.x-branch] Backport #10815: lnwallet/rpcwallet: keep zero-value prev outputs in remote-sign PSBT
4 participants
In this PR, we fix a sharp edge in
RPCKeyRing.remoteSignthat silentlybroke any signing flow whose
to_signtx references a zero-value prevoutput. The canonical hitter is BIP-322 message signing, which mandates
that the virtual
to_spendoutput is exactlyvalue=0with the messagecommitment as
pk_script, and that output is referenced as input 0 ofevery
to_signtx handed toSignOutputRaw.Before forwarding the request,
remoteSignrebuilds a PSBT from theunsigned tx and annotates every non-signed input with a
WitnessUtxo,so the downstream
walletkit.SignPsbtcall can proceed (taproot sighashneeds every prev output, not just the one being signed). When the watch-
only wallet doesn't know an outpoint, the prep stage falls back to the
sign descriptor's
PrevOutputFetcher. That fallback used to requireutxo.Value != 0, which silently dropped legitimate zero-value entrieson the floor. The remote signer would then reject the resulting PSBT
with
input (index=N) doesn't specify any UTXO info, raised inwalletkit.SignPsbtprecisely because input N had neither aWitnessUtxonor aNonWitnessUtxo.The fix
We drop the
Value != 0guard. The only condition that actually mattersis that the fetched entry is usable as a
WitnessUtxo: non-nil with anon-empty
PkScript. A zero-valueWitnessUtxowith a well-formedPkScriptserializes cleanly into a PSBT, so there's nothing else tovalidate at this layer.
We also lift the population loop out of
remoteSigninto a package-level helper,
populateNonSignedInputWitnessUtxos, so the resolutionpolicy can be exercised in unit tests without standing up a full wallet
fetchOutpointInfoFncallbackthat mirrors
lnwallet.WalletController.FetchOutpointInfo. No behaviorchange for the wallet-owns-it path or the no-fallback path.
Tests
New unit tests in
lnwallet/rpcwallet/rpcwallet_test.gowalk througheach resolution branch. The wallet-owns-it case exercises the
FetchOutpointInfohappy path and cross-checks that the signed inputitself is left untouched. The external-fallback case has the wallet
return
ErrNotMineand resolves the prev via the fetcher. The zero-value-fallback case is the BIP-322 regression: same shape as external-
fallback, but the fetcher entry has
Value=0, and the test assertsthat
WitnessUtxolands on the PSBT instead of being silently skipped.Two more cover the unhappy paths: no fetcher attached (input left bare,
warning logged) and a fetcher entry with an empty
PkScript(rejectedas unusable, since a PSBT
WitnessUtxowith empty script is malformedat serialization).
Compat
No behavior change for the wallet-owns-it path, the no-fallback path,
or the non-zero-value fallback path. The only new behavior is that
legitimately zero-value entries from the supplied
PrevOutputFetcherland in the PSBT instead of being silently skipped.