Skip to content
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

askrene: fix integer overflow for dummy channels #8129

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Lagrang3
Copy link
Collaborator

@Lagrang3 Lagrang3 commented Feb 28, 2025

We may introduce high capacity channels in askrene to represent problems
with multiple destinations (eg. multiple blinded paths) or to solve
self-payments. The integer computation of the deliverable amount through
these channels (I tested this with a channel with 21M bitcoin) would
fail due to an integer overflow in the function amount_msat_sub_fee,
21M BTC = 21 x 10^17 msat, which overflows u64 integers when multiplied
by 10^6. We have fixed amount_msat_sub_fee operations, so that it
doesn't overflow.

Basically we had the following integer operation:

floor(x * a/b)

where a<b and a=10^6, so the result fits in u64. The problem is that we first compute a*x and then divide by b.
a*x can overflow.
We fixed the operation by splitting x = q_x * b + r_x where q_x = floor(x/b) and r_x = x mod b, so that
our computation reduces to:

a * q_x + floor(a*r_x / b)

The value a*q_x <= x does not overflow. So the only dangerous operation is a*r_x,
but a*r_x < a*b, so if a*b does not overflow then the entire operation is safe to compute.
In our case a=10^6 and b = 10^6 + ppm, so unless ppm ~ 10^12, which is an absurd value,
the above operation is guaranteed to be successful.

@Lagrang3 Lagrang3 force-pushed the askrene-bad-routes branch from 27fe781 to 9d58875 Compare March 1, 2025 09:05
@Lagrang3 Lagrang3 changed the title [WIP] askrene: fix routing through dummy channels askrene: fix integer overflow for dummy channels Mar 1, 2025
@Lagrang3 Lagrang3 marked this pull request as ready for review March 1, 2025 09:16
@Lagrang3 Lagrang3 requested a review from rustyrussell March 1, 2025 09:16
@Lagrang3 Lagrang3 force-pushed the askrene-bad-routes branch from 9d58875 to 2e46a89 Compare March 2, 2025 08:40
Lagrang3 added 2 commits March 2, 2025 11:26
We may introduce high capacity channels in askrene to represent problems
with multiple destinations (eg. multiple blinded paths) or to solve
self-payments. The integer computation of the deliverable amount through
these channels (I tested this with a channel with 21M bitcoin) would
fail due to an integer overflow in the function `amount_msat_sub_fee`,
21M BTC = 21 x 10^17 msat, which overflows u64 integers when multiplied
by 10^6. We have fixed `amount_msat_sub_fee` operations, so that it
doesn't overflow.

Changelog-Fixed: askrene: fixed routing in high capacity channels.

Signed-off-by: Lagrang3 <[email protected]>
@Lagrang3 Lagrang3 force-pushed the askrene-bad-routes branch from 2e46a89 to e0004fa Compare March 2, 2025 10:28
@Lagrang3
Copy link
Collaborator Author

Lagrang3 commented Mar 2, 2025

Optionally, we can use the same integer decomposition to compute amount_msat_fee.

@rustyrussell rustyrussell added this to the v25.05 milestone Mar 2, 2025
@rustyrussell
Copy link
Contributor

Ack e0004fa

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.

2 participants