From 234b100a95e8e2bfda4a5cafb3e8d27d6042d5d9 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Thu, 27 Jun 2024 19:28:48 +0200 Subject: [PATCH 1/2] Bound max count of lookups --- .../network/src/sync/block_lookups/mod.rs | 37 ++++++++++++------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index f685b7e59db..42cb5a0ea8a 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -45,6 +45,12 @@ const LOOKUP_MAX_DURATION_STUCK_SECS: u64 = 15 * PARENT_DEPTH_TOLERANCE as u64; /// lookup at most after 4 seconds, the lookup should gain peers. const LOOKUP_MAX_DURATION_NO_PEERS_SECS: u64 = 10; +/// Lookups contain untrusted data, including blocks that have not yet been validated. In case of +/// bugs or malicious activity we want to bound how much memory these lookups can consume. Aprox the +/// max size of a lookup is ~ 10 MB (current max size of gossip and RPC blocks). 200 lookups can +/// take at most 2 GB. 200 lookups allow 3 parallel chains of depth 64 (current maximum). +const MAX_LOOKUPS: usize = 200; + pub enum BlockComponent { Block(DownloadResult>>), Blob(DownloadResult>>), @@ -292,25 +298,18 @@ impl BlockLookups { // signal here to hold processing downloaded data. let mut lookup = SingleBlockLookup::new(block_root, peers, cx.next_id(), awaiting_parent); - let msg = if block_component.is_some() { - "Searching for components of a block with unknown parent" - } else { - "Searching for block components" - }; - debug!( - self.log, - "{}", msg; - "peer_ids" => ?peers, - "block_root" => ?block_root, - "id" => lookup.id, - ); - metrics::inc_counter(&metrics::SYNC_LOOKUP_CREATED); - // Add block components to the new request if let Some(block_component) = block_component { lookup.add_child_components(block_component); } + // Lookups contain untrusted data, bound the total count of lookups hold in memory to reduce + // the risk of OOM in case of bugs of malicious activity. + if self.single_block_lookups.len() > MAX_LOOKUPS { + warn!(self.log, "Dropping lookup reached max"; "block_root" => ?block_root); + return false; + } + let id = lookup.id; let lookup = match self.single_block_lookups.entry(id) { Entry::Vacant(entry) => entry.insert(lookup), @@ -321,6 +320,16 @@ impl BlockLookups { } }; + debug!( + self.log, + "Created block lookup"; + "peer_ids" => ?peers, + "block_root" => ?block_root, + "awaiting_parent" => awaiting_parent.map(|root| root.to_string()).unwrap_or("none".to_owned()), + "id" => lookup.id, + ); + metrics::inc_counter(&metrics::SYNC_LOOKUP_CREATED); + let result = lookup.continue_requests(cx); if self.on_lookup_result(id, result, "new_current_lookup", cx) { self.update_metrics(); From 8f346f9841b70730c1d3a4c3d1184ce2e5e3e539 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Fri, 12 Jul 2024 13:11:05 +0200 Subject: [PATCH 2/2] Move up --- beacon_node/network/src/sync/block_lookups/mod.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index 42cb5a0ea8a..8c894cb7849 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -294,6 +294,13 @@ impl BlockLookups { } } + // Lookups contain untrusted data, bound the total count of lookups hold in memory to reduce + // the risk of OOM in case of bugs of malicious activity. + if self.single_block_lookups.len() > MAX_LOOKUPS { + warn!(self.log, "Dropping lookup reached max"; "block_root" => ?block_root); + return false; + } + // If we know that this lookup has unknown parent (is awaiting a parent lookup to resolve), // signal here to hold processing downloaded data. let mut lookup = SingleBlockLookup::new(block_root, peers, cx.next_id(), awaiting_parent); @@ -303,13 +310,6 @@ impl BlockLookups { lookup.add_child_components(block_component); } - // Lookups contain untrusted data, bound the total count of lookups hold in memory to reduce - // the risk of OOM in case of bugs of malicious activity. - if self.single_block_lookups.len() > MAX_LOOKUPS { - warn!(self.log, "Dropping lookup reached max"; "block_root" => ?block_root); - return false; - } - let id = lookup.id; let lookup = match self.single_block_lookups.entry(id) { Entry::Vacant(entry) => entry.insert(lookup),