lnpeermgr: add_peer: fix check if proxy enabled
What the developer wrote
Authored by f321x
lnpeermgr: add_peer: fix check if proxy enabled
LNPeerManager.add_peer would only check if self.network.proxy is set,
which it is always as Network is initialized with self.proxy =
ProxySettings(). Instead it should check if proxy is set and enabled.
What changed, and why it matters
This commit fixes a bug in Electrum's Lightning Network peer manager. Previously, the code thought a proxy was always configured because the proxy object always existed, even when disabled. As a result, it skipped DNS resolution and could behave incorrectly when connecting to peers, especially for .onion addresses which require a proxy. The fix checks whether the proxy is actually enabled, not just present.
Review is appropriate. The fix is small and targeted; ensure it is backported to supported branches and that the new regression tests pass. No immediate emergency response is indicated, but users relying on Lightning over Tor should verify proxy settings are correctly honored.
Security signals we found
Logic error in proxy-enabled check could cause DNS leak or incorrect routing behavior
Fix prevents skipping DNS resolution when proxy is disabled
Fix ensures .onion addresses are rejected when no proxy is actually enabled
Regression tests added for proxy-disabled and hostname resolution failure cases
Evidence from the diff
LNPeerManager.add_peer() in electrum/lnworker.py previously checked if not self.network.proxy to decide whether to perform DNS resolution. However, Network initializes self.proxy = ProxySettings() unconditionally, so the object is always truthy/non-None. The correct check is if not self.network.proxy or not self.network.proxy.enabled. The patch also updates tests to set self.proxy = ProxySettings() on MockNetwork and adds a new test file verifying that .onion addresses are rejected when no proxy is enabled and that hostname resolution failures produce proper errors.
Changed components
electrum/lnworker.py (LNPeerManager.add_peer)electrum/network.py (ProxySettings, Network proxy initialization)tests/test_lnpeer.py (MockNetwork)tests/test_lnpeermgr.py (new regression tests)Inspect captured patch +63 / −2
diff --git a/electrum/lnworker.py b/electrum/lnworker.py
index 4270dad..f6b5035 100644
--- a/electrum/lnworker.py
+++ b/electrum/lnworker.py
@@ -588,7 +588,7 @@ class LNPeerManager(Logger, EventListener, NetworkRetryManager[LNPeerAddr]):
host, port, timestamp = self.choose_preferred_address(list(addrs))
port = int(port)
- if not self.network.proxy:
+ if not self.network.proxy or not self.network.proxy.enabled:
# Try DNS-resolving the host (if needed). This is simply so that
# the caller gets a nice exception if it cannot be resolved.
# (we don't do the DNS lookup if a proxy is set, to avoid a DNS-leak)
diff --git a/tests/test_lnpeer.py b/tests/test_lnpeer.py
index 6107a8b..be4ca3d 100644
--- a/tests/test_lnpeer.py
+++ b/tests/test_lnpeer.py
@@ -26,7 +26,7 @@ from electrum import bitcoin
from electrum import util
from electrum import constants
from electrum import bip32
-from electrum.network import Network
+from electrum.network import Network, ProxySettings
from electrum import simple_config, lnutil
from electrum.lnaddr import lnencode, LnAddr, lndecode
from electrum.bitcoin import COIN, sha256
@@ -71,6 +71,7 @@ class MockNetwork:
self.path_finder = LNPathFinder(self.channel_db)
self.lngossip = MockLNGossip()
self.tx_queue = asyncio.Queue()
+ self.proxy = ProxySettings()
self._blockchain = MockBlockchain()
def get_local_height(self):
diff --git a/tests/test_lnpeermgr.py b/tests/test_lnpeermgr.py
new file mode 100644
index 0000000..169f7dd
--- /dev/null
+++ b/tests/test_lnpeermgr.py
@@ -0,0 +1,60 @@
+import logging
+import os
+import socket
+import asyncio
+from unittest import mock
+
+from . import ElectrumTestCase
+
+from electrum.lntransport import ConnStringFormatError
+from electrum.logging import console_stderr_handler
+
+
+class TestLNPeerManager(ElectrumTestCase):
+ TESTNET = True
+
+ @classmethod
+ def setUpClass(cls):
+ super().setUpClass()
+ console_stderr_handler.setLevel(logging.DEBUG)
+
+ async def asyncSetUp(self):
+ lnwallet = self.create_mock_lnwallet(name='mock_lnwallet_anchors', has_anchors=True)
+ self.lnpeermgr = lnwallet.lnpeermgr
+ await super().asyncSetUp()
+
+ async def test_add_peer_conn_string_errors(self):
+ unknown_node_id = os.urandom(33)
+ peermgr = self.lnpeermgr
+ peermgr._add_peer = mock.Mock(side_effect=NotImplementedError)
+
+ # Trampoline enabled, unknown node (no address in trampolines)
+ channel_db = peermgr.network.channel_db
+ peermgr.network.channel_db = None
+ try:
+ with self.assertRaises(ConnStringFormatError) as cm:
+ await peermgr.add_peer(unknown_node_id.hex())
+ self.assertIn("Address unknown for node", str(cm.exception))
+ finally:
+ peermgr.network.channel_db = channel_db # re-set channel db
+
+ # Trampoline disabled, unknown node (no address in channel_db)
+ with mock.patch.object(peermgr.network.channel_db, 'get_node_addresses', return_value=[]):
+ with self.assertRaises(ConnStringFormatError) as cm:
+ await peermgr.add_peer(unknown_node_id.hex())
+ self.assertIn("Don't know any addresses for node", str(cm.exception))
+
+ # .onion address, but no proxy configured
+ onion_conn_str = unknown_node_id.hex() + "@somewhere.onion:9735"
+ self.assertFalse(peermgr.network.proxy.enabled)
+ with self.assertRaises(ConnStringFormatError) as cm:
+ await peermgr.add_peer(onion_conn_str)
+ self.assertIn(".onion address, but no proxy configured", str(cm.exception))
+
+ # Hostname does not resolve (getaddrinfo failed)
+ bad_host_conn_str = unknown_node_id.hex() + "@badhost:9735"
+ loop = asyncio.get_running_loop()
+ with mock.patch.object(loop, 'getaddrinfo', side_effect=socket.gaierror):
+ with self.assertRaises(ConnStringFormatError) as cm:
+ await peermgr.add_peer(bad_host_conn_str)
+ self.assertIn("Hostname does not resolve", str(cm.exception))
Why this scored 49/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.