Repository navigation
Classify Payjoin settlements on the Monitor typestate - #1971
xstoicunicornx wants to merge 5 commits into
Conversation
Coverage Report for CI Build 40710937273Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.3%) to 87.744%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
acc4a2c to
9d7f5b3
Compare
DanGould
left a comment
There was a problem hiding this comment.
My main issues are allow(deprecated) in tests without exercising the new interface where the opportunity exists, deprecating proposal_txid_is_stable for the new behavior, and getting the fallback_txid through #1949 instead of an extra method.
proposal_txid_is_stable can be deprecated here in favor of classify, too. Ignores receiver inputs anyway, which is a problem for actually classifying as stable.
Monitor -> cancel() -> close() writes Aborted after the proposal was posted. Both docs need to drop the "before posting" claim.
5ca8c70 to
3cbff7b
Compare
|
I think I got all follow ups, ready for re-review. |
DanGould
left a comment
There was a problem hiding this comment.
incomplete review, but thoughts that came up while reading rust-bitcoin's policy re: deprecation & versioning
| /// match. When [`expected_payjoin_txid`](Monitor::expected_payjoin_txid) | ||
| /// is [`ExpectedTxid::Unstable`] it closes the session without checking. | ||
| /// | ||
| /// Deprecated since payjoin 1.3.0: use [`classify`](Monitor::classify) and |
There was a problem hiding this comment.
this is gonna go stale immediatel and should be left out of comments
There was a problem hiding this comment.
I didn't remove the full deprecation comment (since I think that this is the only indicator of deprecation for check_for_transaction in ffi) but just removed the payjoin version.
| /// transaction ID: any ID derived from the proposal before the sender | ||
| /// signs will never appear on the network, and monitoring concludes the | ||
| /// session without checking. | ||
| /// Deprecated since payjoin 1.3.0: use `expected_payjoin_txid` on |
There was a problem hiding this comment.
Again, just removed the version.
DanGould
left a comment
There was a problem hiding this comment.
Thank you for addressing my prior concerns. Your thinking around proposal_txid_is_stable sounds right to me as well.
One thing before merge. Master now wants a payjoin-ffi/CHANGELOG.md line under Unreleased whenever payjoin-ffi/src changes (5db2b57), so this would fail CI rebased on master as-is today even though the rebase is otherwise clean.
The Monitor docs tell the wallet to watch contested_prevouts (mod.rs#L1830-L1834, #L1855-L1863). Most wallets don't need to. The fallback pays the receiver's address, and the proposal spends the receiver's inputs or pays its output, so ordinary wallet sync by script finds both. Your find_settlement doc says the same (v2/mod.rs#L1244-L1248), which contradicts "this is the only way to find the proposal" in the contested_prevouts doc. contested_prevouts is needed when the proposal pays only substituted outputs and spends no receiver input, and to catch a sender spend that is neither transaction. Could the docs lead with passing what the wallet finds to classify, and describe contested_prevouts for those cases? The bindings carry the same text (ffi#L2001-L2006, #L2023-L2032).
Since this needs to be rebased to satisfy the ffi changelog thing, perhaps you could change the deprecation notice to use TBD as well when addressing these nits? This is money. Almost there!
3cbff7b to
d834864
Compare
|
I think CI is failing for this issue: #1985 Otherwise I think ready for re-review. Sorry the rebase commit got lost here, you can find it on my branch https://github.com/xstoicunicornx/rust-payjoin/tree/monitor-revamp-rebase |
There was a problem hiding this comment.
cACK, this is finally looking like the right shape for the Monitor API. I have some code feedback, and I'm not totally sure what to do about the SessionOutcome change. The current approach of always closing with PayjoinProposalSent feels a bit hacky, but I don't want to force a breaking change now.
| pub fn expected_payjoin_txid(&self) -> ExpectedTxid { | ||
| if !self.sender_script_sigs_are_empty() { | ||
| return ExpectedTxid::Unstable; | ||
| } | ||
| // The posted proposal carries the receiver's finalized inputs and no sender signatures, | ||
| // and the sender adds only witnesses, so extracting it yields the broadcast script_sigs. | ||
| let payjoin_tx = | ||
| self.state.psbt_context.payjoin_psbt.clone().extract_tx_unchecked_fee_rate(); | ||
| ExpectedTxid::Stable(payjoin_tx.compute_txid()) | ||
| } | ||
|
|
||
| /// The sender's original transaction, which it may broadcast instead of the proposal. | ||
| pub fn fallback_tx(&self) -> bitcoin::Transaction { self.state.fallback_tx() } |
There was a problem hiding this comment.
I'm not sure we need to make either of these helpers pub, since classify takes care of classification and it's already mentioned that the wallet would automatically detect either a fallback or payjoin tx anyway?
There was a problem hiding this comment.
Yes a wallet can detect a receive, but that doesn't mean the wallet can detect which payjoin session the tx relates to, or that it relates to a payjoin receive at all. Is it acceptable to force wallets to run all received txs through all active Monitor payjoin session's classify method?
| let unsigned_txid = unsigned_tx.compute_txid(); | ||
| let psbt_context = &self.state.psbt_context; | ||
|
|
||
| if unsigned_txid == psbt_context.original_psbt.unsigned_tx.compute_txid() { |
There was a problem hiding this comment.
could we use self.state.fallback_tx.compute_txid()?
There was a problem hiding this comment.
I don't think so because we need to strip the sigs out before comparing the txids to account for non-segwit sender inputs where sender could re-sign same fallback tx (and we probably don't want to classify these as Unrecognized).
|
|
||
| /// Close the session once the wallet no longer needs to track settlement. | ||
| pub fn close(self) -> TerminalTransition<SessionEvent, ()> { | ||
| TerminalTransition::new(SessionEvent::Closed(SessionOutcome::PayjoinProposalSent), ()) |
There was a problem hiding this comment.
I guess this will result in some unused SessionOutcome variants? I also wonder if we might want to rename this one, but am aware that's a breaking change.
close() could take a SettlementOutcome to turn into a SessionOutcome? IDK if that's desirable.
There was a problem hiding this comment.
Actually I think SessionOutcome::Unrecognized(Txid) would be the right shape for closing the session, so that there is a permanent record of the txid that settled it regardless of what it was. But Unrecognized is even more misleading than PayjoinProposalSent and renaming it is a breaking change even if it's never written as of today.
There was a problem hiding this comment.
I think one of the guiding principles for this API change is that we explicitly avoid storing any permanent settlement outcome information. Settlement should strictly be handled by the wallet and we should avoid trying to insert ourselves in that process.
See #1692 (comment) for background on this direction regarding approach.
d834864 to
3612143
Compare
|
Rebasing to resolve merge conflict |
3612143 to
38a4ab1
Compare
|
Ready for re-review. |
2e84523 to
8c1da26
Compare
The staged Dart package takes payjoin-ffi from the pinned revision but payjoin and payjoin-mailroom from crates.io. Staging only switched a crate to the revision when its required version was missing from crates.io. A pull request that changes core without a version bump, which is how core changes normally land, kept the crates.io core, so the reproducible-linux job compiled the pull request's payjoin-ffi against an older payjoin and failed. payjoin#1971 hits this with new Monitor receiver methods. Add a --core-from-revision option that always patches the sibling release crates to the pinned revision and writes the UNPUBLISHED_CORE marker publish.sh refuses, and pass it on pull request runs only. Release runs keep staging against crates.io, so an unpublished core still fails there before anything is published. This also drops the crates.io API probing.
6f369bd to
902166e
Compare
1056adb to
c61e94c
Compare
spacebear21
left a comment
There was a problem hiding this comment.
I had my bot review this closely and posted the most relevant findings here
| fn original_psbt(&self) -> &bitcoin::Psbt; | ||
|
|
||
| impl FallbackTx for super::MaybeInputsOwned { | ||
| fn fallback_tx(&self) -> bitcoin::Transaction { | ||
| self.original.psbt.clone().extract_tx_unchecked_fee_rate() | ||
| self.original_psbt().clone().extract_tx_unchecked_fee_rate() | ||
| } | ||
|
|
||
| /// The finalized Payjoin proposal, once the sender may have received it. | ||
| fn payjoin_psbt(&self) -> Option<&bitcoin::Psbt> { None } | ||
| } |
There was a problem hiding this comment.
This extends the sealed trait leak from #1994 with original_psbt and payjoin_psbt now also being unintentionally exposed in the public API. These all build in a downstream crate:
pub fn fallback<S: HasFallbackTx>(r: &Receiver<S>) -> Transaction { r.fallback_tx() }
pub fn original<S: HasFallbackTx>(r: &Receiver<S>) -> Psbt { r.original_psbt().clone() }
pub fn proposal<S: HasFallbackTx>(r: &Receiver<S>) -> Option<Psbt> { r.payjoin_psbt().cloned() }I'm also not sure original is necessary, since it's only used for getting the fallback which is already exposed here.
There was a problem hiding this comment.
Good catch. I pulled the seal commit from #1994 into the base of this branch unchanged, so FallbackTx is pub(crate) here too. It's the same patch, so it drops out when this rebases onto master after #1994 merges. I also added compile_fail doctests for original_psbt and payjoin_psbt next to the one for fallback_tx. They're separate tests because one test with all three calls would still pass if only one leaked.
On original_psbt not being necessary: classify_tx uses it to read the contested prevouts and the original's unsigned txid. Getting those from fallback_tx() would clone and extract the whole PSBT on every call, and the script_sigs would need stripping again. With the trait sealed it's no longer public, so it just stays as an internal accessor.
Drafted with AI assistance.
| // A session that posted its proposal is exempt too, whether still in Monitor or cancelled from | ||
| // it into PendingFallback: it has no protocol work left, the transaction it awaits can confirm | ||
| // long after the mailbox is gone, and a wallet gives up on it by cancelling past expiration. | ||
| if !matches!(receiver, ReceiveSession::Closed(_)) && !history.posted_proposal() { |
There was a problem hiding this comment.
A side effect of this a wallet that relied on the previous behavior to cancel the transaction and broadcast (like the LDK PR does now) would now never know that it needs to cancel and broadcast the transaction. I think the way to address this is to have check_for_transaction keep the old outcome by returning a fatal transition to PendingFallback if the expiration has passed, and only callers that migrate take on the new responsibility.
There was a problem hiding this comment.
I didn't like this option very much because even though it didn't result in an API change it still resulted in a change in how it would need to be handled, which is arguably worse because there is no indicator at compile time that this is the case. So it feels a little dishonest.
I opted to just retain the current deficiency in master which means expired PayjoinProposal and Monitor typestates will error on replay and produce the fallback. I tried many other options to trying to make replaying event log only hit expiration error for the applicable Monitor flows but each had its own slightly messy warts. I think it will be cleaner to add this expiration exclusion with the next major version bump.
Follow up uncovered during this review, FFI's ReceiverReplayError exposes is_expired() but not expiry_fallback_tx(), so binding users can't get the fallback from an expired replay. I'll open a separate PR for it.
| }; | ||
| } | ||
|
|
||
| macro_rules! impl_settlement_classification_for_receiver { |
There was a problem hiding this comment.
I now realize that for FFI, callers would need to write a classify_tx match over all 9 Receiver variants... this is not ideal.
Perhaps classify_tx should live on SessionHistory after all?
There was a problem hiding this comment.
I opted to move this to the general ReceiverSession and have classify_tx return a new Settlement which allows for direct closing of the session. This allows the caller to classify the settlement outcome without consuming the session and gating the closing of the session on the predicate that some settlement outcome was actually reached.
Without the coupled Settlement::close() method, the caller still would need to match on the replayed state to act on the result. Classification becomes uniform, but closing and cancelling don't. The previous match over every variant just moves from classifying to acting. For example if classify_tx was implemented on replay_event_log:
let (session, history) = replay_event_log(&persister)?;
let Some(outcome) = history.classify_tx(&tx) else { return Ok(()) };
if confirmations < MY_BAR { return Ok(()) }
match session {
ReceiveSession::Monitor(r) => r.close().save(&persister)?,
ReceiveSession::MaybeInputsOwned(r) => r.cancel().save(&persister)?.close().save(&persister)?,
ReceiveSession::MaybeInputsSeen(r) => r.cancel().save(&persister)?.close().save(&persister)?,
// ...OutputsUnknown, WantsOutputs, WantsInputs, WantsFeeRange, ProvisionalProposal, PayjoinProposal
ReceiveSession::HasReplyableError(r) => match r.cancel().save(&persister)? {
// MaybeTerminalTransition: PendingFallback if the original arrived, otherwise closed
...
},
ReceiveSession::PendingFallback(r) => r.close().save(&persister)?,
// classify_tx returns None for these, so they can't get here
ReceiveSession::Initialized(_) | ReceiveSession::UncheckedOriginalPayload(_) | ReceiveSession::Closed(_) => {}
}| let unsigned_txid = unsigned_tx.compute_txid(); | ||
|
|
||
| if unsigned_txid == self.state.original_psbt().unsigned_tx.compute_txid() { | ||
| return Some(SettlementOutcome::Fallback); |
There was a problem hiding this comment.
classify_tx returning Fallback before Monitor is new and useful. But nothing tells the wallet what to do next. The answer is cancel(), then close(), with no broadcast because the fallback is already out. The CLI doesn't do it either; its commit says that's left for later.
There was a problem hiding this comment.
With the new API the next step is much clearer - use Settlement::close() to close the session.
| /// | ||
| /// Returns a [`MonitorTransition`] that, once persisted, completes | ||
| /// the session if a transaction is found. | ||
| pub fn check_for_transaction( |
There was a problem hiding this comment.
It seems acceptable to break payjoin-ffi only since it's <1.0.
78dd6a8 to
c48f054
Compare
|
I have not had a chance to fully review the changes but I wanted to push them out incase there was any immediate feedback since there are significant changes (though I do think the changes remove a lot of the inconsistencies and clarify the flow). Will be afk for large part of the day tomorrow but trying to keep this PR moving forward. |
The sealed `FallbackTx` trait was `pub` inside a private module, which makes it unnameable from outside the crate but still reachable through the public `HasFallbackTx` bound. Rust lets generic code call the methods of a reachable supertrait, so a downstream function bounded on `S: HasFallbackTx` could call `receiver.fallback_tx()` and read the fallback transaction from any state, bypassing the `cancel` path and the Cancelled event it records. Narrow the trait to `pub(crate)`. The compiler then rejects the call with E0624 even inside a generic function, while code written against `HasFallbackTx` keeps compiling. A private supertrait on a public trait has only been the `private_bounds` lint since Rust 1.74, which the MSRV of 1.85 covers, so the two allows mark the mismatch as deliberate. A compile_fail doctest guards the seal; its error code is enforced on nightly and ignored on stable.
After posting the proposal the receiver still does not know how the sender's inputs were spent. The proposal and the fallback double-spend them, so the receiver holds the only record that can tell which transaction represents which. Expose that as a pure classify_tx over a transaction the wallet found. Wallets should pass every transaction they receive to classify_tx on each open session, rather than probing for predicted txids. A session in any state with a fallback can see the fallback or a re-spend of the sender's inputs, not only one in Monitor. A transaction unrelated to a session is rejected by an outpoint comparison before any hashing, so checking it against every session is cheap. So classify_tx lives on every HasFallbackTx state. The sealed FallbackTx trait now returns a borrowed original PSBT, with fallback_tx derived from it as a default method, and a payjoin_psbt that defaults to None. PayjoinProposal and Monitor return their finalized proposal. PayjoinProposal counts because a post can reach the sender even when persisting the transition out of that state fails. ProvisionalProposal returns None: its unsigned transaction matches what finalization produces, but the sender cannot have it yet. Before PayjoinProposal, classify_tx never reports Payjoin. The trait is pub(crate), so generic code bounded on HasFallbackTx cannot call original_psbt or payjoin_psbt from outside the crate any more than fallback_tx. Add a compile_fail doctest for each next to the one guarding fallback_tx. Each needs its own, since one test with several calls would pass while any one of them still failed to compile. classify_tx answers whether this session's proposal or fallback settled, not what happened to the prevouts. Its outcome is Payjoin for the proposal, Fallback for the fallback, and Unrecognized for any other transaction that spends a contested prevout. A transaction spending none of them says nothing about the session, so it gets None rather than Unrecognized. The outcome carries no txid, since the caller passed in the transaction and already has it. Identity compares the transaction's txid, with its script_sigs cleared, against each PSBT's unsigned transaction. Signing fills only script_sig and witness, and the txid already excludes the witness, so for a stable session this is the plain txid check. It also recognizes the proposal after a non-SegWit sender's signature changed its txid, and the fallback after a third party malleated it. Matching on the receiver's inputs or the proposal's output set would also accept a different transaction that happens to share them. A reordered output set is a different transaction no receiver signature covers. The fallback is checked first. The two only coincide when the proposal is the original unchanged, and then no cooperation happened. Because every candidate spends the contested prevouts, one transaction is the whole answer, and choosing which to ask about is the wallet's job. Keep the contested prevouts private. A wallet that classifies every transaction it receives does not need to watch them: sync by script finds the fallback, which pays the receiver, and the proposal, which spends the receiver's inputs or pays its outputs. The one proposal sync misses spends no receiver input and pays only scripts the wallet does not own, as when it forwards the payment to a third party. The wallet chose those scripts itself, so it is the wallet's job to watch them, across restarts too. Document that on substitute_receiver_script and Monitor. A wallet that classifies what it receives has no use for a predicted txid, the fallback, or the receiver's prevouts, and SessionHistory::fallback_tx already returns the fallback for a caller that wants it. It does need a deadline: nothing else ends a session the sender never settles, or settles with a re-spend the wallet never sees. The session's expiration stays that deadline. Past it, replay returns the expired error carrying the fallback, as for any open session, and wallets already broadcast on that error. Document it on Monitor and pin it with a test. check_for_transaction still probes by txid, so compute the proposal's txid privately from the posted proposal. Its txid then includes the script_sigs of the receiver's finalized inputs. The proposal carries no sender signatures, and a SegWit sender adds only witnesses, so this is the txid the broadcast transaction has. Hashing the unsigned proposal instead misses a legacy or P2SH-wrapped receiver input and predicts a txid that never appears on chain. proposal_txid_is_stable checks only the sender's inputs, so it has the same flaw. Before the receiver signs there is no script_sig to read, and fixing it there would mean inferring from script types. Deprecate it in favor of classify_tx, which recognizes the proposal whatever its txid, and narrow its doc to what it checks. classify_tx returns a Settlement rather than the bare outcome, and Settlement::close ends the session once the wallet's confirmation bar is met. Settlement has private fields and no constructor, so only classify_tx makes one, and a session can close this way only after a transaction spent its contested prevouts. Then no fallback needs broadcasting: the fallback is that transaction or conflicts with it. Closing without that evidence would let a wallet end a session holding a fallback it never saw, so every other way out still goes through cancel, which hands the fallback over. Settlement takes no session, so it cannot be paired with the wrong one. It is not Serialize, so it cannot be forged by deserializing. No SettlementOutcome is ever persisted: only confirmation resolves a double-spend race and a reorg can always reverse it, while the log is append-only and Closed is terminal, so a verdict written there would be permanent and could later be wrong. Closing records what the protocol did. A Monitor session closes as PayjoinProposalSent whichever transaction settled it. A PayjoinProposal session settled by its Payjoin closes the same way: the sender signing the proposal proves the post reached it, and a reorg cannot undo that. Any other settlement before the handoff closes as Aborted. Keeping the session open until the wallet closes it also makes the log the record of which sessions still owe an answer, which a wallet-side cache cannot replace because a restored backup brings the logs back without it. A wallet that gives up on a session nothing settled cancels it and broadcasts the fallback. Add ReceiveSession::classify_tx, which dispatches to the state's receiver and returns None for states without a fallback. A wallet that replays its open sessions then classifies without matching on every state, and closes through the Settlement it gets back. Re-express check_for_transaction over classify_tx so a found transaction is confirmed as the proposal rather than trusted from the txid probe alone, and route its non-SegWit exit through the private txid helper. Deprecate it in favor of classify_tx and Settlement::close. It closes on first sight rather than at a confirmation bar, never sees a spend that is neither transaction, and closes an unstable session without checking at all. The binding and the CLI still call the deprecated methods, so allow the deprecation at those call sites. Both deprecations use since = "0.0.0-unreleased", a placeholder the release replaces with the real version rather than this commit guessing it. The existing Monitor tests keep covering check_for_transaction, its early exit for non-SegWit senders included, under allow(deprecated). New regression tests cover its probe of a txid with a receiver script_sig, and proposal_txid_is_stable, until both are removed. The new tests classify each kind of spend and close the Settlement from MaybeInputsOwned, PayjoinProposal and Monitor, checking the outcome each writes.
Every terminal outcome reads as a payment verdict today. Aborted says the payjoin "was not successful" when a fallback may well have been broadcast and confirmed, and PayjoinProposalSent still describes non-SegWit senders as unmonitorable, which classify_tx now makes untrue. Say what each one means. Aborted is that the session was cancelled or failed, or a transaction settled it before the receiver handed off its proposal, and the receiver closed it. That can happen after the proposal was posted, since Monitor can still cancel into PendingFallback and close from there. PayjoinProposalSent is that the receiver handed off the proposal and the wallet closed the session after settling its outcome, whichever transaction that was. Settlement also writes it for a PayjoinProposal session settled by its Payjoin, so a session closed this way without a posted proposal in its log was settled by its Payjoin; say so. Note on Success and FallbackBroadcasted that only the deprecated check_for_transaction writes them, so sessions settled through classify_tx and Settlement::close never carry them. SessionStatus::Completed needs the same treatment and matters more, since it is the end state of every successful session and wallet UIs read status(). Document it as the wallet having settled the outcome, without saying which transaction settled it, and document the remaining variants rather than leaving one commented among five bare ones. Record on Unrecognized that the receiver never writes it, and point to classify_tx, which reports such a spend without persisting it. The variant was reserved for a classifier that would have persisted its verdict. It stays because the enum is exhaustive, so removing it is a semver break. Note on SessionStatus::FallbackBroadcasted, as on its SessionOutcome, that only the deprecated check_for_transaction produces it.
The receiver gained a classifier on every state with a fallback, and a Settlement that closes the session it classified, but bindings could still only call check_for_transaction, which probes two predicted txids and closes on the first match. Wallets built on the bindings had no way to apply their own confirmation bar or to recognize a Payjoin whose txid a non-SegWit sender changed. Expose classify_tx on every state from MaybeInputsOwned through Monitor, the same states that cancel into PendingFallback, through one macro like the cancel methods. Wallets are meant to classify every received transaction against each open session, so the binding needs it wherever core has it. Expose Settlement as an object with outcome and close, SettlementOutcome as a uniffi enum, and a SettlementCloseTransition to persist the close. A uniffi object has no constructor unless one is exported, so foreign code cannot make a Settlement either. Expose ReceiveSession::classify_tx as the top-level function classify_receiver_tx rather than a method on the ReceiveSession enum. uniffi-dart 0.2.1 renders methods only on objects, so an enum method would be missing from the Dart bindings, and uniffi-bindgen-react-native would render it as a static function taking the enum anyway. It decodes the transaction in every state, so bad bytes fail the same way whatever the session's state. Carry core's note on watching forwarded outputs over to substitute_receiver_script and Monitor. classify_tx takes consensus-encoded bytes like the rest of the binding, so it returns a TransactionParseError when they do not decode. The core SettlementOutcome is non_exhaustive; a verdict this binding does not know maps to Unrecognized rather than panicking. Remove check_for_transaction, with the TransactionFinder callback and MonitorTransition it needed, and proposal_txid_is_stable from every receiver state, rather than deprecating them as core does. payjoin-ffi is not yet 1.0, so it can drop them now, and uniffi cannot carry #[deprecated] into the bindings, so a deprecation would only be a doc note binding users may never see. Record the additions and removals under Unreleased in the FFI changelog, as the changelog check requires for a change to payjoin-ffi/src. Kotlin objects are AutoCloseable, whose close() drops the Rust handle, so rename Settlement.close to closeSession there, as uniffi.toml already does for the other types that export a close. Exercise the new API in every binding's tests. The v2 integration test saves the receiver's Monitor, classifies the broadcast Payjoin with Monitor.classify_tx and with classify_receiver_tx on the replayed session, and closes the session through the Settlement. A unit test checks that classify_receiver_tx returns nothing for a session without a fallback and rejects bytes that are not a transaction.
After posting its proposal the receiver polled the mempool for a predicted txid for a few seconds and closed on first sight. It closed sessions with non-SegWit senders without checking at all. Have resume check the wallet once for each open receiver session, in any state, before driving it. Every wallet transaction paying the session's address is classified against the session. The CLI never substitutes the receiver output, so both the fallback and the Payjoin pay that address. A session settled with one confirmation is closed. A session settled with fewer is left open without advancing its protocol. Otherwise the session is driven as before and stops at Monitor until a later resume finds its settlement. bitcoind-async-client has no wrapper for listreceivedbyaddress, so enable its raw_rpc feature and call it directly. Stop printing the proposal txid for the user to watch. It was the unsigned proposal's, which is wrong for non-SegWit senders.
c48f054 to
83821b1
Compare
Supersedes #1692 .
After posting a proposal, the receiver has to find out whether the Payjoin, the fallback, or some other transaction spent the sender's inputs. Today
check_for_transactionhandles this by probing two predicted txids and closing the session on the first match. It writes a verdict a reorg can reverse, it never sees an unrelated spend, and it closes non-SegWit sessions without checking at all.This PR gives the wallet what it needs to answer the question itself, and lets the wallet decide when to stop asking.
Much of the design iteration and rationale for this implementation lives in #1692.
Changes
Receiver<Monitor>gains:contested_prevouts: the sender's prevouts that the proposal and fallback both spend.classify(tx): returnsPayjoin,FallbackorUnrecognizedfor a transaction spending them, orNoneif it doesn't. The match ignores script_sig, so a non-SegWit signature or third-party malleation doesn't hide the proposal or the fallback.close(): recordsPayjoinProposalSentonce the wallet stops tracking. No settlement verdict is ever persisted.expected_payjoin_txid(Stable/Unstable),fallback_txidandreceiver_prevouts.check_for_transactionis deprecated since 1.3.0 and now goes throughclassify.Active. The transaction it waits on can confirm after the mailbox is gone.SessionOutcomeandSessionStatusnow describe what the protocol did, not how the payment settled.SettlementOutcome/ExpectedTxidenums.Followups
The CLI only finds settling transactions its wallet already knows about: the fallback, a stable Payjoin probed by txid, and a non-SegWit Payjoin found by scanning recent wallet transactions. It never asks bitcoind what spent the contested prevouts. If the sender double-spends its inputs in an unrelated transaction, the session never closes and times out on every resume. Fixing it needs gettxspendingprevout for mempool spends and a bounded block scan for confirmed ones.
Assigning to @DanGould for review.
Disclosure: This description and the code changes were written with Claude Code.
Pull Request Checklist
Please confirm the following before requesting review: