Limit payment path length based on `payment_metadata`, custom TLVs, etc. #3026

valentinewallace · 2024-04-25T21:13:02Z

Currently the maximum payment path length is hardcoded in the router to 19. However, the presence of payment_metadata, custom TLVs, and/or blinded paths may mean that the actual limit is much shorter. Account for these extra onion fields when calculating and setting the maximum path length for use in pathfinding.

Closes #2201.

valentinewallace · 2024-04-25T21:14:12Z

lightning/src/ln/outbound_payment.rs

+
+	let num_reserved_bytes = match &route_params.payment_params.payee {
+		Payee::Blinded { route_hints, .. } => {
+			let largest_path = route_hints


Note: currently I’m evaluating the max path length based on the largest blinded path, i.e. the blinded path that limits us the most. Therefore, if some blinded paths are larger than others, this could unnecessarily limit us in pathfinding. We could set the max path length on a per-blinded-path basis? In practice, ISTM that blinded paths within the same invoice are likely to be of similar sizes to each other.

lightning/src/ln/max_payment_path_len_tests.rs

codecov-commenter · 2024-04-25T23:05:30Z

Codecov Report

Attention: Patch coverage is 97.88732% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 89.85%. Comparing base (da7a916) to head (3cc673b).
Report is 4 commits behind head on main.

Files	Patch %	Lines
lightning/src/routing/router.rs	77.14%	8 Missing ⚠️
lightning/src/ln/max_payment_path_len_tests.rs	99.22%	2 Missing ⚠️
lightning/src/ln/outbound_payment.rs	91.66%	1 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3026      +/-   ##
==========================================
+ Coverage   89.79%   89.85%   +0.06%     
==========================================
  Files         116      117       +1     
  Lines       96466    97278     +812     
  Branches    96466    97278     +812     
==========================================
+ Hits        86619    87413     +794     
- Misses       7290     7314      +24     
+ Partials     2557     2551       -6

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

valentinewallace · 2024-04-29T19:44:41Z

I found some more edge cases around how get_route counts hops as part of a path, so feel free to hold off on review there.

valentinewallace · 2024-04-30T19:23:41Z

Rebased to (hopefully) fix CI. This should be good for review.

lightning/src/routing/router.rs

lightning/src/ln/outbound_payment.rs

lightning/src/routing/router.rs

valentinewallace · 2024-05-13T23:10:27Z

Feedback should be addressed. Also this needs a rebase so let me know when I can do that.

TheBlueMatt

Feel free to squash, IMO.

lightning/src/ln/onion_utils.rs

TheBlueMatt · 2024-05-14T15:22:45Z

lightning/src/ln/onion_utils.rs

+		node_features: NodeFeatures::empty(),
+		short_channel_id: 42,
+		channel_features: ChannelFeatures::empty(),
+		fee_msat: route_params.final_value_msat,


This could be higher if we have to overpay when routing, so the object could serialize a bit longer.

Added some buffer here. I think my buffer is a bit conservative so let me know if it needs tuning.

Will be useful when we reuse this enum to calculate the maximum path length, to avoid cloning the vecs.

Doable now that we take Vecs by reference in OutboundOnionPayload.

Will be used in the next commit when we make this configurable in PaymentParameters.

lightning/src/ln/onion_utils.rs

lightning/src/routing/router.rs

lightning/src/ln/onion_utils.rs

jkczyz · 2024-05-15T18:52:40Z

lightning/src/ln/onion_utils.rs

+		node_features: NodeFeatures::empty(),
+		short_channel_id: 42,
+		channel_features: ChannelFeatures::empty(),


In practice, would the features not be empty? Perhaps it is negligible?

These features aren't included in onion payloads so don't impact encoding size.

lightning/src/ln/max_payment_path_len_tests.rs

jkczyz · 2024-05-15T19:20:02Z

lightning/src/routing/router.rs

+	/// Sets the maximum number of hops that can be included in a payment path, based on the provided
+	/// [`RecipientOnionFields`] and blinded paths.
+	pub fn set_max_path_length(


Why do we expose this if max_path_length is overridden by outbound_payments.rs?

This was requested here: #3026 (comment). I suppose it could be useful if the user is calculating routes separately from sending a payment. Don't feel strongly about including it, though.

Hmmm... I see, though we should update ChannelManager::send_payment's docs to indicate that max_path_length is not honored then. Or maybe it would it be better to use the existing setting if it is smaller than what is computed instead of the default MAX_PATH_LENGTH_ESTIMATE?

Or maybe it would it be better to use the existing setting if it is smaller than what is computed instead of the default MAX_PATH_LENGTH_ESTIMATE?

I think that's a good point, went this way in a fixup.

jkczyz

I'm good if you want to squash

jkczyz · 2024-05-16T19:16:31Z

lightning/src/routing/router.rs

+	/// Sets the maximum number of hops that can be included in a payment path, based on the provided
+	/// [`RecipientOnionFields`] and blinded paths.
+	pub fn set_max_path_length(


Hmmm... I see, though we should update ChannelManager::send_payment's docs to indicate that max_path_length is not honored then. Or maybe it would it be better to use the existing setting if it is smaller than what is computed instead of the default MAX_PATH_LENGTH_ESTIMATE?

jkczyz · 2024-05-16T19:35:30Z

lightning/src/ln/onion_utils.rs

@@ -328,8 +328,9 @@ pub(crate) fn set_max_path_length(
 	.serialized_length()
 	.saturating_add(PAYLOAD_HMAC_LEN);

+	const OVERPAY_ESTIMATE_MULTIPLER: u64 = 3;


Just checking if the change from 4 to 3 was intentional.

Yeah, should've called this out but 3 felt cleaner somehow? As noted here #3026 (comment), these numbers may want tuning.

Will be useful when we want to calculate the total size of the payloads without actually allocating for them.

valentinewallace · 2024-05-16T22:21:32Z

Squashed with 1 new fixup.

jkczyz · 2024-05-17T14:48:32Z

lightning/src/ln/onion_utils.rs

+		.ok_or(())?;
+
+	route_params.payment_params.max_path_length =
+		[max_path_length, route_params.payment_params.max_path_length, MAX_PATH_LENGTH_ESTIMATE]


Should we even include MAX_PATH_LENGTH_ESTIMATE? If the computed max_path_length is smaller than the given one, shouldn't we prefer that over an estimated one?

We need to include MAX_PATH_LENGTH_ESTIMATE here because the router currently has hardcoded assumptions that 19 will be the maximum path length. So if the computed value and the user-provided value are both higher than that, we still want to set the parameter to be the estimate.

I see. Should that be capped in the router, though? That is, is it conceivable that a different router implementation would not have that assumption?

We do cap it in the router so you're right it's not necessary. I thought it made sense to have some kind of upper bound, but we can remove it if you prefer.

Went ahead and removed it in the latest push.

TheBlueMatt · 2024-05-17T15:06:03Z

lightning/src/routing/router.rs

@@ -684,6 +698,11 @@ pub struct PaymentParameters {
 	/// Defaults to [`DEFAULT_MAX_PATH_COUNT`].
 	pub max_path_count: u8,

+	/// The maximum total number of hops in any returned path, including [`Path::hops`] and
+	/// [`BlindedTail::hops`].


I guess this is fine, but should we really be included the blinded tail? Its not really a part of the "path" in the sense that the router didn't pick it and we refer to it separately from the hops themselves. It also results in code specifically to add/remove it as an offset here and in the limit calculator, which we could just elide.

Fair enough, done in fixups.

jkczyz

LGTM. Feel free to squash.

Will be used so the outbound_payment module can calculate the maximum path length with knowledge of any custom TLVs or payment metadata present.

Also adds some testing by augmenting existing tests.

So the router knows how long the maximum payment path can be.

valentinewallace · 2024-05-20T23:01:53Z

Squashed.

valentinewallace commented Apr 25, 2024

View reviewed changes

valentinewallace force-pushed the 2024-04-limit-path-len branch from 3f0da18 to 52bb5d2 Compare April 25, 2024 21:20

valentinewallace commented Apr 25, 2024

View reviewed changes

lightning/src/ln/max_payment_path_len_tests.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the 2024-04-limit-path-len branch from 52bb5d2 to 1af8a0c Compare April 26, 2024 14:53

valentinewallace added this to the 0.0.124 milestone Apr 29, 2024

valentinewallace force-pushed the 2024-04-limit-path-len branch 2 times, most recently from 8ef1770 to c8fbb96 Compare April 30, 2024 19:21

valentinewallace requested review from TheBlueMatt and jkczyz April 30, 2024 19:23

TheBlueMatt reviewed May 9, 2024

View reviewed changes

valentinewallace force-pushed the 2024-04-limit-path-len branch 3 times, most recently from 7ee7c37 to 8de126d Compare May 13, 2024 23:08

TheBlueMatt reviewed May 14, 2024

View reviewed changes

valentinewallace added 4 commits May 14, 2024 09:57

OutboundOnionPayload: hold Vec fields as references.

e4e6e09

Will be useful when we reuse this enum to calculate the maximum path length, to avoid cloning the vecs.

Remove unnecessary RecipientOnionFields clone.

960dd65

Doable now that we take Vecs by reference in OutboundOnionPayload.

TestRouter: support checking only that route params are as expected.

336c77c

Make MAX_PATH_LENGTH_ESTIMATE const pub and expand doc comment.

2de600b

Will be used in the next commit when we make this configurable in PaymentParameters.

valentinewallace force-pushed the 2024-04-limit-path-len branch from 8de126d to 6255996 Compare May 14, 2024 20:48

jkczyz reviewed May 15, 2024

View reviewed changes

valentinewallace force-pushed the 2024-04-limit-path-len branch from 6255996 to 08fada7 Compare May 15, 2024 21:14

jkczyz reviewed May 16, 2024

View reviewed changes

Extract onion_utils::build_onion_payloads_callback helper.

bcdb67e

Will be useful when we want to calculate the total size of the payloads without actually allocating for them.

valentinewallace force-pushed the 2024-04-limit-path-len branch from 08fada7 to e6785dd Compare May 16, 2024 22:16

jkczyz reviewed May 17, 2024

View reviewed changes

TheBlueMatt reviewed May 17, 2024

View reviewed changes

valentinewallace force-pushed the 2024-04-limit-path-len branch 2 times, most recently from 7d417a6 to 6622736 Compare May 20, 2024 19:26

jkczyz reviewed May 20, 2024

View reviewed changes

valentinewallace added 6 commits May 20, 2024 18:57

Add max path length knob to PaymentParameters.

7f7f375

Will be used so the outbound_payment module can calculate the maximum path length with knowledge of any custom TLVs or payment metadata present.

Abide by max path length param in router.

baa727e

Also adds some testing by augmenting existing tests.

Test utils: include payment_metadata in PassAlongPathArgs.

c6de5e1

outbound_payment: set max path length in PaymentParameters.

f0b5c58

So the router knows how long the maximum payment path can be.

Test routing payment parameters' max path length setting and usage.

c07245f

Add public util on RouteParameters for setting max path length.

3cc673b

valentinewallace force-pushed the 2024-04-limit-path-len branch from 6622736 to 3cc673b Compare May 20, 2024 22:57

jkczyz approved these changes May 20, 2024

View reviewed changes

TheBlueMatt approved these changes May 21, 2024

View reviewed changes

TheBlueMatt merged commit 37bf61c into lightningdevkit:main May 21, 2024
16 checks passed

valentinewallace mentioned this pull request Jul 3, 2024

Set max path length when paying BOLT 12 invoices. #3156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit payment path length based on `payment_metadata`, custom TLVs, etc. #3026

Limit payment path length based on `payment_metadata`, custom TLVs, etc. #3026

valentinewallace commented Apr 25, 2024

valentinewallace Apr 25, 2024

codecov-commenter commented Apr 25, 2024 •

edited

Loading

valentinewallace commented Apr 29, 2024

valentinewallace commented Apr 30, 2024

valentinewallace commented May 13, 2024

TheBlueMatt left a comment

TheBlueMatt May 14, 2024

valentinewallace May 14, 2024

jkczyz May 15, 2024

valentinewallace May 15, 2024

jkczyz May 15, 2024

valentinewallace May 15, 2024

jkczyz May 16, 2024

valentinewallace May 16, 2024

jkczyz left a comment

jkczyz May 16, 2024

jkczyz May 16, 2024

valentinewallace May 16, 2024

valentinewallace commented May 16, 2024 •

edited

Loading

jkczyz May 17, 2024

valentinewallace May 17, 2024

jkczyz May 20, 2024

valentinewallace May 20, 2024

valentinewallace May 20, 2024

TheBlueMatt May 17, 2024

valentinewallace May 17, 2024

jkczyz left a comment

valentinewallace commented May 20, 2024

Limit payment path length based on payment_metadata, custom TLVs, etc. #3026

Limit payment path length based on payment_metadata, custom TLVs, etc. #3026

Conversation

valentinewallace commented Apr 25, 2024

Choose a reason for hiding this comment

codecov-commenter commented Apr 25, 2024 • edited Loading

Codecov Report

valentinewallace commented Apr 29, 2024

valentinewallace commented Apr 30, 2024

valentinewallace commented May 13, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

valentinewallace commented May 20, 2024

Limit payment path length based on `payment_metadata`, custom TLVs, etc. #3026

Limit payment path length based on `payment_metadata`, custom TLVs, etc. #3026

codecov-commenter commented Apr 25, 2024 •

edited

Loading

valentinewallace commented May 16, 2024 •

edited

Loading