#35750 addrman: make `m_last_good` network-specific
https://github.com/bitcoin/bitcoin/pull/35750 · · +61/-4 in 3 files, 2 commits · labels: P2P
Goal
- Prevent nodes from incorrectly penalizing peers on one network when local reachability fails on another
- Keep valid peer addresses from being marked terrible and dropped from relay due to local network outages
This pull request makes `m_last_good` in AddrMan network-specific rather than global, and initializes it to zero. It ensures that failed connection attempts against an address on one network (such as IPv6 or Tor) are only counted if the node has recently established a successful connection on that same network.
Problem: Currently, `m_last_good` is global, meaning successful connections on IPv4 update `m_last_good` across all networks. If a node temporarily or permanently loses reachability on IPv6 or Tor while IPv4 continues working, failed attempts on those networks are counted against remote addresses, eventually marking them terrible and halting their relay even though the fault is local.
Category: P2P (#17 of 65)
P3 · bug fix
- P3 because it fixes an address-management bug where local outages degrade remote peer scores
- Improves address relay quality for nodes running with partial network connectivity
- Affects only nodes with isolated network failures and does not fix an urgent security vulnerability
P3 because it fixes an address-management bug where local network outages pollute addrman peer scoring. While it improves address relay health for nodes with partial connectivity, it does not address an urgent denial-of-service or network partition threat.
Membership: Modifies AddrMan's failure counting and address reachability logic in src/addrman.cpp.
Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0
Reviewability: Ready
- Ready to review
- Small diff with unit tests, clean rebase, and all discussion points addressed
The PR is small (+61/-4), includes a unit test covering the scenario, has no merge conflicts, and all discussion points have been addressed by the author.
Author status: active
Resolved concerns:
- stratospher asked whether unreachable addresses might remain in addrman indefinitely without eviction if gossip keeps refreshing their timestamps; mzumsande clarified this is an acceptable tradeoff unless the user explicitly disables the network with `-onlynet`.
Agreement: Strong
- Broad agreement on the concept across multiple reviewers (fjahr, brunoerg, stratospher, naiyoma)
- Verified with manual tests and logs confirming cross-network failure isolation (naiyoma)
- Concern about retaining unreachable addresses resolved as an acceptable tradeoff (stratospher)
Multiple contributors support the change and naiyoma verified the behavior with logs
Four contributors provided Concept ACKs, and naiyoma posted manual test results confirming the fix prevents cross-network failure counting. stratospher's eviction question was resolved satisfactorily.
- fjahr, brunoerg, stratospher, naiyoma gave Concept ACKs
- naiyoma verified through logs that onion handshakes no longer trigger failure counting for subsequent IPv4 attempts
- stratospher noted 'this is what m_last_good is ideally supposed to be doing'
Review verdicts (DrahtBot): 0
- Concept ACK: fjahr, brunoerg, stratospher, naiyoma
Files
54 lines under test/bench/ci.
- src/test/addrman_tests.cpp +54/-0
- src/addrman_impl.h +5/-2
- src/addrman.cpp +2/-2
Card
This pull request tracks `m_last_good` per network in AddrMan so that failed connection attempts are only counted when the node has demonstrated connectivity on that specific network. It prevents legitimate addresses on unreachable networks (like IPv6 or Tor) from being classified as terrible and dropped from gossip merely because IPv4 connections succeed. Review is in a strong state with multiple concept ACKs, manual log verification, and a unit test, with no open blockers.