simplifies gossip::Protocol signature verification (#4259)

Gossip packets which do not sigverify can be any bogus value and it does not make sense to add overhead for them just to collect metrics. The number of gossip packets which do not pass sigverify is already captured in the difference of these two metrics: https://github.com/anza-xyz/agave/blob/64435164f/gossip/src/cluster_info.rs#L2329-L2331 https://github.com/anza-xyz/agave/blob/64435164f/gossip/src/cluster_info.rs#L2353-L255 Additionally if any of the CrdsValues in a PullResponse or PushMessage does not sigverify, we can discard the entire packet.
anza-xyz · Jan 3, 2025 · d68b5de · d68b5de
1 parent eda57c9
commit d68b5de
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 103 deletions.
diff --git a/gossip/src/cluster_info.rs b/gossip/src/cluster_info.rs
@@ -2316,15 +2316,16 @@ impl ClusterInfo {
                     .add_relaxed(excess_count as u64);
             }
         }
-        let verify_packet = |packet: Packet| {
+        fn verify_packet(packet: &Packet) -> Option<(SocketAddr, Protocol)> {
             let protocol: Protocol = packet.deserialize_slice(..).ok()?;
             protocol.sanitize().ok()?;
-            let protocol = protocol.par_verify(&self.stats)?;
-            Some((packet.meta().socket_addr(), protocol))
-        };
+            protocol
+                .par_verify()
+                .then(|| (packet.meta().socket_addr(), protocol))
+        }
         let packets: Vec<_> = {
             let _st = ScopedTimer::from(&self.stats.verify_gossip_packets_time);
-            thread_pool.install(|| packets.into_par_iter().filter_map(verify_packet).collect())
+            thread_pool.install(|| packets.par_iter().filter_map(verify_packet).collect())
         };
         self.stats
             .packets_received_count

diff --git a/gossip/src/cluster_info_metrics.rs b/gossip/src/cluster_info_metrics.rs
@@ -106,15 +106,9 @@ pub struct GossipStats {
     pub(crate) gossip_listen_loop_iterations_since_last_report: Counter,
     pub(crate) gossip_listen_loop_time: Counter,
     pub(crate) gossip_packets_dropped_count: Counter,
-    pub(crate) gossip_ping_msg_verify_fail: Counter,
-    pub(crate) gossip_pong_msg_verify_fail: Counter,
-    pub(crate) gossip_prune_msg_verify_fail: Counter,
     pub(crate) gossip_pull_request_dropped_requests: Counter,
     pub(crate) gossip_pull_request_no_budget: Counter,
     pub(crate) gossip_pull_request_sent_requests: Counter,
-    pub(crate) gossip_pull_request_verify_fail: Counter,
-    pub(crate) gossip_pull_response_verify_fail: Counter,
-    pub(crate) gossip_push_msg_verify_fail: Counter,
     pub(crate) gossip_transmit_loop_iterations_since_last_report: Counter,
     pub(crate) gossip_transmit_loop_time: Counter,
     pub(crate) handle_batch_ping_messages_time: Counter,
@@ -584,36 +578,6 @@ pub(crate) fn submit_gossip_stats(
             stats.trim_crds_table_purged_values_count.clear(),
             i64
         ),
-        (
-            "gossip_pull_request_verify_fail",
-            stats.gossip_pull_request_verify_fail.clear(),
-            i64
-        ),
-        (
-            "gossip_pull_response_verify_fail",
-            stats.gossip_pull_response_verify_fail.clear(),
-            i64
-        ),
-        (
-            "gossip_push_msg_verify_fail",
-            stats.gossip_push_msg_verify_fail.clear(),
-            i64
-        ),
-        (
-            "gossip_prune_msg_verify_fail",
-            stats.gossip_prune_msg_verify_fail.clear(),
-            i64
-        ),
-        (
-            "gossip_ping_msg_verify_fail",
-            stats.gossip_ping_msg_verify_fail.clear(),
-            i64
-        ),
-        (
-            "gossip_pong_msg_verify_fail",
-            stats.gossip_pong_msg_verify_fail.clear(),
-            i64
-        ),
     );
     datapoint_info!(
         "cluster_info_crds_stats",

diff --git a/gossip/src/protocol.rs b/gossip/src/protocol.rs
@@ -1,6 +1,5 @@
 use {
     crate::{
-        cluster_info_metrics::GossipStats,
         crds_data::MAX_WALLCLOCK,
         crds_gossip_pull::CrdsFilter,
         crds_value::CrdsValue,
@@ -86,68 +85,16 @@ impl Protocol {
             .unwrap()
     }
 
-    pub(crate) fn par_verify(self, stats: &GossipStats) -> Option<Self> {
+    // Returns true if all signatures verify.
+    #[must_use]
+    pub(crate) fn par_verify(&self) -> bool {
         match self {
-            Protocol::PullRequest(_, ref caller) => {
-                if caller.verify() {
-                    Some(self)
-                } else {
-                    stats.gossip_pull_request_verify_fail.add_relaxed(1);
-                    None
-                }
-            }
-            Protocol::PullResponse(from, data) => {
-                let size = data.len();
-                let data: Vec<_> = data.into_par_iter().filter(Signable::verify).collect();
-                if size != data.len() {
-                    stats
-                        .gossip_pull_response_verify_fail
-                        .add_relaxed((size - data.len()) as u64);
-                }
-                if data.is_empty() {
-                    None
-                } else {
-                    Some(Protocol::PullResponse(from, data))
-                }
-            }
-            Protocol::PushMessage(from, data) => {
-                let size = data.len();
-                let data: Vec<_> = data.into_par_iter().filter(Signable::verify).collect();
-                if size != data.len() {
-                    stats
-                        .gossip_push_msg_verify_fail
-                        .add_relaxed((size - data.len()) as u64);
-                }
-                if data.is_empty() {
-                    None
-                } else {
-                    Some(Protocol::PushMessage(from, data))
-                }
-            }
-            Protocol::PruneMessage(_, ref data) => {
-                if data.verify() {
-                    Some(self)
-                } else {
-                    stats.gossip_prune_msg_verify_fail.add_relaxed(1);
-                    None
-                }
-            }
-            Protocol::PingMessage(ref ping) => {
-                if ping.verify() {
-                    Some(self)
-                } else {
-                    stats.gossip_ping_msg_verify_fail.add_relaxed(1);
-                    None
-                }
-            }
-            Protocol::PongMessage(ref pong) => {
-                if pong.verify() {
-                    Some(self)
-                } else {
-                    stats.gossip_pong_msg_verify_fail.add_relaxed(1);
-                    None
-                }
-            }
+            Self::PullRequest(_, caller) => caller.verify(),
+            Self::PullResponse(_, data) => data.par_iter().all(CrdsValue::verify),
+            Self::PushMessage(_, data) => data.par_iter().all(CrdsValue::verify),
+            Self::PruneMessage(_, data) => data.verify(),
+            Self::PingMessage(ping) => ping.verify(),
+            Self::PongMessage(pong) => pong.verify(),
         }
     }
 }