addrman: remove unreachable tried-collision branch
What the developer wrote
Authored by Bruno Garcia
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_()`.
What changed, and why it matters
This commit removes a fallback code path in Bitcoin Core's address manager that the authors say can never be reached under current rules. It replaces that fallback with internal consistency checks (assertions). The change is a code-cleanup and hardening patch, not a fix for a known exploitable bug.
No immediate action required. Treat as routine hardening/refactoring. Reviewers may want to independently verify the claimed invariant that the tried slot cannot become empty while a collision is pending.
Security signals we found
Removal of purportedly unreachable branch in collision-resolution logic
Addition of Assume() assertions to enforce internal invariant
No change to network-facing behavior or data formats
Evidence from the diff
In src/addrman.cpp, ResolveCollisions_() previously had an else branch handling the case where a pending tried-collision’s destination tried slot became empty (vvTried[tried_bucket][tried_bucket_pos] == -1). The commit removes that branch and adds Assume() assertions in both ResolveCollisions_() and SelectTriedCollision_() that the slot is non-empty. The rationale is that addrman invariants guarantee the slot stays occupied until the collision is resolved, making the branch dead code.
Changed components
src/addrman.cppAddrManImpl::ResolveCollisions_()AddrManImpl::SelectTriedCollision_()Inspect captured patch +5 / −4
diff --git a/src/addrman.cpp b/src/addrman.cpp
index 3050beb7..554e5ad0 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -910,7 +910,10 @@ void AddrManImpl::ResolveCollisions_()
int tried_bucket_pos = info_new.GetBucketPosition(nKey, false, tried_bucket);
if (!info_new.IsValid()) { // id_new may no longer map to a valid address
erase_collision = true;
- } else if (vvTried[tried_bucket][tried_bucket_pos] != -1) { // The position in the tried bucket is not empty
+ } else {
+ // A pending tried collision implies that the destination tried slot
+ // remains occupied until we resolve it.
+ Assume(vvTried[tried_bucket][tried_bucket_pos] != -1);
// Get the to-be-evicted address that is being tested
nid_type id_old = vvTried[tried_bucket][tried_bucket_pos];
@@ -939,9 +942,6 @@ void AddrManImpl::ResolveCollisions_()
Good_(info_new, false, current_time);
erase_collision = true;
}
- } else { // Collision is not actually a collision anymore
- Good_(info_new, false, Now<NodeSeconds>());
- erase_collision = true;
}
}
@@ -977,6 +977,7 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::SelectTriedCollision_()
int tried_bucket = newInfo.GetTriedBucket(nKey, m_netgroupman);
int tried_bucket_pos = newInfo.GetBucketPosition(nKey, false, tried_bucket);
+ Assume(vvTried[tried_bucket][tried_bucket_pos] != -1);
const AddrInfo& info_old = mapInfo[vvTried[tried_bucket][tried_bucket_pos]];
return {info_old, info_old.m_last_try};
}
Why this scored 18/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.