Skip to content

Improve Sender Input Validation Tests and Error Types #1836

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Jolah1 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Improve Sender Input Validation Tests and Error Types #1836

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2922a5b
Test receiver checks against the receiver input
Jolah1 Aug 25, 2026
283663e
Reject proposals missing original sender inputs
Jolah1 Aug 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 69 additions & 28 deletions payjoin/src/core/send/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

use bitcoin::psbt::{Psbt, PsbtSighashType};
use bitcoin::sighash::TapSighashType;
use bitcoin::{Amount, FeeRate, Script, ScriptBuf, TxOut, Weight};
use bitcoin::{Amount, FeeRate, Script, ScriptBuf, TxIn, TxOut, Weight};
pub use error::{BuildSenderError, ResponseError, ValidationError, WellKnownError};
pub(crate) use error::{InternalBuildSenderError, InternalProposalError, InternalValidationError};

Expand Down Expand Up @@ -285,6 +285,21 @@ fn commits_to_all_inputs_and_outputs(sighash_type: PsbtSighashType) -> bool {
matches!(sighash_type.taproot_hash_ty(), Ok(TapSighashType::Default | TapSighashType::All))
}

/// Whether `originals` appear among `proposed` in the same relative order.
///
/// BIP 78 forbids the receiver from shuffling: its additional inputs "must be inserted at a
/// random index", so the sender's inputs must survive as an ordered subsequence of the
/// proposal's.
fn is_ordered_subsequence<'a>(
originals: impl IntoIterator<Item = &'a TxIn>,
proposed: impl IntoIterator<Item = &'a TxIn>,
) -> bool {
let mut proposed = proposed.into_iter();
originals
.into_iter()
.all(|original| proposed.any(|p| p.previous_output == original.previous_output))
}

impl PsbtContext {
fn process_proposal(self, mut proposal: Psbt) -> InternalResult<Psbt> {
self.basic_checks(&proposal)?;
Expand Down Expand Up @@ -367,6 +382,14 @@ impl PsbtContext {
proposal: &Psbt,
ensure_receiver_input_finalized: bool,
) -> InternalResult<()> {
ensure(
is_ordered_subsequence(
&self.original_psbt.unsigned_tx.input,
&proposal.unsigned_tx.input,
),
InternalProposalError::MissingOrShuffledInputs,
)?;

let mut original_inputs = self.original_psbt.input_pairs().peekable();

for proposed in proposal.input_pairs() {
Expand Down Expand Up @@ -717,10 +740,11 @@ mod test {
use bitcoin::absolute::LockTime;
use bitcoin::bip32::{DerivationPath, Fingerprint};
use bitcoin::ecdsa::Signature;
use bitcoin::hashes::Hash;
use bitcoin::hex::FromHex;
use bitcoin::secp256k1::{Message, PublicKey, Secp256k1, SecretKey, SECP256K1};
use bitcoin::taproot::TaprootBuilder;
use bitcoin::{Amount, FeeRate, OutPoint, Script, ScriptBuf, Sequence, Witness};
use bitcoin::{Amount, FeeRate, OutPoint, Script, ScriptBuf, Sequence, Txid, Witness};
use payjoin_test_utils::{
BoxError, ADDITIONAL_FEE_OUTPUT_INDEX, MAX_ADDITIONAL_FEE_CONTRIBUTION,
PARSED_ORIGINAL_PSBT, PARSED_PAYJOIN_PROPOSAL, PARSED_PAYJOIN_PROPOSAL_WITH_SENDER_INFO,
Expand All @@ -747,6 +771,29 @@ mod test {
})
}

#[test]
fn ordered_subsequence_requires_original_order() {
let txin = |vout| TxIn {
previous_output: OutPoint::new(Txid::all_zeros(), vout),
..Default::default()
};
let originals = vec![txin(0), txin(1)];

assert!(is_ordered_subsequence(&originals, &vec![txin(0), txin(1)]));
// Receiver inputs may be inserted before, between, and after the sender's.
assert!(is_ordered_subsequence(&originals, &vec![txin(0), txin(9), txin(1)]));
Comment thread
xstoicunicornx marked this conversation as resolved.
assert!(is_ordered_subsequence(
&originals,
&vec![txin(3), txin(0), txin(9), txin(1), txin(3)]
));
assert!(!is_ordered_subsequence(
&originals,
&vec![txin(3), txin(1), txin(9), txin(0), txin(3)]
));
assert!(!is_ordered_subsequence(&originals, &vec![txin(1), txin(0)]));
assert!(!is_ordered_subsequence(&originals, &vec![txin(0)]));
}

#[test]
fn test_restore_original_utxos() -> Result<(), BoxError> {
let mut original_psbt = PARSED_ORIGINAL_PSBT.clone();
Expand Down Expand Up @@ -1241,6 +1288,19 @@ mod test {
Ok(())
}

#[test]
fn test_sender_input_outpoint_changed() -> Result<(), BoxError> {
let ctx = create_psbt_context()?;
let mut proposal: bitcoin::Psbt = PARSED_PAYJOIN_PROPOSAL.clone();
proposal.unsigned_tx.input[0].previous_output.vout += 1;

assert_eq!(
ctx.process_proposal(proposal).unwrap_err().to_string(),
InternalProposalError::MissingOrShuffledInputs.to_string()
);
Ok(())
}

#[test]
fn test_sender_input_final_script_sig_is_present() -> Result<(), BoxError> {
let ctx = create_psbt_context()?;
Expand Down Expand Up @@ -1397,14 +1457,9 @@ mod test {
let ctx = create_psbt_context()?;
let mut proposal: bitcoin::Psbt = PARSED_PAYJOIN_PROPOSAL.clone();

// If the outpoints are different, they are considered a receiver input and will be checked as such
let proposed_outpoint = proposal.unsigned_tx.input.first().unwrap().previous_output;
proposal.unsigned_tx.input.get_mut(0).unwrap().previous_output =
OutPoint::new(proposed_outpoint.txid, proposed_outpoint.vout + 1);

// Make the receiver's input un-finalized
proposal.inputs.get_mut(0).unwrap().final_script_sig = None;
proposal.inputs.get_mut(0).unwrap().final_script_witness = None;
// Make the receiver's input unfinalized.
proposal.inputs.get_mut(1).unwrap().final_script_sig = None;
proposal.inputs.get_mut(1).unwrap().final_script_witness = None;

assert_eq!(
ctx.process_proposal(proposal).unwrap_err().to_string(),
Expand All @@ -1419,16 +1474,9 @@ mod test {
let ctx = create_psbt_context()?;
let mut proposal: bitcoin::Psbt = PARSED_PAYJOIN_PROPOSAL.clone();

// If the outpoints are different, they are considered a receiver input and will be checked as such
let proposed_outpoint = proposal.unsigned_tx.input.first().unwrap().previous_output;
proposal.unsigned_tx.input.get_mut(0).unwrap().previous_output =
OutPoint::new(proposed_outpoint.txid, proposed_outpoint.vout + 1);
proposal.inputs.get_mut(0).unwrap().final_script_sig = Some(ScriptBuf::new());
proposal.inputs.get_mut(0).unwrap().final_script_witness = Some(Witness::new());

// Make the receiver's input un-finalized
proposal.inputs.get_mut(0).unwrap().witness_utxo = None;
proposal.inputs.get_mut(0).unwrap().non_witness_utxo = None;
// Remove the receiver's UTXO information.
proposal.inputs.get_mut(1).unwrap().witness_utxo = None;
proposal.inputs.get_mut(1).unwrap().non_witness_utxo = None;

assert_eq!(
ctx.process_proposal(proposal).unwrap_err().to_string(),
Expand All @@ -1443,16 +1491,9 @@ mod test {
let mut ctx = create_psbt_context()?;
let mut proposal: bitcoin::Psbt = PARSED_PAYJOIN_PROPOSAL.clone();

// If the outpoints are different, they are considered a receiver input and will be checked as such
let proposed_outpoint = proposal.unsigned_tx.input.first().unwrap().previous_output;
proposal.unsigned_tx.input.get_mut(0).unwrap().previous_output =
OutPoint::new(proposed_outpoint.txid, proposed_outpoint.vout + 1);
proposal.inputs.get_mut(0).unwrap().final_script_sig = Some(ScriptBuf::new());
proposal.inputs.get_mut(0).unwrap().final_script_witness = Some(Witness::new());

// Ensure the sequence is different
let sequence = ctx.original_psbt.unsigned_tx.input.get_mut(0).unwrap().sequence;
proposal.unsigned_tx.input.get_mut(0).unwrap().sequence =
proposal.unsigned_tx.input.get_mut(1).unwrap().sequence =
Sequence::from_consensus(sequence.to_consensus_u32() + 1);

assert_eq!(
Expand Down
Loading