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
ACK	fjahr
Concept ACK	brunoerg, stratospher, naiyoma

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36449 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36449.svg"></sub> (addrman: use uniform sampling inside buckets by sipa)

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.

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.

mzumsande commented at 1:03 PM on August 12, 2026: contributor

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.

Good point, at the end of the day I think we'd have to live with that. If the user knows that they will not be able to reach IPv6 permanently, they would best specify that we -onlynet=ipv4. It would be ideal if we could auto-detect IPv6 connectivity in a privacy-preserving way (that doesn't involve e.g. pinging publicly known IPv6 addresses), but that doesn't seem trivial.

But I think it is also very common, that nodes (e.g. laptops) will be run from different network environments, so that they only temporarily cannot reach IPv6 peers.

sedited requested review from naiyoma on Sep 23, 2026

sedited requested review from stratospher on Sep 23, 2026

in src/addrman_impl.h:216 in b8840fd1c6 outdated

 210 | @@ -211,8 +211,10 @@ class AddrManImpl
 211 |      //! list of "new" buckets
 212 |      nid_type vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs);
 213 |  
 214 | -    //! last time Good was called (memory only). Initially set to 1 so that "never" is strictly worse.
 215 | -    NodeSeconds m_last_good GUARDED_BY(cs){1s};
 216 | +    //! last time Good was called (memory only). While unset, no connection failures
 217 | +    //! are counted, because we have no evidence that it is due to them instead of
 218 | +    //! our connectivity.

fjahr commented at 9:15 AM on September 28, 2026:

nit: Minor wording improvement to make it a bit more readable

    //! are counted, because we can not know if this happens due to connectivity
    //! issues on their side or on our side.

in src/addrman_impl.h:217 in b8840fd1c6 outdated

 210 | @@ -211,8 +211,10 @@ class AddrManImpl
 211 |      //! list of "new" buckets
 212 |      nid_type vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs);
 213 |  
 214 | -    //! last time Good was called (memory only). Initially set to 1 so that "never" is strictly worse.
 215 | -    NodeSeconds m_last_good GUARDED_BY(cs){1s};
 216 | +    //! last time Good was called (memory only). While unset, no connection failures
 217 | +    //! are counted, because we have no evidence that it is due to them instead of
 218 | +    //! our connectivity.
 219 | +    NodeSeconds m_last_good GUARDED_BY(cs){0s};

fjahr commented at 9:17 AM on September 28, 2026:

nit: Could be just

    NodeSeconds m_last_good GUARDED_BY(cs){};

now

in src/addrman.cpp:688 in 31191b5239

 684 | @@ -685,7 +685,7 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, NodeSeconds
 685 |  
 686 |      // update info
 687 |      info.m_last_try = time;
 688 | -    if (fCountFailure && info.m_last_count_attempt < m_last_good) {
 689 | +    if (fCountFailure && info.m_last_count_attempt < m_last_good[info.GetNetwork()]) {

fjahr commented at 9:37 AM on September 28, 2026:

This means that if our IPv6 isn't working the IPv6 addrs also don't collect any nAttemps which means they are scoring higher in GetChance() so we will try them more often. I guess the impact is minor and a clean fix seems to take a lot bigger change, so maybe not worth addressing but you could consider documenting this somewhere along with @stratospher 's comment here: #35750 (comment)

fjahr commented at 9:38 AM on September 28, 2026: contributor

Code review ACK 31191b52396b245294efecf7d601476fc6734bc4

Looks good to me, feel free to ignore my nits but adding a bit more documentation on the trade-offs mentioned would be nice if you have to retouch.

DrahtBot requested review from brunoerg on Sep 28, 2026