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 8a085fc.

* reverse in the compare function

* lint mdfiles

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),
                 }
             }
         }

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<E: EthSpec> PeerDB<E> {
     /// 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<E>)> {
-        let mut connected = self
-            .peers
+        self.peers
             .iter()
             .filter(|(_, info)| info.is_connected())
-            .collect::<Vec<_>>();
-
-        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::<Vec<_>>()
     }
 
     /// Returns a vector containing peers (their ids and info), sorted by
@@ -307,13 +303,11 @@ impl<E: EthSpec> PeerDB<E> {
     where
         F: Fn(&PeerInfo<E>) -> bool,
     {
-        let mut by_status = self
-            .peers
+        self.peers
             .iter()
             .filter(|(_, info)| is_status(info))
-            .collect::<Vec<_>>();
-        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::<Vec<_>>()
     }
 
     /// Returns the peer with highest reputation that satisfies `is_status`
@@ -324,7 +318,7 @@ impl<E: EthSpec> PeerDB<E> {
         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)
     }
 

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<std::cmp::Ordering> {
-        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,
+        }
     }
 }
 

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 |