diff --git a/eth/dropper.go b/eth/dropper.go index d79edc9795..5b49a98185 100644 --- a/eth/dropper.go +++ b/eth/dropper.go @@ -159,27 +159,23 @@ func (cm *dropper) dropRandomPeer() bool { } numDialed := len(peers) - numInbound + // Compute the set of inclusion-protected peers before filtering. + protected := cm.protectedPeers(peers) + selectDoNotDrop := func(p *p2p.Peer) bool { - // Avoid dropping trusted and static peers, or recent peers. - // Only drop peers if their respective category (dialed/inbound) - // is close to limit capacity. return p.Trusted() || p.StaticDialed() || p.Lifetime() < mclock.AbsTime(doNotDropBefore) || (p.DynDialed() && cm.maxDialPeers-numDialed > peerDropThreshold) || - (p.Inbound() && cm.maxInboundPeers-numInbound > peerDropThreshold) + (p.Inbound() && cm.maxInboundPeers-numInbound > peerDropThreshold) || + protected[p] } droppable := slices.DeleteFunc(peers, selectDoNotDrop) if len(droppable) == 0 { - return false - } - // Protect peers with the highest inclusion stats. - if cm.peerStatsFunc != nil { - droppable = cm.filterProtectedPeers(droppable) - if len(droppable) == 0 { + if len(protected) > 0 { dropSkipped.Mark(1) - return false } + return false } p := droppable[mrand.Intn(len(droppable))] log.Debug("Dropping random peer", "inbound", p.Inbound(), @@ -193,34 +189,35 @@ func (cm *dropper) dropRandomPeer() bool { return true } -// filterProtectedPeers removes peers from the droppable list that are -// protected by any of the protection categories. Each category independently -// selects the top-N peers per inbound/dialed pool by score; the union of all -// selections is protected. -func (cm *dropper) filterProtectedPeers(droppable []*p2p.Peer) []*p2p.Peer { +// protectedPeers computes the set of peers that should not be dropped based +// on inclusion stats. Each protection category independently selects its +// top-N peers per inbound/dialed pool; the union is returned. +func (cm *dropper) protectedPeers(peers []*p2p.Peer) map[*p2p.Peer]bool { + if cm.peerStatsFunc == nil { + return nil + } stats := cm.peerStatsFunc() if len(stats) == 0 { - return droppable + return nil } type peerWithStats struct { peer *p2p.Peer s PeerInclusionStats } var inbound, dialed []peerWithStats - for _, p := range droppable { - id := p.ID().String() - entry := peerWithStats{p, stats[id]} + for _, p := range peers { + entry := peerWithStats{p, stats[p.ID().String()]} if p.Inbound() { inbound = append(inbound, entry) } else { dialed = append(dialed, entry) } } - protectedSet := make(map[*p2p.Peer]struct{}) + result := make(map[*p2p.Peer]bool) protectTopN := func(entries []peerWithStats, cat protectionCategory) { n := int(float64(len(entries)) * cat.frac) - if n == 0 || len(entries) == 0 { + if n == 0 { return } sort.Slice(entries, func(i, j int) bool { @@ -228,7 +225,7 @@ func (cm *dropper) filterProtectedPeers(droppable []*p2p.Peer) []*p2p.Peer { }) for i := 0; i < n && i < len(entries); i++ { if cat.score(entries[i].s) > 0 { - protectedSet[entries[i].peer] = struct{}{} + result[entries[i].peer] = true } } } @@ -237,21 +234,11 @@ func (cm *dropper) filterProtectedPeers(droppable []*p2p.Peer) []*p2p.Peer { copy(inCopy, inbound) dialCopy := make([]peerWithStats, len(dialed)) copy(dialCopy, dialed) - protectTopN(inCopy, cat) protectTopN(dialCopy, cat) } - if len(protectedSet) == 0 { - return droppable - } - log.Debug("Protecting high-value peers from drop", - "protected", len(protectedSet), "droppable", len(droppable)) - - result := make([]*p2p.Peer, 0, len(droppable)) - for _, p := range droppable { - if _, ok := protectedSet[p]; !ok { - result = append(result, p) - } + if len(result) > 0 { + log.Debug("Protecting high-value peers from drop", "protected", len(result)) } return result } diff --git a/eth/dropper_test.go b/eth/dropper_test.go index 484b0cd23b..ba1eb66faf 100644 --- a/eth/dropper_test.go +++ b/eth/dropper_test.go @@ -33,114 +33,92 @@ func makePeers(n int) []*p2p.Peer { return peers } -func TestFilterProtectedNoStats(t *testing.T) { - // When the stats func returns nil/empty, all peers remain droppable. +func TestProtectedPeersNoStats(t *testing.T) { cm := &dropper{maxDialPeers: 20, maxInboundPeers: 30} cm.peerStatsFunc = func() map[string]PeerInclusionStats { return nil } peers := makePeers(10) - result := cm.filterProtectedPeers(peers) - if len(result) != len(peers) { - t.Fatalf("expected all peers droppable with nil stats, got %d/%d", len(result), len(peers)) + protected := cm.protectedPeers(peers) + if len(protected) != 0 { + t.Fatalf("expected no protected peers with nil stats, got %d", len(protected)) } } -func TestFilterProtectedEmptyStats(t *testing.T) { +func TestProtectedPeersEmptyStats(t *testing.T) { cm := &dropper{maxDialPeers: 20, maxInboundPeers: 30} cm.peerStatsFunc = func() map[string]PeerInclusionStats { return map[string]PeerInclusionStats{} } peers := makePeers(10) - result := cm.filterProtectedPeers(peers) - if len(result) != len(peers) { - t.Fatalf("expected all peers droppable with empty stats, got %d/%d", len(result), len(peers)) + protected := cm.protectedPeers(peers) + if len(protected) != 0 { + t.Fatalf("expected no protected peers with empty stats, got %d", len(protected)) } } -func TestFilterProtectedTopPeer(t *testing.T) { +func TestProtectedPeersTopPeer(t *testing.T) { // 20 peers, 10% of 20 = 2 protected per category. - // NewPeer creates non-inbound peers, so all go to dialed bucket. cm := &dropper{maxDialPeers: 20, maxInboundPeers: 30} peers := makePeers(20) stats := make(map[string]PeerInclusionStats) - // Peer 0: top by Finalized stats[peers[0].ID().String()] = PeerInclusionStats{Finalized: 100} - // Peer 1: top by RecentIncluded stats[peers[1].ID().String()] = PeerInclusionStats{RecentIncluded: 5.0} cm.peerStatsFunc = func() map[string]PeerInclusionStats { return stats } - result := cm.filterProtectedPeers(peers) - // 2 categories × 2 protected each = up to 4, but peers 0 and 1 are - // different so both should be removed. Other peers have zero scores. - protected := len(peers) - len(result) - if protected != 2 { - t.Fatalf("expected 2 protected peers, got %d", protected) + protected := cm.protectedPeers(peers) + if len(protected) != 2 { + t.Fatalf("expected 2 protected peers, got %d", len(protected)) } - // Verify peers 0 and 1 are not in result. - for _, p := range result { - id := p.ID().String() - if id == peers[0].ID().String() || id == peers[1].ID().String() { - t.Fatalf("peer %s should be protected", id) - } + if !protected[peers[0]] { + t.Fatal("peer 0 should be protected (top Finalized)") + } + if !protected[peers[1]] { + t.Fatal("peer 1 should be protected (top RecentIncluded)") } } -func TestFilterProtectedZeroScore(t *testing.T) { +func TestProtectedPeersZeroScore(t *testing.T) { cm := &dropper{maxDialPeers: 20, maxInboundPeers: 30} peers := makePeers(10) stats := make(map[string]PeerInclusionStats) - // All peers have zero stats. for _, p := range peers { stats[p.ID().String()] = PeerInclusionStats{} } cm.peerStatsFunc = func() map[string]PeerInclusionStats { return stats } - result := cm.filterProtectedPeers(peers) - if len(result) != len(peers) { - t.Fatalf("expected no protection with zero scores, got %d protected", len(peers)-len(result)) + protected := cm.protectedPeers(peers) + if len(protected) != 0 { + t.Fatalf("expected no protection with zero scores, got %d", len(protected)) } } -func TestFilterProtectedOverlap(t *testing.T) { - // One peer is top in both categories — should only be removed once. +func TestProtectedPeersOverlap(t *testing.T) { + // One peer is top in both categories — counted once. cm := &dropper{maxDialPeers: 20, maxInboundPeers: 30} peers := makePeers(20) stats := make(map[string]PeerInclusionStats) - // Peer 0 is top in both. stats[peers[0].ID().String()] = PeerInclusionStats{Finalized: 100, RecentIncluded: 5.0} cm.peerStatsFunc = func() map[string]PeerInclusionStats { return stats } - result := cm.filterProtectedPeers(peers) - protected := len(peers) - len(result) - if protected != 1 { - t.Fatalf("expected 1 protected peer (overlap), got %d", protected) + protected := cm.protectedPeers(peers) + if len(protected) != 1 { + t.Fatalf("expected 1 protected peer (overlap), got %d", len(protected)) } } -func TestFilterProtectedAllProtected(t *testing.T) { - // 10 peers: 10% = 1 per category. Give all 10 peers high scores in - // one of the two categories so the union covers everyone. +func TestProtectedPeersNilFunc(t *testing.T) { cm := &dropper{maxDialPeers: 20, maxInboundPeers: 30} + // peerStatsFunc is nil (default). peers := makePeers(10) - stats := make(map[string]PeerInclusionStats) - // Peer 0 has the highest Finalized → protected by total-finalized. - stats[peers[0].ID().String()] = PeerInclusionStats{Finalized: 100} - // Peer 1 has the highest RecentIncluded → protected by recent-included. - stats[peers[1].ID().String()] = PeerInclusionStats{RecentIncluded: 5.0} - - cm.peerStatsFunc = func() map[string]PeerInclusionStats { return stats } - - result := cm.filterProtectedPeers(peers) - // 10% of 10 = 1 per category, 2 categories = 2 protected. - // 10 - 2 = 8 droppable. - if len(result) != 8 { - t.Fatalf("expected 8 droppable peers, got %d", len(result)) + protected := cm.protectedPeers(peers) + if protected != nil { + t.Fatalf("expected nil with nil stats func, got %v", protected) } }