addrman: make `m_last_good` network-specific #35750

mzumsande commented at 9:55 PM on July 19, 2026: contributor

m_last_good is used to prevent recording failed attempts for addresses when it is our connectivity that is at fault. However, it is quite common that only some networks are unreachable - e.g. I have encountered it frequently that IPv6 is unreachable, while IPv4 works fine. In this case it is not correct to increase the failed counter for an IPv6 address and ultimately making it Terrible, just because we are making IPv4 connections.

Fix this by making m_last_good network-specific. Also change the initial value from 1 to 0, so that in a situation when there was no previous attempt for an address, but also no previous successful connection to its network, the failed count is not increased. The intial value was not a major issue previously because we would only count failures after having at least two outbound connections (in which case m_last_good would have been set).

addrman: don't count connection failures before the first success

m_last_good was initialised to 1 instead of 0, so that an entry whose
m_last_count_attempt is still 0 is eligible for one counted failure even
though we have never connected to anything successfully.

In practice, this makes little difference: net.cpp normally only passes
fCountFailure once we have at least two outbound peers, which implies
that Good() has already been called.
However, it is changed here in preparation of making m_last_good a
network-specific array.

b8840fd1c6

DrahtBot added the label P2P on Jul 19, 2026

DrahtBot commented at 9:55 PM on July 19, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35750.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	fjahr, brunoerg, stratospher, naiyoma

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

addrman: make m_last_good network-specific

Attempt_() only counts a failed connection attempt if
we have made a successful connection since the last attempt,
so that we don't blame addresses for our own lack of connectivity.
This was network-agnostic: successful connections on any
network re-enabled the check for all addresses.

As a result, a network that is unreachable for us while others keep
working - e.g. if IPv6 is not reachable, or a non-working Tor proxy -
would lead to addresses of that network becoming Terrible.

Avoid this by tracking the last successful connection per
network instead.

31191b5239

mzumsande force-pushed on Jul 19, 2026

DrahtBot added the label CI failed on Jul 19, 2026

DrahtBot removed the label CI failed on Jul 19, 2026

fjahr commented at 8:29 AM on July 20, 2026: contributor

Concept ACK

brunoerg commented at 1:26 PM on July 22, 2026: contributor

Concept ACK

stratospher commented at 9:56 AM on July 28, 2026: contributor

concept ACK. interesting find! this is what m_last_good is ideally supposed to be doing.

I'm mostly wondering about the balance here between keeping vs evicting these addresses. suppose ipv6 stays unreachable and we keep receiving ipv6 gossip (timestamps refresh) - then these addresses would stay indefinitely in the addrman (and won't be evicted unless they have nRefCount > 1) and are kind of useless to us till the connectivity issue is fixed.

on this branch such an entry mainly leaves the new table when:

it is >30 days stale and nobody on the network gossips them anymore
we can reach IPv6 again and this address is genuinely unreachable.

is that too strict? that's the only confusing part. everything else looks good to me!

naiyoma commented at 9:14 PM on August 4, 2026: contributor

Concept ACK

naiyoma commented at 6:43 PM on August 5, 2026: contributor

tested m_last_good on master and on this PR.

On master, m_last_good is written after receiving a VERSION message and is then used to update the attempt count for the next peer we try to connect to. As a result, an address can have its failure counted and eventually become terrible based on the previous peer's handshake on an unrelated network.

From my logs, a single onion peer completes its handshake and writes m_last_good, which persists and is subsequently used on the next connection just before we update the attempt count.

2026-08-04T17:25:28Z [addrman] Good_ address: 6c52w67j3fzmbig3ikjn3dx5w3ne2v57lys7u3whahwu6teb4atr35qd.onion:8333, nTime: 2026-07-27T11:29:22Z, nLastSuccess: 2026-08-04T17:25:28Z, nLastTry: 2026-08-04T17:25:28Z, fInTried: true, nRefcount 0, nAttempt 0

2026-08-04T17:25:31Z [addrman] Attempting connection to 174.89.232.9:8333: nTime: 2026-07-07T00:47:05Z, fCountFailure 1, m_last_count_attempt 1970-01-01T00:00:00Z, nLastSucess: 1970-01-01T00:00:00Z, nLastTry: 2026-08-04T17:25:31Z, fInTried: false, nRefcount 2, nAttempt 0, m_last_good 2026-08-04T17:25:28Z

On this PR, an onion peer completes its handshake and updates only the onion network's m_last_good timestamp.

2026-08-05T14:22:12Z [addrman] Good_ flp63d7zcfzoqfzjkvwcgdxrwcf4e4jmjyyvlwjv2sepzkm2kj526oid.onion:8333, nTime: 2026-08-03T00:50:51Z, nLastSuccess: 2026-08-05T14:22:12Z, nLastTry: 2026-08-05T14:22:12Z, fInTried: true, nRefcount 0, nAttempt 0, m_last_good 2026-08-05T14:22:12Z

When attempting a connection on another network, a separate m_last_good timestamp is used instead. 2026-08-05T14:22:16Z [addrman] Attempting connection to 45.155.40.189:8333: nTime: 2026-07-05T11:22:31Z, fCountFailure 0, m_last_count_attempt 1970-01-01T00:00:00Z, nLastSucess: 1970-01-01T00:00:00Z, nLastTry: 2026-08-05T14:22:16Z, fInTried: false, nRefcount 1, nAttempt 0, m_last_good 1970-01-01T00:00:00Z

Good_ is also reached from ResolveCollisions_ without a live connection, so m_last_good is advanced there as well. bt this PR does not affect that path.