From 5a3ec45b1654c5445c8481b72c7a676f6a90a69d Mon Sep 17 00:00:00 2001 From: SomberNight Date: Mon, 29 Mar 2021 20:51:54 +0200 Subject: [PATCH] lnworker: fix another peer-handling race (related to prev commit, but really another bug) If we had two peers with the same pubkey (peer A in the process of teardown, peer B ~freshly connected), peer A might remove peer B from lnworker.peers via close_and_cleanup(). rm `close_and_cleanup()` call from reestablish_channel - it was added as a workaround for this bug (in 8b95b2127de767661ad2bac16afcf4c067a7dd54) before we understood the cause. --- electrum/lnpeer.py | 4 +++- electrum/lnworker.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/electrum/lnpeer.py b/electrum/lnpeer.py index 33d6eb926..7c2d696f8 100644 --- a/electrum/lnpeer.py +++ b/electrum/lnpeer.py @@ -478,6 +478,9 @@ class Peer(Logger): self.querying.set() def close_and_cleanup(self): + # note: This method might get called multiple times! + # E.g. if you call close_and_cleanup() to cause a disconnection from the peer, + # it will get called a second time in handle_disconnect(). try: if self.transport: self.transport.close() @@ -1081,7 +1084,6 @@ class Peer(Logger): elif we_are_ahead: self.logger.warning(f"channel_reestablish ({chan.get_id_for_log()}): we are ahead of remote! trying to force-close.") await self.lnworker.try_force_closing(chan_id) - self.close_and_cleanup() return chan.peer_state = PeerState.GOOD diff --git a/electrum/lnworker.py b/electrum/lnworker.py index f7e8fd3f2..e7a0a864d 100644 --- a/electrum/lnworker.py +++ b/electrum/lnworker.py @@ -316,7 +316,9 @@ class LNWorker(Logger, NetworkRetryManager[LNPeerAddr]): def peer_closed(self, peer: Peer) -> None: with self.lock: - self._peers.pop(peer.pubkey, None) + peer2 = self._peers.get(peer.pubkey) + if peer2 is peer: + self._peers.pop(peer.pubkey) def num_peers(self) -> int: return sum([p.is_initialized() for p in self.peers.values()])