Downgrade gossipsub duplicate logs (sigp#5163)

* Downgrade duplicate publish logs * Maintain backwards compatiblity, deprecate flag * The tests had to go, because there's no config to test against * Update help_bn.md
dapplion · Feb 6, 2024 · 8530427 · 8530427
1 parent 5cc29e4
commit 8530427
Show file tree

Hide file tree

Showing 7 changed files with 19 additions and 59 deletions.
diff --git a/beacon_node/lighthouse_network/src/config.rs b/beacon_node/lighthouse_network/src/config.rs
@@ -157,10 +157,6 @@ pub struct Config {
 
     /// Configuration for the inbound rate limiter (requests received by this node).
     pub inbound_rate_limiter_config: Option<InboundRateLimiterConfig>,
-
-    /// Whether to disable logging duplicate gossip messages as WARN. If set to true, duplicate
-    /// errors will be logged at DEBUG level.
-    pub disable_duplicate_warn_logs: bool,
 }
 
 impl Config {
@@ -378,7 +374,6 @@ impl Default for Config {
             outbound_rate_limiter_config: None,
             invalid_block_storage: None,
             inbound_rate_limiter_config: None,
-            disable_duplicate_warn_logs: false,
         }
     }
 }

diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs
@@ -127,8 +127,6 @@ pub struct Network<AppReqId: ReqId, TSpec: EthSpec> {
     gossip_cache: GossipCache,
     /// This node's PeerId.
     pub local_peer_id: PeerId,
-    /// Flag to disable warning logs for duplicate gossip messages and log at DEBUG level instead.
-    pub disable_duplicate_warn_logs: bool,
     /// Logger for behaviour actions.
     log: slog::Logger,
 }
@@ -427,7 +425,6 @@ impl<AppReqId: ReqId, TSpec: EthSpec> Network<AppReqId, TSpec> {
             update_gossipsub_scores,
             gossip_cache,
             local_peer_id,
-            disable_duplicate_warn_logs: config.disable_duplicate_warn_logs,
             log,
         };
 
@@ -746,21 +743,23 @@ impl<AppReqId: ReqId, TSpec: EthSpec> Network<AppReqId, TSpec> {
                     .gossipsub_mut()
                     .publish(Topic::from(topic.clone()), message_data.clone())
                 {
-                    if self.disable_duplicate_warn_logs && matches!(e, PublishError::Duplicate) {
-                        debug!(
-                            self.log,
-                            "Could not publish message";
-                            "error" => ?e,
-                            "kind" => %topic.kind(),
-                        );
-                    } else {
-                        warn!(
-                            self.log,
-                            "Could not publish message";
-                            "error" => ?e,
-                            "kind" => %topic.kind(),
-                        );
-                    };
+                    match e {
+                        PublishError::Duplicate => {
+                            debug!(
+                                self.log,
+                                "Attempted to publish duplicate message";
+                                "kind" => %topic.kind(),
+                            );
+                        }
+                        ref e => {
+                            warn!(
+                                self.log,
+                                "Could not publish message";
+                                "error" => ?e,
+                                "kind" => %topic.kind(),
+                            );
+                        }
+                    }
 
                     // add to metrics
                     match topic.kind() {

diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs
@@ -1271,11 +1271,7 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
         .arg(
             Arg::with_name("disable-duplicate-warn-logs")
                 .long("disable-duplicate-warn-logs")
-                .help("Disable warning logs for duplicate gossip messages. The WARN level log is \
-                    useful for detecting a duplicate validator key running elsewhere. However, this may \
-                    result in excessive warning logs if the validator is broadcasting messages to \
-                    multiple beacon nodes via the validator client --broadcast flag. In this case, \
-                    disabling these warn logs may be useful.")
+                .help("This flag is deprecated and has no effect.")
                 .takes_value(false)
         )
         .group(ArgGroup::with_name("enable_http").args(&["http", "gui", "staking"]).multiple(true))

diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs
@@ -1425,9 +1425,6 @@ pub fn set_network_config(
             Some(config_str.parse()?)
         }
     };
-
-    config.disable_duplicate_warn_logs = cli_args.is_present("disable-duplicate-warn-logs");
-
     Ok(())
 }
 

diff --git a/book/src/help_bn.md b/book/src/help_bn.md
@@ -32,11 +32,7 @@ FLAGS:
         --disable-deposit-contract-sync        Explicitly disables syncing of deposit logs from the execution node. This
                                                overrides any previous option that depends on it. Useful if you intend to
                                                run a non-validating beacon node.
-        --disable-duplicate-warn-logs          Disable warning logs for duplicate gossip messages. The WARN level log is
-                                               useful for detecting a duplicate validator key running elsewhere.
-                                               However, this may result in excessive warning logs if the validator is
-                                               broadcasting messages to multiple beacon nodes via the validator client
-                                               --broadcast flag. In this case, disabling these warn logs may be useful.
+        --disable-duplicate-warn-logs          This flag is deprecated and has no effect.
     -x, --disable-enr-auto-update              Discovery automatically updates the nodes local ENR with an external IP
                                                address and port as seen by other peers on the network. This disables
                                                this feature, fixing the ENR's IP/PORT to those specified on boot.

diff --git a/book/src/redundancy.md b/book/src/redundancy.md
@@ -101,10 +101,6 @@ from this list:
 - `none`: Disable all broadcasting. This option only has an effect when provided alone, otherwise
    it is ignored. Not recommended except for expert tweakers.
 
-Broadcasting attestation, blocks and sync committee messages may result in excessive warning logs in the beacon node 
-due to duplicate gossip messages. In this case, it may be desirable to disable warning logs for duplicates using the 
-beacon node `--disable-duplicate-warn-logs` flag.
-
 The default is `--broadcast subscriptions`. To also broadcast blocks for example, use
 `--broadcast subscriptions,blocks`.
 

diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs
@@ -2584,22 +2584,3 @@ fn genesis_state_url_value() {
             assert_eq!(config.genesis_state_url_timeout, Duration::from_secs(42));
         });
 }
-
-#[test]
-fn disable_duplicate_warn_logs_default() {
-    CommandLineTest::new()
-        .run_with_zero_port()
-        .with_config(|config| {
-            assert_eq!(config.network.disable_duplicate_warn_logs, false);
-        });
-}
-
-#[test]
-fn disable_duplicate_warn_logs() {
-    CommandLineTest::new()
-        .flag("disable-duplicate-warn-logs", None)
-        .run_with_zero_port()
-        .with_config(|config| {
-            assert_eq!(config.network.disable_duplicate_warn_logs, true);
-        });
-}