Modify try_preserving_privacy function signature (payjoin#377)

Addresses payjoin#32. Instead of
having `try_preserving_privacy` modify the PSBT directly as suggested in
that issue, we keep the "coin selection" and "input contribution" steps
separate but simplify the function interface so that they're easier to
chain and reason about.

`try_preserving_privacy` now takes a `InputPair` iterator
instead of a bespoke `(Amount, Outpoint)` iterator which is lossy and
unwieldy. It also returns a single `InputPair` instead of just
an `OutPoint`.

This is the same type that `contribute_inputs` takes as a parameter.

Author: spacebear21

payjoin-cli/src/app/mod.rs

@@ -7,6 +7,7 @@ use bitcoin::TxIn;
 use bitcoincore_rpc::bitcoin::Amount;
 use bitcoincore_rpc::RpcApi;
 use payjoin::bitcoin::psbt::Psbt;
+use payjoin::receive::InputPair;
 use payjoin::send::Sender;
 use payjoin::{bitcoin, PjUri};
 
@@ -124,8 +125,8 @@ fn read_local_cert() -> Result<Vec<u8>> {
 }
 
 pub fn input_pair_from_list_unspent(
-    utxo: &bitcoincore_rpc::bitcoincore_rpc_json::ListUnspentResultEntry,
-) -> (PsbtInput, TxIn) {
+    utxo: bitcoincore_rpc::bitcoincore_rpc_json::ListUnspentResultEntry,
+) -> InputPair {
     let psbtin = PsbtInput {
         // NOTE: non_witness_utxo is not necessary because bitcoin-cli always supplies
         // witness_utxo, even for non-witness inputs
@@ -141,5 +142,5 @@ pub fn input_pair_from_list_unspent(
         previous_output: bitcoin::OutPoint { txid: utxo.txid, vout: utxo.vout },
         ..Default::default()
     };
-    (psbtin, txin)
+    InputPair::new(txin, psbtin).expect("Input pair should be valid")
 }

payjoin-cli/src/app/v1.rs

@@ -375,28 +375,18 @@ fn try_contributing_inputs(
     payjoin: payjoin::receive::WantsInputs,
     bitcoind: &bitcoincore_rpc::Client,
 ) -> Result<payjoin::receive::ProvisionalProposal> {
-    use bitcoin::OutPoint;
-
-    let available_inputs = bitcoind
+    let candidate_inputs = bitcoind
         .list_unspent(None, None, None, None, None)
-        .context("Failed to list unspent from bitcoind")?;
-    let candidate_inputs: HashMap<Amount, OutPoint> = available_inputs
-        .iter()
-        .map(|i| (i.amount, OutPoint { txid: i.txid, vout: i.vout }))
-        .collect();
-
-    let selected_outpoint = payjoin
+        .context("Failed to list unspent from bitcoind")?
+        .into_iter()
+        .map(input_pair_from_list_unspent);
+    let selected_input = payjoin
         .try_preserving_privacy(candidate_inputs)
         .map_err(|e| anyhow!("Failed to make privacy preserving selection: {}", e))?;
-    let selected_utxo = available_inputs
-        .iter()
-        .find(|i| i.txid == selected_outpoint.txid && i.vout == selected_outpoint.vout)
-        .context("This shouldn't happen. Failed to retrieve the privacy preserving utxo from those we provided to the seclector.")?;
-    log::debug!("selected utxo: {:#?}", selected_utxo);
-    let input_pair = input_pair_from_list_unspent(selected_utxo);
+    log::debug!("selected input: {:#?}", selected_input);
 
     Ok(payjoin
-        .contribute_inputs(vec![input_pair])
+        .contribute_inputs(vec![selected_input])
         .expect("This shouldn't happen. Failed to contribute inputs.")
         .commit_inputs())
 }

payjoin-cli/src/app/v2.rs

@@ -1,4 +1,3 @@
-use std::collections::HashMap;
 use std::str::FromStr;
 use std::sync::Arc;
 
@@ -365,28 +364,18 @@ fn try_contributing_inputs(
     payjoin: payjoin::receive::v2::WantsInputs,
     bitcoind: &bitcoincore_rpc::Client,
 ) -> Result<payjoin::receive::v2::ProvisionalProposal> {
-    use bitcoin::OutPoint;
-
-    let available_inputs = bitcoind
+    let candidate_inputs = bitcoind
         .list_unspent(None, None, None, None, None)
-        .context("Failed to list unspent from bitcoind")?;
-    let candidate_inputs: HashMap<Amount, OutPoint> = available_inputs
-        .iter()
-        .map(|i| (i.amount, OutPoint { txid: i.txid, vout: i.vout }))
-        .collect();
-
-    let selected_outpoint = payjoin
+        .context("Failed to list unspent from bitcoind")?
+        .into_iter()
+        .map(input_pair_from_list_unspent);
+    let selected_input = payjoin
         .try_preserving_privacy(candidate_inputs)
         .map_err(|e| anyhow!("Failed to make privacy preserving selection: {}", e))?;
-    let selected_utxo = available_inputs
-        .iter()
-        .find(|i| i.txid == selected_outpoint.txid && i.vout == selected_outpoint.vout)
-        .context("This shouldn't happen. Failed to retrieve the privacy preserving utxo from those we provided to the seclector.")?;
-    log::debug!("selected utxo: {:#?}", selected_utxo);
-    let input_pair = input_pair_from_list_unspent(selected_utxo);
+    log::debug!("selected input: {:#?}", selected_input);
 
     Ok(payjoin
-        .contribute_inputs(vec![input_pair])
+        .contribute_inputs(vec![selected_input])
         .expect("This shouldn't happen. Failed to contribute inputs.")
         .commit_inputs())
 }

payjoin/src/psbt.rs

@@ -35,7 +35,7 @@ pub(crate) trait PsbtExt: Sized {
     ) -> &mut BTreeMap<bip32::Xpub, (bip32::Fingerprint, bip32::DerivationPath)>;
     fn proprietary_mut(&mut self) -> &mut BTreeMap<psbt::raw::ProprietaryKey, Vec<u8>>;
     fn unknown_mut(&mut self) -> &mut BTreeMap<psbt::raw::Key, Vec<u8>>;
-    fn input_pairs(&self) -> Box<dyn Iterator<Item = InputPair<'_>> + '_>;
+    fn input_pairs(&self) -> Box<dyn Iterator<Item = InternalInputPair<'_>> + '_>;
     // guarantees that length of psbt input matches that of unsigned_tx inputs and same
     /// thing for outputs.
     fn validate(self) -> Result<Self, InconsistentPsbt>;
@@ -59,13 +59,13 @@ impl PsbtExt for Psbt {
 
     fn unknown_mut(&mut self) -> &mut BTreeMap<psbt::raw::Key, Vec<u8>> { &mut self.unknown }
 
-    fn input_pairs(&self) -> Box<dyn Iterator<Item = InputPair<'_>> + '_> {
+    fn input_pairs(&self) -> Box<dyn Iterator<Item = InternalInputPair<'_>> + '_> {
         Box::new(
             self.unsigned_tx
                 .input
                 .iter()
                 .zip(&self.inputs)
-                .map(|(txin, psbtin)| InputPair { txin, psbtin }),
+                .map(|(txin, psbtin)| InternalInputPair { txin, psbtin }),
         )
     }
 
@@ -106,12 +106,13 @@ fn redeem_script(script_sig: &Script) -> Option<&Script> {
 // https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#p2wpkh-nested-in-bip16-p2sh
 const NESTED_P2WPKH_MAX: InputWeightPrediction = InputWeightPrediction::from_slice(23, &[72, 33]);
 
-pub(crate) struct InputPair<'a> {
+#[derive(Clone, Debug)]
+pub(crate) struct InternalInputPair<'a> {
     pub txin: &'a TxIn,
     pub psbtin: &'a psbt::Input,
 }
 
-impl<'a> InputPair<'a> {
+impl<'a> InternalInputPair<'a> {
     /// Returns TxOut associated with the input
     pub fn previous_txout(&self) -> Result<&TxOut, PrevTxOutError> {
         match (&self.psbtin.non_witness_utxo, &self.psbtin.witness_utxo) {
@@ -132,10 +133,13 @@ impl<'a> InputPair<'a> {
         }
     }
 
-    pub fn validate_utxo(&self, treat_missing_as_error: bool) -> Result<(), PsbtInputError> {
+    pub fn validate_utxo(
+        &self,
+        treat_missing_as_error: bool,
+    ) -> Result<(), InternalPsbtInputError> {
         match (&self.psbtin.non_witness_utxo, &self.psbtin.witness_utxo) {
             (None, None) if treat_missing_as_error =>
-                Err(PsbtInputError::PrevTxOut(PrevTxOutError::MissingUtxoInformation)),
+                Err(InternalPsbtInputError::PrevTxOut(PrevTxOutError::MissingUtxoInformation)),
             (None, None) => Ok(()),
             (Some(tx), None) if tx.compute_txid() == self.txin.previous_output.txid => tx
                 .output
@@ -153,7 +157,7 @@ impl<'a> InputPair<'a> {
                     .into()
                 })
                 .map(drop),
-            (Some(_), None) => Err(PsbtInputError::UnequalTxid),
+            (Some(_), None) => Err(InternalPsbtInputError::UnequalTxid),
             (None, Some(_)) => Ok(()),
             (Some(tx), Some(witness_txout))
                 if tx.compute_txid() == self.txin.previous_output.txid =>
@@ -173,10 +177,10 @@ impl<'a> InputPair<'a> {
                 if witness_txout == non_witness_txout {
                     Ok(())
                 } else {
-                    Err(PsbtInputError::SegWitTxOutMismatch)
+                    Err(InternalPsbtInputError::SegWitTxOutMismatch)
                 }
             }
-            (Some(_), Some(_)) => Err(PsbtInputError::UnequalTxid),
+            (Some(_), Some(_)) => Err(InternalPsbtInputError::UnequalTxid),
         }
     }
 
@@ -245,41 +249,66 @@ impl fmt::Display for PrevTxOutError {
 impl std::error::Error for PrevTxOutError {}
 
 #[derive(Debug)]
-pub(crate) enum PsbtInputError {
+pub(crate) enum InternalPsbtInputError {
     PrevTxOut(PrevTxOutError),
     UnequalTxid,
     /// TxOut provided in `segwit_utxo` doesn't match the one in `non_segwit_utxo`
     SegWitTxOutMismatch,
+    AddressType(AddressTypeError),
+    NoRedeemScript,
 }
 
-impl fmt::Display for PsbtInputError {
+impl fmt::Display for InternalPsbtInputError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
-            PsbtInputError::PrevTxOut(_) => write!(f, "invalid previous transaction output"),
-            PsbtInputError::UnequalTxid => write!(f, "transaction ID of previous transaction doesn't match one specified in input spending it"),
-            PsbtInputError::SegWitTxOutMismatch => write!(f, "transaction output provided in SegWit UTXO field doesn't match the one in non-SegWit UTXO field"),
+            Self::PrevTxOut(_) => write!(f, "invalid previous transaction output"),
+            Self::UnequalTxid => write!(f, "transaction ID of previous transaction doesn't match one specified in input spending it"),
+            Self::SegWitTxOutMismatch => write!(f, "transaction output provided in SegWit UTXO field doesn't match the one in non-SegWit UTXO field"),
+            Self::AddressType(_) => write!(f, "invalid address type"),
+            Self::NoRedeemScript => write!(f, "provided p2sh PSBT input is missing a redeem_script"),
         }
     }
 }
 
-impl std::error::Error for PsbtInputError {
+impl std::error::Error for InternalPsbtInputError {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         match self {
-            PsbtInputError::PrevTxOut(error) => Some(error),
-            PsbtInputError::UnequalTxid => None,
-            PsbtInputError::SegWitTxOutMismatch => None,
+            Self::PrevTxOut(error) => Some(error),
+            Self::UnequalTxid => None,
+            Self::SegWitTxOutMismatch => None,
+            Self::AddressType(error) => Some(error),
+            Self::NoRedeemScript => None,
         }
     }
 }
 
-impl From<PrevTxOutError> for PsbtInputError {
-    fn from(value: PrevTxOutError) -> Self { PsbtInputError::PrevTxOut(value) }
+impl From<PrevTxOutError> for InternalPsbtInputError {
+    fn from(value: PrevTxOutError) -> Self { InternalPsbtInputError::PrevTxOut(value) }
+}
+
+impl From<AddressTypeError> for InternalPsbtInputError {
+    fn from(value: AddressTypeError) -> Self { Self::AddressType(value) }
+}
+
+#[derive(Debug)]
+pub struct PsbtInputError(InternalPsbtInputError);
+
+impl From<InternalPsbtInputError> for PsbtInputError {
+    fn from(e: InternalPsbtInputError) -> Self { PsbtInputError(e) }
+}
+
+impl fmt::Display for PsbtInputError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "{}", self.0) }
+}
+
+impl std::error::Error for PsbtInputError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { Some(&self.0) }
 }
 
 #[derive(Debug)]
 pub struct PsbtInputsError {
     index: usize,
-    error: PsbtInputError,
+    error: InternalPsbtInputError,
 }
 
 impl fmt::Display for PsbtInputsError {

payjoin/src/receive/error.rs

@@ -289,8 +289,6 @@ pub struct InputContributionError(InternalInputContributionError);
 
 #[derive(Debug)]
 pub(crate) enum InternalInputContributionError {
-    /// Missing previous txout information
-    PrevTxOut(crate::psbt::PrevTxOutError),
     /// The address type could not be determined
     AddressType(crate::psbt::AddressTypeError),
     /// The original PSBT has no inputs
@@ -304,8 +302,6 @@ pub(crate) enum InternalInputContributionError {
 impl fmt::Display for InputContributionError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match &self.0 {
-            InternalInputContributionError::PrevTxOut(e) =>
-                write!(f, "Missing previous txout information: {}", e),
             InternalInputContributionError::AddressType(e) =>
                 write!(f, "The address type could not be determined: {}", e),
             InternalInputContributionError::NoSenderInputs =>