Enable Creation of Offers and Refunds Without Blinded Path #3246
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3246 +/- ##
==========================================
- Coverage 89.72% 89.69% -0.03%
==========================================
Files 130 131 +1
Lines 107882 108010 +128
Branches 107882 108010 +128
==========================================
+ Hits 96799 96884 +85
- Misses 8679 8716 +37
- Partials 2404 2410 +6 ☔ View full report in Codecov by Sentry. |
lightning/src/ln/offers_tests.rs
Outdated
| assert_eq!(refund.payer_id(), alice_id); | ||
| assert!(refund.paths().is_empty()); | ||
| } | ||
|
|
||
| /// Checks that blinded paths are compact for short-lived offers. | ||
| #[test] | ||
| fn creates_short_lived_offer() { |
There was a problem hiding this comment.
The naming at least for the following tests is no longer relevant, it seems.
There was a problem hiding this comment.
As far as I understand, with the introduction of BlindedPathParameters, it seems like the behavior these tests were checking isn't relevant anymore. Should we think about removing these tests?
There was a problem hiding this comment.
Let's wait on the resolution of #3246 (comment). We'll ultimately want to test cases allowed by whatever interface is exposed to the user.
shaavan
force-pushed
the
blinded_api
branch
from
7a18051 to
bc00884
Compare
|
Updated from pr3246.06 to pr3246.07 (diff): Changes:
|
shaavan
force-pushed
the
blinded_api
branch
from
bc00884 to
3bcc420
Compare
lightning/src/ln/channelmanager.rs
Outdated
| let context = MessageContext::Offers(context); | ||
| let path = $self | ||
| .create_blinded_paths(context) | ||
| .and_then(|paths| paths.into_iter().next().ok_or(())) |
There was a problem hiding this comment.
Hmm... @TheBlueMatt I don't think this interface is sufficient to allow someone to specify they want more than one path in the offer, unless we include all the paths returned by the MessageRouter instead of just one.
There was a problem hiding this comment.
Yea, IMO we should include all paths - if the router wants to do something funky in the offer, so be it.
There was a problem hiding this comment.
What should we have DefaultMessageRouter do?
There was a problem hiding this comment.
What we currently do (use the context of why we're asking for a path to decide how many paths to include)?
There was a problem hiding this comment.
I suppose DefaultMessageRouter could use both MessageContex and BlindedPathType -- or which MessageRouter method is called, if we revert to that state -- to determine if more than one path should be returned. That way we'd allow users to pass BlindedPathType::Full to create_offer_builder such that they can create an offer with more than one path (e.g., when an offer doesn't need to be in a QR code).
There was a problem hiding this comment.
Right, that makes sense but then I'm confused about your previous comment about keeping get interface the same as it is?
There was a problem hiding this comment.
If we change MessageRouter to only have one method, then we need to pass in BlindedPathType. But then the peers parameter will need to be Vec<MessageForwardNode> even when used with BlindedPathType::Full instead of Vec<PublicKey>. So callers could pass in a MessageForwardNode with an scid even though it isn't expected, resulting in the last hop being compact.
There was a problem hiding this comment.
So callers could pass in a MessageForwardNode with an scid even though it isn't expected, resulting in the last hop being compact.
Makes sense! I have reintroduced the two function flow back in pr3246.11
Also, I have updated the DefaultMessageRouter's create_compact_blinded_paths to only return a single path in pr3246.11
There was a problem hiding this comment.
I'm confused why this is a concern, if the MessageRouter insists on a specific type of blinded path in general, so what? Ideally the MessageRouter gets to pick what it wants to do, though I think we're not allowed to have a compact introduction point in reply paths? Even in that case, though, ISTM we can communicate the restrictions and if the MessageRouter is buggy we can just fail.
There was a problem hiding this comment.
I'm confused why this is a concern, if the
MessageRouterinsists on a specific type of blinded path in general, so what? Ideally theMessageRoutergets to pick what it wants to do,
Nothing is preventing a MessageRouter from doing so. It would just need access to the channels.
It's just kinda ugly for DefaultMessageRouter to need to clear short_channel_id depending on how it is called. Plus, ChannelManager needs to do additional lookups to get this information just to have it discarded. I'm fine either way, but it doesn't optimize the usual case (i.e., use non-compact paths for reply paths).
though I think we're not allowed to have a compact introduction point in reply paths? Even in that case, though, ISTM we can communicate the restrictions and if the
MessageRouteris buggy we can just fail.
Yeah, though it doesn't look like we are enforcing this at the moment.
|
A gentle ping! Would love to get your thoughts on this new approach, when you get a moment! |
To decouple offers and onion message-related code from `ChannelManager`, this commit introduces the `message_received` function in `Offer/OnionMessageHandler`. Currently, the function focuses on handling the retry logic for `InvoiceRequest` messages. Moving this responsibility ensures a cleaner separation of concerns and sets the foundation for managing offers/onion messages directly within the appropriate handler.
Since `ChannelMessageHandler`'s `message_received` function was solely used for handling invoice request retries. This function has been removed from `ChannelMessageHandler`, and the relevant code has been migrated to `OffersMessageHandler`'s `message_received`. This ensures invoice request retries are now handled in the appropriate context.
This commit temporarily removes support for async BOLT12 message handling to enable a smoother transition in the upcoming refactor. The current implementation of async handling is abrupt, as it requires delving into the synchronous case and generating an event mid-flow based on the `manual_handling` flag. This approach adds unnecessary complexity and coupling. A later commit will introduce a new struct, `OffersMessageFlow`, designed to handle and create offer messages. This new struct will support async handling in a more structured way by allowing users to implement a parameterized trait for asynchronous message handling. Removing the existing async support now ensures a cleaner and more seamless migration of offer-related code from `ChannelManager` to `OffersMessageFlow`.
This commit introduces a new struct, `OffersMessageFlow`, to extract all offers message-related code out of `ChannelManager`. By moving this logic into a dedicated struct, it creates a foundation for separating responsibilities and sets up a base for further code restructuring in subsequent commits.
A new trait, `OffersMessageCommons`, is introduced to encapsulate functions that are commonly used by both BOLT12-related functionality and other parts of `ChannelManager`. This enables a clean transition of BOLT12 code to `OffersMessageFlow` by moving shared functions into the new trait, ensuring they remain accessible to both `ChannelManager` and the refactored BOLT12 code.
This commit introduces the `OffersMessageHandler` implementation for `OffersMessageFlow`, enabling direct access to offer-specific functionality through `OffersMessageFlow`. With `OffersMessageFlow` now serving as the source of `OffersMessageHandler` implementation, the implementation in `ChannelManager` is no longer needed and has been safely removed.
- This commit introduces a new struct, `AnOffersMessageFlow`, which generically implements `OffersMessageFlow`. - In subsequent commits, this struct will be utilized for documentation purposes.
- The _persistence_guard here was not serving any purpose, and hence can be removed in this refactoring.
1. This allow removing an extra function from commons, simplifying the flow trait.
Add a new `RouterConfig` to the `DefaultMessageRouter`, enabling control over the type of blinded path (or no blinded path) generated when `create_blinded_paths` is invoked. This enhancement introduces flexibility to skip path creation when needed, while unifying the blinded path creation logic into a single, cohesive function.
Prepare `create_blinded_paths` to handle both normal blinded paths and compact blinded paths creation by enabling it to accept a `Vec<MessageForwardNode>` as input. This change ensures the function has sufficient information for flexible and future-proof path creation, aligning with upcoming changes.
This commit updates the `create_blinded_path` implementation to use the `config` field introduced earlier. The `config` field determines which type of blinded path to create or whether to skip creating blinded paths altogether.
This update adds the flexibility to pass a router directly to the `create_offer_builder` for blinded path creation. The reasoning behind this change is twofold: 1. It streamlines the blinded path creation process by allowing users to specify the type of blinded path (or no blinded paths) they want in the offer through the selected router behavior. 2. It introduces the option to pass a custom router, enabling users to implement custom behavior for blinded path handling if needed.
This update modifies `create_refund_builder` to accept a router as a parameter. The reasoning for this change aligns with the `create_offer_builder` case, enabling users to specify the desired type of blinded path (or no blinded paths) and allowing flexibility to pass a custom router for tailored behavior.
Following the updates in previous commits, the other variants of `create_blinded_paths`—namely `create_compact_blinded_paths` and `create_blinded_paths_using_absolute_expiry`—are now redundant. This commit removes these variants as they are no longer necessary.
PR Description:
An Offer and a Refund can exist without a blinded path, where the
signing_pubkeyis used to determine the destination. While we could already handle Offers and Refunds without a blinded path, we didn’t have a way to create them like this. This PR addresses that gap and simplifies the blinded path creation flow.The Key Changes:
Pass Router to Builders:
Both
create_offer_builderandcreate_refund_buildernow take a router as a parameter. This gives users control over how blinded paths are created. For example, users can now choose not to include any blinded paths, or use a custom router for specific behavior.Unified Blinded Path Creation:
The Blinded Path creation has been simplified by only keeping a single function for Blinded Path creation, i.e.,
create_blinded_path. Older, separate variants likecreate_compact_blinded_pathsandcreate_blinded_paths_using_absolute_expiryare now redundant and have been removed. This makes the flow cleaner and easier to maintain.Router-Driven Flexibility:
The
DefaultMessageRouternow uses aRouterConfigto allow it to create any kind of blinded path (if any) to create. This simplifies the flow, while maintaining the same functionality.This PR makes blinded path creation more straightforward and gives users better control, while preserving the core logic of the
MessageRouter.blocked on #3412