lnwallet: fix rebalancing/pathfinding regression, add unittests #10653
Conversation
f321x
force-pushed
the
fix_gossip_rebalance
branch
from
56ad5fd to
b76d951
Compare
| async def _connect_two_lnwallets(alice: 'MockLNWallet', bob: 'MockLNWallet') -> tuple[PeerInTests, PeerInTests]: | ||
| """Mock transports and add the wallets as Peers to each others lnpeermgr.""" | ||
| t_ab, t_ba = transport_pair( | ||
| alice.node_keypair, | ||
| bob.node_keypair, | ||
| name1=f"{alice.name}->{bob.name}", | ||
| name2=f"{bob.name}->{alice.name}", | ||
| ) | ||
| peer_ab = PeerInTests(alice, bob.node_keypair.pubkey, t_ab) | ||
| peer_ba = PeerInTests(bob, alice.node_keypair.pubkey, t_ba) | ||
| alice.lnpeermgr._peers[bob.node_keypair.pubkey] = peer_ab | ||
| bob.lnpeermgr._peers[alice.node_keypair.pubkey] = peer_ba | ||
| await alice.lnpeermgr.taskgroup.spawn(peer_ab.main_loop()) | ||
| await bob.lnpeermgr.taskgroup.spawn(peer_ba.main_loop()) | ||
| return peer_ab, peer_ba | ||
|
|
||
|
|
||
| def _add_channels_to_peers( | ||
| peer_ab: 'PeerInTests', | ||
| peer_ba: 'PeerInTests', | ||
| *, | ||
| local_msat: Optional[int] = None, | ||
| remote_msat: Optional[int] = None, | ||
| ) -> tuple[Channel, Channel]: | ||
| """Create channels for given peers and mark them open""" | ||
| chan_ab, chan_ba = create_test_channels( | ||
| alice_lnwallet=peer_ab.lnworker, | ||
| bob_lnwallet=peer_ba.lnworker, | ||
| local_msat=local_msat, | ||
| remote_msat=remote_msat, | ||
| ) | ||
| peer_ab.lnworker._add_channel(chan_ab) | ||
| peer_ba.lnworker._add_channel(chan_ba) | ||
| chan_ab._state = ChannelState.FUNDED | ||
| chan_ba._state = ChannelState.FUNDED | ||
| peer_ab.mark_open(chan_ab) | ||
| peer_ba.mark_open(chan_ba) | ||
| return chan_ab, chan_ba |
There was a problem hiding this comment.
I understand why you are adding this, but I don't like it.
It is mostly just duplicating test_lnpeer.TestPeer.prepare_chans_and_peers_in_graph. We should de-dupe it somehow.
We could split that off into a new module and reuse it. Except we also need to extend graph_definition to allow multiple direct channels between a given pair of nodes - but that could come in handy for other things too anyway.
I hacked together something along those lines:
master...SomberNight:electrum:202605_tests_lnhelpers_2 (b8491bd)
but my approach is also a bit ugly :(
idk, what do you think?
There was a problem hiding this comment.
Yes I was already considering including the test in test_lnpeer.py but it felt misplaced to test the LNWallet logic there.
I looked at your approach, i think it makes sense to have a shared helper module as there are quite some similarities between the setups required in test_lnpeer and test_lnwallet. I cherry-picked it into this branch so we can move forward with this.
Should we move the graph definitions and the create_test_channels() function into lnhelpers.py as well? Or did you consider them too specific to move them into lnhelpers?
There was a problem hiding this comment.
Should we move the graph definitions
atm they are mostly used from lnpeer, we could wait to see if it spreads more
Should we move [...]
create_test_channels()
you're right, that seems to be used from all over
f321x
force-pushed
the
fix_gossip_rebalance
branch
from
b76d951 to
962dfaa
Compare
Don't exclude r_tags for frozen channels from the route creation in create_route_for_single_htlc if the start_node of the routing hint is unequal to our node id. When doing a rebalance us (chan_1) -> bob -> us (chan_2, frozen for sending), we would exclude the invoice r_tag for chan_2 because the chan is our chan and frozen for sending, resulting in us being unable to find a route back to us trough bob. This is a regression from spesmilo#9692 (964ffbd).
Don't allow rebalancing through a channel if it is frozen for the direction required for the rebalance. If this is allowed by the gui it creates opaque failures as then pathfinding potentially fails downstream.
Add some unittest coverage for the channel rebalance flow. Co-authored-by: SomberNight <somber.night@protonmail.com>
f321x
force-pushed
the
fix_gossip_rebalance
branch
from
962dfaa to
b183a52
Compare
2 participants
Don't exclude r_tags for frozen channels from the route creation in
create_route_for_single_htlc if the start_node of the routing hint is not our node id.
When doing a rebalance from us (chan_1) -> bob -> us (chan_2, frozen for sending),
we would exclude the invoice r_tag for chan_2 because the chan
is our chan and frozen for sending, resulting in us being unable to find a route back to us trough bob.
This is a regression from #9692 (964ffbd).
Also prevents rebalancing if the required channel direction is frozen as this can lead to
opaque failures somewhere downstream (e.g. in the pathfinding).