Expose proposal txid stability on receiver states#1718
Conversation
Coverage Report for CI Build 29586661698Coverage increased (+0.09%) to 86.279%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
The current downstream solution to the missing signal, from the BTCPay Server payjoin plugin BTCPay pins the proposal's txid when the proposal is handed out So the plugin re-derives the predicate host-side: it extracts the fallback transaction and checks internal static bool HasNonWitnessInput(Transaction transaction)
{
return transaction.Inputs.Any(input => input.WitScript is null || input.WitScript == WitScript.Empty);
}That is a host-side copy of the exact check |
| /// Whether the payjoin proposal's transaction ID is knowable in advance. | ||
| /// | ||
| /// If every sender input is SegWit, the sender's final signatures live in | ||
| /// the witness, so the transaction ID computed from the unsigned proposal | ||
| /// remains valid once the sender re-signs — the receiver can record it | ||
| /// (e.g. to reconcile an incoming payment) and monitor the network for it. | ||
| /// | ||
| /// If any sender input is non-SegWit, the sender's final signature | ||
| /// rewrites that input's script_sig, which changes the transaction ID. | ||
| /// Any transaction ID derived from the proposal before the sender signs | ||
| /// will never appear on the network, and | ||
| /// [`Receiver<Monitor>::check_for_transaction`] concludes the session | ||
| /// without monitoring. Receivers that track payments by transaction ID | ||
| /// can use this to choose a policy up front: decline to contribute and | ||
| /// let the fallback pay, or reconcile that session by script instead of | ||
| /// by transaction ID. | ||
| pub fn proposal_txid_is_stable(&self) -> bool { | ||
| !self.state.fallback_tx().input.iter().any(|txin| txin.witness.is_empty()) | ||
| } | ||
| } |
There was a problem hiding this comment.
P2SH-P2WSH has a non-empty witness but its redeemScript push still lands in scriptSig, so it'd cause this function to return the wrong value.
I think input.iter().all(|txin| txin.script_sig.is_empty()) covers all of the cases you want.
|
Funny here rust-payjoin/payjoin/src/core/receive/v2/mod.rs Lines 1722 to 1723 in f3658a5 |
I assume it applies to any receiver that records or watches payments by transaction ID |
|
Seems dependencies changed beneath us; the Windows failure is tokio 1.53.0. The C # workflow builds without a lockfile, resolves to whatever seems latest. Actually, seems all the other language workflows are floating too. @DanGould worth a separate PR? |
|
The Windows csharp failure here is unrelated to this PR: the csharp CI resolves dependencies fresh on every run and picked up tokio 1.53.0, which uses |
Monitor::check_for_transaction already knows the proposal's transaction ID cannot be tracked when the sender contributed non-SegWit inputs, and concludes the session as PayjoinProposalSent without checking the network. That knowledge arrives too late for receivers that record the expected transaction ID when handing out the proposal, e.g. to reconcile an incoming payment: with a non-SegWit sender input the recorded ID is silently wrong, because the sender's final signature rewrites the script_sig and changes the ID. Expose the predicate Monitor uses as proposal_txid_is_stable() on every state that holds the fallback transaction, and mirror it in payjoin-ffi. Receivers can now pick a policy as soon as the original payload is known: decline to contribute and let the fallback pay, or reconcile that session by script instead of by transaction ID.
proposal_txid_is_stable() tested the wrong axis: it looked for an empty witness, but what changes the transaction ID is the script_sig. A P2SH-wrapped SegWit input (P2SH-P2WPKH/P2SH-P2WSH) finalizes with a non-empty witness AND a script_sig carrying the redeem script push, and the script_sig is part of the txid preimage. The witness test called such inputs stable, so receivers would record and monitor a transaction ID that can never appear on the network. Test for empty script_sigs instead: an input whose final script_sig is empty is exactly one whose signature data lives entirely in the witness. This also corrects Receiver<Monitor>::check_for_transaction, which previously polled forever for nested SegWit senders and now concludes the session as PayjoinProposalSent, matching the existing legacy-input behavior. The canonical BIP78 test vectors spend P2SH-P2WPKH, so the monitor typestate test was itself asserting the bug when it called those fixtures stable. Give it a native P2WPKH fixture pair for the monitorable paths, and add a nested SegWit regression test proving the txid changes on finalization while the witness is non-empty.
The exported accessor's documentation still described the witness-based predicate. Match the core method: stability requires every sender input to finalize with an empty script_sig, which nested SegWit inputs do not.
e9638ff to
e3eec67
Compare
|
Just forced push with lease |
4 participants
This is the 1.0-sized companion to the problem #1692 solves properly. #1692 makes non-SegWit sessions monitorable by classifying spends of the contested outpoints; this PR only makes the existing limitation visible at the right moment, without touching any frozen type. It's intended as the machine-readable form of the disclaimer suggested as the 1.0 mitigation in #1692 (document that
check_for_transactiondoesn't properly support non-SegWit inputs).Today,
Monitor::check_for_transactionknows the proposal's transaction ID cannot be tracked when the sender contributed non-SegWit inputs, and concludes the session asPayjoinProposalSentwithout checking the network. That knowledge arrives too late for receivers that do settlement accounting. A receiver that credits payments (a merchant server, an exchange) typically records the expected final transaction ID when it hands out the proposal — for SegWit-only sender inputs,unsigned_tx.compute_txid()stays valid once the sender re-signs, so pinning it enables exact reconciliation later. With any non-SegWit sender input the pinned ID is silently wrong: the sender's final signature rewrites the script_sig and changes the ID. Nothing warns the host at the moment the expectation is recorded; the first signal is the blind conclude at Monitor time.This exposes the predicate Monitor already uses as
proposal_txid_is_stable()on every state that holds the fallback transaction (theHasFallbackTxstates), refactorscheck_for_transactionto use it (behavior unchanged), and mirrors it in payjoin-ffi. Receivers can then pick a policy as soon as the original payload is checked: decline to contribute and let the fallback pay, or reconcile that session by script/outpoint instead of by transaction ID. The BTCPay Server payjoin plugin currently implements the conservative policy by parsing the original transaction host-side (ValeraFinebits/btcpayserver-payjoin-plugin#78); this accessor lets any wallet make the same call without re-deriving protocol internals.Relationship to #1692: strictly additive and shape-neutral, so it neither constrains nor competes with contested-outpoint classification. If/when that lands, this accessor degrades gracefully from "necessary policy input" to "convenience": hosts that reconcile by pinned transaction ID still want to know stability up front, while hosts on
classify()won't need it for monitoring at all.Disclosure: co-authored by Claude Code
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.