Skip check_payment in Receiver<Monitor> if the sender is using non-segwit address

The previous implementation of the check_payment function assumed that
if the outpoints of the Payjoin transaction have been removed from the
UTXO set, it is an indication of the Payjoin being broadcasted by the
sender. Moreover, it relied on the same closure to detect whether there
is a double-spend attempt from the sender, if only a subset of the
outpoints have been spent.

Both of the usages of the outpoint closure is incorrect. If the sender
does RBF on the fallback transaction, this would change the transaction
ID and cause the previous implementation to incorrectly detect
double-spend. Moreover, the sender can use some of the UTXOs in the
Payjoin session if they wish without necessarily "attacking" the
receiver.

This change removes the outpoint closure, and instead skips the check if
the fallback transaction has any inputs which does not have witness
data. This assumes that the fallback transaction has been signed which
is a certainty at this point in the Payjoin session.

Author: mehmetefeumit

payjoin-cli/src/app/v2/mod.rs

@@ -82,6 +82,8 @@ impl StatusText for ReceiveSession {
                 ReceiverSessionOutcome::Success(_) => "Session success",
                 ReceiverSessionOutcome::Cancel => "Session cancelled",
                 ReceiverSessionOutcome::FallbackBroadcasted => "Fallback broadcasted",
+                ReceiverSessionOutcome::BroadcastStatusUnknown =>
+                    "Broadcast status of Payjoin transaction unknown",
             },
         }
     }
@@ -778,18 +780,11 @@ impl App {
             loop {
                 interval.tick().await;
                 let check_result = proposal
-                    .check_payment(
-                        |txid| {
-                            self.wallet()
-                                .get_raw_transaction(&txid)
-                                .map_err(|e| ImplementationError::from(e.into_boxed_dyn_error()))
-                        },
-                        |outpoint| {
-                            self.wallet()
-                                .is_outpoint_spent(&outpoint)
-                                .map_err(|e| ImplementationError::from(e.into_boxed_dyn_error()))
-                        },
-                    )
+                    .check_payment(|txid| {
+                        self.wallet()
+                            .get_raw_transaction(&txid)
+                            .map_err(|e| ImplementationError::from(e.into_boxed_dyn_error()))
+                    })
                     .save(persister);
 
                 match check_result {

payjoin-cli/src/app/wallet.rs

@@ -134,26 +134,6 @@ impl BitcoindWallet {
         }
     }
 
-    #[cfg(feature = "v2")]
-    pub fn is_outpoint_spent(&self, outpoint: &OutPoint) -> Result<bool> {
-        let _ = tokio::task::block_in_place(|| {
-            tokio::runtime::Handle::current()
-                // Note: explicitly ignore txouts in the mempool. Those should be considered spent for our purposes
-                .block_on(async {
-                    match self.rpc.get_tx_out(&outpoint.txid, outpoint.vout, false).await {
-                        Ok(_) => Ok(true),
-                        Err(e) =>
-                            if e.is_missing_or_invalid_input() {
-                                Ok(false)
-                            } else {
-                                Err(e)
-                            },
-                    }
-                })
-        })?;
-        Ok(true)
-    }
-
     #[cfg(feature = "v2")]
     pub fn get_raw_transaction(
         &self,

payjoin-ffi/src/receive/mod.rs

@@ -1136,24 +1136,13 @@ fn try_deserialize_tx(
 
 #[uniffi::export]
 impl Monitor {
-    pub fn monitor(
-        &self,
-        transaction_exists: Arc<dyn TransactionExists>,
-        outpoint_spent: Arc<dyn OutpointSpent>,
-    ) -> MonitorTransition {
-        MonitorTransition(Arc::new(RwLock::new(Some(self.0.clone().check_payment(
-            |txid| {
-                transaction_exists
-                    .callback(txid.to_string())
-                    .and_then(|buf| buf.map(try_deserialize_tx).transpose())
-                    .map_err(|e| ImplementationError::new(e).into())
-            },
-            |outpoint| {
-                outpoint_spent
-                    .callback(outpoint.into())
-                    .map_err(|e| ImplementationError::new(e).into())
-            },
-        )))))
+    pub fn monitor(&self, transaction_exists: Arc<dyn TransactionExists>) -> MonitorTransition {
+        MonitorTransition(Arc::new(RwLock::new(Some(self.0.clone().check_payment(|txid| {
+            transaction_exists
+                .callback(txid.to_string())
+                .and_then(|buf| buf.map(try_deserialize_tx).transpose())
+                .map_err(|e| ImplementationError::new(e).into())
+        })))))
     }
 }
 

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

@@ -1223,11 +1223,29 @@ impl Receiver<Monitor> {
     pub fn check_payment(
         &self,
         transaction_exists: impl Fn(Txid) -> Result<Option<bitcoin::Transaction>, ImplementationError>,
-        outpoint_spent: impl Fn(OutPoint) -> Result<bool, ImplementationError>,
     ) -> MaybeFatalOrSuccessTransition<SessionEvent, Self, Error> {
+        let fallback_tx = self
+            .state
+            .psbt_context
+            .original_psbt
+            .clone()
+            .extract_tx_fee_rate_limit()
+            .expect("fallback transaction should be in the receiver context");
+
+        // If the fallback transaction included any non-SegWit inputs, then the transaction ID of
+        // the Payjoin proposal is going to change when the sender signs their non-SegWit address
+        // one more time. The receiver cannot monitor the broadcast, and should conclude the sesion.
+        if fallback_tx.input.iter().any(|txin| txin.witness.is_empty()) {
+            return MaybeFatalOrSuccessTransition::success(SessionEvent::Closed(
+                SessionOutcome::BroadcastStatusUnknown,
+            ));
+        }
+
         let payjoin_proposal = &self.state.psbt_context.payjoin_psbt;
         let payjoin_txid = payjoin_proposal.unsigned_tx.compute_txid();
-        // If we have a payjoin transaction with segwit inputs, we can check for the txid
+        // If the sender is using a SegWit address, then the transaction ID of the Payjoin proposal
+        // is not going to change when the sender signs it. So we can use the TXID to determine if
+        // the Payjoin proposal has been broadcasted.
         match transaction_exists(payjoin_txid) {
             Ok(Some(tx)) => {
                 let tx_id = tx.compute_txid();
@@ -1253,14 +1271,8 @@ impl Receiver<Monitor> {
             Err(e) => return MaybeFatalOrSuccessTransition::transient(Error::Implementation(e)),
         }
 
-        // Check for fallback being broadcasted
-        let fallback_tx = self
-            .state
-            .psbt_context
-            .original_psbt
-            .clone()
-            .extract_tx_fee_rate_limit()
-            .expect("Checked in earlier typestates");
+        // If the Payjoin proposal was not found, check the fallback transaction, at it is
+        // the second of two transactions whose IDs the receiver is aware of.
         match transaction_exists(fallback_tx.compute_txid()) {
             Ok(Some(_)) =>
                 return MaybeFatalOrSuccessTransition::success(SessionEvent::Closed(
@@ -1270,29 +1282,6 @@ impl Receiver<Monitor> {
             Err(e) => return MaybeFatalOrSuccessTransition::transient(Error::Implementation(e)),
         }
 
-        let mut outpoints_spend = 0;
-        for ot in payjoin_proposal.unsigned_tx.input.iter() {
-            match outpoint_spent(ot.previous_output) {
-                Ok(false) => {}
-                Ok(true) => outpoints_spend += 1,
-                Err(e) =>
-                    return MaybeFatalOrSuccessTransition::transient(Error::Implementation(e)),
-            }
-        }
-
-        if outpoints_spend == payjoin_proposal.unsigned_tx.input.len() {
-            // All the payjoin proposal outpoints were spent. This means our payjoin proposal has non-segwit inputs and is broadcasted.
-            return MaybeFatalOrSuccessTransition::success(SessionEvent::Closed(
-                // TODO: there seems to be not great way to get the tx of the tx that spent these outpoints.
-                SessionOutcome::Success(vec![]),
-            ));
-        } else if outpoints_spend > 0 {
-            // Some outpoints were spent but not in the payjoin proposal. This is a double spend.
-            return MaybeFatalOrSuccessTransition::success(SessionEvent::Closed(
-                SessionOutcome::Failure,
-            ));
-        }
-
         MaybeFatalOrSuccessTransition::no_results(self.clone())
     }
 }
@@ -1333,7 +1322,7 @@ pub(crate) fn pj_uri<'a>(
 pub mod test {
     use std::str::FromStr;
 
-    use bitcoin::FeeRate;
+    use bitcoin::{FeeRate, ScriptBuf, Witness};
     use once_cell::sync::Lazy;
     use payjoin_test_utils::{
         BoxError, EXAMPLE_URL, KEM, KEY_ID, ORIGINAL_PSBT, PARSED_ORIGINAL_PSBT,
@@ -1418,7 +1407,7 @@ pub mod test {
         // Nothing was spent, should be in the same state
         let persister = InMemoryTestPersister::default();
         let res = monitor
-            .check_payment(|_| Ok(None), |_| Ok(false))
+            .check_payment(|_| Ok(None))
             .save(&persister)
             .expect("InMemoryTestPersister shouldn't fail");
         assert!(matches!(res, OptionalTransitionOutcome::Stasis(_)));
@@ -1428,7 +1417,7 @@ pub mod test {
         // Payjoin was broadcasted, should progress to success
         let persister = InMemoryTestPersister::default();
         let res = monitor
-            .check_payment(|_| Ok(Some(payjoin_tx.clone())), |_| Ok(false))
+            .check_payment(|_| Ok(Some(payjoin_tx.clone())))
             .save(&persister)
             .expect("InMemoryTestPersister shouldn't fail");
 
@@ -1446,17 +1435,14 @@ pub mod test {
         // Fallback was broadcasted, should progress to success
         let persister = InMemoryTestPersister::default();
         let res = monitor
-            .check_payment(
-                |txid| {
-                    // Emulate if one of the fallback outpoints was double spent
-                    if txid == original_tx.compute_txid() {
-                        Ok(Some(original_tx.clone()))
-                    } else {
-                        Ok(None)
-                    }
-                },
-                |_| Ok(false),
-            )
+            .check_payment(|txid| {
+                // Emulate if one of the fallback outpoints was double spent
+                if txid == original_tx.compute_txid() {
+                    Ok(Some(original_tx.clone()))
+                } else {
+                    Ok(None)
+                }
+            })
             .save(&persister)
             .expect("InMemoryTestPersister shouldn't fail");
 
@@ -1467,49 +1453,35 @@ pub mod test {
             Some(&SessionEvent::Closed(SessionOutcome::FallbackBroadcasted))
         );
 
-        let persister = InMemoryTestPersister::default();
-        let res = monitor
-            .check_payment(|_| Ok(None), |_| Ok(true))
-            .save(&persister)
-            .expect("InMemoryTestPersister shouldn't fail");
+        // Fallback transaction is non-SegWit address type, should end the session without checking
+        // the network for broadcasts.
+        // Not using the test-utils vectors here as they are SegWit.
+        let parsed_original_psbt_p2pkh = Psbt::from_str("cHNidP8BAFICAAAAAd5tU7sqAGa46oUVdEfV1HTeVVPYqvSvxy8/dvF3dwpZAQAAAAD9////AUTxBSoBAAAAFgAUhV1NWa6seBB5g6VZC2lnduxfEaUAAAAAAAEA/QoBAgAAAAIT2eO393FPqJ4fw6NH0rXALebtTCderecX0y6DumtjNgAAAAAA/f///5hrwcRiTXqXScbvk3APDdzy162Yj+6JD/iSEO9KYQl+AQAAAGpHMEQCIGcFm57xH5tQvJMipWfzxS7OGRi7+JfTT6WA27kOt8fVAiAp2I3WGdLk3/dVhoVxN6Jl9Wp/xeCIZZ1OTukSs8jszgEhAjjEq9kNnhvQbdVlWsE9QTIe4h39UPQ8flvU5Ivq6DFm/f///wIo3gUqAQAAABl2qRTWng6zTFWPZX1k12UqqBI6kLz8z4isAPIFKgEAAAAZdqkUIz2wzl605b3cg3j72nXReQuXXaWIrGcAAAABB2pHMEQCIEP33+9X/ecNmaiydM54HS+HoHfZygAQ/vMlc5r1IWkeAiA9oKjOVmp+RnrDF4zzHHGtoG1yy1+UWXBNaDiwd0LokgEhAmfCwbIv1mi5psiB3HFqXN1bFAo+goNUPWIso60J1matAAA=").expect("known psbt should parse");
+        let parsed_payjoin_proposal_p2pkh: Psbt =
+            Psbt::from_str("cHNidP8BAHsCAAAAAphrwcRiTXqXScbvk3APDdzy162Yj+6JD/iSEO9KYQl+AAAAAAD9////3m1TuyoAZrjqhRV0R9XUdN5VU9iq9K/HLz928Xd3ClkBAAAAAP3///8BsOILVAIAAAAWABSFXU1Zrqx4EHmDpVkLaWd27F8RpQAAAAAAAQCgAgAAAAJgEjBIihNzFXar4wIYepzXJwQVpbqZep9GCY8pQCqh3wAAAAAA/f///x8caN/onT7AOPRWJz7vnT6yiNxcsAIs/U3RcgU4kiq4AAAAAAD9////AgDyBSoBAAAAGXapFDGh2kOIa5aNVHT2bHSoFfcawEMiiKyk6QUqAQAAABl2qRQY8AsQvx+jg9NdGUwCuShS3qk2KYisZwAAAAEBIgDyBSoBAAAAGXapFDGh2kOIa5aNVHT2bHSoFfcawEMiiKwBB2pHMEQCICQEE2dMDzlyH3ojsc0l98Da0yd2ARuy5AcWQjlgHHjkAiA70WPB+yQhW5zhsOBTg6qLsi0KzoofRAj1BZFpKT2QwAEhA68L99Q+xdIIp0rinuVDs+4qmqMZwg4E+aqbTQ8RClXLAAEA/QoBAgAAAAIT2eO393FPqJ4fw6NH0rXALebtTCderecX0y6DumtjNgAAAAAA/f///5hrwcRiTXqXScbvk3APDdzy162Yj+6JD/iSEO9KYQl+AQAAAGpHMEQCIGcFm57xH5tQvJMipWfzxS7OGRi7+JfTT6WA27kOt8fVAiAp2I3WGdLk3/dVhoVxN6Jl9Wp/xeCIZZ1OTukSs8jszgEhAjjEq9kNnhvQbdVlWsE9QTIe4h39UPQ8flvU5Ivq6DFm/f///wIo3gUqAQAAABl2qRTWng6zTFWPZX1k12UqqBI6kLz8z4isAPIFKgEAAAAZdqkUIz2wzl605b3cg3j72nXReQuXXaWIrGcAAAAAAA==").expect("known psbt should parse");
 
-        assert!(matches!(res, OptionalTransitionOutcome::Progress(_)));
-        assert!(persister.inner.read().expect("Shouldn't be poisoned").is_closed);
-        assert_eq!(persister.inner.read().expect("Shouldn't be poisoned").events.len(), 1);
+        let psbt_ctx_p2pkh = PsbtContext {
+            original_psbt: parsed_original_psbt_p2pkh.clone(),
+            payjoin_psbt: parsed_payjoin_proposal_p2pkh.clone(),
+        };
+        let monitor = Receiver {
+            state: Monitor { psbt_context: psbt_ctx_p2pkh },
+            session_context: SHARED_CONTEXT.clone(),
+        };
 
         let persister = InMemoryTestPersister::default();
-        monitor
-            .check_payment(
-                |_| Ok(None),
-                |outpoint| {
-                    if outpoint == payjoin_tx.input[0].previous_output {
-                        Ok(true)
-                    } else {
-                        Ok(false)
-                    }
-                },
-            )
+        let res = monitor
+            .check_payment(|_| panic!("check_payment should return before this closure is called"))
             .save(&persister)
             .expect("InMemoryTestPersister shouldn't fail");
 
+        assert!(matches!(res, OptionalTransitionOutcome::Progress(_)));
         assert!(persister.inner.read().expect("Shouldn't be poisoned").is_closed);
-        assert_eq!(persister.inner.read().expect("Shouldn't be poisoned").events.len(), 1);
         assert_eq!(
             persister.inner.read().expect("Shouldn't be poisoned").events.last(),
-            Some(&SessionEvent::Closed(SessionOutcome::Failure))
+            Some(&SessionEvent::Closed(SessionOutcome::BroadcastStatusUnknown))
         );
 
-        // assert_eq!(
-        //     err.to_string(),
-        //     Error::Protocol(ProtocolError::V2(
-        //         InternalSessionError::FallbackOutpointsSpent(vec![
-        //             payjoin_tx.input[0].previous_output
-        //         ],)
-        //         .into()
-        //     ))
-        //     .to_string()
-        // );
-
         Ok(())
     }
 

payjoin/src/core/receive/v2/session.rs

@@ -119,7 +119,8 @@ impl SessionHistory {
         }
         match self.events.last() {
             Some(SessionEvent::Closed(outcome)) => match outcome {
-                SessionOutcome::Success(_) => SessionStatus::Completed,
+                SessionOutcome::Success(_) | SessionOutcome::BroadcastStatusUnknown =>
+                    SessionStatus::Completed,
                 SessionOutcome::Failure | SessionOutcome::Cancel => SessionStatus::Failed,
                 SessionOutcome::FallbackBroadcasted => SessionStatus::FallbackBroadcasted,
             },
@@ -169,6 +170,9 @@ pub enum SessionOutcome {
     Cancel,
     /// Fallback transaction was broadcasted
     FallbackBroadcasted,
+    /// The broadcast status of the sent Payjoin proposal cannot be tracked because the sender is
+    /// using non-SegWit addresses which will change the transaction ID of the proposal.
+    BroadcastStatusUnknown,
 }
 
 #[cfg(test)]

payjoin/tests/integration.rs

@@ -630,19 +630,14 @@ mod integration {
                 );
 
                 // monitor the payment on the receiver side
-                monitoring_payment.check_payment(
-                    |txid| {
-                        let tx = receiver
-                            .get_raw_transaction(txid)
-                            .expect("transaction should exist")
-                            .transaction()
-                            .expect("transaction should be decodable");
-                        Ok(Some(tx))
-                    },
-                    |_| {
-                        panic!("We should never check outpoints since the payjoin tx has been broadcasted")
-                    },
-                );
+                monitoring_payment.check_payment(|txid| {
+                    let tx = receiver
+                        .get_raw_transaction(txid)
+                        .expect("transaction should exist")
+                        .transaction()
+                        .expect("transaction should be decodable");
+                    Ok(Some(tx))
+                });
                 Ok(())
             }