Use native rust-bitcoin methods for weight calculations by spacebear21 · Pull Request #360 · payjoin/rust-payjoin

spacebear21 · 2024-09-26T20:01:52Z

This deletes the custom weight.rs traits and input_type.rs helpers in favor of out-of-the-box rust-bitcoin methods. It also removes the unused output_type.rs.

A question that may be relevant to this PR: do we want to commit to allowing mixed input types (despite it being a violation of the BIP78 spec) to better enable batching use cases? Maybe we could introduce a allow_mixed_input_types parameter that can be set by the sender and defaults to false if not set?

DanGould · 2024-09-26T20:27:00Z

pretty massive delete! eager to follow

DanGould · 2024-09-26T20:47:26Z

I'm partial to allow mixed input types, and that opinion appears to be consensus. In addition, allow by default is more permissive and should have fewer failed attempts, I'm not sure I'd even want to expose the option unless it were strictly necessary for a downstream usecase if the logic weren't already in the codebase for v1.

Use upstream fee and weight methods from the `psbt` crate instead of manually calculating everything.

DanGould · 2024-10-05T23:49:28Z

payjoin/src/input_type.rs

+            // witness: <signature> <pubkey> = 72, 33 bytes
+            // https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#p2wpkh-nested-in-bip16-p2sh
+                InputWeightPrediction::new(23, &[72, 33]),
            SegWitV0 { ty: SegWitV0Type::Script, nested: _ } => unimplemented!(),


The suggestion for P2SH-P2WPKH listed as 91 in the fee output section of BIP 78, is this estimate removed because we can't tell wether the nested script is P2WPKH or not?

This clause applies to P2SH-P2WSH. The suggestion for P2SH-P2WPKH is implemented just above it with InputWeightPrediction::new(23, &[72, 33]) which corresponds to the suggested 91 vsize after adding the txid, index, and sequence bytes.

DanGould · 2024-10-05T23:55:45Z

payjoin/src/psbt.rs

+                    // input script: 0x160014{20-byte-key-hash} = 23 bytes
+                    // witness: <signature> <pubkey> = 72, 33 bytes
+                    // https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#p2wpkh-nested-in-bip16-p2sh
+                        InputWeightPrediction::new(23, &[72, 33]),


Seems like this is a constant value shared with the input_type match block. Perhaps it should be declared as a static value in our lib or even upstreamed as const into rust-bitcoin.

Opened a PR in rust-bitcoin to upstream it: rust-bitcoin/rust-bitcoin#3443

DanGould · 2024-10-06T00:22:40Z

payjoin/src/send/mod.rs

+            let input_pairs = self.psbt.input_pairs().collect::<Vec<InputPair>>();
+
+            let first_input_pair =
+                input_pairs.first().ok_or(InternalCreateRequestError::NoInputs)?;
+            let input_weight = if input_pairs
+                .iter()
+                .try_fold(true, |_, input_pair| -> Result<bool, crate::psbt::AddressTypeError> {
+                    Ok(input_pair.address_type()? == first_input_pair.address_type()?)
                })


Two things here

I believe the try_fold implementation is incorrect unless the accumulator is 'folded' into the condition. In the current implementation, if all of the addresses were mismatches except for the first and last, then true is still returned. Folding in the bool checked against each other using the accumulator.

I think there's a slight performance improvement by using next rather than first. collect` the input_pairs, since once the first_input_pair is retrieved the iterator can proceed rather than iterating from the beginning

1 fixes correctness 2 is just a performance improvement.

Suggested change

let input_pairs = self.psbt.input_pairs().collect::<Vec<InputPair>>();

let first_input_pair =

input_pairs.first().ok_or(InternalCreateRequestError::NoInputs)?;

let input_weight = if input_pairs

.iter()

.try_fold(true, |_, input_pair| -> Result<bool, crate::psbt::AddressTypeError> {

Ok(input_pair.address_type()? == first_input_pair.address_type()?)

})

let mut input_pairs = self.psbt.input_pairs().collect::<Vec<InputPair>>().into_iter();

let first_input_pair =

input_pairs.next().ok_or(InternalCreateRequestError::NoInputs)?;

let input_weight = if input_pairs

.try_fold(true, |acc, input_pair| -> Result<bool, crate::psbt::AddressTypeError> {

Ok(acc && input_pair.address_type()? == first_input_pair.address_type()?)

})

Good catch. I rewrote this as a simple while loop which breaks if an input is not the same type as the first one. That seems easier to reason about.

The new while loop is way easier to reason about

I can't recall where the rule to // use cheapest default if mixed input types` (P2TR) estimation method comes from when mixed input types are present. Wouldn't it make more sense to use the cheapest of the scripts proposed? As long as we make it easy for another compatible implementation to come along, I don't think it so much matters either way.

spacebear21 · 2024-10-08T02:34:55Z

@DanGould Summary of changes since your last review:

Fixed the try_fold issue you brought up.
Removed the input_type and sequence fields from the ContextV1 and RequestContext structs, as they can be computed from the original psbt.
Added a unit test for input weight calculations. I used payjoin-cli with RUST_LOG=trace to generate and obtain the original and payjoin PSBT values, and a new bitcoin-cli wallet for each address type.

DanGould · 2024-10-08T05:04:14Z

Try_fold fix looks good
input_type and sequence removal also looking good

I haven't checked the PSBT values in the known-weight tests, so I'm on that tomorrow. I think that's the last substantive check to make.

DanGould

I verified the additional input weight comes from scripts of the stated address types. The UTXO locking scripts are as follows and seem to be the the specified AddressType

p2pkh_proposal adds vout: 0 having p2pkh script OP_DUP OP_HASH160 OP_PUSHBYTES_20 3d786edc8e83f51d30529bd3bef526f9518d70db OP_EQUALVERIFY OP_CHECKSIG
nested_p2wpkh_proposal adds vout: 0 having p2sh script OP_HASH160 OP_PUSHBYTES_20 c7ff991c104167ee84fd44b537639eec80c8b578 OP_EQUAL
p2wpkh_proposal adds vout: 0 having p2wpkh script OP_0 OP_PUSHBYTES_20 916d91f1d3b77cb510331a37a0858e3356b69091
p2tr_proposal adds vout: 0 having p2tr script OP_PUSHNUM_1 OP_PUSHBYTES_32 9f6db5fe14725bcc2ef578414bb0c8c3a2e3096eef2881e47478da0dc4c33d0b

The only nits I'd ask for are for the warnings of unused variables to be removed from

secp256k1 dependencies in send/mod.rs
let sequence = original_psbt.unsigned_tx.input[0].sequence; in send/mod.rs

run cargo clippy and the warnings should become apparent, as well as using a for loop in send/mod.rs:136 and dropping the & reference on [72,33] at psbt.rs:201.

and for the input_weight_calculations test name to be additional_input_matches_known_weight or similar boolean statement

The only field that's necessary to keep is contributed_fee, which is now returned as standalone bitcoin::Amount in check_outputs()

…tion Instead of hardcoded values, use InputWeightPredictions to calculate expected input weight.

bitcoin::AddressType can act as a substitute for InputType, and we can use that to compute expected input weights. InputPair contains all the context necessary to derive those properties, so input_type.rs is obsolete.

Code cleanup: deletes input_type.rs, output_type.rs and weight.rs which are no longer needed.

spacebear21 · 2024-10-08T22:14:05Z

Thanks - fixed the clippy warnings and test name.

DanGould · 2024-10-08T22:49:49Z

payjoin/src/receive/mod.rs

+            .scan(&mut err, |err, input| match Ok(input.address_type()) {
+                Ok(address_type) => Some(address_type),


Seems like this will never reach the Err(e) branch since match is directly called on an Ok value?

Yes - this was a temporary workaround before I had address_type() return a Result. It gets updated in 6938b70#diff-5fba6fc39c59b07e5980d87da92537138068364d192a2b63eee5e57af01a07bb

DanGould · 2024-10-08T22:55:04Z

payjoin/src/send/mod.rs

+                        .original_psbt
+                        .input_pairs()
+                        .next()
+                        .expect("original PSBT should have an input");


Can we handle this error? Or you think it's so extremely unlikely it doesn't matter?

It would mean the original PSBT had no inputs, which seems improbable at this stage since the receiver would've accepted it and responded with a payjoin proposal. I could add a proper error type for it though.

Added proper handling for this error in 68fc635

Input type and sequence can be computed at little cost from the original PSBT, which is already included in RequestContext/ContextV1. Remove those fields from those structs and compute them when needed.

I used payjoin-cli with `RUST_LOG=trace` to generate and obtain the original and payjoin PSBT values, and a new bitcoin-cli wallet for each address type.

spacebear21 force-pushed the predict-weight branch 5 times, most recently from 20c1273 to f580454 Compare October 3, 2024 15:49

Simplify check_fees()

d3e123f

Use upstream fee and weight methods from the `psbt` crate instead of manually calculating everything.

spacebear21 force-pushed the predict-weight branch 2 times, most recently from f9ff6ad to 500894a Compare October 3, 2024 17:54

spacebear21 marked this pull request as ready for review October 3, 2024 18:23

spacebear21 mentioned this pull request Oct 3, 2024

Moving bunch of stuff from my payjoin crate here rust-bitcoin/rust-bitcoin#630

Open

spacebear21 requested a review from DanGould October 3, 2024 18:24

spacebear21 force-pushed the predict-weight branch from 500894a to 08f3d9e Compare October 3, 2024 21:28

DanGould reviewed Oct 6, 2024

View reviewed changes

spacebear21 force-pushed the predict-weight branch from 08f3d9e to ddcaa05 Compare October 7, 2024 20:48

DanGould self-requested a review October 8, 2024 05:30

spacebear21 force-pushed the predict-weight branch from 252381a to 2cef4ea Compare October 8, 2024 16:49

DanGould approved these changes Oct 8, 2024

View reviewed changes

spacebear21 added 5 commits October 8, 2024 18:01

Remove InputStats and OutputStats

1969f4f

The only field that's necessary to keep is contributed_fee, which is now returned as standalone bitcoin::Amount in check_outputs()

Refactor input_type.rs to use bitcoin::transaction::InputWeightPredic…

475c811

…tion Instead of hardcoded values, use InputWeightPredictions to calculate expected input weight.

Compute input type and weight on InputPair

424f9c8

bitcoin::AddressType can act as a substitute for InputType, and we can use that to compute expected input weights. InputPair contains all the context necessary to derive those properties, so input_type.rs is obsolete.

Delete unused code

282614e

Code cleanup: deletes input_type.rs, output_type.rs and weight.rs which are no longer needed.

Add proper error handling to new InputPair functions

6938b70

spacebear21 force-pushed the predict-weight branch from 2cef4ea to 77e567c Compare October 8, 2024 22:12

DanGould reviewed Oct 8, 2024

View reviewed changes

Remove input_type and sequence from Context

68fc635

Input type and sequence can be computed at little cost from the original PSBT, which is already included in RequestContext/ContextV1. Remove those fields from those structs and compute them when needed.

Add unit test for input weight calculations

3bb3df3

I used payjoin-cli with `RUST_LOG=trace` to generate and obtain the original and payjoin PSBT values, and a new bitcoin-cli wallet for each address type.

spacebear21 force-pushed the predict-weight branch from 77e567c to 3bb3df3 Compare October 8, 2024 23:16

DanGould merged commit a9b9a34 into payjoin:master Oct 8, 2024

spacebear21 deleted the predict-weight branch October 9, 2024 01:53

spacebear21 mentioned this pull request Oct 9, 2024

Use rust-bitcoin methods for weight calculations #353

Closed

		.scan(&mut err, \|err, input\| match Ok(input.address_type()) {
		Ok(address_type) => Some(address_type),

Conversation

spacebear21 commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould commented Sep 26, 2024

Uh oh!

DanGould commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould Oct 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacebear21 commented Oct 8, 2024

Uh oh!

DanGould commented Oct 8, 2024

Uh oh!

DanGould left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacebear21 commented Oct 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

spacebear21 commented Sep 26, 2024 •

edited

Loading

DanGould commented Sep 26, 2024 •

edited

Loading

DanGould Oct 6, 2024 •

edited

Loading

DanGould left a comment •

edited

Loading

DanGould Oct 8, 2024 •

edited

Loading