#34486 net: Reduce local network activity when networkactive=0
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #34486 net: Reduce local network activity when networkactive=0 | Needs rebase | 0 (+1) | Mild | 139 + 217 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
This pull request wires NAT-PMP port mapping and Tor control port connections into CConnman's network-active lifecycle. When networking is disabled via `-networkactive=0` or the `setnetworkactive` RPC, NAT-PMP mapping stops and the Tor controller idles without attempting connection or reconnection. Auxiliary network lifecycle updates and subsystem callbacks are serialized under a mutex so state changes cannot race across RPC and GUI calls.
Problem: When `-networkactive=0` is set or networking is toggled off, bitcoind continues performing local network activity: NAT-PMP continues querying local gateways, and the Tor controller repeatedly attempts to connect or reconnect to the Tor control port in the background, violating user expectations of a completely disabled network state.
Evidence of importance:
- Fixes linked issue #34190 ('bitcoind shouldn't perform network activity if the option networkactive=0')
- Author description: 'When networkactive=0 is set, NAT-PMP port mapping and Tor control connections still run in the background, mapping ports and logging retry attempts despite the node being "inactive."'
Risk of a mistake: medium (p2p). Flaws in the synchronization of network active state or thread interruption in mapport and torcontrol could cause deadlocks or prevent Tor hidden services and port mapping from initializing properly.
Changed since the previous assessment: Added commit 799116c005 serializing auxiliary network lifecycle callbacks under `m_network_active_mutex` to protect against concurrent RPC and GUI preference toggles, handled reconnect backoff resetting during rapid off/on cycles while sleeping, and added unit and functional tests covering these edge cases.
Reviewability
Ready · Needs rebase
Conflicts with master; review the logic now, the rebase is mechanical.
Waiting on (from the thread read):
- author: address fjahr's review comments 2026-09-28
Agreement
Mild
objection that needs an answer has none yet (fjahr)
Thread notes: Author willcl-ark acknowledged fjahr's review in c:5874364163, deferring detailed responses. All of fjahr's claims remain open, waiting on the author.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| bradleystachurski (none) | objection, correctness | re-enabling network after backoff has grown continues from stale backoff value instead of resetting | fixed · fix pushed · author replied in c:3840137835; fixed by c:3840137835 · v1, not yet re-checked | answered | yes | 2026-02-02: 'TorController::SetNetworkActive doesn't reset reconnect_timeout, so re-enabling network after backoff has grown continues from the stale value.' Status from: 2026-02-03: willcl-ark: 'Thanks @bradleystachurski that's a nice suggestion which I've taken in 745aed37224102888332823aadc6d766e701aa0b' |
| winterrdog (contributor) | objection, correctness | rapid off/on toggling during reconnect backoff sleep increases the backoff delay instead of resetting it back to 1.0s | answered · author pushed fix using a network-change counter to reset reconnect delay on rapid toggles · v1, not yet re-checked | answered | yes | 2026-08-18: 'punishing the node with a longer reconnect delay instead of resetting it back to 1.0s despite a fresh reactivation occurring' Status from: 2026-09-22: willcl-ark: '' |
| fjahr (member) | suggestion, approach | avoid sending real requests to the local router in functional tests and rely on unit tests instead | open | answered | no | 2026-09-27: 'I am not sure if we should add this or maybe rather avoid it and rely on unit tests for anything other than the negative test above.' |
| fjahr (member) | suggestion, approach | avoid sleeps in functional tests by specifying expected_msgs in assert_debug_log | open | answered | no | 2026-09-27: 'I would rather like to avoid the `sleep`s here and below.' |
| fjahr (member) | objection, correctness | test assertion checks for 3.38s backoff step which has already passed due to earlier assertion instead of checking subsequent steps | open | answered | no | 2026-09-27: 'This should check for the next backoff steps rather than 3.38s since that must have already passed due to the assert above.' |
| fjahr (member) | suggestion, approach | use a wake signal rather than polling and counter logic to handle reactivation during Tor reconnect backoff | open | answered | no | 2026-09-27: 'if we want to avoid this issue I think it would be cleaner to use a wake signal than polling' |
| fjahr (member) | question, approach | why ensure_for is needed when the status would not otherwise change | open | answered | no | 2026-09-27: 'I don't understand the purpose of this `ensure_for`, why would the status change if nothing otherwise happens?' |
| fjahr (member) | suggestion, usefulness | remove or replace unhelpful assert_debug_log on setnetworkactive that prints unconditionally even if status change fails | open | answered | no | 2026-09-27: 'These `assert_debug_log` on `setnetworkactive` are not helpful since the log is printed unconditionally even if the status change fails' |
| fjahr (member) | suggestion, scope | move Tor check from p2p_private_broadcast.py to feature_torcontrol.py or remove it if already covered | open | answered | no | 2026-09-27: 'I'm not loving that this adds this unrelated check to the private broadcast test. Even if this means adding more code I think this should go in the torcontrol test.' |
Support:
- sedited (member) (Concept ACK, 2026-02-02): Concept ACK [not substantive]
- fanquake (member) (Concept ACK, 2026-02-03): Concept ACK [not substantive]
- Jhackman2019 (none) (Concept ACK, 2026-02-07) [tested]: Tested this on my Pi 5 (ARM64, Debian Bookworm). Builds clean, all the networking-related tests pass
- brunoerg (member) (Concept ACK, 2026-02-17): Concept ACK [not substantive]
- brunoerg (member) (ACK, 2026-07-14): ACK e5ed5f279b471c533ea94140aadcb447c480f473 [not substantive]
- winterrdog (contributor) (Approach ACK, 2026-08-18): approach ACK 48f1321a603f565b8264bb2adb9d0bb4c54a68c6 [not substantive]
- fjahr (member) (Concept ACK, 2026-09-27): Concept ACK (somehow this wasn't on my radar until now) [not substantive]
Participants:
- bradleystachurski (none, objection, 1 statements 2026-02-02 to 2026-02-02): tested on Linux; pointed out reconnect backoff was not reset on reactivation (resolved)
- sedited (member, support, 1 statements 2026-02-02 to 2026-02-02): Concept ACK
- fanquake (member, support, 1 statements 2026-02-03 to 2026-02-03): Concept ACK
- Jhackman2019 (none, support, 1 statements 2026-02-07 to 2026-02-07): Concept ACK after testing networking-related tests on ARM64 Pi 5
- brunoerg (member, support, 6 statements 2026-02-17 to 2026-07-14): Concept ACK, tested NAT-PMP and Tor traffic teardown in Docker, and approved (ACK)
- winterrdog (contributor, objection, 3 statements 2026-08-15 to 2026-08-18): Approach ACK, but noted reconnect backoff increases rather than resetting on rapid off/on cycles during sleep
- fjahr (member, objection, 12 statements 2026-09-27 to 2026-09-27): Concept ACK, but noted an ineffective test assertion checking an already-passed backoff step and suggested wake signal over polling/counter
Files
217 lines under test/bench/ci.
- test/functional/feature_torcontrol.py +81/-0
- src/test/net_tests.cpp +76/-0
- src/torcontrol.cpp +55/-3
- test/functional/feature_mapport.py +52/-0
- src/net.cpp +33/-4
- src/mapport.cpp +15/-18
- src/net.h +18/-3
- src/torcontrol.h +9/-0
- src/init.cpp +5/-3
- test/functional/p2p_private_broadcast.py +7/-0
- src/node/interfaces.cpp +3/-2
- src/mapport.h +1/-1
- test/functional/test_runner.py +1/-0
Card
The short text the ranking pass compares across PRs.
PR #34486 wires the lifecycle of NAT-PMP port mapping and Tor control connections into CConnman's network-active state so that both pause when networking is disabled and resume upon reactivation. It fixes issue #34190, where bitcoind continued background gateway queries and Tor control reconnect attempts despite running with -networkactive=0. Reconnection backoff is reset on reactivation, and concurrent state changes between RPC and GUI are serialized to avoid desynchronization.
Data
thread read through 2026-09-28 (35 statements), head 799116c005
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: P2P, Needs rebase · draft
Categories
P2P
P3 · bug fix (ranking pass: Local-only misbehaviour of an operator toggle; ranks below relay and address correctness fixes.)
Fixes an issue where disabling networking via `-networkactive=0` leaves background NAT-PMP port mapping queries and Tor control port reconnection attempts active. As noted in issue #34190, background retries and port mappings continued despite networking being marked inactive, violating expected isolation.
Membership: Has the P2P label and modifies connection management, Tor control, and port mapping in src/net.cpp, src/torcontrol.cpp, and src/mapport.cpp.
Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0