addrman: make `m_last_good` network-specific #35750

pull mzumsande wants to merge 2 commits into bitcoin:master from mzumsande:202607_addrman_last_good changing 5 files +135 −4
  1. 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).

  2. DrahtBot added the label P2P on Jul 19, 2026
  3. DrahtBot commented at 9:55 PM on July 19, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    External sites

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    Type Reviewers
    Concept ACK brunoerg, stratospher
    Stale ACK fjahr, naiyoma

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. mzumsande force-pushed on Jul 19, 2026
  5. DrahtBot added the label CI failed on Jul 19, 2026
  6. DrahtBot removed the label CI failed on Jul 19, 2026
  7. fjahr commented at 8:29 AM on July 20, 2026: contributor

    Concept ACK

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

    Concept ACK

  9. 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:

    1. it is >30 days stale and nobody on the network gossips them anymore
    2. 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!

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

    Concept ACK

  11. 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.

  12. 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.

  13. sedited requested review from naiyoma on Sep 23, 2026
  14. sedited requested review from stratospher on Sep 23, 2026
  15. 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.
    mzumsande commented at 8:45 AM on October 21, 2026:

    done

  16. 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

  17. 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)

    mzumsande commented at 8:47 AM on October 21, 2026:

    mentioned the impact on GetChance() in the comment.

  18. 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.

  19. DrahtBot requested review from brunoerg on Sep 28, 2026
  20. naiyoma commented at 2:02 PM on October 14, 2026: contributor

    tACK 31191b52396b245294efecf7d601476fc6734bc4

    tested the IPv4-unreachable / IPv6-reachable scenario on mainnet, before and after this PR. I blocked outgoing ipv4 tcp iptables -I OUTPUT -p tcp --syn ! -o lo -j REJECT --reject-with tcp-reset on this branch m_last_good was updated for ipv6 only and not but not for ipv4, so no ipv4 failure is counted.

    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.

    I think its okay for us to keep thise addresses in our addrman. A refreshed timestamp means that the node is alive . this has the befit that we can relay them via Getaddr and we can use if we become reachable. They also stay in the new table and never make it to the tried table.

    Since they’ll keep being selected , this will add failed attempts, on top of the already high new table failure rate . But the effect seems small and is better than wrongly marking them as terrible.

  21. sedited commented at 2:50 PM on October 19, 2026: contributor

    @mzumsande do you want to take the nits here?

  22. stratospher commented at 2:27 AM on October 20, 2026: contributor

    If the user knows that they will not be able to reach IPv6 permanently, they would best specify that we -onlynet=ipv4.

    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.

    have you also considered cloud node operators? there are many of them and I think they often don't have ipv6 enabled. but they receive ipv6 gossip and store in their addrman. not sure how many would change bitcoin.conf to specify -onlynet=ipv4.

    so not fully comfortable with the idea of them getting squatter ipv6 addresses in their new table which would slowly grow over time. for these cloud nodes, an ipv6 entry could then take a terrible ipv4 slot but never give it back (under normal self announcement gossip scenario), so i'd expect the ipv6 share to creep up rather than stay where it is?

    just for reference - some stats from different peer.observer addrman snapshots on master which don't support ipv6:

    • seeing 3.8k - 15.8k slots in the new table occupied by ipv6 addresses (out of 64k slots)
      • for the 3.8k ipv6 in new table : 605 (nAttempts=0) + 1,122 (nAttempts=1) + 2,045 (nAttempts=2) + 67 ( (nAttempts>=3)
      • for the 15.8k ipv6 in new table: 3,285 (nAttempts=0) + 4,713 (nAttempts=1) + 7,547 (nAttempts=2) + 22 (nAttempts>=3)
    • currently IPv6 gossip is 25.9–27.6% of IPv4+IPv6 gossip per day. this could change over the years.

    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.

    makes sense. wondering if it's worth it to find a possible middle-ground between:

    • stopping a temporary network outage on our side from marking good addresses isTerrible (marking all ipv6 as isTerrible and everything getting evicted)
    • on a node that doesn't support ipv6 - stopping ipv6 addresses from holding new-table slots indefinitely (marking no ipv6 as isTerrible and nothing getting evicted)

    maybe If we haven't connected to ipv6 for 7 days while other networks work, an ipv4 gossip received can take the new table slot of an address on the broken connectivity network?

    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.

    that would have been so amazing haha.

  23. mzumsande force-pushed on Oct 21, 2026
  24. 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.
    61b88aeeba
  25. mzumsande force-pushed on Oct 21, 2026
  26. mzumsande marked this as a draft on Oct 21, 2026
  27. mzumsande commented at 8:54 AM on October 21, 2026: contributor

    maybe If we haven't connected to ipv6 for 7 days while other networks work, an ipv4 gossip received can take the new table slot of an address on the broken connectivity network?

    I changed the PR to revert to the old behaviour after 7 days of no connections. This means that mobile nodes with changing networks won't be affected - but long-running nodes will still make addresses terrible after some times.

    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.

    that would have been so amazing haha.

    I think I want to look into this a bit more in the coming days - maybe it is possible at least in some cases. I will put this PR into draft until then, because if we could auto-detect missing IPv6 connectivity in most cases as the cloud-node one, the 7-day fallback might not be necessary.

  28. 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.

However, if we haven't been able to connect to a network for
ADDRMAN_NET_UNREACHABLE (7 days, or since startup), we are probably
unable to reach it at all (e.g. a node without IPv6 connectivity).
Fall back to successful connections on any network in this case, so
that addresses of that network can still become Terrible and don't
occupy addrman slots indefinitely.
    a8b39dde37
  29. mzumsande force-pushed on Oct 21, 2026
  30. DrahtBot added the label CI failed on Oct 21, 2026
  31. DrahtBot commented at 9:22 AM on October 21, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task macOS native, fuzz: https://github.com/bitcoin/bitcoin/actions/runs/39571546703/job/119126694900</sub> <sub>LLM reason (✨ experimental): The fuzz CI failed because data_stream_addr_man exited with code 1 while processing a corpus input, after detecting system-time access without mocked time.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  32. DrahtBot removed the label CI failed on Oct 21, 2026
Contributors
mzumsandeDrahtBotfjahrbrunoergstratosphernaiyomasedited


brunoergstratospher

Labels
P2P
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-10-30 09:40 UTC