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 Offers message flow #2039
Conversation
07a2742
to
6ab6525
Compare
840d4a5
to
f167b0b
Compare
Mind rebasing this? 🙏 |
f167b0b
to
7f67ee5
Compare
Rebased and included recent changes from #1989. |
7f67ee5
to
ebc0436
Compare
ebc0436
to
45181c9
Compare
45181c9
to
e3fec6e
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2039 +/- ##
==========================================
- Coverage 88.93% 88.76% -0.17%
==========================================
Files 112 112
Lines 87850 88258 +408
Branches 87850 88258 +408
==========================================
+ Hits 78129 78346 +217
- Misses 7473 7667 +194
+ Partials 2248 2245 -3
☔ View full report in Codecov by Sentry. |
e3fec6e
to
f962dfb
Compare
f962dfb
to
05d2755
Compare
acb28b9
to
082f6e9
Compare
082f6e9
to
736fe4a
Compare
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.
Needs rebase now 🎉.
Sadly, I think we should cfg-gate (or non-pub) the new pub methods here just in case at least one-hop blinded path receive doesn't manage to land.
736fe4a
to
37c2492
Compare
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.
Rebased
4aa8fa1
to
3395bce
Compare
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 a commit to use timer-ticks for InvoiceRequest
. Addressed most comments, but need to address a few stray ones still, though mostly for docs.
@@ -7203,6 +7203,109 @@ where | |||
Ok(builder) | |||
} | |||
|
|||
/// Pays for an [`Offer`] using the given parameters by creating an [`InvoiceRequest`] and |
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 believe this is already described below under Privacy
, at least from the reply path perspective. But for requesting the invoice, there needs to be a direct connection to a introduction node in the offer, though not necessarily to a public node, IIUC.
lightning/src/ln/channelmanager.rs
Outdated
.duration_since(std::time::SystemTime::UNIX_EPOCH) | ||
.expect("SystemTime::now() should come after SystemTime::UNIX_EPOCH"); | ||
#[cfg(not(feature = "std"))] | ||
let duration_since_epoch = Duration::from_secs( |
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.
Discussed offline and added a timer-tick based approach for short-lived InvoiceRequests
. Could do something similar for short-lived refunds, but left it as exclusively an absolute expiry for now.
lightning/src/ln/channelmanager.rs
Outdated
let payee_tlvs = ReceiveTlvs { | ||
payment_secret, | ||
payment_constraints: PaymentConstraints { | ||
max_cltv_expiry: LATENCY_GRACE_PERIOD_BLOCKS, |
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... I'm not very educated on this, though I think the value came from an offline discussion between us and @TheBlueMatt. Also, note this is where we are the introduction node.
Oh, also had to trivially update |
9658805
to
a06cea3
Compare
Looks like this needs rebase now. |
Upcoming commits will add utilities for sending an InvoiceRequest for an Offer and an Invoice for a Refund. These messages need to be enqueued so that they can be released in ChannelManager's implementation of OffersMessageHandler to OnionMessenger for sending. These messages do not need to be serialized as they must be resent upon restart.
Pending outbound payments use an absolute expiry to determine when they are considered stale and should be fail. In `no-std`, this may result in long timeouts as the highest seen block time is used. Instead, allow for expiration based on timer ticks. This will be use in an upcoming commit for invoice request expiration.
Add a utility to ChannelManager for sending an InvoiceRequest for an Offer such that derived keys are used for the payer id. This allows for stateless verification of any Invoice messages before it is paid. Also tracks future payments using the given PaymentId such that the corresponding Invoice is paid only once.
Add a utility to ChannelManager for creating a Bolt12Invoice for a Refund such that the ChannelManager can recognize the PaymentHash and reconstruct the PaymentPreimage from the PaymentSecret, the latter of which is contained in a BlindedPath within the invoice.
Building an invoice will fail if the underlying offer or refund has already expired. The check was skipped in no-std since there is no system clock. However, the invoice creation time can be used instead. This prevents responding to an invoice request if the offer has already expired.
Define the BOLT 12 message flow in ChannelManager's OffersMessageHandler implementation. - An invoice_request message results in responding with an invoice message if it can be verified that the request is for a valid offer. - An invoice is paid if it can be verified to have originated from a sent invoice_request or a refund. - An invoice_error is sent in some failure cases. - Initial messages enqueued for sending are released to OnionMessenger
This reverts commit c7219e4.
DefaultMessageRouter always fails. Update it so that it can route to a directly connected peer. This is needed for an Offers minimum viable product.
a06cea3
to
fe90448
Compare
Rebased |
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.
In a followup, I'd like to see the default max_total_routing_fee_msat
in send_payment_for_bolt12_invoice
not be None
, but the default set in RouteParameters::from_payment_params_and_value
. Users should have to opt out of a fee ceiling (by setting a max of infinite) rather than opt-in to one (by setting one), I think. We can also drop the allow(unused)
on send_payment_for_bolt12_invoice
.
A few other nits here but nothing worth fixing in this PR.
let route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat); | ||
let route = get_route( | ||
&nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph.read_only(), None, | ||
nodes[0].logger, &scorer, &(), &random_seed_bytes |
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.
nit: Please leave the Default::default
in place.
@@ -48,6 +48,16 @@ pub struct ErroneousField { | |||
pub suggested_value: Option<Vec<u8>>, | |||
} | |||
|
|||
impl InvoiceError { | |||
/// Creates an [`InvoiceError`] with the given message. | |||
pub fn from_str(s: &str) -> 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.
Should just take the String
- in one call we format!()
create a String
, then pass by reference here, and end up cloning it.
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.
FWIW, I have a basic functional test for the invreq flow passing 🎉
Define the BOLT 12 message flow in
ChannelManager
'sOffersMessageHandler
implementation.invoice_request
message results in responding with aninvoice
message if it can be verified that the request is for a valid offer.invoice
is paid if it can be verified to have originated from a sentinvoice_request
or arefund
.invoice_error
is sent in some failure cases.Further, adds a utility for constructing a
Bolt12Invoice
for aRefund
that can be used withChannelManager
.Based on #2578