From 50d8375d4666e6deb9bee9cfab396ce767ef15ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Oliveira?= Date: Wed, 25 Sep 2024 14:45:35 +0100 Subject: [PATCH] Remove Score Ord, PartialOrd, Eq and PartialEq impls (#6420) * drop score Ord, PartialOrd, Eq and PartialEq impls and impl total_cmp instead * Revert "Fix test failure on Rust v1.81 (#6407)" This reverts commit 8a085fc828cef14674ef342906b715dd816e8047. * reverse in the compare function * lint mdfiles --- .../src/peer_manager/mod.rs | 14 ++------ .../src/peer_manager/peerdb.rs | 22 +++++------- .../src/peer_manager/peerdb/score.rs | 35 +++++++++++-------- book/src/developers.md | 1 + 4 files changed, 31 insertions(+), 41 deletions(-) diff --git a/beacon_node/lighthouse_network/src/peer_manager/mod.rs b/beacon_node/lighthouse_network/src/peer_manager/mod.rs index b8dce6667e4..9f46f5daa09 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/mod.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/mod.rs @@ -2340,16 +2340,6 @@ mod tests { gossipsub_score: f64, } - // generate an arbitrary f64 while preventing NaN values - fn arbitrary_f64(g: &mut Gen) -> f64 { - loop { - let val = f64::arbitrary(g); - if !val.is_nan() { - return val; - } - } - } - impl Arbitrary for PeerCondition { fn arbitrary(g: &mut Gen) -> Self { let attestation_net_bitfield = { @@ -2375,9 +2365,9 @@ mod tests { outgoing: bool::arbitrary(g), attestation_net_bitfield, sync_committee_net_bitfield, - score: arbitrary_f64(g), + score: f64::arbitrary(g), trusted: bool::arbitrary(g), - gossipsub_score: arbitrary_f64(g), + gossipsub_score: f64::arbitrary(g), } } } diff --git a/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs b/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs index 08d9e5209c8..d2effd4d037 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs @@ -1,8 +1,8 @@ use crate::discovery::enr::PEERDAS_CUSTODY_SUBNET_COUNT_ENR_KEY; use crate::discovery::{peer_id_to_node_id, CombinedKey}; use crate::{metrics, multiaddr::Multiaddr, types::Subnet, Enr, EnrExt, Gossipsub, PeerId}; +use itertools::Itertools; use peer_info::{ConnectionDirection, PeerConnectionStatus, PeerInfo}; -use rand::seq::SliceRandom; use score::{PeerAction, ReportSource, Score, ScoreState}; use slog::{crit, debug, error, trace, warn}; use std::net::IpAddr; @@ -290,15 +290,11 @@ impl PeerDB { /// Returns a vector of all connected peers sorted by score beginning with the worst scores. /// Ties get broken randomly. pub fn worst_connected_peers(&self) -> Vec<(&PeerId, &PeerInfo)> { - let mut connected = self - .peers + self.peers .iter() .filter(|(_, info)| info.is_connected()) - .collect::>(); - - connected.shuffle(&mut rand::thread_rng()); - connected.sort_by_key(|(_, info)| info.score()); - connected + .sorted_by(|(_, info_a), (_, info_b)| info_a.score().total_cmp(info_b.score(), false)) + .collect::>() } /// Returns a vector containing peers (their ids and info), sorted by @@ -307,13 +303,11 @@ impl PeerDB { where F: Fn(&PeerInfo) -> bool, { - let mut by_status = self - .peers + self.peers .iter() .filter(|(_, info)| is_status(info)) - .collect::>(); - by_status.sort_by_key(|(_, info)| info.score()); - by_status.into_iter().rev().collect() + .sorted_by(|(_, info_a), (_, info_b)| info_a.score().total_cmp(info_b.score(), true)) + .collect::>() } /// Returns the peer with highest reputation that satisfies `is_status` @@ -324,7 +318,7 @@ impl PeerDB { self.peers .iter() .filter(|(_, info)| is_status(info)) - .max_by_key(|(_, info)| info.score()) + .max_by(|(_, info_a), (_, info_b)| info_a.score().total_cmp(info_b.score(), false)) .map(|(id, _)| id) } diff --git a/beacon_node/lighthouse_network/src/peer_manager/peerdb/score.rs b/beacon_node/lighthouse_network/src/peer_manager/peerdb/score.rs index c8425fc104d..995ebf90646 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/peerdb/score.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/peerdb/score.rs @@ -7,6 +7,7 @@ //! The scoring algorithms are currently experimental. use crate::service::gossipsub_scoring_parameters::GREYLIST_THRESHOLD as GOSSIPSUB_GREYLIST_THRESHOLD; use serde::Serialize; +use std::cmp::Ordering; use std::sync::LazyLock; use std::time::Instant; use strum::AsRefStr; @@ -260,7 +261,7 @@ impl RealScore { } } -#[derive(PartialEq, Clone, Debug, Serialize)] +#[derive(Clone, Debug, Serialize)] pub enum Score { Max, Real(RealScore), @@ -323,21 +324,25 @@ impl Score { Self::Real(score) => score.is_good_gossipsub_peer(), } } -} - -impl Eq for Score {} -impl PartialOrd for Score { - fn partial_cmp(&self, other: &Score) -> Option { - Some(self.cmp(other)) - } -} - -impl Ord for Score { - fn cmp(&self, other: &Score) -> std::cmp::Ordering { - self.score() - .partial_cmp(&other.score()) - .unwrap_or(std::cmp::Ordering::Equal) + /// Instead of implementing `Ord` for `Score`, as we are underneath dealing with f64, + /// follow std convention and impl `Score::total_cmp` similar to `f64::total_cmp`. + pub fn total_cmp(&self, other: &Score, reverse: bool) -> Ordering { + match self.score().partial_cmp(&other.score()) { + Some(v) => { + // Only reverse when none of the items is NAN, + // so that NAN's are never considered. + if reverse { + v.reverse() + } else { + v + } + } + None if self.score().is_nan() && !other.score().is_nan() => Ordering::Less, + None if !self.score().is_nan() && other.score().is_nan() => Ordering::Greater, + // Both are NAN. + None => Ordering::Equal, + } } } diff --git a/book/src/developers.md b/book/src/developers.md index 244c935ac2f..d90708c5a97 100644 --- a/book/src/developers.md +++ b/book/src/developers.md @@ -20,6 +20,7 @@ Lighthouse currently uses the following ENR fields: ### Lighthouse Custom Fields Lighthouse is currently using the following custom ENR fields. + | Field | Description | | ---- | ---- | | `quic` | The UDP port on which the QUIC transport is listening on IPv4 |