Annotate the reference implementation for Audit #52

DanGould · 2023-04-13T15:36:50Z

A high level, annotated "TODO" for integrating payjoin in your app. This info should make its way into the docs.rs documentation and payjoin.org. To start I've drafted plain ol' markdown so you can review and suggest edits to the doc directly.

It's a mental model of what the payjoin-client logic does and where/how the coin selection happens and the interplay with a wallet to understand how it could work with others like bdk.

This is a tool to try and figure out the bare-minimum shape of what you'd need as an API to use it on a mobile phone which has already access to e.g. bdk bindings

Thanks to @thunderbiscuit for making this request.

cc: @fiatjaf @kaloudis who also requested more detail on how to integrate the protocol

One of Kixunil's criteria for taking his original work into widespread production is getting an outside audit. This is a good start.

DanGould · 2023-04-15T16:35:45Z

MENTAL-MODEL.md

+let link = link
+    .check_pj_supported()
+    .map_err(|e| anyhow!("The provided URI doesn't support payjoin (BIP78): {}", e))?;
+```


Refactoring (see issue #19) to use a builder pattern may eliminate the need to call check_pj_supported or expose bip21 at all.

DanGould · 2023-04-15T16:36:59Z

MENTAL-MODEL.md

+    .wallet_process_psbt(&psbt, None, None, None)
+    .with_context(|| "Failed to process PSBT")?
+    .psbt;
+let psbt = load_psbt_from_base64(psbt.as_bytes()) // SHOULD BE PROVIDED BY CRATE AS HELPER USING rust-bitcoin base64 feature


The load_psbt_from_base64 helper method could be refactored to use the bitcoin::base64 feature. I imagine the rust-bitcoin crate would be open to Psbt::to_base64 and Psbt::from_base64 feature-gated helpers being added as well. see #30

DanGould · 2023-04-15T16:37:28Z

MENTAL-MODEL.md

+    payjoin::bitcoin::Amount::from_sat(10000),
+    None,
+);
+```


I think the current flow works and it does not need to change to prove the concept, but it may also be improved with the builder pattern (#19). The builder might take the bip21, an optional amount that differs from that in the bip21, fee rate, a closure to fund and sign an Original PSBT. The result should be a request containing (original_psbt, optional_parameters) and state in context.

DanGould · 2023-04-15T16:38:08Z

MENTAL-MODEL.md

+
+Senders request a payjoin from the receiver with a payload containing the Original PSBT  and optional parameters. They require a secure endpoint for authentication and message secrecy to prevent that transaction from being modified by a malicious third party during transit or being snooped on. Only https and .onion endpoints are spec-compatible payjoin endpoints.
+
+Avoiding the secure endpoint requirement is convenient for testing.


(but again, Signal guidelines remind me of the folly of options). I wonder if there's a better way to set up a testing environment with a valid https cert with something like Nix.

DanGould · 2023-04-15T16:40:40Z

MENTAL-MODEL.md

+log::debug!("Proposed psbt: {:#?}", psbt);
+```
+
+Payjoin response errors (called [receiver's errors in spec](https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#receivers-well-known-errors)) come from a remote server and can be used to "maliciously to phish a non technical user." Only those listed as "well known" in the spec should be displayed with preset messages to prevent phishing.


Errors from process_response may be improved by being separated into safe ReceiverError::WellKnown standard error types that can be displayed in the UI and ReceiverError::DebugOnly which "can only appear in debug logs." Separation would simplify an integration's error handling. (#53)

DanGould · 2023-04-15T16:49:23Z

MENTAL-MODEL.md

+        copy.find(|(k, _)| k.eq_ignore_ascii_case(key)).map(|(_, v)| v)
+    }
+}
+```


I reckon bindings will run into a problem with this Trait-based design. @thunderbiscuit, what do you think?

DanGould · 2023-04-15T16:50:03Z

MENTAL-MODEL.md

+// in a payment processor where the sender could go offline, this is where you schedule to broadcast the original_tx
+let _to_broadcast_in_failure_case = proposal.get_transaction_to_schedule_broadcast();


The broadcast out of order! This should be after check 1 making sure it's broadcastable.

DanGould · 2023-04-15T16:51:54Z

MENTAL-MODEL.md

+}
+```
+
+Serious, in-depth research has gone into proper transaction construction. [Here's a good starting point from the JoinMarket repo](https://gist.github.com/AdamISZ/4551b947789d3216bacfcb7af25e029e#gistcomment-2796539). Using methods for coin selection not provided by this library may have dire implications for privacy.


Downstream projects may benefit to consume expose the selection algorithm inside try_preserving_privacy as a static function. It may be easier easier and more lightweight to bind to and avoid dangerous instances of UIH in addition to the typestate method. Look at BDK Coin Selection for interface inspiration.

DanGould · 2023-04-15T16:53:11Z

MENTAL-MODEL.md

+Fees are applied to the augmented Payjoin Proposal PSBT using calculation factoring the receiver's own preferred feerate and the sender's fee-related [optional parameters](https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#optional-parameters). The current `apply_fee` method is primitive, disregarding PSBT fee estimation and only adding fees coming from the sender's budget. When more accurate tools are available to calculate a PSBT's fee-dependent weight (slightly more complicated than it sounds, but solved, just unimplemented in rust-bitcoin), this `apply_fee` should be improved.
+
+```rs
+let payjoin_proposal_psbt = payjoin.apply_fee(min_feerate_sat_per_vb: Some(1))?;


The current apply_fee method is primitive, disregarding fee estimation based on PSBT contents and only adding fees coming from the sender's budget. When more accurate tools are available to calculate a PSBT's fee-dependent weight (slightly more complicated than it sounds, but solved, just unimplemented in rust-bitcoin), this apply_fee should be improved.

DanGould · 2023-04-15T16:53:36Z

MENTAL-MODEL.md

+log::debug!("Extracted PSBT: {:#?}", payjoin_proposal_psbt);
+// Sign payjoin psbt
+let payjoin_base64_string =
+    base64::encode(bitcoin::consensus::serialize(&payjoin_proposal_psbt));
+// `wallet_process_psbt` adds available utxo data and finalizes
+let payjoin_proposal_psbt =
+    bitcoind.wallet_process_psbt(&payjoin_base64_string, sign: None, sighash_type: None, bip32derivs: Some(false))?.psbt;
+let payjoin_proposal_psbt =
+    load_psbt_from_base64(payjoin_proposal_psbt.as_bytes()).context("Failed to parse PSBT")?;
+```


The signing algorithm could be passed as a closure, but may encounter runtime issues as mentioned previously. I.e., rust does not yet support async closures.

While bitcoind's wallet_process_psbt handles most everything you throw at it with grace, LND's PSBT signing gets very cranky when fed inputs belonging to other wallets and requires a complicated workaround of multiple gRPC calls to function properly. It might be good to note this in the documentation since it's such a pain in the kiester.

cryptoquick

Just some suggestions. I kind of ran out of steam near the end there, it's a pretty lengthy document. Hopefully what I was able to provide was helpful.

cryptoquick · 2023-04-27T11:27:23Z

MENTAL-MODEL.md

@@ -0,0 +1,376 @@
+# What is the Payjoin SDK and How does it work?
+
+The Payjoin SDK/`rust-payjoin` is the most well-tested and flexible library for BIP 78 Payjoin and related privacy practices.


Suggested change

The Payjoin SDK/`rust-payjoin` is the most well-tested and flexible library for BIP 78 Payjoin and related privacy practices.

The Payjoin SDK/[`rust-payjoin`](https://crates.io/crates/payjoin) is the most well-tested and flexible library for [BIP 78 Payjoin](https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki) and related privacy practices.

cryptoquick · 2023-04-27T11:28:02Z

MENTAL-MODEL.md

+
+The Payjoin SDK/`rust-payjoin` is the most well-tested and flexible library for BIP 78 Payjoin and related privacy practices.
+
+The primary crate, `payjoin`, is runtime-agnostic. Data persistence, chain interactions, and networking may be provided by custom implementations or copy the reference `payjoin-client` + bitcoind, `nolooking` + LND integration, or `bitmask-core` + BDK integrations.


Suggested change

The primary crate, `payjoin`, is runtime-agnostic. Data persistence, chain interactions, and networking may be provided by custom implementations or copy the reference `payjoin-client` + bitcoind, `nolooking` + LND integration, or `bitmask-core` + BDK integrations.

The primary crate, `payjoin`, is runtime-agnostic. Data persistence, chain interactions, and networking may be provided by custom implementations or copy the reference `payjoin-client` + bitcoind, `nolooking` + LND integration, or `bitmask-core` + BDK integrations, compatible within either desktop, mobile, or WASM contexts.

cryptoquick · 2023-04-27T11:29:37Z

MENTAL-MODEL.md

+
+### 1. Parse BIP21 as `payjoin::Uri`
+
+Start by parsing a valid BIP 21 uri having the `pj` parameter. This is the [`bip21`](https://crates.io/crates/bip21) crate under the hood.


Suggested change

Start by parsing a valid BIP 21 uri having the `pj` parameter. This is the [`bip21`](https://crates.io/crates/bip21) crate under the hood.

Start by parsing a valid [BIP 21](https://github.com/bitcoin/bips/blob/master/bip-0021.mediawiki) uri having the `pj` parameter. This is the [`bip21`](https://crates.io/crates/bip21) crate under the hood.

cryptoquick · 2023-04-27T11:32:48Z

MENTAL-MODEL.md

+        &outputs,
+        None, // locktime
+        Some(options),
+        None,


Where is the bitcoind variable from? And what is the last option? (I realize what they are, but it helps to be more explicit about this in the docs)

cryptoquick · 2023-04-27T11:35:59Z

MENTAL-MODEL.md

+I wrote this in the original docs, but I think it should be amended.
+
+In case the payjoin goes through but you still want to pay by default. This missing `payjoin-client`


Suggested change

I wrote this in the original docs, but I think it should be amended.

In case the payjoin goes through but you still want to pay by default. This missing `payjoin-client`

This was written in the original docs, but it should be clarified: In case the payjoin goes through but you still want to pay by default. This is missing `payjoin-client`.

This seems like an incomplete thought, but I did my best to piece it together. Definitely try to avoid documenting in the first-person throughout the rest of this doc.

cryptoquick · 2023-04-27T12:03:30Z

MENTAL-MODEL.md

+// TODO display well-known errors and log::debug the rest
+let psbt = ctx.process_response(response).with_context(|| "Failed to process response")?;
+log::debug!("Proposed psbt: {:#?}", psbt);
+```


Where is ctx from? Please try to make examples more complete.

ctx is the context returned from create_pj_request in step 4, so I've added a comment. Would it be better to have a complete send_payjoin method based on a file in examples rather than this breakdown, or something else?

DanGould · 2023-05-23T02:14:31Z

I've moved this into rustdoc for send and receive features. I've adjusted the example to take into consideration feedback and changes for 0.8.0. I plan to send it with that update tomorrow unless some major error is found in review.

The original audit doc can be found here: https://gist.github.com/DanGould/595865688cabcfec42eb0e2a36f65490

DanGould commented Apr 15, 2023

View reviewed changes

cryptoquick suggested changes Apr 27, 2023

View reviewed changes

DanGould added 4 commits May 22, 2023 22:05

Move payjoin-client payjoin-cli

aa08e52

Move PayJoin Payjoin

82fdc35

Document send feature walkthrough

af73a5d

Address clippy lints

64b1d3b

DanGould force-pushed the mental branch from ff712e6 to a052c1b Compare May 23, 2023 02:07

DanGould marked this pull request as ready for review May 23, 2023 02:12

Document receive feature walkthrough

580d4af

DanGould force-pushed the mental branch from a052c1b to 580d4af Compare May 24, 2023 15:47

DanGould merged commit 860e7c7 into payjoin:master May 24, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotate the reference implementation for Audit #52

Annotate the reference implementation for Audit #52

DanGould commented Apr 13, 2023

DanGould Apr 15, 2023

DanGould Apr 15, 2023

DanGould Apr 15, 2023

DanGould Apr 15, 2023

DanGould Apr 15, 2023

DanGould Apr 15, 2023

DanGould Apr 15, 2023

DanGould Apr 15, 2023

DanGould May 21, 2023

DanGould Apr 15, 2023

DanGould Apr 15, 2023

DanGould Apr 15, 2023

cryptoquick left a comment

cryptoquick Apr 27, 2023

cryptoquick Apr 27, 2023

cryptoquick Apr 27, 2023

cryptoquick Apr 27, 2023

cryptoquick Apr 27, 2023

cryptoquick Apr 27, 2023

DanGould May 21, 2023

DanGould commented May 23, 2023


		Senders request a payjoin from the receiver with a payload containing the Original PSBT and optional parameters. They require a secure endpoint for authentication and message secrecy to prevent that transaction from being modified by a malicious third party during transit or being snooped on. Only https and .onion endpoints are spec-compatible payjoin endpoints.

		Avoiding the secure endpoint requirement is convenient for testing.

		// in a payment processor where the sender could go offline, this is where you schedule to broadcast the original_tx
		let _to_broadcast_in_failure_case = proposal.get_transaction_to_schedule_broadcast();

		@@ -0,0 +1,376 @@
		# What is the Payjoin SDK and How does it work?

		The Payjoin SDK/`rust-payjoin` is the most well-tested and flexible library for BIP 78 Payjoin and related privacy practices.

	The Payjoin SDK/`rust-payjoin` is the most well-tested and flexible library for BIP 78 Payjoin and related privacy practices.
	The Payjoin SDK/[`rust-payjoin`](https://crates.io/crates/payjoin) is the most well-tested and flexible library for [BIP 78 Payjoin](https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki) and related privacy practices.


		The Payjoin SDK/`rust-payjoin` is the most well-tested and flexible library for BIP 78 Payjoin and related privacy practices.

		The primary crate, `payjoin`, is runtime-agnostic. Data persistence, chain interactions, and networking may be provided by custom implementations or copy the reference `payjoin-client` + bitcoind, `nolooking` + LND integration, or `bitmask-core` + BDK integrations.


		### 1. Parse BIP21 as `payjoin::Uri`

		Start by parsing a valid BIP 21 uri having the `pj` parameter. This is the [`bip21`](https://crates.io/crates/bip21) crate under the hood.

	Start by parsing a valid BIP 21 uri having the `pj` parameter. This is the [`bip21`](https://crates.io/crates/bip21) crate under the hood.
	Start by parsing a valid [BIP 21](https://github.com/bitcoin/bips/blob/master/bip-0021.mediawiki) uri having the `pj` parameter. This is the [`bip21`](https://crates.io/crates/bip21) crate under the hood.

		I wrote this in the original docs, but I think it should be amended.

		In case the payjoin goes through but you still want to pay by default. This missing `payjoin-client`

Annotate the reference implementation for Audit #52

Annotate the reference implementation for Audit #52

Conversation

DanGould commented Apr 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryptoquick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanGould commented May 23, 2023