From ca8eaea11677cd6832205ff02db91cfd115ae45f Mon Sep 17 00:00:00 2001 From: ThreeHrSleep <151536303+ThreeHrSleep@users.noreply.github.com> Date: Thu, 27 Mar 2025 06:56:03 +0530 Subject: [PATCH 01/17] Remove `crit` as an option from the CLI entirely (#7169) https://github.com/sigp/lighthouse/issues/7165 --- book/src/help_bn.md | 5 ++--- book/src/help_general.md | 5 ++--- book/src/help_vc.md | 5 ++--- book/src/help_vm.md | 5 ++--- book/src/help_vm_create.md | 5 ++--- book/src/help_vm_import.md | 5 ++--- book/src/help_vm_move.md | 5 ++--- lighthouse/src/main.rs | 4 ++-- 8 files changed, 16 insertions(+), 23 deletions(-) diff --git a/book/src/help_bn.md b/book/src/help_bn.md index a99aae30b1..224db099e7 100644 --- a/book/src/help_bn.md +++ b/book/src/help_bn.md @@ -78,8 +78,7 @@ Options: custom datadirs for different networks. --debug-level Specifies the verbosity level used when emitting logs to the terminal. - [default: info] [possible values: info, debug, trace, warn, error, - crit] + [default: info] [possible values: info, debug, trace, warn, error] --discovery-port The UDP port that discovery will listen on. Defaults to `port` --discovery-port6 @@ -247,7 +246,7 @@ Options: [possible values: JSON] --logfile-debug-level The verbosity level used when emitting logs to the log file. [default: - debug] [possible values: info, debug, trace, warn, error, crit] + debug] [possible values: info, debug, trace, warn, error] --logfile-dir Directory path where the log file will be stored --logfile-format diff --git a/book/src/help_general.md b/book/src/help_general.md index 9c449b2835..4de3270cfc 100644 --- a/book/src/help_general.md +++ b/book/src/help_general.md @@ -42,8 +42,7 @@ Options: custom datadirs for different networks. --debug-level Specifies the verbosity level used when emitting logs to the terminal. - [default: info] [possible values: info, debug, trace, warn, error, - crit] + [default: info] [possible values: info, debug, trace, warn, error] --genesis-state-url A URL of a beacon-API compatible server from which to download the genesis state. Checkpoint sync server URLs can generally be used with @@ -58,7 +57,7 @@ Options: [possible values: JSON] --logfile-debug-level The verbosity level used when emitting logs to the log file. [default: - debug] [possible values: info, debug, trace, warn, error, crit] + debug] [possible values: info, debug, trace, warn, error] --logfile-dir Directory path where the log file will be stored --logfile-format diff --git a/book/src/help_vc.md b/book/src/help_vc.md index fb2c8de876..e01828aea2 100644 --- a/book/src/help_vc.md +++ b/book/src/help_vc.md @@ -35,8 +35,7 @@ Options: custom datadirs for different networks. --debug-level Specifies the verbosity level used when emitting logs to the terminal. - [default: info] [possible values: info, debug, trace, warn, error, - crit] + [default: info] [possible values: info, debug, trace, warn, error] --gas-limit The gas limit to be used in all builder proposals for all validators managed by this validator client. Note this will not necessarily be @@ -79,7 +78,7 @@ Options: [possible values: JSON] --logfile-debug-level The verbosity level used when emitting logs to the log file. [default: - debug] [possible values: info, debug, trace, warn, error, crit] + debug] [possible values: info, debug, trace, warn, error] --logfile-dir Directory path where the log file will be stored --logfile-format diff --git a/book/src/help_vm.md b/book/src/help_vm.md index 3907064696..fdb035aa05 100644 --- a/book/src/help_vm.md +++ b/book/src/help_vm.md @@ -39,8 +39,7 @@ Options: custom datadirs for different networks. --debug-level Specifies the verbosity level used when emitting logs to the terminal. - [default: info] [possible values: info, debug, trace, warn, error, - crit] + [default: info] [possible values: info, debug, trace, warn, error] --genesis-state-url A URL of a beacon-API compatible server from which to download the genesis state. Checkpoint sync server URLs can generally be used with @@ -55,7 +54,7 @@ Options: [possible values: JSON] --logfile-debug-level The verbosity level used when emitting logs to the log file. [default: - debug] [possible values: info, debug, trace, warn, error, crit] + debug] [possible values: info, debug, trace, warn, error] --logfile-dir Directory path where the log file will be stored --logfile-format diff --git a/book/src/help_vm_create.md b/book/src/help_vm_create.md index d83b1000b9..c6bfe2e95a 100644 --- a/book/src/help_vm_create.md +++ b/book/src/help_vm_create.md @@ -33,8 +33,7 @@ Options: custom datadirs for different networks. --debug-level Specifies the verbosity level used when emitting logs to the terminal. - [default: info] [possible values: info, debug, trace, warn, error, - crit] + [default: info] [possible values: info, debug, trace, warn, error] --deposit-gwei The GWEI value of the deposit amount. Defaults to the minimum amount required for an active validator (MAX_EFFECTIVE_BALANCE) @@ -62,7 +61,7 @@ Options: [possible values: JSON] --logfile-debug-level The verbosity level used when emitting logs to the log file. [default: - debug] [possible values: info, debug, trace, warn, error, crit] + debug] [possible values: info, debug, trace, warn, error] --logfile-dir Directory path where the log file will be stored --logfile-format diff --git a/book/src/help_vm_import.md b/book/src/help_vm_import.md index 2dd7d5f70a..8da3eee9b0 100644 --- a/book/src/help_vm_import.md +++ b/book/src/help_vm_import.md @@ -23,8 +23,7 @@ Options: custom datadirs for different networks. --debug-level Specifies the verbosity level used when emitting logs to the terminal. - [default: info] [possible values: info, debug, trace, warn, error, - crit] + [default: info] [possible values: info, debug, trace, warn, error] --gas-limit When provided, the imported validator will use this gas limit. It is recommended to leave this as the default value by not specifying this @@ -47,7 +46,7 @@ Options: [possible values: JSON] --logfile-debug-level The verbosity level used when emitting logs to the log file. [default: - debug] [possible values: info, debug, trace, warn, error, crit] + debug] [possible values: info, debug, trace, warn, error] --logfile-dir Directory path where the log file will be stored --logfile-format diff --git a/book/src/help_vm_move.md b/book/src/help_vm_move.md index 2f068a1f88..83824ce768 100644 --- a/book/src/help_vm_move.md +++ b/book/src/help_vm_move.md @@ -25,8 +25,7 @@ Options: custom datadirs for different networks. --debug-level Specifies the verbosity level used when emitting logs to the terminal. - [default: info] [possible values: info, debug, trace, warn, error, - crit] + [default: info] [possible values: info, debug, trace, warn, error] --dest-vc-token The file containing a token required by the destination validator client. @@ -51,7 +50,7 @@ Options: [possible values: JSON] --logfile-debug-level The verbosity level used when emitting logs to the log file. [default: - debug] [possible values: info, debug, trace, warn, error, crit] + debug] [possible values: info, debug, trace, warn, error] --logfile-dir Directory path where the log file will be stored --logfile-format diff --git a/lighthouse/src/main.rs b/lighthouse/src/main.rs index a2432e282d..2b7387e076 100644 --- a/lighthouse/src/main.rs +++ b/lighthouse/src/main.rs @@ -140,7 +140,7 @@ fn main() { .value_name("LEVEL") .help("The verbosity level used when emitting logs to the log file.") .action(ArgAction::Set) - .value_parser(["info", "debug", "trace", "warn", "error", "crit"]) + .value_parser(["info", "debug", "trace", "warn", "error"]) .default_value("debug") .global(true) .display_order(0) @@ -261,7 +261,7 @@ fn main() { .value_name("LEVEL") .help("Specifies the verbosity level used when emitting logs to the terminal.") .action(ArgAction::Set) - .value_parser(["info", "debug", "trace", "warn", "error", "crit"]) + .value_parser(["info", "debug", "trace", "warn", "error"]) .global(true) .default_value("info") .display_order(0) From 0875326cb6885b9b40b79f3520ecac64bb5a3a00 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 27 Mar 2025 12:53:38 +1100 Subject: [PATCH 02/17] Prevent duplicate effective balance processing (#7209) --- .../src/per_epoch_processing/single_pass.rs | 88 +++++++++++++++---- 1 file changed, 71 insertions(+), 17 deletions(-) diff --git a/consensus/state_processing/src/per_epoch_processing/single_pass.rs b/consensus/state_processing/src/per_epoch_processing/single_pass.rs index 5c31669a60..af6a0936e2 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -175,6 +175,7 @@ pub fn process_epoch_single_pass( let mut earliest_exit_epoch = state.earliest_exit_epoch().ok(); let mut exit_balance_to_consume = state.exit_balance_to_consume().ok(); + let validators_in_consolidations = get_validators_in_consolidations(state); // Split the state into several disjoint mutable borrows. let ( @@ -317,17 +318,26 @@ pub fn process_epoch_single_pass( // `process_effective_balance_updates` if conf.effective_balance_updates { - process_single_effective_balance_update( - validator_info.index, - *balance, - &mut validator, - validator_info.current_epoch_participation, - &mut next_epoch_cache, - progressive_balances, - effective_balances_ctxt, - state_ctxt, - spec, - )?; + if validators_in_consolidations.contains(&validator_info.index) { + process_single_dummy_effective_balance_update( + validator_info.index, + &validator, + &mut next_epoch_cache, + state_ctxt, + )?; + } else { + process_single_effective_balance_update( + validator_info.index, + *balance, + &mut validator, + validator_info.current_epoch_participation, + &mut next_epoch_cache, + progressive_balances, + effective_balances_ctxt, + state_ctxt, + spec, + )?; + } } } @@ -430,6 +440,7 @@ pub fn process_epoch_single_pass( if fork_name.electra_enabled() && conf.pending_consolidations { process_pending_consolidations( state, + &validators_in_consolidations, &mut next_epoch_cache, effective_balances_ctxt, conf.effective_balance_updates, @@ -1026,12 +1037,38 @@ fn process_pending_deposits_for_validator( Ok(()) } +/// Return the set of validators referenced by consolidations, either as source or target. +/// +/// This function is blind to whether the consolidations are valid and capable of being processed, +/// it just returns the set of all indices present in consolidations. This is *sufficient* to +/// make consolidations play nicely with effective balance updates. The algorithm used is: +/// +/// - In the single pass: apply effective balance updates for all validators *not* referenced by +/// consolidations. +/// - Apply consolidations. +/// - Apply effective balance updates for all validators previously skipped. +/// +/// Prior to Electra, the empty set is returned. +fn get_validators_in_consolidations(state: &BeaconState) -> BTreeSet { + let mut referenced_validators = BTreeSet::new(); + + if let Ok(pending_consolidations) = state.pending_consolidations() { + for pending_consolidation in pending_consolidations { + referenced_validators.insert(pending_consolidation.source_index as usize); + referenced_validators.insert(pending_consolidation.target_index as usize); + } + } + + referenced_validators +} + /// We process pending consolidations after all of single-pass epoch processing, and then patch up /// the effective balances for affected validators. /// /// This is safe because processing consolidations does not depend on the `effective_balance`. fn process_pending_consolidations( state: &mut BeaconState, + validators_in_consolidations: &BTreeSet, next_epoch_cache: &mut PreEpochCache, effective_balances_ctxt: &EffectiveBalancesContext, perform_effective_balance_updates: bool, @@ -1042,8 +1079,6 @@ fn process_pending_consolidations( let next_epoch = state.next_epoch()?; let pending_consolidations = state.pending_consolidations()?.clone(); - let mut affected_validators = BTreeSet::new(); - for pending_consolidation in &pending_consolidations { let source_index = pending_consolidation.source_index as usize; let target_index = pending_consolidation.target_index as usize; @@ -1069,9 +1104,6 @@ fn process_pending_consolidations( decrease_balance(state, source_index, source_effective_balance)?; increase_balance(state, target_index, source_effective_balance)?; - affected_validators.insert(source_index); - affected_validators.insert(target_index); - next_pending_consolidation.safe_add_assign(1)?; } @@ -1087,7 +1119,7 @@ fn process_pending_consolidations( // Re-process effective balance updates for validators affected by consolidations. let (validators, balances, _, current_epoch_participation, _, progressive_balances, _, _) = state.mutable_validator_fields()?; - for validator_index in affected_validators { + for &validator_index in validators_in_consolidations { let balance = *balances .get(validator_index) .ok_or(BeaconStateError::UnknownValidator(validator_index))?; @@ -1129,6 +1161,28 @@ impl EffectiveBalancesContext { } } +/// This function is called for validators that do not have their effective balance updated as +/// part of the single-pass loop. For these validators we compute their true effective balance +/// update after processing consolidations. However, to maintain the invariants of the +/// `PreEpochCache` we must register _some_ effective balance for them immediately. +fn process_single_dummy_effective_balance_update( + validator_index: usize, + validator: &Cow, + next_epoch_cache: &mut PreEpochCache, + state_ctxt: &StateContext, +) -> Result<(), Error> { + // Populate the effective balance cache with the current effective balance. This will be + // overriden when `process_single_effective_balance_update` is called. + let is_active_next_epoch = validator.is_active_at(state_ctxt.next_epoch); + let temporary_effective_balance = validator.effective_balance; + next_epoch_cache.update_effective_balance( + validator_index, + temporary_effective_balance, + is_active_next_epoch, + )?; + Ok(()) +} + /// This function abstracts over phase0 and Electra effective balance processing. #[allow(clippy::too_many_arguments)] fn process_single_effective_balance_update( From 7d792e615cfaf8afb9eb342a1b6260dfd513b16e Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 27 Mar 2025 13:25:50 +1100 Subject: [PATCH 03/17] Fix xdelta3 output buffer issue (#7174) * Fix xdelta3 output buffer issue * Fix buckets * Update commit hash to `main` * Tag TODO(hdiff) * Update cargo lock --- Cargo.lock | 2 +- Cargo.toml | 2 +- beacon_node/store/src/hdiff.rs | 43 ++++++++++++++++++++++++++------ beacon_node/store/src/metrics.rs | 7 ++++++ 4 files changed, 45 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ac4248319f..b86ee106e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10760,7 +10760,7 @@ dependencies = [ [[package]] name = "xdelta3" version = "0.1.5" -source = "git+http://github.com/sigp/xdelta3-rs?rev=50d63cdf1878e5cf3538e9aae5eed34a22c64e4a#50d63cdf1878e5cf3538e9aae5eed34a22c64e4a" +source = "git+http://github.com/sigp/xdelta3-rs?rev=4db64086bb02e9febb584ba93b9d16bb2ae3825a#4db64086bb02e9febb584ba93b9d16bb2ae3825a" dependencies = [ "bindgen", "cc", diff --git a/Cargo.toml b/Cargo.toml index 3df158e5a5..49ea6a1108 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -289,7 +289,7 @@ validator_metrics = { path = "validator_client/validator_metrics" } validator_store = { path = "validator_client/validator_store" } validator_test_rig = { path = "testing/validator_test_rig" } warp_utils = { path = "common/warp_utils" } -xdelta3 = { git = "http://github.com/sigp/xdelta3-rs", rev = "50d63cdf1878e5cf3538e9aae5eed34a22c64e4a" } +xdelta3 = { git = "http://github.com/sigp/xdelta3-rs", rev = "4db64086bb02e9febb584ba93b9d16bb2ae3825a" } zstd = "0.13" [profile.maxperf] diff --git a/beacon_node/store/src/hdiff.rs b/beacon_node/store/src/hdiff.rs index a29e680eb5..a659c65452 100644 --- a/beacon_node/store/src/hdiff.rs +++ b/beacon_node/store/src/hdiff.rs @@ -21,8 +21,8 @@ static EMPTY_PUBKEY: LazyLock = LazyLock::new(PublicKeyBytes::em pub enum Error { InvalidHierarchy, DiffDeletionsNotSupported, - UnableToComputeDiff, - UnableToApplyDiff, + UnableToComputeDiff(xdelta3::Error), + UnableToApplyDiff(xdelta3::Error), BalancesIncompleteChunk, Compression(std::io::Error), InvalidSszState(ssz::DecodeError), @@ -323,9 +323,15 @@ impl BytesDiff { } pub fn compute_xdelta(source_bytes: &[u8], target_bytes: &[u8]) -> Result { - let bytes = xdelta3::encode(target_bytes, source_bytes) - .ok_or(Error::UnableToComputeDiff) - .unwrap(); + // TODO(hdiff): Use a smaller estimate for the output diff buffer size, currently the + // xdelta3 lib will use 2x the size of the source plus the target length, which is 4x the + // size of the hdiff buffer. In practice, diffs are almost always smaller than buffers (by a + // signficiant factor), so this is 4-16x larger than necessary in a temporary allocation. + // + // We should use an estimated size that *should* be enough, and then dynamically increase it + // if we hit an insufficient space error. + let bytes = + xdelta3::encode(target_bytes, source_bytes).map_err(Error::UnableToComputeDiff)?; Ok(Self { bytes }) } @@ -334,8 +340,31 @@ impl BytesDiff { } pub fn apply_xdelta(&self, source: &[u8], target: &mut Vec) -> Result<(), Error> { - *target = xdelta3::decode(&self.bytes, source).ok_or(Error::UnableToApplyDiff)?; - Ok(()) + // TODO(hdiff): Dynamic buffer allocation. This is a stopgap until we implement a schema + // change to store the output buffer size inside the `BytesDiff`. + let mut output_length = ((source.len() + self.bytes.len()) * 3) / 2; + let mut num_resizes = 0; + loop { + match xdelta3::decode_with_output_len(&self.bytes, source, output_length as u32) { + Ok(result_buffer) => { + *target = result_buffer; + + metrics::observe( + &metrics::BEACON_HDIFF_BUFFER_APPLY_RESIZES, + num_resizes as f64, + ); + return Ok(()); + } + Err(xdelta3::Error::InsufficientOutputLength) => { + // Double the output buffer length and try again. + output_length *= 2; + num_resizes += 1; + } + Err(err) => { + return Err(Error::UnableToApplyDiff(err)); + } + } + } } /// Byte size of this instance diff --git a/beacon_node/store/src/metrics.rs b/beacon_node/store/src/metrics.rs index 6f9f667917..5da73c3cad 100644 --- a/beacon_node/store/src/metrics.rs +++ b/beacon_node/store/src/metrics.rs @@ -202,6 +202,13 @@ pub static BEACON_HDIFF_BUFFER_CLONE_TIMES: LazyLock> = LazyLo "Time required to clone hierarchical diff buffer bytes", ) }); +pub static BEACON_HDIFF_BUFFER_APPLY_RESIZES: LazyLock> = LazyLock::new(|| { + try_create_histogram_with_buckets( + "store_hdiff_buffer_apply_resizes", + "Number of times during diff application that the output buffer had to be resized before decoding succeeded", + Ok(vec![0.0, 1.0, 2.0, 3.0, 4.0, 5.0]) + ) +}); /* * Beacon Block */ From 6d5a2be7f99e877dafc46c7ad5d8b89c37780656 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 27 Mar 2025 14:42:34 +1100 Subject: [PATCH 04/17] Release v7.0.0-beta.5 (#7210) New release for Pectra-enabled networks. --- Cargo.lock | 8 ++++---- beacon_node/Cargo.toml | 2 +- boot_node/Cargo.toml | 2 +- common/lighthouse_version/src/lib.rs | 6 +++--- lcli/Cargo.toml | 2 +- lighthouse/Cargo.toml | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b86ee106e4..f1a284ab65 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -860,7 +860,7 @@ dependencies = [ [[package]] name = "beacon_node" -version = "7.0.0-beta.4" +version = "7.0.0-beta.5" dependencies = [ "account_utils", "beacon_chain", @@ -1108,7 +1108,7 @@ dependencies = [ [[package]] name = "boot_node" -version = "7.0.0-beta.4" +version = "7.0.0-beta.5" dependencies = [ "beacon_node", "bytes", @@ -4811,7 +4811,7 @@ checksum = "830d08ce1d1d941e6b30645f1a0eb5643013d835ce3779a5fc208261dbe10f55" [[package]] name = "lcli" -version = "7.0.0-beta.4" +version = "7.0.0-beta.5" dependencies = [ "account_utils", "beacon_chain", @@ -5366,7 +5366,7 @@ dependencies = [ [[package]] name = "lighthouse" -version = "7.0.0-beta.4" +version = "7.0.0-beta.5" dependencies = [ "account_manager", "account_utils", diff --git a/beacon_node/Cargo.toml b/beacon_node/Cargo.toml index 06ef24e90c..d6c61341a3 100644 --- a/beacon_node/Cargo.toml +++ b/beacon_node/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "beacon_node" -version = "7.0.0-beta.4" +version = "7.0.0-beta.5" authors = [ "Paul Hauner ", "Age Manning "] edition = { workspace = true } diff --git a/common/lighthouse_version/src/lib.rs b/common/lighthouse_version/src/lib.rs index 1c62cd7b8a..bd5e31e3ab 100644 --- a/common/lighthouse_version/src/lib.rs +++ b/common/lighthouse_version/src/lib.rs @@ -17,8 +17,8 @@ pub const VERSION: &str = git_version!( // NOTE: using --match instead of --exclude for compatibility with old Git "--match=thiswillnevermatchlol" ], - prefix = "Lighthouse/v7.0.0-beta.4-", - fallback = "Lighthouse/v7.0.0-beta.4" + prefix = "Lighthouse/v7.0.0-beta.5-", + fallback = "Lighthouse/v7.0.0-beta.5" ); /// Returns the first eight characters of the latest commit hash for this build. @@ -54,7 +54,7 @@ pub fn version_with_platform() -> String { /// /// `1.5.1` pub fn version() -> &'static str { - "7.0.0-beta.4" + "7.0.0-beta.5" } /// Returns the name of the current client running. diff --git a/lcli/Cargo.toml b/lcli/Cargo.toml index c7d3ee8fb8..0b5355d249 100644 --- a/lcli/Cargo.toml +++ b/lcli/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "lcli" description = "Lighthouse CLI (modeled after zcli)" -version = "7.0.0-beta.4" +version = "7.0.0-beta.5" authors = ["Paul Hauner "] edition = { workspace = true } diff --git a/lighthouse/Cargo.toml b/lighthouse/Cargo.toml index 8c08666ec1..e8c5874a91 100644 --- a/lighthouse/Cargo.toml +++ b/lighthouse/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "lighthouse" -version = "7.0.0-beta.4" +version = "7.0.0-beta.5" authors = ["Sigma Prime "] edition = { workspace = true } autotests = false From a5ea05ce2a0db7c4a30a63d530d28848bbbaa276 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Fri, 28 Mar 2025 02:29:19 -0600 Subject: [PATCH 05/17] Top-up pubkey cache on startup (#7217) This is a workaround for #7216 In the case of gaps between the in-memory pub key cache and its on-disk representation, use the head state on startup to "top-up" the cache/db w/ any missing validators --- beacon_node/beacon_chain/src/builder.rs | 29 +++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 02b566971a..a05b5e4ea5 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -847,10 +847,31 @@ where )); } - let validator_pubkey_cache = self.validator_pubkey_cache.map(Ok).unwrap_or_else(|| { - ValidatorPubkeyCache::new(&head_snapshot.beacon_state, store.clone()) - .map_err(|e| format!("Unable to init validator pubkey cache: {:?}", e)) - })?; + let validator_pubkey_cache = self + .validator_pubkey_cache + .map(|mut validator_pubkey_cache| { + // If any validators weren't persisted to disk on previous runs, this will use the head state to + // "top-up" the in-memory validator cache and its on-disk representation with any missing validators. + let pubkey_store_ops = validator_pubkey_cache + .import_new_pubkeys(&head_snapshot.beacon_state) + .map_err(|e| format!("Unable to top-up persisted pubkey cache {:?}", e))?; + if !pubkey_store_ops.is_empty() { + // Write any missed validators to disk + debug!( + store.log, + "Topping up validator pubkey cache"; + "missing_validators" => pubkey_store_ops.len() + ); + store + .do_atomically_with_block_and_blobs_cache(pubkey_store_ops) + .map_err(|e| format!("Unable to write pubkeys to disk {:?}", e))?; + } + Ok(validator_pubkey_cache) + }) + .unwrap_or_else(|| { + ValidatorPubkeyCache::new(&head_snapshot.beacon_state, store.clone()) + .map_err(|e| format!("Unable to init validator pubkey cache: {:?}", e)) + })?; let migrator_config = self.store_migrator_config.unwrap_or_default(); let store_migrator = BackgroundMigrator::new( From 54aef2d0436edb0ace56837232f24f9dcfb6d0d1 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 28 Mar 2025 05:59:09 -0700 Subject: [PATCH 06/17] Admin add/remove peer (#7198) N/A Adds endpoints to add and remove trusted peers from the http api. The added peers are trusted peers so they won't be disconnected for bad scores. We try to maintain a connection to the peer in case they disconnect from us by trying to dial it every heartbeat. --- beacon_node/http_api/src/lib.rs | 78 ++++++++++++++++++- beacon_node/http_api/tests/tests.rs | 23 ++++++ .../src/peer_manager/mod.rs | 20 ++++- .../src/peer_manager/peerdb.rs | 29 ++++++- .../src/peer_manager/peerdb/peer_info.rs | 4 +- .../lighthouse_network/src/service/mod.rs | 15 ++++ .../lighthouse_network/src/types/globals.rs | 12 +++ beacon_node/network/src/service.rs | 11 +++ common/eth2/src/lighthouse.rs | 27 ++++++- common/eth2/src/types.rs | 5 ++ 10 files changed, 217 insertions(+), 7 deletions(-) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index d76f24709a..a33508dde9 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -53,7 +53,7 @@ use eth2::types::{ use eth2::{CONSENSUS_VERSION_HEADER, CONTENT_TYPE_HEADER, SSZ_CONTENT_TYPE_HEADER}; use health_metrics::observe::Observe; use lighthouse_network::rpc::methods::MetaData; -use lighthouse_network::{types::SyncState, EnrExt, NetworkGlobals, PeerId, PubsubMessage}; +use lighthouse_network::{types::SyncState, Enr, EnrExt, NetworkGlobals, PeerId, PubsubMessage}; use lighthouse_version::version_with_platform; use logging::SSELoggingComponents; use network::{NetworkMessage, NetworkSenders, ValidatorSubscriptionMessage}; @@ -72,6 +72,7 @@ use std::future::Future; use std::net::{IpAddr, Ipv4Addr, SocketAddr}; use std::path::PathBuf; use std::pin::Pin; +use std::str::FromStr; use std::sync::Arc; use sysinfo::{System, SystemExt}; use system_health::{observe_nat, observe_system_health_bn}; @@ -3676,7 +3677,7 @@ pub fn serve( .and(task_spawner_filter.clone()) .and(chain_filter.clone()) .and(warp_utils::json::json()) - .and(network_tx_filter) + .and(network_tx_filter.clone()) .and(log_filter.clone()) .then( |not_synced_filter: Result<(), Rejection>, @@ -4122,6 +4123,77 @@ pub fn serve( }, ); + // POST lighthouse/add_peer + let post_lighthouse_add_peer = warp::path("lighthouse") + .and(warp::path("add_peer")) + .and(warp::path::end()) + .and(warp_utils::json::json()) + .and(task_spawner_filter.clone()) + .and(network_globals.clone()) + .and(network_tx_filter.clone()) + .and(log_filter.clone()) + .then( + |request_data: api_types::AdminPeer, + task_spawner: TaskSpawner, + network_globals: Arc>, + network_tx: UnboundedSender>, + log: Logger| { + task_spawner.blocking_json_task(Priority::P0, move || { + let enr = Enr::from_str(&request_data.enr).map_err(|e| { + warp_utils::reject::custom_bad_request(format!("invalid enr error {}", e)) + })?; + info!( + log, + "Adding trusted peer"; + "peer_id" => %enr.peer_id(), + "multiaddr" => ?enr.multiaddr() + ); + network_globals.add_trusted_peer(enr.clone()); + + publish_network_message(&network_tx, NetworkMessage::ConnectTrustedPeer(enr))?; + + Ok(()) + }) + }, + ); + + // POST lighthouse/remove_peer + let post_lighthouse_remove_peer = warp::path("lighthouse") + .and(warp::path("remove_peer")) + .and(warp::path::end()) + .and(warp_utils::json::json()) + .and(task_spawner_filter.clone()) + .and(network_globals.clone()) + .and(network_tx_filter.clone()) + .and(log_filter.clone()) + .then( + |request_data: api_types::AdminPeer, + task_spawner: TaskSpawner, + network_globals: Arc>, + network_tx: UnboundedSender>, + log: Logger| { + task_spawner.blocking_json_task(Priority::P0, move || { + let enr = Enr::from_str(&request_data.enr).map_err(|e| { + warp_utils::reject::custom_bad_request(format!("invalid enr error {}", e)) + })?; + info!( + log, + "Removing trusted peer"; + "peer_id" => %enr.peer_id(), + "multiaddr" => ?enr.multiaddr() + ); + network_globals.remove_trusted_peer(enr.clone()); + + publish_network_message( + &network_tx, + NetworkMessage::DisconnectTrustedPeer(enr), + )?; + + Ok(()) + }) + }, + ); + // POST lighthouse/liveness let post_lighthouse_liveness = warp::path("lighthouse") .and(warp::path("liveness")) @@ -4896,6 +4968,8 @@ pub fn serve( .uor(post_lighthouse_ui_validator_info) .uor(post_lighthouse_finalize) .uor(post_lighthouse_compaction) + .uor(post_lighthouse_add_peer) + .uor(post_lighthouse_remove_peer) .recover(warp_utils::reject::handle_rejection), ), ) diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index a1241f4929..6ddd49bfd9 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -5768,6 +5768,27 @@ impl ApiTester { self } + pub async fn test_post_lighthouse_add_remove_peer(self) -> Self { + let trusted_peers = self.ctx.network_globals.as_ref().unwrap().trusted_peers(); + // Check that there aren't any trusted peers on startup + assert!(trusted_peers.is_empty()); + let enr = AdminPeer {enr: "enr:-QESuEDpVVjo8dmDuneRhLnXdIGY3e9NQiaG4sJR3GS-VMQCQDsmBYoQhJRaPeZzPlTsZj2F8v-iV4lKJEYIRIyztqexHodhdHRuZXRziAwAAAAAAAAAhmNsaWVudNiKTGlnaHRob3VzZYw3LjAuMC1iZXRhLjSEZXRoMpDS8Zl_YAAJEAAIAAAAAAAAgmlkgnY0gmlwhIe11XmDaXA2kCoBBPkAOitZAAAAAAAAAAKEcXVpY4IjKYVxdWljNoIjg4lzZWNwMjU2azGhA43ihEr9BUVVnIHIfFqBR3Izs4YRHHPsTqIbUgEb3Hc8iHN5bmNuZXRzD4N0Y3CCIyiEdGNwNoIjgoN1ZHCCIyiEdWRwNoIjgg".to_string()}; + self.client + .post_lighthouse_add_peer(enr.clone()) + .await + .unwrap(); + let trusted_peers = self.ctx.network_globals.as_ref().unwrap().trusted_peers(); + // Should have 1 trusted peer + assert_eq!(trusted_peers.len(), 1); + + self.client.post_lighthouse_remove_peer(enr).await.unwrap(); + let trusted_peers = self.ctx.network_globals.as_ref().unwrap().trusted_peers(); + // Should be empty after removing + assert!(trusted_peers.is_empty()); + + self + } + pub async fn test_post_lighthouse_liveness(self) -> Self { let epoch = self.chain.epoch().unwrap(); let head_state = self.chain.head_beacon_state_cloned(); @@ -7334,6 +7355,8 @@ async fn lighthouse_endpoints() { .test_post_lighthouse_database_reconstruct() .await .test_post_lighthouse_liveness() + .await + .test_post_lighthouse_add_remove_peer() .await; } diff --git a/beacon_node/lighthouse_network/src/peer_manager/mod.rs b/beacon_node/lighthouse_network/src/peer_manager/mod.rs index 07c4be7959..6067d52042 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/mod.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/mod.rs @@ -114,6 +114,7 @@ pub struct PeerManager { metrics_enabled: bool, /// Keeps track of whether the QUIC protocol is enabled or not. quic_enabled: bool, + trusted_peers: HashSet, /// The logger associated with the `PeerManager`. log: slog::Logger, } @@ -195,6 +196,7 @@ impl PeerManager { discovery_enabled, metrics_enabled, quic_enabled, + trusted_peers: Default::default(), log: log.clone(), }) } @@ -894,7 +896,7 @@ impl PeerManager { } // Gracefully disconnects a peer without banning them. - fn disconnect_peer(&mut self, peer_id: PeerId, reason: GoodbyeReason) { + pub fn disconnect_peer(&mut self, peer_id: PeerId, reason: GoodbyeReason) { self.events .push(PeerManagerEvent::DisconnectPeer(peer_id, reason)); self.network_globals @@ -943,6 +945,13 @@ impl PeerManager { } } + fn maintain_trusted_peers(&mut self) { + let trusted_peers = self.trusted_peers.clone(); + for trusted_peer in trusted_peers { + self.dial_peer(trusted_peer); + } + } + /// This function checks the status of our current peers and optionally requests a discovery /// query if we need to find more peers to maintain the current number of peers fn maintain_peer_count(&mut self, dialing_peers: usize) { @@ -1234,6 +1243,7 @@ impl PeerManager { fn heartbeat(&mut self) { // Optionally run a discovery query if we need more peers. self.maintain_peer_count(0); + self.maintain_trusted_peers(); // Cleans up the connection state of dialing peers. // Libp2p dials peer-ids, but sometimes the response is from another peer-id or libp2p @@ -1470,6 +1480,14 @@ impl PeerManager { ) }) } + + pub fn add_trusted_peer(&mut self, enr: Enr) { + self.trusted_peers.insert(enr); + } + + pub fn remove_trusted_peer(&mut self, enr: Enr) { + self.trusted_peers.remove(&enr); + } } enum ConnectingType { diff --git a/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs b/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs index 37cb5df6ea..b692639911 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs @@ -9,7 +9,7 @@ use std::net::IpAddr; use std::time::Instant; use std::{cmp::Ordering, fmt::Display}; use std::{ - collections::{HashMap, HashSet}, + collections::{hash_map::Entry, HashMap, HashSet}, fmt::Formatter, }; use sync_status::SyncStatus; @@ -79,6 +79,33 @@ impl PeerDB { self.peers.iter() } + pub fn set_trusted_peer(&mut self, enr: Enr) { + match self.peers.entry(enr.peer_id()) { + Entry::Occupied(mut info) => { + let entry = info.get_mut(); + entry.score = Score::max_score(); + entry.is_trusted = true; + } + Entry::Vacant(entry) => { + entry.insert(PeerInfo::trusted_peer_info()); + } + } + } + + pub fn unset_trusted_peer(&mut self, enr: Enr) { + if let Some(info) = self.peers.get_mut(&enr.peer_id()) { + info.is_trusted = false; + info.score = Score::default(); + } + } + + pub fn trusted_peers(&self) -> Vec { + self.peers + .iter() + .filter_map(|(id, info)| if info.is_trusted { Some(*id) } else { None }) + .collect() + } + /// Gives the ids of all known peers. pub fn peer_ids(&self) -> impl Iterator { self.peers.keys() diff --git a/beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs b/beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs index 2e8f462565..05a29ac47e 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs @@ -21,7 +21,7 @@ use PeerConnectionStatus::*; #[serde(bound = "E: EthSpec")] pub struct PeerInfo { /// The peers reputation - score: Score, + pub(crate) score: Score, /// Client managing this peer client: Client, /// Connection status of this peer @@ -50,7 +50,7 @@ pub struct PeerInfo { #[serde(skip)] min_ttl: Option, /// Is the peer a trusted peer. - is_trusted: bool, + pub(crate) is_trusted: bool, /// Direction of the first connection of the last (or current) connected session with this peer. /// None if this peer was never connected. connection_direction: Option, diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs index 8586fd9cd3..06d806ce0b 100644 --- a/beacon_node/lighthouse_network/src/service/mod.rs +++ b/beacon_node/lighthouse_network/src/service/mod.rs @@ -1236,6 +1236,21 @@ impl Network { } } + /// Adds the given `enr` to the trusted peers mapping and tries to dial it + /// every heartbeat to maintain the connection. + pub fn dial_trusted_peer(&mut self, enr: Enr) { + self.peer_manager_mut().add_trusted_peer(enr.clone()); + self.peer_manager_mut().dial_peer(enr); + } + + /// Remove the given peer from the trusted peers mapping if it exists and disconnect + /// from it. + pub fn remove_trusted_peer(&mut self, enr: Enr) { + self.peer_manager_mut().remove_trusted_peer(enr.clone()); + self.peer_manager_mut() + .disconnect_peer(enr.peer_id(), GoodbyeReason::TooManyPeers); + } + /* Sub-behaviour event handling functions */ /// Handle a gossipsub event. diff --git a/beacon_node/lighthouse_network/src/types/globals.rs b/beacon_node/lighthouse_network/src/types/globals.rs index 2800b75133..b63754fd4e 100644 --- a/beacon_node/lighthouse_network/src/types/globals.rs +++ b/beacon_node/lighthouse_network/src/types/globals.rs @@ -162,6 +162,18 @@ impl NetworkGlobals { .unwrap_or_default() } + pub fn add_trusted_peer(&self, enr: Enr) { + self.peers.write().set_trusted_peer(enr); + } + + pub fn remove_trusted_peer(&self, enr: Enr) { + self.peers.write().unset_trusted_peer(enr); + } + + pub fn trusted_peers(&self) -> Vec { + self.peers.read().trusted_peers() + } + /// Updates the syncing state of the node. /// /// The old state is returned diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index 1b2a681c64..42889169a2 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -14,6 +14,7 @@ use futures::StreamExt; use lighthouse_network::rpc::{RequestId, RequestType}; use lighthouse_network::service::Network; use lighthouse_network::types::GossipKind; +use lighthouse_network::Enr; use lighthouse_network::{prometheus_client::registry::Registry, MessageAcceptance}; use lighthouse_network::{ rpc::{GoodbyeReason, RpcErrorResponse}, @@ -101,6 +102,10 @@ pub enum NetworkMessage { reason: GoodbyeReason, source: ReportSource, }, + /// Connect to a trusted peer and try to maintain the connection. + ConnectTrustedPeer(Enr), + /// Disconnect from a trusted peer and remove it from the `trusted_peers` mapping. + DisconnectTrustedPeer(Enr), } /// Messages triggered by validators that may trigger a subscription to a subnet. @@ -688,6 +693,12 @@ impl NetworkService { reason, source, } => self.libp2p.goodbye_peer(&peer_id, reason, source), + NetworkMessage::ConnectTrustedPeer(enr) => { + self.libp2p.dial_trusted_peer(enr); + } + NetworkMessage::DisconnectTrustedPeer(enr) => { + self.libp2p.remove_trusted_peer(enr); + } NetworkMessage::SubscribeCoreTopics => { if self.subscribed_core_topics() { return; diff --git a/common/eth2/src/lighthouse.rs b/common/eth2/src/lighthouse.rs index badc4857c4..2f61c52476 100644 --- a/common/eth2/src/lighthouse.rs +++ b/common/eth2/src/lighthouse.rs @@ -9,7 +9,8 @@ mod sync_committee_rewards; use crate::{ types::{ - DepositTreeSnapshot, Epoch, EthSpec, FinalizedExecutionBlock, GenericResponse, ValidatorId, + AdminPeer, DepositTreeSnapshot, Epoch, EthSpec, FinalizedExecutionBlock, GenericResponse, + ValidatorId, }, BeaconNodeHttpClient, DepositData, Error, Eth1Data, Hash256, Slot, }; @@ -406,6 +407,30 @@ impl BeaconNodeHttpClient { self.post_with_response(path, &()).await } + /// `POST lighthouse/add_peer` + pub async fn post_lighthouse_add_peer(&self, req: AdminPeer) -> Result<(), Error> { + let mut path = self.server.full.clone(); + + path.path_segments_mut() + .map_err(|()| Error::InvalidUrl(self.server.clone()))? + .push("lighthouse") + .push("add_peer"); + + self.post_with_response(path, &req).await + } + + /// `POST lighthouse/remove_peer` + pub async fn post_lighthouse_remove_peer(&self, req: AdminPeer) -> Result<(), Error> { + let mut path = self.server.full.clone(); + + path.path_segments_mut() + .map_err(|()| Error::InvalidUrl(self.server.clone()))? + .push("lighthouse") + .push("remove_peer"); + + self.post_with_response(path, &req).await + } + /* Analysis endpoints. */ diff --git a/common/eth2/src/types.rs b/common/eth2/src/types.rs index 7d70d242be..dd4f5437ae 100644 --- a/common/eth2/src/types.rs +++ b/common/eth2/src/types.rs @@ -1431,6 +1431,11 @@ pub struct ManualFinalizationRequestData { pub block_root: Hash256, } +#[derive(Debug, Serialize, Deserialize, Clone)] +pub struct AdminPeer { + pub enr: String, +} + #[derive(Debug, Serialize, Deserialize)] pub struct LivenessRequestData { pub epoch: Epoch, From 9bc0d5161edb00140addc1d06271baf01ede4c54 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 1 Apr 2025 18:56:06 +1100 Subject: [PATCH 07/17] Disable LevelDB snappy feature (#7235) Disable the `snappy` feature of LevelDB to prevent compilation issues with CMake 4.0, e.g. https://github.com/sigp/lighthouse/actions/runs/14182783816/job/39732457274?pr=7231 We do not use Snappy compression in LevelDB, and do not need to compile this. This might also shave a few seconds off compilation! --- beacon_node/store/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/store/Cargo.toml b/beacon_node/store/Cargo.toml index d2f3a5c562..57330eb583 100644 --- a/beacon_node/store/Cargo.toml +++ b/beacon_node/store/Cargo.toml @@ -22,7 +22,7 @@ directory = { workspace = true } ethereum_ssz = { workspace = true } ethereum_ssz_derive = { workspace = true } itertools = { workspace = true } -leveldb = { version = "0.8.6", optional = true } +leveldb = { version = "0.8.6", optional = true, default-features = false } logging = { workspace = true } lru = { workspace = true } metrics = { workspace = true } From fb7ec0d151d4a43937b344f805f483e795df8711 Mon Sep 17 00:00:00 2001 From: antondlr Date: Tue, 1 Apr 2025 10:30:39 +0200 Subject: [PATCH 08/17] Change `genesis-state-url-timeout` (#7112) Timeouts sometimes occur on downloading the Holeksy genesis state from AWS, we've had reputable outside reports on this. It's around 200MB and hosted in APAC, it makes sense to bump the default, at least for Holesky. Bump default timeout from 180 to 300 secs --- book/src/help_bn.md | 2 +- book/src/help_general.md | 2 +- book/src/help_vc.md | 2 +- book/src/help_vm.md | 2 +- book/src/help_vm_create.md | 2 +- book/src/help_vm_import.md | 2 +- book/src/help_vm_move.md | 2 +- lighthouse/src/main.rs | 2 +- lighthouse/tests/beacon_node.rs | 2 +- 9 files changed, 9 insertions(+), 9 deletions(-) diff --git a/book/src/help_bn.md b/book/src/help_bn.md index 224db099e7..35ad020b74 100644 --- a/book/src/help_bn.md +++ b/book/src/help_bn.md @@ -166,7 +166,7 @@ Options: then this value will be ignored. --genesis-state-url-timeout The timeout in seconds for the request to --genesis-state-url. - [default: 180] + [default: 300] --graffiti Specify your custom graffiti to be included in blocks. Defaults to the current version and commit, truncated to fit in 32 bytes. diff --git a/book/src/help_general.md b/book/src/help_general.md index 4de3270cfc..fbc3ca2557 100644 --- a/book/src/help_general.md +++ b/book/src/help_general.md @@ -51,7 +51,7 @@ Options: then this value will be ignored. --genesis-state-url-timeout The timeout in seconds for the request to --genesis-state-url. - [default: 180] + [default: 300] --log-format Specifies the log format used when emitting logs to the terminal. [possible values: JSON] diff --git a/book/src/help_vc.md b/book/src/help_vc.md index e01828aea2..c32104b17a 100644 --- a/book/src/help_vc.md +++ b/book/src/help_vc.md @@ -49,7 +49,7 @@ Options: then this value will be ignored. --genesis-state-url-timeout The timeout in seconds for the request to --genesis-state-url. - [default: 180] + [default: 300] --graffiti Specify your custom graffiti to be included in blocks. --graffiti-file diff --git a/book/src/help_vm.md b/book/src/help_vm.md index fdb035aa05..85e1a1168f 100644 --- a/book/src/help_vm.md +++ b/book/src/help_vm.md @@ -48,7 +48,7 @@ Options: then this value will be ignored. --genesis-state-url-timeout The timeout in seconds for the request to --genesis-state-url. - [default: 180] + [default: 300] --log-format Specifies the log format used when emitting logs to the terminal. [possible values: JSON] diff --git a/book/src/help_vm_create.md b/book/src/help_vm_create.md index c6bfe2e95a..3b88206397 100644 --- a/book/src/help_vm_create.md +++ b/book/src/help_vm_create.md @@ -55,7 +55,7 @@ Options: then this value will be ignored. --genesis-state-url-timeout The timeout in seconds for the request to --genesis-state-url. - [default: 180] + [default: 300] --log-format Specifies the log format used when emitting logs to the terminal. [possible values: JSON] diff --git a/book/src/help_vm_import.md b/book/src/help_vm_import.md index 8da3eee9b0..63cca91ee5 100644 --- a/book/src/help_vm_import.md +++ b/book/src/help_vm_import.md @@ -36,7 +36,7 @@ Options: then this value will be ignored. --genesis-state-url-timeout The timeout in seconds for the request to --genesis-state-url. - [default: 180] + [default: 300] --keystore-file The path to a keystore JSON file to be imported to the validator client. This file is usually created using staking-deposit-cli or diff --git a/book/src/help_vm_move.md b/book/src/help_vm_move.md index 83824ce768..b7320ca290 100644 --- a/book/src/help_vm_move.md +++ b/book/src/help_vm_move.md @@ -44,7 +44,7 @@ Options: then this value will be ignored. --genesis-state-url-timeout The timeout in seconds for the request to --genesis-state-url. - [default: 180] + [default: 300] --log-format Specifies the log format used when emitting logs to the terminal. [possible values: JSON] diff --git a/lighthouse/src/main.rs b/lighthouse/src/main.rs index 2b7387e076..a6ab1cfb6b 100644 --- a/lighthouse/src/main.rs +++ b/lighthouse/src/main.rs @@ -411,7 +411,7 @@ fn main() { "The timeout in seconds for the request to --genesis-state-url.", ) .action(ArgAction::Set) - .default_value("180") + .default_value("300") .global(true) .display_order(0) ) diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 1fb9e40c23..aad11c50d7 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -2758,7 +2758,7 @@ fn genesis_state_url_default() { .run_with_zero_port() .with_config(|config| { assert_eq!(config.genesis_state_url, None); - assert_eq!(config.genesis_state_url_timeout, Duration::from_secs(180)); + assert_eq!(config.genesis_state_url_timeout, Duration::from_secs(300)); }); } From 4839ed620fa952959a93da6eb54f06c2c0805136 Mon Sep 17 00:00:00 2001 From: Mac L Date: Tue, 1 Apr 2025 21:51:09 +1100 Subject: [PATCH 09/17] Tracing cleanup (#7168) #7153 #7146 #7147 #7148 -> Thanks to @ackintosh This PR does the following: 1. Disable logging to file when using either `--logfile-max-number 0` or `--logfile-max-size 0`. Note that disabling the log file in this way will also disable `discv5` and `libp2p` logging. 1. `discv5` and `libp2p` logging will be disabled by default unless running `beacon_node` or `boot_node`. This also should fix the VC panic we were seeing. 1. Removes log rotation and compression from `libp2p` and `discv5` logs. It is now limited to 1 file and will rotate based on the value of the `--logfile-max-size` flag. We could potentially add flags specifically to control the size/number of these, however I felt a single log file was sufficient. Perhaps @AgeManning has opinions about this? 1. Removes all dependency logging and references to `dep_log`. 1. Introduces workspace filtering to file and stdout. This explicitly allows logs from members of the Lighthouse workspace, disallowing all others. It uses a proc macro which pulls the member list from cargo metadata at compile time. This might be over-engineered but my hope is that this list will not require maintenance. 1. Unifies file and stdout JSON format. With slog, the formats were slightly different. @threehrsleep worked to maintain that format difference, to ensure there was no breaking changes. If these format differences are actually problematic we can restore it, however I felt the added complexity wasn't worth it. 1. General code improvements and cleanup. --- Cargo.lock | 29 +++- Cargo.toml | 3 + common/logging/Cargo.toml | 3 +- common/logging/src/lib.rs | 142 +----------------- .../tracing_libp2p_discv5_logging_layer.rs | 113 ++++++++++++++ common/logging/src/tracing_logging_layer.rs | 116 +++----------- common/logging/src/utils.rs | 31 ++++ common/workspace_members/Cargo.toml | 11 ++ common/workspace_members/src/lib.rs | 39 +++++ lighthouse/environment/src/lib.rs | 86 ++++------- lighthouse/environment/src/tracing_common.rs | 78 +++++----- lighthouse/src/main.rs | 70 ++++++--- testing/simulator/src/basic_sim.rs | 12 +- testing/simulator/src/fallback_sim.rs | 14 +- 14 files changed, 382 insertions(+), 365 deletions(-) create mode 100644 common/logging/src/tracing_libp2p_discv5_logging_layer.rs create mode 100644 common/logging/src/utils.rs create mode 100644 common/workspace_members/Cargo.toml create mode 100644 common/workspace_members/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index b3b4069e8c..e6fca4c052 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1187,6 +1187,20 @@ dependencies = [ "thiserror 1.0.69", ] +[[package]] +name = "cargo_metadata" +version = "0.19.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dd5eb614ed4c27c5d706420e4320fbe3216ab31fa1c33cd8246ac36dae4479ba" +dependencies = [ + "camino", + "cargo-platform", + "semver 1.0.26", + "serde", + "serde_json", + "thiserror 2.0.12", +] + [[package]] name = "cast" version = "0.3.0" @@ -1915,7 +1929,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "18e4fdb82bd54a12e42fb58a800dcae6b9e13982238ce2296dc3570b92148e1f" dependencies = [ "data-encoding", - "syn 1.0.109", + "syn 2.0.100", ] [[package]] @@ -2853,7 +2867,7 @@ checksum = "ade3e9c97727343984e1ceada4fdab11142d2ee3472d2c67027d56b1251d4f15" dependencies = [ "arrayvec", "bytes", - "cargo_metadata", + "cargo_metadata 0.15.4", "chrono", "elliptic-curve 0.12.3", "ethabi 18.0.0", @@ -4761,7 +4775,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fc2f4eb4bc735547cfed7c0a4922cbd04a4655978c09b54f1f7b228750664c34" dependencies = [ "cfg-if", - "windows-targets 0.48.5", + "windows-targets 0.52.6", ] [[package]] @@ -5458,6 +5472,7 @@ dependencies = [ "tracing-core", "tracing-log", "tracing-subscriber", + "workspace_members", ] [[package]] @@ -10152,6 +10167,14 @@ dependencies = [ "bitflags 2.9.0", ] +[[package]] +name = "workspace_members" +version = "0.1.0" +dependencies = [ + "cargo_metadata 0.19.2", + "quote", +] + [[package]] name = "write16" version = "1.0.0" diff --git a/Cargo.toml b/Cargo.toml index 5284713fc2..de5d6b541e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -51,6 +51,7 @@ members = [ "common/unused_port", "common/validator_dir", "common/warp_utils", + "common/workspace_members", "consensus/fixed_bytes", "consensus/fork_choice", @@ -120,6 +121,7 @@ bincode = "1" bitvec = "1" byteorder = "1" bytes = "1" +cargo_metadata = "0.19" clap = { version = "4.5.4", features = ["derive", "cargo", "wrap_help"] } # Turn off c-kzg's default features which include `blst/portable`. We can turn on blst's portable # feature ourselves when desired. @@ -246,6 +248,7 @@ kzg = { path = "crypto/kzg" } metrics = { path = "common/metrics" } lighthouse_network = { path = "beacon_node/lighthouse_network" } lighthouse_version = { path = "common/lighthouse_version" } +workspace_members = { path = "common/workspace_members" } lockfile = { path = "common/lockfile" } logging = { path = "common/logging" } lru_cache = { path = "common/lru_cache" } diff --git a/common/logging/Cargo.toml b/common/logging/Cargo.toml index a69bc6ab23..6975e04505 100644 --- a/common/logging/Cargo.toml +++ b/common/logging/Cargo.toml @@ -16,8 +16,9 @@ parking_lot = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } tokio = { workspace = true, features = [ "time" ] } -tracing = "0.1" +tracing = { workspace = true } tracing-appender = { workspace = true } tracing-core = { workspace = true } tracing-log = { workspace = true } tracing-subscriber = { workspace = true } +workspace_members = { workspace = true } diff --git a/common/logging/src/lib.rs b/common/logging/src/lib.rs index 403f682a06..5c4de1fd61 100644 --- a/common/logging/src/lib.rs +++ b/common/logging/src/lib.rs @@ -1,24 +1,24 @@ -use chrono::Local; -use logroller::{Compression, LogRollerBuilder, Rotation, RotationSize}; use metrics::{try_create_int_counter, IntCounter, Result as MetricsResult}; -use std::io::Write; -use std::path::PathBuf; use std::sync::LazyLock; use std::time::{Duration, Instant}; -use tracing::Subscriber; -use tracing_appender::non_blocking::{NonBlocking, WorkerGuard}; -use tracing_subscriber::layer::Context; -use tracing_subscriber::{EnvFilter, Layer}; +use tracing_subscriber::EnvFilter; pub const MAX_MESSAGE_WIDTH: usize = 40; pub mod macros; mod sse_logging_components; +mod tracing_libp2p_discv5_logging_layer; pub mod tracing_logging_layer; mod tracing_metrics_layer; +mod utils; pub use sse_logging_components::SSELoggingComponents; +pub use tracing_libp2p_discv5_logging_layer::{ + create_libp2p_discv5_tracing_layer, Libp2pDiscv5TracingLayer, +}; +pub use tracing_logging_layer::LoggingLayer; pub use tracing_metrics_layer::MetricsLayer; +pub use utils::build_workspace_filter; /// The minimum interval between log messages indicating that a queue is full. const LOG_DEBOUNCE_INTERVAL: Duration = Duration::from_secs(30); @@ -51,132 +51,6 @@ impl TimeLatch { } } -pub struct Libp2pDiscv5TracingLayer { - pub libp2p_non_blocking_writer: NonBlocking, - pub _libp2p_guard: WorkerGuard, - pub discv5_non_blocking_writer: NonBlocking, - pub _discv5_guard: WorkerGuard, -} - -impl Layer for Libp2pDiscv5TracingLayer -where - S: Subscriber, -{ - fn on_event(&self, event: &tracing::Event<'_>, _ctx: Context) { - let meta = event.metadata(); - let log_level = meta.level(); - let timestamp = Local::now().format("%Y-%m-%d %H:%M:%S").to_string(); - - let target = match meta.target().split_once("::") { - Some((crate_name, _)) => crate_name, - None => "unknown", - }; - - let mut writer = match target { - "gossipsub" => self.libp2p_non_blocking_writer.clone(), - "discv5" => self.discv5_non_blocking_writer.clone(), - _ => return, - }; - - let mut visitor = LogMessageExtractor { - message: String::default(), - }; - - event.record(&mut visitor); - let message = format!("{} {} {}\n", timestamp, log_level, visitor.message); - - if let Err(e) = writer.write_all(message.as_bytes()) { - eprintln!("Failed to write log: {}", e); - } - } -} - -struct LogMessageExtractor { - message: String, -} - -impl tracing_core::field::Visit for LogMessageExtractor { - fn record_debug(&mut self, _: &tracing_core::Field, value: &dyn std::fmt::Debug) { - self.message = format!("{} {:?}", self.message, value); - } -} - -pub fn create_libp2p_discv5_tracing_layer( - base_tracing_log_path: Option, - max_log_size: u64, - compression: bool, - max_log_number: usize, -) -> Libp2pDiscv5TracingLayer { - if let Some(mut tracing_log_path) = base_tracing_log_path { - // Ensure that `tracing_log_path` only contains directories. - for p in tracing_log_path.clone().iter() { - tracing_log_path = tracing_log_path.join(p); - if let Ok(metadata) = tracing_log_path.metadata() { - if !metadata.is_dir() { - tracing_log_path.pop(); - break; - } - } - } - - let mut libp2p_writer = - LogRollerBuilder::new(tracing_log_path.clone(), PathBuf::from("libp2p.log")) - .rotation(Rotation::SizeBased(RotationSize::MB(max_log_size))) - .max_keep_files(max_log_number.try_into().unwrap_or_else(|e| { - eprintln!("Failed to convert max_log_number to u64: {}", e); - 10 - })); - - let mut discv5_writer = - LogRollerBuilder::new(tracing_log_path.clone(), PathBuf::from("discv5.log")) - .rotation(Rotation::SizeBased(RotationSize::MB(max_log_size))) - .max_keep_files(max_log_number.try_into().unwrap_or_else(|e| { - eprintln!("Failed to convert max_log_number to u64: {}", e); - 10 - })); - - if compression { - libp2p_writer = libp2p_writer.compression(Compression::Gzip); - discv5_writer = discv5_writer.compression(Compression::Gzip); - } - - let libp2p_writer = match libp2p_writer.build() { - Ok(writer) => writer, - Err(e) => { - eprintln!("Failed to initialize libp2p rolling file appender: {e}"); - std::process::exit(1); - } - }; - - let discv5_writer = match discv5_writer.build() { - Ok(writer) => writer, - Err(e) => { - eprintln!("Failed to initialize discv5 rolling file appender: {e}"); - std::process::exit(1); - } - }; - - let (libp2p_non_blocking_writer, _libp2p_guard) = NonBlocking::new(libp2p_writer); - let (discv5_non_blocking_writer, _discv5_guard) = NonBlocking::new(discv5_writer); - - Libp2pDiscv5TracingLayer { - libp2p_non_blocking_writer, - _libp2p_guard, - discv5_non_blocking_writer, - _discv5_guard, - } - } else { - let (libp2p_non_blocking_writer, _libp2p_guard) = NonBlocking::new(std::io::sink()); - let (discv5_non_blocking_writer, _discv5_guard) = NonBlocking::new(std::io::sink()); - Libp2pDiscv5TracingLayer { - libp2p_non_blocking_writer, - _libp2p_guard, - discv5_non_blocking_writer, - _discv5_guard, - } - } -} - /// Return a tracing subscriber suitable for test usage. /// /// By default no logs will be printed, but they can be enabled via diff --git a/common/logging/src/tracing_libp2p_discv5_logging_layer.rs b/common/logging/src/tracing_libp2p_discv5_logging_layer.rs new file mode 100644 index 0000000000..90033d11ad --- /dev/null +++ b/common/logging/src/tracing_libp2p_discv5_logging_layer.rs @@ -0,0 +1,113 @@ +use chrono::Local; +use logroller::{LogRollerBuilder, Rotation, RotationSize}; +use std::io::Write; +use std::path::PathBuf; +use tracing::Subscriber; +use tracing_appender::non_blocking::{NonBlocking, WorkerGuard}; +use tracing_subscriber::{layer::Context, Layer}; + +pub struct Libp2pDiscv5TracingLayer { + pub libp2p_non_blocking_writer: NonBlocking, + _libp2p_guard: WorkerGuard, + pub discv5_non_blocking_writer: NonBlocking, + _discv5_guard: WorkerGuard, +} + +impl Layer for Libp2pDiscv5TracingLayer +where + S: Subscriber, +{ + fn on_event(&self, event: &tracing::Event<'_>, _ctx: Context) { + let meta = event.metadata(); + let log_level = meta.level(); + let timestamp = Local::now().format("%Y-%m-%d %H:%M:%S").to_string(); + + let target = match meta.target().split_once("::") { + Some((crate_name, _)) => crate_name, + None => "unknown", + }; + + let mut writer = match target { + "libp2p_gossipsub" => self.libp2p_non_blocking_writer.clone(), + "discv5" => self.discv5_non_blocking_writer.clone(), + _ => return, + }; + + let mut visitor = LogMessageExtractor { + message: String::default(), + }; + + event.record(&mut visitor); + let message = format!("{} {} {}\n", timestamp, log_level, visitor.message); + + if let Err(e) = writer.write_all(message.as_bytes()) { + eprintln!("Failed to write log: {}", e); + } + } +} + +struct LogMessageExtractor { + message: String, +} + +impl tracing_core::field::Visit for LogMessageExtractor { + fn record_debug(&mut self, _: &tracing_core::Field, value: &dyn std::fmt::Debug) { + self.message = format!("{} {:?}", self.message, value); + } +} + +pub fn create_libp2p_discv5_tracing_layer( + base_tracing_log_path: Option, + max_log_size: u64, +) -> Option { + if let Some(mut tracing_log_path) = base_tracing_log_path { + // Ensure that `tracing_log_path` only contains directories. + for p in tracing_log_path.clone().iter() { + tracing_log_path = tracing_log_path.join(p); + if let Ok(metadata) = tracing_log_path.metadata() { + if !metadata.is_dir() { + tracing_log_path.pop(); + break; + } + } + } + + let libp2p_writer = + LogRollerBuilder::new(tracing_log_path.clone(), PathBuf::from("libp2p.log")) + .rotation(Rotation::SizeBased(RotationSize::MB(max_log_size))) + .max_keep_files(1); + + let discv5_writer = + LogRollerBuilder::new(tracing_log_path.clone(), PathBuf::from("discv5.log")) + .rotation(Rotation::SizeBased(RotationSize::MB(max_log_size))) + .max_keep_files(1); + + let libp2p_writer = match libp2p_writer.build() { + Ok(writer) => writer, + Err(e) => { + eprintln!("Failed to initialize libp2p rolling file appender: {e}"); + std::process::exit(1); + } + }; + + let discv5_writer = match discv5_writer.build() { + Ok(writer) => writer, + Err(e) => { + eprintln!("Failed to initialize discv5 rolling file appender: {e}"); + std::process::exit(1); + } + }; + + let (libp2p_non_blocking_writer, _libp2p_guard) = NonBlocking::new(libp2p_writer); + let (discv5_non_blocking_writer, _discv5_guard) = NonBlocking::new(discv5_writer); + + Some(Libp2pDiscv5TracingLayer { + libp2p_non_blocking_writer, + _libp2p_guard, + discv5_non_blocking_writer, + _discv5_guard, + }) + } else { + None + } +} diff --git a/common/logging/src/tracing_logging_layer.rs b/common/logging/src/tracing_logging_layer.rs index 4478e1facb..810f7e960e 100644 --- a/common/logging/src/tracing_logging_layer.rs +++ b/common/logging/src/tracing_logging_layer.rs @@ -1,3 +1,5 @@ +use crate::utils::is_ascii_control; + use chrono::prelude::*; use serde_json::{Map, Value}; use std::collections::HashMap; @@ -13,14 +15,11 @@ use tracing_subscriber::Layer; pub struct LoggingLayer { pub non_blocking_writer: NonBlocking, - pub guard: WorkerGuard, + _guard: WorkerGuard, pub disable_log_timestamp: bool, pub log_color: bool, - pub logfile_color: bool, pub log_format: Option, - pub logfile_format: Option, pub extra_info: bool, - pub dep_logs: bool, span_fields: Arc>>, } @@ -28,25 +27,19 @@ impl LoggingLayer { #[allow(clippy::too_many_arguments)] pub fn new( non_blocking_writer: NonBlocking, - guard: WorkerGuard, + _guard: WorkerGuard, disable_log_timestamp: bool, log_color: bool, - logfile_color: bool, log_format: Option, - logfile_format: Option, extra_info: bool, - dep_logs: bool, ) -> Self { Self { non_blocking_writer, - guard, + _guard, disable_log_timestamp, log_color, - logfile_color, log_format, - logfile_format, extra_info, - dep_logs, span_fields: Arc::new(Mutex::new(HashMap::new())), } } @@ -84,16 +77,6 @@ where String::new() }; - if !self.dep_logs { - if let Some(file) = meta.file() { - if file.contains("/.cargo/") { - return; - } - } else { - return; - } - } - let mut writer = self.non_blocking_writer.clone(); let mut visitor = LogMessageExtractor { @@ -122,16 +105,10 @@ where None => "".to_string(), }; - if module.contains("discv5") { - visitor - .fields - .push(("service".to_string(), "\"discv5\"".to_string())); - } - let gray = "\x1b[90m"; let reset = "\x1b[0m"; let location = if self.extra_info { - if self.logfile_color { + if self.log_color { format!("{}{}::{}:{}{}", gray, module, file, line, reset) } else { format!("{}::{}:{}", module, file, line) @@ -164,33 +141,16 @@ where } }; - if self.dep_logs { - if self.logfile_format.as_deref() == Some("JSON") { - build_json_log_file( - &visitor, - plain_level_str, - meta, - &ctx, - &self.span_fields, - event, - &mut writer, - ); - } else { - build_log_text( - &visitor, - plain_level_str, - ×tamp, - &ctx, - &self.span_fields, - event, - &location, - color_level_str, - self.logfile_color, - &mut writer, - ); - } - } else if self.log_format.as_deref() == Some("JSON") { - build_json_log_stdout(&visitor, plain_level_str, ×tamp, &mut writer); + if self.log_format.as_deref() == Some("JSON") { + build_log_json( + &visitor, + plain_level_str, + meta, + &ctx, + &self.span_fields, + event, + &mut writer, + ); } else { build_log_text( &visitor, @@ -300,49 +260,7 @@ impl tracing_core::field::Visit for LogMessageExtractor { } } -/// Function to filter out ascii control codes. -/// -/// This helps to keep log formatting consistent. -/// Whitespace and padding control codes are excluded. -fn is_ascii_control(character: &u8) -> bool { - matches!( - character, - b'\x00'..=b'\x08' | - b'\x0b'..=b'\x0c' | - b'\x0e'..=b'\x1f' | - b'\x7f' | - b'\x81'..=b'\x9f' - ) -} - -fn build_json_log_stdout( - visitor: &LogMessageExtractor, - plain_level_str: &str, - timestamp: &str, - writer: &mut impl Write, -) { - let mut log_map = Map::new(); - log_map.insert("msg".to_string(), Value::String(visitor.message.clone())); - log_map.insert( - "level".to_string(), - Value::String(plain_level_str.to_string()), - ); - log_map.insert("ts".to_string(), Value::String(timestamp.to_string())); - - for (key, val) in visitor.fields.clone().into_iter() { - let parsed_val = parse_field(&val); - log_map.insert(key, parsed_val); - } - - let json_obj = Value::Object(log_map); - let output = format!("{}\n", json_obj); - - if let Err(e) = writer.write_all(output.as_bytes()) { - eprintln!("Failed to write log: {}", e); - } -} - -fn build_json_log_file<'a, S>( +fn build_log_json<'a, S>( visitor: &LogMessageExtractor, plain_level_str: &str, meta: &tracing::Metadata<'_>, diff --git a/common/logging/src/utils.rs b/common/logging/src/utils.rs new file mode 100644 index 0000000000..784cd5ca70 --- /dev/null +++ b/common/logging/src/utils.rs @@ -0,0 +1,31 @@ +use std::collections::HashSet; +use tracing_subscriber::filter::FilterFn; +use workspace_members::workspace_crates; + +const WORKSPACE_CRATES: &[&str] = workspace_crates!(); + +/// Constructs a filter which only permits logging from crates which are members of the workspace. +pub fn build_workspace_filter( +) -> Result bool + Clone>, String> { + let workspace_crates: HashSet<&str> = WORKSPACE_CRATES.iter().copied().collect(); + + Ok(tracing_subscriber::filter::FilterFn::new(move |metadata| { + let target_crate = metadata.target().split("::").next().unwrap_or(""); + workspace_crates.contains(target_crate) + })) +} + +/// Function to filter out ascii control codes. +/// +/// This helps to keep log formatting consistent. +/// Whitespace and padding control codes are excluded. +pub fn is_ascii_control(character: &u8) -> bool { + matches!( + character, + b'\x00'..=b'\x08' | + b'\x0b'..=b'\x0c' | + b'\x0e'..=b'\x1f' | + b'\x7f' | + b'\x81'..=b'\x9f' + ) +} diff --git a/common/workspace_members/Cargo.toml b/common/workspace_members/Cargo.toml new file mode 100644 index 0000000000..05924590e3 --- /dev/null +++ b/common/workspace_members/Cargo.toml @@ -0,0 +1,11 @@ +[package] +name = "workspace_members" +version = "0.1.0" +edition = { workspace = true } + +[lib] +proc-macro = true + +[dependencies] +cargo_metadata = { workspace = true } +quote = { workspace = true } diff --git a/common/workspace_members/src/lib.rs b/common/workspace_members/src/lib.rs new file mode 100644 index 0000000000..1eea0e60e2 --- /dev/null +++ b/common/workspace_members/src/lib.rs @@ -0,0 +1,39 @@ +use cargo_metadata::MetadataCommand; +use proc_macro::TokenStream; +use quote::quote; +use std::error::Error; + +fn get_workspace_crates() -> Result, Box> { + let metadata = MetadataCommand::new().no_deps().exec()?; + + Ok(metadata + .workspace_members + .iter() + .filter_map(|member_id| { + metadata + .packages + .iter() + .find(|package| &package.id == member_id) + .map(|package| package.name.clone()) + }) + .collect()) +} + +#[proc_macro] +pub fn workspace_crates(_input: TokenStream) -> TokenStream { + match get_workspace_crates() { + Ok(crate_names) => { + let crate_strs = crate_names.iter().map(|s| s.as_str()); + quote! { + &[#(#crate_strs),*] + } + } + Err(e) => { + let msg = format!("Failed to get workspace crates: {e}"); + quote! { + compile_error!(#msg); + } + } + } + .into() +} diff --git a/lighthouse/environment/src/lib.rs b/lighthouse/environment/src/lib.rs index f427836751..9b0284e06d 100644 --- a/lighthouse/environment/src/lib.rs +++ b/lighthouse/environment/src/lib.rs @@ -197,6 +197,13 @@ impl EnvironmentBuilder { Ok(self) } + /// Initialize the Lighthouse-specific tracing logging components from + /// the provided config. + /// + /// This consists of 3 tracing `Layers`: + /// - A `Layer` which logs to `stdout` + /// - An `Option` which logs to a log file + /// - An `Option` which emits logs to an SSE stream pub fn init_tracing( mut self, config: LoggerConfig, @@ -204,7 +211,7 @@ impl EnvironmentBuilder { ) -> ( Self, LoggingLayer, - LoggingLayer, + Option, Option, ) { let filename_prefix = match logfile_prefix { @@ -216,72 +223,48 @@ impl EnvironmentBuilder { #[cfg(target_family = "unix")] let file_mode = if config.is_restricted { 0o600 } else { 0o644 }; - let file_logging_layer = { - if let Some(path) = config.path { - let mut appender = LogRollerBuilder::new( - path.clone(), - PathBuf::from(format!("{}.log", filename_prefix)), - ) - .rotation(Rotation::SizeBased(RotationSize::MB(config.max_log_size))) - .max_keep_files(config.max_log_number.try_into().unwrap_or_else(|e| { - eprintln!("Failed to convert max_log_number to u64: {}", e); - 10 - })); + let file_logging_layer = match config.path { + None => { + eprintln!("No logfile path provided, logging to file is disabled"); + None + } + Some(_) if config.max_log_number == 0 || config.max_log_size == 0 => { + // User has explicitly disabled logging to file, so don't emit a message. + None + } + Some(path) => { + let log_filename = PathBuf::from(format!("{}.log", filename_prefix)); + let mut appender = LogRollerBuilder::new(path.clone(), log_filename) + .rotation(Rotation::SizeBased(RotationSize::MB(config.max_log_size))) + .max_keep_files(config.max_log_number.try_into().unwrap_or_else(|e| { + eprintln!("Failed to convert max_log_number to u64: {}", e); + 10 + })); if config.compression { appender = appender.compression(Compression::Gzip); } + match appender.build() { Ok(file_appender) => { #[cfg(target_family = "unix")] set_logfile_permissions(&path, filename_prefix, file_mode); - let (file_non_blocking_writer, file_guard) = - tracing_appender::non_blocking(file_appender); - - LoggingLayer::new( - file_non_blocking_writer, - file_guard, + let (writer, guard) = tracing_appender::non_blocking(file_appender); + Some(LoggingLayer::new( + writer, + guard, config.disable_log_timestamp, - false, config.logfile_color, - config.log_format.clone(), config.logfile_format.clone(), config.extra_info, - false, - ) + )) } Err(e) => { eprintln!("Failed to initialize rolling file appender: {}", e); - let (sink_writer, sink_guard) = - tracing_appender::non_blocking(std::io::sink()); - LoggingLayer::new( - sink_writer, - sink_guard, - config.disable_log_timestamp, - false, - config.logfile_color, - config.log_format.clone(), - config.logfile_format.clone(), - config.extra_info, - false, - ) + None } } - } else { - eprintln!("No path provided. File logging is disabled."); - let (sink_writer, sink_guard) = tracing_appender::non_blocking(std::io::sink()); - LoggingLayer::new( - sink_writer, - sink_guard, - config.disable_log_timestamp, - false, - true, - config.log_format.clone(), - config.logfile_format.clone(), - config.extra_info, - false, - ) } }; @@ -293,11 +276,8 @@ impl EnvironmentBuilder { stdout_guard, config.disable_log_timestamp, config.log_color, - true, config.log_format, - config.logfile_format, config.extra_info, - false, ); let sse_logging_layer_opt = if config.sse_logging { @@ -310,8 +290,8 @@ impl EnvironmentBuilder { ( self, - file_logging_layer, stdout_logging_layer, + file_logging_layer, sse_logging_layer_opt, ) } diff --git a/lighthouse/environment/src/tracing_common.rs b/lighthouse/environment/src/tracing_common.rs index 893f50dae5..dd9fe45cad 100644 --- a/lighthouse/environment/src/tracing_common.rs +++ b/lighthouse/environment/src/tracing_common.rs @@ -1,47 +1,67 @@ use crate::{EnvironmentBuilder, LoggerConfig}; use clap::ArgMatches; use logging::Libp2pDiscv5TracingLayer; -use logging::{tracing_logging_layer::LoggingLayer, SSELoggingComponents}; +use logging::{ + create_libp2p_discv5_tracing_layer, tracing_logging_layer::LoggingLayer, SSELoggingComponents, +}; use std::process; -use tracing_subscriber::filter::{FilterFn, LevelFilter}; + +use tracing_subscriber::filter::LevelFilter; use types::EthSpec; +/// Constructs all logging layers including both Lighthouse-specific and +/// dependency logging. +/// +/// The `Layer`s are as follows: +/// - A `Layer` which logs to `stdout` +/// - An `Option` which logs to a log file +/// - An `Option` which emits logs to an SSE stream +/// - An `Option` which logs relevant dependencies to their +/// own log files. (Currently only `libp2p` and `discv5`) pub fn construct_logger( logger_config: LoggerConfig, matches: &ArgMatches, environment_builder: EnvironmentBuilder, ) -> ( EnvironmentBuilder, - Libp2pDiscv5TracingLayer, - LoggingLayer, - LoggingLayer, - Option, LoggerConfig, - FilterFn, + LoggingLayer, + Option, + Option, + Option, ) { - let libp2p_discv5_layer = logging::create_libp2p_discv5_tracing_layer( - logger_config.path.clone(), - logger_config.max_log_size, - logger_config.compression, - logger_config.max_log_number, - ); + let subcommand_name = matches.subcommand_name(); + let logfile_prefix = subcommand_name.unwrap_or("lighthouse"); - let logfile_prefix = matches.subcommand_name().unwrap_or("lighthouse"); - - let (builder, file_logging_layer, stdout_logging_layer, sse_logging_layer_opt) = + let (builder, stdout_logging_layer, file_logging_layer, sse_logging_layer_opt) = environment_builder.init_tracing(logger_config.clone(), logfile_prefix); - let dependency_log_filter = - FilterFn::new(filter_dependency_log as fn(&tracing::Metadata<'_>) -> bool); + let libp2p_discv5_layer = if let Some(subcommand_name) = subcommand_name { + if subcommand_name == "beacon_node" || subcommand_name == "boot_node" { + if logger_config.max_log_size == 0 || logger_config.max_log_number == 0 { + // User has explicitly disabled logging to file. + None + } else { + create_libp2p_discv5_tracing_layer( + logger_config.path.clone(), + logger_config.max_log_size, + ) + } + } else { + // Disable libp2p and discv5 logs when running other subcommands. + None + } + } else { + None + }; ( builder, - libp2p_discv5_layer, - file_logging_layer, - stdout_logging_layer, - sse_logging_layer_opt, logger_config, - dependency_log_filter, + stdout_logging_layer, + file_logging_layer, + sse_logging_layer_opt, + libp2p_discv5_layer, ) } @@ -58,15 +78,3 @@ pub fn parse_level(level: &str) -> LevelFilter { } } } - -fn filter_dependency_log(meta: &tracing::Metadata<'_>) -> bool { - if let Some(file) = meta.file() { - let target = meta.target(); - if file.contains("/.cargo/") { - return target.contains("discv5") || target.contains("libp2p"); - } else { - return !file.contains("gossipsub") && !target.contains("hyper"); - } - } - true -} diff --git a/lighthouse/src/main.rs b/lighthouse/src/main.rs index a6ab1cfb6b..60e65e6470 100644 --- a/lighthouse/src/main.rs +++ b/lighthouse/src/main.rs @@ -17,17 +17,15 @@ use eth2_network_config::{Eth2NetworkConfig, DEFAULT_HARDCODED_NETWORK, HARDCODE use ethereum_hashing::have_sha_extensions; use futures::TryFutureExt; use lighthouse_version::VERSION; -use logging::crit; -use logging::MetricsLayer; +use logging::{build_workspace_filter, crit, MetricsLayer}; use malloc_utils::configure_memory_allocator; use std::backtrace::Backtrace; use std::path::PathBuf; use std::process::exit; use std::sync::LazyLock; use task_executor::ShutdownReason; -use tracing::{info, warn}; -use tracing_subscriber::prelude::*; -use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt}; +use tracing::{info, warn, Level}; +use tracing_subscriber::{filter::EnvFilter, layer::SubscriberExt, util::SubscriberInitExt, Layer}; use types::{EthSpec, EthSpecId}; use validator_client::ProductionValidatorClient; @@ -592,12 +590,11 @@ fn run( let ( builder, - libp2p_discv5_layer, - file_logging_layer, - stdout_logging_layer, - sse_logging_layer_opt, logger_config, - dependency_log_filter, + stdout_logging_layer, + file_logging_layer, + sse_logging_layer_opt, + libp2p_discv5_layer, ) = tracing_common::construct_logger( LoggerConfig { path: log_path.clone(), @@ -619,21 +616,50 @@ fn run( environment_builder, ); - let logging = tracing_subscriber::registry() - .with(dependency_log_filter) - .with(file_logging_layer.with_filter(logger_config.logfile_debug_level)) - .with(stdout_logging_layer.with_filter(logger_config.debug_level)) - .with(MetricsLayer) - .with(libp2p_discv5_layer); + let workspace_filter = build_workspace_filter()?; - let logging_result = if let Some(sse_logging_layer) = sse_logging_layer_opt { - logging.with(sse_logging_layer).try_init() - } else { - logging.try_init() - }; + let mut logging_layers = Vec::new(); + + logging_layers.push( + stdout_logging_layer + .with_filter(logger_config.debug_level) + .with_filter(workspace_filter.clone()) + .boxed(), + ); + + if let Some(file_logging_layer) = file_logging_layer { + logging_layers.push( + file_logging_layer + .with_filter(logger_config.logfile_debug_level) + .with_filter(workspace_filter) + .boxed(), + ); + } + + if let Some(sse_logging_layer) = sse_logging_layer_opt { + logging_layers.push(sse_logging_layer.boxed()); + } + + if let Some(libp2p_discv5_layer) = libp2p_discv5_layer { + logging_layers.push( + libp2p_discv5_layer + .with_filter( + EnvFilter::builder() + .with_default_directive(Level::DEBUG.into()) + .from_env_lossy(), + ) + .boxed(), + ); + } + + logging_layers.push(MetricsLayer.boxed()); + + let logging_result = tracing_subscriber::registry() + .with(logging_layers) + .try_init(); if let Err(e) = logging_result { - eprintln!("Failed to initialize dependency logging: {e}"); + eprintln!("Failed to initialize logger: {e}"); } let mut environment = builder diff --git a/testing/simulator/src/basic_sim.rs b/testing/simulator/src/basic_sim.rs index 4cd599f845..6afc7771d4 100644 --- a/testing/simulator/src/basic_sim.rs +++ b/testing/simulator/src/basic_sim.rs @@ -15,7 +15,6 @@ use std::sync::Arc; use std::time::Duration; use environment::tracing_common; -use logging::MetricsLayer; use tracing_subscriber::prelude::*; use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt}; @@ -90,12 +89,11 @@ pub fn run_basic_sim(matches: &ArgMatches) -> Result<(), String> { let ( env_builder, - _libp2p_discv5_layer, - file_logging_layer, - stdout_logging_layer, - _sse_logging_layer_opt, logger_config, - _dependency_log_filter, + stdout_logging_layer, + _file_logging_layer, + _sse_logging_layer_opt, + _libp2p_discv5_layer, ) = tracing_common::construct_logger( LoggerConfig { path: None, @@ -118,9 +116,7 @@ pub fn run_basic_sim(matches: &ArgMatches) -> Result<(), String> { ); if let Err(e) = tracing_subscriber::registry() - .with(file_logging_layer.with_filter(logger_config.logfile_debug_level)) .with(stdout_logging_layer.with_filter(logger_config.debug_level)) - .with(MetricsLayer) .try_init() { eprintln!("Failed to initialize dependency logging: {e}"); diff --git a/testing/simulator/src/fallback_sim.rs b/testing/simulator/src/fallback_sim.rs index 384699c64c..f4e0d20f38 100644 --- a/testing/simulator/src/fallback_sim.rs +++ b/testing/simulator/src/fallback_sim.rs @@ -5,7 +5,6 @@ use clap::ArgMatches; use crate::retry::with_retry; use environment::tracing_common; use futures::prelude::*; -use logging::MetricsLayer; use node_test_rig::{ environment::{EnvironmentBuilder, LoggerConfig}, testing_validator_config, ValidatorFiles, @@ -94,12 +93,11 @@ pub fn run_fallback_sim(matches: &ArgMatches) -> Result<(), String> { let ( env_builder, - libp2p_discv5_layer, - file_logging_layer, - stdout_logging_layer, - _sse_logging_layer_opt, logger_config, - dependency_log_filter, + stdout_logging_layer, + _file_logging_layer, + _sse_logging_layer_opt, + _libp2p_discv5_layer, ) = tracing_common::construct_logger( LoggerConfig { path: None, @@ -122,11 +120,7 @@ pub fn run_fallback_sim(matches: &ArgMatches) -> Result<(), String> { ); if let Err(e) = tracing_subscriber::registry() - .with(dependency_log_filter) - .with(file_logging_layer.with_filter(logger_config.logfile_debug_level)) .with(stdout_logging_layer.with_filter(logger_config.debug_level)) - .with(libp2p_discv5_layer) - .with(MetricsLayer) .try_init() { eprintln!("Failed to initialize dependency logging: {e}"); From 33e41f7249b7c34d9d3271fe4408eaa2a6dfeedc Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Tue, 1 Apr 2025 16:24:07 -0700 Subject: [PATCH 10/17] Consensus spec tests beta4 (#7231) N/A Run latest consensus spec tests on CI https://github.com/ethereum/consensus-specs/releases/tag/v1.5.0-beta.4 --- testing/ef_tests/Makefile | 2 +- testing/ef_tests/check_all_files_accessed.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index c32a670e9a..c3a56ec11a 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -1,4 +1,4 @@ -TESTS_TAG := v1.5.0-beta.2 +TESTS_TAG := v1.5.0-beta.4 TESTS = general minimal mainnet TARBALLS = $(patsubst %,%-$(TESTS_TAG).tar.gz,$(TESTS)) diff --git a/testing/ef_tests/check_all_files_accessed.py b/testing/ef_tests/check_all_files_accessed.py index 4e744b797a..3aeff8ce06 100755 --- a/testing/ef_tests/check_all_files_accessed.py +++ b/testing/ef_tests/check_all_files_accessed.py @@ -50,6 +50,8 @@ excluded_paths = [ # TODO(das): Fulu tests are ignored for now "tests/.*/fulu", "tests/.*/fulu/ssz_static/MatrixEntry", + "tests/.*/eip7441", + "tests/.*/eip7732", ] From 0850bcfb89d1048030c1aced795f3d43d91abeb0 Mon Sep 17 00:00:00 2001 From: Varun Doshi <61531351+varun-doshi@users.noreply.github.com> Date: Wed, 2 Apr 2025 08:02:08 +0530 Subject: [PATCH 11/17] feat: add more bootnodes for Hoodi and Sepolia (#7222) Closes #7218 Add more bootnodes for Sepolia and Hoodi --- .../built_in_network_configs/hoodi/boot_enr.yaml | 3 +++ .../built_in_network_configs/sepolia/boot_enr.yaml | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/common/eth2_network_config/built_in_network_configs/hoodi/boot_enr.yaml b/common/eth2_network_config/built_in_network_configs/hoodi/boot_enr.yaml index 33eaa7e8a9..5d8df4006c 100644 --- a/common/eth2_network_config/built_in_network_configs/hoodi/boot_enr.yaml +++ b/common/eth2_network_config/built_in_network_configs/hoodi/boot_enr.yaml @@ -11,3 +11,6 @@ - enr:-Ku4QIC89sMC0o-irosD4_23lJJ4qCGOvdUz7SmoShWx0k6AaxCFTKviEHa-sa7-EzsiXpDp0qP0xzX6nKdXJX3X-IQBh2F0dG5ldHOIAAAAAAAAAACEZXRoMpBd9cEGEAAJEP__________gmlkgnY0gmlwhIbRilSJc2VjcDI1NmsxoQK_m0f1DzDc9Cjrspm36zuRa7072HSiMGYWLsKiVSbP34N1ZHCCIyk - enr:-Ku4QNkWjw5tNzo8DtWqKm7CnDdIq_y7xppD6c1EZSwjB8rMOkSFA1wJPLoKrq5UvA7wcxIotH6Usx3PAugEN2JMncIBh2F0dG5ldHOIAAAAAAAAAACEZXRoMpBd9cEGEAAJEP__________gmlkgnY0gmlwhIbHuBeJc2VjcDI1NmsxoQP3FwrhFYB60djwRjAoOjttq6du94DtkQuaN99wvgqaIYN1ZHCCIyk - enr:-OS4QMJGE13xEROqvKN1xnnt7U-noc51VXyM6wFMuL9LMhQDfo1p1dF_zFdS4OsnXz_vIYk-nQWnqJMWRDKvkSK6_CwDh2F0dG5ldHOIAAAAADAAAACGY2xpZW502IpMaWdodGhvdXNljDcuMC4wLWJldGEuM4RldGgykNLxmX9gAAkQAAgAAAAAAACCaWSCdjSCaXCEhse4F4RxdWljgiMqiXNlY3AyNTZrMaECef77P8k5l3PC_raLw42OAzdXfxeQ-58BJriNaqiRGJSIc3luY25ldHMAg3RjcIIjKIN1ZHCCIyg +# Teku +- enr:-LK4QDwhXMitMbC8xRiNL-XGMhRyMSOnxej-zGifjv9Nm5G8EF285phTU-CAsMHRRefZimNI7eNpAluijMQP7NDC8kEMh2F0dG5ldHOIAAAAAAAABgCEZXRoMpDS8Zl_YAAJEAAIAAAAAAAAgmlkgnY0gmlwhAOIT_SJc2VjcDI1NmsxoQMoHWNL4MAvh6YpQeM2SUjhUrLIPsAVPB8nyxbmckC6KIN0Y3CCIyiDdWRwgiMo +- enr:-LK4QPYl2HnMPQ7b1es6Nf_tFYkyya5bj9IqAKOEj2cmoqVkN8ANbJJJK40MX4kciL7pZszPHw6vLNyeC-O3HUrLQv8Mh2F0dG5ldHOIAAAAAAAAAMCEZXRoMpDS8Zl_YAAJEAAIAAAAAAAAgmlkgnY0gmlwhAMYRG-Jc2VjcDI1NmsxoQPQ35tjr6q1qUqwAnegQmYQyfqxC_6437CObkZneI9n34N0Y3CCIyiDdWRwgiMo diff --git a/common/eth2_network_config/built_in_network_configs/sepolia/boot_enr.yaml b/common/eth2_network_config/built_in_network_configs/sepolia/boot_enr.yaml index 22b711861f..ba9a3e8354 100644 --- a/common/eth2_network_config/built_in_network_configs/sepolia/boot_enr.yaml +++ b/common/eth2_network_config/built_in_network_configs/sepolia/boot_enr.yaml @@ -1,6 +1,12 @@ -# EF bootnodes +# EF - enr:-Ku4QDZ_rCowZFsozeWr60WwLgOfHzv1Fz2cuMvJqN5iJzLxKtVjoIURY42X_YTokMi3IGstW5v32uSYZyGUXj9Q_IECh2F0dG5ldHOIAAAAAAAAAACEZXRoMpCo_ujukAAAaf__________gmlkgnY0gmlwhIpEe5iJc2VjcDI1NmsxoQNHTpFdaNSCEWiN_QqT396nb0PzcUpLe3OVtLph-AciBYN1ZHCCIy0 - enr:-Ku4QHRyRwEPT7s0XLYzJ_EeeWvZTXBQb4UCGy1F_3m-YtCNTtDlGsCMr4UTgo4uR89pv11uM-xq4w6GKfKhqU31hTgCh2F0dG5ldHOIAAAAAAAAAACEZXRoMpCo_ujukAAAaf__________gmlkgnY0gmlwhIrFM7WJc2VjcDI1NmsxoQI4diTwChN3zAAkarf7smOHCdFb1q3DSwdiQ_Lc_FdzFIN1ZHCCIy0 - enr:-Ku4QOkvvf0u5Hg4-HhY-SJmEyft77G5h3rUM8VF_e-Hag5cAma3jtmFoX4WElLAqdILCA-UWFRN1ZCDJJVuEHrFeLkDh2F0dG5ldHOIAAAAAAAAAACEZXRoMpCo_ujukAAAaf__________gmlkgnY0gmlwhJK-AWeJc2VjcDI1NmsxoQLFcT5VE_NMiIC8Ll7GypWDnQ4UEmuzD7hF_Hf4veDJwIN1ZHCCIy0 - enr:-Ku4QH6tYsHKITYeHUu5kdfXgEZWI18EWk_2RtGOn1jBPlx2UlS_uF3Pm5Dx7tnjOvla_zs-wwlPgjnEOcQDWXey51QCh2F0dG5ldHOIAAAAAAAAAACEZXRoMpCo_ujukAAAaf__________gmlkgnY0gmlwhIs7Mc6Jc2VjcDI1NmsxoQIET4Mlv9YzhrYhX_H9D7aWMemUrvki6W4J2Qo0YmFMp4N1ZHCCIy0 - enr:-Ku4QDmz-4c1InchGitsgNk4qzorWMiFUoaPJT4G0IiF8r2UaevrekND1o7fdoftNucirj7sFFTTn2-JdC2Ej0p1Mn8Ch2F0dG5ldHOIAAAAAAAAAACEZXRoMpCo_ujukAAAaf__________gmlkgnY0gmlwhKpA-liJc2VjcDI1NmsxoQMpHP5U1DK8O_JQU6FadmWbE42qEdcGlllR8HcSkkfWq4N1ZHCCIy0 + +# Teku bootnode +- enr:-KO4QP7MmB3juk8rUjJHcUoxZDU9Np4FlW0HyDEGIjSO7GD9PbSsabu7713cWSUWKDkxIypIXg1A-6lG7ySRGOMZHeGCAmuEZXRoMpDTH2GRkAAAc___________gmlkgnY0gmlwhBSoyGOJc2VjcDI1NmsxoQNta5b_bexSSwwrGW2Re24MjfMntzFd0f2SAxQtMj3ueYN0Y3CCIyiDdWRwgiMo + +# Lodestar +- enr:-KG4QJejf8KVtMeAPWFhN_P0c4efuwu1pZHELTveiXUeim6nKYcYcMIQpGxxdgT2Xp9h-M5pr9gn2NbbwEAtxzu50Y8BgmlkgnY0gmlwhEEVkQCDaXA2kCoBBPnAEJg4AAAAAAAAAAGJc2VjcDI1NmsxoQLEh_eVvk07AQABvLkTGBQTrrIOQkzouMgSBtNHIRUxOIN1ZHCCIyiEdWRwNoIjKA From 80626e58d224ddd9ded3cd0ae67f02da59cbcba9 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Thu, 3 Apr 2025 15:01:34 +1100 Subject: [PATCH 12/17] Attempt to fix flaky network tests (#7244) --- .../network/src/subnet_service/tests/mod.rs | 93 ++++++++++--------- 1 file changed, 49 insertions(+), 44 deletions(-) diff --git a/beacon_node/network/src/subnet_service/tests/mod.rs b/beacon_node/network/src/subnet_service/tests/mod.rs index 7e274850b5..6f9e8cd41a 100644 --- a/beacon_node/network/src/subnet_service/tests/mod.rs +++ b/beacon_node/network/src/subnet_service/tests/mod.rs @@ -116,18 +116,16 @@ fn get_subnet_service() -> SubnetService { ) } -// gets a number of events from the subscription service, or returns none if it times out after a number -// of slots -async fn get_events + Unpin>( +// gets a number of events from the subscription service, or returns none if it times out after a +// specified duration. +async fn get_events_until_timeout + Unpin>( stream: &mut S, num_events: Option, - num_slots_before_timeout: u32, + timeout: Duration, ) -> Vec { let mut events = Vec::new(); - - let timeout = - tokio::time::sleep(Duration::from_millis(SLOT_DURATION_MILLIS) * num_slots_before_timeout); - futures::pin_mut!(timeout); + let sleep = tokio::time::sleep(timeout); + futures::pin_mut!(sleep); loop { tokio::select! { @@ -139,7 +137,7 @@ async fn get_events + Unpin>( } } } - _ = timeout.as_mut() => { + _ = sleep.as_mut() => { break; } @@ -149,6 +147,17 @@ async fn get_events + Unpin>( events } +// gets a number of events from the subscription service, or returns none if it times out after a number +// of slots +async fn get_events_until_num_slots + Unpin>( + stream: &mut S, + num_events: Option, + num_slots_before_timeout: u32, +) -> Vec { + let timeout = Duration::from_millis(SLOT_DURATION_MILLIS) * num_slots_before_timeout; + get_events_until_timeout(stream, num_events, timeout).await +} + mod test { #[cfg(not(windows))] @@ -196,7 +205,7 @@ mod test { // create the attestation service and subscriptions let mut subnet_service = get_subnet_service(); - let _events = get_events(&mut subnet_service, None, 1).await; + let _events = get_events_until_num_slots(&mut subnet_service, None, 1).await; let current_slot = subnet_service .beacon_chain @@ -249,7 +258,7 @@ mod test { ]; // Wait for 1 slot duration to get the unsubscription event - let events = get_events( + let events = get_events_until_num_slots( &mut subnet_service, Some(2), (MainnetEthSpec::slots_per_epoch()) as u32, @@ -281,7 +290,7 @@ mod test { // create the subnet service and subscriptions let mut subnet_service = get_subnet_service(); - let _events = get_events(&mut subnet_service, None, 0).await; + let _events = get_events_until_num_slots(&mut subnet_service, None, 0).await; let current_slot = subnet_service .beacon_chain .slot_clock @@ -330,14 +339,14 @@ mod test { if subnet_service.is_subscribed(&Subnet::Attestation(subnet_id1)) { // If we are permanently subscribed to this subnet, we won't see a subscribe message - let _ = get_events(&mut subnet_service, None, 1).await; + let _ = get_events_until_num_slots(&mut subnet_service, None, 1).await; } else { - let subscription = get_events(&mut subnet_service, None, 1).await; + let subscription = get_events_until_num_slots(&mut subnet_service, None, 1).await; assert_eq!(subscription, [expected]); } // Get event for 1 more slot duration, we should get the unsubscribe event now. - let unsubscribe_event = get_events(&mut subnet_service, None, 1).await; + let unsubscribe_event = get_events_until_num_slots(&mut subnet_service, None, 1).await; // If the long lived and short lived subnets are different, we should get an unsubscription // event. @@ -376,7 +385,7 @@ mod test { // submit the subscriptions subnet_service.validator_subscriptions(subscriptions.into_iter()); - let events = get_events(&mut subnet_service, Some(130), 10).await; + let events = get_events_until_num_slots(&mut subnet_service, Some(130), 10).await; let mut discover_peer_count = 0; let mut enr_add_count = 0; let mut unsubscribe_event_count = 0; @@ -445,7 +454,7 @@ mod test { // submit the subscriptions subnet_service.validator_subscriptions(subscriptions.into_iter()); - let events = get_events(&mut subnet_service, None, 3).await; + let events = get_events_until_num_slots(&mut subnet_service, None, 3).await; let mut discover_peer_count = 0; let mut enr_add_count = 0; let mut unexpected_msg_count = 0; @@ -495,7 +504,7 @@ mod test { // create the attestation service and subscriptions let mut subnet_service = get_subnet_service(); // Remove permanent events - let _events = get_events(&mut subnet_service, None, 0).await; + let _events = get_events_until_num_slots(&mut subnet_service, None, 0).await; let current_slot = subnet_service .beacon_chain @@ -560,7 +569,7 @@ mod test { // Unsubscription event should happen at the end of the slot. // We wait for 2 slots, to avoid timeout issues - let events = get_events(&mut subnet_service, None, 2).await; + let events = get_events_until_num_slots(&mut subnet_service, None, 2).await; let expected_subscription = SubnetServiceMessage::Subscribe(Subnet::Attestation(subnet_id1)); @@ -577,28 +586,26 @@ mod test { println!("{events:?}"); let subscription_slot = current_slot + subscription_slot2 - 1; // one less do to the // advance subscription time - let wait_slots = subnet_service + let wait_duration = subnet_service .beacon_chain .slot_clock .duration_to_slot(subscription_slot) - .unwrap() - .as_millis() as u64 - / SLOT_DURATION_MILLIS; + .unwrap(); - let no_events = dbg!(get_events(&mut subnet_service, None, wait_slots as u32).await); + let no_events = + dbg!(get_events_until_timeout(&mut subnet_service, None, wait_duration).await); assert_eq!(no_events, []); let subscription_end_slot = current_slot + subscription_slot2 + 2; // +1 to get to the end of the duty slot, +1 for the slot to complete - let wait_slots = subnet_service + let wait_duration = subnet_service .beacon_chain .slot_clock .duration_to_slot(subscription_end_slot) - .unwrap() - .as_millis() as u64 - / SLOT_DURATION_MILLIS; + .unwrap(); - let second_subscribe_event = get_events(&mut subnet_service, None, wait_slots as u32).await; + let second_subscribe_event = + get_events_until_timeout(&mut subnet_service, None, wait_duration).await; // If the permanent and short lived subnets are different, we should get an unsubscription event. if !subnet_service.is_subscribed_permanent(&Subnet::Attestation(subnet_id1)) { assert_eq!( @@ -612,28 +619,26 @@ mod test { let subscription_slot = current_slot + subscription_slot3 - 1; - let wait_slots = subnet_service + let wait_duration = subnet_service .beacon_chain .slot_clock .duration_to_slot(subscription_slot) - .unwrap() - .as_millis() as u64 - / SLOT_DURATION_MILLIS; + .unwrap(); - let no_events = dbg!(get_events(&mut subnet_service, None, wait_slots as u32).await); + let no_events = + dbg!(get_events_until_timeout(&mut subnet_service, None, wait_duration).await); assert_eq!(no_events, []); let subscription_end_slot = current_slot + subscription_slot3 + 2; // +1 to get to the end of the duty slot, +1 for the slot to complete - let wait_slots = subnet_service + let wait_duration = subnet_service .beacon_chain .slot_clock .duration_to_slot(subscription_end_slot) - .unwrap() - .as_millis() as u64 - / SLOT_DURATION_MILLIS; + .unwrap(); - let third_subscribe_event = get_events(&mut subnet_service, None, wait_slots as u32).await; + let third_subscribe_event = + get_events_until_timeout(&mut subnet_service, None, wait_duration).await; if !subnet_service.is_subscribed_permanent(&Subnet::Attestation(subnet_id1)) { assert_eq!( @@ -652,7 +657,7 @@ mod test { // create the attestation service and subscriptions let mut subnet_service = get_subnet_service(); - let _events = get_events(&mut subnet_service, None, 0).await; + let _events = get_events_until_num_slots(&mut subnet_service, None, 0).await; let subscriptions = std::iter::once(Subscription::SyncCommittee(SyncCommitteeSubscription { @@ -673,7 +678,7 @@ mod test { let subnet_id = subnet_ids.iter().next().unwrap(); // Note: the unsubscription event takes 2 epochs (8 * 2 * 0.4 secs = 3.2 secs) - let events = get_events( + let events = get_events_until_num_slots( &mut subnet_service, Some(5), (MainnetEthSpec::slots_per_epoch() * 3) as u32, // Have some buffer time before getting 5 events @@ -709,7 +714,7 @@ mod test { // create the attestation service and subscriptions let mut subnet_service = get_subnet_service(); // Get the initial events from permanent subnet subscriptions - let _events = get_events(&mut subnet_service, None, 1).await; + let _events = get_events_until_num_slots(&mut subnet_service, None, 1).await; let subscriptions = std::iter::once(Subscription::SyncCommittee(SyncCommitteeSubscription { @@ -722,7 +727,7 @@ mod test { subnet_service.validator_subscriptions(subscriptions); // Get all immediate events (won't include unsubscriptions) - let events = get_events(&mut subnet_service, None, 1).await; + let events = get_events_until_num_slots(&mut subnet_service, None, 1).await; matches::assert_matches!( events[..], [ @@ -752,7 +757,7 @@ mod test { subnet_service.validator_subscriptions(subscriptions.into_iter()); // Get all immediate events (won't include unsubscriptions) - let events = get_events(&mut subnet_service, None, 1).await; + let events = get_events_until_num_slots(&mut subnet_service, None, 1).await; matches::assert_matches!(events[..], [SubnetServiceMessage::DiscoverPeers(_),]); // Should be unsubscribed at the end. From d6cd049a453bb5c091931b24408750699f6d4939 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Thu, 3 Apr 2025 21:10:15 +1100 Subject: [PATCH 13/17] RPC RequestId Cleanup (#7238) I've been working at updating another library to latest Lighthouse and got very confused with RPC request Ids. There were types that had fields called `request_id` and `id`. And interchangeably could have types `PeerRequestId`, `rpc::RequestId`, `AppRequestId`, `api_types::RequestId` or even `Request.id`. I couldn't keep track of which Id was linked to what and what each type meant. So this PR mainly does a few things: - Changes the field naming to match the actual type. So any field that has an `AppRequestId` will be named `app_request_id` rather than `id` or `request_id` for example. - I simplified the types. I removed the two different `RequestId` types (one in Lighthouse_network the other in the rpc) and grouped them into one. It has one downside tho. I had to add a few unreachable lines of code in the beacon processor, which the extra type would prevent, but I feel like it might be worth it. Happy to add an extra type to avoid those few lines. - I also removed the concept of `PeerRequestId` which sometimes went alongside a `request_id`. There were times were had a `PeerRequest` and a `Request` being returned, both of which contain a `RequestId` so we had redundant information. I've simplified the logic by removing `PeerRequestId` and made a `ResponseId`. I think if you look at the code changes, it simplifies things a bit and removes the redundant extra info. I think with this PR things are a little bit easier to reasonable about what is going on with all these RPC Ids. NOTE: I did this with the help of AI, so probably should be checked --- beacon_node/lighthouse_network/src/lib.rs | 2 +- .../lighthouse_network/src/rpc/handler.rs | 74 +++--- beacon_node/lighthouse_network/src/rpc/mod.rs | 98 +++----- .../src/rpc/self_limiter.rs | 16 +- .../src/service/api_types.rs | 15 +- .../lighthouse_network/src/service/mod.rs | 127 +++++----- .../lighthouse_network/tests/rpc_tests.rs | 113 ++++----- .../src/network_beacon_processor/mod.rs | 136 ++-------- .../network_beacon_processor/rpc_methods.rs | 237 ++++-------------- .../src/network_beacon_processor/tests.rs | 10 +- beacon_node/network/src/router.rs | 180 +++++-------- beacon_node/network/src/service.rs | 52 ++-- beacon_node/network/src/sync/manager.rs | 46 ++-- .../network/src/sync/network_context.rs | 16 +- beacon_node/network/src/sync/tests/lookups.rs | 43 ++-- beacon_node/network/src/sync/tests/range.rs | 12 +- 16 files changed, 438 insertions(+), 739 deletions(-) diff --git a/beacon_node/lighthouse_network/src/lib.rs b/beacon_node/lighthouse_network/src/lib.rs index 2f8fd82c51..dbeb0c2c2b 100644 --- a/beacon_node/lighthouse_network/src/lib.rs +++ b/beacon_node/lighthouse_network/src/lib.rs @@ -122,6 +122,6 @@ pub use peer_manager::{ ConnectionDirection, PeerConnectionStatus, PeerInfo, PeerManager, SyncInfo, SyncStatus, }; // pub use service::{load_private_key, Context, Libp2pEvent, Service, NETWORK_KEY_FILENAME}; -pub use service::api_types::{PeerRequestId, Response}; +pub use service::api_types::Response; pub use service::utils::*; pub use service::{Gossipsub, NetworkEvent}; diff --git a/beacon_node/lighthouse_network/src/rpc/handler.rs b/beacon_node/lighthouse_network/src/rpc/handler.rs index 8353b661c5..b86e2b3a6f 100644 --- a/beacon_node/lighthouse_network/src/rpc/handler.rs +++ b/beacon_node/lighthouse_network/src/rpc/handler.rs @@ -4,8 +4,7 @@ use super::methods::{GoodbyeReason, RpcErrorResponse, RpcResponse}; use super::outbound::OutboundRequestContainer; use super::protocol::{InboundOutput, Protocol, RPCError, RPCProtocol, RequestType}; -use super::RequestId; -use super::{RPCReceived, RPCSend, ReqId, Request}; +use super::{RPCReceived, RPCSend, ReqId}; use crate::rpc::outbound::OutboundFramed; use crate::rpc::protocol::InboundFramed; use fnv::FnvHashMap; @@ -91,6 +90,11 @@ pub struct RPCHandler where E: EthSpec, { + /// The PeerId matching this `ConnectionHandler`. + peer_id: PeerId, + + /// The ConnectionId matching this `ConnectionHandler`. + connection_id: ConnectionId, /// The upgrade for inbound substreams. listen_protocol: SubstreamProtocol, ()>, @@ -139,9 +143,6 @@ where /// Timeout that will me used for inbound and outbound responses. resp_timeout: Duration, - - /// Information about this handler for logging purposes. - log_info: (PeerId, ConnectionId), } enum HandlerState { @@ -228,6 +229,8 @@ where connection_id: ConnectionId, ) -> Self { RPCHandler { + connection_id, + peer_id, listen_protocol, events_out: SmallVec::new(), dial_queue: SmallVec::new(), @@ -244,7 +247,6 @@ where fork_context, waker: None, resp_timeout, - log_info: (peer_id, connection_id), } } @@ -255,8 +257,8 @@ where if !self.dial_queue.is_empty() { debug!( unsent_queued_requests = self.dial_queue.len(), - peer_id = %self.log_info.0, - connection_id = %self.log_info.1, + peer_id = %self.peer_id, + connection_id = %self.connection_id, "Starting handler shutdown" ); } @@ -306,8 +308,8 @@ where if !matches!(response, RpcResponse::StreamTermination(..)) { // the stream is closed after sending the expected number of responses trace!(%response, id = ?inbound_id, - peer_id = %self.log_info.0, - connection_id = %self.log_info.1, + peer_id = %self.peer_id, + connection_id = %self.connection_id, "Inbound stream has expired. Response not sent"); } return; @@ -324,8 +326,8 @@ where if matches!(self.state, HandlerState::Deactivated) { // we no longer send responses after the handler is deactivated debug!(%response, id = ?inbound_id, - peer_id = %self.log_info.0, - connection_id = %self.log_info.1, + peer_id = %self.peer_id, + connection_id = %self.connection_id, "Response not sent. Deactivated handler"); return; } @@ -394,8 +396,8 @@ where Poll::Ready(_) => { self.state = HandlerState::Deactivated; debug!( - peer_id = %self.log_info.0, - connection_id = %self.log_info.1, + peer_id = %self.peer_id, + connection_id = %self.connection_id, "Shutdown timeout elapsed, Handler deactivated" ); return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( @@ -445,8 +447,8 @@ where ))); } else { crit!( - peer_id = %self.log_info.0, - connection_id = %self.log_info.1, + peer_id = %self.peer_id, + connection_id = %self.connection_id, stream_id = ?outbound_id.get_ref(), "timed out substream not in the books"); } } @@ -577,8 +579,8 @@ where // Its useful to log when the request was completed. if matches!(info.protocol, Protocol::BlocksByRange) { debug!( - peer_id = %self.log_info.0, - connection_id = %self.log_info.1, + peer_id = %self.peer_id, + connection_id = %self.connection_id, duration = Instant::now() .duration_since(info.request_start_time) .as_secs(), @@ -587,8 +589,8 @@ where } if matches!(info.protocol, Protocol::BlobsByRange) { debug!( - peer_id = %self.log_info.0, - connection_id = %self.log_info.1, + peer_id = %self.peer_id, + connection_id = %self.connection_id, duration = Instant::now() .duration_since(info.request_start_time) .as_secs(), @@ -617,16 +619,16 @@ where if matches!(info.protocol, Protocol::BlocksByRange) { debug!( - peer_id = %self.log_info.0, - connection_id = %self.log_info.1, + peer_id = %self.peer_id, + connection_id = %self.connection_id, duration = info.request_start_time.elapsed().as_secs(), "BlocksByRange Response failed" ); } if matches!(info.protocol, Protocol::BlobsByRange) { debug!( - peer_id = %self.log_info.0, - connection_id = %self.log_info.1, + peer_id = %self.peer_id, + connection_id = %self.connection_id, duration = info.request_start_time.elapsed().as_secs(), "BlobsByRange Response failed" ); @@ -816,8 +818,8 @@ where } OutboundSubstreamState::Poisoned => { crit!( - peer_id = %self.log_info.0, - connection_id = %self.log_info.1, + peer_id = %self.peer_id, + connection_id = %self.connection_id, "Poisoned outbound substream" ); unreachable!("Coding Error: Outbound substream is poisoned") @@ -852,8 +854,8 @@ where && self.dial_negotiated == 0 { debug!( - peer_id = %self.log_info.0, - connection_id = %self.log_info.1, + peer_id = %self.peer_id, + connection_id = %self.connection_id, "Goodbye sent, Handler deactivated" ); self.state = HandlerState::Deactivated; @@ -986,12 +988,13 @@ where self.shutdown(None); } - self.events_out - .push(HandlerEvent::Ok(RPCReceived::Request(Request { - id: RequestId::next(), + self.events_out.push(HandlerEvent::Ok(RPCReceived::Request( + super::InboundRequestId { + connection_id: self.connection_id, substream_id: self.current_inbound_substream_id, - r#type: req, - }))); + }, + req, + ))); self.current_inbound_substream_id.0 += 1; } @@ -1049,9 +1052,8 @@ where .is_some() { crit!( - peer_id = %self.log_info.0, - connection_id = %self.log_info.1, - + peer_id = %self.peer_id, + connection_id = %self.connection_id, id = ?self.current_outbound_substream_id, "Duplicate outbound substream id"); } self.current_outbound_substream_id.0 += 1; diff --git a/beacon_node/lighthouse_network/src/rpc/mod.rs b/beacon_node/lighthouse_network/src/rpc/mod.rs index f5085e798c..1156447d56 100644 --- a/beacon_node/lighthouse_network/src/rpc/mod.rs +++ b/beacon_node/lighthouse_network/src/rpc/mod.rs @@ -16,7 +16,6 @@ use libp2p::PeerId; use logging::crit; use rate_limiter::{RPCRateLimiter as RateLimiter, RateLimitedErr}; use std::marker::PhantomData; -use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; use std::task::{Context, Poll}; use std::time::Duration; @@ -49,8 +48,6 @@ mod protocol; mod rate_limiter; mod self_limiter; -static NEXT_REQUEST_ID: AtomicUsize = AtomicUsize::new(1); - /// Composite trait for a request id. pub trait ReqId: Send + 'static + std::fmt::Debug + Copy + Clone {} impl ReqId for T where T: Send + 'static + std::fmt::Debug + Copy + Clone {} @@ -80,7 +77,7 @@ pub enum RPCReceived { /// /// The `SubstreamId` is given by the `RPCHandler` as it identifies this request with the /// *inbound* substream over which it is managed. - Request(Request), + Request(InboundRequestId, RequestType), /// A response received from the outside. /// /// The `Id` corresponds to the application given ID of the original request sent to the @@ -91,35 +88,30 @@ pub enum RPCReceived { EndOfStream(Id, ResponseTermination), } -/// Rpc `Request` identifier. -#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] -pub struct RequestId(usize); +// An identifier for the inbound requests received via Rpc. +#[derive(Debug, Hash, PartialEq, Eq, Clone, Copy)] +pub struct InboundRequestId { + /// The connection ID of the peer that sent the request. + connection_id: ConnectionId, + /// The ID of the substream that sent the request. + substream_id: SubstreamId, +} -impl RequestId { - /// Returns the next available [`RequestId`]. - pub fn next() -> Self { - Self(NEXT_REQUEST_ID.fetch_add(1, Ordering::SeqCst)) - } - - /// Creates an _unchecked_ [`RequestId`]. +impl InboundRequestId { + /// Creates an _unchecked_ [`InboundRequestId`]. /// - /// [`Rpc`] enforces that [`RequestId`]s are unique and not reused. + /// [`Rpc`] enforces that [`InboundRequestId`]s are unique and not reused. /// This constructor does not, hence the _unchecked_. /// /// It is primarily meant for allowing manual tests. - pub fn new_unchecked(id: usize) -> Self { - Self(id) + pub fn new_unchecked(connection_id: usize, substream_id: usize) -> Self { + Self { + connection_id: ConnectionId::new_unchecked(connection_id), + substream_id: SubstreamId::new(substream_id), + } } } -/// An Rpc Request. -#[derive(Debug, Clone)] -pub struct Request { - pub id: RequestId, - pub substream_id: SubstreamId, - pub r#type: RequestType, -} - impl std::fmt::Display for RPCSend { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -136,7 +128,7 @@ pub struct RPCMessage { /// The peer that sent the message. pub peer_id: PeerId, /// Handler managing this message. - pub conn_id: ConnectionId, + pub connection_id: ConnectionId, /// The message that was sent. pub message: Result, HandlerErr>, } @@ -215,14 +207,13 @@ impl RPC { pub fn send_response( &mut self, peer_id: PeerId, - id: (ConnectionId, SubstreamId), - _request_id: RequestId, - event: RpcResponse, + request_id: InboundRequestId, + response: RpcResponse, ) { self.events.push(ToSwarm::NotifyHandler { peer_id, - handler: NotifyHandler::One(id.0), - event: RPCSend::Response(id.1, event), + handler: NotifyHandler::One(request_id.connection_id), + event: RPCSend::Response(request_id.substream_id, response), }); } @@ -387,7 +378,7 @@ where for (id, proto) in limiter.peer_disconnected(peer_id) { let error_msg = ToSwarm::GenerateEvent(RPCMessage { peer_id, - conn_id: connection_id, + connection_id, message: Err(HandlerErr::Outbound { id, proto, @@ -408,7 +399,7 @@ where } if *p == peer_id => { *event = ToSwarm::GenerateEvent(RPCMessage { peer_id, - conn_id: connection_id, + connection_id, message: Err(HandlerErr::Outbound { id: *request_id, proto: req.versioned_protocol().protocol(), @@ -424,21 +415,17 @@ where fn on_connection_handler_event( &mut self, peer_id: PeerId, - conn_id: ConnectionId, + connection_id: ConnectionId, event: ::ToBehaviour, ) { match event { - HandlerEvent::Ok(RPCReceived::Request(Request { - id, - substream_id, - r#type, - })) => { + HandlerEvent::Ok(RPCReceived::Request(request_id, request_type)) => { if let Some(limiter) = self.limiter.as_mut() { // check if the request is conformant to the quota - match limiter.allows(&peer_id, &r#type) { + match limiter.allows(&peer_id, &request_type) { Err(RateLimitedErr::TooLarge) => { // we set the batch sizes, so this is a coding/config err for most protocols - let protocol = r#type.versioned_protocol().protocol(); + let protocol = request_type.versioned_protocol().protocol(); if matches!( protocol, Protocol::BlocksByRange @@ -448,7 +435,7 @@ where | Protocol::BlobsByRoot | Protocol::DataColumnsByRoot ) { - debug!(request = %r#type, %protocol, "Request too large to process"); + debug!(request = %request_type, %protocol, "Request too large to process"); } else { // Other protocols shouldn't be sending large messages, we should flag the peer kind crit!(%protocol, "Request size too large to ever be processed"); @@ -457,8 +444,7 @@ where // the handler upon receiving the error code will send it back to the behaviour self.send_response( peer_id, - (conn_id, substream_id), - id, + request_id, RpcResponse::Error( RpcErrorResponse::RateLimited, "Rate limited. Request too large".into(), @@ -467,13 +453,12 @@ where return; } Err(RateLimitedErr::TooSoon(wait_time)) => { - debug!(request = %r#type, %peer_id, wait_time_ms = wait_time.as_millis(), "Request exceeds the rate limit"); + debug!(request = %request_type, %peer_id, wait_time_ms = wait_time.as_millis(), "Request exceeds the rate limit"); // send an error code to the peer. // the handler upon receiving the error code will send it back to the behaviour self.send_response( peer_id, - (conn_id, substream_id), - id, + request_id, RpcResponse::Error( RpcErrorResponse::RateLimited, format!("Wait {:?}", wait_time).into(), @@ -487,12 +472,11 @@ where } // If we received a Ping, we queue a Pong response. - if let RequestType::Ping(_) = r#type { - trace!(connection_id = %conn_id, %peer_id, "Received Ping, queueing Pong"); + if let RequestType::Ping(_) = request_type { + trace!(connection_id = %connection_id, %peer_id, "Received Ping, queueing Pong"); self.send_response( peer_id, - (conn_id, substream_id), - id, + request_id, RpcResponse::Success(RpcSuccessResponse::Pong(Ping { data: self.seq_number, })), @@ -501,25 +485,21 @@ where self.events.push(ToSwarm::GenerateEvent(RPCMessage { peer_id, - conn_id, - message: Ok(RPCReceived::Request(Request { - id, - substream_id, - r#type, - })), + connection_id, + message: Ok(RPCReceived::Request(request_id, request_type)), })); } HandlerEvent::Ok(rpc) => { self.events.push(ToSwarm::GenerateEvent(RPCMessage { peer_id, - conn_id, + connection_id, message: Ok(rpc), })); } HandlerEvent::Err(err) => { self.events.push(ToSwarm::GenerateEvent(RPCMessage { peer_id, - conn_id, + connection_id, message: Err(err), })); } diff --git a/beacon_node/lighthouse_network/src/rpc/self_limiter.rs b/beacon_node/lighthouse_network/src/rpc/self_limiter.rs index af6ac37d2c..e4af977a6c 100644 --- a/beacon_node/lighthouse_network/src/rpc/self_limiter.rs +++ b/beacon_node/lighthouse_network/src/rpc/self_limiter.rs @@ -207,7 +207,7 @@ mod tests { use crate::rpc::rate_limiter::Quota; use crate::rpc::self_limiter::SelfRateLimiter; use crate::rpc::{Ping, Protocol, RequestType}; - use crate::service::api_types::{AppRequestId, RequestId, SingleLookupReqId, SyncRequestId}; + use crate::service::api_types::{AppRequestId, SingleLookupReqId, SyncRequestId}; use libp2p::PeerId; use logging::create_test_tracing_subscriber; use std::time::Duration; @@ -226,7 +226,7 @@ mod tests { Hash256::ZERO, &MainnetEthSpec::default_spec(), )); - let mut limiter: SelfRateLimiter = + let mut limiter: SelfRateLimiter = SelfRateLimiter::new(config, fork_context).unwrap(); let peer_id = PeerId::random(); let lookup_id = 0; @@ -234,12 +234,12 @@ mod tests { for i in 1..=5u32 { let _ = limiter.allows( peer_id, - RequestId::Application(AppRequestId::Sync(SyncRequestId::SingleBlock { + AppRequestId::Sync(SyncRequestId::SingleBlock { id: SingleLookupReqId { lookup_id, req_id: i, }, - })), + }), RequestType::Ping(Ping { data: i as u64 }), ); } @@ -256,9 +256,9 @@ mod tests { for i in 2..=5u32 { assert!(matches!( iter.next().unwrap().request_id, - RequestId::Application(AppRequestId::Sync(SyncRequestId::SingleBlock { + AppRequestId::Sync(SyncRequestId::SingleBlock { id: SingleLookupReqId { req_id, .. }, - })) if req_id == i, + }) if req_id == i, )); } @@ -281,9 +281,9 @@ mod tests { for i in 3..=5 { assert!(matches!( iter.next().unwrap().request_id, - RequestId::Application(AppRequestId::Sync(SyncRequestId::SingleBlock { + AppRequestId::Sync(SyncRequestId::SingleBlock { id: SingleLookupReqId { req_id, .. }, - })) if req_id == i, + }) if req_id == i, )); } diff --git a/beacon_node/lighthouse_network/src/service/api_types.rs b/beacon_node/lighthouse_network/src/service/api_types.rs index 894fff5074..b36f8cc215 100644 --- a/beacon_node/lighthouse_network/src/service/api_types.rs +++ b/beacon_node/lighthouse_network/src/service/api_types.rs @@ -1,8 +1,4 @@ -use crate::rpc::{ - methods::{ResponseTermination, RpcResponse, RpcSuccessResponse, StatusMessage}, - SubstreamId, -}; -use libp2p::swarm::ConnectionId; +use crate::rpc::methods::{ResponseTermination, RpcResponse, RpcSuccessResponse, StatusMessage}; use std::fmt::{Display, Formatter}; use std::sync::Arc; use types::{ @@ -10,9 +6,6 @@ use types::{ LightClientFinalityUpdate, LightClientOptimisticUpdate, LightClientUpdate, SignedBeaconBlock, }; -/// Identifier of requests sent by a peer. -pub type PeerRequestId = (ConnectionId, SubstreamId); - pub type Id = u32; #[derive(Debug, Hash, PartialEq, Eq, Clone, Copy)] @@ -130,12 +123,6 @@ pub struct CustodyRequester(pub SingleLookupReqId); pub enum AppRequestId { Sync(SyncRequestId), Router, -} - -/// Global identifier of a request. -#[derive(Debug, Clone, Copy)] -pub enum RequestId { - Application(AppRequestId), Internal, } diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs index 9650976c63..bc9f2011f8 100644 --- a/beacon_node/lighthouse_network/src/service/mod.rs +++ b/beacon_node/lighthouse_network/src/service/mod.rs @@ -10,8 +10,9 @@ use crate::peer_manager::{ use crate::peer_manager::{MIN_OUTBOUND_ONLY_FACTOR, PEER_EXCESS_FACTOR, PRIORITY_PEER_EXCESS}; use crate::rpc::methods::MetadataRequest; use crate::rpc::{ - self, GoodbyeReason, HandlerErr, NetworkParams, Protocol, RPCError, RPCMessage, RPCReceived, - RequestType, ResponseTermination, RpcErrorResponse, RpcResponse, RpcSuccessResponse, RPC, + GoodbyeReason, HandlerErr, InboundRequestId, NetworkParams, Protocol, RPCError, RPCMessage, + RPCReceived, RequestType, ResponseTermination, RpcErrorResponse, RpcResponse, + RpcSuccessResponse, RPC, }; use crate::types::{ all_topics_at_fork, core_topics_to_subscribe, is_fork_non_core_topic, subnet_from_topic_hash, @@ -20,7 +21,7 @@ use crate::types::{ use crate::EnrExt; use crate::Eth2Enr; use crate::{metrics, Enr, NetworkGlobals, PubsubMessage, TopicHash}; -use api_types::{AppRequestId, PeerRequestId, RequestId, Response}; +use api_types::{AppRequestId, Response}; use futures::stream::StreamExt; use gossipsub::{ IdentTopic as Topic, MessageAcceptance, MessageAuthenticity, MessageId, PublishError, @@ -66,7 +67,7 @@ pub enum NetworkEvent { /// An RPC Request that was sent failed. RPCFailed { /// The id of the failed request. - id: AppRequestId, + app_request_id: AppRequestId, /// The peer to which this request was sent. peer_id: PeerId, /// The error of the failed request. @@ -76,15 +77,15 @@ pub enum NetworkEvent { /// The peer that sent the request. peer_id: PeerId, /// Identifier of the request. All responses to this request must use this id. - id: PeerRequestId, + inbound_request_id: InboundRequestId, /// Request the peer sent. - request: rpc::Request, + request_type: RequestType, }, ResponseReceived { /// Peer that sent the response. peer_id: PeerId, /// Id of the request to which the peer is responding. - id: AppRequestId, + app_request_id: AppRequestId, /// Response the peer sent. response: Response, }, @@ -126,7 +127,7 @@ where /// The peer manager that keeps track of peer's reputation and status. pub peer_manager: PeerManager, /// The Eth2 RPC specified in the wire-0 protocol. - pub eth2_rpc: RPC, + pub eth2_rpc: RPC, /// Discv5 Discovery protocol. pub discovery: Discovery, /// Keep regular connection to peers and disconnect if absent. @@ -669,7 +670,7 @@ impl Network { name = "libp2p", skip_all )] - pub fn eth2_rpc_mut(&mut self) -> &mut RPC { + pub fn eth2_rpc_mut(&mut self) -> &mut RPC { &mut self.swarm.behaviour_mut().eth2_rpc } /// Discv5 Discovery protocol. @@ -720,7 +721,7 @@ impl Network { name = "libp2p", skip_all )] - pub fn eth2_rpc(&self) -> &RPC { + pub fn eth2_rpc(&self) -> &RPC { &self.swarm.behaviour().eth2_rpc } /// Discv5 Discovery protocol. @@ -1104,16 +1105,16 @@ impl Network { pub fn send_request( &mut self, peer_id: PeerId, - request_id: AppRequestId, + app_request_id: AppRequestId, request: RequestType, ) -> Result<(), (AppRequestId, RPCError)> { // Check if the peer is connected before sending an RPC request if !self.swarm.is_connected(&peer_id) { - return Err((request_id, RPCError::Disconnected)); + return Err((app_request_id, RPCError::Disconnected)); } self.eth2_rpc_mut() - .send_request(peer_id, RequestId::Application(request_id), request); + .send_request(peer_id, app_request_id, request); Ok(()) } @@ -1127,12 +1128,11 @@ impl Network { pub fn send_response( &mut self, peer_id: PeerId, - id: PeerRequestId, - request_id: rpc::RequestId, + inbound_request_id: InboundRequestId, response: Response, ) { self.eth2_rpc_mut() - .send_response(peer_id, id, request_id, response.into()) + .send_response(peer_id, inbound_request_id, response.into()) } /// Inform the peer that their request produced an error. @@ -1145,15 +1145,13 @@ impl Network { pub fn send_error_response( &mut self, peer_id: PeerId, - id: PeerRequestId, - request_id: rpc::RequestId, + inbound_request_id: InboundRequestId, error: RpcErrorResponse, reason: String, ) { self.eth2_rpc_mut().send_response( peer_id, - id, - request_id, + inbound_request_id, RpcResponse::Error(error, reason.into()), ) } @@ -1374,7 +1372,7 @@ impl Network { skip_all )] fn ping(&mut self, peer_id: PeerId) { - self.eth2_rpc_mut().ping(peer_id, RequestId::Internal); + self.eth2_rpc_mut().ping(peer_id, AppRequestId::Internal); } /// Sends a METADATA request to a peer. @@ -1394,7 +1392,7 @@ impl Network { RequestType::MetaData(MetadataRequest::new_v2()) }; self.eth2_rpc_mut() - .send_request(peer_id, RequestId::Internal, event); + .send_request(peer_id, AppRequestId::Internal, event); } /// Sends a METADATA response to a peer. @@ -1407,15 +1405,14 @@ impl Network { fn send_meta_data_response( &mut self, _req: MetadataRequest, - id: PeerRequestId, - request_id: rpc::RequestId, + inbound_request_id: InboundRequestId, peer_id: PeerId, ) { let metadata = self.network_globals.local_metadata.read().clone(); // The encoder is responsible for sending the negotiated version of the metadata let event = RpcResponse::Success(RpcSuccessResponse::MetaData(Arc::new(metadata))); self.eth2_rpc_mut() - .send_response(peer_id, id, request_id, event); + .send_response(peer_id, inbound_request_id, event); } // RPC Propagation methods @@ -1429,17 +1426,17 @@ impl Network { )] fn build_response( &mut self, - id: RequestId, + app_request_id: AppRequestId, peer_id: PeerId, response: Response, ) -> Option> { - match id { - RequestId::Application(id) => Some(NetworkEvent::ResponseReceived { + match app_request_id { + AppRequestId::Internal => None, + _ => Some(NetworkEvent::ResponseReceived { peer_id, - id, + app_request_id, response, }), - RequestId::Internal => None, } } @@ -1643,7 +1640,7 @@ impl Network { name = "libp2p", skip_all )] - fn inject_rpc_event(&mut self, event: RPCMessage) -> Option> { + fn inject_rpc_event(&mut self, event: RPCMessage) -> Option> { let peer_id = event.peer_id; // Do not permit Inbound events from peers that are being disconnected or RPC requests, @@ -1656,7 +1653,6 @@ impl Network { return None; } - let connection_id = event.conn_id; // The METADATA and PING RPC responses are handled within the behaviour and not propagated match event.message { Err(handler_err) => { @@ -1686,16 +1682,20 @@ impl Network { ConnectionDirection::Outgoing, ); // inform failures of requests coming outside the behaviour - if let RequestId::Application(id) = id { - Some(NetworkEvent::RPCFailed { peer_id, id, error }) - } else { + if let AppRequestId::Internal = id { None + } else { + Some(NetworkEvent::RPCFailed { + peer_id, + app_request_id: id, + error, + }) } } } } - Ok(RPCReceived::Request(request)) => { - match request.r#type { + Ok(RPCReceived::Request(inbound_request_id, request_type)) => { + match request_type { /* Behaviour managed protocols: Ping and Metadata */ RequestType::Ping(ping) => { // inform the peer manager and send the response @@ -1704,12 +1704,7 @@ impl Network { } RequestType::MetaData(req) => { // send the requested meta-data - self.send_meta_data_response( - req, - (connection_id, request.substream_id), - request.id, - peer_id, - ); + self.send_meta_data_response(req, inbound_request_id, peer_id); None } RequestType::Goodbye(reason) => { @@ -1734,8 +1729,8 @@ impl Network { // propagate the STATUS message upwards Some(NetworkEvent::RequestReceived { peer_id, - id: (connection_id, request.substream_id), - request, + inbound_request_id, + request_type, }) } RequestType::BlocksByRange(ref req) => { @@ -1757,32 +1752,32 @@ impl Network { ); Some(NetworkEvent::RequestReceived { peer_id, - id: (connection_id, request.substream_id), - request, + inbound_request_id, + request_type, }) } RequestType::BlocksByRoot(_) => { metrics::inc_counter_vec(&metrics::TOTAL_RPC_REQUESTS, &["blocks_by_root"]); Some(NetworkEvent::RequestReceived { peer_id, - id: (connection_id, request.substream_id), - request, + inbound_request_id, + request_type, }) } RequestType::BlobsByRange(_) => { metrics::inc_counter_vec(&metrics::TOTAL_RPC_REQUESTS, &["blobs_by_range"]); Some(NetworkEvent::RequestReceived { peer_id, - id: (connection_id, request.substream_id), - request, + inbound_request_id, + request_type, }) } RequestType::BlobsByRoot(_) => { metrics::inc_counter_vec(&metrics::TOTAL_RPC_REQUESTS, &["blobs_by_root"]); Some(NetworkEvent::RequestReceived { peer_id, - id: (connection_id, request.substream_id), - request, + inbound_request_id, + request_type, }) } RequestType::DataColumnsByRoot(_) => { @@ -1792,8 +1787,8 @@ impl Network { ); Some(NetworkEvent::RequestReceived { peer_id, - id: (connection_id, request.substream_id), - request, + inbound_request_id, + request_type, }) } RequestType::DataColumnsByRange(_) => { @@ -1803,8 +1798,8 @@ impl Network { ); Some(NetworkEvent::RequestReceived { peer_id, - id: (connection_id, request.substream_id), - request, + inbound_request_id, + request_type, }) } RequestType::LightClientBootstrap(_) => { @@ -1814,8 +1809,8 @@ impl Network { ); Some(NetworkEvent::RequestReceived { peer_id, - id: (connection_id, request.substream_id), - request, + inbound_request_id, + request_type, }) } RequestType::LightClientOptimisticUpdate => { @@ -1825,8 +1820,8 @@ impl Network { ); Some(NetworkEvent::RequestReceived { peer_id, - id: (connection_id, request.substream_id), - request, + inbound_request_id, + request_type, }) } RequestType::LightClientFinalityUpdate => { @@ -1836,8 +1831,8 @@ impl Network { ); Some(NetworkEvent::RequestReceived { peer_id, - id: (connection_id, request.substream_id), - request, + inbound_request_id, + request_type, }) } RequestType::LightClientUpdatesByRange(_) => { @@ -1847,8 +1842,8 @@ impl Network { ); Some(NetworkEvent::RequestReceived { peer_id, - id: (connection_id, request.substream_id), - request, + inbound_request_id, + request_type, }) } } @@ -2010,7 +2005,7 @@ impl Network { debug!(%peer_id, %reason, "Peer Manager disconnecting peer"); // send one goodbye self.eth2_rpc_mut() - .shutdown(peer_id, RequestId::Internal, reason); + .shutdown(peer_id, AppRequestId::Internal, reason); None } } diff --git a/beacon_node/lighthouse_network/tests/rpc_tests.rs b/beacon_node/lighthouse_network/tests/rpc_tests.rs index d736fefa5f..aedd507751 100644 --- a/beacon_node/lighthouse_network/tests/rpc_tests.rs +++ b/beacon_node/lighthouse_network/tests/rpc_tests.rs @@ -98,7 +98,7 @@ fn test_tcp_status_rpc() { } NetworkEvent::ResponseReceived { peer_id: _, - id: AppRequestId::Router, + app_request_id: AppRequestId::Router, response, } => { // Should receive the RPC response @@ -118,13 +118,17 @@ fn test_tcp_status_rpc() { match receiver.next_event().await { NetworkEvent::RequestReceived { peer_id, - id, - request, + inbound_request_id, + request_type, } => { - if request.r#type == rpc_request { + if request_type == rpc_request { // send the response debug!("Receiver Received"); - receiver.send_response(peer_id, id, request.id, rpc_response.clone()); + receiver.send_response( + peer_id, + inbound_request_id, + rpc_response.clone(), + ); } } _ => {} // Ignore other events @@ -204,7 +208,7 @@ fn test_tcp_blocks_by_range_chunked_rpc() { } NetworkEvent::ResponseReceived { peer_id: _, - id: _, + app_request_id: _, response, } => { warn!("Sender received a response"); @@ -240,10 +244,10 @@ fn test_tcp_blocks_by_range_chunked_rpc() { match receiver.next_event().await { NetworkEvent::RequestReceived { peer_id, - id, - request, + inbound_request_id, + request_type, } => { - if request.r#type == rpc_request { + if request_type == rpc_request { // send the response warn!("Receiver got request"); for i in 0..messages_to_send { @@ -258,16 +262,14 @@ fn test_tcp_blocks_by_range_chunked_rpc() { }; receiver.send_response( peer_id, - id, - request.id, + inbound_request_id, rpc_response.clone(), ); } // send the stream termination receiver.send_response( peer_id, - id, - request.id, + inbound_request_id, Response::BlocksByRange(None), ); } @@ -338,7 +340,7 @@ fn test_blobs_by_range_chunked_rpc() { } NetworkEvent::ResponseReceived { peer_id: _, - id: _, + app_request_id: _, response, } => { warn!("Sender received a response"); @@ -368,10 +370,10 @@ fn test_blobs_by_range_chunked_rpc() { match receiver.next_event().await { NetworkEvent::RequestReceived { peer_id, - id, - request, + inbound_request_id, + request_type, } => { - if request.r#type == rpc_request { + if request_type == rpc_request { // send the response warn!("Receiver got request"); for _ in 0..messages_to_send { @@ -379,16 +381,14 @@ fn test_blobs_by_range_chunked_rpc() { // second as altair and third as bellatrix. receiver.send_response( peer_id, - id, - request.id, + inbound_request_id, rpc_response.clone(), ); } // send the stream termination receiver.send_response( peer_id, - id, - request.id, + inbound_request_id, Response::BlobsByRange(None), ); } @@ -459,8 +459,8 @@ fn test_tcp_blocks_by_range_over_limit() { .unwrap(); } // The request will fail because the sender will refuse to send anything > MAX_RPC_SIZE - NetworkEvent::RPCFailed { id, .. } => { - assert!(matches!(id, AppRequestId::Router)); + NetworkEvent::RPCFailed { app_request_id, .. } => { + assert!(matches!(app_request_id, AppRequestId::Router)); return; } _ => {} // Ignore other behaviour events @@ -474,26 +474,24 @@ fn test_tcp_blocks_by_range_over_limit() { match receiver.next_event().await { NetworkEvent::RequestReceived { peer_id, - id, - request, + inbound_request_id, + request_type, } => { - if request.r#type == rpc_request { + if request_type == rpc_request { // send the response warn!("Receiver got request"); for _ in 0..messages_to_send { let rpc_response = rpc_response_bellatrix_large.clone(); receiver.send_response( peer_id, - id, - request.id, + inbound_request_id, rpc_response.clone(), ); } // send the stream termination receiver.send_response( peer_id, - id, - request.id, + inbound_request_id, Response::BlocksByRange(None), ); } @@ -566,7 +564,7 @@ fn test_tcp_blocks_by_range_chunked_rpc_terminates_correctly() { } NetworkEvent::ResponseReceived { peer_id: _, - id: _, + app_request_id: _, response, } => // Should receive the RPC response @@ -608,15 +606,15 @@ fn test_tcp_blocks_by_range_chunked_rpc_terminates_correctly() { futures::future::Either::Left(( NetworkEvent::RequestReceived { peer_id, - id, - request, + inbound_request_id, + request_type, }, _, )) => { - if request.r#type == rpc_request { + if request_type == rpc_request { // send the response warn!("Receiver got request"); - message_info = Some((peer_id, id, request.id)); + message_info = Some((peer_id, inbound_request_id)); } } futures::future::Either::Right((_, _)) => {} // The timeout hit, send messages if required @@ -626,8 +624,8 @@ fn test_tcp_blocks_by_range_chunked_rpc_terminates_correctly() { // if we need to send messages send them here. This will happen after a delay if message_info.is_some() { messages_sent += 1; - let (peer_id, stream_id, request_id) = message_info.as_ref().unwrap(); - receiver.send_response(*peer_id, *stream_id, *request_id, rpc_response.clone()); + let (peer_id, inbound_request_id) = message_info.as_ref().unwrap(); + receiver.send_response(*peer_id, *inbound_request_id, rpc_response.clone()); debug!("Sending message {}", messages_sent); if messages_sent == messages_to_send + extra_messages_to_send { // stop sending messages @@ -700,7 +698,7 @@ fn test_tcp_blocks_by_range_single_empty_rpc() { } NetworkEvent::ResponseReceived { peer_id: _, - id: AppRequestId::Router, + app_request_id: AppRequestId::Router, response, } => match response { Response::BlocksByRange(Some(_)) => { @@ -727,26 +725,24 @@ fn test_tcp_blocks_by_range_single_empty_rpc() { match receiver.next_event().await { NetworkEvent::RequestReceived { peer_id, - id, - request, + inbound_request_id, + request_type, } => { - if request.r#type == rpc_request { + if request_type == rpc_request { // send the response warn!("Receiver got request"); for _ in 1..=messages_to_send { receiver.send_response( peer_id, - id, - request.id, + inbound_request_id, rpc_response.clone(), ); } // send the stream termination receiver.send_response( peer_id, - id, - request.id, + inbound_request_id, Response::BlocksByRange(None), ); } @@ -837,7 +833,7 @@ fn test_tcp_blocks_by_root_chunked_rpc() { } NetworkEvent::ResponseReceived { peer_id: _, - id: AppRequestId::Router, + app_request_id: AppRequestId::Router, response, } => match response { Response::BlocksByRoot(Some(_)) => { @@ -870,10 +866,10 @@ fn test_tcp_blocks_by_root_chunked_rpc() { match receiver.next_event().await { NetworkEvent::RequestReceived { peer_id, - id, - request, + inbound_request_id, + request_type, } => { - if request.r#type == rpc_request { + if request_type == rpc_request { // send the response debug!("Receiver got request"); @@ -886,14 +882,13 @@ fn test_tcp_blocks_by_root_chunked_rpc() { } else { rpc_response_bellatrix_small.clone() }; - receiver.send_response(peer_id, id, request.id, rpc_response); + receiver.send_response(peer_id, inbound_request_id, rpc_response); debug!("Sending message"); } // send the stream termination receiver.send_response( peer_id, - id, - request.id, + inbound_request_id, Response::BlocksByRange(None), ); debug!("Send stream term"); @@ -977,7 +972,7 @@ fn test_tcp_blocks_by_root_chunked_rpc_terminates_correctly() { } NetworkEvent::ResponseReceived { peer_id: _, - id: AppRequestId::Router, + app_request_id: AppRequestId::Router, response, } => { debug!("Sender received a response"); @@ -1019,15 +1014,15 @@ fn test_tcp_blocks_by_root_chunked_rpc_terminates_correctly() { futures::future::Either::Left(( NetworkEvent::RequestReceived { peer_id, - id, - request, + inbound_request_id, + request_type, }, _, )) => { - if request.r#type == rpc_request { + if request_type == rpc_request { // send the response warn!("Receiver got request"); - message_info = Some((peer_id, id, request.id)); + message_info = Some((peer_id, inbound_request_id)); } } futures::future::Either::Right((_, _)) => {} // The timeout hit, send messages if required @@ -1037,8 +1032,8 @@ fn test_tcp_blocks_by_root_chunked_rpc_terminates_correctly() { // if we need to send messages send them here. This will happen after a delay if message_info.is_some() { messages_sent += 1; - let (peer_id, stream_id, request_id) = message_info.as_ref().unwrap(); - receiver.send_response(*peer_id, *stream_id, *request_id, rpc_response.clone()); + let (peer_id, inbound_request_id) = message_info.as_ref().unwrap(); + receiver.send_response(*peer_id, *inbound_request_id, rpc_response.clone()); debug!("Sending message {}", messages_sent); if messages_sent == messages_to_send + extra_messages_to_send { // stop sending messages diff --git a/beacon_node/network/src/network_beacon_processor/mod.rs b/beacon_node/network/src/network_beacon_processor/mod.rs index 1329936932..3431c1abb9 100644 --- a/beacon_node/network/src/network_beacon_processor/mod.rs +++ b/beacon_node/network/src/network_beacon_processor/mod.rs @@ -15,12 +15,11 @@ use beacon_processor::{ work_reprocessing_queue::ReprocessQueueMessage, BeaconProcessorSend, DuplicateCache, GossipAggregatePackage, GossipAttestationPackage, Work, WorkEvent as BeaconWorkEvent, }; -use lighthouse_network::discovery::ConnectionId; use lighthouse_network::rpc::methods::{ BlobsByRangeRequest, BlobsByRootRequest, DataColumnsByRangeRequest, DataColumnsByRootRequest, LightClientUpdatesByRangeRequest, }; -use lighthouse_network::rpc::{RequestId, SubstreamId}; +use lighthouse_network::rpc::InboundRequestId; use lighthouse_network::{ rpc::{BlocksByRangeRequest, BlocksByRootRequest, LightClientBootstrapRequest, StatusMessage}, Client, MessageId, NetworkGlobals, PeerId, PubsubMessage, @@ -647,21 +646,13 @@ impl NetworkBeaconProcessor { pub fn send_blocks_by_range_request( self: &Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, // Use ResponseId here request: BlocksByRangeRequest, ) -> Result<(), Error> { let processor = self.clone(); let process_fn = async move { processor - .handle_blocks_by_range_request( - peer_id, - connection_id, - substream_id, - request_id, - request, - ) + .handle_blocks_by_range_request(peer_id, inbound_request_id, request) .await; }; @@ -675,21 +666,13 @@ impl NetworkBeaconProcessor { pub fn send_blocks_by_roots_request( self: &Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, // Use ResponseId here request: BlocksByRootRequest, ) -> Result<(), Error> { let processor = self.clone(); let process_fn = async move { processor - .handle_blocks_by_root_request( - peer_id, - connection_id, - substream_id, - request_id, - request, - ) + .handle_blocks_by_root_request(peer_id, inbound_request_id, request) .await; }; @@ -703,21 +686,12 @@ impl NetworkBeaconProcessor { pub fn send_blobs_by_range_request( self: &Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, request: BlobsByRangeRequest, ) -> Result<(), Error> { let processor = self.clone(); - let process_fn = move || { - processor.handle_blobs_by_range_request( - peer_id, - connection_id, - substream_id, - request_id, - request, - ) - }; + let process_fn = + move || processor.handle_blobs_by_range_request(peer_id, inbound_request_id, request); self.try_send(BeaconWorkEvent { drop_during_sync: false, @@ -729,21 +703,12 @@ impl NetworkBeaconProcessor { pub fn send_blobs_by_roots_request( self: &Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, request: BlobsByRootRequest, ) -> Result<(), Error> { let processor = self.clone(); - let process_fn = move || { - processor.handle_blobs_by_root_request( - peer_id, - connection_id, - substream_id, - request_id, - request, - ) - }; + let process_fn = + move || processor.handle_blobs_by_root_request(peer_id, inbound_request_id, request); self.try_send(BeaconWorkEvent { drop_during_sync: false, @@ -755,20 +720,12 @@ impl NetworkBeaconProcessor { pub fn send_data_columns_by_roots_request( self: &Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, request: DataColumnsByRootRequest, ) -> Result<(), Error> { let processor = self.clone(); let process_fn = move || { - processor.handle_data_columns_by_root_request( - peer_id, - connection_id, - substream_id, - request_id, - request, - ) + processor.handle_data_columns_by_root_request(peer_id, inbound_request_id, request) }; self.try_send(BeaconWorkEvent { @@ -781,20 +738,12 @@ impl NetworkBeaconProcessor { pub fn send_data_columns_by_range_request( self: &Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, request: DataColumnsByRangeRequest, ) -> Result<(), Error> { let processor = self.clone(); let process_fn = move || { - processor.handle_data_columns_by_range_request( - peer_id, - connection_id, - substream_id, - request_id, - request, - ) + processor.handle_data_columns_by_range_request(peer_id, inbound_request_id, request) }; self.try_send(BeaconWorkEvent { @@ -807,21 +756,12 @@ impl NetworkBeaconProcessor { pub fn send_light_client_bootstrap_request( self: &Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, request: LightClientBootstrapRequest, ) -> Result<(), Error> { let processor = self.clone(); - let process_fn = move || { - processor.handle_light_client_bootstrap( - peer_id, - connection_id, - substream_id, - request_id, - request, - ) - }; + let process_fn = + move || processor.handle_light_client_bootstrap(peer_id, inbound_request_id, request); self.try_send(BeaconWorkEvent { drop_during_sync: true, @@ -833,19 +773,11 @@ impl NetworkBeaconProcessor { pub fn send_light_client_optimistic_update_request( self: &Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, ) -> Result<(), Error> { let processor = self.clone(); - let process_fn = move || { - processor.handle_light_client_optimistic_update( - peer_id, - connection_id, - substream_id, - request_id, - ) - }; + let process_fn = + move || processor.handle_light_client_optimistic_update(peer_id, inbound_request_id); self.try_send(BeaconWorkEvent { drop_during_sync: true, @@ -857,19 +789,11 @@ impl NetworkBeaconProcessor { pub fn send_light_client_finality_update_request( self: &Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, ) -> Result<(), Error> { let processor = self.clone(); - let process_fn = move || { - processor.handle_light_client_finality_update( - peer_id, - connection_id, - substream_id, - request_id, - ) - }; + let process_fn = + move || processor.handle_light_client_finality_update(peer_id, inbound_request_id); self.try_send(BeaconWorkEvent { drop_during_sync: true, @@ -881,20 +805,12 @@ impl NetworkBeaconProcessor { pub fn send_light_client_updates_by_range_request( self: &Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, request: LightClientUpdatesByRangeRequest, ) -> Result<(), Error> { let processor = self.clone(); let process_fn = move || { - processor.handle_light_client_updates_by_range( - peer_id, - connection_id, - substream_id, - request_id, - request, - ) + processor.handle_light_client_updates_by_range(peer_id, inbound_request_id, request) }; self.try_send(BeaconWorkEvent { diff --git a/beacon_node/network/src/network_beacon_processor/rpc_methods.rs b/beacon_node/network/src/network_beacon_processor/rpc_methods.rs index 7beadffc06..4694c926c9 100644 --- a/beacon_node/network/src/network_beacon_processor/rpc_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/rpc_methods.rs @@ -4,12 +4,11 @@ use crate::status::ToStatusMessage; use crate::sync::SyncMessage; use beacon_chain::{BeaconChainError, BeaconChainTypes, WhenSlotSkipped}; use itertools::process_results; -use lighthouse_network::discovery::ConnectionId; use lighthouse_network::rpc::methods::{ BlobsByRangeRequest, BlobsByRootRequest, DataColumnsByRangeRequest, DataColumnsByRootRequest, }; use lighthouse_network::rpc::*; -use lighthouse_network::{PeerId, PeerRequestId, ReportSource, Response, SyncInfo}; +use lighthouse_network::{PeerId, ReportSource, Response, SyncInfo}; use methods::LightClientUpdatesByRangeRequest; use slot_clock::SlotClock; use std::collections::{hash_map::Entry, HashMap}; @@ -34,15 +33,12 @@ impl NetworkBeaconProcessor { pub fn send_response( &self, peer_id: PeerId, + inbound_request_id: InboundRequestId, response: Response, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, ) { self.send_network_message(NetworkMessage::SendResponse { peer_id, - request_id, - id: (connection_id, substream_id), + inbound_request_id, response, }) } @@ -52,15 +48,13 @@ impl NetworkBeaconProcessor { peer_id: PeerId, error: RpcErrorResponse, reason: String, - id: PeerRequestId, - request_id: RequestId, + inbound_request_id: InboundRequestId, ) { self.send_network_message(NetworkMessage::SendErrorResponse { peer_id, error, reason, - id, - request_id, + inbound_request_id, }) } @@ -161,24 +155,14 @@ impl NetworkBeaconProcessor { pub async fn handle_blocks_by_root_request( self: Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, request: BlocksByRootRequest, ) { self.terminate_response_stream( peer_id, - connection_id, - substream_id, - request_id, + inbound_request_id, self.clone() - .handle_blocks_by_root_request_inner( - peer_id, - connection_id, - substream_id, - request_id, - request, - ) + .handle_blocks_by_root_request_inner(peer_id, inbound_request_id, request) .await, Response::BlocksByRoot, ); @@ -188,9 +172,7 @@ impl NetworkBeaconProcessor { pub async fn handle_blocks_by_root_request_inner( self: Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, request: BlocksByRootRequest, ) -> Result<(), (RpcErrorResponse, &'static str)> { let log_results = |peer_id, requested_blocks, send_block_count| { @@ -220,10 +202,8 @@ impl NetworkBeaconProcessor { Ok(Some(block)) => { self.send_response( peer_id, + inbound_request_id, Response::BlocksByRoot(Some(block.clone())), - connection_id, - substream_id, - request_id, ); send_block_count += 1; } @@ -265,23 +245,13 @@ impl NetworkBeaconProcessor { pub fn handle_blobs_by_root_request( self: Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, request: BlobsByRootRequest, ) { self.terminate_response_stream( peer_id, - connection_id, - substream_id, - request_id, - self.handle_blobs_by_root_request_inner( - peer_id, - connection_id, - substream_id, - request_id, - request, - ), + inbound_request_id, + self.handle_blobs_by_root_request_inner(peer_id, inbound_request_id, request), Response::BlobsByRoot, ); } @@ -290,9 +260,7 @@ impl NetworkBeaconProcessor { pub fn handle_blobs_by_root_request_inner( &self, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, request: BlobsByRootRequest, ) -> Result<(), (RpcErrorResponse, &'static str)> { let Some(requested_root) = request.blob_ids.as_slice().first().map(|id| id.block_root) @@ -314,10 +282,8 @@ impl NetworkBeaconProcessor { if let Ok(Some(blob)) = self.chain.data_availability_checker.get_blob(id) { self.send_response( peer_id, + inbound_request_id, Response::BlobsByRoot(Some(blob)), - connection_id, - substream_id, - request_id, ); send_blob_count += 1; } else { @@ -339,10 +305,8 @@ impl NetworkBeaconProcessor { if blob_sidecar.index == *index { self.send_response( peer_id, + inbound_request_id, Response::BlobsByRoot(Some(blob_sidecar.clone())), - connection_id, - substream_id, - request_id, ); send_blob_count += 1; break 'inner; @@ -375,23 +339,13 @@ impl NetworkBeaconProcessor { pub fn handle_data_columns_by_root_request( self: Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, request: DataColumnsByRootRequest, ) { self.terminate_response_stream( peer_id, - connection_id, - substream_id, - request_id, - self.handle_data_columns_by_root_request_inner( - peer_id, - connection_id, - substream_id, - request_id, - request, - ), + inbound_request_id, + self.handle_data_columns_by_root_request_inner(peer_id, inbound_request_id, request), Response::DataColumnsByRoot, ); } @@ -400,9 +354,7 @@ impl NetworkBeaconProcessor { pub fn handle_data_columns_by_root_request_inner( &self, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, request: DataColumnsByRootRequest, ) -> Result<(), (RpcErrorResponse, &'static str)> { let mut send_data_column_count = 0; @@ -416,10 +368,8 @@ impl NetworkBeaconProcessor { send_data_column_count += 1; self.send_response( peer_id, + inbound_request_id, Response::DataColumnsByRoot(Some(data_column)), - connection_id, - substream_id, - request_id, ); } Ok(None) => {} // no-op @@ -449,22 +399,16 @@ impl NetworkBeaconProcessor { pub fn handle_light_client_updates_by_range( self: &Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, request: LightClientUpdatesByRangeRequest, ) { self.terminate_response_stream( peer_id, - connection_id, - substream_id, - request_id, + inbound_request_id, self.clone() .handle_light_client_updates_by_range_request_inner( peer_id, - connection_id, - substream_id, - request_id, + inbound_request_id, request, ), Response::LightClientUpdatesByRange, @@ -475,9 +419,7 @@ impl NetworkBeaconProcessor { pub fn handle_light_client_updates_by_range_request_inner( self: Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, req: LightClientUpdatesByRangeRequest, ) -> Result<(), (RpcErrorResponse, &'static str)> { debug!( @@ -516,8 +458,7 @@ impl NetworkBeaconProcessor { self.send_network_message(NetworkMessage::SendResponse { peer_id, response: Response::LightClientUpdatesByRange(Some(Arc::new(lc_update.clone()))), - request_id, - id: (connection_id, substream_id), + inbound_request_id, }); } @@ -549,16 +490,12 @@ impl NetworkBeaconProcessor { pub fn handle_light_client_bootstrap( self: &Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, request: LightClientBootstrapRequest, ) { self.terminate_response_single_item( peer_id, - connection_id, - substream_id, - request_id, + inbound_request_id, match self.chain.get_light_client_bootstrap(&request.root) { Ok(Some((bootstrap, _))) => Ok(Arc::new(bootstrap)), Ok(None) => Err(( @@ -583,15 +520,11 @@ impl NetworkBeaconProcessor { pub fn handle_light_client_optimistic_update( self: &Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, ) { self.terminate_response_single_item( peer_id, - connection_id, - substream_id, - request_id, + inbound_request_id, match self .chain .light_client_server_cache @@ -611,15 +544,11 @@ impl NetworkBeaconProcessor { pub fn handle_light_client_finality_update( self: &Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, ) { self.terminate_response_single_item( peer_id, - connection_id, - substream_id, - request_id, + inbound_request_id, match self .chain .light_client_server_cache @@ -639,24 +568,14 @@ impl NetworkBeaconProcessor { pub async fn handle_blocks_by_range_request( self: Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, req: BlocksByRangeRequest, ) { self.terminate_response_stream( peer_id, - connection_id, - substream_id, - request_id, + inbound_request_id, self.clone() - .handle_blocks_by_range_request_inner( - peer_id, - connection_id, - substream_id, - request_id, - req, - ) + .handle_blocks_by_range_request_inner(peer_id, inbound_request_id, req) .await, Response::BlocksByRange, ); @@ -666,9 +585,7 @@ impl NetworkBeaconProcessor { pub async fn handle_blocks_by_range_request_inner( self: Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, req: BlocksByRangeRequest, ) -> Result<(), (RpcErrorResponse, &'static str)> { debug!( @@ -789,9 +706,8 @@ impl NetworkBeaconProcessor { blocks_sent += 1; self.send_network_message(NetworkMessage::SendResponse { peer_id, - request_id, + inbound_request_id, response: Response::BlocksByRange(Some(block.clone())), - id: (connection_id, substream_id), }); } } @@ -852,23 +768,13 @@ impl NetworkBeaconProcessor { pub fn handle_blobs_by_range_request( self: Arc, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, req: BlobsByRangeRequest, ) { self.terminate_response_stream( peer_id, - connection_id, - substream_id, - request_id, - self.handle_blobs_by_range_request_inner( - peer_id, - connection_id, - substream_id, - request_id, - req, - ), + inbound_request_id, + self.handle_blobs_by_range_request_inner(peer_id, inbound_request_id, req), Response::BlobsByRange, ); } @@ -877,9 +783,7 @@ impl NetworkBeaconProcessor { fn handle_blobs_by_range_request_inner( &self, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, req: BlobsByRangeRequest, ) -> Result<(), (RpcErrorResponse, &'static str)> { debug!( @@ -1016,9 +920,8 @@ impl NetworkBeaconProcessor { blobs_sent += 1; self.send_network_message(NetworkMessage::SendResponse { peer_id, + inbound_request_id, response: Response::BlobsByRange(Some(blob_sidecar.clone())), - request_id, - id: (connection_id, substream_id), }); } } @@ -1048,23 +951,13 @@ impl NetworkBeaconProcessor { pub fn handle_data_columns_by_range_request( &self, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, req: DataColumnsByRangeRequest, ) { self.terminate_response_stream( peer_id, - connection_id, - substream_id, - request_id, - self.handle_data_columns_by_range_request_inner( - peer_id, - connection_id, - substream_id, - request_id, - req, - ), + inbound_request_id, + self.handle_data_columns_by_range_request_inner(peer_id, inbound_request_id, req), Response::DataColumnsByRange, ); } @@ -1073,9 +966,7 @@ impl NetworkBeaconProcessor { pub fn handle_data_columns_by_range_request_inner( &self, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, req: DataColumnsByRangeRequest, ) -> Result<(), (RpcErrorResponse, &'static str)> { debug!( @@ -1205,11 +1096,10 @@ impl NetworkBeaconProcessor { data_columns_sent += 1; self.send_network_message(NetworkMessage::SendResponse { peer_id, - request_id, + inbound_request_id, response: Response::DataColumnsByRange(Some( data_column_sidecar.clone(), )), - id: (connection_id, substream_id), }); } Ok(None) => {} // no-op @@ -1252,32 +1142,20 @@ impl NetworkBeaconProcessor { fn terminate_response_single_item Response>( &self, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, result: Result, into_response: F, ) { match result { Ok(resp) => { - // Not necessary to explicitly send a termination message if this InboundRequest - // returns <= 1 for InboundRequest::expected_responses - // https://github.com/sigp/lighthouse/blob/3058b96f2560f1da04ada4f9d8ba8e5651794ff6/beacon_node/lighthouse_network/src/rpc/handler.rs#L555-L558 self.send_network_message(NetworkMessage::SendResponse { peer_id, - request_id, + inbound_request_id, response: into_response(resp), - id: (connection_id, substream_id), }); } Err((error_code, reason)) => { - self.send_error_response( - peer_id, - error_code, - reason, - (connection_id, substream_id), - request_id, - ); + self.send_error_response(peer_id, error_code, reason, inbound_request_id); } } } @@ -1287,27 +1165,18 @@ impl NetworkBeaconProcessor { fn terminate_response_stream) -> Response>( &self, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, result: Result<(), (RpcErrorResponse, &'static str)>, into_response: F, ) { match result { Ok(_) => self.send_network_message(NetworkMessage::SendResponse { peer_id, - request_id, + inbound_request_id, response: into_response(None), - id: (connection_id, substream_id), }), Err((error_code, reason)) => { - self.send_error_response( - peer_id, - error_code, - reason.into(), - (connection_id, substream_id), - request_id, - ); + self.send_error_response(peer_id, error_code, reason.into(), inbound_request_id); } } } diff --git a/beacon_node/network/src/network_beacon_processor/tests.rs b/beacon_node/network/src/network_beacon_processor/tests.rs index 69ba5c1dbd..aa5f54ac1f 100644 --- a/beacon_node/network/src/network_beacon_processor/tests.rs +++ b/beacon_node/network/src/network_beacon_processor/tests.rs @@ -14,9 +14,8 @@ use beacon_chain::test_utils::{ }; use beacon_chain::{BeaconChain, WhenSlotSkipped}; use beacon_processor::{work_reprocessing_queue::*, *}; -use lighthouse_network::discovery::ConnectionId; use lighthouse_network::rpc::methods::{BlobsByRangeRequest, MetaDataV3}; -use lighthouse_network::rpc::{RequestId, SubstreamId}; +use lighthouse_network::rpc::InboundRequestId; use lighthouse_network::{ discv5::enr::{self, CombinedKey}, rpc::methods::{MetaData, MetaDataV2}, @@ -366,9 +365,7 @@ impl TestRig { self.network_beacon_processor .send_blobs_by_range_request( PeerId::random(), - ConnectionId::new_unchecked(42), - SubstreamId::new(24), - RequestId::new_unchecked(0), + InboundRequestId::new_unchecked(42, 24), BlobsByRangeRequest { start_slot: 0, count, @@ -1149,8 +1146,7 @@ async fn test_blobs_by_range() { if let NetworkMessage::SendResponse { peer_id: _, response: Response::BlobsByRange(blob), - id: _, - request_id: _, + inbound_request_id: _, } = next { if blob.is_some() { diff --git a/beacon_node/network/src/router.rs b/beacon_node/network/src/router.rs index 7376244501..05c00b76af 100644 --- a/beacon_node/network/src/router.rs +++ b/beacon_node/network/src/router.rs @@ -14,12 +14,10 @@ use beacon_processor::{ work_reprocessing_queue::ReprocessQueueMessage, BeaconProcessorSend, DuplicateCache, }; use futures::prelude::*; -use lighthouse_network::discovery::ConnectionId; use lighthouse_network::rpc::*; use lighthouse_network::{ - rpc, service::api_types::{AppRequestId, SyncRequestId}, - MessageId, NetworkGlobals, PeerId, PeerRequestId, PubsubMessage, Response, + MessageId, NetworkGlobals, PeerId, PubsubMessage, Response, }; use logging::crit; use logging::TimeLatch; @@ -54,19 +52,19 @@ pub enum RouterMessage { /// An RPC request has been received. RPCRequestReceived { peer_id: PeerId, - id: PeerRequestId, - request: rpc::Request, + inbound_request_id: InboundRequestId, + request_type: RequestType, }, /// An RPC response has been received. RPCResponseReceived { peer_id: PeerId, - request_id: AppRequestId, + app_request_id: AppRequestId, response: Response, }, /// An RPC request failed RPCFailed { peer_id: PeerId, - request_id: AppRequestId, + app_request_id: AppRequestId, error: RPCError, }, /// A gossip message has been received. The fields are: message id, the peer that sent us this @@ -159,24 +157,24 @@ impl Router { } RouterMessage::RPCRequestReceived { peer_id, - id, - request, + inbound_request_id, + request_type, } => { - self.handle_rpc_request(peer_id, id, request); + self.handle_rpc_request(peer_id, inbound_request_id, request_type); } RouterMessage::RPCResponseReceived { peer_id, - request_id, + app_request_id, response, } => { - self.handle_rpc_response(peer_id, request_id, response); + self.handle_rpc_response(peer_id, app_request_id, response); } RouterMessage::RPCFailed { peer_id, - request_id, + app_request_id, error, } => { - self.on_rpc_error(peer_id, request_id, error); + self.on_rpc_error(peer_id, app_request_id, error); } RouterMessage::PubsubMessage(id, peer_id, gossip, should_process) => { self.handle_gossip(id, peer_id, gossip, should_process); @@ -190,23 +188,18 @@ impl Router { fn handle_rpc_request( &mut self, peer_id: PeerId, - request_id: PeerRequestId, - rpc_request: rpc::Request, + inbound_request_id: InboundRequestId, // Use ResponseId here + request_type: RequestType, ) { if !self.network_globals.peers.read().is_connected(&peer_id) { - debug!( %peer_id, request = ?rpc_request, "Dropping request of disconnected peer"); + debug!(%peer_id, request = ?request_type, "Dropping request of disconnected peer"); return; } - match rpc_request.r#type { - RequestType::Status(status_message) => self.on_status_request( - peer_id, - request_id.0, - request_id.1, - rpc_request.id, - status_message, - ), + match request_type { + RequestType::Status(status_message) => { + self.on_status_request(peer_id, inbound_request_id, status_message) + } RequestType::BlocksByRange(request) => { - // return just one block in case the step parameter is used. https://github.com/ethereum/consensus-specs/pull/2856 let mut count = *request.count(); if *request.step() > 1 { count = 1; @@ -223,9 +216,7 @@ impl Router { self.handle_beacon_processor_send_result( self.network_beacon_processor.send_blocks_by_range_request( peer_id, - request_id.0, - request_id.1, - rpc_request.id, + inbound_request_id, blocks_request, ), ) @@ -233,86 +224,50 @@ impl Router { RequestType::BlocksByRoot(request) => self.handle_beacon_processor_send_result( self.network_beacon_processor.send_blocks_by_roots_request( peer_id, - request_id.0, - request_id.1, - rpc_request.id, + inbound_request_id, request, ), ), RequestType::BlobsByRange(request) => self.handle_beacon_processor_send_result( self.network_beacon_processor.send_blobs_by_range_request( peer_id, - request_id.0, - request_id.1, - rpc_request.id, + inbound_request_id, request, ), ), RequestType::BlobsByRoot(request) => self.handle_beacon_processor_send_result( self.network_beacon_processor.send_blobs_by_roots_request( peer_id, - request_id.0, - request_id.1, - rpc_request.id, + inbound_request_id, request, ), ), RequestType::DataColumnsByRoot(request) => self.handle_beacon_processor_send_result( self.network_beacon_processor - .send_data_columns_by_roots_request( - peer_id, - request_id.0, - request_id.1, - rpc_request.id, - request, - ), + .send_data_columns_by_roots_request(peer_id, inbound_request_id, request), ), RequestType::DataColumnsByRange(request) => self.handle_beacon_processor_send_result( self.network_beacon_processor - .send_data_columns_by_range_request( - peer_id, - request_id.0, - request_id.1, - rpc_request.id, - request, - ), + .send_data_columns_by_range_request(peer_id, inbound_request_id, request), ), RequestType::LightClientBootstrap(request) => self.handle_beacon_processor_send_result( self.network_beacon_processor - .send_light_client_bootstrap_request( - peer_id, - request_id.0, - request_id.1, - rpc_request.id, - request, - ), + .send_light_client_bootstrap_request(peer_id, inbound_request_id, request), ), RequestType::LightClientOptimisticUpdate => self.handle_beacon_processor_send_result( self.network_beacon_processor - .send_light_client_optimistic_update_request( - peer_id, - request_id.0, - request_id.1, - rpc_request.id, - ), + .send_light_client_optimistic_update_request(peer_id, inbound_request_id), ), RequestType::LightClientFinalityUpdate => self.handle_beacon_processor_send_result( self.network_beacon_processor - .send_light_client_finality_update_request( - peer_id, - request_id.0, - request_id.1, - rpc_request.id, - ), + .send_light_client_finality_update_request(peer_id, inbound_request_id), ), RequestType::LightClientUpdatesByRange(request) => self .handle_beacon_processor_send_result( self.network_beacon_processor .send_light_client_updates_by_range_request( peer_id, - request_id.0, - request_id.1, - rpc_request.id, + inbound_request_id, request, ), ), @@ -324,7 +279,7 @@ impl Router { fn handle_rpc_response( &mut self, peer_id: PeerId, - request_id: AppRequestId, + app_request_id: AppRequestId, response: Response, ) { match response { @@ -336,22 +291,22 @@ impl Router { ) } Response::BlocksByRange(beacon_block) => { - self.on_blocks_by_range_response(peer_id, request_id, beacon_block); + self.on_blocks_by_range_response(peer_id, app_request_id, beacon_block); } Response::BlocksByRoot(beacon_block) => { - self.on_blocks_by_root_response(peer_id, request_id, beacon_block); + self.on_blocks_by_root_response(peer_id, app_request_id, beacon_block); } Response::BlobsByRange(blob) => { - self.on_blobs_by_range_response(peer_id, request_id, blob); + self.on_blobs_by_range_response(peer_id, app_request_id, blob); } Response::BlobsByRoot(blob) => { - self.on_blobs_by_root_response(peer_id, request_id, blob); + self.on_blobs_by_root_response(peer_id, app_request_id, blob); } Response::DataColumnsByRoot(data_column) => { - self.on_data_columns_by_root_response(peer_id, request_id, data_column); + self.on_data_columns_by_root_response(peer_id, app_request_id, data_column); } Response::DataColumnsByRange(data_column) => { - self.on_data_columns_by_range_response(peer_id, request_id, data_column); + self.on_data_columns_by_range_response(peer_id, app_request_id, data_column); } // Light client responses should not be received Response::LightClientBootstrap(_) @@ -563,12 +518,12 @@ impl Router { /// An error occurred during an RPC request. The state is maintained by the sync manager, so /// this function notifies the sync manager of the error. - pub fn on_rpc_error(&mut self, peer_id: PeerId, request_id: AppRequestId, error: RPCError) { + pub fn on_rpc_error(&mut self, peer_id: PeerId, app_request_id: AppRequestId, error: RPCError) { // Check if the failed RPC belongs to sync - if let AppRequestId::Sync(request_id) = request_id { + if let AppRequestId::Sync(sync_request_id) = app_request_id { self.send_to_sync(SyncMessage::RpcError { peer_id, - request_id, + sync_request_id, error, }); } @@ -580,9 +535,7 @@ impl Router { pub fn on_status_request( &mut self, peer_id: PeerId, - connection_id: ConnectionId, - substream_id: SubstreamId, - request_id: RequestId, + inbound_request_id: InboundRequestId, // Use ResponseId here status: StatusMessage, ) { debug!(%peer_id, ?status, "Received Status Request"); @@ -590,9 +543,8 @@ impl Router { // Say status back. self.network.send_response( peer_id, + inbound_request_id, Response::Status(status_message(&self.chain)), - (connection_id, substream_id), - request_id, ); self.handle_beacon_processor_send_result( @@ -606,11 +558,11 @@ impl Router { pub fn on_blocks_by_range_response( &mut self, peer_id: PeerId, - request_id: AppRequestId, + app_request_id: AppRequestId, beacon_block: Option>>, ) { - let request_id = match request_id { - AppRequestId::Sync(sync_id) => match sync_id { + let sync_request_id = match app_request_id { + AppRequestId::Sync(sync_request_id) => match sync_request_id { id @ SyncRequestId::BlocksByRange { .. } => id, other => { crit!(request = ?other, "BlocksByRange response on incorrect request"); @@ -621,6 +573,7 @@ impl Router { crit!(%peer_id, "All BBRange requests belong to sync"); return; } + AppRequestId::Internal => unreachable!("Handled internally"), }; trace!( @@ -631,7 +584,7 @@ impl Router { self.send_to_sync(SyncMessage::RpcBlock { peer_id, - request_id, + sync_request_id, beacon_block, seen_timestamp: timestamp_now(), }); @@ -640,7 +593,7 @@ impl Router { pub fn on_blobs_by_range_response( &mut self, peer_id: PeerId, - request_id: AppRequestId, + app_request_id: AppRequestId, blob_sidecar: Option>>, ) { trace!( @@ -648,10 +601,10 @@ impl Router { "Received BlobsByRange Response" ); - if let AppRequestId::Sync(id) = request_id { + if let AppRequestId::Sync(sync_request_id) = app_request_id { self.send_to_sync(SyncMessage::RpcBlob { peer_id, - request_id: id, + sync_request_id, blob_sidecar, seen_timestamp: timestamp_now(), }); @@ -664,10 +617,10 @@ impl Router { pub fn on_blocks_by_root_response( &mut self, peer_id: PeerId, - request_id: AppRequestId, + app_request_id: AppRequestId, beacon_block: Option>>, ) { - let request_id = match request_id { + let sync_request_id = match app_request_id { AppRequestId::Sync(sync_id) => match sync_id { id @ SyncRequestId::SingleBlock { .. } => id, other => { @@ -679,6 +632,7 @@ impl Router { crit!(%peer_id, "All BBRoot requests belong to sync"); return; } + AppRequestId::Internal => unreachable!("Handled internally"), }; trace!( @@ -687,7 +641,7 @@ impl Router { ); self.send_to_sync(SyncMessage::RpcBlock { peer_id, - request_id, + sync_request_id, beacon_block, seen_timestamp: timestamp_now(), }); @@ -697,10 +651,10 @@ impl Router { pub fn on_blobs_by_root_response( &mut self, peer_id: PeerId, - request_id: AppRequestId, + app_request_id: AppRequestId, blob_sidecar: Option>>, ) { - let request_id = match request_id { + let sync_request_id = match app_request_id { AppRequestId::Sync(sync_id) => match sync_id { id @ SyncRequestId::SingleBlob { .. } => id, other => { @@ -712,6 +666,7 @@ impl Router { crit!(%peer_id, "All BlobsByRoot requests belong to sync"); return; } + AppRequestId::Internal => unreachable!("Handled internally"), }; trace!( @@ -719,7 +674,7 @@ impl Router { "Received BlobsByRoot Response" ); self.send_to_sync(SyncMessage::RpcBlob { - request_id, + sync_request_id, peer_id, blob_sidecar, seen_timestamp: timestamp_now(), @@ -730,10 +685,10 @@ impl Router { pub fn on_data_columns_by_root_response( &mut self, peer_id: PeerId, - request_id: AppRequestId, + app_request_id: AppRequestId, data_column: Option>>, ) { - let request_id = match request_id { + let sync_request_id = match app_request_id { AppRequestId::Sync(sync_id) => match sync_id { id @ SyncRequestId::DataColumnsByRoot { .. } => id, other => { @@ -745,6 +700,7 @@ impl Router { crit!(%peer_id, "All DataColumnsByRoot requests belong to sync"); return; } + AppRequestId::Internal => unreachable!("Handled internally"), }; trace!( @@ -752,7 +708,7 @@ impl Router { "Received DataColumnsByRoot Response" ); self.send_to_sync(SyncMessage::RpcDataColumn { - request_id, + sync_request_id, peer_id, data_column, seen_timestamp: timestamp_now(), @@ -762,7 +718,7 @@ impl Router { pub fn on_data_columns_by_range_response( &mut self, peer_id: PeerId, - request_id: AppRequestId, + app_request_id: AppRequestId, data_column: Option>>, ) { trace!( @@ -770,10 +726,10 @@ impl Router { "Received DataColumnsByRange Response" ); - if let AppRequestId::Sync(id) = request_id { + if let AppRequestId::Sync(sync_request_id) = app_request_id { self.send_to_sync(SyncMessage::RpcDataColumn { peer_id, - request_id: id, + sync_request_id, data_column, seen_timestamp: timestamp_now(), }); @@ -824,7 +780,7 @@ impl HandlerNetworkContext { pub fn send_processor_request(&mut self, peer_id: PeerId, request: RequestType) { self.inform_network(NetworkMessage::SendRequest { peer_id, - request_id: AppRequestId::Router, + app_request_id: AppRequestId::Router, request, }) } @@ -833,14 +789,12 @@ impl HandlerNetworkContext { pub fn send_response( &mut self, peer_id: PeerId, + inbound_request_id: InboundRequestId, response: Response, - id: PeerRequestId, - request_id: RequestId, ) { self.inform_network(NetworkMessage::SendResponse { - request_id, peer_id, - id, + inbound_request_id, response, }) } diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index 778ac63290..7afd62ab2e 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -10,14 +10,15 @@ use beacon_processor::{work_reprocessing_queue::ReprocessQueueMessage, BeaconPro use futures::channel::mpsc::Sender; use futures::future::OptionFuture; use futures::prelude::*; -use lighthouse_network::rpc::{RequestId, RequestType}; +use lighthouse_network::rpc::InboundRequestId; +use lighthouse_network::rpc::RequestType; use lighthouse_network::service::Network; use lighthouse_network::types::GossipKind; use lighthouse_network::Enr; use lighthouse_network::{prometheus_client::registry::Registry, MessageAcceptance}; use lighthouse_network::{ rpc::{GoodbyeReason, RpcErrorResponse}, - Context, PeerAction, PeerRequestId, PubsubMessage, ReportSource, Response, Subnet, + Context, PeerAction, PubsubMessage, ReportSource, Response, Subnet, }; use lighthouse_network::{ service::api_types::AppRequestId, @@ -61,22 +62,20 @@ pub enum NetworkMessage { SendRequest { peer_id: PeerId, request: RequestType, - request_id: AppRequestId, + app_request_id: AppRequestId, }, /// Send a successful Response to the libp2p service. SendResponse { peer_id: PeerId, - request_id: RequestId, + inbound_request_id: InboundRequestId, response: Response, - id: PeerRequestId, }, /// Sends an error response to an RPC request. SendErrorResponse { peer_id: PeerId, - request_id: RequestId, + inbound_request_id: InboundRequestId, error: RpcErrorResponse, reason: String, - id: PeerRequestId, }, /// Publish a list of messages to the gossipsub protocol. Publish { messages: Vec> }, @@ -488,30 +487,34 @@ impl NetworkService { } NetworkEvent::RequestReceived { peer_id, - id, - request, + inbound_request_id, + request_type, } => { self.send_to_router(RouterMessage::RPCRequestReceived { peer_id, - id, - request, + inbound_request_id, + request_type, }); } NetworkEvent::ResponseReceived { peer_id, - id, + app_request_id, response, } => { self.send_to_router(RouterMessage::RPCResponseReceived { peer_id, - request_id: id, + app_request_id, response, }); } - NetworkEvent::RPCFailed { id, peer_id, error } => { + NetworkEvent::RPCFailed { + app_request_id, + peer_id, + error, + } => { self.send_to_router(RouterMessage::RPCFailed { peer_id, - request_id: id, + app_request_id, error, }); } @@ -601,35 +604,34 @@ impl NetworkService { NetworkMessage::SendRequest { peer_id, request, - request_id, + app_request_id, } => { - if let Err((request_id, error)) = - self.libp2p.send_request(peer_id, request_id, request) + if let Err((app_request_id, error)) = + self.libp2p.send_request(peer_id, app_request_id, request) { self.send_to_router(RouterMessage::RPCFailed { peer_id, - request_id, + app_request_id, error, }); } } NetworkMessage::SendResponse { peer_id, + inbound_request_id, response, - id, - request_id, } => { - self.libp2p.send_response(peer_id, id, request_id, response); + self.libp2p + .send_response(peer_id, inbound_request_id, response); } NetworkMessage::SendErrorResponse { peer_id, error, - id, - request_id, + inbound_request_id, reason, } => { self.libp2p - .send_error_response(peer_id, id, request_id, error, reason); + .send_error_response(peer_id, inbound_request_id, error, reason); } NetworkMessage::ValidationResult { propagation_source, diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 9a48e9aa5d..a02302a525 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -108,7 +108,7 @@ pub enum SyncMessage { /// A block has been received from the RPC. RpcBlock { - request_id: SyncRequestId, + sync_request_id: SyncRequestId, peer_id: PeerId, beacon_block: Option>>, seen_timestamp: Duration, @@ -116,7 +116,7 @@ pub enum SyncMessage { /// A blob has been received from the RPC. RpcBlob { - request_id: SyncRequestId, + sync_request_id: SyncRequestId, peer_id: PeerId, blob_sidecar: Option>>, seen_timestamp: Duration, @@ -124,7 +124,7 @@ pub enum SyncMessage { /// A data columns has been received from the RPC RpcDataColumn { - request_id: SyncRequestId, + sync_request_id: SyncRequestId, peer_id: PeerId, data_column: Option>>, seen_timestamp: Duration, @@ -153,7 +153,7 @@ pub enum SyncMessage { /// An RPC Error has occurred on a request. RpcError { peer_id: PeerId, - request_id: SyncRequestId, + sync_request_id: SyncRequestId, error: RPCError, }, @@ -477,9 +477,9 @@ impl SyncManager { } /// Handles RPC errors related to requests that were emitted from the sync manager. - fn inject_error(&mut self, peer_id: PeerId, request_id: SyncRequestId, error: RPCError) { + fn inject_error(&mut self, peer_id: PeerId, sync_request_id: SyncRequestId, error: RPCError) { trace!("Sync manager received a failed RPC"); - match request_id { + match sync_request_id { SyncRequestId::SingleBlock { id } => { self.on_single_block_response(id, peer_id, RpcEvent::RPCError(error)) } @@ -509,8 +509,8 @@ impl SyncManager { fn peer_disconnect(&mut self, peer_id: &PeerId) { // Inject a Disconnected error on all requests associated with the disconnected peer // to retry all batches/lookups - for request_id in self.network.peer_disconnected(peer_id) { - self.inject_error(*peer_id, request_id, RPCError::Disconnected); + for sync_request_id in self.network.peer_disconnected(peer_id) { + self.inject_error(*peer_id, sync_request_id, RPCError::Disconnected); } // Remove peer from all data structures @@ -751,25 +751,27 @@ impl SyncManager { self.add_peers_force_range_sync(&peers, head_root, head_slot); } SyncMessage::RpcBlock { - request_id, + sync_request_id, peer_id, beacon_block, seen_timestamp, } => { - self.rpc_block_received(request_id, peer_id, beacon_block, seen_timestamp); + self.rpc_block_received(sync_request_id, peer_id, beacon_block, seen_timestamp); } SyncMessage::RpcBlob { - request_id, + sync_request_id, peer_id, blob_sidecar, seen_timestamp, - } => self.rpc_blob_received(request_id, peer_id, blob_sidecar, seen_timestamp), + } => self.rpc_blob_received(sync_request_id, peer_id, blob_sidecar, seen_timestamp), SyncMessage::RpcDataColumn { - request_id, + sync_request_id, peer_id, data_column, seen_timestamp, - } => self.rpc_data_column_received(request_id, peer_id, data_column, seen_timestamp), + } => { + self.rpc_data_column_received(sync_request_id, peer_id, data_column, seen_timestamp) + } SyncMessage::UnknownParentBlock(peer_id, block, block_root) => { let block_slot = block.slot(); let parent_root = block.parent_root(); @@ -845,9 +847,9 @@ impl SyncManager { } SyncMessage::RpcError { peer_id, - request_id, + sync_request_id, error, - } => self.inject_error(peer_id, request_id, error), + } => self.inject_error(peer_id, sync_request_id, error), SyncMessage::BlockComponentProcessed { process_type, result, @@ -1018,12 +1020,12 @@ impl SyncManager { fn rpc_block_received( &mut self, - request_id: SyncRequestId, + sync_request_id: SyncRequestId, peer_id: PeerId, block: Option>>, seen_timestamp: Duration, ) { - match request_id { + match sync_request_id { SyncRequestId::SingleBlock { id } => self.on_single_block_response( id, peer_id, @@ -1060,12 +1062,12 @@ impl SyncManager { fn rpc_blob_received( &mut self, - request_id: SyncRequestId, + sync_request_id: SyncRequestId, peer_id: PeerId, blob: Option>>, seen_timestamp: Duration, ) { - match request_id { + match sync_request_id { SyncRequestId::SingleBlob { id } => self.on_single_blob_response( id, peer_id, @@ -1084,12 +1086,12 @@ impl SyncManager { fn rpc_data_column_received( &mut self, - request_id: SyncRequestId, + sync_request_id: SyncRequestId, peer_id: PeerId, data_column: Option>>, seen_timestamp: Duration, ) { - match request_id { + match sync_request_id { SyncRequestId::DataColumnsByRoot(req_id) => { self.on_data_columns_by_root_response( req_id, diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index 16fcf93bcf..69b350f8cb 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -372,11 +372,11 @@ impl SyncNetworkContext { ); let request = RequestType::Status(status_message.clone()); - let request_id = AppRequestId::Router; + let app_request_id = AppRequestId::Router; let _ = self.send_network_msg(NetworkMessage::SendRequest { peer_id, request, - request_id, + app_request_id, }); } } @@ -595,7 +595,7 @@ impl SyncNetworkContext { .send(NetworkMessage::SendRequest { peer_id, request: RequestType::BlocksByRoot(request.into_request(&self.fork_context)), - request_id: AppRequestId::Sync(SyncRequestId::SingleBlock { id }), + app_request_id: AppRequestId::Sync(SyncRequestId::SingleBlock { id }), }) .map_err(|_| RpcRequestSendError::NetworkSendError)?; @@ -684,7 +684,7 @@ impl SyncNetworkContext { .send(NetworkMessage::SendRequest { peer_id, request: RequestType::BlobsByRoot(request.clone().into_request(&self.fork_context)), - request_id: AppRequestId::Sync(SyncRequestId::SingleBlob { id }), + app_request_id: AppRequestId::Sync(SyncRequestId::SingleBlob { id }), }) .map_err(|_| RpcRequestSendError::NetworkSendError)?; @@ -733,7 +733,7 @@ impl SyncNetworkContext { self.send_network_msg(NetworkMessage::SendRequest { peer_id, request: RequestType::DataColumnsByRoot(request.clone().into_request(&self.chain.spec)), - request_id: AppRequestId::Sync(SyncRequestId::DataColumnsByRoot(id)), + app_request_id: AppRequestId::Sync(SyncRequestId::DataColumnsByRoot(id)), })?; debug!( @@ -839,7 +839,7 @@ impl SyncNetworkContext { .send(NetworkMessage::SendRequest { peer_id, request: RequestType::BlocksByRange(request.clone().into()), - request_id: AppRequestId::Sync(SyncRequestId::BlocksByRange(id)), + app_request_id: AppRequestId::Sync(SyncRequestId::BlocksByRange(id)), }) .map_err(|_| RpcRequestSendError::NetworkSendError)?; @@ -880,7 +880,7 @@ impl SyncNetworkContext { .send(NetworkMessage::SendRequest { peer_id, request: RequestType::BlobsByRange(request.clone()), - request_id: AppRequestId::Sync(SyncRequestId::BlobsByRange(id)), + app_request_id: AppRequestId::Sync(SyncRequestId::BlobsByRange(id)), }) .map_err(|_| RpcRequestSendError::NetworkSendError)?; @@ -919,7 +919,7 @@ impl SyncNetworkContext { self.send_network_msg(NetworkMessage::SendRequest { peer_id, request: RequestType::DataColumnsByRange(request.clone()), - request_id: AppRequestId::Sync(SyncRequestId::DataColumnsByRange(id)), + app_request_id: AppRequestId::Sync(SyncRequestId::DataColumnsByRange(id)), }) .map_err(|_| RpcRequestSendError::NetworkSendError)?; diff --git a/beacon_node/network/src/sync/tests/lookups.rs b/beacon_node/network/src/sync/tests/lookups.rs index fe72979930..3864e66e1b 100644 --- a/beacon_node/network/src/sync/tests/lookups.rs +++ b/beacon_node/network/src/sync/tests/lookups.rs @@ -460,7 +460,7 @@ impl TestRig { ) { self.log("parent_lookup_block_response"); self.send_sync_message(SyncMessage::RpcBlock { - request_id: SyncRequestId::SingleBlock { id }, + sync_request_id: SyncRequestId::SingleBlock { id }, peer_id, beacon_block, seen_timestamp: D, @@ -475,7 +475,7 @@ impl TestRig { ) { self.log("single_lookup_block_response"); self.send_sync_message(SyncMessage::RpcBlock { - request_id: SyncRequestId::SingleBlock { id }, + sync_request_id: SyncRequestId::SingleBlock { id }, peer_id, beacon_block, seen_timestamp: D, @@ -493,7 +493,7 @@ impl TestRig { blob_sidecar.as_ref().map(|b| b.index) )); self.send_sync_message(SyncMessage::RpcBlob { - request_id: SyncRequestId::SingleBlob { id }, + sync_request_id: SyncRequestId::SingleBlob { id }, peer_id, blob_sidecar, seen_timestamp: D, @@ -507,7 +507,7 @@ impl TestRig { blob_sidecar: Option>>, ) { self.send_sync_message(SyncMessage::RpcBlob { - request_id: SyncRequestId::SingleBlob { id }, + sync_request_id: SyncRequestId::SingleBlob { id }, peer_id, blob_sidecar, seen_timestamp: D, @@ -583,7 +583,7 @@ impl TestRig { fn parent_lookup_failed(&mut self, id: SingleLookupReqId, peer_id: PeerId, error: RPCError) { self.send_sync_message(SyncMessage::RpcError { peer_id, - request_id: SyncRequestId::SingleBlock { id }, + sync_request_id: SyncRequestId::SingleBlock { id }, error, }) } @@ -602,7 +602,7 @@ impl TestRig { fn single_lookup_failed(&mut self, id: SingleLookupReqId, peer_id: PeerId, error: RPCError) { self.send_sync_message(SyncMessage::RpcError { peer_id, - request_id: SyncRequestId::SingleBlock { id }, + sync_request_id: SyncRequestId::SingleBlock { id }, error, }) } @@ -614,11 +614,11 @@ impl TestRig { } } - fn return_empty_sampling_request(&mut self, (request_id, _): DCByRootId) { + fn return_empty_sampling_request(&mut self, (sync_request_id, _): DCByRootId) { let peer_id = PeerId::random(); // Send stream termination self.send_sync_message(SyncMessage::RpcDataColumn { - request_id, + sync_request_id, peer_id, data_column: None, seen_timestamp: timestamp_now(), @@ -631,10 +631,10 @@ impl TestRig { peer_id: PeerId, error: RPCError, ) { - for (request_id, _) in sampling_ids { + for (sync_request_id, _) in sampling_ids { self.send_sync_message(SyncMessage::RpcError { peer_id, - request_id, + sync_request_id, error: error.clone(), }) } @@ -760,14 +760,14 @@ impl TestRig { fn complete_data_columns_by_root_request( &mut self, - (request_id, _): DCByRootId, + (sync_request_id, _): DCByRootId, data_columns: &[Arc>], ) { let peer_id = PeerId::random(); for data_column in data_columns { // Send chunks self.send_sync_message(SyncMessage::RpcDataColumn { - request_id, + sync_request_id, peer_id, data_column: Some(data_column.clone()), seen_timestamp: timestamp_now(), @@ -775,7 +775,7 @@ impl TestRig { } // Send stream termination self.send_sync_message(SyncMessage::RpcDataColumn { - request_id, + sync_request_id, peer_id, data_column: None, seen_timestamp: timestamp_now(), @@ -785,17 +785,17 @@ impl TestRig { /// Return RPCErrors for all active requests of peer fn rpc_error_all_active_requests(&mut self, disconnected_peer_id: PeerId) { self.drain_network_rx(); - while let Ok(request_id) = self.pop_received_network_event(|ev| match ev { + while let Ok(sync_request_id) = self.pop_received_network_event(|ev| match ev { NetworkMessage::SendRequest { peer_id, - request_id: AppRequestId::Sync(id), + app_request_id: AppRequestId::Sync(id), .. } if *peer_id == disconnected_peer_id => Some(*id), _ => None, }) { self.send_sync_message(SyncMessage::RpcError { peer_id: disconnected_peer_id, - request_id, + sync_request_id, error: RPCError::Disconnected, }); } @@ -879,7 +879,7 @@ impl TestRig { NetworkMessage::SendRequest { peer_id: _, request: RequestType::BlocksByRoot(request), - request_id: AppRequestId::Sync(SyncRequestId::SingleBlock { id }), + app_request_id: AppRequestId::Sync(SyncRequestId::SingleBlock { id }), } if request.block_roots().to_vec().contains(&for_block) => Some(*id), _ => None, }) @@ -899,7 +899,7 @@ impl TestRig { NetworkMessage::SendRequest { peer_id: _, request: RequestType::BlobsByRoot(request), - request_id: AppRequestId::Sync(SyncRequestId::SingleBlob { id }), + app_request_id: AppRequestId::Sync(SyncRequestId::SingleBlob { id }), } if request .blob_ids .to_vec() @@ -924,7 +924,7 @@ impl TestRig { NetworkMessage::SendRequest { peer_id: _, request: RequestType::BlocksByRoot(request), - request_id: AppRequestId::Sync(SyncRequestId::SingleBlock { id }), + app_request_id: AppRequestId::Sync(SyncRequestId::SingleBlock { id }), } if request.block_roots().to_vec().contains(&for_block) => Some(*id), _ => None, }) @@ -946,7 +946,7 @@ impl TestRig { NetworkMessage::SendRequest { peer_id: _, request: RequestType::BlobsByRoot(request), - request_id: AppRequestId::Sync(SyncRequestId::SingleBlob { id }), + app_request_id: AppRequestId::Sync(SyncRequestId::SingleBlob { id }), } if request .blob_ids .to_vec() @@ -974,7 +974,8 @@ impl TestRig { NetworkMessage::SendRequest { peer_id: _, request: RequestType::DataColumnsByRoot(request), - request_id: AppRequestId::Sync(id @ SyncRequestId::DataColumnsByRoot { .. }), + app_request_id: + AppRequestId::Sync(id @ SyncRequestId::DataColumnsByRoot { .. }), } if request .data_column_ids .to_vec() diff --git a/beacon_node/network/src/sync/tests/range.rs b/beacon_node/network/src/sync/tests/range.rs index ca4344c0b2..2871ea2a4d 100644 --- a/beacon_node/network/src/sync/tests/range.rs +++ b/beacon_node/network/src/sync/tests/range.rs @@ -223,7 +223,7 @@ impl TestRig { RequestType::BlocksByRange(OldBlocksByRangeRequest::V2( OldBlocksByRangeRequestV2 { start_slot, .. }, )), - request_id: AppRequestId::Sync(SyncRequestId::BlocksByRange(id)), + app_request_id: AppRequestId::Sync(SyncRequestId::BlocksByRange(id)), } if filter_f(*peer_id, *start_slot) => Some((*id, *peer_id)), _ => None, }) @@ -240,7 +240,7 @@ impl TestRig { RequestType::DataColumnsByRange(DataColumnsByRangeRequest { start_slot, .. }), - request_id: AppRequestId::Sync(SyncRequestId::DataColumnsByRange(id)), + app_request_id: AppRequestId::Sync(SyncRequestId::DataColumnsByRange(id)), } if filter_f(*peer_id, *start_slot) => Some((*id, *peer_id)), _ => None, }) { @@ -256,7 +256,7 @@ impl TestRig { NetworkMessage::SendRequest { peer_id, request: RequestType::BlobsByRange(BlobsByRangeRequest { start_slot, .. }), - request_id: AppRequestId::Sync(SyncRequestId::BlobsByRange(id)), + app_request_id: AppRequestId::Sync(SyncRequestId::BlobsByRange(id)), } if filter_f(*peer_id, *start_slot) => Some((*id, *peer_id)), _ => None, }) @@ -283,7 +283,7 @@ impl TestRig { "Completing BlocksByRange request {blocks_req_id:?} with empty stream" )); self.send_sync_message(SyncMessage::RpcBlock { - request_id: SyncRequestId::BlocksByRange(blocks_req_id), + sync_request_id: SyncRequestId::BlocksByRange(blocks_req_id), peer_id: block_peer, beacon_block: None, seen_timestamp: D, @@ -297,7 +297,7 @@ impl TestRig { "Completing BlobsByRange request {id:?} with empty stream" )); self.send_sync_message(SyncMessage::RpcBlob { - request_id: SyncRequestId::BlobsByRange(id), + sync_request_id: SyncRequestId::BlobsByRange(id), peer_id, blob_sidecar: None, seen_timestamp: D, @@ -310,7 +310,7 @@ impl TestRig { "Completing DataColumnsByRange request {id:?} with empty stream" )); self.send_sync_message(SyncMessage::RpcDataColumn { - request_id: SyncRequestId::DataColumnsByRange(id), + sync_request_id: SyncRequestId::DataColumnsByRange(id), peer_id, data_column: None, seen_timestamp: D, From 82d16744557522eb62cf56727767985fcc3d953e Mon Sep 17 00:00:00 2001 From: Mac L Date: Fri, 4 Apr 2025 13:30:22 +1100 Subject: [PATCH 14/17] Rust 1.86.0 lints (#7254) Implement lints for the new Rust compiler version 1.86.0. --- beacon_node/beacon_chain/src/beacon_chain.rs | 6 ++-- .../beacon_chain/src/beacon_proposer_cache.rs | 2 +- .../beacon_chain/src/block_verification.rs | 2 +- .../beacon_chain/src/early_attester_cache.rs | 2 +- .../src/eth1_finalization_cache.rs | 2 +- .../tests/payload_invalidation.rs | 2 +- beacon_node/execution_layer/src/lib.rs | 2 +- .../src/peer_manager/mod.rs | 28 +++++++++---------- .../src/peer_manager/peerdb.rs | 6 ++-- .../lighthouse_network/src/rpc/codec.rs | 4 +-- .../src/types/sync_state.rs | 4 +-- .../lighthouse_network/tests/rpc_tests.rs | 4 +-- .../gossip_methods.rs | 2 +- beacon_node/network/src/sync/manager.rs | 2 +- beacon_node/network/src/sync/tests/lookups.rs | 2 +- .../eth2_wallet_manager/src/locked_wallet.rs | 2 +- consensus/proto_array/src/proto_array.rs | 2 +- .../src/proto_array_fork_choice.rs | 2 +- .../types/src/sync_committee_contribution.rs | 4 +-- .../src/test_utils/test_random/bitfield.rs | 4 +-- crypto/bls/src/lib.rs | 2 +- testing/ef_tests/src/cases.rs | 8 +++--- testing/ef_tests/src/cases/fork_choice.rs | 2 +- .../http_api/src/tests/keystores.rs | 4 +-- validator_client/validator_store/src/lib.rs | 4 +-- 25 files changed, 52 insertions(+), 52 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 24f83179f6..624dc968ad 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -741,7 +741,7 @@ impl BeaconChain { /// /// - `slot` always increases by `1`. /// - Skipped slots contain the root of the closest prior - /// non-skipped slot (identical to the way they are stored in `state.block_roots`). + /// non-skipped slot (identical to the way they are stored in `state.block_roots`). /// - Iterator returns `(Hash256, Slot)`. /// /// Will return a `BlockOutOfRange` error if the requested start slot is before the period of @@ -805,7 +805,7 @@ impl BeaconChain { /// /// - `slot` always decreases by `1`. /// - Skipped slots contain the root of the closest prior - /// non-skipped slot (identical to the way they are stored in `state.block_roots`) . + /// non-skipped slot (identical to the way they are stored in `state.block_roots`) . /// - Iterator returns `(Hash256, Slot)`. /// - The provided `block_root` is included as the first item in the iterator. pub fn rev_iter_block_roots_from( @@ -834,7 +834,7 @@ impl BeaconChain { /// - `slot` always decreases by `1`. /// - Iterator returns `(Hash256, Slot)`. /// - As this iterator starts at the `head` of the chain (viz., the best block), the first slot - /// returned may be earlier than the wall-clock slot. + /// returned may be earlier than the wall-clock slot. pub fn rev_iter_state_roots_from<'a>( &'a self, state_root: Hash256, diff --git a/beacon_node/beacon_chain/src/beacon_proposer_cache.rs b/beacon_node/beacon_chain/src/beacon_proposer_cache.rs index d10bbfbbc5..567433caee 100644 --- a/beacon_node/beacon_chain/src/beacon_proposer_cache.rs +++ b/beacon_node/beacon_chain/src/beacon_proposer_cache.rs @@ -178,7 +178,7 @@ pub fn compute_proposer_duties_from_head( /// - Returns an error if `state.current_epoch() > target_epoch`. /// - No-op if `state.current_epoch() == target_epoch`. /// - It must be the case that `state.canonical_root() == state_root`, but this function will not -/// check that. +/// check that. pub fn ensure_state_is_in_epoch( state: &mut BeaconState, state_root: Hash256, diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 4a5282a1d7..48caea9c7f 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -5,7 +5,7 @@ //! - Verification for gossip blocks (i.e., should we gossip some block from the network). //! - Verification for normal blocks (e.g., some block received on the RPC during a parent lookup). //! - Verification for chain segments (e.g., some chain of blocks received on the RPC during a -//! sync). +//! sync). //! //! The primary source of complexity here is that we wish to avoid doing duplicate work as a block //! moves through the verification process. For example, if some block is verified for gossip, we diff --git a/beacon_node/beacon_chain/src/early_attester_cache.rs b/beacon_node/beacon_chain/src/early_attester_cache.rs index c94ea0e941..b62554f1b4 100644 --- a/beacon_node/beacon_chain/src/early_attester_cache.rs +++ b/beacon_node/beacon_chain/src/early_attester_cache.rs @@ -33,7 +33,7 @@ pub struct CacheItem { /// /// - Produce an attestation without using `chain.canonical_head`. /// - Verify that a block root exists (i.e., will be imported in the future) during attestation -/// verification. +/// verification. /// - Provide a block which can be sent to peers via RPC. #[derive(Default)] pub struct EarlyAttesterCache { diff --git a/beacon_node/beacon_chain/src/eth1_finalization_cache.rs b/beacon_node/beacon_chain/src/eth1_finalization_cache.rs index 24b6542eab..8280d15675 100644 --- a/beacon_node/beacon_chain/src/eth1_finalization_cache.rs +++ b/beacon_node/beacon_chain/src/eth1_finalization_cache.rs @@ -469,7 +469,7 @@ pub mod tests { let last_finalized_eth1 = eth1s_by_count .range(0..(finalized_deposits + 1)) .map(|(_, eth1)| eth1) - .last() + .next_back() .cloned(); assert_eq!( eth1cache.finalize(finalized_checkpoint), diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 01b790bb25..d41c33176a 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -1283,7 +1283,7 @@ impl InvalidHeadSetup { /// /// 1. A chain where the only viable head block has an invalid execution payload. /// 2. A block (`fork_block`) which will become the head of the chain when - /// it is imported. + /// it is imported. async fn new() -> InvalidHeadSetup { let slots_per_epoch = E::slots_per_epoch(); let mut rig = InvalidPayloadRig::new().enable_attestations(); diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index cde6cc6f48..820ec8d6b6 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -1710,7 +1710,7 @@ impl ExecutionLayer { /// /// - `Some(true)` if the given `block_hash` is the terminal proof-of-work block. /// - `Some(false)` if the given `block_hash` is certainly *not* the terminal proof-of-work - /// block. + /// block. /// - `None` if the `block_hash` or its parent were not present on the execution engine. /// - `Err(_)` if there was an error connecting to the execution engine. /// diff --git a/beacon_node/lighthouse_network/src/peer_manager/mod.rs b/beacon_node/lighthouse_network/src/peer_manager/mod.rs index 6067d52042..baeb597676 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/mod.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/mod.rs @@ -992,23 +992,23 @@ impl PeerManager { /// - Do not prune outbound peers to exceed our outbound target. /// - Do not prune more peers than our target peer count. /// - If we have an option to remove a number of peers, remove ones that have the least - /// long-lived subnets. + /// long-lived subnets. /// - When pruning peers based on subnet count. If multiple peers can be chosen, choose a peer - /// that is not subscribed to a long-lived sync committee subnet. + /// that is not subscribed to a long-lived sync committee subnet. /// - When pruning peers based on subnet count, do not prune a peer that would lower us below the - /// MIN_SYNC_COMMITTEE_PEERS peer count. To keep it simple, we favour a minimum number of sync-committee-peers over - /// uniformity subnet peers. NOTE: We could apply more sophisticated logic, but the code is - /// simpler and easier to maintain if we take this approach. If we are pruning subnet peers - /// below the MIN_SYNC_COMMITTEE_PEERS and maintaining the sync committee peers, this should be - /// fine as subnet peers are more likely to be found than sync-committee-peers. Also, we're - /// in a bit of trouble anyway if we have so few peers on subnets. The - /// MIN_SYNC_COMMITTEE_PEERS - /// number should be set low as an absolute lower bound to maintain peers on the sync - /// committees. + /// MIN_SYNC_COMMITTEE_PEERS peer count. To keep it simple, we favour a minimum number of sync-committee-peers over + /// uniformity subnet peers. NOTE: We could apply more sophisticated logic, but the code is + /// simpler and easier to maintain if we take this approach. If we are pruning subnet peers + /// below the MIN_SYNC_COMMITTEE_PEERS and maintaining the sync committee peers, this should be + /// fine as subnet peers are more likely to be found than sync-committee-peers. Also, we're + /// in a bit of trouble anyway if we have so few peers on subnets. The + /// MIN_SYNC_COMMITTEE_PEERS + /// number should be set low as an absolute lower bound to maintain peers on the sync + /// committees. /// - Do not prune trusted peers. NOTE: This means if a user has more trusted peers than the - /// excess peer limit, all of the following logic is subverted as we will not prune any peers. - /// Also, the more trusted peers a user has, the less room Lighthouse has to efficiently manage - /// its peers across the subnets. + /// excess peer limit, all of the following logic is subverted as we will not prune any peers. + /// Also, the more trusted peers a user has, the less room Lighthouse has to efficiently manage + /// its peers across the subnets. /// /// Prune peers in the following order: /// 1. Remove worst scoring peers diff --git a/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs b/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs index b692639911..0912bd1cd2 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs @@ -155,7 +155,7 @@ impl PeerDB { matches!( self.connection_status(peer_id), Some(PeerConnectionStatus::Disconnected { .. }) - | Some(PeerConnectionStatus::Unknown { .. }) + | Some(PeerConnectionStatus::Unknown) | None ) && !self.score_state_banned_or_disconnected(peer_id) } @@ -776,8 +776,8 @@ impl PeerDB { NewConnectionState::Connected { .. } // We have established a new connection (peer may not have been seen before) | NewConnectionState::Disconnecting { .. }// We are disconnecting from a peer that may not have been registered before | NewConnectionState::Dialing { .. } // We are dialing a potentially new peer - | NewConnectionState::Disconnected { .. } // Dialing a peer that responds by a different ID can be immediately - // disconnected without having being stored in the db before + | NewConnectionState::Disconnected // Dialing a peer that responds by a different ID can be immediately + // disconnected without having being stored in the db before ) { warn!(log_ref, "Updating state of unknown peer"; "peer_id" => %peer_id, "new_state" => ?new_state); diff --git a/beacon_node/lighthouse_network/src/rpc/codec.rs b/beacon_node/lighthouse_network/src/rpc/codec.rs index 2bf35b0e35..838f1b8a16 100644 --- a/beacon_node/lighthouse_network/src/rpc/codec.rs +++ b/beacon_node/lighthouse_network/src/rpc/codec.rs @@ -1009,7 +1009,7 @@ mod tests { let mut block: BeaconBlockBellatrix<_, FullPayload> = BeaconBlockBellatrix::empty(&Spec::default_spec()); let tx = VariableList::from(vec![0; 1024]); - let txs = VariableList::from(std::iter::repeat(tx).take(5000).collect::>()); + let txs = VariableList::from(std::iter::repeat_n(tx, 5000).collect::>()); block.body.execution_payload.execution_payload.transactions = txs; @@ -1028,7 +1028,7 @@ mod tests { let mut block: BeaconBlockBellatrix<_, FullPayload> = BeaconBlockBellatrix::empty(&Spec::default_spec()); let tx = VariableList::from(vec![0; 1024]); - let txs = VariableList::from(std::iter::repeat(tx).take(100000).collect::>()); + let txs = VariableList::from(std::iter::repeat_n(tx, 100000).collect::>()); block.body.execution_payload.execution_payload.transactions = txs; diff --git a/beacon_node/lighthouse_network/src/types/sync_state.rs b/beacon_node/lighthouse_network/src/types/sync_state.rs index 0519d6f4b0..0327f7073f 100644 --- a/beacon_node/lighthouse_network/src/types/sync_state.rs +++ b/beacon_node/lighthouse_network/src/types/sync_state.rs @@ -104,8 +104,8 @@ impl std::fmt::Display for SyncState { match self { SyncState::SyncingFinalized { .. } => write!(f, "Syncing Finalized Chain"), SyncState::SyncingHead { .. } => write!(f, "Syncing Head Chain"), - SyncState::Synced { .. } => write!(f, "Synced"), - SyncState::Stalled { .. } => write!(f, "Stalled"), + SyncState::Synced => write!(f, "Synced"), + SyncState::Stalled => write!(f, "Stalled"), SyncState::SyncTransition => write!(f, "Evaluating known peers"), SyncState::BackFillSyncing { .. } => write!(f, "Syncing Historical Blocks"), } diff --git a/beacon_node/lighthouse_network/tests/rpc_tests.rs b/beacon_node/lighthouse_network/tests/rpc_tests.rs index 4b54a24ddc..80364753d7 100644 --- a/beacon_node/lighthouse_network/tests/rpc_tests.rs +++ b/beacon_node/lighthouse_network/tests/rpc_tests.rs @@ -25,7 +25,7 @@ type E = MinimalEthSpec; fn bellatrix_block_small(fork_context: &ForkContext, spec: &ChainSpec) -> BeaconBlock { let mut block = BeaconBlockBellatrix::::empty(spec); let tx = VariableList::from(vec![0; 1024]); - let txs = VariableList::from(std::iter::repeat(tx).take(5000).collect::>()); + let txs = VariableList::from(std::iter::repeat_n(tx, 5000).collect::>()); block.body.execution_payload.execution_payload.transactions = txs; @@ -40,7 +40,7 @@ fn bellatrix_block_small(fork_context: &ForkContext, spec: &ChainSpec) -> Beacon fn bellatrix_block_large(fork_context: &ForkContext, spec: &ChainSpec) -> BeaconBlock { let mut block = BeaconBlockBellatrix::::empty(spec); let tx = VariableList::from(vec![0; 1024]); - let txs = VariableList::from(std::iter::repeat(tx).take(100000).collect::>()); + let txs = VariableList::from(std::iter::repeat_n(tx, 100000).collect::>()); block.body.execution_payload.execution_payload.transactions = txs; diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index 0956c153a6..af75791e4d 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -841,7 +841,7 @@ impl NetworkBeaconProcessor { | GossipDataColumnError::ProposerIndexMismatch { .. } | GossipDataColumnError::IsNotLaterThanParent { .. } | GossipDataColumnError::InvalidSubnetId { .. } - | GossipDataColumnError::InvalidInclusionProof { .. } + | GossipDataColumnError::InvalidInclusionProof | GossipDataColumnError::InvalidKzgProof { .. } | GossipDataColumnError::UnexpectedDataColumn | GossipDataColumnError::InvalidColumnIndex(_) diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index fc31e83727..041b1dba9f 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -688,7 +688,7 @@ impl SyncManager { if new_state.is_synced() && !matches!( old_state, - SyncState::Synced { .. } | SyncState::BackFillSyncing { .. } + SyncState::Synced | SyncState::BackFillSyncing { .. } ) { self.network.subscribe_core_topics(); diff --git a/beacon_node/network/src/sync/tests/lookups.rs b/beacon_node/network/src/sync/tests/lookups.rs index 9ab581950c..271b2322fa 100644 --- a/beacon_node/network/src/sync/tests/lookups.rs +++ b/beacon_node/network/src/sync/tests/lookups.rs @@ -1301,7 +1301,7 @@ impl TestRig { .sync_manager .get_sampling_request_status(block_root, index) .unwrap_or_else(|| panic!("No request state for {index}")); - if !matches!(status, crate::sync::peer_sampling::Status::NoPeers { .. }) { + if !matches!(status, crate::sync::peer_sampling::Status::NoPeers) { panic!("expected {block_root} {index} request to be no peers: {status:?}"); } } diff --git a/common/eth2_wallet_manager/src/locked_wallet.rs b/common/eth2_wallet_manager/src/locked_wallet.rs index a77f9bd780..2af863a4bf 100644 --- a/common/eth2_wallet_manager/src/locked_wallet.rs +++ b/common/eth2_wallet_manager/src/locked_wallet.rs @@ -22,7 +22,7 @@ pub const LOCK_FILE: &str = ".lock"; /// /// - Control over the `.lock` file to prevent concurrent access. /// - A `next_validator` function which wraps `Wallet::next_validator`, ensuring that the wallet is -/// persisted to disk (as JSON) between each consecutive call. +/// persisted to disk (as JSON) between each consecutive call. pub struct LockedWallet { wallet_dir: PathBuf, wallet: Wallet, diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 5d0bee4c85..cf6ebb3b00 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -760,7 +760,7 @@ impl ProtoArray { /// /// - The child is already the best child but it's now invalid due to a FFG change and should be removed. /// - The child is already the best child and the parent is updated with the new - /// best-descendant. + /// best-descendant. /// - The child is not the best child but becomes the best child. /// - The child is not the best child and does not become the best child. fn maybe_update_best_child_and_descendant( diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 88d4660311..4da632bf58 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -1121,7 +1121,7 @@ mod test_compute_deltas { /// /// - `A` (slot 31) is the common descendant. /// - `B` (slot 33) descends from `A`, but there is a single skip slot - /// between it and `A`. + /// between it and `A`. /// - `C` (slot 32) descends from `A` and conflicts with `B`. /// /// Imagine that the `B` chain is finalized at epoch 1. This means that the diff --git a/consensus/types/src/sync_committee_contribution.rs b/consensus/types/src/sync_committee_contribution.rs index 9bae770fe5..090e16fc6d 100644 --- a/consensus/types/src/sync_committee_contribution.rs +++ b/consensus/types/src/sync_committee_contribution.rs @@ -41,8 +41,8 @@ impl SyncCommitteeContribution { /// /// - `message`: A single `SyncCommitteeMessage`. /// - `subcommittee_index`: The subcommittee this contribution pertains to out of the broader - /// sync committee. This can be determined from the `SyncSubnetId` of the gossip subnet - /// this message was seen on. + /// sync committee. This can be determined from the `SyncSubnetId` of the gossip subnet + /// this message was seen on. /// - `validator_sync_committee_index`: The index of the validator **within** the subcommittee. pub fn from_message( message: &SyncCommitteeMessage, diff --git a/consensus/types/src/test_utils/test_random/bitfield.rs b/consensus/types/src/test_utils/test_random/bitfield.rs index 35176d389d..e335ac7fe8 100644 --- a/consensus/types/src/test_utils/test_random/bitfield.rs +++ b/consensus/types/src/test_utils/test_random/bitfield.rs @@ -3,7 +3,7 @@ use smallvec::smallvec; impl TestRandom for BitList { fn random_for_test(rng: &mut impl RngCore) -> Self { - let initial_len = std::cmp::max(1, (N::to_usize() + 7) / 8); + let initial_len = std::cmp::max(1, N::to_usize().div_ceil(8)); let mut raw_bytes = smallvec![0; initial_len]; rng.fill_bytes(&mut raw_bytes); @@ -24,7 +24,7 @@ impl TestRandom for BitList { impl TestRandom for BitVector { fn random_for_test(rng: &mut impl RngCore) -> Self { - let mut raw_bytes = smallvec![0; std::cmp::max(1, (N::to_usize() + 7) / 8)]; + let mut raw_bytes = smallvec![0; std::cmp::max(1, N::to_usize().div_ceil(8))]; rng.fill_bytes(&mut raw_bytes); // If N isn't divisible by 8 // zero out bits greater than N diff --git a/crypto/bls/src/lib.rs b/crypto/bls/src/lib.rs index 6ea85548c0..d05b34f989 100644 --- a/crypto/bls/src/lib.rs +++ b/crypto/bls/src/lib.rs @@ -10,7 +10,7 @@ //! //! - `supranational`: the pure-assembly, highly optimized version from the `blst` crate. //! - `fake_crypto`: an always-returns-valid implementation that is only useful for testing -//! scenarios which intend to *ignore* real cryptography. +//! scenarios which intend to *ignore* real cryptography. //! //! This crate uses traits to reduce code-duplication between the two implementations. For example, //! the `GenericPublicKey` struct exported from this crate is generic across the `TPublicKey` trait diff --git a/testing/ef_tests/src/cases.rs b/testing/ef_tests/src/cases.rs index 4a202ee3d2..31662e831a 100644 --- a/testing/ef_tests/src/cases.rs +++ b/testing/ef_tests/src/cases.rs @@ -84,11 +84,11 @@ pub use transition::TransitionTest; /// /// The feature tests can be run with one of the following methods: /// 1. `handler.run_for_feature(feature_name)` for new tests that are not on existing fork, i.e. a -/// new handler. This will be temporary and the test will need to be updated to use -/// `handle.run()` once the feature is incorporated into a fork. +/// new handler. This will be temporary and the test will need to be updated to use +/// `handle.run()` once the feature is incorporated into a fork. /// 2. `handler.run()` for tests that are already on existing forks, but with new test vectors for -/// the feature. In this case the `handler.is_enabled_for_feature` will need to be implemented -/// to return `true` for the feature in order for the feature test vector to be tested. +/// the feature. In this case the `handler.is_enabled_for_feature` will need to be implemented +/// to return `true` for the feature in order for the feature test vector to be tested. #[derive(Debug, PartialEq, Clone, Copy)] pub enum FeatureName { // TODO(fulu): to be removed once we start using Fulu types for test vectors. diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 05804d7e36..c3835f425e 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -143,7 +143,7 @@ impl LoadCase for ForkChoiceTest { fn load_from_dir(path: &Path, fork_name: ForkName) -> Result { let description = path .iter() - .last() + .next_back() .expect("path must be non-empty") .to_str() .expect("path must be valid OsStr") diff --git a/validator_client/http_api/src/tests/keystores.rs b/validator_client/http_api/src/tests/keystores.rs index 6559a2bb9e..13494e5fa6 100644 --- a/validator_client/http_api/src/tests/keystores.rs +++ b/validator_client/http_api/src/tests/keystores.rs @@ -92,7 +92,7 @@ fn keystore_pubkey(keystore: &Keystore) -> PublicKeyBytes { } fn all_with_status(count: usize, status: T) -> impl Iterator { - std::iter::repeat(status).take(count) + std::iter::repeat_n(status, count) } fn all_imported(count: usize) -> impl Iterator { @@ -1059,7 +1059,7 @@ async fn migrate_some_extra_slashing_protection() { /// - `first_vc_attestations`: attestations to sign on the first VC as `(validator_idx, att)` /// - `delete_indices`: validators to delete from the first VC /// - `slashing_protection_indices`: validators to transfer slashing protection data for. It should -/// be a subset of `delete_indices` or the test will panic. +/// be a subset of `delete_indices` or the test will panic. /// - `import_indices`: validators to transfer. It needn't be a subset of `delete_indices`. /// - `second_vc_attestations`: attestations to sign on the second VC after the transfer. The bool /// indicates whether the signing should be successful. diff --git a/validator_client/validator_store/src/lib.rs b/validator_client/validator_store/src/lib.rs index 5bd9ffd8b2..5114000325 100644 --- a/validator_client/validator_store/src/lib.rs +++ b/validator_client/validator_store/src/lib.rs @@ -265,9 +265,9 @@ impl ValidatorStore { /// are two primary functions used here: /// /// - `DoppelgangerStatus::only_safe`: only returns pubkeys which have passed doppelganger - /// protection and are safe-enough to sign messages. + /// protection and are safe-enough to sign messages. /// - `DoppelgangerStatus::ignored`: returns all the pubkeys from `only_safe` *plus* those still - /// undergoing protection. This is useful for collecting duties or other non-signing tasks. + /// undergoing protection. This is useful for collecting duties or other non-signing tasks. #[allow(clippy::needless_collect)] // Collect is required to avoid holding a lock. pub fn voting_pubkeys(&self, filter_func: F) -> I where From 57abffcd997fc8842f6d357878c1ec23f89a2d3d Mon Sep 17 00:00:00 2001 From: Mac L Date: Fri, 4 Apr 2025 17:14:04 +1100 Subject: [PATCH 15/17] Disable log color when running in non-interactive mode (#7240) #7226 Checks whether the application is running in a terminal, or in non-interactive mode (e.g. using systemd). It will then set the value of `--log-color` to `false` when running non-interactively. --- lighthouse/src/main.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lighthouse/src/main.rs b/lighthouse/src/main.rs index 60e65e6470..66dae05326 100644 --- a/lighthouse/src/main.rs +++ b/lighthouse/src/main.rs @@ -20,6 +20,7 @@ use lighthouse_version::VERSION; use logging::{build_workspace_filter, crit, MetricsLayer}; use malloc_utils::configure_memory_allocator; use std::backtrace::Backtrace; +use std::io::IsTerminal; use std::path::PathBuf; use std::process::exit; use std::sync::LazyLock; @@ -521,10 +522,15 @@ fn run( let log_format = matches.get_one::("log-format"); - let log_color = matches - .get_one::("log-color") - .copied() - .unwrap_or(true); + let log_color = if std::io::stdin().is_terminal() { + matches + .get_one::("log-color") + .copied() + .unwrap_or(true) + } else { + // Disable color when in non-interactive mode. + false + }; let logfile_color = matches.get_flag("logfile-color"); From 6a75f24ab13e5659a7380bc93c1a3c7fc2c7012e Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Fri, 4 Apr 2025 20:01:39 +1100 Subject: [PATCH 16/17] Fix the `getBlobs` metric and ensure it is recorded promptly to prevent miscounts (#7188) From testing conducted by Sunnyside Labs, they noticed that the "expected blobs" are quite low on bandwidth constrained nodes. This observation revealed that we don't record the `beacon_blobs_from_el_expected_total` metric at all if the EL doesn't return any response. The fetch blobs function returns without recording the metric. To fix this, I've moved `BLOBS_FROM_EL_EXPECTED_TOTAL` and `BLOBS_FROM_EL_RECEIVED_TOTAL` to as early as possible, to make the metric more accurate. --- beacon_node/beacon_chain/src/fetch_blobs.rs | 25 ++++++++------------- beacon_node/beacon_chain/src/metrics.rs | 21 ++++++++++++----- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/beacon_node/beacon_chain/src/fetch_blobs.rs b/beacon_node/beacon_chain/src/fetch_blobs.rs index ceb563ffc2..3c28ac9a44 100644 --- a/beacon_node/beacon_chain/src/fetch_blobs.rs +++ b/beacon_node/beacon_chain/src/fetch_blobs.rs @@ -13,7 +13,7 @@ use crate::observed_data_sidecars::DoNotObserve; use crate::{metrics, AvailabilityProcessingStatus, BeaconChain, BeaconChainTypes, BlockError}; use execution_layer::json_structures::BlobAndProofV1; use execution_layer::Error as ExecutionLayerError; -use metrics::{inc_counter, inc_counter_by, TryExt}; +use metrics::{inc_counter, TryExt}; use ssz_types::FixedVector; use state_processing::per_block_processing::deneb::kzg_commitment_to_versioned_hash; use std::sync::Arc; @@ -73,13 +73,20 @@ pub async fn fetch_and_process_engine_blobs( .as_ref() .ok_or(FetchEngineBlobError::ExecutionLayerMissing)?; + metrics::observe(&metrics::BLOBS_FROM_EL_EXPECTED, num_expected_blobs as f64); debug!(num_expected_blobs, "Fetching blobs from the EL"); let response = execution_layer .get_blobs(versioned_hashes) .await + .inspect_err(|_| { + inc_counter(&metrics::BLOBS_FROM_EL_ERROR_TOTAL); + }) .map_err(FetchEngineBlobError::RequestFailed)?; - if response.is_empty() || response.iter().all(|opt| opt.is_none()) { + let num_fetched_blobs = response.iter().filter(|opt| opt.is_some()).count(); + metrics::observe(&metrics::BLOBS_FROM_EL_RECEIVED, num_fetched_blobs as f64); + + if num_fetched_blobs == 0 { debug!(num_expected_blobs, "No blobs fetched from the EL"); inc_counter(&metrics::BLOBS_FROM_EL_MISS_TOTAL); return Ok(None); @@ -99,20 +106,6 @@ pub async fn fetch_and_process_engine_blobs( &chain.spec, )?; - let num_fetched_blobs = fixed_blob_sidecar_list - .iter() - .filter(|b| b.is_some()) - .count(); - - inc_counter_by( - &metrics::BLOBS_FROM_EL_EXPECTED_TOTAL, - num_expected_blobs as u64, - ); - inc_counter_by( - &metrics::BLOBS_FROM_EL_RECEIVED_TOTAL, - num_fetched_blobs as u64, - ); - // Gossip verify blobs before publishing. This prevents blobs with invalid KZG proofs from // the EL making it into the data availability checker. We do not immediately add these // blobs to the observed blobs/columns cache because we want to allow blobs/columns to arrive on gossip diff --git a/beacon_node/beacon_chain/src/metrics.rs b/beacon_node/beacon_chain/src/metrics.rs index d1c7a2a5df..463319a1f5 100644 --- a/beacon_node/beacon_chain/src/metrics.rs +++ b/beacon_node/beacon_chain/src/metrics.rs @@ -1662,28 +1662,37 @@ pub static DATA_COLUMN_SIDECAR_GOSSIP_VERIFICATION_TIMES: LazyLock> = LazyLock::new(|| { try_create_int_counter( "beacon_blobs_from_el_hit_total", - "Number of blob batches fetched from the execution layer", + "Number of non-empty blob batches fetched from the execution layer", ) }); pub static BLOBS_FROM_EL_MISS_TOTAL: LazyLock> = LazyLock::new(|| { try_create_int_counter( "beacon_blobs_from_el_miss_total", - "Number of blob batches failed to fetch from the execution layer", + "Number of empty blob responses from the execution layer", ) }); -pub static BLOBS_FROM_EL_EXPECTED_TOTAL: LazyLock> = LazyLock::new(|| { +pub static BLOBS_FROM_EL_ERROR_TOTAL: LazyLock> = LazyLock::new(|| { try_create_int_counter( - "beacon_blobs_from_el_expected_total", + "beacon_blobs_from_el_error_total", + "Number of failed blob fetches from the execution layer", + ) +}); + +pub static BLOBS_FROM_EL_EXPECTED: LazyLock> = LazyLock::new(|| { + try_create_histogram_with_buckets( + "beacon_blobs_from_el_expected", "Number of blobs expected from the execution layer", + Ok(vec![0.0, 3.0, 6.0, 9.0, 12.0, 18.0, 24.0, 30.0]), ) }); -pub static BLOBS_FROM_EL_RECEIVED_TOTAL: LazyLock> = LazyLock::new(|| { - try_create_int_counter( +pub static BLOBS_FROM_EL_RECEIVED: LazyLock> = LazyLock::new(|| { + try_create_histogram_with_buckets( "beacon_blobs_from_el_received_total", "Number of blobs fetched from the execution layer", + linear_buckets(0.0, 4.0, 20), ) }); From 7cc64cab8352b9f8f82076f5f71fc7a7a08e3376 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Fri, 4 Apr 2025 20:01:42 +1100 Subject: [PATCH 17/17] Add missing error log and remove redundant id field from lookup logs (#6990) Partially #6989. This PR adds the missing error log when a batch fails due to issues with converting the response into `RpcBlock`. See the above linked issue for more details. Adding this log reveals that we're completing range requests with missing columns, hence causing the batch to fail. It looks like we've hit the case where we've received enough stream terminations, but not all columns are returned. ``` Feb 12 06:12:16.558 DEBG Failed to convert range block components into RpcBlock, error: No column for block 0xc5b6c7fa02f5ef603d45819c08c6519f1dba661fd5d44a2fc849d3e7028b6007 index 18, id: 3456/RangeSync/116/3432, service: sync, module: network::sync::network_context:488 ``` I've also removed some redundant `id` logging, as the `id` debug representation is difficult to read, and is now being logged as part of `req_id` in a more succinct format (relevant PR: #6914) --- beacon_node/network/src/sync/network_context/custody.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/beacon_node/network/src/sync/network_context/custody.rs b/beacon_node/network/src/sync/network_context/custody.rs index 018381a850..e7e6e62349 100644 --- a/beacon_node/network/src/sync/network_context/custody.rs +++ b/beacon_node/network/src/sync/network_context/custody.rs @@ -102,7 +102,6 @@ impl ActiveCustodyRequest { ) -> CustodyRequestResult { let Some(batch_request) = self.active_batch_columns_requests.get_mut(&req_id) else { warn!( - id = ?self.custody_id, block_root = ?self.block_root, %req_id, "Received custody column response for unrequested index" @@ -113,7 +112,6 @@ impl ActiveCustodyRequest { match resp { Ok((data_columns, seen_timestamp)) => { debug!( - id = ?self.custody_id, block_root = ?self.block_root, %req_id, %peer_id, @@ -161,7 +159,6 @@ impl ActiveCustodyRequest { if !missing_column_indexes.is_empty() { // Note: Batch logging that columns are missing to not spam logger debug!( - id = ?self.custody_id, block_root = ?self.block_root, %req_id, %peer_id, @@ -175,7 +172,6 @@ impl ActiveCustodyRequest { } Err(err) => { debug!( - id = ?self.custody_id, block_root = ?self.block_root, %req_id, %peer_id,