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

Use payjoin crate #8

Closed
wants to merge 2 commits into from
Closed

Use payjoin crate #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24853e5
Make proposal with payjoin crate
DanGould Jul 28, 2022
45037d0
Add various TODO comments and refactor code
evanlinjin Aug 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 104 additions & 6 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ spec = "config_spec.toml"
test_paths = []

[dependencies]
bitcoin = { version = "0.27.0", features = ["use-serde"] }
bitcoin = { version = "0.28.1", features = ["use-serde"] }
bip78 = { git = "https://github.com/dangould/rust-payjoin", branch = "receive-logic-v3", features = ["sender", "receiver" ] }
url = "2.2.2"
hyper = "0.14.9"
tonic_lnd = "0.4.0"
tokio = { version = "1.7.1", features = ["rt-multi-thread"] }
Expand Down
97 changes: 61 additions & 36 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::fmt;
use std::sync::{Arc, Mutex};

use args::ArgError;
use bip78::receiver::*;
use bitcoin::util::address::Address;
use bitcoin::util::psbt::PartiallySignedTransaction;
use bitcoin::{Script, TxOut};
Expand Down Expand Up @@ -181,7 +182,7 @@ impl From<tonic_lnd::Error> for CheckError {
async fn ensure_connected(client: &mut tonic_lnd::Client, node: &P2PAddress) {
let pubkey = node.node_id.to_string();
let peer_addr =
tonic_lnd::rpc::LightningAddress { pubkey: pubkey, host: node.as_host_port().to_string() };
tonic_lnd::rpc::LightningAddress { pubkey, host: node.as_host_port().to_string() };

let connect_req =
tonic_lnd::rpc::ConnectPeerRequest { addr: Some(peer_addr), perm: true, timeout: 60 };
Expand Down Expand Up @@ -266,6 +267,11 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
Ok(())
}

pub(crate) struct Headers(hyper::HeaderMap);
impl bip78::receiver::Headers for Headers {
fn get_header(&self, key: &str) -> Option<&str> { self.0.get(key)?.to_str().ok() }
}

async fn handle_web_req(
mut handler: Handler,
req: Request<Body>,
Expand Down Expand Up @@ -293,60 +299,79 @@ async fn handle_web_req(
dbg!(req.uri().query());

let mut lnd = handler.client;
let query = req
.uri()
.query()
.into_iter()
.flat_map(|query| query.split('&'))
.map(|kv| {
let eq_pos = kv.find('=').unwrap();
(&kv[..eq_pos], &kv[(eq_pos + 1)..])
})
.collect::<std::collections::HashMap<_, _>>();

if query.get("disableoutputsubstitution") == Some(&"1") {
panic!("Output substitution must be enabled");
}
let base64_bytes = hyper::body::to_bytes(req.into_body()).await?;
let bytes = base64::decode(&base64_bytes).unwrap();
let mut reader = &*bytes;
let mut psbt = PartiallySignedTransaction::consensus_decode(&mut reader).unwrap();
eprintln!("Received transaction: {:#?}", psbt);
for input in &mut psbt.global.unsigned_tx.input {
// clear signature
input.script_sig = bitcoin::blockdata::script::Script::new();

// extract original PSBT from HTTP request
// TODO: `UncheckedProposal` is not really the proposal, but the original PSBT? Is the naming wrong?
Copy link
Contributor

Choose a reason for hiding this comment

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

UncheckedProposal is the OriginalPSBT + the sender's paramaters. It is a PayJoin proposal

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean the UncheckedProposal struct is badly named, because the proposed PSBT is what is returned to the receiver.

// TODO: Unneeded and/or undocumented traits in bip-78 library? (`Proposal`, `Headers`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Proposal is probably unecessary, but I imagine as a way to extract psbt() -> Psbt between check states if necessary. That might only be necessary at the fully checked / unlockedProposal stage. We can play with the interface to arrive at v1.0

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 is also an issue for the bip78.

// TODO: We should really save the original PSBT just in case the sender does not sign
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty much. This is most important in the case when the receiver is a payment processor that generates bip21 URIs on request, called a non-interactive receiver. If you're scanning someone next to you's phone it's not so important to broadcast the UncheckedProposal.extract_tx() in the failure case or even make sure it can be broadcast because humans will be able to interact to solve the fail case by trying again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As previously mentioned, if UncheckedProposal is renamed to OriginalPsbtRequest or something alike, it would be less confusing.

// and broadcast the proposed PSBT, or some other error occurs
let headers = Headers(req.headers().clone());
let query = req.uri().query().map(ToString::to_string);
let body_bytes = dbg!(hyper::body::to_bytes(req.into_body()).await?);
let proposal = UncheckedProposal::from_request(&*body_bytes, query.as_deref(), headers)
.expect("received invalid proposal - TODO: handle this error properly");
if proposal.is_output_substitution_disabled() {
panic!("Output substitution must be enabled"); // TODO: Proper checks for `pjos`
}
let proposal = proposal
.assume_interactive_receive_endpoint() // TODO Check
.assume_no_inputs_owned() // TODO Check
.assume_no_mixed_input_scripts() // This check is silly and could be ignored
.assume_no_inputs_seen_before(); // TODO

// `psbt` is going to ACTUALLY be the proposal PSBT
let mut psbt = proposal.psbt().clone();
eprintln!("Received transaction: {:#?}", psbt); // but not yet
psbt.unsigned_tx
.input
.iter_mut()
.for_each(|txin| txin.script_sig = bitcoin::Script::new());
Comment on lines +325 to +328
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes and this should be automated by the payjoin crate instead of written here. Best downstream code doesn't need to grok this piece


// find our output
// TODO: Would it be possible that the user sends to multiple outputs that we own?
let (our_output, scheduled_payjoin) = handler
.payjoins
.find(&mut psbt.global.unsigned_tx.output)
.find(&mut psbt.unsigned_tx.output)
.expect("the transaction doesn't contain our output");
let total_channel_amount: bitcoin::Amount = scheduled_payjoin

// TODO: make this a member fn of `ScheduledPayJoin`
// TODO: make sure it is impossible to create a BIP21pj linked to `ScheduledPayJoin`
// with a "total channel amount" larger than requested + fee
let total_channel_amount = scheduled_payjoin
Comment on lines +338 to +340
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the problem with total_channel_amount > Bip78Uri.amount()`? The edge case I imagine is BTCPayServer requests an invoice for a strict amount and the excess is either 1. added to on chain single sig or 2. increase a channel amount. It does not seem like requesting more is dire.

.channels
.iter()
.map(|channel| channel.amount)
.fold(bitcoin::Amount::ZERO, std::ops::Add::add);

// TODO: make this a member fn of `ScheduledPayJoin`
let fees = calculate_fees(
scheduled_payjoin.channels.len() as u64,
scheduled_payjoin.fee_rate,
scheduled_payjoin.wallet_amount != bitcoin::Amount::ZERO,
);

// TODO: not great
Copy link
Contributor

Choose a reason for hiding this comment

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

make TODOs actionable. Explain qualitative disagreements.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean we shouldn't have anything that panics when handling an http request

assert_eq!(
our_output.value,
(total_channel_amount + scheduled_payjoin.wallet_amount + fees).as_sat()
);

let chids = (0..scheduled_payjoin.channels.len())
let chan_ids = (0..scheduled_payjoin.channels.len())
.into_iter()
.map(|_| rand::random::<[u8; 32]>())
.collect::<Vec<_>>();

// these are channel-open txouts
// no collect() because of async
let mut txouts = Vec::with_capacity(scheduled_payjoin.channels.len());

for (channel, chid) in scheduled_payjoin.channels.iter().zip(&chids) {
// TODO: Creating `OpenChannelRequest` should be one thing, and async calls another
// TODO: Instead of interfacing with `tonic_lnd` directly, consider having a trait
// so this can be extended in the future
Comment on lines +369 to +370
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. As soon as we duplicate code, abstract into a trait to support LDK & CLN.

// TODO: What if channel open fails or takes too long?
Copy link
Contributor

Choose a reason for hiding this comment

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

We get a corrupt channel state 😬 This is the worst failure case in this code. Most important error to address.

for (channel, chan_id) in scheduled_payjoin.channels.iter().zip(&chan_ids) {
let psbt_shim = tonic_lnd::rpc::PsbtShim {
pending_chan_id: Vec::from(chid as &[_]),
pending_chan_id: Vec::from(chan_id as &[_]),
base_psbt: Vec::new(),
no_publish: true,
};
Expand Down Expand Up @@ -383,7 +408,7 @@ async fn handle_web_req(
while let Some(message) =
update_stream.message().await.expect("failed to receive update")
{
assert_eq!(message.pending_chan_id, chid);
assert_eq!(message.pending_chan_id, chan_id);
if let Some(update) = message.update {
use tonic_lnd::rpc::open_status_update::Update;
match update {
Expand All @@ -392,9 +417,9 @@ async fn handle_web_req(
let tx = PartiallySignedTransaction::consensus_decode(&mut bytes)
.unwrap();
eprintln!("PSBT received from LND: {:#?}", tx);
assert_eq!(tx.global.unsigned_tx.output.len(), 1);
assert_eq!(tx.unsigned_tx.output.len(), 1);

txouts.extend(tx.global.unsigned_tx.output);
txouts.extend(tx.unsigned_tx.output);
break;
}
// panic?
Expand All @@ -412,17 +437,17 @@ async fn handle_web_req(
*our_output = channel_output;
} else {
our_output.value = scheduled_payjoin.wallet_amount.as_sat();
psbt.global.unsigned_tx.output.push(channel_output)
psbt.unsigned_tx.output.push(channel_output)
}

psbt.global.unsigned_tx.output.extend(txouts);
psbt.outputs.resize_with(psbt.global.unsigned_tx.output.len(), Default::default);
psbt.unsigned_tx.output.extend(txouts);
psbt.outputs.resize_with(psbt.unsigned_tx.output.len(), Default::default);

eprintln!("PSBT to be given to LND: {:#?}", psbt);
let mut psbt_bytes = Vec::new();
psbt.consensus_encode(&mut psbt_bytes).unwrap();

for chid in &chids {
for chid in &chan_ids {
let psbt_verify = tonic_lnd::rpc::FundingPsbtVerify {
pending_chan_id: Vec::from(chid as &[_]),
funded_psbt: psbt_bytes.clone(),
Expand All @@ -441,7 +466,7 @@ async fn handle_web_req(
}

// Reset transaction state to be non-finalized
psbt = PartiallySignedTransaction::from_unsigned_tx(psbt.global.unsigned_tx)
let psbt = PartiallySignedTransaction::from_unsigned_tx(psbt.unsigned_tx.clone())
.expect("resetting tx failed");
let mut psbt_bytes = Vec::new();
eprintln!("PSBT that will be returned: {:#?}", psbt);
Expand Down