Skip to content

Remove unneeded PSBT fields when extracting request #907

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
arminsabouri merged 2 commits into from
Jul 29, 2025
Merged

Remove unneeded PSBT fields when extracting request #907

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea8dcd8
Remove unneeded PSBT fields when extracting request
arminsabouri Jul 29, 2025
a276767
Update `test_clear_unneeded_fields` to call `create_v1_post_request`
arminsabouri Jul 29, 2025
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
50 changes: 45 additions & 5 deletions payjoin/src/core/send/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ impl PsbtContextBuilder {
self,
output_substitution: OutputSubstitution,
) -> Result<PsbtContext, BuildSenderError> {
let mut psbt =
let psbt =
self.psbt.validate().map_err(InternalBuildSenderError::InconsistentOriginalPsbt)?;
psbt.validate_input_utxos().map_err(InternalBuildSenderError::InvalidOriginalInput)?;

Expand All @@ -214,7 +214,6 @@ impl PsbtContextBuilder {
self.fee_contribution,
self.clamp_fee_contribution,
)?;
clear_unneeded_fields(&mut psbt);

Ok(PsbtContext {
original_psbt: psbt,
Expand Down Expand Up @@ -662,7 +661,9 @@ pub(crate) fn create_v1_post_request(endpoint: Url, psbt_ctx: PsbtContext) -> (R
psbt_ctx.min_fee_rate,
Version::One,
);
let body = psbt_ctx.original_psbt.to_string().as_bytes().to_vec();
let mut sanitized_psbt = psbt_ctx.original_psbt.clone();
clear_unneeded_fields(&mut sanitized_psbt);
Copy link
Collaborator

@spacebear21 spacebear21 Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to actually validate this behavior in a test, I can comment out this line and all tests are still green despite it violating the BIP78 spec. We could augment test_clear_unneeded_fields so it checks the request body instead of just validating the function internals?

Copy link
Collaborator Author

@arminsabouri arminsabouri Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we could augment the new test that I have introduced. Eitherway happy to make this change

Copy link
Collaborator

@benalleng benalleng Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could add some assertions in our v1 integration test for the most minimal testing.

rust-payjoin/payjoin/tests/integration.rs

Lines 88 to 91 in 1f39170

let (req, ctx) = SenderBuilder::new(psbt, uri)
.build_with_additional_fee(Amount::from_sat(10000), None, FeeRate::ZERO, false)?
.create_v1_post_request();
let headers = HeaderMock::new(&req.body, req.content_type);

Copy link
Collaborator Author

@arminsabouri arminsabouri Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually what you suggested may be easier

Copy link
Collaborator

@benalleng benalleng Jul 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that might not be the right create_v1_post_request method? ... they're all public scratch that it looks good I got all my wires crossed

let body = sanitized_psbt.to_string().as_bytes().to_vec();
(
Request::new_v1(&url, &body),
V1Context {
Expand Down Expand Up @@ -708,9 +709,10 @@ mod test {
use std::str::FromStr;

use bitcoin::absolute::LockTime;
use bitcoin::bip32::{DerivationPath, Fingerprint};
use bitcoin::ecdsa::Signature;
use bitcoin::hex::FromHex;
use bitcoin::secp256k1::{Message, PublicKey, Secp256k1, SecretKey};
use bitcoin::secp256k1::{Message, PublicKey, Secp256k1, SecretKey, SECP256K1};
use bitcoin::{
Amount, FeeRate, OutPoint, Script, ScriptBuf, Sequence, Witness, XOnlyPublicKey,
};
Expand Down Expand Up @@ -740,6 +742,38 @@ mod test {
})
}

#[test]
fn test_restore_original_utxos() -> Result<(), BoxError> {
let mut original_psbt = PARSED_ORIGINAL_PSBT.clone();
let mut payjoin_proposal = PARSED_PAYJOIN_PROPOSAL.clone();
let payee = original_psbt.unsigned_tx.output[1].script_pubkey.clone();
let (_, pk) = SECP256K1.generate_keypair(&mut bitcoin::key::rand::thread_rng());
let x_only = pk.x_only_public_key().0;
// Fill out dummy data in the original PSBT so we can restore it
let _ = original_psbt.inputs[0].tap_internal_key.insert(x_only);
let _ = original_psbt.outputs[0].tap_internal_key.insert(x_only);
original_psbt.inputs[0]
.bip32_derivation
.insert(pk, (Fingerprint::default(), DerivationPath::default()));
original_psbt.inputs[0]
.tap_key_origins
.insert(x_only, (vec![], (Fingerprint::default(), DerivationPath::default())));

let prev_txout = TxOut { value: Amount::ONE_BTC, script_pubkey: payee.clone() };
original_psbt.inputs[0].witness_utxo = Some(prev_txout.clone());
let psbt_ctx = PsbtContextBuilder::new(original_psbt, payee, None)
.build(OutputSubstitution::Disabled)?;
clear_unneeded_fields(&mut payjoin_proposal);

psbt_ctx.restore_original_utxos(&mut payjoin_proposal)?;
assert!(payjoin_proposal.inputs[0].bip32_derivation.contains_key(&pk));
assert!(payjoin_proposal.inputs[0].tap_key_origins.contains_key(&x_only));
assert_eq!(payjoin_proposal.inputs[0].witness_utxo, Some(prev_txout));
assert_eq!(payjoin_proposal.inputs[0].tap_internal_key, Some(x_only));

Ok(())
}

#[test]
fn test_determine_fees() -> Result<(), BoxError> {
let fee_contribution = determine_fee_contribution(
Expand Down Expand Up @@ -978,6 +1012,7 @@ mod test {
#[test]
fn test_clear_unneeded_fields() -> Result<(), BoxError> {
let mut proposal = PARSED_PAYJOIN_PROPOSAL_WITH_SENDER_INFO.clone();
let payee = proposal.unsigned_tx.output[1].script_pubkey.clone();
let x_only_key = XOnlyPublicKey::from_str(
"4f65949efe60e5be80cf171c06144641e832815de4f6ab3fe0257351aeb22a84",
)?;
Expand All @@ -987,7 +1022,12 @@ mod test {
assert!(!proposal.inputs[0].bip32_derivation.is_empty());
assert!(proposal.outputs[0].tap_internal_key.is_some());
assert!(!proposal.outputs[0].bip32_derivation.is_empty());
clear_unneeded_fields(&mut proposal);
let psbt_ctx = PsbtContextBuilder::new(proposal.clone(), payee, None)
.build(OutputSubstitution::Disabled)?;

let body = create_v1_post_request(Url::from_str("HTTPS://EXAMPLE.COM/")?, psbt_ctx).0.body;
let res_str = std::str::from_utf8(&body)?;
let proposal = Psbt::from_str(res_str)?;
assert!(proposal.inputs[0].tap_internal_key.is_none());
assert!(proposal.inputs[0].bip32_derivation.is_empty());
assert!(proposal.outputs[0].tap_internal_key.is_none());
Expand Down