Skip to content

Commit 1eb5997

Browse files
committed
suggest_splits: don't exclude single part configs for too small multi
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 #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.
1 parent 151b64d commit 1eb5997

File tree

3 files changed

+55
-18
lines changed

3 files changed

+55
-18
lines changed

electrum/lnworker.py

+16-17
Original file line numberDiff line numberDiff line change
@@ -1915,20 +1915,22 @@ def suggest_peer(self) -> Optional[bytes]:
19151915
else:
19161916
return random.choice(list(hardcoded_trampoline_nodes().values())).pubkey
19171917

1918-
def suggest_splits(
1918+
def suggest_payment_splits(
19191919
self,
19201920
*,
19211921
amount_msat: int,
19221922
final_total_msat: int,
19231923
my_active_channels: Sequence[Channel],
19241924
invoice_features: LnFeatures,
1925-
r_tags,
1925+
r_tags: Sequence[Sequence[Sequence[Any]]],
1926+
receiver_pubkey: bytes,
19261927
) -> List['SplitConfigRating']:
19271928
channels_with_funds = {
19281929
(chan.channel_id, chan.node_id): int(chan.available_to_spend(HTLCOwner.LOCAL))
19291930
for chan in my_active_channels
19301931
}
1931-
self.logger.info(f"channels_with_funds: {channels_with_funds}")
1932+
have_direct_channel = any(chan.node_id == receiver_pubkey for chan in my_active_channels)
1933+
self.logger.info(f"channels_with_funds: {channels_with_funds}, {have_direct_channel=}")
19321934
exclude_single_part_payments = False
19331935
if self.uses_trampoline():
19341936
# in the case of a legacy payment, we don't allow splitting via different
@@ -1944,22 +1946,18 @@ def suggest_splits(
19441946
if invoice_features.supports(LnFeatures.BASIC_MPP_OPT) and not self.config.TEST_FORCE_DISABLE_MPP:
19451947
# if amt is still large compared to total_msat, split it:
19461948
if (amount_msat / final_total_msat > self.MPP_SPLIT_PART_FRACTION
1947-
and amount_msat > self.MPP_SPLIT_PART_MINAMT_MSAT):
1949+
and amount_msat > self.MPP_SPLIT_PART_MINAMT_MSAT
1950+
and not have_direct_channel):
19481951
exclude_single_part_payments = True
19491952

1950-
def get_splits():
1951-
return suggest_splits(
1952-
amount_msat,
1953-
channels_with_funds,
1954-
exclude_single_part_payments=exclude_single_part_payments,
1955-
exclude_multinode_payments=exclude_multinode_payments,
1956-
exclude_single_channel_splits=exclude_single_channel_splits
1957-
)
1953+
split_configurations = suggest_splits(
1954+
amount_msat,
1955+
channels_with_funds,
1956+
exclude_single_part_payments=exclude_single_part_payments,
1957+
exclude_multinode_payments=exclude_multinode_payments,
1958+
exclude_single_channel_splits=exclude_single_channel_splits
1959+
)
19581960

1959-
split_configurations = get_splits()
1960-
if not split_configurations and exclude_single_part_payments:
1961-
exclude_single_part_payments = False
1962-
split_configurations = get_splits()
19631961
self.logger.info(f'suggest_split {amount_msat} returned {len(split_configurations)} configurations')
19641962
return split_configurations
19651963

@@ -1989,12 +1987,13 @@ async def create_routes_for_payment(
19891987
chan.is_active() and not chan.is_frozen_for_sending()]
19901988
# try random order
19911989
random.shuffle(my_active_channels)
1992-
split_configurations = self.suggest_splits(
1990+
split_configurations = self.suggest_payment_splits(
19931991
amount_msat=amount_msat,
19941992
final_total_msat=paysession.amount_to_pay,
19951993
my_active_channels=my_active_channels,
19961994
invoice_features=paysession.invoice_features,
19971995
r_tags=paysession.r_tags,
1996+
receiver_pubkey=paysession.invoice_pubkey,
19981997
)
19991998
for sc in split_configurations:
20001999
is_multichan_mpp = len(sc.config.items()) > 1

electrum/mpp_split.py

+5
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,11 @@ def suggest_splits(
167167
if config.total_config_amount() != amount_msat:
168168
raise NoPathFound('Cannot distribute payment over channels.')
169169
if target_parts > 1 and config.is_any_amount_smaller_than_min_part_size():
170+
if target_parts == 2:
171+
# if there are already too small parts at the first split excluding single
172+
# part payments may return too few good configurations, this will allow single part
173+
# payments for many payments (also larger ones) but they still get ranked by rate_configs
174+
exclude_single_part_payments = False
170175
continue
171176
assert config.total_config_amount() == amount_msat
172177
configs.append(config)

tests/test_lnpeer.py

+34-1
Original file line numberDiff line numberDiff line change
@@ -323,7 +323,7 @@ async def create_routes_from_invoice(self, amount_msat: int, decoded_invoice: Ln
323323
is_forwarded_htlc = LNWallet.is_forwarded_htlc
324324
notify_upstream_peer = LNWallet.notify_upstream_peer
325325
_force_close_channel = LNWallet._force_close_channel
326-
suggest_splits = LNWallet.suggest_splits
326+
suggest_payment_splits = LNWallet.suggest_payment_splits
327327
register_hold_invoice = LNWallet.register_hold_invoice
328328
unregister_hold_invoice = LNWallet.unregister_hold_invoice
329329
add_payment_info_for_hold_invoice = LNWallet.add_payment_info_for_hold_invoice
@@ -1556,6 +1556,39 @@ def prepare_chans_and_peers_in_graph(self, graph_definition) -> Graph:
15561556
print(f" {keys[a].pubkey.hex()}")
15571557
return graph
15581558

1559+
async def test_payment_direct_channel(self):
1560+
graph = self.prepare_chans_and_peers_in_graph(self.GRAPH_DEFINITIONS['line_graph'])
1561+
peers = graph.peers.values()
1562+
# use same MPP_SPLIT_PART_FRACTION as in LNWallet
1563+
graph.workers['bob'].MPP_SPLIT_PART_FRACTION = 0.2
1564+
async def pay(lnaddr, pay_req):
1565+
self.assertEqual(PR_UNPAID, graph.workers['alice'].get_payment_status(lnaddr.paymenthash))
1566+
result, log = await graph.workers['bob'].pay_invoice(pay_req)
1567+
self.assertTrue(result)
1568+
self.assertEqual(PR_PAID, graph.workers['alice'].get_payment_status(lnaddr.paymenthash))
1569+
async def f():
1570+
async with OldTaskGroup() as group:
1571+
for peer in peers:
1572+
await group.spawn(peer._message_loop())
1573+
await group.spawn(peer.htlc_switch())
1574+
for peer in peers:
1575+
await peer.initialized
1576+
# test multiple iterations of the same payment as there is randomness involved
1577+
# in selecting outgoing channels, however a direct payment should always find a path
1578+
for attempt in range(15):
1579+
lnaddr, pay_req = self.prepare_invoice(
1580+
graph.workers['alice'],
1581+
amount_msat=21_000_000,
1582+
include_routing_hints=True,
1583+
invoice_features=LnFeatures.BASIC_MPP_OPT
1584+
| LnFeatures.PAYMENT_SECRET_REQ
1585+
| LnFeatures.VAR_ONION_REQ
1586+
)
1587+
await pay(lnaddr, pay_req)
1588+
raise PaymentDone()
1589+
with self.assertRaises(PaymentDone):
1590+
await f()
1591+
15591592
async def test_payment_multihop(self):
15601593
graph = self.prepare_chans_and_peers_in_graph(self.GRAPH_DEFINITIONS['square_graph'])
15611594
peers = graph.peers.values()

0 commit comments

Comments
 (0)