Async recipient-side of static invoice server #3618
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,6 +85,7 @@ use crate::ln::outbound_payment::{ | |
| SendAlongPathArgs, StaleExpiration, | ||
| }; | ||
| use crate::ln::types::ChannelId; | ||
| use crate::offers::async_receive_offer_cache::AsyncReceiveOfferCache; | ||
valentinewallace marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| use crate::offers::flow::OffersMessageFlow; | ||
| use crate::offers::invoice::{ | ||
| Bolt12Invoice, DerivedSigningPubkey, InvoiceBuilder, DEFAULT_RELATIVE_EXPIRY, | ||
|
|
@@ -97,7 +98,8 @@ use crate::offers::parse::Bolt12SemanticError; | |
| use crate::offers::refund::Refund; | ||
| use crate::offers::signer; | ||
| use crate::onion_message::async_payments::{ | ||
| AsyncPaymentsMessage, AsyncPaymentsMessageHandler, HeldHtlcAvailable, OfferPaths, | ||
| OfferPathsRequest, ReleaseHeldHtlc, ServeStaticInvoice, StaticInvoicePersisted, | ||
| }; | ||
| use crate::onion_message::dns_resolution::HumanReadableName; | ||
| use crate::onion_message::messenger::{ | ||
|
|
@@ -5103,7 +5105,24 @@ where | |
| } | ||
|
|
||
| #[cfg(async_payments)] | ||
| fn check_refresh_async_receive_offers(&self) { | ||
| let peers = self.get_peers_for_blinded_path(); | ||
| let channels = self.list_usable_channels(); | ||
| let entropy = &*self.entropy_source; | ||
| let router = &*self.router; | ||
| match self.flow.check_refresh_async_receive_offers(peers, channels, entropy, router) { | ||
| Err(()) => { | ||
| log_error!( | ||
| self.logger, | ||
| "Failed to create blinded paths when requesting async receive offer paths" | ||
| ); | ||
| }, | ||
| Ok(()) => {}, | ||
| } | ||
| } | ||
|
|
||
| #[rustfmt::skip] | ||
| #[cfg(async_payments)] | ||
| fn initiate_async_payment( | ||
| &self, invoice: &StaticInvoice, payment_id: PaymentId | ||
| ) -> Result<(), Bolt12PaymentError> { | ||
|
|
@@ -7047,6 +7066,9 @@ where | |
| duration_since_epoch, &self.pending_events | ||
| ); | ||
|
|
||
| #[cfg(async_payments)] | ||
| self.check_refresh_async_receive_offers(); | ||
|
|
||
| // Technically we don't need to do this here, but if we have holding cell entries in a | ||
| // channel that need freeing, it's better to do that here and block a background task | ||
| // than block the message queueing pipeline. | ||
|
|
@@ -10586,9 +10608,23 @@ where | |
| #[cfg(c_bindings)] | ||
| create_refund_builder!(self, RefundMaybeWithDerivedMetadataBuilder); | ||
|
|
||
| /// Retrieve our cached [`Offer`]s for receiving async payments as an often-offline recipient. | ||
valentinewallace marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /// Will only be set if [`UserConfig::paths_to_static_invoice_server`] is set and we succeeded in | ||
| /// interactively building a [`StaticInvoice`] with the static invoice server. | ||
| /// | ||
| /// Useful for posting offers to receive payments later, such as posting an offer on a website. | ||
| #[cfg(async_payments)] | ||
|
There was a problem hiding this comment. Definitely looks like too many async payments cfg directives to me. |
||
| pub fn get_cached_async_receive_offers(&self) -> Vec<Offer> { | ||
|
There was a problem hiding this comment. These pub fns added, do they need a high level test? Also curious to see how this is going to be used in ldk node. There was a problem hiding this comment. This is tested in #3628. As discussed offline, I looked into migrating some of the testing to this PR by using mocks for the server. It doesn't look particularly trivial honestly, we'd still need a lot of the code that's in the follow-up for unwrapping + sending messages and creating blinded paths on behalf of the server. I'm still going to try to get you something to look at on the ldk-node front though 🫡 hoping to have some updates soon. There was a problem hiding this comment. Would the order have mattered here? If the server pr would have been the first, would it have been easier to mock the client? Not asking for this, to be clear. Just curious. There was a problem hiding this comment. Tests in the follow up look good indeed. |
||
| self.flow.get_cached_async_receive_offers() | ||
| } | ||
|
|
||
| /// Create an offer for receiving async payments as an often-offline recipient. | ||
| /// | ||
| /// Instead of using this method, it is preferable to set | ||
| /// [`UserConfig::paths_to_static_invoice_server`] and retrieve the automatically built offer via | ||
| /// [`Self::get_cached_async_receive_offers`]. | ||
| /// | ||
| /// If you want to build the [`StaticInvoice`] manually using this method instead, you MUST: | ||
| /// 1. Provide at least 1 [`BlindedMessagePath`] terminating at an always-online node that will | ||
| /// serve the [`StaticInvoice`] created from this offer on our behalf. | ||
| /// 2. Use [`Self::create_static_invoice_builder`] to create a [`StaticInvoice`] from this | ||
|
|
@@ -10605,6 +10641,10 @@ where | |
| /// Creates a [`StaticInvoiceBuilder`] from the corresponding [`Offer`] and [`Nonce`] that were | ||
| /// created via [`Self::create_async_receive_offer_builder`]. If `relative_expiry` is unset, the | ||
| /// invoice's expiry will default to [`STATIC_INVOICE_DEFAULT_RELATIVE_EXPIRY`]. | ||
| /// | ||
| /// Instead of using this method to manually build the invoice, it is preferable to set | ||
| /// [`UserConfig::paths_to_static_invoice_server`] and retrieve the automatically built offer via | ||
| /// [`Self::get_cached_async_receive_offers`]. | ||
| #[cfg(async_payments)] | ||
| #[rustfmt::skip] | ||
| pub fn create_static_invoice_builder<'a>( | ||
|
|
@@ -11482,6 +11522,13 @@ where | |
| return NotifyOption::SkipPersistHandleEvents; | ||
| //TODO: Also re-broadcast announcement_signatures | ||
| }); | ||
|
|
||
| // While we usually refresh the AsyncReceiveOfferCache on a timer, we also want to start | ||
| // interactively building offers as soon as we can after startup. We can't start building offers | ||
| // until we have some peer connection(s) to send onion messages over, so as a minor optimization | ||
| // refresh the cache when a peer connects. | ||
| #[cfg(async_payments)] | ||
| self.check_refresh_async_receive_offers(); | ||
valentinewallace marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| res | ||
| } | ||
|
|
||
|
|
@@ -12806,6 +12853,60 @@ where | |
| MR::Target: MessageRouter, | ||
| L::Target: Logger, | ||
| { | ||
| fn handle_offer_paths_request( | ||
| &self, _message: OfferPathsRequest, _context: AsyncPaymentsContext, | ||
| _responder: Option<Responder>, | ||
| ) -> Option<(OfferPaths, ResponseInstruction)> { | ||
| None | ||
| } | ||
|
|
||
| fn handle_offer_paths( | ||
| &self, _message: OfferPaths, _context: AsyncPaymentsContext, _responder: Option<Responder>, | ||
| ) -> Option<(ServeStaticInvoice, ResponseInstruction)> { | ||
| #[cfg(async_payments)] | ||
|
There was a problem hiding this comment. Maybe I am still surprised that this lives in channelmanager. Didn't look into the flow refactoring yet, but I thought the goal was to keep high level stuff like offers out of chan mgr? There was a problem hiding this comment. Hmm, from my view this this just calls into the flow so nothing substantial is really living in There was a problem hiding this comment. That is true indeed, makes sense then. Perhaps payments and offers as a while could be more separate from low-level channel management, but that is probably a whole different thing. |
||
| { | ||
| let responder = match _responder { | ||
| Some(responder) => responder, | ||
| None => return None, | ||
| }; | ||
| let (serve_static_invoice, reply_context) = match self.flow.handle_offer_paths( | ||
| _message, | ||
| _context, | ||
| responder.clone(), | ||
| self.get_peers_for_blinded_path(), | ||
| self.list_usable_channels(), | ||
| &*self.entropy_source, | ||
| &*self.router, | ||
| ) { | ||
| Some((msg, ctx)) => (msg, ctx), | ||
| None => return None, | ||
| }; | ||
| let response_instructions = responder.respond_with_reply_path(reply_context); | ||
| return Some((serve_static_invoice, response_instructions)); | ||
| } | ||
|
|
||
| #[cfg(not(async_payments))] | ||
| return None; | ||
| } | ||
|
|
||
| fn handle_serve_static_invoice( | ||
| &self, _message: ServeStaticInvoice, _context: AsyncPaymentsContext, | ||
| _responder: Option<Responder>, | ||
| ) { | ||
| } | ||
|
|
||
| fn handle_static_invoice_persisted( | ||
| &self, _message: StaticInvoicePersisted, _context: AsyncPaymentsContext, | ||
| ) { | ||
| #[cfg(async_payments)] | ||
| { | ||
| let should_persist = self.flow.handle_static_invoice_persisted(_context); | ||
| if should_persist { | ||
| let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); | ||
|
There was a problem hiding this comment. What is the point of taking the There was a problem hiding this comment. Hmm, why would we introduce lock order deps between the |
||
| } | ||
| } | ||
| } | ||
|
|
||
| #[rustfmt::skip] | ||
| fn handle_held_htlc_available( | ||
| &self, _message: HeldHtlcAvailable, _context: AsyncPaymentsContext, | ||
|
|
@@ -13658,6 +13759,7 @@ where | |
| (15, self.inbound_payment_id_secret, required), | ||
| (17, in_flight_monitor_updates, option), | ||
| (19, peer_storage_dir, optional_vec), | ||
| (21, self.flow.writeable_async_receive_offer_cache(), required), | ||
| }); | ||
|
|
||
| Ok(()) | ||
|
|
@@ -14222,6 +14324,7 @@ where | |
| let mut decode_update_add_htlcs: Option<HashMap<u64, Vec<msgs::UpdateAddHTLC>>> = None; | ||
| let mut inbound_payment_id_secret = None; | ||
| let mut peer_storage_dir: Option<Vec<(PublicKey, Vec<u8>)>> = None; | ||
| let mut async_receive_offer_cache: AsyncReceiveOfferCache = AsyncReceiveOfferCache::new(); | ||
| read_tlv_fields!(reader, { | ||
| (1, pending_outbound_payments_no_retry, option), | ||
| (2, pending_intercepted_htlcs, option), | ||
|
|
@@ -14239,6 +14342,7 @@ where | |
| (15, inbound_payment_id_secret, option), | ||
| (17, in_flight_monitor_updates, option), | ||
| (19, peer_storage_dir, optional_vec), | ||
| (21, async_receive_offer_cache, (default_value, async_receive_offer_cache)), | ||
|
There was a problem hiding this comment. This cache seems so far away from channel manager core responsibilities. Can't it be persisted elsewhere / separately? I also thought that for fixing the force closures, we wanted to move away from channel mgr persistence completely and reconstruct everything from the monitors. There was a problem hiding this comment. Yeah, it crossed my mind that this might come up when we address removing There was a problem hiding this comment. Agree that it can be removed easily. And also that it is nice to have an idea about where to take this eventually. |
||
| }); | ||
| let mut decode_update_add_htlcs = decode_update_add_htlcs.unwrap_or_else(|| new_hash_map()); | ||
| let peer_storage_dir: Vec<(PublicKey, Vec<u8>)> = peer_storage_dir.unwrap_or_else(Vec::new); | ||
|
|
@@ -14925,6 +15029,8 @@ where | |
| chain_hash, best_block, our_network_pubkey, | ||
| highest_seen_timestamp, expanded_inbound_key, | ||
| secp_ctx.clone(), args.message_router | ||
| ).with_async_payments_offers_cache( | ||
| async_receive_offer_cache, &args.default_config.paths_to_static_invoice_server[..] | ||
| ); | ||
|
|
||
| let channel_manager = ChannelManager { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good explanation in the commit message. Maybe something can be added to highlight the downside of not having a static invoice server. Suppose the user would just hand over their keysend invoice to the sender or publish it on a website, what goes wrong? I suppose something goes wrong when the paths would need to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit message reads very well. Some of that valuable info might even be moved into code comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for the part discussing alternative designs, I'm going to address those in a bLIP and we can link to that here. Did you want the high-level flow explanation to go in the docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For someone new to the code, it is useful to be able to read the high level flow somewhere. The commit message won't be visible anymore. But a link to a blip explaining the high level flow and alternative designs seems fine too.