From 74e508ee4b165cbe0bb402262a32c5bb89973174 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Tue, 6 Oct 2026 18:34:50 -0600 Subject: [PATCH] addrman: keep a colliding address in its derived bucket (getrawaddrman, test flake) (#923) * addrman: keep a colliding entry in its derived bucket positions() derives each entry's getrawaddrman slot with Core's formulas and moved a collision to the next free slot of the flat table. From a bucket's last position that is the next bucket's first, so a /16's tried entries could be reported in nine buckets where Core's formula allows eight. one_group_still_spans_several_tried_buckets caught it about once in a hundred runs (random slot key). A taken slot now moves to the next free position in its own bucket, wrapping within it, as Core keeps every entry in its formula's bucket. Only a full bucket spills into the next one with room. One u64 mask per bucket replaces the union-find. Co-Authored-By: Claude Opus 5.5 (1M context) * docs: note the getrawaddrman bucket fix for 0.6.1 Co-Authored-By: Claude Opus 5.5 (1M context) --------- Co-authored-by: Claude Opus 5.5 (1M context) (cherry picked from commit e63aa2103f20860b4c4a80497f9e75e463d3a590) --- node/src/net/addrman.rs | 131 +++++++++++++++++++++++++++------------- 1 file changed, 88 insertions(+), 43 deletions(-) diff --git a/node/src/net/addrman.rs b/node/src/net/addrman.rs index 19a1de08b..6e62d618c 100644 --- a/node/src/net/addrman.rs +++ b/node/src/net/addrman.rs @@ -426,20 +426,23 @@ impl AddrMan { /// Core never has two entries in one slot (a collision evicts or is /// refused), and `getrawaddrman` keys its JSON objects by slot, so a /// collision here must not be reported twice under one key. Entries are - /// placed in address order and a taken slot moves on to the next free - /// one, which keeps the result deterministic for a given key. + /// placed in address order, which keeps the result deterministic for a + /// given key. A taken slot moves to the next free position in the same + /// bucket, wrapping within it: Core keeps every entry in the bucket its + /// formula gives, so one group's tried entries never span more than + /// `TRIED_BUCKETS_PER_GROUP` buckets. Only a full bucket spills into the + /// next one with room, since the flat map can hold more of one group + /// than Core's buckets would. pub fn positions(&self) -> Vec<(AddrSlot, &AddrEntry)> { let mut entries: Vec<&AddrEntry> = self.entries.values().collect(); entries.sort_by_key(|e| e.addr); - let mut new = FreeSlots::new(NEW_BUCKET_COUNT * BUCKET_SIZE); - let mut tried = FreeSlots::new(TRIED_BUCKET_COUNT * BUCKET_SIZE); + let mut new = TakenSlots::new(NEW_BUCKET_COUNT); + let mut tried = TakenSlots::new(TRIED_BUCKET_COUNT); let mut out = Vec::with_capacity(entries.len()); for e in entries { let mut slot = self.derived_slot(e); let table = if e.tried { &mut tried } else { &mut new }; - let index = table.take(slot.bucket * BUCKET_SIZE + slot.position); - slot.bucket = index / BUCKET_SIZE; - slot.position = index % BUCKET_SIZE; + (slot.bucket, slot.position) = table.take(slot.bucket, slot.position); out.push((slot, e)); } out @@ -517,29 +520,33 @@ impl AddrMan { /// slots it walked at the one it claimed, so later claims skip a run they /// have already crossed instead of stepping through it slot by slot, which /// goes quadratic once many entries share a group's few buckets. -struct FreeSlots { - next: Vec, +/// The slots [`AddrMan::positions`] has handed out in one table, as one bit +/// per position in a mask per bucket. +struct TakenSlots { + buckets: Vec, } -impl FreeSlots { - fn new(capacity: u64) -> Self { - FreeSlots { next: (0..capacity as u32).collect() } +const _: () = assert!(BUCKET_SIZE == u64::BITS as u64); + +impl TakenSlots { + fn new(buckets: u64) -> Self { + TakenSlots { buckets: vec![0; buckets as usize] } } - /// Only called while a slot is free (see the `MAX_ENTRIES` assertions). - fn take(&mut self, start: u64) -> u64 { - let mut free = start as u32; - while self.next[free as usize] != free { - free = self.next[free as usize]; - } - let mut i = start as u32; - while i != free { - let after = self.next[i as usize]; - self.next[i as usize] = free; - i = after; + /// The first free position at or after `position` in `bucket`, wrapping + /// within the bucket; if the bucket is full, the same search in the next + /// bucket that is not. Only called while a slot is free (see the + /// `MAX_ENTRIES` assertions). + fn take(&mut self, bucket: u64, position: u64) -> (u64, u64) { + let mut bucket = bucket as usize; + while self.buckets[bucket] == u64::MAX { + bucket = (bucket + 1) % self.buckets.len(); } - self.next[free as usize] = (free + 1) % self.next.len() as u32; - u64::from(free) + let free = !self.buckets[bucket]; + let offset = u64::from(free.rotate_right(position as u32).trailing_zeros()); + let position = (position + offset) % BUCKET_SIZE; + self.buckets[bucket] |= 1 << position; + (bucket as u64, position) } } @@ -907,8 +914,9 @@ mod tests { assert_eq!(unique.len(), 300); } - /// The placement `positions` replaced: probe one slot at a time from - /// the derived slot. It must land every entry where this does. + /// The placement `positions` makes, one slot at a time: the derived + /// bucket's positions from the derived one, wrapping within the bucket, + /// then the next bucket's from the same position, and so on. fn probed_positions(a: &AddrMan) -> Vec<(AddrSlot, SocketAddr)> { let mut entries: Vec<&AddrEntry> = a.entries.values().collect(); entries.sort_by_key(|e| e.addr); @@ -917,23 +925,29 @@ mod tests { .into_iter() .map(|e| { let mut slot = a.derived_slot(e); + let (start_bucket, start_position) = (slot.bucket, slot.position); let buckets = if e.tried { TRIED_BUCKET_COUNT } else { NEW_BUCKET_COUNT }; - let mut index = slot.bucket * BUCKET_SIZE + slot.position; - while !taken.insert((e.tried, index)) { - index = (index + 1) % (buckets * BUCKET_SIZE); + let probe = (0..buckets).flat_map(|b| { + (0..BUCKET_SIZE).map(move |p| { + ((start_bucket + b) % buckets, (start_position + p) % BUCKET_SIZE) + }) + }); + for (bucket, position) in probe { + if taken.insert((e.tried, bucket, position)) { + (slot.bucket, slot.position) = (bucket, position); + break; + } } - slot.bucket = index / BUCKET_SIZE; - slot.position = index % BUCKET_SIZE; (slot, e.addr) }) .collect() } /// Tried entries in one /16 share its eight buckets (512 slots), so most - /// of 3000 overflow into long runs of taken slots; new entries from one - /// source group share its 64 buckets. Placement matches a linear probe. + /// of 3000 overflow them; new entries from one source group share its 64 + /// buckets. Placement matches a slot-by-slot probe. #[test] - fn positions_match_a_linear_probe_under_heavy_collision() { + fn positions_match_a_slot_by_slot_probe_under_heavy_collision() { let mut a = AddrMan::new(); for i in 0..3000u32 { a.mark_good(sa(&format!("10.244.{}.{}:18444", i / 256, i % 256)), 1); @@ -948,18 +962,49 @@ mod tests { assert_eq!(placed, probed_positions(&a)); } - /// Claims go to the first free slot at or after the start, wrapping, and - /// the table fills completely. + /// A claim takes the first free position at or after the start in its + /// own bucket, wrapping to the bucket's first position and not into the + /// next bucket. Only a full bucket sends it on, and the last bucket + /// wraps to the first. + #[test] + fn taken_slots_stay_in_their_bucket_until_it_is_full() { + let mut t = TakenSlots::new(3); + assert_eq!(t.take(1, 62), (1, 62)); + assert_eq!(t.take(1, 62), (1, 63)); + assert_eq!(t.take(1, 63), (1, 0)); + assert_eq!(t.take(1, 0), (1, 1)); + for _ in 4..BUCKET_SIZE { + t.take(1, 5); + } + assert_eq!(t.take(1, 40), (2, 40)); + for _ in 1..BUCKET_SIZE { + t.take(2, 0); + } + assert_eq!(t.take(2, 7), (0, 7)); + } + + /// The `getrawaddrman` flake: 32 tried entries in one /16 sometimes came + /// out in nine buckets, when two shared a derived slot at a bucket's last + /// position and the second moved to the next bucket's first. With room + /// in every bucket, each entry must keep its derived bucket, whatever + /// the key. #[test] - fn free_slots_claim_the_next_free_slot_and_wrap() { - let mut f = FreeSlots::new(8); - let claims: Vec = [6, 6, 6, 7, 3, 2, 1, 0].into_iter().map(|s| f.take(s)).collect(); - assert_eq!(claims, [6, 7, 0, 1, 3, 2, 4, 5]); + fn a_collision_keeps_the_entry_in_its_derived_bucket() { + for k in 0..500u64 { + let mut a = AddrMan::new(); + a.slot_key = (k, k.wrapping_mul(0x9e37_79b9_7f4a_7c15)); + for i in 0..32u8 { + a.mark_good(sa(&format!("10.244.{i}.5:18444")), 1); + } + for (slot, e) in a.positions() { + assert_eq!(slot.bucket, a.derived_slot(e).bucket, "key {k}: {}", e.addr); + } + } } /// The worst case: every entry tried and in one /16. The tried table has - /// exactly `MAX_ENTRIES` slots, so every slot ends up used once. A - /// slot-by-slot probe steps over some 50 million taken slots here. + /// exactly `MAX_ENTRIES` slots, so every slot ends up used once, nearly + /// all of them by entries the group's eight full buckets sent on. #[test] fn positions_fill_a_full_tried_table_from_one_group() { let mut a = AddrMan::new();