Introduce ManualRoutingParameters

[shaavan]

trafficstars

Partially addresses #3262

• Introduce a new struct ManualRoutingParameters that optionally takes the parameter to set for routing.
• Pass it to pay_for_offer to use it along with the invoice's payment parameter to set route parameters.

[shaavan]

Hey @TheBlueMatt @tnull,

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[bot]]

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 :warning:
lightning/src/routing/router.rs	67.85%	9 Missing :warning:
lightning/src/ln/channelmanager.rs	83.33%	0 Missing and 1 partial :warning:

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     

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

[shaavan]

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

Changes:

1. Rebase on main

[shaavan]

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

Changes:

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

[TheBlueMatt]

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

[shaavan]

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

Changes:

Update Role of RouteParamsOverride:

1. Instead of presenting this new struct as just overriding the PaymentParams, it is now structured as allowing the user to set configuration parameters.
2. 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:

1. Introduced a new separate function, with_user_config, in PaymentParams, making the function more modular and focused.
2. 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]

@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]

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]

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

Changes:

1. Rebase on main. Rest no changes.

[shaavan]

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

Changes:

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

[TheBlueMatt]

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]

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.

[TheBlueMatt]

Okay, we probably shouldn't have held this up on #3378 now that it took way too long to land, but it did finally land, care to rebase on that?

[shaavan]

Okay, we probably shouldn't have held this up on https://github.com/lightningdevkit/rust-lightning/pull/3378 now that it took way too long to land, but it did finally land, care to rebase on that?

I am glad, it finally landed! Rebase on the way! 🚂

[shaavan]

Updated from pr3342.06 to pr3342.07 (diff): Addressed @TheBlueMatt comments:

Changes:

1. Rebase on main.

[shaavan]

@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!

[jkczyz]

@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]

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

[jkczyz]

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]

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]

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

Changes:

1. Introduce the legacy macro to support the _max_total_routing_fee_msat.

[shaavan]

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

Changes:

1. Updated _encode_tlv macro to introduce compile time type check.

[shaavan]

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

Changes:

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

[shaavan]

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

Changes:

1. Rebase on main.

[shaavan]

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

Changes:

1. Implement default for RouteParametersConfig.
2. Remove extra whitespace.

[jkczyz]

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.

[shaavan]

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

Changes:

1. 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.
2. Squash, and clean-up commit history

[TheBlueMatt]

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 {