Skip to content

Use floor when computing max value of a path, not ceil #3755

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

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

When calculating the maximum contribution of a path to a larger route, we want to ensure we don't overshoot as that might cause us to violate a maximum value limit.

In 209cb2a, we started by calculating with ceil, which can trigger exactly that, so here we drop the extra addition, switching us to floor.

Fixes #3707

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Apr 29, 2025

👋 I see @jkczyz was un-assigned.
If you'd like another reviewer assignemnt, please click here.

When calculating the maximum contribution of a path to a larger
route, we want to ensure we don't overshoot as that might cause us
to violate a maximum value limit.

In 209cb2a, we started by
calculating with `ceil`, which can trigger exactly that, so here we
drop the extra addition, switching us to `floor`.

Found both by the `router` fuzzer as well as the
`generate_large_mpp_routes` test.
@TheBlueMatt TheBlueMatt force-pushed the 2025-04-routing-overshoot branch from a68e90c to 7ebdf45 Compare April 29, 2025 01:24
@ldk-reviews-bot ldk-reviews-bot requested a review from jkczyz April 29, 2025 01:31
@TheBlueMatt TheBlueMatt requested review from valentinewallace and removed request for jkczyz April 29, 2025 13:25
@TheBlueMatt
Copy link
Collaborator Author

Assigned val since it was her pr.

@TheBlueMatt TheBlueMatt requested review from jkczyz and removed request for jkczyz April 29, 2025 19:15
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Test fails as expected on main

@TheBlueMatt TheBlueMatt merged commit c4a5987 into lightningdevkit:main Apr 29, 2025
26 of 27 checks passed
@TheBlueMatt
Copy link
Collaborator Author

Backported in #3757

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Apr 30, 2025
v0.1.3 - Apr 30, 2025 - "Routing Unicode in 2025"

Bug Fixes
=========

 * `Event::InvoiceReceived` is now only generated once for each `Bolt12Invoice`
   received matching a pending outbound payment. Previously it would be provided
   each time we received an invoice, which may happen many times if the sender
   sends redundant messages to improve success rates (lightningdevkit#3658).
 * LDK's router now more fully saturates paths which are subject to HTLC
   maximum restrictions after the first hop. In some rare cases this can result
   in finding paths when it would previously spuriously decide it cannot find
   enough diverse paths (lightningdevkit#3707, lightningdevkit#3755).

Security
========

0.1.3 fixes a denial-of-service vulnerability which cause a crash of an
LDK-based node if an attacker has access to a valid `Bolt12Offer` which the
LDK-based node created.
 * A malicious payer which requests a BOLT 12 Invoice from an LDK-based node
   (via the `Bolt12InvoiceRequest` message) can cause the panic of the
   LDK-based node due to the way `String::truncate` handles UTF-8 codepoints.
   The codepath can only be reached once the received `Botlt12InvoiceRequest`
   has been authenticated to be based on a valid `Bolt12Offer` which the same
   LDK-based node issued (lightningdevkit#3747, lightningdevkit#3750).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants