Skip to content

Add expected_weight field to InputPair #772

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 8, 2025
Merged

Add expected_weight field to InputPair #772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
875d4a2
Add expected_weight field to InputPair
arturgontijo Jun 14, 2025
92996ec
Remove `witness_script` from input ctor
arminsabouri Jul 7, 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
2 changes: 1 addition & 1 deletion payjoin-cli/src/app/wallet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,5 +175,5 @@ pub fn input_pair_from_list_unspent(
previous_output: OutPoint { txid: utxo.txid, vout: utxo.vout },
..Default::default()
};
InputPair::new(txin, psbtin).expect("Input pair should be valid")
InputPair::new(txin, psbtin, None).expect("Input pair should be valid")
Copy link
Contributor

@shinghim shinghim Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Here, and in other places where InputPair::new is called, wouldn't this cause an issue if this is a script (p2sh, p2wsh, p2tr) spend? From @spacebear21's comment in #583:

the unlocking script (i.e. signature) could be any arbitrary script of unpredictable weight and can't be known in advance of signing. Since wallets should have the information required to predict the max size of the eventual signature, we can have them provide an "max expected weight" or similar parameter when constructing InputPairs.

If we're just putting None here, we'd run into the same issue of just not having the expected weight field wouldn't we?

Copy link
Collaborator

@spacebear21 spacebear21 Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we should probably switch to using the new_p2wpkh and other constructors for payjoin-cli. IIRC ListUnspentResultEntry has an address type field we could match on.

I think for payjoin-cli we could leave script spends unsupported for now since it's unlikely for bitcoin-cli users and not supported now anyway.

Copy link
Contributor Author

@arturgontijo arturgontijo Jun 25, 2025

Choose a reason for hiding this comment

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

The InputPair::new() calls the expected_input_weight() (in the first place) that sets the expected_weight (if AddressType is supported), if that call breaks we then try to set the weight using the given expected_weight param (Option<>).
That said, wouldn't that cover the AddressType check?

Copy link
Contributor

@shinghim shinghim Jun 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Am i correct in assuming there's an objective/correct way to calculate the expected weight? If we're calling expected_input_weight in InputPair::new, we would hopefully be doing the calculation the correct way, and wouldn't need the user to provide the expected weight themselves, since they would just be doing what expected_input_weight is doing. If that's the case, i think it would make sense to keep the new field in InputPair, but populate it only through InputPair::new calling expected_input_weight internally. Then, the constructors would be more ergonomic, and the user wouldn't need to do duplicate work of calculating the weight themselves

Copy link
Contributor Author

@arturgontijo arturgontijo Jun 27, 2025

Choose a reason for hiding this comment

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

The expected_weight field (provided by users/applications) would be used in situations where the expected_input_weight() is not able to predict the weight of the given InputPair (the issue that this PR tries to solve points out to one of them: #659)

Copy link
Contributor

@shinghim shinghim Jun 27, 2025

Choose a reason for hiding this comment

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

oh i see, the expected weight can only really be calculated for the key spends, and this is mostly for the script spends, which will just return an error if the user doesn't provide something, as those aren't supported in expected_input_weight()

}
2 changes: 1 addition & 1 deletion payjoin-ffi/python/test/test_payjoin_integration_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ def get_inputs(rpc_connection: RpcClient) -> list[InputPair]:
prev_amount = bitcoinffi.Amount.from_btc(prev_out["value"])
tx_out = bitcoinffi.TxOut(value=prev_amount, script_pubkey=prev_spk)
psbt_in = PsbtInput(witness_utxo=tx_out, redeem_script=None, witness_script=None)
inputs.append(InputPair(txin=txin, psbtin=psbt_in))
inputs.append(InputPair(txin=txin, psbtin=psbt_in, expected_weight=None))

return inputs

Expand Down
13 changes: 13 additions & 0 deletions payjoin-ffi/src/bitcoin_ffi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,16 @@ impl From<PsbtInput> for bitcoin::psbt::Input {
}
}
}

#[cfg_attr(feature = "uniffi", derive(uniffi::Record))]
pub struct Weight {
pub weight_units: u64,
}

impl From<bitcoin::Weight> for Weight {
fn from(weight: bitcoin::Weight) -> Self { Self { weight_units: weight.to_wu() } }
}

impl From<Weight> for bitcoin::Weight {
fn from(weight: Weight) -> Self { bitcoin::Weight::from_wu(weight.weight_units) }
}
7 changes: 6 additions & 1 deletion payjoin-ffi/src/receive/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,8 +690,13 @@ impl InputPair {
pub fn new(
txin: bitcoin_ffi::TxIn,
psbtin: crate::bitcoin_ffi::PsbtInput,
expected_weight: Option<crate::bitcoin_ffi::Weight>,
) -> Result<Self, PsbtInputError> {
Ok(Self(payjoin::receive::InputPair::new(txin.into(), psbtin.into())?))
Ok(Self(payjoin::receive::InputPair::new(
txin.into(),
psbtin.into(),
expected_weight.map(|w| w.into()),
)?))
}
}

Expand Down
2 changes: 1 addition & 1 deletion payjoin-ffi/tests/bdk_integration_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ fn input_pair_from_local_utxo(utxo: LocalUtxo) -> Result<InputPair, BoxError> {
previous_output: payjoin::bitcoin::OutPoint::from_str(&utxo.outpoint.to_string()).unwrap(),
..Default::default()
};
InputPair::new(txin.clone().into(), psbtin.clone().into())
InputPair::new(txin.clone().into(), psbtin.clone().into(), None)
.map_err(|e| format!("Failed to create input pair: {:?}", e).into())
}

Expand Down
14 changes: 9 additions & 5 deletions payjoin/src/core/psbt/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,8 @@ pub(crate) enum InternalPsbtInputError {
/// TxOut provided in `segwit_utxo` doesn't match the one in `non_segwit_utxo`
SegWitTxOutMismatch,
AddressType(AddressTypeError),
NoRedeemScript,
InvalidScriptPubKey(AddressType),
WeightError(InputWeightError),
}

impl fmt::Display for InternalPsbtInputError {
Expand All @@ -254,8 +254,8 @@ impl fmt::Display for InternalPsbtInputError {
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"),
Self::InvalidScriptPubKey(e) => write!(f, "provided script was not a valid type of {e}")
Self::InvalidScriptPubKey(e) => write!(f, "provided script was not a valid type of {e}"),
Self::WeightError(e) => write!(f, "{e}"),
}
}
}
Expand All @@ -267,8 +267,8 @@ impl std::error::Error for InternalPsbtInputError {
Self::UnequalTxid => None,
Self::SegWitTxOutMismatch => None,
Self::AddressType(error) => Some(error),
Self::NoRedeemScript => None,
Self::InvalidScriptPubKey(_) => None,
Self::WeightError(error) => Some(error),
}
}
}
Expand All @@ -281,6 +281,10 @@ impl From<AddressTypeError> for InternalPsbtInputError {
fn from(value: AddressTypeError) -> Self { Self::AddressType(value) }
}

impl From<InputWeightError> for InternalPsbtInputError {
fn from(value: InputWeightError) -> Self { Self::WeightError(value) }
}

#[derive(Debug, PartialEq, Eq)]
pub struct PsbtInputError(InternalPsbtInputError);

Expand Down Expand Up @@ -347,7 +351,7 @@ impl From<FromScriptError> for AddressTypeError {
fn from(value: FromScriptError) -> Self { Self::InvalidScript(value) }
}

#[derive(Debug, PartialEq)]
#[derive(Debug, PartialEq, Eq)]
pub(crate) enum InputWeightError {
AddressType(AddressTypeError),
NoRedeemScript,
Expand Down
5 changes: 0 additions & 5 deletions payjoin/src/core/receive/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,6 @@ pub(crate) enum InternalPayloadError {
#[allow(dead_code)]
/// The sender is trying to spend the receiver input
InputOwned(bitcoin::ScriptBuf),
/// The expected input weight cannot be determined
InputWeight(crate::psbt::InputWeightError),
#[allow(dead_code)]
/// Original PSBT input has been seen before. Only automatic receivers, aka "interactive" in the spec
/// look out for these to prevent probing attacks.
Expand All @@ -208,7 +206,6 @@ impl From<&PayloadError> for JsonReply {
| MissingPayment
| OriginalPsbtNotBroadcastable
| InputOwned(_)
| InputWeight(_)
| InputSeen(_)
| PsbtBelowFeeRate(_, _) => JsonReply::new(OriginalPsbtRejected, e),

Expand Down Expand Up @@ -241,7 +238,6 @@ impl fmt::Display for PayloadError {
MissingPayment => write!(f, "Missing payment."),
OriginalPsbtNotBroadcastable => write!(f, "Can't broadcast. PSBT rejected by mempool."),
InputOwned(_) => write!(f, "The receiver rejected the original PSBT."),
InputWeight(e) => write!(f, "InputWeight Error: {e}"),
InputSeen(_) => write!(f, "The receiver rejected the original PSBT."),
PsbtBelowFeeRate(original_psbt_fee_rate, receiver_min_fee_rate) => write!(
f,
Expand All @@ -264,7 +260,6 @@ impl std::error::Error for PayloadError {
SenderParams(e) => Some(e),
InconsistentPsbt(e) => Some(e),
PrevTxOut(e) => Some(e),
InputWeight(e) => Some(e),
PsbtBelowFeeRate(_, _) => None,
FeeTooHigh(_, _) => None,
MissingPayment => None,
Expand Down
Loading
Loading