-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BOLT 12 Invoice payments #2371
BOLT 12 Invoice payments #2371
Conversation
lightning/src/ln/channelmanager.rs
Outdated
} | ||
}, | ||
OffersMessage::InvoiceError(invoice_error) => { | ||
log_error!(self.logger, "Received invoice_error: {}", invoice_error); |
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.
Is there some way to rate-limit and authenticate these? Logging at the ERROR level any time someone sends us a specific onion message is a great recipe for DoS by log-filling (at some point pre-0.1 I want to define a clear "above this log level we believe we are not DoS-able", but we should think about it as we go today too).
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 authentication, according to the spec (BOLT 4):
MUST ignore the message if the
path_id
does not match the blinded route it created
I don't believe we are doing that now, but I imagine we'd want to do so at a higher level, if possible. Likewise for rate limiting. For authentication, though, the path_id
may be specific to the use case (e.g., offers vs some other onion message payload). Any thoughts on how we'd want to structure this?
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.
If we do tracking of pending-outbound-payments, we could use that to rate-limit this (and generate a PaymentFailed event).
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.
If we do tracking of pending-outbound-payments, we could use that to rate-limit this
Right, we should definitely generate an event. We'll have to tie an invoice_error
to an invoice_request
, though. We could use the path_id
in the request's reply_path
, IIUC. But we'd need to pipe that into the handler.
(and generate a PaymentFailed event).
We'll need to make the PaymentHash
an Option
, though, since we won't have one until we receive an invoice.
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.
We now have this, should we pass this error through to outbound_payments
? Also, let's not allow someone to DoS our log size at level ERROR.
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.
Can we correlate it with an outbound payment? It might not even be for one (i.e., if it is in response to an invoice message).
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.
Ah, I neglected to re-read the full context here. So, I think this requires having the path_id
in the onion message's reply path (for the InvoiceRequest
) contain the payment id. We'd also need to pipe this through to OffersMessageHandler::handle_message
.
Not sure if you prefer to go down this path at the moment. I have it using log_trace
for now. In practice, we'd would generate an InvoiceRequestFailed
immediately instead of waiting the constant three timer ticks for the request to timeout.
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.
Yea, I don't think this is worth worrying about right now.
cfc1975
to
3a16e62
Compare
@TheBlueMatt @valentinewallace Regarding timing out payments, what criteria should be used when awaiting an invoice? I see we remove (I'd imagine in the future we may want to retry the request, but for now we can simply surface a |
Could we split out the signing/verification changes into a prefactor PR? Seems logically separate |
ISTM a similar approach would be good here, with a new const for timing out our outbound invoice requests. Does that make sense to you here? |
Yeah, the PR is starting to grow, too, so this makes sense. |
About to push some WIP commits that get us there, though I still need to address the |
3a16e62
to
c181ec4
Compare
c181ec4
to
1c6937c
Compare
Now in #2432. |
1c6937c
to
04efc9a
Compare
Rebased and pushed latest work. This includes addressing #2371 (comment) by including the Might be worth breaking out everything up to and including that into a separate PR, leaving the code for properly handling invoice payments here. Note that #2039 includes the tracking of |
Codecov ReportPatch coverage:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2371 +/- ##
==========================================
+ Coverage 90.61% 90.63% +0.01%
==========================================
Files 110 110
Lines 57785 58199 +414
Branches 57785 58199 +414
==========================================
+ Hits 52361 52747 +386
- Misses 5424 5452 +28
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. |
I'd be in favor of this plan, would be nice to break this up as much as possible |
04efc9a
to
82814bc
Compare
Took the relevant commits and opened #2468 |
1da3a66
to
9379a38
Compare
Needs rebase 🎉 |
9379a38
to
55e2056
Compare
Rebased and ready for review. The most notable change is that there are two new Alternatively, we could refactor |
lightning/src/ln/channelmanager.rs
Outdated
return Some(OffersMessage::InvoiceError(error.into())); | ||
}, | ||
}; | ||
let relative_expiry = DEFAULT_RELATIVE_EXPIRY.as_secs() as u32; |
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.
Should we be getting an expiry from the offer?
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.
The offer has an absolute expiry, but the invoice expiry is relative to invoice's creation time. Not clear to me if we issue an invoice a few seconds before an offer expires, whether we should have an identical short expiration for the invoice. Seems like we should honor the offer if we issued an invoice, even if the offer expires before the invoice could be paid.
On the other hand, there's really no good way to allow the user to configure the expiry other than inferring it from the offer.
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.
Hmm, yea, that makes sense, but I don't think we want to set the expiry to two hours, instead like...30 seconds?
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.
Yeah, possibly something shorter, but two things to consider:
- For
no-std
, we usehighest_seen_timestamp
as the creation time. But the node we send the invoice to may have a much more recent time, leading them to think the invoice is already expired. - When creating the payment secret, we use
highest_seen_timestamp + invoice_expiry_delta_secs + 7200
. But forstd
,highest_seen_timestamp
won't necessarily match the invoice'screated_at
time. But maybe the two-hour grace period makes this a non-issue?
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 no-std, we use highest_seen_timestamp as the creation time. But the node we send the invoice to may have a much more recent time, leading them to think the invoice is already expired.
Mmm, good point, nevermind.
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.
We should definitely document this, however - we already document elsewhere (IIRC) that we will accept payment for invoices which expired a few blocks ago, but we should also document it on the offer objects.
lightning/src/ln/channelmanager.rs
Outdated
let builder = invoice_request.respond_using_derived_keys_no_std( | ||
payment_paths, payment_hash, created_at | ||
); | ||
match builder.and_then(|b| b.allow_mpp().build_and_sign(secp_ctx)) { |
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.
How are we going to handle phantom offers/invoices?
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.
Hmm... possibly with InvoiceRequest
-level retries by the payer? The spec hasn't really spelled out whether a request should be retried on a failed payment. I know we spoke about it but never came to any conclusion.
For this case, though, where the ExpandedKey
is used for deriving the signing keys, shouldn't all replicas have the same ExpandedKey
and thus this won't be a problem? It's really only an issue in the case below where we need to delegate to the NodeSigner
.
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.
he spec hasn't really spelled out whether a request should be retried on a failed payment. I know we spoke about it but never came to any conclusion.
I was thinking the way we handle requests on our end is we always just send the invoice_request three times across three separate paths, using that as "free online-ness probing" and then don't bother retrying.
For this case, though, where the ExpandedKey is used for deriving the signing keys, shouldn't all replicas have the same ExpandedKey and thus this won't be a problem? It's really only an issue in the case below where we need to delegate to the NodeSigner.
Mmm, yea, I guess we need to pass the expected pubkey to sign_bolt12_invoice
cause presumably cash/etc will want to use a static well-known invoice signing key (or maybe not?).
But, the bigger issue, is the TODO: Include payment_secret in payment_paths.
(and generally the payment path building at all - that has to include knowledge of the other phantom nodes in the payment paths.
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.
Mmm, yea, I guess we need to pass the expected pubkey to
sign_bolt12_invoice
cause presumably cash/etc will want to use a static well-known invoice signing key (or maybe not?).But, the bigger issue, is the
TODO: Include payment_secret in payment_paths.
(and generally the payment path building at all - that has to include knowledge of the other phantom nodes in the payment paths.
Is phantom at the invoice-level necessary if we can rely on liveliness?
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.
Is phantom at the invoice-level necessary if we can rely on liveliness?
Yea, that's a good question, I don't know. There's two cases where it ay:
- If you want to do phantom across two "genuinely different" nodes, then maybe - ie if you have two nodes with differnet peers, channels, and liquidity then it may be that the sender can only in practice pay one of the two nodes, so we want retries to cross them (eg River does this, though I dunno how they do invoice generation, whether they just use one node or not).
- If you want the original promise of "restart-safe payment receipts", this kinda helps? I mean its never fully restart-safe in that if we have an HTLC that came in in an update_add_htlc + commitment_signed right before restart we still block the payment, but at least that's a smaller amount of time than a full sender-roundtrip for an onion message.
Do these two cases matter enough to build some kind of route-hint-caching struct to make it work on BOLT12, I dunno, but its not like its all that hard.
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.
Do these two cases matter enough to build some kind of route-hint-caching struct to make it work on BOLT12, I dunno, but its not like its all that hard.
Isn't this exactly what we do by having the payment paths in Payee::Blinded
, which is stored indirectly in PendingOutboundPayment::Retryable
? Or am I misunderstanding?
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.
Right, but somehow the ChannelManager
actually generating the Invoice
needs to know about the route hints for any other ChannelManager
s which are trying to participate in the payment receipt.
This PR is generally missing the payment_path generation for Invoice
construction, so its all TBD right now anyway, just also means we can't get paid with the current code yet :p
lightning/src/ln/channelmanager.rs
Outdated
} | ||
}, | ||
OffersMessage::InvoiceError(invoice_error) => { | ||
log_error!(self.logger, "Received invoice_error: {}", invoice_error); |
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.
We now have this, should we pass this error through to outbound_payments
? Also, let's not allow someone to DoS our log size at level ERROR.
lightning/src/ln/outbound_payment.rs
Outdated
@@ -538,6 +570,8 @@ pub(super) struct SendAlongPathArgs<'a> { | |||
pub session_priv_bytes: [u8; 32], | |||
} | |||
|
|||
const BOLT_12_INVOICE_RETRY_STRATEGY: Retry = Retry::Attempts(3); |
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.
We'll probably want to make this configurable, right? just not necessarily right now.
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.
Yeah, somehow it didn't occur to me that we can just put the Retry
strategy in the new variants. We'll have to serialize it though. I assume we can't safely serialize usize
as eight bytes since theoretically someone can give a Retry::Attempts
with it greater than u32::MAX on a 64-bit machine, which can't be read on a 32-bit machine. Or should we just consider that undefined behavior?
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 guess my question really is should we change Retry::Attempts
to wrap u32
instead of usize
?
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.
Yea, swapping for a u32 makes sense to me.
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.
Added to this PR but can move it to the next one if you prefer.
lightning/src/ln/outbound_payment.rs
Outdated
final_value_msat: invoice.amount_msats(), | ||
}; | ||
|
||
self.retry_payment_internal( |
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.
Could we rename retry_payment_internal
?
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.
Renamed but will updated the logging verbiage after #2314 lands.
lightning/src/ln/channelmanager.rs
Outdated
@@ -1341,9 +1341,13 @@ const CHECK_CLTV_EXPIRY_SANITY_2: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_G | |||
/// The number of ticks of [`ChannelManager::timer_tick_occurred`] until expiry of incomplete MPPs | |||
pub(crate) const MPP_TIMEOUT_TICKS: u8 = 3; | |||
|
|||
/// The number of ticks of [`ChannelManager::timer_tick_occurred`] until an invoice request without | |||
/// a response is timed out. | |||
pub(crate) const INVOICE_REQUEST_TIMEOUT_TICKS: u8 = 3; |
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.
This is only used in outbound_payment
so it could live there
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.
Moved, as well as IDEMPOTENCY_TIMEOUT_TICKS
.
@@ -158,6 +176,12 @@ impl PendingOutboundPayment { | |||
payment_hash: *payment_hash, | |||
reason: Some(reason) | |||
}; | |||
} else if let PendingOutboundPayment::InvoiceReceived { payment_hash, .. } = self { |
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.
Re: ChannelManager::abandon_payment
-- it currently doesn't have an effect on payments where we're AwaitingInvoice
. Would it be possible to make the payment_hash
optional in PendingOutboundPayment::Abandoned
and generate an InvoiceRequestFailed
when the user calls abandon_payment
?
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 don't think we even need to store it as Abandoned
- we can just remove it immediately and generate the failure event.
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.
Missed this but working on it now. Should we add a reason to InvoiceRequestFailed
like PaymentFailed
now that it may not simply be because of a timeout?
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.
Note that it is a little weird as ChannelManager::abandon_payment
would call OutboundPayments::abandon_payment
with PaymentFailureReason::UserAbandoned
. We would either ignore the reason for one specific to InvoiceRequestFailed
or reuse it, even though all the variants are not possible and would require another variant for timeout. For now, I've left a reason out.
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.
Should we add a reason to InvoiceRequestFailed like PaymentFailed now that it may not simply be because of a timeout?
That seems reasonable but seems fine to leave for follow-up too
0f1bc13
to
4fc04ba
Compare
Looks like I'll need to rebase to get CI to pass now that #2555 has landed. |
@TheBlueMatt I've rebased locally. Will push on your go-ahead. |
Go ahead IMO |
4fc04ba
to
4ced077
Compare
lightning/src/offers/test_utils.rs
Outdated
pub(super) fn pubkey(byte: u8) -> PublicKey { | ||
pub(crate) fn pubkey(byte: u8) -> PublicKey { | ||
let secp_ctx = Secp256k1::new(); | ||
PublicKey::from_secret_key(&secp_ctx, &privkey(byte)) | ||
} | ||
|
||
pub(super) fn privkey(byte: u8) -> SecretKey { | ||
pub(crate) fn privkey(byte: u8) -> SecretKey { | ||
SecretKey::from_slice(&[byte; 32]).unwrap() | ||
} |
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.
These were added to the regular util::test_utils
module, could use those methods instead
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.
privkey
and pubkey
aren't used in the added tests, so I reverted them to be pub(super)
.
@@ -797,7 +797,7 @@ impl OutboundPayments { | |||
Ok(()) | |||
} | |||
|
|||
fn retry_payment_internal<R: Deref, NS: Deref, ES: Deref, IH, SP, L: Deref>( | |||
fn find_route_and_send_payment<R: Deref, NS: Deref, ES: Deref, IH, SP, L: Deref>( |
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.
Kinda sucks to have send_payment_internal
and find_route_and_send_payment
since they both find a route and send a payment... maybe not worth solving now, though.
When an invoice is requested but either receives an error or never receives a response, surface an event to indicate to the user that the corresponding future payment has failed.
4ced077
to
db1c83a
Compare
When a BOLT 12 invoice has been requested, in order to guarantee invoice payment idempotency the future payment needs to be tracked. Add an AwaitingInvoice variant to PendingOutboundPayment such that only requested invoices are paid only once. Timeout after a few timer ticks if a request has not been received.
When a BOLT 12 invoice has been received, a payment attempt is made and any errors result in abandoning the PendingOutboundPayment. This results in generating at PaymentFailed event, which has a PaymentHash. Thus, when receiving an invoice, transition from AwaitingInvoice to a new InvoiceReceived state, the latter of which contains a PaymentHash such the abandon_payment helper can still be used.
Consolidate the creation and insertion of onion_session_privs to the PendingOutboundPayment::Retryable arm. In an upcoming commit, this method will be reused for an initial BOLT 12 invoice payment. However, onion_session_privs are created using a different helper.
It will be used for initial attempts at paying BOLT 12 invoices, so rename it something that covers both that and retries.
Add a send_payment_for_bolt12_invoice method to OutboundPayments for initiating payment of a BOLT 12 invoice. This will be called from an OffersMessageHandler, after which any retries are handled using the Retryable logic.
The test utilities for Offers are needed for testing message handling in ChannelManager and OutboundPayments.
An upcoming commit requires serializing Retry, so use a type with a fixed byte length. Otherwise, using eight bytes to serialize a usize would fail to read on 32-bit machines.
Replace a constant three retry attempts for BOLT 12 invoice payments with a retry strategy specified when creating a pending outbound payment. This is configured by users in a later commit when constructing an InvoiceRequest or a Refund.
db1c83a
to
44b9c54
Compare
Add support to
OutboundPayments
for paying aBolt12Invoice
. This involves:PaymentId
asPendingOutboundPayment::AwaitingInvoice
PendingOutboundPayment::InvoiceReceived
and thenPendingOutboundPayment::Retryable
such that logic inOutboundPayments::retry_payment_internal
can be reused.