Skip to content

Classify Payjoin settlements on the Monitor typestate - #1971

Open
xstoicunicornx wants to merge 5 commits into
payjoin:masterfrom
xstoicunicornx:monitor-revamp
Open

xstoicunicornx wants to merge 5 commits into
payjoin:masterfrom
xstoicunicornx:monitor-revamp

Conversation

@xstoicunicornx

@xstoicunicornx xstoicunicornx commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

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_transaction handles 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): returns Payjoin, Fallback or Unrecognized for a transaction spending them, or None if 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(): records PayjoinProposalSent once the wallet stops tracking. No settlement verdict is ever persisted.
    • expected_payjoin_txid (Stable/Unstable), fallback_txid and receiver_prevouts.
  • check_for_transaction is deprecated since 1.3.0 and now goes through classify.
  • Mailbox expiry: a session that has posted its proposal now replays after the mailbox expires and reports Active. The transaction it waits on can confirm after the mailbox is gone.
  • Docs: SessionOutcome and SessionStatus now describe what the protocol did, not how the payment settled.
  • FFI exposes the new methods and the SettlementOutcome/ExpectedTxid enums.
  • CLI finds the settling transaction in its wallet, classifies it, and closes once it has one confirmation. Before, it closed on the first mempool sighting. An unsettled session stays open across resumes.

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:

@coveralls

coveralls commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

Coverage Report for CI Build 40710937273

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.3%) to 87.744%

Details

  • Coverage increased (+0.3%) from the base build.
  • Patch coverage: 27 uncovered changes across 3 files (453 of 480 lines covered, 94.38%).
  • 2 coverage regressions across 1 file.

Uncovered Changes

File Changed Covered %
payjoin/src/core/receive/v2/mod.rs 329 316 96.05%
payjoin-cli/src/app/v2/mod.rs 61 49 80.33%
payjoin-cli/src/app/wallet.rs 24 22 91.67%
Total (4 files) 480 453 94.38%

Coverage Regressions

2 previously-covered lines in 1 file lost coverage.

File Lines Losing Coverage Coverage
payjoin-cli/src/app/v2/mod.rs 2 58.65%

Coverage Stats

Coverage Status
Relevant Lines: 18236
Covered Lines: 16001
Line Coverage: 87.74%
Coverage Strength: 317.27 hits per line

💛 - Coveralls

@DanGould DanGould left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread payjoin/src/core/receive/v2/session.rs Outdated
Comment thread payjoin-ffi/src/receive/mod.rs Outdated
Comment thread payjoin-ffi/src/receive/mod.rs Outdated
Comment thread payjoin/tests/integration.rs
Comment thread payjoin/tests/integration.rs Outdated
Comment thread payjoin-ffi/src/receive/mod.rs Outdated
Comment thread payjoin/src/core/receive/v2/mod.rs Outdated
Comment thread payjoin/src/core/receive/v2/mod.rs Outdated
@DanGould DanGould added this to the payjoin-1.3 milestone Oct 13, 2026
@xstoicunicornx
xstoicunicornx force-pushed the monitor-revamp branch 3 times, most recently from 5ca8c70 to 3cbff7b Compare October 14, 2026 17:24
@xstoicunicornx

Copy link
Copy Markdown
Collaborator Author

I think I got all follow ups, ready for re-review.

@DanGould DanGould left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incomplete review, but thoughts that came up while reading rust-bitcoin's policy re: deprecation & versioning

Comment thread payjoin-ffi/src/receive/mod.rs Outdated
/// 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is gonna go stale immediatel and should be left out of comments

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread payjoin-ffi/src/receive/mod.rs Outdated
/// 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, just removed the version.

Comment thread payjoin/src/core/receive/v2/mod.rs
DanGould
DanGould previously approved these changes Oct 16, 2026

@DanGould DanGould left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@xstoicunicornx

Copy link
Copy Markdown
Collaborator Author

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

@spacebear21 spacebear21 left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread payjoin/src/core/receive/v2/mod.rs Outdated
Comment on lines +1847 to +1859
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() }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread payjoin/src/core/receive/v2/mod.rs Outdated
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() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we use self.state.fallback_tx.compute_txid()?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment thread payjoin/src/core/receive/v2/mod.rs Outdated

/// Close the session once the wallet no longer needs to track settlement.
pub fn close(self) -> TerminalTransition<SessionEvent, ()> {
TerminalTransition::new(SessionEvent::Closed(SessionOutcome::PayjoinProposalSent), ())

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread payjoin/src/core/receive/v2/mod.rs Outdated
@xstoicunicornx

Copy link
Copy Markdown
Collaborator Author

Rebasing to resolve merge conflict

@xstoicunicornx

Copy link
Copy Markdown
Collaborator Author

Ready for re-review.

@xstoicunicornx
xstoicunicornx force-pushed the monitor-revamp branch 3 times, most recently from 2e84523 to 8c1da26 Compare October 22, 2026 08:36
@spacebear21 spacebear21 mentioned this pull request Oct 22, 2026
3 tasks done
xstoicunicornx pushed a commit to xstoicunicornx/rust-payjoin that referenced this pull request Oct 22, 2026
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.
@xstoicunicornx
xstoicunicornx force-pushed the monitor-revamp branch 2 times, most recently from 6f369bd to 902166e Compare October 22, 2026 12:32
@xstoicunicornx
xstoicunicornx force-pushed the monitor-revamp branch 2 times, most recently from 1056adb to c61e94c Compare October 22, 2026 13:25

@spacebear21 spacebear21 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had my bot review this closely and posted the most relevant findings here

Comment on lines +340 to +348
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 }
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread payjoin/src/core/receive/v2/session.rs Outdated
Comment on lines +34 to +37
// 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() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems acceptable to break payjoin-ffi only since it's <1.0.

@xstoicunicornx
xstoicunicornx force-pushed the monitor-revamp branch 2 times, most recently from 78dd6a8 to c48f054 Compare October 27, 2026 05:15
@xstoicunicornx

Copy link
Copy Markdown
Collaborator Author

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.

spacebear21 and others added 5 commits October 28, 2026 14:32
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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants