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.

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

Jump to bottom

Implement Serverless Payjoin BIP #101

Merged
merged 6 commits into from
Dec 11, 2023
Merged

Implement Serverless Payjoin BIP #101

merged 6 commits into from
Dec 11, 2023

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Aug 19, 2023 •
edited
Loading

The latest spec: bitcoin/bips#1483

Ongoing design discussion: https://delvingbitcoin.org/t/serverless-payjoin-protocol-design/96

  • Communicate payjoin asynchronously via HTTP Polling(single buffer)
  • Multiple Buffers, BIP21 identification & allocation
  • Base64Url (☐ or Blockchain Commons UR) encoding in BIP21
  • e2ee + authentication
  • Message Padding
  • OHTTP (☐ with 🆕 draft standard secp256k1 HPKE)

Refactors for better production quality

  • Simplify message protocol
  • Make message AEAD part of message packing in send/receive APIs
  • Make OHTTP enc/decapsulation part of send/receive APIs
  • Handle Errors
  • Enable backwards compatibility (☐ pass v1 params, ☐ pjos by default))
  • implement example session management
  • Prefer HPKE to own-rolled cryptosystem
  • Prefer Redis over Postgres since we only need key/value store, not relations
  • Use PSBTv2
  • Use released OHTTP lib vs one with ergonomic changes
  • [ ] Remove workspace (used to simplify testing & features)
  • Use cryptographic hash fn to mark subdirectory instead of concatenating key (which has a header)

Prior Attempts:

@DanGould DanGould added enhancement New feature or request payjoin-cli receive receiving payjoin send sending payjoin payjoin-directory api labels Aug 19, 2023
@DanGould DanGould force-pushed the serverless-payjoin branch 2 times, most recently from bb4c1f2 to 9230962 Compare August 23, 2023 16:48
@DanGould DanGould force-pushed the serverless-payjoin branch 2 times, most recently from b47590c to bb2b531 Compare September 12, 2023 17:49
This was referenced Sep 25, 2023
Closed
Merged
@DanGould DanGould force-pushed the serverless-payjoin branch 2 times, most recently from c999b55 to 5834266 Compare September 29, 2023 21:56
jbesraa
jbesraa reviewed Oct 2, 2023
View reviewed changes
Copy link
Contributor

@jbesraa jbesraa left a comment

Choose a reason for hiding this comment

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

So I just did a review to understand the implementation and link it the actual BIP. I went through the BIP again also while reviewing this so it is a great refreshment. the code is quite readable and its not too hard to link to the different parts of the BIP.
I added a few comments to make sure I get things correctly. Ill review this a couple of more times and might have a bit more code/design input then.

payjoin-cli/src/app.rs Show resolved Hide resolved
payjoin-relay/src/main.rs Outdated Show resolved Hide resolved
payjoin-relay/src/main.rs Outdated Show resolved Hide resolved
@DanGould DanGould force-pushed the serverless-payjoin branch 3 times, most recently from 9eb5cfc to aca9982 Compare November 6, 2023 22:43
@DanGould DanGould force-pushed the serverless-payjoin branch 3 times, most recently from b44a085 to c0ce861 Compare November 20, 2023 22:37
@DanGould DanGould force-pushed the serverless-payjoin branch 2 times, most recently from 28cef51 to e66f80d Compare December 5, 2023 21:04
DanGould
DanGould commented Dec 6, 2023
View reviewed changes
payjoin/src/uri.rs Outdated
Comment on lines 174 to 233
} else if endpoint.scheme() == "http" {
Ok(Payjoin::V2Only(PayjoinParams {
_endpoint: endpoint,
disable_output_substitution: pjos.unwrap_or(false),
}))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although v2 is secure with plain http because sender - receiver communications are e2ee using hpke, it adds complexity to the library and specifically THIS PR which is now only used in places which also support https

payjoin/Cargo.toml Outdated
@@ -17,6 +17,7 @@ exclude = ["tests"]
[features]
send = []
receive = ["rand"]
relay = []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is never used

payjoin-relay/tests/integration.rs Outdated
Comment on lines 268 to 271
let http = reqwest::Client::builder()
.danger_accept_invalid_certs(true)
.build()
.expect("Failed to build reqwest http client");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although reqwest is easier to use with tokio than ureq, it's a new dep and makes comparing this to the existing integration more burdenome

payjoin-relay/Cargo.toml Outdated
Comment on lines 21 to 22
reqwest = { version = "0.11.4" } No newline at end of file
reqwest = { version = "0.11.4" }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sp

payjoin-relay/src/main.rs Outdated
@@ -103,28 +111,50 @@ impl From<hyper::http::Error> for HandlerError {
fn from(e: hyper::http::Error) -> Self { HandlerError::InternalServerError(e.into()) }
}

async fn post_enroll(body: Body) -> Result<Response<Body>, HandlerError> {
use payjoin::{base64, bitcoin};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these dependencies are independent reexports of payjoin. consider depending on them directly

payjoin/src/uri.rs
@@ -207,7 +265,10 @@ enum InternalPjParseError {
MultipleParams(&'static str),
MissingEndpoint,
NotUtf8(core::str::Utf8Error),
NotBase64(bitcoin::base64::DecodeError),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cfg feature v2 only?

payjoin/src/v2.rs Outdated
@@ -1,7 +1,6 @@
use std::{error, fmt};

pub const MAX_BUFFER_SIZE: usize = 65536;
pub const RECEIVE: &str = "receive";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this can actually be removed in the postgres / subdir introduction

payjoin/src/v2.rs
message_a: &mut [u8],
s: SecretKey,
) -> Result<(Vec<u8>, PublicKey), Error> {
pub fn decrypt_message_a(message_a: &[u8], s: SecretKey) -> Result<(Vec<u8>, PublicKey), Error> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can this be immutable from the getgo?

payjoin-cli/src/app.rs
#[cfg(feature = "v2")]
async fn long_poll_post(&self, req_ctx: payjoin::send::RequestContext<'_>) -> Result<Psbt> {
loop {
let (req, ctx) = req_ctx.extract_v2(&self.config.ohttp_proxy)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose the loops could be contained within one function that takes this line as a closure

payjoin/src/receive/v2.rs Outdated
Comment on lines 135 to 138
use std::fmt;
use std::str::FromStr;

use serde::de::{self, Deserializer, MapAccess, SeqAccess, Visitor};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be at file top

@DanGould DanGould marked this pull request as ready for review December 11, 2023 02:26
@DanGould DanGould force-pushed the serverless-payjoin branch 4 times, most recently from 34d16e0 to e386f4f Compare December 11, 2023 15:14
@DanGould
Copy link
Contributor Author

This functionality is complete and includes comprehensive testing

However it is rought around the edges and could be a bit more organized with module separation. I'm going to merge it to consider new organizations in independent pull requests

@DanGould DanGould merged commit f87424e into payjoin:master Dec 11, 2023
5 checks passed
This was referenced Dec 11, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbesraa jbesraa jbesraa left review comments

Assignees
No one assigned
Labels
api enhancement New feature or request payjoin-cli payjoin-directory receive receiving payjoin send sending payjoin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DanGould @jbesraa