General question on receiver's ProvisionalProposal and Monitor typestates #1548

Activity

1. xstoicunicornx commented on May 9, 2026

   

   xstoicunicornx

   on May 9, 2026

   CollaboratorAuthor

   More actions

   Cross posting @nothingmuch 's response from #1546

   • check_payment uses extract_tx_fee_rate_limit for the fallback tx but everywhere else uses extract_tx_unchecked_fee_rate, is this an intentional discrepancy? Seems like it has the potential to introduce errors for a fallback tx that was already deemed valid.

   Hmm, good catch.

   Since this happens late in the state machine, it may make sense to do that on the fallback transaction when it is first received, but even then game theoretically that's the sender's concern not the receiver's, the receiver should be concerned with the absolute fees they pay or the feerate for their inputs/outputs in the transaction, less so with the payjoin transaction as a whole concerning fees that are too high which is what this method attempt to prevent.

   what's worse, this unconditionally calls .expect() so there is no way to handle this error.

   Please open an issue even if fixing in this PR so that in case this PR gets bogged down in review we don't lose track of this bugfix

   • Is it enforced that receiver will only add Segwit inputs?

   No

   If not wouldn't using payjoin_proposal.unsigned_tx.compute_txid() as the payjoin_txid in check_payment potentially give wrong txid for inputs that have sig field?

   Yes, that's documented on check_payment. Depending on the wallet/node capabilities, different ways of checking for the payment status, for instance by watching for any transaction that spends or creates specific outputs would be more robust but I think that's up to wallets.

   That said, you point out a flaw in our design, which is that Receiver<Monitor> arguably should not be part of the state machine if the transaction is non-segwit, and clients that want to rely on that should be able to just broadcast the fallback transaction without contributing their inputs. I think it can be split up into Receiver<TxidMonitor> that can only be entered with segwit only txs and which supports the check_payment API as implemented, and a Receiver<SomeOtherMonitorTypeStateName> that only supports asserting the status (presumably by monitoring transaction broadcast or for specific scripts depending on the wallet type)

   Please open an issue for this too, IMO that should be done in a separate PR since this is a conceptual change that is non-trivial

   • Should finalize_proposal validate that the signed PSBT actually has a valid signature?

   This seems unnecessary since that would be the receiver validating its own singuature, if a wallet is not able to produce valid signatures that's not the payjoin crate's responsibility to enforce
2. spacebear21 commented on May 14, 2026

   

   spacebear21

   on May 14, 2026

   Collaborator

   More actions

   check_payment uses extract_tx_fee_rate_limit for the fallback tx but everywhere else uses extract_tx_unchecked_fee_rate, is this an intentional discrepancy? Seems like it has the potential to introduce errors for a fallback tx that was already deemed valid.

   This seems unintentional to me. extract_tx_fee_rate_limit checks against DEFAULT_MAX_FEE_RATE which is 25k sats/vb which is unlikely anyway, but even then that would be the sender's problem not the receiver's. Really we should just use the self.fallback_tx() method on Monitor which already uses extract_tx_unchecked_fee_rate.

   Is it enforced that receiver will only add Segwit inputs? If not wouldn't using payjoin_proposal.unsigned_tx.compute_txid() as the payjoin_txid in check_payment potentially give wrong txid for inputs that have sig field?

   There may be an inconsistency here, the docstring for check_payment says "If the receiver input address type in the fallback transaction is non-SegWit, then this function will directly conclude the Payjoin session with a Success without running the provided transaction_exists closure." The fallback transaction doesn't include receiver inputs though, so the actual check is only on sender inputs:

   rust-payjoin/payjoin/src/core/receive/v2/mod.rs

   Line 1345 in b69034f

   if fallback_tx.input.iter().any(|txin| txin.witness.is_empty()) {

   @arminsabouri is this right or am I missing something?

   In payjoin crate the Monitor typestate's method is called check_payment but in the FFI bindings it is called monitor, was there a reason for this renaming? check_payment seems to be a more accurate and intuitive name for this method, since the state is monitoring but the method itself is only checking for the payment.

   looks like an oversight, it should be check_payment
3. arminsabouri commented on May 15, 2026

   

   arminsabouri

   on May 15, 2026

   Contributor

   More actions

   @arminsabouri is this right or am I missing something?

   Yeah the docstring is wrong, it should be "sender"
4. xstoicunicornx commented on May 15, 2026

   

   xstoicunicornx

   on May 15, 2026

   CollaboratorAuthor

   More actions

   Really we should just use the self.fallback_tx() method on Monitor which already uses extract_tx_unchecked_fee_rate.

   Actually I was trying to do this but was getting this error:

   no method named `fallback_tx` found for struct `Monitor` in the current scope items from traits can only be used if the trait is in scope

   Is there a particular way I need to call this due to the sealed state?
5. spacebear21 commented on May 15, 2026

   

   spacebear21

   on May 15, 2026

   Collaborator

   More actions

   Yeah the docstring is wrong, it should be "sender"

   I wonder if we should also return early if the receiver contributed non-segwit inputs, IIRC those could change in theory due to transaction malleability?

   Is there a particular way I need to call this due to the sealed state?

   Hmm right, an ugly way codex suggested is sealed::State::fallback_tx(&self.state) but it works. If we end up merging #1558 this should be more straightforward because of how fallback_tx is reimplemented.
6. DanGould commented on May 19, 2026

   

   DanGould

   on May 19, 2026

   Last edited by DanGould

   Member

   More actions

   If there is no concrete integration already depending on non-Segwit inputs (non-malleable / stable TxID) I propose we remove their support. It'd be really good to know what wallets (core?) still are dealing with legacy inputs. Sender client'd have to produce a TxIdMalleabilityError or something. Not a BIP77 spec change because it's a tightening of the rules for our PDK client.

   @arminsabouri mentionted that payjoin receiver could even be an upgrade path for legacy wallets because this problem affects ONLY receivers when the sender is using legacy inputs. The receiver could use legacy inputs. Doc / Blog / fun article could come of this idea.