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
   Concept ACK	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

Contributors

brunoergDrahtBotmzumsandedanielabrozzoni

Review Requested
mzumsande