Skip to content

Conversation

@rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Sep 5, 2025

Based on #8688: this is just the last two commits.

This attempts to solve a problem we have with Phoenix clients:

This payment has been split in two many parts by the sender: 31 parts vs max 6 parts allowed for on-the-fly funding.

The problem is that we don't have any way in bolt11 or bolt12 to
specify the maximum number of HTLCs.

As a workaround, we start by restricting askrene to 6 parts if the
node is not openly reachable, and if it struggles, we remove the
restriction. This would work much better if askrene handled maxparts
more completely!

See-Also: #8331

@rustyrussell rustyrussell added this to the v25.12 milestone Sep 5, 2025
@rustyrussell rustyrussell force-pushed the guilt/xpay-phoenix-detect branch from a353419 to cccac43 Compare September 5, 2025 05:41
@Lagrang3 Lagrang3 self-requested a review September 5, 2025 06:35
@rustyrussell rustyrussell marked this pull request as draft September 6, 2025 00:27
@madelinevibes madelinevibes modified the milestones: v25.12, v26.03 Nov 12, 2025
@rustyrussell rustyrussell modified the milestones: v26.03, v25.12 Nov 12, 2025
@rustyrussell rustyrussell force-pushed the guilt/xpay-phoenix-detect branch from cccac43 to faa6643 Compare November 14, 2025 00:12
@rustyrussell rustyrussell marked this pull request as ready for review November 14, 2025 01:15
I added amount_msat_accumulate for the "a+=b" case, but I was struggling
with a name for the subtractive equivalent.  After some prompting, ChatGPT
suggested deduct.

Signed-off-by: Rusty Russell <[email protected]>
This is not worth optimizing that I can see.  Using a non-debug build I get
the following times for tests/test_askrene.py::test_real_data

Before:
	143 seconds

After:
	141 seconds.

Signed-off-by: Rusty Russell <[email protected]>
We've already freed the working_ctx, and the fail path does that again.

Signed-off-by: Rusty Russell <[email protected]>
It can only fail on overflow, but if it did, the fail path frees working_ctx
and returns "error_message".

Signed-off-by: Rusty Russell <[email protected]>
Make it calculate on demand.  This will be useful when we call it from elsewhere.

Signed-off-by: Rusty Russell <[email protected]>
We don't need the indexes array, we can use this directly.

We still set up the indexes array (for now) after we call this.

Signed-off-by: Rusty Russell <[email protected]>
This removes the index array from code after increase_flows()m, so we use the flows
array directly.

The next step will be to make increase_flows() use the flows array, and remove the
index array indirection entirely.

Signed-off-by: Rusty Russell <[email protected]>
We don't need to convert to strings, we can compare directly.  This removes the final
use of the index arrays.

This of course changes the order of returned routes, which alters test_real_biases, since
that biases against the final channel in the *first* route.

Took me far too long to diagnose that!

Signed-off-by: Rusty Russell <[email protected]>
We added _noidx versions of the sort functions, but now they're the only ones, we can
rename them to the old names.

Signed-off-by: Rusty Russell <[email protected]>
Rewrite it, so it properly takes into account interactions between flows
by using reservations.

Signed-off-by: Rusty Russell <[email protected]>
For 1, we use single-path.  For 0, reject.

Signed-off-by: Rusty Russell <[email protected]>
Now we simply call it at the end.  We need to check it hasn't violated fee maxima, but
otherwise it's simple.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Fixed: Plugins: `askrene` now handles limits on number of htlcs much more gracefully.
…e can't route.

This attempts to solve a problem we have with Phoenix clients:

	This payment has been split in two many parts by the sender: 31 parts vs max 6 parts allowed for on-the-fly funding.

The problem is that we don't have any way in bolt11 or bolt12 to
specify the maximum number of HTLCs.

As a workaround, we start by restricting askrene to 6 parts if the
node is not openly reachable, and if it struggles, we remove the
restriction.  This would work much better if askrene handled maxparts
more completely!

See-Also: ElementsProject#8331
Changelog-Fixed: `xpay` will not try to send too many HTLCs through unknown channels (6, as that is Phoenix's limit) unless it has no choice
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell rustyrussell force-pushed the guilt/xpay-phoenix-detect branch from faa6643 to 67d7d4b Compare November 14, 2025 01:15
Copy link
Collaborator

@Lagrang3 Lagrang3 left a comment

Choose a reason for hiding this comment

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

This is straightforward.
I only have some minor requests on the previous PR #8688.

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.

3 participants