Add BOLT12 support to LSPS2 via custom Router implementation#4463
Add BOLT12 support to LSPS2 via custom Router implementation#4463tnull wants to merge 14 commits intolightningdevkit:mainfrom
Router implementation#4463Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
2cb0546 to
25ab3bc
Compare
| &self, payment_context: &PaymentContext, | ||
| ) -> Option<LSPS2Bolt12InvoiceParameters> { | ||
| // We intentionally only match `Bolt12Offer` here and not `AsyncBolt12Offer`, as LSPS2 | ||
| // JIT channels are not applicable to async (always-online) BOLT12 offer flows. |
There was a problem hiding this comment.
I don't think this is true? We need to support JIT opening for async offers as well.
There was a problem hiding this comment.
Yes, should have formulated that better, but IMO that is a next/follow-up step somewhat orthogonal to this PR?
There was a problem hiding this comment.
We can do it in a separate PR indeed, but I'm not really sure LSPS2 support for BOLT12 only for always-online nodes is nearly as useful has for async recipients. ISTM the second part is the more important usecase.
There was a problem hiding this comment.
The big difference is that there are other LSPS2 (client and service) implementations out there that LSPs are running, while async payments isn't deployed at all yet, and will require both sides to be LDK for the time being.
There was a problem hiding this comment.
I mean that's fair but are there other LSPS servers that support intercepting blinded paths and doing a JIT channel? I imagine we'll in practice require LDK for both ends for that as well.
There was a problem hiding this comment.
In any case my point is that both sides are a similar priority, not that they have to happen in one PR.
There was a problem hiding this comment.
Explored this further, but it seems there might be a conflict between approaches here:
No
receive_async_via_jit_channel()API —receive_async()creates offers viaChannelManager::get_async_receive_offer()which bypasses the LSPS2 router entirely. The static invoice's payment paths don't use the intercept SCID.
So to add BOLT12-async-payments-via-LSPS2-JIT support we might need to reconsider how we could inject the respective data into the blinded paths. Not sure if @valentinewallace would have an opinion here.
Also, to quote Claude:
The simplest approach: the LSPS2 buy dance happens on the client side, before the static invoice is created. The client:
- Calls an LSPS2 buy request to get intercept_scid + cltv_expiry_delta
- Calls router.register_offer_nonce(offer_nonce, params)
- Then triggers the static invoice creation flow
Since the LSPS2BOLT12Router is both the payment router and the message router, when create_static_invoice_for_server() calls router.create_blinded_payment_paths() with AsyncBolt12OfferContext { offer_nonce }, the router finds the registered nonce and injects the intercept SCID.
But there's a problem: the static invoice is created on the server side (LSP), not the client side. The server calls create_static_invoice_for_server() which calls its own router. The client's router registration is irrelevant — it's the server's router that builds the payment paths.
So either:
- (A) The server (LSP) needs to know about the LSPS2 intercept SCID for this client and register it on its own router before creating the static invoice. This means the LSPS2 buy flow needs to complete before static invoice creation, and the server must register the result on its router.
- (B) The client creates the static invoice itself (not the server), but that's not how async payments work.
- (C) Add a callback/hook in create_static_invoice_for_server() that lets the server inject custom payment paths.
Option (A) seems most natural: the LSP (as LSPS2 service) already knows about the client's intercept SCIDs. When the server creates the static invoice for a client, it could register the intercept SCID on its router so the payment paths go through the JIT channel. But this requires the LSP
to proactively register offer nonces for each client's async offers.
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
| pub struct LSPS2Bolt12InvoiceParameters { | ||
| /// The LSP node id to use as the blinded path introduction node. | ||
| pub counterparty_node_id: PublicKey, | ||
| /// The LSPS2 intercept short channel id. | ||
| pub intercept_scid: u64, | ||
| /// The CLTV expiry delta the LSP requires for forwarding over `intercept_scid`. | ||
| pub cltv_expiry_delta: u32, | ||
| } |
There was a problem hiding this comment.
Would it be too expensive to store this in the Offer's blinded path? Though I suppose the Router doesn't have access to that, so we'd have to provide it the MessageContext.
There was a problem hiding this comment.
I imagine it would be. Adding yet another 45 bytes might be a bit much w.r.t. to QR encoding?
There was a problem hiding this comment.
Right, that would be and additional 72 bytes more when encoded as bech32.
Maybe a compact representation (SCID and direction) could be used similar to what we do in blinded paths? That would use 9 bytes instead of 33 for the pubkey, so 21 bytes instead of 45. Encoded that would be 33/34 more bytes instead of 72.
There was a problem hiding this comment.
Could you expand on what exactly you imagine we store? And is this mostly around not requiring the client to remember anything outside the offer locally?
There was a problem hiding this comment.
Yeah, I was thinking we wouldn't need to make a custom Router or use any additional storage for Offer registration. Instead, it would be something like:
- Include
LSPS2Bolt12InvoiceParametersin anOffer's blinded paths usingMessageContextwhen building theOffer. - When
InvoiceRequestis received, extract the parameters from theMessageContextif set. - Use them to determine how to build the
BlindedPaymentPaths.
(Alternatively, given the InvoiceRequest contains the Offer's message paths, if the LSP is the introduction node, we can use that directly instead of storing it in the MessageContext. Then, we'd just need the intercept_scid and cltv_expiry_delta as additional data.)
For the last step, we could either (a) bypass the Router entirely and directly build the BlindedPaymentPath from the parameters, (b) pass Option<MessageContext> to Router and implement DefaultRouter to recognize it, or (c) something similar but with a different interface (e.g., passing Option<LSPS2Bolt12InvoiceParameters> instead).
We could use the IntroductionNode::DirectedShortChannelId directly in the BlindedPaymentPath, too, or look it up and use IntroductionNode::NodeId. I believe we currently support routing over the former but don't yet support creating them unlike for BlindedMessagePath.
25ab3bc to
5786409
Compare
|
The tests look well-structured. I notice the compact message path test ( Now I've completed my thorough review. Let me post any NEW inline comments and the summary. Based on my analysis: Issues already posted in prior reviews that are still valid:
Issues from prior reviews that are now resolved:
New issues found in this review: Since all my prior inline comments about the current code issues are still posted and valid, I should not re-post them. My instructions say "Do NOT repeat these." Review SummaryNo new issues found in this review pass. All previously identified issues that are still valid have been posted as inline comments in prior passes. Previously posted issues (still applicable)
Previously posted issues (now resolved in current code)
Cross-cutting observations
|
5786409 to
98a9e9d
Compare
8800d48 to
7ca886d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4463 +/- ##
==========================================
- Coverage 86.20% 86.17% -0.03%
==========================================
Files 160 161 +1
Lines 107545 108760 +1215
Branches 107545 108760 +1215
==========================================
+ Hits 92707 93726 +1019
- Misses 12214 12399 +185
- Partials 2624 2635 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7ca886d to
2ff16d7
Compare
bcc4e10 to
5602e07
Compare
ea05389 to
3acf915
Compare
jkczyz
left a comment
There was a problem hiding this comment.
Ok, this could be a gap in my knowledge, but as mentioned in some of the comments, why do we need onion message interception for this? The rationale doesn't appear to be clearly spelled out anywhere.
| /// The LSPS2 intercept short channel id. | ||
| pub intercept_scid: u64, | ||
| /// The CLTV expiry delta the LSP requires for forwarding over `intercept_scid`. | ||
| pub cltv_expiry_delta: u32, |
| pub struct LSPS2Bolt12InvoiceParameters { | ||
| /// The LSP node id to use as the blinded path introduction node. | ||
| pub counterparty_node_id: PublicKey, | ||
| /// The LSPS2 intercept short channel id. | ||
| pub intercept_scid: u64, | ||
| /// The CLTV expiry delta the LSP requires for forwarding over `intercept_scid`. | ||
| pub cltv_expiry_delta: u32, | ||
| } |
There was a problem hiding this comment.
Yeah, I was thinking we wouldn't need to make a custom Router or use any additional storage for Offer registration. Instead, it would be something like:
- Include
LSPS2Bolt12InvoiceParametersin anOffer's blinded paths usingMessageContextwhen building theOffer. - When
InvoiceRequestis received, extract the parameters from theMessageContextif set. - Use them to determine how to build the
BlindedPaymentPaths.
(Alternatively, given the InvoiceRequest contains the Offer's message paths, if the LSP is the introduction node, we can use that directly instead of storing it in the MessageContext. Then, we'd just need the intercept_scid and cltv_expiry_delta as additional data.)
For the last step, we could either (a) bypass the Router entirely and directly build the BlindedPaymentPath from the parameters, (b) pass Option<MessageContext> to Router and implement DefaultRouter to recognize it, or (c) something similar but with a different interface (e.g., passing Option<LSPS2Bolt12InvoiceParameters> instead).
We could use the IntroductionNode::DirectedShortChannelId directly in the BlindedPaymentPath, too, or look it up and use IntroductionNode::NodeId. I believe we currently support routing over the former but don't yet support creating them unlike for BlindedMessagePath.
| /// A router wrapper that injects LSPS2-specific BOLT12 blinded paths for registered offer ids | ||
| /// while delegating all other routing behavior to the inner routers. |
There was a problem hiding this comment.
Should be more specific that this is for blinded path creation operations. Path finding is not affected.
| /// For **payment** blinded paths (in invoices), it injects the intercept SCID as the forwarding | ||
| /// hop so that the LSP can intercept the HTLC and open a JIT channel. |
There was a problem hiding this comment.
"injects" is probably not the right terminology. IIUC, it bypasses the wrapped router entirely, unlike the one for message paths.
| /// | ||
| /// [`OnionMessageInterceptor::register_scid_for_interception`]: lightning::onion_message::messenger::OnionMessageInterceptor::register_scid_for_interception | ||
| pub struct LSPS2BOLT12Router<R: Router, MR: MessageRouter, ES: EntropySource + Send + Sync> { | ||
| inner_router: R, |
There was a problem hiding this comment.
Wrapping NullMessageRouter essentially makes the wrapper a no-opt. That's fine since we can't support the "no blinded path" use case, but may not be obvious.
| } | ||
| peer | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
No need to re-allocate. We can modify in-place instead.
If the peer is offline and we receive an invoice-request, we need to be able to use LSPS5 to send a notification to the client and wake them up to get them to respond. |
e7047c5 to
8a2e30a
Compare
We extend the `OnionMessenger` capabilities to also intercept onion messages if they are for unknown SCIDs. Co-Authored-By: HAL 9000
88e98ae to
ade41b1
Compare
Add `intercept_unknown_scid_oms` test that verifies the `OnionMessenger` correctly generates `OnionMessageIntercepted` events with a `ShortChannelId` next hop when a blinded path uses an unresolvable SCID. This complements the existing `intercept_offline_peer_oms` test which only covers the `NodeId` variant (offline peer case). Co-Authored-By: HAL 9000
ade41b1 to
8d04200
Compare
Add backwards compatibility tests for `Event::OnionMessageIntercepted` serialization to verify that: - Events serialized by LDK 0.2 (with `peer_node_id` in TLV field 0) can be deserialized by the current version as `NextMessageHop::NodeId`. - Events with `NodeId` next hop serialized by the current version can be deserialized by LDK 0.2 (which reads `peer_node_id` from field 0). - Events with `ShortChannelId` next hop (which omit TLV field 0) correctly fail to deserialize in LDK 0.2, since the `peer_node_id` field is required there. Co-Authored-By: HAL 9000
Introduce `LSPS2BOLT12Router` to map registered offers to LSPS2 invoice parameters and build blinded payment paths through the negotiated intercept `SCID`. All other routing behavior still delegates to the wrapped router. Co-Authored-By: HAL 9000
8d04200 to
720e739
Compare
Signed-off-by: Elias Rohrer <dev@tnull.de>
Describe how `InvoiceParametersReady` feeds both the existing `BOLT11` route-hint flow and the new `LSPS2BOLT12Router` registration path for `BOLT12` offers. Co-Authored-By: HAL 9000
Exercise the LSPS2 buy flow and assert that a registered `OfferId` produces a blinded payment path whose first forwarding hop uses the negotiated intercept `SCID`. Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de>
Allow tests to inject a custom `create_blinded_payment_paths` hook while preserving the normal `ReceiveTlvs` bindings. This makes it possible to exercise LSPS2-specific `BOLT12` path construction in integration tests. Co-Authored-By: HAL 9000
Cover the full offer-payment flow from onion-message invoice exchange through HTLC interception, JIT channel opening, and settlement. This confirms the LSPS2 router and service handler work together in the integrated path. Co-Authored-By: HAL 9000
720e739 to
5d6e7c4
Compare
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
Closes #4272.
This is an alternative approach to #4394 which leverages a custom
Routerimplementation on the client side to inject the respective.LDK Node integration PR over at lightningdevkit/ldk-node#817