Remove todo from code and provide some preliminary peer review config…

… values (#1380) This was left behind from this work: FuelLabs/fuel-core#1356 As we did for the syncing peer config, we left hard-coded values for each variant. The followup work for that is tracked here: FuelLabs/fuel-core#1340 --------- Co-authored-by: Green Baneling <XgreenX9999@gmail.com>
crypto523 · Sep 25, 2023 · ca6728d · ca6728d
1 parent b59f93d
commit ca6728d
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 17 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -26,7 +26,8 @@ Description of the upcoming release here.
 
 ### Changed
 
-- [#1377](https://github.com/FuelLabs/fuel-core/pull/1377): Remove `DiscoveryEvent` and use `KademliaEvent` directly in `DiscoveryBehavior`
+- [#1380](https://github.com/FuelLabs/fuel-core/pull/1380): Add preliminary, hard-coded config values for heartbeat peer reputation, removing `todo`.
+- [#1377](https://github.com/FuelLabs/fuel-core/pull/1377): Remove `DiscoveryEvent` and use `KademliaEvent` directly in `DiscoveryBehavior`.
 - [#1366](https://github.com/FuelLabs/fuel-core/pull/1366): Improve caching during docker builds in CI by replacing gha
 - [#1358](https://github.com/FuelLabs/fuel-core/pull/1358): Upgraded the Rust version used in CI to 1.72.0. Also includes associated Clippy changes.
 - [#1318](https://github.com/FuelLabs/fuel-core/pull/1318): Modified block synchronization to use asynchronous task execution when retrieving block headers.

diff --git a/crates/services/p2p/src/service.rs b/crates/services/p2p/src/service.rs
@@ -124,12 +124,6 @@ pub enum HeartBeatPeerReportReason {
     LowHeartBeatFrequency,
 }
 
-impl PeerReport for HeartBeatPeerReportReason {
-    fn get_score_from_report(&self) -> AppScore {
-        todo!()
-    }
-}
-
 pub trait TaskP2PService: Send {
     fn get_peer_ids(&self) -> Vec<PeerId>;
     fn get_all_peer_info(&self) -> Vec<(&PeerId, &PeerInfo)>;
@@ -242,7 +236,7 @@ pub trait Broadcast: Send {
     fn report_peer(
         &self,
         peer_id: FuelPeerId,
-        report: HeartBeatPeerReportReason,
+        report: AppScore,
         reporting_service: &'static str,
     ) -> anyhow::Result<()>;
 
@@ -258,7 +252,7 @@ impl Broadcast for SharedState {
     fn report_peer(
         &self,
         peer_id: FuelPeerId,
-        report: HeartBeatPeerReportReason,
+        report: AppScore,
         reporting_service: &'static str,
     ) -> anyhow::Result<()> {
         self.report_peer(peer_id, report, reporting_service)
@@ -293,6 +287,13 @@ pub struct Task<P, D, B> {
     heartbeat_max_avg_interval: Duration,
     heartbeat_max_time_since_last: Duration,
     next_check_time: Instant,
+    heartbeat_peer_reputation_config: HeartbeatPeerReputationConfig,
+}
+
+#[derive(Clone)]
+pub struct HeartbeatPeerReputationConfig {
+    old_heartbeat_penalty: AppScore,
+    low_heartbeat_frequency_penalty: AppScore,
 }
 
 impl<D> Task<FuelP2PService<PostcardCodec>, D, SharedState> {
@@ -313,6 +314,13 @@ impl<D> Task<FuelP2PService<PostcardCodec>, D, SharedState> {
         let (tx_broadcast, _) = broadcast::channel(100);
         let (block_height_broadcast, _) = broadcast::channel(100);
 
+        // Hardcoded for now, but left here to be configurable in the future.
+        // TODO: https://github.com/FuelLabs/fuel-core/issues/1340
+        let heartbeat_peer_reputation_config = HeartbeatPeerReputationConfig {
+            old_heartbeat_penalty: -5.,
+            low_heartbeat_frequency_penalty: -5.,
+        };
+
         let next_block_height = block_importer.next_block_height();
         let p2p_service = FuelP2PService::new(config, PostcardCodec::new(max_block_size));
 
@@ -337,6 +345,7 @@ impl<D> Task<FuelP2PService<PostcardCodec>, D, SharedState> {
             heartbeat_max_avg_interval,
             heartbeat_max_time_since_last,
             next_check_time,
+            heartbeat_peer_reputation_config,
         }
     }
 }
@@ -348,21 +357,39 @@ impl<P: TaskP2PService, D, B: Broadcast> Task<P, D, B> {
             {
                 tracing::debug!("Peer {:?} has old heartbeat", peer_id);
                 let report = HeartBeatPeerReportReason::OldHeartBeat;
-                let service = "p2p";
                 let peer_id = convert_peer_id(peer_id)?;
-                self.broadcast.report_peer(peer_id, report, service)?;
+                self.report_peer(peer_id, report)?;
             } else if peer_info.heartbeat_data.average_time_between_heartbeats()
                 > self.heartbeat_max_avg_interval
             {
                 tracing::debug!("Peer {:?} has low heartbeat frequency", peer_id);
                 let report = HeartBeatPeerReportReason::LowHeartBeatFrequency;
-                let service = "p2p";
                 let peer_id = convert_peer_id(peer_id)?;
-                self.broadcast.report_peer(peer_id, report, service)?;
+                self.report_peer(peer_id, report)?;
             }
         }
         Ok(())
     }
+
+    fn report_peer(
+        &self,
+        peer_id: FuelPeerId,
+        report: HeartBeatPeerReportReason,
+    ) -> anyhow::Result<()> {
+        let app_score = match report {
+            HeartBeatPeerReportReason::OldHeartBeat => {
+                self.heartbeat_peer_reputation_config.old_heartbeat_penalty
+            }
+            HeartBeatPeerReportReason::LowHeartBeatFrequency => {
+                self.heartbeat_peer_reputation_config
+                    .low_heartbeat_frequency_penalty
+            }
+        };
+        let reporting_service = "p2p";
+        self.broadcast
+            .report_peer(peer_id, app_score, reporting_service)?;
+        Ok(())
+    }
 }
 
 fn convert_peer_id(peer_id: &PeerId) -> anyhow::Result<FuelPeerId> {
@@ -976,14 +1003,14 @@ pub mod tests {
     }
 
     struct FakeBroadcast {
-        pub peer_reports: mpsc::Sender<(FuelPeerId, HeartBeatPeerReportReason, String)>,
+        pub peer_reports: mpsc::Sender<(FuelPeerId, AppScore, String)>,
     }
 
     impl Broadcast for FakeBroadcast {
         fn report_peer(
             &self,
             peer_id: FuelPeerId,
-            report: HeartBeatPeerReportReason,
+            report: AppScore,
             reporting_service: &'static str,
         ) -> anyhow::Result<()> {
             self.peer_reports.try_send((
@@ -1044,6 +1071,12 @@ pub mod tests {
         // Greater than actual
         let heartbeat_max_time_since_last = Duration::from_secs(40);
 
+        // Arbitrary values
+        let heartbeat_peer_reputation_config = HeartbeatPeerReputationConfig {
+            old_heartbeat_penalty: 5.6,
+            low_heartbeat_frequency_penalty: 20.45,
+        };
+
         let mut task = Task {
             p2p_service,
             db: Arc::new(FakeDB),
@@ -1055,6 +1088,7 @@ pub mod tests {
             heartbeat_max_avg_interval,
             heartbeat_max_time_since_last,
             next_check_time: Instant::now(),
+            heartbeat_peer_reputation_config: heartbeat_peer_reputation_config.clone(),
         };
         let (watch_sender, watch_receiver) = tokio::sync::watch::channel(State::Started);
         let mut watcher = StateWatcher::from(watch_receiver);
@@ -1072,7 +1106,10 @@ pub mod tests {
             FuelPeerId::from(peer_id.to_bytes().to_vec()),
             report_peer_id
         );
-        assert_eq!(report, HeartBeatPeerReportReason::LowHeartBeatFrequency);
+        assert_eq!(
+            report,
+            heartbeat_peer_reputation_config.low_heartbeat_frequency_penalty
+        );
         assert_eq!(reporting_service, "p2p");
     }
 
@@ -1112,6 +1149,12 @@ pub mod tests {
         // Less than actual
         let heartbeat_max_time_since_last = Duration::from_secs(40);
 
+        // Arbitrary values
+        let heartbeat_peer_reputation_config = HeartbeatPeerReputationConfig {
+            old_heartbeat_penalty: 5.6,
+            low_heartbeat_frequency_penalty: 20.45,
+        };
+
         let mut task = Task {
             p2p_service,
             db: Arc::new(FakeDB),
@@ -1123,6 +1166,7 @@ pub mod tests {
             heartbeat_max_avg_interval,
             heartbeat_max_time_since_last,
             next_check_time: Instant::now(),
+            heartbeat_peer_reputation_config: heartbeat_peer_reputation_config.clone(),
         };
         let (watch_sender, watch_receiver) = tokio::sync::watch::channel(State::Started);
         let mut watcher = StateWatcher::from(watch_receiver);
@@ -1140,7 +1184,10 @@ pub mod tests {
             FuelPeerId::from(peer_id.to_bytes().to_vec()),
             report_peer_id
         );
-        assert_eq!(report, HeartBeatPeerReportReason::OldHeartBeat);
+        assert_eq!(
+            report,
+            heartbeat_peer_reputation_config.old_heartbeat_penalty
+        );
         assert_eq!(reporting_service, "p2p");
     }
 }
diff --git a/crates/types/src/services/p2p/peer_reputation.rs b/crates/types/src/services/p2p/peer_reputation.rs
@@ -15,3 +15,9 @@ pub trait PeerReport {
     /// Extracts PeerScore from the Report
     fn get_score_from_report(&self) -> AppScore;
 }
+
+impl PeerReport for AppScore {
+    fn get_score_from_report(&self) -> AppScore {
+        *self
+    }
+}