Fix long route failure attribution by joostjager · Pull Request #3709 · lightningdevkit/rust-lightning

joostjager · 2025-04-04T12:16:09Z

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

Fixes#3708

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

ldk-reviews-bot · 2025-04-04T12:16:12Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt

Cool, thanks. Gonna go ahead and land this because its basically trivial.

ldk-reviews-bot · 2025-04-04T12:52:16Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

joostjager · 2025-04-04T13:23:32Z

Well not completely trivial although the code change seems to be. I had to look twice at the fix myself. But hopefully the comments explain it well enough.

TheBlueMatt · 2025-04-04T13:29:43Z

Well the test is quite compelling :)

carlaKC

post-merge review, lgtm!

carlaKC · 2025-04-04T18:13:40Z


+	// In the best case, paths can be up to 27 hops. But attribution data can only be conveyed back to the sender from
+	// the first 20 hops. Determine the number of hops to be used for attribution data.
+	let attributable_hop_count = usize::min(path.hops.len(), MAX_HOPS);


Still find myself wishing that the caller didn't need to concern itself with this level of detail about attributable faliures, but don't see an obvious way to improve it.

At the point of interpreting the failure message, there isn't much we can do about it. But one open question is whether we should limit pathfinding to 20 hops instead of 27 in pathfinding, so that we're sure every failure is attributable?

I doubt it's worth it. In the rare case that we are sending more than 20 hops it's probably because someone is doing something insane where they have a many-hop route hint, in which case it's not useful to get attribution data beyond the first handful.

joostjager · 2025-04-04T12:16:09Z

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

Fixes#3708

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

ldk-reviews-bot · 2025-04-04T12:16:12Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt

Cool, thanks. Gonna go ahead and land this because its basically trivial.

ldk-reviews-bot · 2025-04-04T12:52:16Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

joostjager · 2025-04-04T13:23:32Z

Well not completely trivial although the code change seems to be. I had to look twice at the fix myself. But hopefully the comments explain it well enough.

TheBlueMatt · 2025-04-04T13:29:43Z

Well the test is quite compelling :)

carlaKC

post-merge review, lgtm!

carlaKC · 2025-04-04T18:13:40Z


+	// In the best case, paths can be up to 27 hops. But attribution data can only be conveyed back to the sender from
+	// the first 20 hops. Determine the number of hops to be used for attribution data.
+	let attributable_hop_count = usize::min(path.hops.len(), MAX_HOPS);


Still find myself wishing that the caller didn't need to concern itself with this level of detail about attributable faliures, but don't see an obvious way to improve it.

At the point of interpreting the failure message, there isn't much we can do about it. But one open question is whether we should limit pathfinding to 20 hops instead of 27 in pathfinding, so that we're sure every failure is attributable?

I doubt it's worth it. In the rare case that we are sending more than 20 hops it's probably because someone is doing something insane where they have a many-hop route hint, in which case it's not useful to get attribution data beyond the first handful.

joostjager · 2025-04-04T12:16:09Z

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

Fixes#3708

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

ldk-reviews-bot · 2025-04-04T12:16:12Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt

Cool, thanks. Gonna go ahead and land this because its basically trivial.

ldk-reviews-bot · 2025-04-04T12:52:16Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

joostjager · 2025-04-04T13:23:32Z

Well not completely trivial although the code change seems to be. I had to look twice at the fix myself. But hopefully the comments explain it well enough.

TheBlueMatt · 2025-04-04T13:29:43Z

Well the test is quite compelling :)

carlaKC

post-merge review, lgtm!

carlaKC · 2025-04-04T18:13:40Z


+	// In the best case, paths can be up to 27 hops. But attribution data can only be conveyed back to the sender from
+	// the first 20 hops. Determine the number of hops to be used for attribution data.
+	let attributable_hop_count = usize::min(path.hops.len(), MAX_HOPS);


Still find myself wishing that the caller didn't need to concern itself with this level of detail about attributable faliures, but don't see an obvious way to improve it.

At the point of interpreting the failure message, there isn't much we can do about it. But one open question is whether we should limit pathfinding to 20 hops instead of 27 in pathfinding, so that we're sure every failure is attributable?

I doubt it's worth it. In the rare case that we are sending more than 20 hops it's probably because someone is doing something insane where they have a many-hop route hint, in which case it's not useful to get attribution data beyond the first handful.

joostjager · 2025-04-04T12:16:09Z

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

Fixes#3708

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

ldk-reviews-bot · 2025-04-04T12:16:12Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt

Cool, thanks. Gonna go ahead and land this because its basically trivial.

ldk-reviews-bot · 2025-04-04T12:52:16Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

joostjager · 2025-04-04T13:23:32Z

Well not completely trivial although the code change seems to be. I had to look twice at the fix myself. But hopefully the comments explain it well enough.

TheBlueMatt · 2025-04-04T13:29:43Z

Well the test is quite compelling :)

carlaKC

post-merge review, lgtm!

carlaKC · 2025-04-04T18:13:40Z


+	// In the best case, paths can be up to 27 hops. But attribution data can only be conveyed back to the sender from
+	// the first 20 hops. Determine the number of hops to be used for attribution data.
+	let attributable_hop_count = usize::min(path.hops.len(), MAX_HOPS);


Still find myself wishing that the caller didn't need to concern itself with this level of detail about attributable faliures, but don't see an obvious way to improve it.

At the point of interpreting the failure message, there isn't much we can do about it. But one open question is whether we should limit pathfinding to 20 hops instead of 27 in pathfinding, so that we're sure every failure is attributable?

I doubt it's worth it. In the rare case that we are sending more than 20 hops it's probably because someone is doing something insane where they have a many-hop route hint, in which case it's not useful to get attribution data beyond the first handful.

joostjager · 2025-04-04T12:16:09Z

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

Fixes#3708

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

ldk-reviews-bot · 2025-04-04T12:16:12Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt

Cool, thanks. Gonna go ahead and land this because its basically trivial.

ldk-reviews-bot · 2025-04-04T12:52:16Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

joostjager · 2025-04-04T13:23:32Z

Well not completely trivial although the code change seems to be. I had to look twice at the fix myself. But hopefully the comments explain it well enough.

TheBlueMatt · 2025-04-04T13:29:43Z

Well the test is quite compelling :)

carlaKC

post-merge review, lgtm!

carlaKC · 2025-04-04T18:13:40Z


+	// In the best case, paths can be up to 27 hops. But attribution data can only be conveyed back to the sender from
+	// the first 20 hops. Determine the number of hops to be used for attribution data.
+	let attributable_hop_count = usize::min(path.hops.len(), MAX_HOPS);


Still find myself wishing that the caller didn't need to concern itself with this level of detail about attributable faliures, but don't see an obvious way to improve it.

At the point of interpreting the failure message, there isn't much we can do about it. But one open question is whether we should limit pathfinding to 20 hops instead of 27 in pathfinding, so that we're sure every failure is attributable?

I doubt it's worth it. In the rare case that we are sending more than 20 hops it's probably because someone is doing something insane where they have a many-hop route hint, in which case it's not useful to get attribution data beyond the first handful.

joostjager · 2025-04-04T12:16:09Z

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

Fixes#3708

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

ldk-reviews-bot · 2025-04-04T12:16:12Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt

Cool, thanks. Gonna go ahead and land this because its basically trivial.

ldk-reviews-bot · 2025-04-04T12:52:16Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

joostjager · 2025-04-04T13:23:32Z

Well not completely trivial although the code change seems to be. I had to look twice at the fix myself. But hopefully the comments explain it well enough.

TheBlueMatt · 2025-04-04T13:29:43Z

Well the test is quite compelling :)

carlaKC

post-merge review, lgtm!

carlaKC · 2025-04-04T18:13:40Z


+	// In the best case, paths can be up to 27 hops. But attribution data can only be conveyed back to the sender from
+	// the first 20 hops. Determine the number of hops to be used for attribution data.
+	let attributable_hop_count = usize::min(path.hops.len(), MAX_HOPS);


Still find myself wishing that the caller didn't need to concern itself with this level of detail about attributable faliures, but don't see an obvious way to improve it.

At the point of interpreting the failure message, there isn't much we can do about it. But one open question is whether we should limit pathfinding to 20 hops instead of 27 in pathfinding, so that we're sure every failure is attributable?

I doubt it's worth it. In the rare case that we are sending more than 20 hops it's probably because someone is doing something insane where they have a many-hop route hint, in which case it's not useful to get attribution data beyond the first handful.

joostjager · 2025-04-04T12:16:09Z

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

Fixes#3708

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

ldk-reviews-bot · 2025-04-04T12:16:12Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt

Cool, thanks. Gonna go ahead and land this because its basically trivial.

ldk-reviews-bot · 2025-04-04T12:52:16Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

joostjager · 2025-04-04T13:23:32Z

Well not completely trivial although the code change seems to be. I had to look twice at the fix myself. But hopefully the comments explain it well enough.

TheBlueMatt · 2025-04-04T13:29:43Z

Well the test is quite compelling :)

carlaKC

post-merge review, lgtm!

carlaKC · 2025-04-04T18:13:40Z


+	// In the best case, paths can be up to 27 hops. But attribution data can only be conveyed back to the sender from
+	// the first 20 hops. Determine the number of hops to be used for attribution data.
+	let attributable_hop_count = usize::min(path.hops.len(), MAX_HOPS);


Still find myself wishing that the caller didn't need to concern itself with this level of detail about attributable faliures, but don't see an obvious way to improve it.

At the point of interpreting the failure message, there isn't much we can do about it. But one open question is whether we should limit pathfinding to 20 hops instead of 27 in pathfinding, so that we're sure every failure is attributable?

I doubt it's worth it. In the rare case that we are sending more than 20 hops it's probably because someone is doing something insane where they have a many-hop route hint, in which case it's not useful to get attribution data beyond the first handful.

joostjager · 2025-04-04T12:16:09Z

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

Fixes#3708

An out of bound error could occur when attribution data was provided by the downstream hop for an exceptionally long route. The fix limits the verification of attribution data hmacs up to hop 20. If the sender chooses to use a longer route, failures in the final part of the route won't be attributable.

ldk-reviews-bot · 2025-04-04T12:16:12Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt

Cool, thanks. Gonna go ahead and land this because its basically trivial.

ldk-reviews-bot · 2025-04-04T12:52:16Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

joostjager · 2025-04-04T13:23:32Z

Well not completely trivial although the code change seems to be. I had to look twice at the fix myself. But hopefully the comments explain it well enough.

TheBlueMatt · 2025-04-04T13:29:43Z

Well the test is quite compelling :)

carlaKC

post-merge review, lgtm!

carlaKC · 2025-04-04T18:13:40Z


+	// In the best case, paths can be up to 27 hops. But attribution data can only be conveyed back to the sender from
+	// the first 20 hops. Determine the number of hops to be used for attribution data.
+	let attributable_hop_count = usize::min(path.hops.len(), MAX_HOPS);


Still find myself wishing that the caller didn't need to concern itself with this level of detail about attributable faliures, but don't see an obvious way to improve it.

At the point of interpreting the failure message, there isn't much we can do about it. But one open question is whether we should limit pathfinding to 20 hops instead of 27 in pathfinding, so that we're sure every failure is attributable?

I doubt it's worth it. In the rare case that we are sending more than 20 hops it's probably because someone is doing something insane where they have a many-hop route hint, in which case it's not useful to get attribution data beyond the first handful.

joostjager requested a review from TheBlueMatt April 4, 2025 12:19

TheBlueMatt approved these changes Apr 4, 2025
View reviewed changes