addrman: remove unreachable tried-collision branch #35692

1. 
   brunoerg commented at 12:44 PM on July 9, 2026: contributor

   ResolveCollisions_() had a fallback for the case where a pending tried collision no longer collided because the destination tried slot became empty.

   Under current addrman invariants this cannot happen: once an entry is added to m_tried_collisions, the corresponding tried slot remains occupied until the collision is resolved. The only other valid outcomes are that the pending new entry disappears or becomes invalid, both of which are already handled.

   Remove the dead branch and replace the implicit assumption with assertions in ResolveCollisions_() and SelectTriedCollision_().

   It came to my mind when taking a look at the fuzz coverage report for the addrman harness. After years (?) of fuzzing, I was trying to understand if it was a fault on the harness or a dead branch.

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

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage & Benchmarks

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

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

   Type	Reviewers
   ACK	danielabrozzoni, stratospher, Herb-ops, naiyoma, mzumsande

   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.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

4. in src/addrman.cpp:916 in e40613dccf

    909 | @@ -910,7 +910,10 @@ void AddrManImpl::ResolveCollisions_()
    910 |              int tried_bucket_pos = info_new.GetBucketPosition(nKey, false, tried_bucket);
    911 |              if (!info_new.IsValid()) { // id_new may no longer map to a valid address
    912 |                  erase_collision = true;
    913 | -            } else if (vvTried[tried_bucket][tried_bucket_pos] != -1) { // The position in the tried bucket is not empty
    914 | +            } else {
    915 | +                // A pending tried collision implies that the destination tried slot
    916 | +                // remains occupied until we resolve it.
    917 | +                assert(vvTried[tried_bucket][tried_bucket_pos] != -1);
   

   ---

   ---

   

   mzumsande commented at 12:58 PM on July 10, 2026:

   I think we should move away from asserts in addrman. Even if some of these conditions may turn out to be broken due to internal corruption (e.g. CVE-2024-52919), this is likely not critical enough to shut down the node. So i would prefer to use Assume() instead.

   ---

   

   brunoerg commented at 4:34 PM on July 10, 2026:

   100% agreed. Just changed it to use Assume(). Thanks.

5. 
   mzumsande commented at 1:02 PM on July 10, 2026: contributor

   Concept ACK

6. addrman: remove unreachable tried-collision branch

   `ResolveCollisions_()` had a fallback for the case where a pending tried
   collision no longer collided because the destination tried slot became
   empty.
   
   Under current addrman invariants this cannot happen: once an entry is
   added to `m_tried_collisions`, the corresponding tried slot remains
   occupied until the collision is resolved. The only other valid outcomes
   are that the pending new entry disappears or becomes invalid, both of
   which are already handled.
   
   Remove the dead branch and replace the implicit assumption with
   assertions in `ResolveCollisions_()` and `SelectTriedCollision_()`.

   bc7d905046
7. brunoerg force-pushed on Jul 10, 2026
8. 
   danielabrozzoni commented at 10:55 PM on July 10, 2026: member

   tACK bc7d9050467fb387c01a73d59cdb914c839964fc

   Nice find!

9. DrahtBot requested review from mzumsande on Jul 10, 2026
10. 
    naiyoma commented at 6:44 PM on July 17, 2026: contributor

    Concept ACK

11. 
    stratospher commented at 2:03 PM on July 20, 2026: contributor

    ACK bc7d905. MakeTried is the only place where we clear the tried table slot but we also refill the same slot here under the cs hold. so makes sense that during a node's runtime destination tried slot which has a previous entry/collison can't be empty (unless some id internal corruption).

    tested this PR by keeping an assert(false) in this unreachable branch and didn't see any crashes for ~3 days.

    or becomes invalid

    I think this is unreachable too - though maybe it's good to keep for testing? everything inserted in addrman goes through AddSingle which requires the address to be IsValid().

12. DrahtBot requested review from naiyoma on Jul 20, 2026
13. 
    Herb-ops commented at 7:19 PM on July 21, 2026: none

    ACK bc7d9050467fb387c01a73d59cdb914c839964fc

14. sedited requested review from ajtowns on Jul 22, 2026
15. 
    naiyoma commented at 11:51 AM on July 27, 2026: contributor

    ACK bc7d9050467fb387c01a73d59cdb914c839964fc

    As far as I can tell, there is no way we could suddenly end up with an empty slot. It seems this branch has either been dead since its introduction in e68172ed9f or after 7e6e65918f i think at some point it was possible to clear() the whole tried table .

    I compiled locally with both Assume() statements reverted, and inverting each condition in turn causes addrman_tests/addrman_evictionworks to abort.

    Micro nit: The commit message is still pointing to using assert instead Of Assume

16. 
    mzumsande commented at 1:06 PM on July 27, 2026: contributor

    Code Review ACK bc7d9050467fb387c01a73d59cdb914c839964fc

17. sedited merged this on Jul 27, 2026
18. sedited closed this on Jul 27, 2026

    ---

Contributors

brunoergDrahtBotmzumsandedanielabrozzoninaiyomastratospherHerb-ops

Review Requested
ajtowns