Static invoice server #3628
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1582,6 +1582,78 @@ pub enum Event { | |
| /// onion messages. | ||
| peer_node_id: PublicKey, | ||
| }, | ||
| /// As a static invoice server, we received a [`StaticInvoice`] from an async recipient that wants | ||
| /// us to serve the invoice to payers on their behalf when they are offline. This event will only | ||
| /// be generated if we previously created paths using | ||
| /// [`ChannelManager::blinded_paths_for_async_recipient`] and the recipient was configured with | ||
| /// them via [`ChannelManager::set_paths_to_static_invoice_server`]. | ||
| /// | ||
| /// [`ChannelManager::blinded_paths_for_async_recipient`]: crate::ln::channelmanager::ChannelManager::blinded_paths_for_async_recipient | ||
| /// [`ChannelManager::set_paths_to_static_invoice_server`]: crate::ln::channelmanager::ChannelManager::set_paths_to_static_invoice_server | ||
| #[cfg(async_payments)] | ||
| PersistStaticInvoice { | ||
|
There was a problem hiding this comment. I assume there has been prior discussion around this that I'm unware of, but it strikes me as a bit odd to manage persistence via the event queue, if we have Pushing/popping the event will result in at least two additional There was a problem hiding this comment. This is similar to the approach that was taken in #2973, though unfortunately I can't find a link to any discussion that took place landing on that approach... But, either way, I think the server needs to verify the
Hopefully @joostjager and I can get rid of There was a problem hiding this comment.
Hmm, but even then we'd repersist the event queue twice, no? Granted, it would be much less overhead, but we'd still eat the IO latency once or twice before actually getting to persist the data (incurring more IO latency). There was a problem hiding this comment. The link to I do see some intersection with #3778. The events as proposed in this PR would allow for async persistence, but it is yet again another way to do it alongside a native async interface and the InProgress/callback mechanism in chain monitor. There was a problem hiding this comment.
Hmm... Thoughts on a separate PR that would allow us to process the event queue and skip manager persistence entirely for for certain events? With these events and several others I can think of, no manager persistence is needed and an event being dropped/failed to handle/redundantly handled should not be a problem. I guess I'm just not sure how we can properly rate limit without knowing anything about the underlying database and its limitations. It feels like an instance where finer-grained control might be desirable and There was a problem hiding this comment. Assuming almost all users would use ldk-node, it doesn't matter so much in which layer There was a problem hiding this comment.
I agree with this sentiment: generally I don't have too much of a strong opinion on how we do it, but (ab)using the persisted event queue for persistence calls seems like unnecessary overhead, IMO. There was a problem hiding this comment. As mentioned, I think we can avoid persisting the event queue when these events are present specifically, as well as existing non-critical events that are in the same boat like But, a separate trait is another approach that has been discussed in the past that we can definitely revisit, will start an offline discussion. Edit: discussed offline, all the options have downsides so going with the event approach for now. A separate trait would mean blocking other onion messenger operations on persistence. |
||
| /// The invoice that should be persisted and later provided to payers when handling a future | ||
| /// [`Event::StaticInvoiceRequested`]. | ||
| invoice: StaticInvoice, | ||
| /// Useful for the recipient to replace a specific invoice stored by us as the static invoice | ||
| /// server. | ||
| /// | ||
| /// When this invoice and its metadata are persisted, this slot number should be included so if | ||
| /// we receive another [`Event::PersistStaticInvoice`] containing the same slot number we can | ||
| /// swap the existing invoice out for the new one. | ||
| invoice_slot: u16, | ||
| /// An identifier for the recipient, originally provided to | ||
| /// [`ChannelManager::blinded_paths_for_async_recipient`]. | ||
| /// | ||
| /// When an [`Event::StaticInvoiceRequested`] comes in for the invoice, this id will be surfaced | ||
| /// and can be used alongside the `invoice_id` to retrieve the invoice from the database. | ||
| recipient_id: Vec<u8>, | ||
| /// A random identifier for the invoice. When an [`Event::StaticInvoiceRequested`] comes in for | ||
| /// the invoice, this id will be surfaced and can be used alongside the `recipient_id` to | ||
| /// retrieve the invoice from the database. | ||
| /// | ||
| /// Note that this id will remain the same for all invoice updates corresponding to a particular | ||
| /// offer that the recipient has cached. | ||
| invoice_id: u128, | ||
| /// Once the [`StaticInvoice`], `invoice_slot` and `invoice_id` are persisted, | ||
| /// [`ChannelManager::static_invoice_persisted`] should be called with this responder to confirm | ||
| /// to the recipient that their [`Offer`] is ready to be used for async payments. | ||
| /// | ||
| /// [`ChannelManager::static_invoice_persisted`]: crate::ln::channelmanager::ChannelManager::static_invoice_persisted | ||
| /// [`Offer`]: crate::offers::offer::Offer | ||
| invoice_persisted_path: Responder, | ||
| }, | ||
| /// As a static invoice server, we received an [`InvoiceRequest`] on behalf of an often-offline | ||
| /// recipient for whom we are serving [`StaticInvoice`]s. | ||
| /// | ||
| /// This event will only be generated if we previously created paths using | ||
| /// [`ChannelManager::blinded_paths_for_async_recipient`] and the recipient was configured with | ||
| /// them via [`ChannelManager::set_paths_to_static_invoice_server`]. | ||
| /// | ||
| /// If we previously persisted a [`StaticInvoice`] from an [`Event::PersistStaticInvoice`] that | ||
| /// matches the below `recipient_id` and `invoice_id`, that invoice should be retrieved now | ||
| /// and forwarded to the payer via [`ChannelManager::send_static_invoice`]. | ||
| /// | ||
| /// [`ChannelManager::blinded_paths_for_async_recipient`]: crate::ln::channelmanager::ChannelManager::blinded_paths_for_async_recipient | ||
| /// [`ChannelManager::set_paths_to_static_invoice_server`]: crate::ln::channelmanager::ChannelManager::set_paths_to_static_invoice_server | ||
| /// [`InvoiceRequest`]: crate::offers::invoice_request::InvoiceRequest | ||
| /// [`ChannelManager::send_static_invoice`]: crate::ln::channelmanager::ChannelManager::send_static_invoice | ||
| #[cfg(async_payments)] | ||
| StaticInvoiceRequested { | ||
| /// An identifier for the recipient previously surfaced in | ||
| /// [`Event::PersistStaticInvoice::recipient_id`]. Useful when paired with the `invoice_id` to | ||
| /// retrieve the [`StaticInvoice`] requested by the payer. | ||
| recipient_id: Vec<u8>, | ||
| /// A random identifier for the invoice being requested, previously surfaced in | ||
| /// [`Event::PersistStaticInvoice::invoice_id`]. Useful when paired with the `recipient_id` to | ||
| /// retrieve the [`StaticInvoice`] requested by the payer. | ||
| invoice_id: u128, | ||
| /// The path over which the [`StaticInvoice`] will be sent to the payer, which should be | ||
| /// provided to [`ChannelManager::send_static_invoice`] along with the invoice. | ||
| /// | ||
| /// [`ChannelManager::send_static_invoice`]: crate::ln::channelmanager::ChannelManager::send_static_invoice | ||
| reply_path: Responder, | ||
| }, | ||
| } | ||
|
|
||
| impl Writeable for Event { | ||
|
|
@@ -2012,6 +2084,17 @@ impl Writeable for Event { | |
| (8, former_temporary_channel_id, required), | ||
| }); | ||
| }, | ||
| #[cfg(async_payments)] | ||
| &Event::PersistStaticInvoice { .. } => { | ||
| 45u8.write(writer)?; | ||
|
There was a problem hiding this comment. Is it necessary to write anything at all? There was a problem hiding this comment. I'm not 100% sure, but we do this for several other events and generally always write something for all enum variants, so would like to be consistent There was a problem hiding this comment. Maybe the serialize/deserialize round trip test trips over this? It seems a waste of space and disk access. Also rate limiting is only happening after the persist cycle? On the other hand, an LSP probably needs to rate limit at a different level (also). |
||
| // No need to write these events because we can just restart the static invoice negotiation | ||
| // on startup. | ||
| }, | ||
| #[cfg(async_payments)] | ||
| &Event::StaticInvoiceRequested { .. } => { | ||
| 47u8.write(writer)?; | ||
| // Never write StaticInvoiceRequested events as buffered onion messages aren't serialized. | ||
| }, | ||
| // Note that, going forward, all new events must only write data inside of | ||
| // `write_tlv_fields`. Versions 0.0.101+ will ignore odd-numbered events that write | ||
| // data via `write_tlv_fields`. | ||
|
|
@@ -2583,6 +2666,12 @@ impl MaybeReadable for Event { | |
| former_temporary_channel_id: former_temporary_channel_id.0.unwrap(), | ||
| })) | ||
| }, | ||
| // Note that we do not write a length-prefixed TLV for PersistStaticInvoice events. | ||
| #[cfg(async_payments)] | ||
| 45u8 => Ok(None), | ||
| // Note that we do not write a length-prefixed TLV for StaticInvoiceRequested events. | ||
| #[cfg(async_payments)] | ||
| 47u8 => Ok(None), | ||
| // Versions prior to 0.0.100 did not ignore odd types, instead returning InvalidValue. | ||
| // Version 0.0.100 failed to properly ignore odd types, possibly resulting in corrupt | ||
| // reads. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.