Allow LDK node to send payjoin transactions#295
Allow LDK node to send payjoin transactions#295jbesraa wants to merge 6 commits intolightningdevkit:mainfrom
Conversation
tnull
left a comment
There was a problem hiding this comment.
Cool! Let me know when this is ready for a first round of review.
Until then only one early comment:
Cargo.toml
Outdated
| bdk = { version = "0.29.0", default-features = false, features = ["std", "async-interface", "use-esplora-async", "sqlite-bundled", "keys-bip39"]} | ||
|
|
||
| reqwest = { version = "0.11", default-features = false, features = ["json", "rustls-tls"] } | ||
| reqwest = { version = "0.11", default-features = false, features = ["json", "rustls-tls", "blocking"] } |
There was a problem hiding this comment.
FWIW, I'd prefer to stick with the async variant, as the blocking variant introduces even more downstream dependencies, some of which just recently had some security issues, see #283 (comment).
71f7ce5 to
b444fab
Compare
79f8b3f to
532c6ef
Compare
e929b4f to
0dea51e
Compare
|
@tnull this is ready for review |
Cargo.toml
Outdated
| esplora-client = { version = "0.6", default-features = false } | ||
| libc = "0.2" | ||
| uniffi = { version = "0.26.0", features = ["build"], optional = true } | ||
| payjoin = { version = "0.15.0", features = ["send", "receive", "v2"] } |
There was a problem hiding this comment.
Why do we need receive if this is only the sending side?
There was a problem hiding this comment.
yea we dont, it should be fixed in the next payjoin crate release payjoin/rust-payjoin#258
src/builder.rs
Outdated
| } | ||
|
|
||
| /// Configures the [`Node`] instance to enable sending payjoin transactions. | ||
| pub fn set_payjoin_sender_config(&mut self, payjoin_relay: payjoin::Url) -> &mut Self { |
There was a problem hiding this comment.
To expose this in bindings we can't use Url as-is, we should likely just take a String here.
nit: Also possibly just?:
| pub fn set_payjoin_sender_config(&mut self, payjoin_relay: payjoin::Url) -> &mut Self { | |
| pub fn set_payjoin_relay(&mut self, relay: payjoin::Url) -> &mut Self { |
There was a problem hiding this comment.
I think this naming would be confusing when we add the payjoin receiver.
the payjoin receiver config will also require payjoin_relay argument.
the receiver config will be:
{
payjoin_directory: Url,
payjoin_relay: Url,
ohttp_keys: Option<OhttpKeys>
}
src/builder.rs
Outdated
| } | ||
|
|
||
| /// Configures the [`Node`] instance to enable sending payjoin transactions. | ||
| pub fn set_payjoin_sender_config(&self, payjoin_relay: payjoin::Url) { |
There was a problem hiding this comment.
Same as above: could use a rename and we need to use a String.
There was a problem hiding this comment.
Updated to String, please see the comment above regarding the naming
src/builder.rs
Outdated
| gossip_source_config: Option<&GossipSourceConfig>, | ||
| liquidity_source_config: Option<&LiquiditySourceConfig>, seed_bytes: [u8; 64], | ||
| logger: Arc<FilesystemLogger>, kv_store: Arc<DynStore>, | ||
| payjoin_sender_config: Option<&PayjoinSenderConfig>, |
There was a problem hiding this comment.
Let's move this argument up to the other _configs, i.e., before seed_bytes.
src/builder.rs
Outdated
| }; | ||
|
|
||
| let (stop_sender, _) = tokio::sync::watch::channel(()); | ||
| let payjoin_sender = if let Some(payjoin_sender_config) = payjoin_sender_config { |
There was a problem hiding this comment.
Lets use payjoin_sender_config.as_ref().and_then(|psc| { .. }) pattern as for liquidity_source above.
src/payjoin_sender.rs
Outdated
| use std::ops::Deref; | ||
| use std::sync::Arc; | ||
| use std::time::Instant; | ||
| use tokio::time::sleep; |
There was a problem hiding this comment.
nit: Please don't import this globally, but rather use tokio::time::sleep in the code itself. Otherwise it's a bit confusing which sleep is meant and the blocking one from std really can't be used in async contexts.
src/payjoin_sender.rs
Outdated
| ) -> Option<Vec<u8>> { | ||
| let duration = std::time::Duration::from_secs(3600); | ||
| let sleep = || sleep(std::time::Duration::from_secs(10)); | ||
| loop { |
There was a problem hiding this comment.
Let's use the dedicated tokio macros for these control flows: tokio::select/tokio::time::timeout/tokio::time::interval, etc.
There was a problem hiding this comment.
not sure I fully follow how tokio::select can be utilised here, should I add a ticker similar to how its implement in the lib.rs and replace the loop with tokio::select ?
fixed the imports
There was a problem hiding this comment.
Yeah, if you figure out a way to do so, a combination of select and interval similar to lib.rs would be preferable to loop. If not, it's not that important.
There was a problem hiding this comment.
Still think we should clean this up using interval or similar.
src/payjoin_sender.rs
Outdated
| } | ||
|
|
||
| // get original inputs from original psbt clone (ocean_psbt) | ||
| let mut original_inputs = input_pairs(&mut ocean_psbt).peekable(); |
src/payjoin_sender.rs
Outdated
| let mut ocean_psbt = ocean_psbt.clone(); | ||
| // for BDK, we need to reintroduce utxo from original psbt. | ||
| // Otherwise we wont be able to sign the transaction. | ||
| fn input_pairs( |
There was a problem hiding this comment.
sorry, here is a link explaining why this is needed bitcoindevkit/bdk-cli#156 (comment)
There was a problem hiding this comment.
Thanks for the link but I mostly meant the local function? Given the complex function definition and the additional Box, couldn't we just psbt.unsigned_tx.input.iter().zip(&mut psbt.inputs) directly where it's used?
src/payjoin_sender.rs
Outdated
| } | ||
| } | ||
|
|
||
| let mut sign_options = SignOptions::default(); |
There was a problem hiding this comment.
As above, I think these specifics should live in the Wallet-side methods.
|
Thanks @tnull |
21d1070 to
c050240
Compare
src/payment/payjoin.rs
Outdated
| let sender = Arc::clone(&self.sender); | ||
| let (mut original_psbt, request, context) = | ||
| sender.create_payjoin_request(payjoin_uri, amount, fee_rate)?; | ||
| let response = sender.fetch(&request).await; |
There was a problem hiding this comment.
the idea here is that we try to fetch a response first, if we dont get one after the timeout we set on the request, we will start the background process to wait for the receiver response. I think we could emit an event in line 63
There was a problem hiding this comment.
I think it's kind of confusing to provide two separate control flows to the user. Can't we just tell the user to check back regularly instead of polling for an ~arbitrary amount of time after which we'll fail quietly?
If we really think we need the background polling, why not avoid returing anything here and just return the necessary information via an event? This would also solve the async/sync issue above.
edb5e21 to
01e8ab8
Compare
|
I believe I resolved all the points mentioned above but I still need to do another few things like examining the new structure when adding the payjoin receiver part and the channel opening and a bit more testing around the events. |
bec9439 to
aec7c39
Compare
|
Ready for another review. This commit is quite different from the last one.
.. and more(docs, error handling). Sorry for the delay. |
tnull
left a comment
There was a problem hiding this comment.
Thanks for the update! For now mostly had a look at the new events.
I'm not entirely convinced we need all of them? Or rather, I'm not sure how we expect users to act on them?
I also think we really need to find an identifier by which we can track payments for the whole flow.
Moreover, please proofread/spellcheck the docs and make sure they adhere to the usual formatting before pushing.
bindings/ldk_node.udl
Outdated
| "KVStoreSetupFailed", | ||
| "WalletSetupFailed", | ||
| "LoggerSetupFailed", | ||
| "InvalidPayjoinConfig", |
There was a problem hiding this comment.
Could you move this up to the other Invalid variants?
bindings/ldk_node.udl
Outdated
| ChannelPending(ChannelId channel_id, UserChannelId user_channel_id, ChannelId former_temporary_channel_id, PublicKey counterparty_node_id, OutPoint funding_txo); | ||
| ChannelReady(ChannelId channel_id, UserChannelId user_channel_id, PublicKey? counterparty_node_id); | ||
| ChannelClosed(ChannelId channel_id, UserChannelId user_channel_id, PublicKey? counterparty_node_id, ClosureReason? reason); | ||
| PayjoinPaymentPending(Txid txid, u64 amount, ScriptBuf receipient); |
There was a problem hiding this comment.
There's a few things here:
- fix typo (here and below):
| PayjoinPaymentPending(Txid txid, u64 amount, ScriptBuf receipient); | |
| PayjoinPaymentPending(Txid txid, u64 amount, ScriptBuf recipient); |
- Also, for amounts please always give the denomination in the variable name. I assume this should be
amount_sats? - Is there a better type available for the recipient rather than the generic
ScriptBuf? If not, can we create one?
There was a problem hiding this comment.
Fixed (1) and (2).
Regarding (3), we could use bitcoin::Address I guess. Any other types you think we should consider?
If we go with bitcoin::Address we would need to make a small addition to lightning::util::ser https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/util/ser.rs
There was a problem hiding this comment.
Mh, yeah, Address seems like a more suited type. Feel free to open a draft PR upstream I think. We can discuss there if others agree it makes sense.
There was a problem hiding this comment.
I just synced offline with @DanGould, seems the most suitable type to identify the receiver and the session end-to-end would be the receiver's public key? So, IIUC, we probably don't need lightningdevkit/rust-lightning#3206 after all?
EDIT: That is, https://docs.rs/payjoin/0.19.0/payjoin/receive/v2/struct.ActiveSession.html#method.public_key
src/event.rs
Outdated
| /// This will be `None` for events serialized by LDK Node v0.2.1 and prior. | ||
| reason: Option<ClosureReason>, | ||
| }, | ||
| /// This event is emitted when we initiate a Payjoin transaction and before negotiating with |
There was a problem hiding this comment.
Why do we emit an event if nothing happened yet?
There was a problem hiding this comment.
In start_request we basically create the original_psbt we are going to share with the receiver, we also register it with Filter and other stuff (https://github.com/lightningdevkit/ldk-node/pull/295/files#diff-87bb691298fda83f0bcc2d9145208f7725ed0aca25764ef1fd1839002e2bc72aR79), after we get a response from start_request we start the background process(PayjoinHandler::send_request) waiting for response from the Payjoin receiver. This event is basically indicating we are pending, waiting for a response. Maybe it should be called just before invoking send_request rather than in start_request
src/event.rs
Outdated
| /// receiver and we have finalised and broadcasted the transaction. | ||
| /// | ||
| /// This does not neccessarily imply the Payjoin transaction is fully successful. | ||
| PayjoinPaymentBroadcasted { |
There was a problem hiding this comment.
How do we envision the user to act on this event? Why is it useful?
Note that we can't technically emit this event when we have finalised and broadcasted the transaction, as broadcasting is always fallible, so at best we tried broadcasting.
There was a problem hiding this comment.
This is indicating to the user the payjoin process is completed and now they should wait for the on-chain confirmations.
..Pending is 'We are attempting a payjoin transaction'
..Broadcasted is 'Finished payjoin and tried to broadcast'
and then there is
..Failure and ..Success
src/event.rs
Outdated
| /// This event is emitted when the Payjoin receiver has received our offer but decided to | ||
| /// broadcast the `original_psbt` without any modifications. i.e., the receiver has declined to | ||
| /// participate in the Payjoin transaction and will receive the funds in a regular transaction. | ||
| PayjoinPaymentBroadcastedByReceiver { |
There was a problem hiding this comment.
Similar as above: Do we need this event? How do we expect a user to act on it? Should we rather act on it for the user? Generally, this seems very very specific to the Payjoin flow, and in LDK Node we try to provide a simplified interface that makes sense even if you're not aware of protocol details.
There was a problem hiding this comment.
Yes, I think so. We let the user know that the Payjoin flow ended without conducting the actual flow but the receiver decided to broadcast the original psbt.
Users might decide to not payjoin in the future with those type of Payjoin receivers in the future. From our side we should update the payment status and wait for it to be confirmed.
src/event.rs
Outdated
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub enum PayjoinPaymentFailureReason { | ||
| /// Didnt receive a response from the receiver. This is considered a failure but the receiver |
There was a problem hiding this comment.
Please write documentation in full sentences and double-check the spelling and form before pushing.
Also, the documentation of all fields should start by a single-sentence paragraph describing the field, before going into detail in further paragraphs.
| /// Didnt receive a response from the receiver. This is considered a failure but the receiver | |
| /// The request failed as we did not receive a response in time. | |
| /// | |
| /// This is considered a failure but the receiver |
src/event.rs
Outdated
| /// can still broadcast the original PSBT, in which case a | ||
| /// `PayjoinPaymentBroadcastedByReceiver` event will be emitted. | ||
| Timeout, | ||
| /// This can be due to insufficient funds, network issues, or other reasons. The exact reason |
There was a problem hiding this comment.
What is "This" referring to?
src/event.rs
Outdated
| ResponseProcessingFailed, | ||
| } | ||
|
|
||
| impl Readable for PayjoinPaymentFailureReason { |
There was a problem hiding this comment.
Please use LDK's serialization macros (in this case impl_writeable_tlv_based_enum) rather than manually implementing Readable/Writeable.
src/lib.rs
Outdated
|
|
||
| /// Returns a payjoin payment handler allowing to send payjoin transactions | ||
| /// | ||
| /// In order to utilize the Payjoin functionality, it's necessary to configure your node using |
There was a problem hiding this comment.
| /// In order to utilize the Payjoin functionality, it's necessary to configure your node using | |
| /// In order to utilize the Payjoin functionality, it is necessary to configure a Payjoin relay using |
src/payment/payjoin/handler.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl Filter for PayjoinHandler { |
There was a problem hiding this comment.
This shouldn't implment but just use Filter.
| self.internal_best_block_updated(height); | ||
| } | ||
|
|
||
| fn transaction_unconfirmed(&self, _txid: &Txid) {} |
There was a problem hiding this comment.
Do we need to handle unconfirmations?
There was a problem hiding this comment.
Yea. how do you think it should be approached?
Thought about restarting states(confirmation hight, and others) and try to rebroadcast.
src/payment/payjoin/handler.rs
Outdated
| fn internal_transactions_confirmed( | ||
| &self, header: &Header, txdata: &TransactionData, height: u32, | ||
| ) { | ||
| let (_, tx) = txdata[0]; |
There was a problem hiding this comment.
So we're only ever interested in the first confirmed transaction in a block?
There was a problem hiding this comment.
hmm I might have misunderstood how this behaves. let me revisit this.
tnull
left a comment
There was a problem hiding this comment.
A few more comments, have yet to conduct a full top-bottom review of the current state.
src/uniffi_types.rs
Outdated
| type Builtin = String; | ||
|
|
||
| fn into_custom(val: Self::Builtin) -> uniffi::Result<Self> { | ||
| Ok(ScriptBuf::from_hex(&val).map_err(|_| Error::InvalidPublicKey)?) |
There was a problem hiding this comment.
Is this supposed to be a PublicKey? What if we'd ever want to use ScriptBuf outside of the narrow context you're using it for currently?
src/payment/payjoin/mod.rs
Outdated
| break; | ||
| }, | ||
| Ok(None) => { | ||
| dbg!("Payjoin request sent, waiting for response..."); |
There was a problem hiding this comment.
Please remove dbg lines before pushing.
|
@tnull bear with me a moment please as I am adding tests and polishing the pr to make it more clear |
Of course, please ping me whenever you feel it's ready for the next round of review! |
|
@tnull Thanks for the ongoing review. Few things done:
There are still few things to cover/discuss, but I would appreciate a review over the usage of the |
tnull
left a comment
There was a problem hiding this comment.
Did another ~half pass, have yet to take a closer look at the PayjoinHandler and Confirm logic (note we should probably also implement Listen while we're at it, as syncing full blocks is coming up).
|
|
||
| impl Confirm for PayjoinHandler { | ||
| fn transactions_confirmed(&self, header: &Header, txdata: &TransactionData, height: u32) { | ||
| self.internal_transactions_confirmed(header, txdata, height); |
There was a problem hiding this comment.
Why not inline these internal_ methods here? Do we gain something from them being separate?
Maybe they would be reusabe for implementing Listen, which we should probably also do?
Payjoin [`BIP77`] implementation. Compatible with previous Payjoin version [`BIP78`]. Should be retrieved by calling [`Node::payjoin_payment`]. Payjoin transactions can be used to improve privacy by breaking the common-input-ownership heuristic when Payjoin receivers contribute input(s) to the transaction. They can also be used to save on fees, as the Payjoin receiver can direct the incoming funds to open a lightning channel, forwards the funds to another address, or simply consolidate UTXOs. In a Payjoin transaction, both the sender and receiver contribute inputs to the transaction in a coordinated manner. The Payjoin mechanism is also called pay-to-endpoint(P2EP). The Payjoin receiver endpoint address is communicated through a [`BIP21`] URI, along with the payment address and an optional amount parameter. In the Payjoin process, parties edit, sign and pass iterations of the transaction between each other, before a final version is broadcasted by the Payjoin sender. [`BIP77`] codifies a protocol with 2 iterations (or one round of interaction beyond address sharing). [`BIP77`] Defines the Payjoin process to happen asynchronously, with the Payjoin receiver enrolling with a Payjoin Directory to receive Payjoin requests. The Payjoin sender can then make requests through a proxy server, Payjoin Relay, to the Payjoin receiver even if the receiver is offline. This mechanism requires the Payjoin sender to regularly check for response from the Payjoin receiver as implemented in [`Node::payjoin_payment::send`]. A Payjoin Relay is a proxy server that forwards Payjoin requests from the Payjoin sender to the Payjoin receiver subdirectory. A Payjoin Relay can be run by anyone. Public Payjoin Relay servers are: - https://pj.bobspacebkk.com A Payjoin directory is a service that allows Payjoin receivers to receive Payjoin requests offline. A Payjoin directory can be run by anyone. Public Payjoin Directory servers are: - https://payjo.in
The `Confirm` trait is implemented in order to track the Payjoin transaction(s). We track two different transaction: 1. Original PSBT, which is the initial transaction sent to the Payjoin receiver. The receiver can decide to broadcast this transaction instead of finishing the Payjoin flow. Those we track it. 2. Final Payjoin transaction. The transaction constructed after completing the Payjoin flow, validated and broadcasted by the Payjoin sender.
Payjoin transactions
|
It seems that |
Alright. Your choice when to bump here and in the other PR. Feel free to go the path of least effort/lowest likelihood of errors. |
|
The latest release is planned to be the final breaking BIP 77 wire protocol changes. I recommend beginning to upgrade since it's possible to make a working solution based on the reference implementation in payjoin-cli (using Receiver.id() and Sender.pj_url() as the id for the Sender.) Since these are short-lived session IDs, even if the data identifier is changed after merge here (which it won't be) there's no critical persisted. data lost. I'll ask @nothingmuch what he thinks about this issue in more detail now that we've firmed up the wire protocol |
|
As there hasn't been any movement here for a long time, I'm closing this as abandoned. If someone is going to pick up the work again, this will need some serious rebase work. |
Partially addresses #177
This adds the ability for the on chain wallet to send payjoin transactions as described in BIP 77 https://github.com/bitcoin/bips/blob/d7ffad81e605e958dcf7c2ae1f4c797a8631f146/bip-0077.mediawiki
The payjoin receiver part will be added in a separate PR with e2e tests with this pr code.