rust-lightning icon indicating copy to clipboard operation
rust-lightning copied to clipboard

Enable Creation of Offers and Refunds Without Blinded Path

Open shaavan opened this issue 1 year ago • 23 comments

PR Description:

An Offer and a Refund can exist without a blinded path, where the signing_pubkey is 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:

  1. Pass Router to Builders:
    Both create_offer_builder and create_refund_builder now 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.

  2. 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 like create_compact_blinded_paths and create_blinded_paths_using_absolute_expiry are now redundant and have been removed. This makes the flow cleaner and easier to maintain.

  3. Router-Driven Flexibility:
    The DefaultMessageRouter now uses a RouterConfig to 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

shaavan avatar Aug 16 '24 08:08 shaavan

Codecov Report

Attention: Patch coverage is 96.10028% with 14 lines in your changes missing coverage. Please review.

Project coverage is 88.95%. Comparing base (ebe571a) to head (596cf47). Report is 47 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/onion_message/messenger.rs 84.84% 5 Missing :warning:
lightning/src/ln/channelmanager.rs 90.62% 2 Missing and 1 partial :warning:
lightning/src/ln/offers_tests.rs 98.01% 1 Missing and 1 partial :warning:
lightning/src/util/test_utils.rs 94.44% 2 Missing :warning:
lightning-dns-resolver/src/lib.rs 66.66% 1 Missing :warning:
lightning/src/offers/flow.rs 98.98% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3246      +/-   ##
==========================================
+ Coverage   88.93%   88.95%   +0.01%     
==========================================
  Files         168      173       +5     
  Lines      121962   123516    +1554     
  Branches   121962   123516    +1554     
==========================================
+ Hits       108471   109870    +1399     
- Misses      11088    11194     +106     
- Partials     2403     2452      +49     
Flag Coverage Δ
fuzzing 22.28% <0.00%> (-0.42%) :arrow_down:
tests 88.77% <96.10%> (+0.01%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Aug 16 '24 08:08 codecov[bot]

Updated from pr3246.01 to pr3246.02 (diff):

Changes:

  1. Rebase on main to resolve merge conflicts.

shaavan avatar Aug 19 '24 12:08 shaavan

Updated from pr3246.02 to pr3246.03 (diff):

Changes:

  1. Fix docs.
  2. Introduce tests for offer and refund with no blinded paths.

shaavan avatar Aug 19 '24 13:08 shaavan

Updated from pr3246.03 to pr3246.04 (diff):

Changes:

  1. Rebase on main, to resolve merge conflicts.
  2. Fix CI.

shaavan avatar Aug 20 '24 12:08 shaavan

Updated from pr3246.04 to pr3246.05 (diff):

Changes:

  1. Rebase on main.

shaavan avatar Sep 04 '24 11:09 shaavan

Updated from pr3246.05 to pr3246.06 (diff): Addressed @jkczyz comments

Changes:

  1. Removed the global constant PATHS_PLACEHOLDER, and instead use a default constructor, and local DEFAULT_VALUE.
  2. Remove the redundant functions in their appropriate commits.
  3. Use match to avoid mut variables.

shaavan avatar Sep 05 '24 20:09 shaavan

Updated from pr3246.06 to pr3246.07 (diff):

Changes:

  1. Introduce a new approach using the BlindedPathType enum.
  2. The enum allows for the specification of the type of Blinded Path (Compact or Full) that a user can specify to create the desired type of Blinded Path.
  3. Update offer_builder and refund_builder so that user can explicitly specify the type of Blinded Path they want to create.
  4. Update the Blinded Path creation flow so that only one function flow is responsible for creating both kinds of Blinded Paths.

shaavan avatar Sep 19 '24 15:09 shaavan

Updated from pr3246.07 to pr3246.08 (diff):

Changes:

  1. Rebase on main to resolve merge conflicts.

shaavan avatar Sep 19 '24 16:09 shaavan

Updated from pr3246.08 to pr3246.09 (diff):

Changes:

  1. Rebase on main, and fix ci.

shaavan avatar Oct 03 '24 12:10 shaavan

Updated from pr3246.09 to pr3246.10 (diff): Addressed @jkczyz comments

Changes:

  1. Update BlindedPathType documentation.
  2. Update code to amend all returned paths by MessageRouter to offers, and refund
  3. DRY create_offer_builder, and create_refund_builder

shaavan avatar Oct 11 '24 14:10 shaavan

Updated from pr3246.10 to pr3246.11 (diff): Addressed @bluematt & @jkczyz comments

Changes:

  1. Reverted the "Unified Flow" commits, hence re-introducing the two function flows for blinded path creation.
  2. Squashed the rest of the commits.
  3. Moved BlindedPathType to channelmanager, as it is only used there.
  4. Update DefaultMessageRouter::create_compact_blinded_paths to only return a single path.

shaavan avatar Oct 19 '24 12:10 shaavan

Updated from pr3246.11 to pr3246.12 (diff):

Changes:

  1. Rebase on main to resolve conflicts

shaavan avatar Oct 19 '24 12:10 shaavan

Updated from pr3246.12 to pr3246.13 (diff):

Changes:

  1. Rebase on main.

shaavan avatar Oct 24 '24 12:10 shaavan

Updated from pr3246.13 to pr3246.14 (diff):

Changes:

  1. Rebase on #3412
  2. Introduce the "Router-based" approach.
  3. With this a user can either pass the DefaultMessageRouter, configured to return a particular type of blinded path (or no blinded paths), or can pass a custom message router to the create_{offer/refund}_builder, to return customised blinded paths.

shaavan avatar Dec 13 '24 16:12 shaavan

@TheBlueMatt, @jkczyz

A gentle ping! Would love to get your thoughts on this new approach, when you get a moment! Thanks a lot! :)

shaavan avatar Dec 13 '24 16:12 shaavan

Updated from pr3246.14 to pr3246.15 (diff):

Changes:

  1. Rebase on main, to resolve merge conflicts

shaavan avatar Dec 16 '24 14:12 shaavan

Updated from pr3246.15 to pr3246.16 (diff): Addressed @jkczyz comments:

Changes:

  1. Introduce create_{offer, refund}_builder_using_router, to create blinded paths using custom router.

shaavan avatar Jan 11 '25 14:01 shaavan

Updated from pr3246.16 to pr3246.17 (diff):

Changes

  • Reverted the rebase from #3412.

Reasoning Since the approach in #3412 is still under discussion, I felt it might be better to un-rebase this PR for now. This keeps the thread active and allows us to continue the conversation here in parallel with the ongoing OffersMessageFlow work.

Open to thoughts or suggestions, of course!

shaavan avatar Mar 28 '25 12:03 shaavan

Updated from pr3246.17 to pr3246.18 (diff): Addressed @jkczyz comments

Changes:

  1. Update create_blinded_paths_from_iter() to ignore scid when user wants to create full-length blinded paths.
  2. Make MAX_SHORT_LIVED_RELATIVE_EXPIRY test only const.
  3. Update documentation on ways user can create compact blinded paths.
  4. Correct create_offer_builder_using_router documentation.
  5. DRY up create_{offer_,refund}_builder.

shaavan avatar Apr 02 '25 13:04 shaavan

I'm definitely okay with having methods to override the router used, and like the changes to not decide in ChannelManager if a path should be compact or not, but I'm a bit confused because now it seems like we default to doing the wrong thing - if an offer is short-lived we never build a compact blinded path unless the user manually asks for it by passing a CompactMessageRouter? That seems like an important regression (unless we think actually its not worth it to have compact BPs in short-lived offers cause most offers are long-lived?).

TheBlueMatt avatar Apr 15 '25 17:04 TheBlueMatt

I'm definitely okay with having methods to override the router used, and like the changes to not decide in ChannelManager if a path should be compact or not, but I'm a bit confused because now it seems like we default to doing the wrong thing - if an offer is short-lived we never build a compact blinded path unless the user manually asks for it by passing a CompactMessageRouter? That seems like an important regression (unless we think actually its not worth it to have compact BPs in short-lived offers cause most offers are long-lived?).

It could create compact paths by default if ChannelManager were parameterized by CompactMessageRouter. And besides from having the user explicitly pass CompactMessageRouter to a one of the methods, how could it be done without having ChannelManager decide?

I guess by passing the Option<Duration> through from ChannelManager to MessageRouter? Not sure it if is even worth having CompactMessageRouter then. NullMessageRouter may be sufficient to pass as an override. Don't have strong opinion, though it would be nice if create_offer_builder didn't need an optional parameter.

jkczyz avatar Apr 15 '25 20:04 jkczyz

Updated from pr3246.18 to pr3246.19 (diff): Addressed @jkczyz comments

Changes:

  • Expanded NullMessageRouter, and CompactMessageRouter document.

shaavan avatar Apr 18 '25 18:04 shaavan

It could create compact paths by default if ChannelManager were parameterized by CompactMessageRouter. And besides from having the user explicitly pass CompactMessageRouter to a one of the methods, how could it be done without having ChannelManager decide?

I guess by passing the Option<Duration> through from ChannelManager to MessageRouter?

Right, this is what I was thinking - give the router enough information to decide and let it decide, then let users override the router if they want to override the decision.

Not sure it if is even worth having CompactMessageRouter then. NullMessageRouter may be sufficient to pass as an override.

I guess if you want to always use a compact path (even for long-lived paths)? Could also have some kind of configurable router that just lets you pick the time limit.

Don't have strong opinion, though it would be nice if create_offer_builder didn't need an optional parameter.

Agreed, it can be a separate function tho.

TheBlueMatt avatar Apr 29 '25 18:04 TheBlueMatt

@TheBlueMatt

I see the point you're suggesting, Matt, and I agree there’s some convenience in having the router automatically decide the blinded path type based on Option<Duration>. But I think having the user explicitly call the offer builder with a CompactMessageRouter is just as convenient—and arguably cleaner.

The current approach has the benefit of being explicit and predictable. By default, we always generate a full-length path, providing a consistent baseline across all offers. If a user wants a compact path—for example, for short-lived offers—they can opt into it clearly via the router they pass in. Conversely, if the user parameterizes ChannelManager with a CompactMessageRouter, they’ll consistently get compact paths—unless they manually override that choice.

This trades away automatic decision-making by the router in favor of something I believe is more valuable in the long run: user awareness and control. It also avoids introducing heuristics that could surprise users unless they’re quite familiar with the internal logic.

In that sense, prioritizing clear defaults and explicit opt-in behavior, I believe, makes the design more intuitive and easier to reason about over time.

Curious to hear your thoughts—thanks!

shaavan avatar May 05 '25 15:05 shaavan

I see the point you're suggesting, Matt, and I agree there’s some convenience in having the router automatically decide the blinded path type based on Option<Duration>. But I think having the user explicitly call the offer builder with a CompactMessageRouter is just as convenient—and arguably cleaner.

Regarding this point, I tend to agree that removing the Option<Duration> from methods like create_offer_builder is more desirable. The existing behavior uses this to set the expiry and provide additional information for creating the path. But upon returning an OfferBuilder, the user can then override the expiry with a longer one and be left with a long-lived offer using compact blinded paths.

jkczyz avatar May 06 '25 23:05 jkczyz

Updated from pr3246.19 to pr3246.20 (diff): Addressed @TheBlueMatt comments

Changes:

  1. Update DefaultMessageRouter to create Compact BlindedMessagePath by default.
  2. When a user wants to create full-length blinded path, they can parameterise using the NodeIdMessageRouter

shaavan avatar Jun 19 '25 18:06 shaavan

Updated from pr3246.20 to pr3246.21 (diff):

Changes:

  1. Rebase on main
  2. Clean-up commits history and messages.
  3. Address other minor nit-changes.

shaavan avatar Jun 24 '25 12:06 shaavan

Updated from pr3246.21 to pr3246.22 (diff): Addressed @TheBlueMatt and @jkczyz comments

Changes:

  1. Addressed documentation updates, other nit suggestions.

shaavan avatar Jul 01 '25 12:07 shaavan

Updated from pr3246.22 to pr3246.23 (diff):

Changes:

  1. Rebase on main

shaavan avatar Jul 01 '25 12:07 shaavan

Updated from pr3246.23 to pr3246.24 (diff): Addressed @jkczyz comments

Changes:

  1. Addressed documentation fixup, and test improvements.

shaavan avatar Jul 02 '25 17:07 shaavan