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

add payjoin support #156

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

Conversation

willowenss
Copy link

Adding payjoin support to bdk-cli

src/handlers.rs Outdated Show resolved Hide resolved
src/handlers.rs Outdated
.ok_or_else(|| Error::Payjoin("No amount specified in your URI".to_string()))?;

// creates mutable outputs hashmap with at most one key-value pair.
let mut outputs = HashMap::with_capacity(1);
Copy link
Member

Choose a reason for hiding this comment

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

check here for how to use tx_builder to add the recipient.

https://docs.rs/bdk/latest/bdk/wallet/tx_builder/index.html#


// all logic below this is temporary and will be deleted or changed


Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

In testing we realized minreq has no support for trusting non-root store certificates from the client. So either local testers would have to sudo add the self-signed certificates to their OS root store or get legit CA cert servers for testing. When minreq TLS matches our target MSRV I would definitely consider it again.

For now, reqwest gives us the ability to trust self-signed for testing so it makes more sense to use.

src/handlers.rs Outdated

// send payjoin request via minreq
let response = minreq::post(uri.payjoin_url)
Copy link
Author

Choose a reason for hiding this comment

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

Might need
let response = response.send() after this

@willowenss willowenss changed the title add payjoin support (very incomplete draft) add payjoin support Jun 26, 2023
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Great work. The code looks very clean. I am yet to test it out locally. I like your approach to documenting all the steps. The best way to start writing code is doc everything, even if it's extra it doesn't matter. It's always easier deleting docs later than writing them. 😆

Initial Concept ACK.

I have some minor nit comments, but I will refrain from them until the code finalizes.

I've included one structural comment below for future consideration.

src/handlers.rs Outdated
Comment on lines 407 to 408
SendPayjoin{
uri,
} => {

// convert the bip21 uri into a payjoin uri, and handle error if necessary
let uri = payjoin::Uri::try_from(uri)
.map_err(|e| Error::Generic(format!("Unable to convert BIP21 into Payjoin URI: {}", e)))?;

Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine extracting this logic from this match statement into a stand-alone public function could be helpful. If not for export, at least for maintainability.

src/handlers.rs Outdated
let finalized = wallet // this returns a bool here
.sign(&mut psbt, sign_options);
// maybe try allow_grinding?
let finalized = wallet.finalize_psbt(&mut psbt, sign_options1)?; // this returns a bool here
Copy link
Author

Choose a reason for hiding this comment

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

When testing, the PSBT response is getting returned as expected from the receiver. However, the bdk-cli sender isn't signing all inputs. I'm getting an ERROR non-mandatory-script-verify-flag when trying to broadcast the transaction because there aren't any signatures. I don't know how to approach this right now, since I thought wallet.sign "Adds to the PSBT all the signatures it can produce".

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you paste the PSBT here after finalizing it?

Copy link
Contributor

@rajarshimaitra rajarshimaitra Jul 5, 2023

Choose a reason for hiding this comment

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

From the look of it, the PSBT looks malformed. It has only one witness in one of the inputs, and the other input is empty. There also don't seem to be any PSBT outputs, whereas there are two P2WPKH outputs in the unsigned transaction.

I am not fully sure what's going wrong, but it seems like the payjoin party isn't returning the correct psbt? The one signature in the psbt is probably coming from the wallet itself.

To dig deeper, you probably need to understand psbts a bit better. The easiest thing to do is go by the docs of the psbt structure and try to match why certain components could be missing. https://github.com/rust-bitcoin/rust-bitcoin/blob/83cf389a02d6147a4cd3fe9c42388c61abd40ec2/bitcoin/src/psbt/mod.rs#L44-L63

Copy link
Collaborator

@DanGould DanGould Jul 6, 2023

Choose a reason for hiding this comment

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

The Problem

I don't believe the PSBT is malformed, it's partial! I found the problem in BDK: unlike Bitcoin Core's walletprocesspsbt RPC, finalize_psbt only checks if the TxOut script in the PSBT matches the descriptor and does not check whether it has control of the OutPoint specified in the unsigned_tx's TxIn. LND's PSBT signing software does the same thing. While I think BDK should support this lookup in its signer (after 1.0), BIP 78 is also imo flawed by removing the sender's utxo data and ensuring it's not specified. I'll have to keep knocking on @NicolasDorier's door until he believes me.

A Solution

@willowens14 should re-introduce the UTXO (Input Map's TxOut) data to the psbt manually before sending it to the finalizer. Will should be able to copy the Input from the Original PSBT's input map and remove the signature / finalized pieces of data. Once that's in there the finalizer should be able to match the TxO to a descriptor, sign, and finalize.

small note @willowens14 next time you put logs in github you can fence (``` ```) it so it maintains formatting and you don't have to point to a gist if it's short enough

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the detailed write-up here. That's a deep nuance I missed on BIP78/174 interaction.

src/handlers.rs Outdated Show resolved Hide resolved
src/handlers.rs Outdated Show resolved Hide resolved
src/handlers.rs Outdated Show resolved Hide resolved
src/handlers.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

It works on my machine!

I've asked to clean up a few loose ends and then it's looking ready to merge.

I'm not sure what BDK-CLI's policy is as far as commits go, but this one looks like a contender for squash and merge. @willowens14 for projects who prefer strict control over commit history, you may want to get more acquainted with this kind of commit culture: https://cbea.ms/git-commit/

src/commands.rs Outdated
@@ -622,7 +628,7 @@ mod test {
feature = "compact_filters",
feature = "rpc"
))]
use super::OnlineWalletSubCommand::{Broadcast, Sync};
use super::OnlineWalletSubCommand::{Broadcast, SendPayjoin, Sync}; // added sendpayjoin here
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for being proactive with comments! Time to get deeper into design review now that you've got payjoin working 🎉

In my experience, comments that describe what changed rather than why can be confusing. The diff tells me SendPayjoin is added here already, so I think there has to be some context that compelled you to write a comment. Why? I think it's better to leave comments describing what changed out. Below, I'll show you what I mean by "why" comments being helpful.

src/handlers.rs Outdated
// set this to account for original_inputs being of different length than psbt.inputs
let mut og = 0;

// Iterate over the inputs in the PSBT
Copy link
Collaborator

Choose a reason for hiding this comment

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

A for loop comes next, so I know iteration is happening, but I'm not sure why. Isn't the response PSBT processed?

Telling someone why this part of the code exists could help in code comprehension in comparison to "what" comments that can be gleaned from reading the syntax alone. Something like the following should explain why this logic exists.

// BDK only signs scripts that match its target descriptor by iterating through input map.
// The BIP 78 spec makes receiver clear sender input map UTXOs, so process_response will fail unless they're cleared.
// A PSBT unsigned_tx.input references input OutPoints and not a Script, so the sender signer must either
// be able to sign based on OutPoint UTXO lookup or otherwise re-introduce the Script from original_psbt.
// Since BDK PSBT signer only checks Input map Scripts for match against its descriptor, it won't sign if they're empty.
// Re-add the scripts from the original_psbt in order for BDK to sign properly.

Bonus points for documenting reasons behind particular design decision tradeoffs.

src/handlers.rs Outdated
Comment on lines 483 to 484
// set this to account for original_inputs being of different length than psbt.inputs
let mut og = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is exactly what I needed 🙌. It explains why the magic number exists. Without I'd be scratching my head.

src/handlers.rs Outdated
Comment on lines 438 to 443
// set signoptions
let mut sign_options = SignOptions::default();
sign_options.trust_witness_utxo = true;

// check + sign psbt
let finalized = wallet.sign(&mut psbt, sign_options)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Default SignOptions seem to work now that we've got the right data to sign available

Suggested change
// set signoptions
let mut sign_options = SignOptions::default();
sign_options.trust_witness_utxo = true;
// check + sign psbt
let finalized = wallet.sign(&mut psbt, sign_options)?;
let finalized = wallet.sign(&mut psbt, SignOptions::default())?;

src/handlers.rs Outdated
Comment on lines 506 to 511
let mut sign_options1 = SignOptions::default();
sign_options1.trust_witness_utxo = true;
sign_options1.allow_all_sighashes = true;

// sign
wallet.sign(&mut psbt, sign_options1.clone())?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Default SignOptions seem to work now that we've got the right data to sign available

Suggested change
let mut sign_options1 = SignOptions::default();
sign_options1.trust_witness_utxo = true;
sign_options1.allow_all_sighashes = true;
// sign
wallet.sign(&mut psbt, sign_options1.clone())?;
wallet.sign(&mut psbt, SignOptions::default())?;

src/handlers.rs Outdated
Comment on lines 483 to 497
// set this to account for original_inputs being of different length than psbt.inputs
let mut og = 0;

// Iterate over the inputs in the PSBT
for input in psbt.inputs.iter_mut() {
// If this input is some skip it
if input.witness_utxo.is_some() || input.non_witness_utxo.is_some() {
continue;
}
// If index is within the range of original_inputs
else if og < original_inputs.len() {
// Reintroduce the utxo from the original_inputs
input.witness_utxo = original_inputs[og].witness_utxo.clone();
input.non_witness_utxo = original_inputs[og].non_witness_utxo.clone();

// increment original_index only when an original input is used
og += 1;
}
}
Copy link
Collaborator

@DanGould DanGould Jul 12, 2023

Choose a reason for hiding this comment

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

We should be able to iterate without explicit indexing as in process_response's check_inputs

I suggest renaming psbt to original_psbt and payjoin_psbt respectively. This snippet also requires you to pass a cloned psbt to create_pj_request to keep the original around. It clones slightly more bytes for imo a more readable, maintainable piece of code. I've opted to clone the maps instead of dealing with mutable references in input_pairs for the same reason. I justify this tradeoff because I imagine this BDK-CLI command will be used for individual wallets and sends, and not high throughput enterprise send-a-million-payjoins-at-once scenarios. We need to use mutable references in order to modify the payjoin PSBT. cloning was a bug! PSBTv2 makes this pairing nonsense go away and can't come soon enough!

Payjoin's internal PsbtExt's helpers like this duplicated/modified input_pairs function may be worth exposing publicly in PDK if not rust-bitcoin or BDK. Tagging rust-bitcoin & PDK API expert @Kixunil to weigh in.

Suggested change
// set this to account for original_inputs being of different length than psbt.inputs
let mut og = 0;
// Iterate over the inputs in the PSBT
for input in psbt.inputs.iter_mut() {
// If this input is some skip it
if input.witness_utxo.is_some() || input.non_witness_utxo.is_some() {
continue;
}
// If index is within the range of original_inputs
else if og < original_inputs.len() {
// Reintroduce the utxo from the original_inputs
input.witness_utxo = original_inputs[og].witness_utxo.clone();
input.non_witness_utxo = original_inputs[og].non_witness_utxo.clone();
// increment original_index only when an original input is used
og += 1;
}
}
fn input_pairs(psbt: &mut PartiallySignedTransaction) -> Box<dyn Iterator<Item = (&bdk::bitcoin::TxIn, &mut Input)> + '_> {
Box::new(
psbt.unsigned_tx
.input
.iter()
.zip(&mut psbt.inputs)
)
}
let mut original_inputs = input_pairs(&mut original_psbt).peekable();
for (proposed_txin, mut proposed_psbtin) in input_pairs(&mut payjoin_psbt) {
if let Some((original_txin, original_psbtin)) = original_inputs.peek() {
if proposed_txin.previous_output == original_txin.previous_output {
proposed_psbtin.witness_utxo = original_psbtin.witness_utxo.clone();
proposed_psbtin.non_witness_utxo = original_psbtin.non_witness_utxo.clone();
}
original_inputs.next();
}
}

Copy link

Choose a reason for hiding this comment

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

Yes, for a long time I wanted to move parts from payjoin to rust-bitcoin. This looks like one of them but I don't remember for sure.


// all logic below this is temporary and will be deleted or changed


Copy link
Collaborator

Choose a reason for hiding this comment

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

In testing we realized minreq has no support for trusting non-root store certificates from the client. So either local testers would have to sudo add the self-signed certificates to their OS root store or get legit CA cert servers for testing. When minreq TLS matches our target MSRV I would definitely consider it again.

For now, reqwest gives us the ability to trust self-signed for testing so it makes more sense to use.

@DanGould
Copy link
Collaborator

Note from @notmandatory : "FYI CI is failing on Will’s PR because some dependencies need to be pinned to older versions to work with rust 1.57.. it’s not related to his changes so I’ll need to fix in a different PR"


// set payjoin params
let pj_params = payjoin::send::Configuration::with_fee_contribution(
payjoin::bitcoin::Amount::from_sat(100),
Copy link
Collaborator

@DanGould DanGould Jul 12, 2023

Choose a reason for hiding this comment

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

This should 1. match the tx_builder's fee rate using min_fee_rate_sat_per_vb or else the receiver can add their input without any fees, reducing the total fee rate and 2. consider adding a fee budget equivalent to receiver adding one input using with_fee_contribution (The recommended fee is size_of_one_input * fee_rate.)

Just use Configuration::non_incentivising for now, we need a utility to identify that recommended fee and change output. Unless there's an easy way to identify the change in BDK already? @notmandatory

Copy link
Member

Choose a reason for hiding this comment

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

I can't think of any easy way to figure out the change output except to look at all of them and use the Wallet.is_mine() function to see which ones are yours, and then take which ever one is not the same as the spending amount, though this is pretty hackey and could fail if both are the same amount. Ideally we should change the Wallet.is_mine() function to return which descriptor the script goes with.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the receiver's change output passed to Wallet.is_mine() will return false? Only true for external account descriptor?

@notmandatory
Copy link
Member

notmandatory commented Sep 21, 2023

I screwed up while trying to push a CI fix to this PR. @willowenss please force push 5cc9dd4 from your local branch back to this PR branch. Then we can re-open it.

@notmandatory notmandatory reopened this Sep 21, 2023
@notmandatory
Copy link
Member

notmandatory commented Sep 21, 2023

Looks like it's passing CI testing! Next steps are:

  1. get commit signing working for your github account
  2. then squash and/or reword your commits to be more descriptive
  3. do final manual testing to confirm it all still works
  4. add any missing tests to make sure it will keep working

https://docs.github.com/en/authentication/managing-commit-signature-verification
https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History

Remove Cargo.lock

fixed handlers formatting

payjoin 0.8.2

regenerated cargo.lock - pin tokio+tempfile to build with MSRV 1.57.0

Updated pin 8/28

added psbt::input crate
Signed-off-by: willowens14 <will.b.owens@vanderbilt.edu>
@notmandatory
Copy link
Member

Some new MSRV issues have cropped up, you'll need to amend your last commit to use these cargo update lines:

cargo update -p log --precise 0.4.18
cargo update -p hashlink --precise 0.8.0
cargo update -p tempfile --precise 3.6.0
cargo update -p base64ct --precise 1.5.3
cargo update -p flate2 --precise 1.0.26
cargo update -p h2:0.3.21 --precise 0.3.20
cargo update -p reqwest:0.11.22 --precise 0.11.18
cargo update -p tokio:1.33.0 --precise 1.29.1
cargo update -p tokio-util:0.7.9 --precise 0.7.8
cargo update -p rustls --precise 0.20.8
cargo update -p byteorder --precise "1.4.3"
cargo update -p webpki --precise 0.22.2
cargo update -p cc --precise 1.0.81
cargo update -p os_str_bytes --precise 6.5.1
cargo update -p jobserver --precise 0.1.26

src/handlers.rs Outdated
Comment on lines 493 to 496
proposed_psbtin.non_witness_utxo = original_psbtin.non_witness_utxo.clone();
}
original_inputs.next();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
proposed_psbtin.non_witness_utxo = original_psbtin.non_witness_utxo.clone();
}
original_inputs.next();
}
proposed_psbtin.non_witness_utxo = original_psbtin.non_witness_utxo.clone();
original_inputs.next();
}
}

Oops! This will skip checking an original_input even if it's not a match, then failing will sign if a match is missed.

@DanGould
Copy link
Collaborator

DanGould commented Jan 4, 2024

Looks like this is failing because it depends on bdk 0.27.1 which depends on an ahash version which was yanked due to a side channel attack vulnerability

I'd really like to close this out, but it looks blocked until #166 is merged or otherwise a 0.27.x patch is released with the new ahash 0.7.7. Tricky situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants