Skip to content
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

BOLT12 sending doesn't allow to override {Route,Payment}Parameters #3262

Open
tnull opened this issue Aug 21, 2024 · 5 comments
Open

BOLT12 sending doesn't allow to override {Route,Payment}Parameters #3262

tnull opened this issue Aug 21, 2024 · 5 comments
Assignees
Labels
Take a Friday Leave a Friday Stomp the Bugs, Without Much Commitment
Milestone

Comments

@tnull
Copy link
Contributor

tnull commented Aug 21, 2024

For BOLT11 we allow to set RouteParameters as an argument to send_payment. However, pay_for_offer only allows to set max_total_routing_fee_msat, but not other routing-related fields such as max_total_cltv_expiry_delta, max_path_count, max_channel_saturation_power_of_half.

I'm a bit dubious if pay_for_offer and paying for refunds should be parametrized by RouteParameters, PaymentParameters, or the relevant fields independently, but we should provide a way to set them for BOLT12 payments, too.

@TheBlueMatt
Copy link
Collaborator

Right, so the {Route,Payment}Parameters abstractions don't make any sense in a BOLT 12 world. They also don't really make sense in a BOLT 11 world if we pass a Bolt11Invoice directly to ChannelManager (which we should do now that lightning depends on lightning-invoice).

So I think this tees us up for a nice patchset - define some new struct which captures the options you describe, pass it to pay_for_offer and use that + the BOLT 12 invoice to build an initial RouteParameters, then do the same on the BOLT 11 side - drop lightning::ln::bolt11_payment and replace it with a Channelmanager method that does the same thing as the BOLT 12 path, just with BOLT 11 and the new parameters struct.

@TheBlueMatt TheBlueMatt added the Take a Friday Leave a Friday Stomp the Bugs, Without Much Commitment label Sep 9, 2024
@shaavan
Copy link
Contributor

shaavan commented Sep 25, 2024

Giving this a shot! 🚀

@vincenzopalazzo
Copy link
Contributor

It this issue include two separate PRs @TheBlueMatt ?

The first is adding some kind of structure (Probably with @shaavan did in #3342 (review)) for the BOLT 12 world that includes the parameters needed by @tnull

Then, add a method inside the ChannelManager like pay_for_offer that do the same thing like with the bolt11.

Not sure what you mean by dropping lightning::ln::bolt11_payment and replacing it with a Channelmanager methods but probably I am missing something here

@shaavan
Copy link
Contributor

shaavan commented Oct 3, 2024

Hey @vincenzopalazzo!

From what I understand, the {bolt11, bolt12} invoice contains the PaymentParams, and RouteParams includes the PaymentParams plus two additional fields

Since send_payment_for_bolt12_invoice takes the whole BOLT12Invoice as input, it can build the RouteParams on the fly based on BOLT12Invoice::PaymentParams.

Now, if we tweak the bolt11's send_payment to also take the bolt11 invoice directly as input, it could construct the RouteParams similarly, eliminating the need for that extra step of manually extracting params from the invoice.

Given that lightning::ln::bolt11_payment contains the functions for extraction, this change will make the file redundant, allowing us to clean things up a bit by removing it.

@TheBlueMatt, does this sound right to you? Thanks so much!

@TheBlueMatt
Copy link
Collaborator

It this issue include two separate PRs @TheBlueMatt ?

That's how I was thinking of it, yea.

Then, add a method inside the ChannelManager like pay_for_offer that do the same thing like with the bolt11.

Right.

Not sure what you mean by dropping lightning::ln::bolt11_payment and replacing it with a Channelmanager methods but probably I am missing something here

Once we have ChannelManager::pay_for_bolt11_invoice, lightning::ln::bolt11_payment should be basically redundant and can just be dropped.

From what I understand, the {bolt11, bolt12} invoice contains the PaymentParams, and RouteParams includes the PaymentParams plus two additional fields

Almost, but its still kinda a mix. The old PaymentParameters/RouteParameters distinction was/is "RouteParameters are the params for picking an individual route, PaymentParameters are the ones that apply to all route-finding session for a payment across retries". Thus, you can convert a BOLT 11/12 invoice plus some additional options into a PaymentParameters and then outbound_payment can build per-path-finding RouteParameters.

With pay_for_offer/pay_for_bolt12_invoice/a future ChannelManager::pay_for_bolt11_invoice we need to change the distinction to "ManualRoutingParameters (or maybe a different name) contains all the parameters that the sender picks, with the rest coming from the BOLT 11/12 invoice".

Fields like max_total_cltv_expiry_delta, max_path_count, max_path_length (though the invoice could further reduce the max length the user specifies), and max_channel_saturation_power_of_half are all things the user should be able to specify, whereas payee/expiry_time/previously_failed_* are things that the invoice/payment session should take control over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Take a Friday Leave a Friday Stomp the Bugs, Without Much Commitment
Projects
None yet
Development

No branches or pull requests

4 participants