Skip to content

ln: don't exclude single part configs for too small payments #9753

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

f321x
Copy link
Member

@f321x f321x commented Apr 22, 2025

I noticed certain ln payments become very unreliable. These payments are ~21k sat, from gossip sender, with direct channel.

Due to the recent fix #9723 LNPathFinder.get_shortest_path_hops() will not use channels for the last hop of a route anymore that aren't also passed to it in my_sending_channels:

if edge_startnode == nodeA and my_sending_channels:  # payment outgoing, on our channel
    if edge_channel_id not in my_sending_channels:
        continue

Conceptually this makes sense as we only want to send through my_sending_channels, however if the only channel between us and the receiver is a direct channel that it got from the r_tag and it's not passed in my_sending_channel it's not able to construct a route now.

Previously this type of payment worked as get_shortest_path_hops() knew of the direct channel between us and nodeB from the invoices r_tag and then just used this channel as the route, even though it was (often) not contained in my_sending_channels.

my_sending_channels, passed in LNWallet.create_routes_for_payment() is either a single channel or all our channels, depending on is_multichan_mpp:

for sc in split_configurations:
	  is_multichan_mpp = len(sc.config.items()) > 1

This causes the unreliable, random behavior as LNWallet.suggest_splits() is supposed to exclude_single_part_payments if the amount is > MPP_SPLIT_PART_MINAMT_SAT (5000 sat). As mpp_split.py suggest_splits() is selecting channels randomly, and then removes single part configs, it sometimes doesn't return a single configuration, as it removes single part splits, and also removes multi part splits if a part is below 10 000 sat:

if target_parts > 1 and config.is_any_amount_smaller_than_min_part_size():
    continue

This will result in a fallback to allow single part payments:

split_configurations = get_splits()
if not split_configurations and exclude_single_part_payments:
    exclude_single_part_payments = False
    split_configurations = get_splits()

Then the payment works as all the channels are passed as my_sending_channels to LNWallet.create_routes_for_payment() for single part configs.

However sometimes this fallback doesn't happen, because a few mpp configurations found in the first call of suggest_splits have been kept, e.g. [10500, 10500], but at the same time most others have been removed as they exceeded the limit, e.g. [11001, 9999], (which happens sometimes with payments ~20k sat), this makes suggest_splits return very few usable channels/configurations (sometimes just one or two, even with way more available channels). This makes payments in this range unreliable as we do not retry to generate new split configurations if the following path finding fails with NoPathFound(), and there is no single part configuration that allows the path finding to access all channels. Also this does not only affect direct channel payments, but all gossip payments in this amount range.

There seem to be multiple ways to fix this, i think one simple approach is to disable exclude_single_part_payments if the splitting loop already begins to sort out configs on the second iteration (the first split), as this indicates that the amount may be too small to split within the given limits, and prevents the issue of having only few valid splits returned and not going into the fallback. However this also results in increased usage of single part payments.

@@ -1556,6 +1556,39 @@ def prepare_chans_and_peers_in_graph(self, graph_definition) -> Graph:
print(f" {keys[a].pubkey.hex()}")
return graph

async def test_payment_direct_channel(self):
Copy link
Member

Choose a reason for hiding this comment

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

if possible, please move this test to TestPeertDirect, as it is not involving any forwarding

Copy link
Member Author

Choose a reason for hiding this comment

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

For the test to work properly the sending peer has to have multiple channels, so it tries to split the payment across the available channels. In TestPeerDirect there is only functionality to mock a single channel while TestPeerForwarding can create the mock Graph in which bob has more channels.
Alternatively i could move def prepare_chans_and_peers_in_graph() into TestPeer so it is available in both child classes?

part configs

I noticed certain ln payments become very unreliable. These payments are ~21k sat, from gossip to gossip sender, with direct, unannounced channel.

Due to the recent fix spesmilo#9723 `LNPathFinder.get_shortest_path_hops()` will not use channels for the last hop of a route anymore that aren't also passed to it in `my_sending_channels`:

```python
if edge_startnode == nodeA and my_sending_channels:  # payment outgoing, on our channel
    if edge_channel_id not in my_sending_channels:
        continue
```

Conceptually this makes sense as we only want to send through `my_sending_channels`, however if the only channel between us and the receiver is a direct channel that we got from the r_tag and it's not passed in `my_sending_channel` it's not able to construct a route now.

Previously this type of payment worked as `get_shortest_path_hops()` knew of the direct channel between us and `nodeB` from the invoices r_tag and then just used this channel as the route, even though it was (often) not contained in `my_sending_channels`.

`my_sending_channels`, passed in `LNWallet.create_routes_for_payment()` is either a single channel or all our channels, depending on `is_multichan_mpp`:

```python
for sc in split_configurations:
	  is_multichan_mpp = len(sc.config.items()) > 1
```

This causes the unreliable, random behavior as `LNWallet.suggest_splits()` is supposed to `exclude_single_part_payments` if the amount is > `MPP_SPLIT_PART_MINAMT_SAT` (5000 sat).
As `mpp_split.py suggest_splits()` is selecting channels randomly, and then removes single part configs, it sometimes doesn't return a single configuration, as it removes single part splits, and also removes multi part splits if a part is below 10 000 sat:

```python
if target_parts > 1 and config.is_any_amount_smaller_than_min_part_size():
    continue
```

This will result in a fallback to allow single part payments:

```python
split_configurations = get_splits()
if not split_configurations and exclude_single_part_payments:
    exclude_single_part_payments = False
    split_configurations = get_splits()
```

Then the payment works as all our channels are passed as `my_sending_channels` to  `LNWallet.create_routes_for_payment()`.

However sometimes this fallback doesn't happen, because a few mpp configurations found in the first iteration of `suggest_splits` have been kept, e.g. [10500, 10500], but at the same time most others have been removed as they crossed the limit, e.g. [11001, 9999], (which happens sometimes with payments ~20k sat), this makes `suggest_splits` return very few usable channels/configurations (sometimes just one or two, even with way more available channels).
This makes payments in this range unreliable as we do not retry to generate new split configurations if the following path finding fails with `NoPathFound()`, and there is no single part configuration that allows the path finding to access all channels. Also this does not only affect direct channel payments, but all gossip payments in this amount range.

There seem to be multiple ways to fix this, i think one simple approach is to just disable `exclude_single_part_payments` if the splitting loop already begins to sort out configs on the second iteration (the first split), as this indicates that the amount may be too small to split within the given limits, and prevents the issue of having only few valid splits returned and not going into the fallback. However this also results in increased usage of single part payments.
@f321x f321x force-pushed the debug_ln_payment_failure branch 2 times, most recently from b522b31 to 1eb5997 Compare May 7, 2025 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants