Skip to content
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.

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

Use native rust-bitcoin methods for weight calculations #360

Merged
merged 8 commits into from
Oct 8, 2024

Conversation

spacebear21
Copy link
Collaborator

@spacebear21 spacebear21 commented Sep 26, 2024

See #353

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
Copy link
Contributor

pretty massive delete! eager to follow

@DanGould
Copy link
Contributor

DanGould commented Sep 26, 2024

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.

@spacebear21 spacebear21 force-pushed the predict-weight branch 5 times, most recently from 20c1273 to f580454 Compare October 3, 2024 15:49
Use upstream fee and weight methods from the `psbt` crate instead of
manually calculating everything.
// 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]),
SegWitV0 { ty: SegWitV0Type::Script, nested: _ } => unimplemented!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

// 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]),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Comment on lines 130 to 138
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()?)
})
Copy link
Contributor

@DanGould DanGould Oct 6, 2024

Choose a reason for hiding this comment

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

Two things here

  1. 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.
  2. 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()?)
})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Collaborator Author

@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
Copy link
Contributor

DanGould commented Oct 8, 2024

  • 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 DanGould self-requested a review October 8, 2024 05:30
Copy link
Contributor

@DanGould DanGould left a comment

Choose a reason for hiding this comment

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

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
Copy link
Collaborator Author

Thanks - fixed the clippy warnings and test name.

Comment on lines 231 to 232
.scan(&mut err, |err, input| match Ok(input.address_type()) {
Ok(address_type) => Some(address_type),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

.original_psbt
.input_pairs()
.next()
.expect("original PSBT should have an input");
Copy link
Contributor

@DanGould DanGould Oct 8, 2024

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.
@DanGould DanGould merged commit a9b9a34 into payjoin:master Oct 8, 2024
4 checks passed
@spacebear21 spacebear21 deleted the predict-weight branch October 9, 2024 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants