extract warn side-effect from ProposalMonitor

Author: ikwuoz

crates/malachite-app/src/handlers/get_value.rs

@@ -26,7 +26,7 @@ use malachitebft_app_channel::app::types::LocallyProposedValue;
 use malachitebft_app_channel::{NetworkMsg, Reply};
 use malachitebft_core_types::Validity;
 
-use arc_consensus_types::{Address, ArcContext, Height};
+use arc_consensus_types::{Address, ArcContext, Height, ValueId};
 use arc_eth_engine::engine::Engine;
 use arc_eth_engine::json_structures::ExecutionBlock;
 use arc_signer::ArcSigningProvider;
@@ -90,12 +90,7 @@ pub async fn handle(
 
     if let Some(proposed_value) = proposed_value {
         if round.as_i64() == 0 {
-            if let Some(monitor) = &mut state.proposal_monitor {
-                debug_assert_eq!(monitor.height, height, "proposal monitor height mismatch");
-                monitor.record_proposal(proposed_value.value.id());
-            } else {
-                warn!(%height, %round, "No proposal monitor present");
-            }
+            record_self_proposal_in_monitor(state, height, proposed_value.value.id());
         }
 
         if let Err(e) = reply.send(proposed_value) {
@@ -308,3 +303,20 @@ async fn get_previously_built_block(
     let block = blocks.into_iter().find(|p| p.proposer == proposer);
     Ok(block)
 }
+
+/// Records the proposed value in the proposal monitor for the given height, if the monitor exists.
+fn record_self_proposal_in_monitor(state: &mut State, height: Height, value_id: ValueId) {
+    let Some(monitor) = &mut state.proposal_monitor else {
+        warn!(%height, round = 0, "No proposal monitor present");
+        return;
+    };
+    debug_assert_eq!(monitor.height, height, "proposal monitor height mismatch");
+    if !monitor.record_proposal(value_id) {
+        warn!(
+            height = %monitor.height,
+            first_value = %monitor.value_id.unwrap(),
+            new_value = %value_id,
+            "Equivocating proposal at round 0"
+        );
+    }
+}

crates/malachite-app/src/handlers/received_proposal_part.rs

@@ -128,7 +128,14 @@ fn record_proposal_in_monitor(state: &mut State, proposed_value: &ProposedValue<
         return;
     }
 
-    monitor.record_proposal(proposed_value.value.id());
+    if !monitor.record_proposal(proposed_value.value.id()) {
+        warn!(
+            height = %monitor.height,
+            first_value = %monitor.value_id.unwrap(),
+            new_value = %proposed_value.value.id(),
+            "Equivocating proposal at round 0"
+        );
+    }
 }
 
 struct HandlerContext<'a, 'b> {

crates/types/src/proposal_monitor.rs

@@ -27,7 +27,7 @@
 //! By design, monitoring data is only stored for round-0 proposals.
 
 use std::time::SystemTime;
-use tracing::warn;
+// use tracing::warn;
 
 use crate::{Address, Height, ValueId};
 
@@ -117,22 +117,16 @@ impl ProposalMonitor {
 
     /// Record that proposal was received.
     /// Takes precedence over synced value.
-    pub fn record_proposal(&mut self, value_id: ValueId) {
-        // FIXME: this log message should not be produced here.
-        if let Some(first_value) = self.value_id
-            && !self.synced
-        {
-            warn!(
-                height = %self.height,
-                %first_value,
-                new_value = %value_id,
-                "Equivocating proposal at round 0"
-            );
-            return;
+    /// Returns `true` if this is the first proposal recorded,
+    /// `false` if a previous proposal exist (equivocation).
+    pub fn record_proposal(&mut self, value_id: ValueId) -> bool {
+        if self.value_id.is_some() && !self.synced {
+            return false;
         }
         self.proposal_receive_time = Some(SystemTime::now());
         self.value_id = Some(value_id);
         self.synced = false;
+        true
     }
 
     /// Check if the decided value matches the recorded proposal.
@@ -236,6 +230,22 @@ mod tests {
         assert_eq!(monitor.value_id, Some(value_id1));
     }
 
+    #[test]
+    fn test_record_proposal_duplicate_returns_false() {
+        let mut monitor = ProposalMonitor::new(Height::new(1), test_address(), SystemTime::now());
+
+        let value_id1 = test_value_id(0x11);
+        assert!(monitor.record_proposal(value_id1));
+        let time1 = monitor.proposal_receive_time.unwrap();
+
+        // Second proposal returns false (equivocation), first value preserved
+        let value_id2 = test_value_id(0x22);
+        assert!(!monitor.record_proposal(value_id2));
+
+        assert_eq!(monitor.proposal_receive_time, Some(time1));
+        assert_eq!(monitor.value_id, Some(value_id1));
+    }
+
     #[test]
     fn test_mark_decided_successful() {
         let mut monitor = ProposalMonitor::new(Height::new(1), test_address(), SystemTime::now());