From 5ca909cc09ca039d0fa4d61b02efb7ab85828c7c Mon Sep 17 00:00:00 2001 From: ordian Date: Mon, 23 Oct 2023 16:22:37 +0200 Subject: [PATCH] polkadot: eradicate `LeafStatus` (#1565) Fixes #768. --- Cargo.lock | 3 -- .../relay-chain-minimal-node/Cargo.toml | 1 - .../src/collator_overseer.rs | 4 +-- .../node/core/bitfield-signing/src/lib.rs | 14 ++------- polkadot/node/core/provisioner/src/lib.rs | 12 ++----- .../src/requester/mod.rs | 5 ++- polkadot/node/overseer/Cargo.toml | 1 - polkadot/node/overseer/src/dummy.rs | 3 -- polkadot/node/overseer/src/lib.rs | 31 ++++++------------- polkadot/node/service/Cargo.toml | 1 - polkadot/node/service/src/overseer.rs | 4 --- .../node/subsystem-test-helpers/src/mock.rs | 3 +- polkadot/node/subsystem-types/src/lib.rs | 31 ------------------- polkadot/node/subsystem/src/lib.rs | 2 +- .../src/types/overseer-protocol.md | 12 +------ 15 files changed, 21 insertions(+), 106 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5dcca22dd0..90fda2d0ec 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3892,7 +3892,6 @@ dependencies = [ "sc-service", "sc-tracing", "sc-utils", - "schnellru", "sp-api", "sp-consensus", "sp-consensus-babe", @@ -12527,7 +12526,6 @@ dependencies = [ "polkadot-primitives-test-helpers", "prioritized-metered-channel", "sc-client-api", - "schnellru", "sp-api", "sp-core", "tikv-jemalloc-ctl", @@ -12914,7 +12912,6 @@ dependencies = [ "sc-telemetry", "sc-transaction-pool", "sc-transaction-pool-api", - "schnellru", "serde", "serde_json", "serial_test", diff --git a/cumulus/client/relay-chain-minimal-node/Cargo.toml b/cumulus/client/relay-chain-minimal-node/Cargo.toml index 5188986594..f132b1a765 100644 --- a/cumulus/client/relay-chain-minimal-node/Cargo.toml +++ b/cumulus/client/relay-chain-minimal-node/Cargo.toml @@ -37,7 +37,6 @@ cumulus-relay-chain-rpc-interface = { path = "../relay-chain-rpc-interface" } cumulus-primitives-core = { path = "../../primitives/core" } array-bytes = "6.1" -schnellru = "0.2.1" tracing = "0.1.37" async-trait = "0.1.73" futures = "0.3.28" diff --git a/cumulus/client/relay-chain-minimal-node/src/collator_overseer.rs b/cumulus/client/relay-chain-minimal-node/src/collator_overseer.rs index 49332cc350..216ca0d396 100644 --- a/cumulus/client/relay-chain-minimal-node/src/collator_overseer.rs +++ b/cumulus/client/relay-chain-minimal-node/src/collator_overseer.rs @@ -15,7 +15,6 @@ // along with Polkadot. If not, see . use futures::{select, StreamExt}; -use schnellru::{ByLength, LruMap}; use std::sync::Arc; use polkadot_availability_recovery::AvailabilityRecoverySubsystem; @@ -36,7 +35,7 @@ use polkadot_node_network_protocol::{ use polkadot_node_subsystem_util::metrics::{prometheus::Registry, Metrics}; use polkadot_overseer::{ BlockInfo, DummySubsystem, Handle, Overseer, OverseerConnector, OverseerHandle, SpawnGlue, - UnpinHandle, KNOWN_LEAVES_CACHE_SIZE, + UnpinHandle, }; use polkadot_primitives::CollatorPair; @@ -156,7 +155,6 @@ fn build_overseer( .span_per_active_leaf(Default::default()) .active_leaves(Default::default()) .supports_parachains(runtime_client) - .known_leaves(LruMap::new(ByLength::new(KNOWN_LEAVES_CACHE_SIZE))) .metrics(Metrics::register(registry)?) .spawner(spawner); diff --git a/polkadot/node/core/bitfield-signing/src/lib.rs b/polkadot/node/core/bitfield-signing/src/lib.rs index f29e827e10..0fc0bb3d27 100644 --- a/polkadot/node/core/bitfield-signing/src/lib.rs +++ b/polkadot/node/core/bitfield-signing/src/lib.rs @@ -32,8 +32,8 @@ use polkadot_node_subsystem::{ messages::{ AvailabilityStoreMessage, BitfieldDistributionMessage, RuntimeApiMessage, RuntimeApiRequest, }, - overseer, ActivatedLeaf, FromOrchestra, LeafStatus, OverseerSignal, PerLeafSpan, - SpawnedSubsystem, SubsystemError, SubsystemResult, SubsystemSender, + overseer, ActivatedLeaf, FromOrchestra, OverseerSignal, PerLeafSpan, SpawnedSubsystem, + SubsystemError, SubsystemResult, SubsystemSender, }; use polkadot_node_subsystem_util::{self as util, Validator}; use polkadot_primitives::{AvailabilityBitfield, CoreState, Hash, ValidatorIndex}; @@ -257,16 +257,6 @@ async fn handle_active_leaves_update( where Sender: overseer::BitfieldSigningSenderTrait, { - if let LeafStatus::Stale = leaf.status { - gum::debug!( - target: LOG_TARGET, - relay_parent = ?leaf.hash, - block_number = ?leaf.number, - "Skip bitfield signing for stale leaf" - ); - return Ok(()) - } - let span = PerLeafSpan::new(leaf.span, "bitfield-signing"); let span_delay = span.child("delay"); let wait_until = Instant::now() + SPAWNED_TASK_DELAY; diff --git a/polkadot/node/core/provisioner/src/lib.rs b/polkadot/node/core/provisioner/src/lib.rs index f81e5550b1..8893bdc654 100644 --- a/polkadot/node/core/provisioner/src/lib.rs +++ b/polkadot/node/core/provisioner/src/lib.rs @@ -31,8 +31,8 @@ use polkadot_node_subsystem::{ CandidateBackingMessage, ChainApiMessage, ProspectiveParachainsMessage, ProvisionableData, ProvisionerInherentData, ProvisionerMessage, RuntimeApiMessage, RuntimeApiRequest, }, - overseer, ActivatedLeaf, ActiveLeavesUpdate, FromOrchestra, LeafStatus, OverseerSignal, - PerLeafSpan, RuntimeApiError, SpawnedSubsystem, SubsystemError, + overseer, ActivatedLeaf, ActiveLeavesUpdate, FromOrchestra, OverseerSignal, PerLeafSpan, + RuntimeApiError, SpawnedSubsystem, SubsystemError, }; use polkadot_node_subsystem_util::{ request_availability_cores, request_persisted_validation_data, @@ -421,13 +421,7 @@ async fn send_inherent_data( "Selected disputes" ); - // Only include bitfields on fresh leaves. On chain reversions, we want to make sure that - // there will be at least one block, which cannot get disputed, so the chain can make progress. - let bitfields = match leaf.status { - LeafStatus::Fresh => - select_availability_bitfields(&availability_cores, bitfields, &leaf.hash), - LeafStatus::Stale => Vec::new(), - }; + let bitfields = select_availability_bitfields(&availability_cores, bitfields, &leaf.hash); gum::trace!( target: LOG_TARGET, diff --git a/polkadot/node/network/availability-distribution/src/requester/mod.rs b/polkadot/node/network/availability-distribution/src/requester/mod.rs index 446988f7cc..97e80d696e 100644 --- a/polkadot/node/network/availability-distribution/src/requester/mod.rs +++ b/polkadot/node/network/availability-distribution/src/requester/mod.rs @@ -35,7 +35,7 @@ use futures::{ use polkadot_node_subsystem::{ jaeger, messages::{ChainApiMessage, RuntimeApiMessage}, - overseer, ActivatedLeaf, ActiveLeavesUpdate, LeafStatus, + overseer, ActivatedLeaf, ActiveLeavesUpdate, }; use polkadot_node_subsystem_util::runtime::{get_occupied_cores, RuntimeInfo}; use polkadot_primitives::{CandidateHash, Hash, OccupiedCore, SessionIndex}; @@ -105,8 +105,7 @@ impl Requester { ) -> Result<()> { gum::trace!(target: LOG_TARGET, ?update, "Update fetching heads"); let ActiveLeavesUpdate { activated, deactivated } = update; - // Stale leaves happen after a reversion - we don't want to re-run availability there. - if let Some(leaf) = activated.filter(|leaf| leaf.status == LeafStatus::Fresh) { + if let Some(leaf) = activated { let span = spans .get(&leaf.hash) .map(|span| span.child("update-fetching-heads")) diff --git a/polkadot/node/overseer/Cargo.toml b/polkadot/node/overseer/Cargo.toml index 5d41407ef8..376ebe0375 100644 --- a/polkadot/node/overseer/Cargo.toml +++ b/polkadot/node/overseer/Cargo.toml @@ -18,7 +18,6 @@ polkadot-node-metrics = { path = "../metrics" } polkadot-primitives = { path = "../../primitives" } orchestra = { version = "0.3.3", default-features = false, features=["futures_channel"] } gum = { package = "tracing-gum", path = "../gum" } -schnellru = "0.2.1" sp-core = { path = "../../../substrate/primitives/core" } async-trait = "0.1.57" tikv-jemalloc-ctl = { version = "0.5.0", optional = true } diff --git a/polkadot/node/overseer/src/dummy.rs b/polkadot/node/overseer/src/dummy.rs index fea96e5dba..fc5f007077 100644 --- a/polkadot/node/overseer/src/dummy.rs +++ b/polkadot/node/overseer/src/dummy.rs @@ -17,11 +17,9 @@ use crate::{ prometheus::Registry, HeadSupportsParachains, InitializedOverseerBuilder, MetricsTrait, Overseer, OverseerMetrics, OverseerSignal, OverseerSubsystemContext, SpawnGlue, - KNOWN_LEAVES_CACHE_SIZE, }; use orchestra::{FromOrchestra, SpawnedSubsystem, Subsystem, SubsystemContext}; use polkadot_node_subsystem_types::{errors::SubsystemError, messages::*}; -use schnellru::{ByLength, LruMap}; // Generated dummy messages use crate::messages::*; @@ -193,7 +191,6 @@ where .activation_external_listeners(Default::default()) .span_per_active_leaf(Default::default()) .active_leaves(Default::default()) - .known_leaves(LruMap::new(ByLength::new(KNOWN_LEAVES_CACHE_SIZE))) .spawner(SpawnGlue(spawner)) .metrics(metrics) .supports_parachains(supports_parachains); diff --git a/polkadot/node/overseer/src/lib.rs b/polkadot/node/overseer/src/lib.rs index 673569ab94..6802426d3c 100644 --- a/polkadot/node/overseer/src/lib.rs +++ b/polkadot/node/overseer/src/lib.rs @@ -70,7 +70,6 @@ use std::{ }; use futures::{channel::oneshot, future::BoxFuture, select, Future, FutureExt, StreamExt}; -use schnellru::LruMap; use client::{BlockImportNotification, BlockchainEvents, FinalityNotification}; use polkadot_primitives::{Block, BlockNumber, Hash}; @@ -88,8 +87,8 @@ use polkadot_node_subsystem_types::messages::{ pub use polkadot_node_subsystem_types::{ errors::{SubsystemError, SubsystemResult}, - jaeger, ActivatedLeaf, ActiveLeavesUpdate, LeafStatus, OverseerSignal, - RuntimeApiSubsystemClient, UnpinHandle, + jaeger, ActivatedLeaf, ActiveLeavesUpdate, OverseerSignal, RuntimeApiSubsystemClient, + UnpinHandle, }; pub mod metrics; @@ -112,10 +111,6 @@ pub use orchestra::{ SubsystemSender, TimeoutExt, ToOrchestra, TrySendError, }; -/// Store 2 days worth of blocks, not accounting for forks, -/// in the LRU cache. Assumes a 6-second block time. -pub const KNOWN_LEAVES_CACHE_SIZE: u32 = 2 * 24 * 3600 / 6; - #[cfg(any(target_os = "linux", feature = "jemalloc-allocator"))] mod memory_stats; #[cfg(test)] @@ -283,6 +278,11 @@ impl From> for BlockInfo { /// as the substrate framework or user interaction. pub enum Event { /// A new block was imported. + /// + /// This event is not sent if the block was already known + /// and we reorged to it e.g. due to a reversion. + /// + /// Also, these events are not sent during a major sync. BlockImported(BlockInfo), /// A block was finalized with i.e. babe or another consensus algorithm. BlockFinalized(BlockInfo), @@ -641,9 +641,6 @@ pub struct Overseer { /// An implementation for checking whether a header supports parachain consensus. pub supports_parachains: SupportsParachains, - /// An LRU cache for keeping track of relay-chain heads that have already been seen. - pub known_leaves: LruMap, - /// Various Prometheus metrics. pub metrics: OverseerMetrics, } @@ -802,10 +799,9 @@ where }; let mut update = match self.on_head_activated(&block.hash, Some(block.parent_hash)).await { - Some((span, status)) => ActiveLeavesUpdate::start_work(ActivatedLeaf { + Some(span) => ActiveLeavesUpdate::start_work(ActivatedLeaf { hash: block.hash, number: block.number, - status, unpin_handle: block.unpin_handle, span, }), @@ -864,7 +860,7 @@ where &mut self, hash: &Hash, parent_hash: Option, - ) -> Option<(Arc, LeafStatus)> { + ) -> Option> { if !self.supports_parachains.head_supports_parachains(hash).await { return None } @@ -891,14 +887,7 @@ where let span = Arc::new(span); self.span_per_active_leaf.insert(*hash, span.clone()); - let status = if self.known_leaves.get(hash).is_some() { - LeafStatus::Stale - } else { - self.known_leaves.insert(*hash, ()); - LeafStatus::Fresh - }; - - Some((span, status)) + Some(span) } fn on_head_deactivated(&mut self, hash: &Hash) { diff --git a/polkadot/node/service/Cargo.toml b/polkadot/node/service/Cargo.toml index ba5976fdce..899a95dd3a 100644 --- a/polkadot/node/service/Cargo.toml +++ b/polkadot/node/service/Cargo.toml @@ -86,7 +86,6 @@ parity-db = { version = "0.4.8", optional = true } codec = { package = "parity-scale-codec", version = "3.6.1" } async-trait = "0.1.57" -schnellru = "0.2.1" log = "0.4.17" is_executable = "1.0.1" diff --git a/polkadot/node/service/src/overseer.rs b/polkadot/node/service/src/overseer.rs index 7d1add1182..fd618863ee 100644 --- a/polkadot/node/service/src/overseer.rs +++ b/polkadot/node/service/src/overseer.rs @@ -40,7 +40,6 @@ use polkadot_overseer::{ metrics::Metrics as OverseerMetrics, InitializedOverseerBuilder, MetricsTrait, Overseer, OverseerConnector, OverseerHandle, SpawnGlue, }; -use schnellru::{ByLength, LruMap}; use polkadot_primitives::runtime_api::ParachainHost; use sc_authority_discovery::Service as AuthorityDiscoveryService; @@ -342,7 +341,6 @@ where .span_per_active_leaf(Default::default()) .active_leaves(Default::default()) .supports_parachains(runtime_api_client) - .known_leaves(LruMap::new(ByLength::new(KNOWN_LEAVES_CACHE_SIZE))) .metrics(metrics) .spawner(spawner); @@ -380,8 +378,6 @@ pub trait OverseerGen { // as consequence make this rather annoying to implement and use. } -use polkadot_overseer::KNOWN_LEAVES_CACHE_SIZE; - /// The regular set of subsystems. pub struct RealOverseerGen; diff --git a/polkadot/node/subsystem-test-helpers/src/mock.rs b/polkadot/node/subsystem-test-helpers/src/mock.rs index 35d74e27c9..522bc3c2cc 100644 --- a/polkadot/node/subsystem-test-helpers/src/mock.rs +++ b/polkadot/node/subsystem-test-helpers/src/mock.rs @@ -16,7 +16,7 @@ use std::sync::Arc; -use polkadot_node_subsystem::{jaeger, ActivatedLeaf, LeafStatus}; +use polkadot_node_subsystem::{jaeger, ActivatedLeaf}; use sc_client_api::UnpinHandle; use sc_keystore::LocalKeystore; use sc_utils::mpsc::tracing_unbounded; @@ -55,7 +55,6 @@ pub fn new_leaf(hash: Hash, number: BlockNumber) -> ActivatedLeaf { ActivatedLeaf { hash, number, - status: LeafStatus::Fresh, unpin_handle: dummy_unpin_handle(hash), span: Arc::new(jaeger::Span::Disabled), } diff --git a/polkadot/node/subsystem-types/src/lib.rs b/polkadot/node/subsystem-types/src/lib.rs index 02651ace1e..e3d6e4decf 100644 --- a/polkadot/node/subsystem-types/src/lib.rs +++ b/polkadot/node/subsystem-types/src/lib.rs @@ -51,35 +51,6 @@ pub use polkadot_node_jaeger as jaeger; /// If there are greater than this number of slots, then we fall back to a heap vector. const ACTIVE_LEAVES_SMALLVEC_CAPACITY: usize = 8; -/// The status of an activated leaf. -#[derive(Clone, Debug, PartialEq)] -pub enum LeafStatus { - /// A leaf is fresh when it's the first time the leaf has been encountered. - /// Most leaves should be fresh. - Fresh, - /// A leaf is stale when it's encountered for a subsequent time. This will happen - /// when the chain is reverted or the fork-choice rule abandons some chain. - Stale, -} - -impl LeafStatus { - /// Returns a `bool` indicating fresh status. - pub fn is_fresh(&self) -> bool { - match *self { - LeafStatus::Fresh => true, - LeafStatus::Stale => false, - } - } - - /// Returns a `bool` indicating stale status. - pub fn is_stale(&self) -> bool { - match *self { - LeafStatus::Fresh => false, - LeafStatus::Stale => true, - } - } -} - /// Activated leaf. #[derive(Debug, Clone)] pub struct ActivatedLeaf { @@ -87,8 +58,6 @@ pub struct ActivatedLeaf { pub hash: Hash, /// The block number. pub number: BlockNumber, - /// The status of the leaf. - pub status: LeafStatus, /// A handle to unpin the block on drop. pub unpin_handle: UnpinHandle, /// An associated [`jaeger::Span`]. diff --git a/polkadot/node/subsystem/src/lib.rs b/polkadot/node/subsystem/src/lib.rs index df379f2d97..8b407c75a0 100644 --- a/polkadot/node/subsystem/src/lib.rs +++ b/polkadot/node/subsystem/src/lib.rs @@ -28,7 +28,7 @@ pub use polkadot_overseer::{self as overseer, *}; pub use polkadot_node_subsystem_types::{ errors::{self, *}, - ActivatedLeaf, LeafStatus, + ActivatedLeaf, }; /// Re-export of all messages type, including the wrapper type. diff --git a/polkadot/roadmap/implementers-guide/src/types/overseer-protocol.md b/polkadot/roadmap/implementers-guide/src/types/overseer-protocol.md index fababbff35..54cdc2edd1 100644 --- a/polkadot/roadmap/implementers-guide/src/types/overseer-protocol.md +++ b/polkadot/roadmap/implementers-guide/src/types/overseer-protocol.md @@ -32,18 +32,8 @@ Indicates a change in active leaves. Activated leaves should have jobs, whereas winding-down of work based on those leaves. ```rust -enum LeafStatus { - // A leaf is fresh when it's the first time the leaf has been encountered. - // Most leaves should be fresh. - Fresh, - // A leaf is stale when it's encountered for a subsequent time. This will - // happen when the chain is reverted or the fork-choice rule abandons some - // chain. - Stale, -} - struct ActiveLeavesUpdate { - activated: [(Hash, Number, LeafStatus)], // in practice, these should probably be a SmallVec + activated: [(Hash, Number)], deactivated: [Hash], } ```