net: Reduce local network activity when networkactive=0#34486
net: Reduce local network activity when networkactive=0#34486willcl-ark wants to merge 4 commits intobitcoin:masterfrom
Conversation
When -networkactive=0 is set, NAT-PMP port mapping was still running and attempting gateway queries. Move mapport lifecycle management into CConnman, which already owns SetNetworkActive, so it can control mapport based on both the -natpmp setting and network state. EnableMapPort is injected as a callback via CConnman::Options to avoid a circular dependency between net and mapport.
Add functional test to verify that PCP/NAT-PMP port mapping respects the networkactive state: - Does not run when started with -networkactive=0 - Starts when network is activated via setnetworkactive RPC - Stops when network is deactivated - Resumes when network is reactivated
|
This is an alternative to #34467. The key difference is that #34467 gates mapport and tor control at init time with if (networkactive) guards, which means those subsystems are permanently disabled for the lifetime of the process. This PR instead wires both subsystems into CConnman::SetNetworkActive so they follow the network state through the full lifecycle: starting with -networkactive=0 and later calling setnetworkactive true works as expected. I'm unsure how this will conflict with the work currently being done to remove libevent. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK |
bradleystachurski
left a comment
There was a problem hiding this comment.
Reviewed 35da4e0
Tested on Linux: started with -networkactive=0 -natpmp=1 -debug=net -debug=tor, confirmed no portmap/tor activity at startup, toggled via setnetworkactive, confirmed mapport thread start/stop and tor connection attempts follow network state.
Verified feature_mapport.py fails when the m_mapport callback is removed from SetNetworkActive.
nit: TorController::SetNetworkActive doesn't reset reconnect_timeout, so re-enabling network after backoff has grown continues from the stale value. Verified this fixes it:
diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp
index d5aeb55a72..934499d72b 100644
--- a/src/torcontrol.cpp
+++ b/src/torcontrol.cpp
@@ -677,6 +677,8 @@ void TorController::SetNetworkActive(bool set_active)
// Disconnect if currently connected
conn.Disconnect();
}
+ // Reset backoff when re-enabling network
+ reconnect_timeout = RECONNECT_TIMEOUT_START;
// Connect handles both cases: connects if active, reschedules if inactive
Connect();
}When -networkactive=0 is set, Tor control connection attempts were still running and logging retry messages. Wire tor control lifecycle to CConnman similar to mapport: inject EnableTorControl as a callback via CConnman::Options, called from Start() and SetNetworkActive(). TorController starts disconnected and waits for the callback from CConnman::Start() before connecting. The callback dispatches onto the tor control event loop via event_base_once rather than calling TorController::SetNetworkActive directly, since TorController state and libevent structures are owned by the event loop thread. This also means the gTorController pointer cannot be observed after destruction: once event_base_dispatch returns, no further callbacks execute.
35da4e0 to
745aed3
Compare
|
Thanks @bradleystachurski that's a nice suggestion which I've taken in 745aed3 I also reworked the |
|
Concept ACK |
|
I will address the LLM linter suggestion if I push again. |
|
Tested this on my Pi 5 (ARM64, Debian Bookworm). Builds clean, all the networking-related tests pass: Had to clear my Concept ACK |
|
Concept ACK |
| if (!reconnect) | ||
| return; | ||
|
|
||
| if (!m_network_active) { |
There was a problem hiding this comment.
Is there any way to test this condition? I've tried several scenarios manually, but haven't been able to achieve it.
However, checking that it is not connected to any Tor control port and then will retry was easier, could also check it on a functional test, e.g:
diff --git a/test/functional/p2p_private_broadcast.py b/test/functional/p2p_private_broadcast.py
index 4943841790..b163020e85 100755
--- a/test/functional/p2p_private_broadcast.py
+++ b/test/functional/p2p_private_broadcast.py
@@ -433,9 +433,11 @@ class P2PPrivateBroadcast(BitcoinTestFramework):
# the RPC should throw.
"-torcontrol=127.0.0.1:1",
"-listenonion",
+ "-debug=tor",
])
- assert_raises_rpc_error(-1, "none of the Tor or I2P networks is reachable",
- tx_originator.sendrawtransaction, hexstring=txs[0]["hex"], maxfeerate=0.1)
+ with tx_originator.assert_debug_log(['Not connected to Tor control port'], timeout=5):
+ assert_raises_rpc_error(-1, "none of the Tor or I2P networks is reachable",
+ tx_originator.sendrawtransaction, hexstring=txs[0]["hex"], maxfeerate=0.1)
There was a problem hiding this comment.
Thanksf or the review!
Yes I think this is hard to reach in a functional test, as conn.Disconnect() frees the bufferevent without firing the callback, so this guard only triggers if a libevent disconnect event was already queued when SetNetworkActive(false) runs.
It's mainly intended to be defensive against an event-loop ordering edge case, and even if it wasn't here, Connect() has its own m_network_active check, but this guard also prevents a misleading "retrying" log message.
Your p2p_private_broadcast.py change seems like good coverage for the reworked connection initiation path. Happy to include it.
Verify that the tor controller attempts reconnection to the control port by asserting the retry log message when started with an unreachable `-torcontrol` address. Co-authored-by: brunoerg <brunoerg@users.noreply.github.com>
Fixes #34190
When
networkactive=0is set, NAT-PMP port mapping and Tor control connectionsstill run in the background, mapping ports and logging retry attempts despite
the node being "inactive."
This wires both subsystems to CConnman::SetNetworkActive so they start and stop
with the network state:
mapport: EnableMapPort is injected as a callback via CConnman::Options.
SetNetworkActive and SetMapPortEnabled both gate on network state.
torcontrol: is injected similarly. The callback dispatches onto the tor event loop via
event_base_once to respect thread ownership.
EVLOOP_NO_EXIT_ON_EMPTYisused to keep the event loop alive without keepalive timers, so the controller starts disconnected and waits for CConnman::Start() to signal.