`PayjoinProposal` typestate is leaky

[DanGould]

prepare_psbt requires no changes be done to the internal psbt after apply_fee is called. These two should be separated into something like ProvisionalProposal, from which inputs/outputs can be changed until apply_fee is called, which returns PayjoinProposal from which the Payjoin Proposal PSBT can be extracted.

[jbesraa]

happy to look into this, here is a start:

what struct fields do you think we should add to ProvisionalPayjoinProposal?
based on where/how they are used, I guess prepare_psbt should return Provisional..Proposal and apply_fee returns PayjoinProposal ?

few points about this:

• apply_fee input looks ugly
• we need to clone the output payjoin_psbt.unsigned_tx.output.clone()[.. in apply_fee function

/// Provisional payjoin proposal.
struct ProvisionalPayjoinProposal {}

impl ProvisionalPayjoinProposal {
    /// Apply additional fee contribution now that the receiver has contributed input
    /// this is kind of a "build_proposal" step before we sign and finalize and extract
    ///
    /// WARNING: DO NOT ALTER INPUTS OR OUTPUTS AFTER THIS STEP
    pub fn apply_fee(
        payjoin_psbt: Psbt,
        params: Params,
        owned_vouts: Vec<usize>,
        min_feerate_sat_per_vb: Option<u64>,
        original_psbt: Psbt,
    ) -> Result<PayjoinProposal, RequestError> {
        let min_feerate =
            FeeRate::from_sat_per_vb_unchecked(min_feerate_sat_per_vb.unwrap_or_default());
        log::trace!("min_feerate: {:?}", min_feerate);
        log::trace!("params.min_feerate: {:?}", params.min_feerate);
        let min_feerate = max(min_feerate, params.min_feerate);
        log::debug!("min_feerate: {:?}", min_feerate);

        // this error should never happen. We check for at least one input in the constructor
        let input_pair = payjoin_psbt
            .input_pairs()
            .next()
            .ok_or(InternalRequestError::OriginalPsbtNotBroadcastable)?;
        let txo = input_pair.previous_txout().map_err(InternalRequestError::PrevTxOut)?;
        let input_type = InputType::from_spent_input(txo, &payjoin_psbt.inputs[0])
            .map_err(InternalRequestError::InputType)?;
        let contribution_weight = input_type.expected_input_weight();
        log::trace!("contribution_weight: {}", contribution_weight);
        let mut additional_fee = contribution_weight * min_feerate;
        let max_additional_fee_contribution =
            params.additional_fee_contribution.unwrap_or_default().0;
        if additional_fee >= max_additional_fee_contribution {
            // Cap fee at the sender's contribution to simplify this method
            additional_fee = max_additional_fee_contribution;
        }
        log::trace!("additional_fee: {}", additional_fee);
        if additional_fee > bitcoin::Amount::ZERO {
            log::trace!(
                "self.params.additional_fee_contribution: {:?}",
                params.additional_fee_contribution
            );
            if let Some((_, additional_fee_output_index)) = params.additional_fee_contribution {
                if !owned_vouts.contains(&additional_fee_output_index) {
                    // remove additional miner fee from the sender's specified output
                    payjoin_psbt.unsigned_tx.output.clone()[additional_fee_output_index].value -=
                        additional_fee.to_sat();
                }
            }
        }
        Ok(PayjoinProposal {
            original_psbt: original_psbt,
            payjoin_psbt: payjoin_psbt,
            params: params,
            owned_vouts,
        })
    }

    /// Return a Payjoin Proposal PSBT that the sender will find acceptable.
    ///
    /// This attempts to calculate any network fee owed by the receiver, subtract it from their output,
    /// and return a PSBT that can produce a consensus-valid transaction that the sender will accept.
    ///
    /// wallet_process_psbt should sign and finalize receiver inputs
    pub fn prepare_psbt(
        original_psbt: Psbt,
        mut processed_psbt: Psbt,
    ) -> Result<Psbt, RequestError> {
        log::trace!("Preparing PSBT {:#?}", processed_psbt);
        for input in processed_psbt.inputs_mut() {
            input.bip32_derivation = BTreeMap::new();
            input.partial_sigs = BTreeMap::new();
        }
        // iterate proposal as mutable WITH the outpoint (previous_output) available too
        let mut original_inputs = original_psbt.input_pairs().peekable();
        let mut sender_input_indexes = vec![];
        for (i, input) in processed_psbt.input_pairs().enumerate() {
            if let Some(original) = original_inputs.peek() {
                log::trace!(
                    "match previous_output: {} == {}",
                    input.txin.previous_output,
                    original.txin.previous_output
                );
                if input.txin.previous_output == original.txin.previous_output {
                    sender_input_indexes.push(i);
                    original_inputs.next();
                }
            }
        }

        for i in sender_input_indexes {
            log::trace!("Clearing sender input {}", i);
            processed_psbt.inputs[i].non_witness_utxo = None;
            processed_psbt.inputs[i].witness_utxo = None;
            processed_psbt.inputs[i].final_script_sig = None;
            processed_psbt.inputs[i].final_script_witness = None;
        }
        Ok(processed_psbt)
    }
}

[jbesraa]

another point, in the original implementation in prepare_psbt function
in the first line we self.payjoin_psbt = processed_psbt;, in the new implementation I guess we just use the processed_psbt variable directly?

[DanGould]

By leaky, I mean that PayjoinProposal can be modified to be put into an invalid state. By returning a newtype, the downstream user should not be able create an invalid Payjoin PSBT with these methods. This is our goal. apply_fee calculates a transaction fee based on sender parameters and receiver inputs which subtract value from some of the outputs. Once this is done, the Ok Result should be a newtype preventing downstream user from adding in/outputs and breaking the fee calculation.

In my view, apply_fee and prepare_psbt should be combined in one step a sort of finalize_proposal which consumes a proposed ProvisionalProposal into a final PayjoinProposal type (containing payjoin_psbt, params, and owned_vouts). A ProvisionalProposal type would have the methods to modify inputs and outputs.

The imagined ProvisionalProposal methods should be improved but are out of scope for this issue.

[jbesraa]

thanks for the explanation, will try to resolve this