Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

polkadot: eradicate LeafStatus #1565

Merged
merged 2 commits into from
Oct 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion cumulus/client/relay-chain-minimal-node/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>.

use futures::{select, StreamExt};
use schnellru::{ByLength, LruMap};
use std::sync::Arc;

use polkadot_availability_recovery::AvailabilityRecoverySubsystem;
Expand All @@ -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;

Expand Down Expand Up @@ -158,7 +157,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);

Expand Down
14 changes: 2 additions & 12 deletions polkadot/node/core/bitfield-signing/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -257,16 +257,6 @@ async fn handle_active_leaves_update<Sender>(
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;
Expand Down
12 changes: 3 additions & 9 deletions polkadot/node/core/provisioner/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Comment on lines -424 to -425
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do we ensure that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By not including bitfields in the very first block after a reversion. If there are no bitfields, nothing can become included as of that block, hence it can not become a reversion target gain.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the already existing leaf we are reverting to already exists, the stale/fresh mechanism should not be necessary to achieve this as we should not get that leaf activated again, hence nothing in parachain consensus is supposed to do anything -> paras inherent should be empty/not existing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! So it works already 👍

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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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"))
Expand Down
1 change: 0 additions & 1 deletion polkadot/node/overseer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
3 changes: 0 additions & 3 deletions polkadot/node/overseer/src/dummy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;

Expand Down Expand Up @@ -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);
Expand Down
31 changes: 10 additions & 21 deletions polkadot/node/overseer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,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};
Expand All @@ -86,8 +85,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;
Expand All @@ -110,10 +109,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)]
Expand Down Expand Up @@ -281,6 +276,11 @@ impl From<FinalityNotification<Block>> 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),
Expand Down Expand Up @@ -639,9 +639,6 @@ pub struct Overseer<SupportsParachains> {
/// 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<Hash, ()>,

/// Various Prometheus metrics.
pub metrics: OverseerMetrics,
}
Expand Down Expand Up @@ -800,10 +797,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,
}),
Expand Down Expand Up @@ -862,7 +858,7 @@ where
&mut self,
hash: &Hash,
parent_hash: Option<Hash>,
) -> Option<(Arc<jaeger::Span>, LeafStatus)> {
) -> Option<Arc<jaeger::Span>> {
if !self.supports_parachains.head_supports_parachains(hash).await {
return None
}
Expand All @@ -889,14 +885,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) {
Expand Down
1 change: 0 additions & 1 deletion polkadot/node/service/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
4 changes: 0 additions & 4 deletions polkadot/node/service/src/overseer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -344,7 +343,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);

Expand Down Expand Up @@ -382,8 +380,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;

Expand Down
3 changes: 1 addition & 2 deletions polkadot/node/subsystem-test-helpers/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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),
}
Expand Down
31 changes: 0 additions & 31 deletions polkadot/node/subsystem-types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,44 +51,13 @@ 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 {
/// The block hash.
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`].
Expand Down
2 changes: 1 addition & 1 deletion polkadot/node/subsystem/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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],
}
```
Expand Down