From 6bc70f5c2464b16740ea7cbc79b1b269a31885e9 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Wed, 31 Jan 2024 18:44:37 +0000 Subject: [PATCH 1/4] Downgrade duplicate publish logs --- beacon_node/lighthouse_network/src/config.rs | 5 --- .../lighthouse_network/src/service/mod.rs | 35 +++++++++---------- beacon_node/src/cli.rs | 10 ------ beacon_node/src/config.rs | 3 -- book/src/help_bn.md | 5 --- book/src/redundancy.md | 4 --- lighthouse/tests/beacon_node.rs | 19 ---------- 7 files changed, 17 insertions(+), 64 deletions(-) diff --git a/beacon_node/lighthouse_network/src/config.rs b/beacon_node/lighthouse_network/src/config.rs index 86bae4331b7..93ab3d2d81b 100644 --- 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, - - /// 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 index 5d35ed509db..73e414e946f 100644 --- 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 { 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, } @@ -426,7 +424,6 @@ impl Network { update_gossipsub_scores, gossip_cache, local_peer_id, - disable_duplicate_warn_logs: config.disable_duplicate_warn_logs, log, }; @@ -745,21 +742,23 @@ impl Network { .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 index 002bb344a3e..6022eae39aa 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -1268,15 +1268,5 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .default_value("64") .takes_value(true) ) - .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.") - .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 index fff384e195f..b485775dbfe 100644 --- 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 index dff2ab6876a..ec550a45ad5 100644 --- a/book/src/help_bn.md +++ b/book/src/help_bn.md @@ -32,11 +32,6 @@ 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. -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 index 11b98456587..bd1976f9503 100644 --- 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 index 3efcb2d0e5f..f97f17a6677 100644 --- 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); - }); -} From 0a947c79ba2deb72c8e0e85f267a0db588fea56d Mon Sep 17 00:00:00 2001 From: Age Manning Date: Sun, 4 Feb 2024 15:59:49 +0000 Subject: [PATCH 2/4] Maintain backwards compatiblity, deprecate flag --- beacon_node/src/cli.rs | 6 ++++++ lighthouse/tests/beacon_node.rs | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index 6022eae39aa..4ed60d27b0f 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -1268,5 +1268,11 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .default_value("64") .takes_value(true) ) + .arg( + Arg::with_name("disable-duplicate-warn-logs") + .long("disable-duplicate-warn-logs") + .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/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index f97f17a6677..3efcb2d0e5f 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -2584,3 +2584,22 @@ 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); + }); +} From 7c35e2a5a601bae4ffd37759af4d6905bec54b2e Mon Sep 17 00:00:00 2001 From: Age Manning Date: Mon, 5 Feb 2024 09:27:21 +0000 Subject: [PATCH 3/4] The tests had to go, because there's no config to test against --- lighthouse/tests/beacon_node.rs | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 3efcb2d0e5f..f97f17a6677 100644 --- 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); - }); -} From 88b21a6c776f730507a8ae847cde0006f8cad765 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Tue, 6 Feb 2024 11:44:03 +1100 Subject: [PATCH 4/4] Update help_bn.md --- book/src/help_bn.md | 1 + 1 file changed, 1 insertion(+) diff --git a/book/src/help_bn.md b/book/src/help_bn.md index ec550a45ad5..3d2124964b2 100644 --- a/book/src/help_bn.md +++ b/book/src/help_bn.md @@ -32,6 +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 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.