Provide Bolt12Invoice used for inbound payment
#2929
Conversation
In order to generate and InvoiceSent event, it would be cleaner to have one location where a Bolt12Invoice is successfully generated. Refactor the handling code to this end and clean-up line length by making some of the type conversions more streamlined.
In order to provide an InvoiceGenerated event, it would be cleaner to have one location where a Bolt12Invoice is successfully created. Refactor the handling code to this end and clean-up line length by making some of the type conversions more streamlined.
Bolt12Invoice will be added to a new Event::InvoiceGenerated variant. These traits along with PartialEq are required to be implemented for any type used in an Event.
When responding to an InvoiceRequest for an Offer, provide an event when an invoice has been generated. This allows event handler to know when an inbound payment may occur along with information from the invoice such as metadata, payer_id, and payment_hash.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2929 +/- ##
==========================================
+ Coverage 89.11% 89.30% +0.19%
==========================================
Files 117 117
Lines 95029 95929 +900
Branches 95029 95929 +900
==========================================
+ Hits 84685 85672 +987
+ Misses 7856 7783 -73
+ Partials 2488 2474 -14 ☔ View full report in Codecov by Sentry. |
When sending an invoice for a refund, information from the invoice may be useful for caller. For instance, the payment_hash can be used to track whether the refund was paid. Return the invoice to facilitate this use case.
jkczyz
force-pushed
the
2024-03-offers-events
branch
from
fc9fe8a to
35c408f
Compare
| /// | ||
| /// Thus, this event is largely for informational purposes as the corresponding [`Offer`] and | ||
| /// [`InvoiceRequest`] fields are accessible from the invoice. In particular: | ||
| /// - [`Bolt12Invoice::metadata`] can help identify the corresponding [`Offer`] |
There was a problem hiding this comment.
Can we make it a bit more explicit that this is the only way users can associate incoming payments with a previously-generated offer? I.e., the only way to know what was actually paid for is to handle this event and keep track of all sent-out payment hashes to be able to associate them with the offer based on metadata.
While this is probably fine for now, I really wonder if we eventually should take care of the tracking in LDK (or offer a utility for it at least), as every node receiving payments will want to know what a payment is for.
| @@ -1668,6 +1708,18 @@ impl MaybeReadable for Event { | |||
| }, | |||
| // Note that we do not write a length-prefixed TLV for ConnectionNeeded events. | |||
| 35u8 => Ok(None), | |||
| 37u8 => { | |||
| let f = || { | |||
| let mut invoice_bytes = WithoutLength(Vec::new()); | |||
There was a problem hiding this comment.
Any reason we can't just implement Readable for Bolt12Invoice? Seems avoiding it forces users (namely LDK Node, heh.) to write the same custom deserializaiton code. Same goes for Offer, Refund, and Bolt11Invoice, fwiw.
There was a problem hiding this comment.
serde would be great too
| /// | ||
| /// [`InvoiceRequest`]: crate::offers::invoice_request::InvoiceRequest | ||
| invoice: Bolt12Invoice, | ||
| }, |
There was a problem hiding this comment.
I wonder if we should provide the preimage/payment secret also while we're here?
|
I'm not quite clear on why we need to expose this when the Invoice is generated (I mean, I guess we should, as an informational and debug thing, but we historically have stayed away from informational |
|
I actually wonder if we should store the whole (or maybe select fields of the) invoice_request in the blinded path we give to the sender. That way it would be stateless and we could get the invoice back on payment. It'd bloat the blinded path, but in some cases that may be okay. |
The use case is for LDK Node showing a pending payment. In BOLT 11, we do this when creating the invoice. In BOLT 12, when creating an offer (1) we don't have a payment hash and (2) there could be many invoices corresponding to a single offer.
Yeah, I considered there may be a DoS risk, but we already rate limit outbound onion messages. This is worth discussing though. I suppose an adversary could send many invoice requests, each with a different introduction node in the reply path. But we'd still hit our overall buffer limit. Regarding lifetime, potentially some combination of the offer and invoice expirations could be used to expire pending payments by LDK Node.
Do you mean the invoice? For example, we'd probably want the payment hash. Possibly fields from the offer and invoice request, too, I suppose.
Not sure I understand what you mean by "we could get the invoice back on payment". We can't put the invoice in the blinded path because the blinded path is part of the invoice. Or are you saying to elide those? Note also there may be more than one in the invoice. Or did you really mean just the invoice request earlier? |
I'm not sure how to do this without turning it into a trivial DoS vector, so I'm not entirely convinced we should do this (maybe unless we receive a single HTLC of a potentially-MPP payment?
We rate-limit outbound onion messages to line rate (unless the user rate-limits their NIC in the OS), which is potentially a lot of onion messages. I don't think we should rely on that for pending-payments-list DoS resistance (especially if that list is stored on disk).
I don't think this is sufficient. Imagine a line-rate of 1Gbps (hosted node or someone with fiber in their home, which is increasingly common), assuming the pending payments are roughly the same size as an invoice_request (or contain the invoice_request), that means someone can fill 1Gbps * the timeout, which lets say is 1 hour, means someone can put 450 GB of crap on our disk (and burn out our SSD's write lifetime in a month or two).
Yea, indeed, you can't see when someone may have initiated a payment to your offer, but I'm not sure we can do a lot better without being a big DoS risk. The other potential approach we could do here is some kind of "tracked offer", ie create an offer which you only intend to receive payment to once, then track whether that offer gets paid (sure, it could get paid twice, but someone can always overpay us).
Yes, I meant to elide the self-referential parts. But we could also just store the invreq if that makes sense. |
Seems that would prevent having stateless inbound payments.
Yeah, those are fair points.
In this approach, would we provide an We could potentially not serialize those events, FWIW. Is it probable that if the event hasn't been processed yet then the onion message may not have been sent either (and therefore would be dropped on restart)?
Gotcha. It does seem like a lot of data to place in each blinded payment path, potentially blowing up the onion message beyond its limit. Maybe a subset of fields would be sufficient for this and potentially other use cases? Seems like for async payments it is only applicable for the offline case where a static invoice would be used. Therefore, this wouldn't be helpful there. So I think this comes down to whether we want to support pending inbound payment tracking in LDK node (in which case we need the event) or only success/failure tracking by adding some data to the invoice's blinded paths such that it can be included in |
It may require some additional in-LDK tracking, not sure. Its certainly not my preferred approach. There could be two separate APIs depending on if state tracking is desired, but that complexifies the API somewhat. Ultimately if we're worried about the cost of any state as being too high then BOLT 11 is also problematic because the ldk-node API tracks state per invoice (I believe?).
I just assume not generate any events (we'd just track a payment as "pending" once the offer is generated, the same as we do for BOLT 11 today), but we could if we want. I'm not quite sure I understand the desire to map "invoice requested" to "pending" vs "offer generated". Certainly "invoice requested" is a better "payment is pending", but if you're a merchant expecting to receive payment, "offer is generated" is also "payment pending". More generally, if we're looking to keep a similar API for BOLT 11 and BOLT 12 (which I think we should!), "offer is generated" seems like the clear equivalent to "BOLT11 invoice generated".
True, but I'm just worried that we start generating a ton of these events and the "obvious" way to handle them is to store them and suddenly we have a big DoS problem.
Indeed, maybe we only store the payer_id and the payer_message? |
Would we store the data in
Will let @tnull comment, but I'm fine with taking this approach instead. Then if you do want to track multiple payments per offer, you still can only upon receiving the payment.
We'll need the offer's metadata, too, if we want to correlated the payment with an offer. But seems like this is all pretty straight-forward. |
No, I was assuming in ldk-node. I don't think LDK should become stateless around invoices at all. |
|
Sorry for the delay here, currently a bit busy due to the hackathon, hence also keeping it brief here. So just a few brief notes on the API here, will possibly add more eventually:
|
ISTM the only way we an realistically achieve this without substantial DoS risk is to re-encode these in the blinded path.
I think it depends a bit on the use-case for payment tracking. If the point of payment tracking is for an store that needs to track when a payment completes, the 1:N relationship doesn't matter, they can that its been paid > 0 times. For those who do care about how many times an offer has been paid, they don't care too much about whether an invoice has been paid, only that the offer they posted as their public donation address has received N payments for M sats (maybe with a list of payer notes?). In any case, all of these can be accomplished by tracking payments on an offer -> list of payments basis, I don't think they require tracking on an invoice level. IOW we could do tracking of offers issued in LDK Node and re-encode payer information in the blinded path + PaymentClaimable event and I think it would suffice for all the use-cases I can come up with?
Do you mean the payment preimage? There is no payment secret anymore with BOLT 12. We should definitely give the payer the invoice we're paying, IMO - unlike the DoS issues with an event when issuing an invoice, I don't think there's any providing the invoice when paying it (its always one extra event per user action). |
If LDK Node stores the offer upon creation keyed by its metadata (let's call it an offer id), then for a given payment we can look up the offer from storage and also store the payment with offer id as part of a hierarchical key. Then we just need fields from the invoice request. We should limit which of these we store in the invoice's blinded payment paths for size constraints. Otherwise, storing, say, a long payer note or large payer metadata in each of the invoice's blinded payment paths would prevent constructing an onion message for the invoice, IIUC. |
I disagree. If a coffee shop generates an offer say, e.g., for each specific type of coffee they sell, I might pay this single offer every morning. They payee needs to be able to a) track each individual payment with a unique payment ID and b) know which invoice the payment was for and c) retrieve any data fields associated with the offer or invoice such as description (stored in the offer), payer note (sent via the invoice request, mirrored in the invoice), etc.
Sure, due to the 1:N relationship of offer:payment LDK Node will likely need to store offers in a separate structure and only associate payments by some kind of unique offer id. However, even if we assume payees store the generated offers, we need to expose all relevant fields from invoices/invoice requests and data that would allow us to associate payments with offers in
Yes, also the preimage, although we get that at least as part of
Ugh, hum, good point, but what value is then returned on the receiver side in |
If we think this is the only way we allow users to associate offers, we should commit to it by creating an Also, would using |
I think in practice you wouldn't see this type of use. Instead, you'd generate an offer per customer order. Otherwise, if the customer wants to purchase two different coffees (or a coffee and a pastry), they would need to pay two invoices. Seems this would get impractical as the number of items increase. Somewhat related, there was some discussion on the spec PR around quantity being more of an application layer concern and can't really represent the myriad possibilities (lightning/bolts#798 (comment)). But not much came of it. I seem to recall Rusty once saying there could be an extension to the BOLT to represent multiple items. |
IIUC, it's not directly in the invoice, but we include one in the blinded paths so we can use it for stateless inbound payments in the same way we do for BOLT 11. |
Hm, I mean it could be used this way though and even if it's only happening rarely we still need to support it?
Yes, I agree that the quantity field seems out-of-place. Just earlier today a user was confused by it, i.e., was wondering if it refers to a technical limit how often an offer could be paid, even though it's purely informational. |
Hmm... we could possibly do this. To make it all compile-time checkable, we'd probably want the
For the |
I don't think we need to support particular uses that aren't well-supported by BOLT12 - we should build an interface for users that makes sense and nudges them in a sensible direction :). But, generally, I think this is supported by the separate-payment-offer tracking I suggested above. If we track the issued offers and, for each offer, each payment that has completed for it, the user can always just query "hey, for this offer, tell me the most recent payment(s)". We can also still have a semi-unified BOLT11/12 API (eventually), with BOLT11 just always having at max one payment, and the "has this been paid" query working the same across 11/12. |
I generally agree, but I assume paying an offer multiple times isn't that unusual? Also, even if we didn't support it we need to at least make sure that we don't do something entirely buggy when users do it via other implementations, and allowing for collisions on payment ID could lead to unexpected behavior.
Yes, I agree, if we expose the offer/invoice fields and offer ID in |
In this case I was referring to, specifically, the idea that a merchant would have a few offers for specific items on the menu and customers would pay by attaching their name to the offer's payer_message. Rather, I think we should always assume (and design our API as if) multi-payment offers are used for donations, and single-payment offers for more traditional merchant flows. |
Okay, I think we're all on the same page - we should copy the offer/invoice_request/invoice fields that are relevant into the blinded path we include in the invoice and pipe them through to the payment events. |
|
Closed in favor of #2970. |
Bolt12Invoices are generated internally byChannelManager, so the payment recipient never sees apayment_hashuntil aPaymentClaimableis handled. This prevents tracking any pending inbound payments. Expose theBolt12Invoicein a newInvoiceGeneratedevent when responding to anInvoiceRequestfor anOffer. Similarly, return the invoice fromChannelManager::request_refund_payment.For the
Offercase, theoffer_metadatareturned byBolt12Invoice::metadatamay be used as an offer id, if desired, since it is a randomly generated 16-byte nonce forChannelManager-originating offers. Also, a commonBolt12Invoice::payer_idmay indicated redundant invoice requests.For the
Refundcase, the metadata will be empty, but thepayment_hashmay be used as an identifier.