Add Payjoin Receiver Support (BIP 77)#746
Add Payjoin Receiver Support (BIP 77)#746Camillarhi wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @bc1cindy as a reviewer! |
1db2b08 to
49017bd
Compare
|
We've merged the async persistence PR you mentioned. You might want to build your draft PR on the merged commit from there on until we cut you a release. |
Thanks for letting me know. I'll build on the merged commit. |
8f6ba65 to
6499918
Compare
|
Are you stuck? Did something in our library break CI @Camillarhi |
Not stuck at all. I was just closing out some other PRs. Still working on this one, I'll let you know when it's ready. |
12a41ad to
eb97832
Compare
120b089 to
8cc2a31
Compare
5fe1a5c to
0411ada
Compare
|
Marking this as ready for review. The core receiver flow is implemented, including session persistence, PSBT handling, input contribution, mempool monitoring for payjoin transactions, and node restart recovery. Two things still pending that I'll follow up with:
Happy to get early feedback on the overall approach in the meantime. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
I'm using as default nightly toolchain and ldk node uses stable, so that's what was happening |
spacebear21
left a comment
There was a problem hiding this comment.
This is looking great! I have high-level feedback from the payjoin side, but overall agree with the approach and implementation.
src/chain/mod.rs
Outdated
| ChainSourceKind::Esplora{..} => { | ||
| // Esplora does not support a testmempoolaccept equivalent, so we skip | ||
| // the broadcast suitability check and assume the transaction is broadcastable. | ||
| log_debug!(self.logger, "Skipping broadcast suitability check: not supported by Esplora backend."); | ||
| Ok(true) | ||
| }, | ||
| ChainSourceKind::Electrum{..} => { | ||
| // Electrum does not support a testmempoolaccept equivalent, so we skip | ||
| // the broadcast suitability check and assume the transaction is broadcastable. | ||
| log_debug!(self.logger, "Skipping broadcast suitability check: not supported by Electrum backend."); | ||
| Ok(true) |
There was a problem hiding this comment.
The broadcast suitability check is important for non-interactive receivers (e.g. payment processors) to protect against probing attacks, where a malicious sender can repeatedly send proposals to have the non-interactive receiver reveal the UTXOs it owns with the proposals it modifies. The "broadcastable" check ensures the receiver can broadcast the fallback transaction in case the payjoin fails, effectively incurring a cost to the attacker.
Electrum's lack of support for this kind of check is a longstanding issue unfortunately.
I'm not familiar enough with LDK-node usage to know if it's typically used in an interactive way (receiver takes manual action to receive payment) or non-interactive (e.g. bolt12 or similar static payment with no receiver action per payment)?
In interactive conditions, the receiver may use the assume_interactive_receiver transition method to explicitly skip the broadcast check altogether. In non-interactive conditions though, we should find an alternative way to perform this check. Or at least return an error instead of silently skipping the check, so it can be handled by the implementer.
There was a problem hiding this comment.
I'm not familiar enough with LDK-node usage to know if it's typically used in an interactive way (receiver takes manual action to receive payment) or non-interactive
it's mostly non-interactive
There was a problem hiding this comment.
It is non-interactive. I'll update both Esplora and Electrum to return an error instead so the limitation is explicit. Thanks for the context on the probing attack.
| // is assumed to be broadcastable. | ||
| let proposal = proposal | ||
| .check_broadcast_suitability(None, |tx| { | ||
| self.chain_source |
There was a problem hiding this comment.
So here for example we could call proposal.assume_interactive_receiver() if applicable.
| Error::PayjoinSessionFailed | ||
| })?; | ||
| let proposal = proposal | ||
| .contribute_inputs(vec![selected_input]) | ||
| .map_err(|e| { | ||
| log_error!(self.logger, "Failed to contribute inputs to payjoin: {}", e); | ||
| Error::PayjoinSessionFailed |
There was a problem hiding this comment.
Are these failure modes handled by broadcasting the fallback transaction? I'm trying to find where these errors are handled but not sure I'm tracing it correctly.
There was a problem hiding this comment.
When the session propagates to a Closed state which is handled in handle_closed_session, On Failure or Cancel, the fallback transaction (extracted and stored at the start of check_proposal) is broadcast to ensure the receiver still gets paid.
There was a problem hiding this comment.
I also have a question, Should failures at create_poll_request, try_preserving_privacy, contribute_inputs, and create_post_request trigger fallback broadcast immediately, or are these considered retryable until the session expires or propagates a Closed state? Wanted to align on the intended behavior before handling these inline.
There was a problem hiding this comment.
Good question, I had to go back and think through each case which means our API should be clearer about this... Anyway these are my thoughts:
create_poll_requestandcreate_post_requestare fatal, they only error due to crypto or config errors that won't resolve. In these cases we should trigger fallback immediately.try_preserving_privacyandcontribute_inputsdepend on the provided candidate inputs, so they are retryable in the sense that they could succeed with a different input set. Sincelist_input_pairs()is returning all spendable utxos, the "right" answer here depends on whether you want the user to maybe wait until new inputs become available, or just assume they won't change within the expiration window and broadcast the fallback immediately. I would guess there aren't many on-chain state changes to a typical ldk-node within a 24h window, so the latter choice seems appropriate.
There was a problem hiding this comment.
Thanks. I will update the failure handling for these cases accordingly
There was a problem hiding this comment.
try_preserving_privacyandcontribute_inputsdepend on the provided candidate inputs, so they are retryable in the sense that they could succeed with a different input set. Sincelist_input_pairs()is returning all spendable utxos, the "right" answer here depends on whether you want the user to maybe wait until new inputs become available, or just assume they won't change within the expiration window and broadcast the fallback immediately. I would guess there aren't many on-chain state changes to a typical ldk-node within a 24h window, so the latter choice seems appropriate.
Since these aren't fatal, I'd lean toward letting the session expire naturally rather than broadcasting immediately. The receiver is still covered on expiry.
src/config.rs
Outdated
| /// The URL of the OHTTP relay to use for sending OHTTP requests to PayJoin receivers. | ||
| pub ohttp_relay: URL, |
There was a problem hiding this comment.
We can make this a list of OHTTP relays and randomize the relay per-request. This also improves reliability by enabling retries to other relays in case one is offline for any reason.
There was a problem hiding this comment.
I changed ohttp_relay to ohttp_relays: Vec<URL>. Each request now picks a random starting relay and iterates through the rest as fallbacks, so if one relay is offline the next is tried immediately without waiting for a retry cycle.
| .payjoin_session_store | ||
| .list_filter(|s| { | ||
| let is_terminal = | ||
| s.status == PayjoinStatus::Completed || s.status == PayjoinStatus::Failed; |
There was a problem hiding this comment.
It may be desirable to keep "Completed" sessions ~forever since it makes it possible to reconstruct past Payjoins for e.g. displaying transaction history.
There was a problem hiding this comment.
Completed payjoin sessions are already recorded as Payjoin payments in the PaymentStore with full details on success, so the session itself doesn't need to be kept long-term for history purposes. @tnull, what are your thoughts on this as well.
There was a problem hiding this comment.
Makes sense, I see now PaymentDetails already contains all necessary information regarding payment amount and direction that would be ambiguous from the txid alone.
0411ada to
8ceddbe
Compare
|
🔔 3rd Reminder Hey @tnull @zealsham @spacebear21 @bc1cindy! This PR has been waiting for your review. |
bc1cindy
left a comment
There was a problem hiding this comment.
this is really looking good! all fixes applied.
here are some minor points:
| // Esplora does not support a testmempoolaccept equivalent | ||
| log_error!( | ||
| self.logger, | ||
| "Broadcast suitability check not supported by Esplora backend." |
There was a problem hiding this comment.
I think it's important to be explicit about probing attack, like: "Broadcast suitability check not supported by Esplora backend. Probing attack risk not mitigated."
There was a problem hiding this comment.
This method could be called from other contexts down the line where probing attacks aren't relevant, so I'd rather keep the message generic and let the caller log any threat-specific context.
| // Electrum does not support a testmempoolaccept equivalent. | ||
| log_error!( | ||
| self.logger, | ||
| "Broadcast suitability check not supported by Electrum backend." |
There was a problem hiding this comment.
I think it's important to be explicit about probing attack, like: "Broadcast suitability check not supported by Electrum backend. Probing attack risk not mitigated."
There was a problem hiding this comment.
This method could be called from other contexts down the line where probing attacks aren't relevant, so I'd rather keep the message generic and let the caller log any threat-specific context.
| let count = payjoin_config.ohttp_relays.len(); | ||
| let start = if count > 0 { | ||
| let mut bytes = [0u8; 8]; | ||
| getrandom::fill(&mut bytes).unwrap_or(()); |
There was a problem hiding this comment.
If getrandom::fill fails silently via unwrap_or(()) the selection it's going to be deterministically and not random
There was a problem hiding this comment.
The randomisation here is just for load balancing. It's not security-critical. Even if getrandom fails, we still try all relays in order, so nothing breaks. The worst case is relay[0] always goes first. Failing hard on this would be overkill.
src/payment/payjoin/manager.rs
Outdated
| self.payjoin_session_store.insert_or_update(session.clone())?; | ||
|
|
||
| let fallback_tx = session.fallback_tx.as_ref().ok_or_else(|| { | ||
| log_error!(self.logger, "Payjoin session {} missing txid.", session_id); |
There was a problem hiding this comment.
I believe it will be more clear using "Payjoin session {} missing fallback transaction", session_id) here
src/payment/payjoin/manager.rs
Outdated
| self.payjoin_session_store.insert_or_update(session.clone())?; | ||
|
|
||
| let fallback_tx = session.fallback_tx.as_ref().ok_or_else(|| { | ||
| log_error!(self.logger, "Payjoin session {} missing txid.", session_id); |
There was a problem hiding this comment.
I believe it will be more clear using "Payjoin session {} missing fallback transaction", session_id) here
src/payment/payjoin/manager.rs
Outdated
|
|
||
| // Note: broadcast suitability can only be fully verified when using the BitcoindRpc | ||
| // backend. For Esplora and Electrum backends this check is skipped and the transaction | ||
| // is assumed to be broadcastable. |
There was a problem hiding this comment.
it might also be good to explain probing attacks risk
There was a problem hiding this comment.
Thanks for pointing this out, I will also update the comment here, as the broadcast check will no longer be skipped
| } | ||
| } | ||
|
|
||
| fn close( |
There was a problem hiding this comment.
close() sets Completed status, but its called from close_failed_session() (manager.rs:820), with log message "Closed failed {} session", which is semantically inconsistent. Should this be addressed?
There was a problem hiding this comment.
Yes, it should be addressed. Thanks
|
🔔 1st Reminder Hey @tnull @zealsham @spacebear21! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @tnull @zealsham @spacebear21! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull @zealsham @spacebear21! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull @zealsham @spacebear21! This PR has been waiting for your review. |
1 similar comment
|
🔔 2nd Reminder Hey @tnull @zealsham @spacebear21! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tnull @zealsham @spacebear21! This PR has been waiting for your review. |
src/config.rs
Outdated
| #[derive(Debug, Clone)] | ||
| #[cfg_attr(feature = "uniffi", derive(uniffi::Record))] | ||
| pub struct PayjoinConfig { | ||
| /// The URL of the PayJoin directory to use for discovering PayJoin receivers. |
There was a problem hiding this comment.
Nit: the sender does not discover receivers using the directory.
i think just "The url of the payjoin directory" is fine
8ceddbe to
37cfb71
Compare
Implements the receiver side of the BIP 77 Payjoin v2 protocol, allowing LDK Node users to receive payjoin payments via a payjoin directory and OHTTP relay. - Adds a `PayjoinPayment` handler exposing a `receive()` method that returns a BIP 21 URI the sender can use to initiate the payjoin flow. The full receiver state machine is implemented covering all `ReceiveSession` states: polling the directory, validating the sender's proposal, contributing inputs, finalizing the PSBT, and monitoring the mempool. - Session state is persisted via `KVStorePayjoinReceiverPersister` and survives node restarts through event log replay. Sender inputs are tracked by `OutPoint` across polling attempts to prevent replay attacks. The sender's fallback transaction is broadcast on cancellation or failure to ensure the receiver still gets paid. - Adds `PaymentKind::Payjoin` to the payment store, `PayjoinConfig` for configuring the payjoin directory and OHTTP relay via `Builder::set_payjoin_config`, and background tasks for session resumption every 15 seconds and cleanup of terminal sessions after 24 hours.
37cfb71 to
98a3bc5
Compare
|
🔔 3rd Reminder Hey @tnull @zealsham @spacebear21 @bc1cindy! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tnull @zealsham @spacebear21 @bc1cindy! This PR has been waiting for your review. |
This PR adds support for receiving payjoin payments in LDK Node. This is currently a work in progress and implements the receiver side of the payjoin protocol.
KVStorePayjoinReceiverPersisterto handle session persistencePayjoinas aPaymentKindto the payment storeNote on persistence: The payjoin library currently only supports synchronous persistence, but they're working on adding async support(payjoin/rust-payjoin#1235). This PR sets up the persistence structure (
KVStorePayjoinReceiverPersister), which will be updated to use async operations once the upstream PR is merged.This PR will partially address #177