Introduce RouteParametersConfig by shaavan · Pull Request #3342 · lightningdevkit/rust-lightning

shaavan · 2024-09-26T12:33:19Z

Partially addresses #3262

Introduce a struct that allows the user to configure the parameters for routing the payment.
Use the struct for BOLT12 payment flow.

Blocked on #3378

shaavan · 2024-09-26T12:38:02Z

A gentle ping! Would love to get your feedback or approach ACK before moving forward with testing and applying a similar approach on the BOLT11 side.

Thanks a lot!

codecov · 2024-09-26T12:40:07Z

Codecov Report

Attention: Patch coverage is 81.81818% with 22 lines in your changes missing coverage. Please review.

Project coverage is 88.58%. Comparing base (2d2c542) to head (d9d73ea).

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	70.73%	12 Missing ⚠️
lightning/src/routing/router.rs	67.85%	9 Missing ⚠️
lightning/src/ln/channelmanager.rs	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3342 +/- ##
==========================================
- Coverage 88.61% 88.58% -0.03% 
==========================================
Files 149 149 Lines 116877 116931 +54 Branches 116877 116931 +54 ==========================================
+ Hits 103568 103586 +18 - Misses 10802 10833 +31 - Partials 2507 2512 +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vincenzopalazzo

LGTM overall.

However I am not the right person to judge if the PR is what the ldk team is looking for.

For instance the following is confusing me :) but I think this is a discussion to be done inside the issue

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).

BTW Nice git history!

P.S: I left some API idea comment in some commit review, idk if they are useful or not. probably they will remove a lot of map(_ {}) code, but I had to write the code to confirm

vincenzopalazzo · 2024-10-02T16:13:15Z

 payer_note: Option<String>, payment_id: PaymentId, retry_strategy: Retry,
-max_total_routing_fee_msat: Option<u64>
+manual_routing_params: Option<ManualRoutingParameters>
 	) -> Result<(), Bolt12SemanticError> {


OK no needed option here but: what do you think about having the function call like pay_for_offer(offer, OfferParams::default()) where OfferParams is something like that
structOfferParams{quantity:Option<u64>,amount_msats:Option<u64>,manual_routing_params:Option<ManualRoutingParameters>,}
I think is a lot of more verbose in this way, but I also this that this will be a lot of cleaner.
Now I do not think this will fit well in this PR maybe can be a followup one, but with this patter, it is possible to hide future updates to offers (e.g: recurrence or market place feature) without breaking too much the code

I think this is a great improvement! It will help keep the function signature cleaner while remaining maintainable for future parameter increases.
Maybe we can take this in a follow-up. @TheBlueMatt, what do you think?

No strong opinions.

TheBlueMatt

High-level design looks right to me.

TheBlueMatt · 2024-10-08T18:16:09Z

 payer_note: Option<String>, payment_id: PaymentId, retry_strategy: Retry,
-max_total_routing_fee_msat: Option<u64>
+manual_routing_params: Option<ManualRoutingParameters>
 	) -> Result<(), Bolt12SemanticError> {


No strong opinions.

shaavan · 2024-10-13T13:31:59Z

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

Changes:

Rebase on main

shaavan · 2024-10-13T15:30:12Z

Updated from pr3342.02 to pr3342.03 (diff):
Addressed @vincenzopalazzo, @jkczyz, @TheBlueMatt comments

Changes:

Renamed ManualRoutingParameters -> RouteParametersOverride
Reintroduce the max_total_routing_fee_msat in AwaitingInvoice to support downgrading.
Update API, so that the PaymentParameters are created using the Overrides, and then the RouteParameters are created using it.

TheBlueMatt · 2024-10-16T23:48:21Z

+ // Deprecated: Retained for backward compatibility.
+ // If set during read, this field overrides `RouteParameters::max_total_routing_fee_msat`
+ // instead of `RouteParametersOverride::max_total_routing_fee_msat`.
 max_total_routing_fee_msat: Option<u64>,


Ugh, I really prefer to handle this kind of thing at the serialization layer rather than in our logic. It'll be pretty annoying to break out impl_writeable_tlv_based_enum_upgradable! here, though....I wonder if we shouldn't try to (yet again) expand the featureset of our enum-deser logic. Lets get this PR ready in every other way and then I can play with it, maybe in a followup.

Done! 🚀 Thanks a lot!

TheBlueMatt · 2024-10-16T23:51:25Z

Looks like most of my previous comments went unaddressed? Just checking cause I interpreted your comment as you addressing them.

shaavan · 2024-10-18T12:59:08Z

Updated from pr3342.03 to pr3342.04 (diff):
Addressed @TheBlueMatt’s comments:

Changes:

Update Role of RouteParamsOverride:

Instead of presenting this new struct as just overriding the PaymentParams, it is now structured as allowing the user to set configuration parameters.
This brings some changes to the struct:
- It has been renamed from RouteParamsOverride to RouteParamsConfig.
- The parameters are now required rather than optional.
- Documentation has been expanded to make the struct stand independently of {Route, Payment} Params.

Cleanups:

Introduced a new separate function, with_user_config, in PaymentParams, making the function more modular and focused.
RouteParamsConfig is now a required field in AwaitingInvoice and InvoiceReceived, initialized with default values if absent. This eliminates redundant optioning and switches to default values where necessary.

shaavan · 2024-10-18T13:02:31Z

@TheBlueMatt

Looks like most of my previous comments went unaddressed? Just checking cause I interpreted your comment as you addressing them.

Hey Matt, I’m really sorry for missing your previous comments – not sure how that slipped through! I've gone through everything now and made sure to address them all in the latest update. Thanks so much for your patience! 🌟

TheBlueMatt · 2024-10-22T00:05:56Z

Okay, I probably spent a bit too long on it, but #3378 should let us drop the legacy fields in outbound_payment.rs at de-serialization time.

shaavan · 2024-10-22T11:46:43Z

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

Changes:

Rebase on main. Rest no changes.

shaavan · 2024-10-22T12:08:16Z

Updated from pr3342.05 to pr3342.06 (diff):
Addressed @jkczyz, @TheBlueMatt comments

Changes:

Fix the comment regarding supporting standard behaviour during upgrade.
Set the max_total_routing_fee_msat along with route_params config to support downgrade.
Fix the fn static_invoice_received to ensure proper compilation and behaviour async_payment case as well.
Refactor the code to use if let instead of map for better readability.
Introduce the full config parameter for create_refund_builder as well.
Structured the commits slightly to have better logical progression.

TheBlueMatt · 2024-10-22T19:00:07Z

Lets get a review pass or two on #3378 and see if we want to move forward with it. If we do, we should rebase this on that and do the conversion in outbound_payment.rs at deserialization time.

TheBlueMatt · 2024-12-08T00:55:32Z

While it would be very nice to have this in 0.1, we kinda lost track of 3378 and thus I don't think its realistic to get this in in time. IIRC ldk-node wanted it, so we can certainly do an 0.2 not too long after 0.1, but I don't really think that this is worth holding up 0.1 for.

jkczyz · 2025-02-11T16:02:00Z

@TheBlueMatt

Hi, after rebasing, I tried using the new syntax to handle the legacy fields. Here's the diff I wrote:

(5, AwaitingInvoice) => {
(0, expiration, required),
(2, retry_strategy, required),
- (4, max_total_routing_fee_msat, option),+ (4, _max_total_routing_fee_msat, (legacy, u64,+ |us: &PendingOutboundPayment| match us {+ PendingOutboundPayment::AwaitingInvoice { route_params_config, .. } => Some(route_params_config.max_total_routing_fee_msat),+ _ => None,+ }+ )),
(5, retryable_invoice_request, option),
- (7, route_params_config, (default_value, RouteParametersConfig::new())),+ (7, route_params_config, (default_value, (+ if let Some(fee_msat) = _max_total_routing_fee_msat {+ RouteParametersConfig::new().with_max_total_routing_fee_msat(fee_msat)+ } else {+ RouteParametersConfig::new()+ }+ ))),
},

The code compiles, but I ran into errors in multiple tests. I would love to get your insight on this—am I missing something or approaching it incorrectly? Thanks a lot!

Discussed offline. Just needed to remove the Some from the legacy write. Also, wondering if it should be Option<u64> and if the compiler would then catch it?

TheBlueMatt · 2025-02-11T16:20:54Z

Hmm, that's weird, legacy writes are supposed to be Options. Can you push the code somewhere?

jkczyz · 2025-02-11T16:25:12Z

Hmm, that's weird, legacy writes are supposed to be Options. Can you push the code somewhere?

Problem was max_total_routing_fee_msat is also an Option, so it was being double wrapped. I think we can force the compiler to catch this with an explicit type hint at:

https://github.com/shaavan/rust-lightning/blob/737867c3d44fb0f9922a25827cd654c9247102af/lightning/src/util/ser_macros.rs#L43

Something like:

{let value:Option<$fieldty> = $write($($self)?); value }

shaavan · 2025-02-11T16:25:44Z

legacy writes are supposed to be Options.

Yep! I believe that's working perfectly.
The issue was route_params_config.max_total_routing_fee_msat is already Option<u64>, but I was trying to return Some(route_params_config.max_total_routing_fee_msat) which caused the issue.

shaavan · 2025-02-11T16:31:26Z

Updated from pr3342.07 to pr3342.08 (diff):
Addressed @jkczyz suggestion

Changes:

Introduce the legacy macro to support the _max_total_routing_fee_msat.

shaavan · 2025-02-12T14:12:43Z

Updated from pr3342.08 to pr3342.09 (diff):
Addressed @jkczyz comment

Changes:

Updated _encode_tlv macro to introduce compile time type check.

shaavan · 2025-02-17T14:31:06Z

Updated from pr3342.09 to pr3342.10 (diff):
Addressed @jkczyz, @TheBlueMatt comments

Changes:

Update call-site so that user can call RouteParameters::default() instead of None, in default case.
Reorder tlv ordering so that legacy is appropriately read before the new field.

shaavan · 2025-02-17T14:52:22Z

Updated from pr3342.10 to pr3342.11 (diff):

Changes:

Rebase on main.

shaavan · 2025-02-19T12:57:32Z

Updated from pr3342.11 to pr3342.12 (diff):
Addressed @jkczyz comments.

Changes:

Implement default for RouteParametersConfig.
Remove extra whitespace.

jkczyz · 2025-02-19T16:16:39Z

LGTM. For the first commit's message, you can remove the "f: " prefix since it isn't a fixup (i.e., won't be squashed). @TheBlueMatt I'm happy to have the fixups squashed if you are.

TheBlueMatt

One comment on the last commit, but please do squash the fixups and clean up the history, I think this is basically there.

TheBlueMatt · 2025-02-24T20:59:59Z

 }
 	}

+	/// Update the parameters with the configuration provided by user.


This reads like a comment rather than documentation. Should read something like "Update the parameters with the given configuration". This also needs to mention that we do not apply the max_total_routing_fee_msat here, which is quite a potential footgun, so honestly I think maybe we should make this pub(crate) and even maybe call it with_user_config_ignoring_fee_limit.

Thanks for the pointer! Addressed in pr3342.13
I updated the naming and change the function to pub(crate) to make it explicit that max_total_routing_fee_msat is not applied here, and prevent potential footgunning.
Also, I added an explicit line of documentation to make it clearer.
Let me know if it looks good now! Thanks!

With the current architecture, Bolt12 payers & builders only allows setting `max_total_routing_fee_msat` as a route parameter. However, it doesn't provide users the flexibility to set other important parameters. This commit introduces a new struct, `RouteParametersConfig`, that optionally allows users to set additional routing parameters. In later commits, this struct will be utilized when paying BOLT12 invoices.

…sConfig` When Bolt12 payers & builders are called, they creates a new `PendingOutboundPayment` entry with relevant values that will be used when the corresponding invoice is received. This update modifies `AwaitingInvoice` & `AwaitingOffer` to include the entire `RouteParametersConfig` struct instead of just `max_total_routing_fee_msat`. This change ensures all manual routing parameters are available when finding payment routes. Decisions & Reasoning: **Introduction of `route_params_config` in `InvoiceReceived`:** This was added for the same reason that `max_total_routing_fee_msat` was originally introduced in PR lightningdevkit#2417. The documentation has been updated to reflect this, based on [this comment](lightningdevkit@d7e2ff6#r1334619765).

This update allows users to call `pay_for_offer`, `pay_for_offer_from_human_readable` and `create_refund_builder` with a set of parameters they wish to manually set for routing the corresponding invoice. By accepting `RouteParametersConfig`, users gain greater control over the routing process.

- This finally allow overriding the {Route, Payment} parameters with the RouteParametersConfig value provided by the user.

shaavan · 2025-02-25T13:16:07Z

Updated from pr3342.12 to pr3342.13 (diff):
Addressed @TheBlueMatt comments:

Changes:

Update the documentation and naming for with_user_config to be clearer and explicitly state that we ignore the max_total_routing_fee_msat application within it.
Squash, and clean-up commit history

TheBlueMatt · 2025-02-25T15:11:08Z

+ amt,
+ payment_id,
+ retry,
+ RouteParametersConfig::default(),


nit: For stuff like this where rustfmt requires we blow up our code to cover 20 lines we should really create intermediate variables to see if we can avoid it.

TheBlueMatt · 2025-02-25T15:12:44Z

Landing as the full diff since @jkczyz's "LGTM" is only

$ git diff-tree -U1 d9d73ead1f7fd65342f483d00686ccc845b40c59 78f6ccb5ca06f4f290ee46e2e9b945d47e38de53
diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index a6c0cc1ff..f56795cf5 100644
--- a/lightning/src/ln/outbound_payment.rs+++ b/lightning/src/ln/outbound_payment.rs@@ -893,3 +893,4 @@ impl OutboundPayments {
let mut route_params = RouteParameters::from_payment_params_and_value(
- PaymentParameters::from_bolt12_invoice(&invoice).with_user_config(params_config), invoice.amount_msats()+ PaymentParameters::from_bolt12_invoice(&invoice)+ .with_user_config_ignoring_fee_limit(params_config), invoice.amount_msats()
);
@@ -1068,3 +1069,4 @@ impl OutboundPayments {
let payment_hash = PaymentHash(Sha256::hash(&keysend_preimage.0).to_byte_array());
- let pay_params = PaymentParameters::from_static_invoice(invoice).with_user_config(*route_params_config);+ let pay_params = PaymentParameters::from_static_invoice(invoice)+ .with_user_config_ignoring_fee_limit(*route_params_config);
let mut route_params = RouteParameters::from_payment_params_and_value(pay_params, amount_msat);
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index e68322294..017496f26 100644
--- a/lightning/src/routing/router.rs+++ b/lightning/src/routing/router.rs@@ -945,4 +945,9 @@ impl PaymentParameters {
-	/// Update the parameters with the configuration provided by user.-	pub fn with_user_config(self, params_config: RouteParametersConfig) -> Self {+	/// Updates the parameters with the given route parameters configuration.+	///+	/// Note:+	/// We *do not* apply `max_total_routing_fee_msat` here, as it is unique to each route.+	/// Instead, we apply only the parameters that are common across multiple route-finding sessions+	/// for a payment across retries.+	pub(crate) fn with_user_config_ignoring_fee_limit(self, params_config: RouteParametersConfig) -> Self {
Self {

vincenzopalazzo reviewed Oct 2, 2024
View reviewed changes

vincenzopalazzo mentioned this pull request Oct 2, 2024
BOLT12 sending doesn't allow to override {Route,Payment}Parameters#3262
Closed

jkczyz self-requested a review October 3, 2024 16:03

jkczyz reviewed Oct 8, 2024
View reviewed changes

Comment threadlightning/src/ln/outbound_payment.rs Outdated
Comment threadlightning/src/ln/outbound_payment.rs

TheBlueMatt reviewed Oct 8, 2024
View reviewed changes

shaavan force-pushed the i3262 branch from 271522d to 0fc35e1Compare October 13, 2024 13:31

shaavan force-pushed the i3262 branch from 0fc35e1 to a608b55Compare October 13, 2024 15:30

shaavan changed the title ~~Introduce ManualRoutingParameters~~Introduce RouteParametersOverrideOct 14, 2024

TheBlueMatt reviewed Oct 16, 2024
View reviewed changes

shaavan force-pushed the i3262 branch 2 times, most recently from 7cdb03f to 63b08a4Compare October 18, 2024 12:58

shaavan changed the title ~~Introduce RouteParametersOverride~~Introduce RouteParametersConfigOct 18, 2024

jkczyz reviewed Oct 21, 2024
View reviewed changes

Comment threadlightning/src/ln/outbound_payment.rs Outdated
Comment threadlightning/src/ln/outbound_payment.rs Outdated
Comment threadlightning/src/ln/outbound_payment.rs Outdated
Comment threadlightning/src/ln/outbound_payment.rs Outdated
Comment threadlightning/src/ln/outbound_payment.rs Outdated

TheBlueMatt reviewed Oct 22, 2024
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

shaavan force-pushed the i3262 branch from 63b08a4 to d744f38Compare October 22, 2024 11:46

shaavan force-pushed the i3262 branch from d744f38 to 4221929Compare October 22, 2024 12:08

valentinewallace added this to the 0.1 milestone Nov 26, 2024

TheBlueMatt removed this from the 0.1 milestone Dec 8, 2024

shaavan force-pushed the i3262 branch from 737867c to 11d031eCompare February 11, 2025 16:31

jkczyz reviewed Feb 11, 2025
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/outbound_payment.rs

shaavan force-pushed the i3262 branch from 11d031e to 17351bbCompare February 12, 2025 14:12

TheBlueMatt reviewed Feb 12, 2025
View reviewed changes

Comment threadlightning/src/ln/outbound_payment.rs Outdated

shaavan force-pushed the i3262 branch from 17351bb to a0f8cfeCompare February 17, 2025 14:31

shaavan force-pushed the i3262 branch from a0f8cfe to e0e9a8cCompare February 17, 2025 14:51

jkczyz reviewed Feb 18, 2025
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated

shaavan force-pushed the i3262 branch from e0e9a8c to d9d73eaCompare February 19, 2025 12:57

shaavan mentioned this pull request Feb 24, 2025
Simplify Bolt11 Payments and Remove Redundant Code #3617
Merged

TheBlueMatt reviewed Feb 24, 2025
View reviewed changes

jkczyzand others added 5 commits February 25, 2025 18:40

Expand macro to add compile-time type check
8da348e

Use RouteParametersConfig to create new RouteParameters
78f6ccb
- This finally allow overriding the {Route, Payment} parameters with the RouteParametersConfig value provided by the user.

shaavan force-pushed the i3262 branch from d9d73ea to 78f6ccbCompare February 25, 2025 13:15

TheBlueMatt approved these changes Feb 25, 2025
View reviewed changes

TheBlueMatt merged commit ece4ab7 into lightningdevkit:mainFeb 25, 2025

Uh oh!

Conversation

shaavan commented Sep 26, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaavan commented Sep 26, 2024

Uh oh!

codecovBot commented Sep 26, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shaavan commented Oct 13, 2024

Uh oh!

shaavan commented Oct 13, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 16, 2024

Uh oh!

shaavan commented Oct 18, 2024

Update Role of RouteParamsOverride:

Cleanups:

Uh oh!

shaavan commented Oct 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Oct 22, 2024

Uh oh!

shaavan commented Oct 22, 2024

Uh oh!

shaavan commented Oct 22, 2024

Uh oh!

TheBlueMatt commented Oct 22, 2024

Uh oh!

TheBlueMatt commented Dec 8, 2024

Uh oh!

jkczyz commented Feb 11, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Feb 11, 2025

Uh oh!

jkczyz commented Feb 11, 2025

Uh oh!

shaavan commented Feb 11, 2025

Uh oh!

shaavan commented Feb 11, 2025

Uh oh!

Uh oh!

shaavan commented Sep 26, 2024•
edited
Loading

codecovBot commented Sep 26, 2024•
edited
Loading

jkczyz commented Feb 11, 2025•
edited
Loading