Repository navigation
Conversation
8832e47 to
e7c0dde
Compare
Coverage Report for CI Build 29565128648Coverage increased (+0.3%) to 86.441%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
|
IMO such an API change should wait until after 1.0 release and is a candidate for a 2.0 API:
In the meantime, I think it's sufficient to add a big disclaimer in the
|
|
Field evidence in favor of this direction, from the BTCPay Server payjoin plugin (a settlement-accounting receiver over payjoin-ffi): we independently hit both flaws this PR identifies. Our accounting layer pins the proposal's expected transaction ID at hand-out time for exact reconciliation, which is silently wrong for non-SegWit senders (we currently reject them as policy, ValeraFinebits/btcpayserver-payjoin-plugin#78), and our stored reconciliation verdict shares the stale-on-reorg problem — a re-runnable In the meantime, for whatever it's worth to the 1.0 discussion: #1718 proposes the machine-readable version of the disclaimer suggested above — |
e7c0dde to
47b49c5
Compare
The persisted SessionEvent format freezes at the payjoin-cli 1.0 database-backwards-compatibility commitment, so reshape the receiver's terminal SessionOutcome while a format break is still free. Change Success(Vec<(ScriptBuf, Witness)>) to Success(Txid). The witness payload was recorded so the receiver could reconstruct the final Payjoin on the non-segwit path, but payjoin#1218 removed that path entirely, leaving the enum payload as dead code. The Receiver settlement classifier (payjoin#1692) will restore non-segwit detection, which would make the payload live again, but sufficient record of a settlement is the txid of the transaction that settled it rather than the witness list. The wallet that observed the spend already holds the full transaction. Recording Success(Txid) also keeps the terminal variants consistent where the outcome for the transaction we recognize carries the same evidence as the one we do not. Add Other(Txid), mapped to SessionStatus::Failed, for contested outpoints settled by an unrecognized transaction. Nothing produces it yet. The reason to reserve it now is API semver, not data compatibility. 8c92050 deliberately kept SessionOutcome exhaustive. A wildcard match arm has no meaningful semantics for a terminal outcome, and the compile error on a new variant is a feature. So adding a variant after 1.0 would be a semver-breaking change that we want. Naming Other before the freeze makes the settlement classifier additive by construction while keeping the enum exhaustive. (Forward compatibility of the persisted data is explicitly not a goal. See the persist module docs, we want unknown state to error.) Update payjoin-cli to match the new shapes exhaustively. no wildcard arms, per 8c92050 surfacing the settlement txid in the history and cancel output, and pin the sealed JSON encodings of the terminal events in a serialization-shape test. StatusText becomes String rather than &'static str in order to accomodate the txid in format!.
Introduce outpoint-based settlement attribution for a monitored
session. A pure, total classify maps caller-supplied chain facts to a
settlement status without any I/O or persisted events.
Add the compute types SettlementStatus and SettlementOutcome
{Cooperative(Txid), Fallback, Other(Txid)} plus ChainView/OutpointSpend,
and the primitive getters the wallet needs (payjoin_txid,
fallback_txid, contested_outpoints, receiver_input).
classify recognizes a cooperative settlement by either signal: the
spending transaction carries the receiver's contributed input (the
input fingerprint) or, for an output-substitution proposal, reproduces
the proposal's output set (a cut-through). Both are structural, so a
non-SegWit txid change does not hide them; Cooperative therefore
carries the observed settling txid, which in that case differs from
the predicted payjoin_txid and is otherwise unavailable to the caller
of a txid-oblivious classifier. Precedence is
Cooperative > Fallback > Other.
SettlementOutcome is #[non_exhaustive]. This is a deliberate exception
to 8c92050's keep-exhaustive list, judged by that commit's own test:
does a wildcard arm have meaningful semantics? For a *persisted*
terminal outcome it does not (SessionOutcome stays exhaustive). But
SettlementOutcome is a never-persisted classification verdict, and a
finer future label (say, distinguishing a sender re-spend from a
third-party spend) degrades soundly in a wildcard arm to "settled by
something this build cannot label -- surface for review". No session
state is resumed through it, so the failure mode 8c92050 guards
against does not arise. SettlementStatus itself stays exhaustive:
Pending/Detected is a closed shape.
Add Receiver<Monitor>::conclude, which persists the single terminal Closed(SessionOutcome) event once the caller's confidence bar is met, mapping Cooperative(txid) -> Success(txid), Fallback -> FallbackBroadcasted, and Other(txid) -> Other(txid). The persisted SessionOutcome shape (Success(Txid), the reserved Other(Txid)) was sealed with the 1.0 format freeze; this wires the settlement classifier to it and makes the classifier produce the reserved Other variant, so drop the reservation note from its docs. The Success txid is the observed settling transaction's, which for a non-SegWit sender differs from the predicted payjoin txid the receiver could compute on its own. Re-express the legacy check_for_transaction over classify via a txid-only ChainView: each found transaction is classified structurally before concluding, instead of trusting the txid probe alone. The consume-self signature and transient-error flow are unchanged.
Add the foreign-facing settlement surface: a ChainBackend trait that reports each contested outpoint's spend status, the ChainSpend record, and SettlementStatus/SettlementOutcome uniffi mirrors. Monitor::classify queries the backend, builds a ChainView, and returns the status without persisting anything. Cooperative mirrors the core variant's payload, carrying the observed settling txid as a string. SettlementOutcome is #[non_exhaustive] in core, so the conversion keeps a wildcard arm that degrades a finer label this binding predates to Other until the binding is regenerated.
Drive the monitor step from classify: build a ChainView by querying the wallet for each contested outpoint (get_raw_transaction_verbose for the spending tx and its confirmations), then render the status each tick as Pending / cooperative / fallback / other -- surfacing the settling txid for cooperative and other spends -- concluding once a configurable confirmation bar is met. Status lines go through the session-scoped printer so concurrently resumed sessions stay attributable. SettlementOutcome is #[non_exhaustive], so the render match keeps a wildcard arm that shows a generic settled line for any finer label this build predates.
Spell out the Monitor contract so integrators do not trust an imprecise 0-conf verdict. The proposal and the fallback double-spend the sender's inputs, so detection is necessary but not sufficient: only confirmation resolves the race, and confirmation depth is a confidence dial, not a state transition. Document the division of labor (library knows the semantics, wallet knows the ground truth and holds the spending transaction), the classify/conclude flow, and that the legacy check_for_transaction cannot surface Other and concludes optimistically on first sight.
47b49c5 to
9fc58e5
Compare
|
Changed to be additive to target 1.1 on top of #1747 |
The persisted SessionEvent format freezes at the payjoin-cli 1.0 database-backwards-compatibility commitment, so reshape the receiver's terminal SessionOutcome while a format break is still free. - Change `Success(Vec<(ScriptBuf, Witness)>)` to `Success(Txid)`. The witness payload was recorded so the receiver could reconstruct the final Payjoin on the non-segwit path, but payjoin#1218 removed that path entirely, leaving the enum payload as dead code. The Receiver settlement classifier (payjoin#1692) will restore non-segwit detection, which would make the payload live again, but sufficient record of a settlement is the txid of the transaction that settled it rather than the witness list. The wallet that observed the spend already holds the full transaction. Recording `Success(Txid)` also keeps the terminal variants consistent where the outcome for the transaction we recognize carries the same evidence as the one we do not. - Add `Unrecognized(Txid)` for contested outpoints settled by an unrecognized transaction. Nothing produces it yet but it is reserved now because adding a variant after 1.0 would be a semver-breaking change. Naming `Unrecognized` before the freeze makes the settlement classifier additive in a 1.x follow-up while keeping the enum exhaustive. Co-authored-by: DanGould <d@ngould.dev>
The persisted SessionEvent format freezes at the payjoin-cli 1.0 database-backwards-compatibility commitment, so reshape the receiver's terminal SessionOutcome while a format break is still free. - Change `Success(Vec<(ScriptBuf, Witness)>)` to `Success(Txid)`. The witness payload was recorded so the receiver could reconstruct the final Payjoin on the non-segwit path, but payjoin#1218 removed that path entirely, leaving the enum payload as dead code. The Receiver settlement classifier (payjoin#1692) will restore non-segwit detection, which would make the payload live again, but sufficient record of a settlement is the txid of the transaction that settled it rather than the witness list. The wallet that observed the spend already holds the full transaction. Recording `Success(Txid)` also keeps the terminal variants consistent where the outcome for the transaction we recognize carries the same evidence as the one we do not. - Add `Unrecognized(Txid)` for contested outpoints settled by an unrecognized transaction. Nothing produces it yet but it is reserved now because adding a variant after 1.0 would be a semver-breaking change. Naming `Unrecognized` before the freeze makes the settlement classifier additive in a 1.x follow-up while keeping the enum exhaustive. Co-authored-by: DanGould <d@ngould.dev>
|
@xstoicunicornx I'd like to hand this off to you. This is the right shape but the FFI needs to change. This should follow #1446's shape, taking a uniffi record of Bull Bitcoin's exchange hit the fallback monitoring problem which this fixes (SatoshiPortal/payjoin#6) and their monitor code seems not written yet. This saves them writing it on their own and rewriting it with PDK's API later. |
Introduce an implementation-agnostic interface for receiver typestates that currently require callback-based validation to advance. Previously, each validation step demanded a synchronous closure, coupling the state machine to the caller's execution model. This made integration difficult for wallets where signing, broadcast checks, or ownership lookups are asynchronous or handled by a separate process. Each callback-based transition is now split into a two-phase pattern: a method to extract the data that needs checking (*_checklist, extract_tx_*, psbt_to_sign) and a corresponding method to submit results and advance the state (apply_*_checklist, apply_broadcast_suitability, finalize_signed_proposal). The submitted ChecklistItem/MarkedChecklistItem list is validated against the inputs or outputs of the receiver's own original PSBT, so a list marked for a different proposal is rejected, and a PSBT with no inputs or no outputs is handled as an empty list. This applies across v1 and v2 receiver flows, including input ownership, input-seen, output ownership, broadcast suitability and proposal finalization. Transaction monitoring is left to the settlement status work in payjoin#1692. The original closure-based methods are preserved as convenience wrappers over the new API, so this is backward-compatible for existing integrators. Co-authored-by: spacebear21 <git@spacebear.dev> Co-authored-by: DanGould <d@ngould.dev>
|
After reviewing this PR I had a couple questions on design and had pretty significant change I'd like to propose to the PR's current API surface. Looking for your opinion @DanGould . Questions:
I would like to remove |
I don't believe we want to make it possible to represent a receiver who is monitoring as having bailed out. The only way to enter Monitor is by having processed a response that actually sends the Payjoin Proposal, in which case either the payjoin succeeds, fallback is detected, or the input is double spent (which is Thinking about it more, I don't want the library to persist a settlement outcome at all, which means Posting the proposal is the receiver's last protocol action and Closing at posting also settles an expiry problem that exists today. The session expiry is the mailbox expiry, and replay refuses any session that isn't closed once it has passed. A Monitor left open past that point can't be replayed, so a payjoin that confirms after the mailbox expired could never be recorded. That is true of
Private, as we discussed. They exist for cut-through detection. It's possible for the receiver to create a proposal where they contribute no input so the Regarding
|
|
This mostly sounds good to me. I was working on implementing this but thought of an issue with getting rid of Just wanted to let you know about this change in direction. Let me know if you have objections or can find flaws in my reasoning. |
|
Superseded by #1971 Feel free to close when you supersede next time @xstoicunicornx |
This is following up on #1684 where I figured out Monitor's status can go stale in the case of a reorg. Instead, we can run a classifier that can always classify the status based on mempool+chain state data instead.
This classification, that tracks
OutPointstatus also makes the #1218 workaround that skips monitoring the status of non-segwit outputs obsolete and can actually monitor non-segwit spending Payjoin cooperative txs.Sessions still close once a
Receiver<Monitor>detects a transaction but the actual SettlementStatus is determined by callingclassifyon chain state each time.Co-authored by Claude code
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.