Repository navigation
Sign receiver inputs inside rust-payjoin finalize callback - #64
ValeraFinebits merged 3 commits into
Conversation
|
This stripping isn't done by the library already? If it isn't, it should be addressed there so that other implementations don't have the same problem. Confirm against the BIP77/BIP78 checklists. What prompted you to view manual stripping as something that needed to be done here? |
@DanGould I think that question is really for me rather than for @chavic. The underlying post-signing normalization was already in the plugin before this PR. I added it originally because without that normalization, the proposal PSBT was being rejected by the current rust-payjoin checks. What @chavic changed here was just to consolidate those existing steps into NormalizeContributedProposalSignatures() and make the invariant explicit in one place. |
@DanGould Sftlr, 🤔 That actually simplifies this a bit more. Was digging into BTCPay's SignAll method and trying to understand why it was necessary in the first place. Knowing the lib does this is good (new to me, but also makes it less of an issue than initially assumed)... Currently looking into a way to do this without having to use SignAll |
ac0eafc to
e678da6
Compare
@chavic I think removing SignAll is a good direction here, but the tests should catch up with that behavioral change. Since this PR now changes the signing path itself, I’d like to see targeted coverage for the per-input signing flow. |
@chavic Do these tests pass for you locally? |
The issue seemed to be in |
@chavic I think this may be the same flaky test tracked in #55 |
@ValeraFinebits @DanGould, looking into addressing this directly, it seems both this and the previous approaches can now be seen as compensations for the root cause. I still need to look into what forced this path |
To be clear, the immediate root cause here was that I originally went through |
Basically, just going to try and take advantage of the library as much as possible, 00cce6f tries to do that |
|
@ValeraFinebits, I think this is ready for another review |
ValeraFinebits
left a comment
ValeraFinebits
left a comment
There was a problem hiding this comment.
The PR title and description are stale. They still describe the intermediate NormalizeContributedProposalSignatures approach, but the final code now signs inside the rust-payjoin callback and removes the manual stripping entirely. Please update the PR metadata to match the actual final implementation.
@chavic Done. |
chavic
changed the title
The signer signed the whole proposal with SignAll over the store's derivation, which signs every wallet-owned input the receiver has keys for — including any the sender placed in the original PSBT — and then relied on cleanup to strip the signatures back off everything except the contributed inputs. Sign each contributed input individually instead, addressed by its outpoint and signed with the key derived from that coin's key path, so foreign and sender inputs are never signed in the first place. This mirrors BTCPayServer's core payjoin endpoint, which signs each selected UTXO individually. The post-signing normalization (finalize the contributed inputs, strip any sender-provided signatures, clear partial signatures and key paths) stays, now as a single NormalizeContributedProposalSignatures step.
The signer fetched the proposal PSBT through the public PsbtToSign() accessor, signed it externally, and handed it back through a pass-through finalize_proposal callback that ignored the PSBT the library supplied. That public accessor returns the raw payjoin PSBT with the sender's now-invalid signatures still attached, so the plugin had to strip those (plus partial signatures and key paths) by hand afterwards. Move signing into the finalize_proposal callback so it operates on the PSBT rust-payjoin hands the wallet — which already has the sender signatures cleared — and sign only the receiver's contributed inputs there. The library's prepare_psbt then drops partial signatures and key paths, so the manual stripping is removed entirely. The account key is resolved before finalize because the callback is synchronous. Because the signed PSBT now only exists after the callback runs, the expected settlement transaction is recorded from the resulting proposal after Save rather than from a pre-signed PSBT beforehand.
68b4589 to
8314dcb
Compare
This updates the receiver signing path so the plugin no longer signs the whole proposal and then strips sender input data afterwards.
The receiver now resolves the hot-wallet account key asynchronously, passes a synchronous ProcessPsbt callback into rust-payjoin's FinalizeProposal flow, and signs only the receiver coins that were contributed by this session. The callback fails if a persisted contributed input is missing from the PSBT or if that input cannot be finalized.
This leaves sender inputs untouched by the plugin signer and relies on rust-payjoin's finalize path for proposal cleanup instead of maintaining a separate manual normalization pipeline in the plugin.
The latest update also moves the callback signer tests into PayjoinReceiverProposalSignerTests and adds targeted coverage for the missing-input and cannot-finalize failure paths requested in review.