Unverified Commit a64476f1 authored by Bernhard Schuster's avatar Bernhard Schuster Committed by GitHub
Browse files

make chain selection typed and more explicit in naming and logging (#4345)

* minor: assure conditions match

This simplifies visual integrity checks that an overseer is connected
when it has to be.

* fix: avoid printing a misleading log in case of the disabled disputes feature

* chore: comments

* add expressive types for the selection algorithm
parent 64e1616f
Pipeline #168565 failed with stages
in 45 minutes and 29 seconds
...@@ -719,15 +719,21 @@ where ...@@ -719,15 +719,21 @@ where
let chain_spec = config.chain_spec.cloned_box(); let chain_spec = config.chain_spec.cloned_box();
let local_keystore = basics.keystore_container.local_keystore(); let local_keystore = basics.keystore_container.local_keystore();
let requires_overseer_for_chain_sel = let auth_or_collator = role.is_authority() || is_collator.is_collator();
local_keystore.is_some() && (role.is_authority() || is_collator.is_collator()); let requires_overseer_for_chain_sel = local_keystore.is_some() && auth_or_collator;
let select_chain = SelectRelayChain::new( let select_chain = if requires_overseer_for_chain_sel {
basics.backend.clone(), let metrics =
overseer_handle.clone(), polkadot_node_subsystem_util::metrics::Metrics::register(prometheus_registry.as_ref())?;
requires_overseer_for_chain_sel,
polkadot_node_subsystem_util::metrics::Metrics::register(prometheus_registry.as_ref())?, SelectRelayChain::new_disputes_aware(
); basics.backend.clone(),
overseer_handle.clone(),
metrics,
)
} else {
SelectRelayChain::new_longest_chain(basics.backend.clone())
};
let service::PartialComponents::<_, _, SelectRelayChain<_>, _, _, _> { let service::PartialComponents::<_, _, SelectRelayChain<_>, _, _, _> {
client, client,
...@@ -876,7 +882,7 @@ where ...@@ -876,7 +882,7 @@ where
let active_leaves = let active_leaves =
futures::executor::block_on(active_leaves(select_chain.as_longest_chain(), &*client))?; futures::executor::block_on(active_leaves(select_chain.as_longest_chain(), &*client))?;
let authority_discovery_service = if role.is_authority() || is_collator.is_collator() { let authority_discovery_service = if auth_or_collator {
use futures::StreamExt; use futures::StreamExt;
use sc_network::Event; use sc_network::Event;
......
...@@ -108,11 +108,31 @@ impl Metrics { ...@@ -108,11 +108,31 @@ impl Metrics {
} }
} }
/// Determines whether the chain is a relay chain
/// and hence has to take approval votes and disputes
/// into account.
enum IsDisputesAwareWithOverseer<B: sc_client_api::Backend<PolkadotBlock>> {
Yes(SelectRelayChainInner<B, Handle>),
No,
}
impl<B> Clone for IsDisputesAwareWithOverseer<B>
where
B: sc_client_api::Backend<PolkadotBlock>,
SelectRelayChainInner<B, Handle>: Clone,
{
fn clone(&self) -> Self {
match self {
Self::Yes(ref inner) => Self::Yes(inner.clone()),
Self::No => Self::No,
}
}
}
/// A chain-selection implementation which provides safety for relay chains. /// A chain-selection implementation which provides safety for relay chains.
pub struct SelectRelayChain<B: sc_client_api::Backend<PolkadotBlock>> { pub struct SelectRelayChain<B: sc_client_api::Backend<PolkadotBlock>> {
is_relay_chain: bool,
longest_chain: sc_consensus::LongestChain<B, PolkadotBlock>, longest_chain: sc_consensus::LongestChain<B, PolkadotBlock>,
selection: SelectRelayChainInner<B, Handle>, selection: IsDisputesAwareWithOverseer<B>,
} }
impl<B> Clone for SelectRelayChain<B> impl<B> Clone for SelectRelayChain<B>
...@@ -121,11 +141,7 @@ where ...@@ -121,11 +141,7 @@ where
SelectRelayChainInner<B, Handle>: Clone, SelectRelayChainInner<B, Handle>: Clone,
{ {
fn clone(&self) -> Self { fn clone(&self) -> Self {
Self { Self { longest_chain: self.longest_chain.clone(), selection: self.selection.clone() }
longest_chain: self.longest_chain.clone(),
is_relay_chain: self.is_relay_chain,
selection: self.selection.clone(),
}
} }
} }
...@@ -133,18 +149,35 @@ impl<B> SelectRelayChain<B> ...@@ -133,18 +149,35 @@ impl<B> SelectRelayChain<B>
where where
B: sc_client_api::Backend<PolkadotBlock> + 'static, B: sc_client_api::Backend<PolkadotBlock> + 'static,
{ {
/// Use the plain longest chain algorithm exclusively.
pub fn new_longest_chain(backend: Arc<B>) -> Self {
tracing::debug!(target: LOG_TARGET, "Using {} chain selection algorithm", "longest");
Self {
longest_chain: sc_consensus::LongestChain::new(backend.clone()),
selection: IsDisputesAwareWithOverseer::No,
}
}
/// Create a new [`SelectRelayChain`] wrapping the given chain backend /// Create a new [`SelectRelayChain`] wrapping the given chain backend
/// and a handle to the overseer. /// and a handle to the overseer.
pub fn new(backend: Arc<B>, overseer: Handle, is_relay_chain: bool, metrics: Metrics) -> Self { pub fn new_disputes_aware(backend: Arc<B>, overseer: Handle, metrics: Metrics) -> Self {
tracing::debug!( tracing::debug!(
target: LOG_TARGET, target: LOG_TARGET,
"Using {} as chain selection algorithm", "Using {} chain selection algorithm",
if is_relay_chain { "dispute aware relay" } else { "longest" } if cfg!(feature = "disputes") {
"dispute aware relay"
} else {
// no disputes are queried, that logic is disabled
// in `fn finality_target_with_longest_chain`.
"short-circuited relay"
}
); );
SelectRelayChain { SelectRelayChain {
longest_chain: sc_consensus::LongestChain::new(backend.clone()), longest_chain: sc_consensus::LongestChain::new(backend.clone()),
selection: SelectRelayChainInner::new(backend, overseer, metrics), selection: IsDisputesAwareWithOverseer::Yes(SelectRelayChainInner::new(
is_relay_chain, backend, overseer, metrics,
)),
} }
} }
...@@ -160,18 +193,17 @@ where ...@@ -160,18 +193,17 @@ where
B: sc_client_api::Backend<PolkadotBlock> + 'static, B: sc_client_api::Backend<PolkadotBlock> + 'static,
{ {
async fn leaves(&self) -> Result<Vec<Hash>, ConsensusError> { async fn leaves(&self) -> Result<Vec<Hash>, ConsensusError> {
if !self.is_relay_chain { match self.selection {
return self.longest_chain.leaves().await IsDisputesAwareWithOverseer::Yes(ref selection) => selection.leaves().await,
IsDisputesAwareWithOverseer::No => self.longest_chain.leaves().await,
} }
self.selection.leaves().await
} }
async fn best_chain(&self) -> Result<PolkadotHeader, ConsensusError> { async fn best_chain(&self) -> Result<PolkadotHeader, ConsensusError> {
if !self.is_relay_chain { match self.selection {
return self.longest_chain.best_chain().await IsDisputesAwareWithOverseer::Yes(ref selection) => selection.best_chain().await,
IsDisputesAwareWithOverseer::No => self.longest_chain.best_chain().await,
} }
self.selection.best_chain().await
} }
async fn finality_target( async fn finality_target(
...@@ -182,12 +214,17 @@ where ...@@ -182,12 +214,17 @@ where
let longest_chain_best = let longest_chain_best =
self.longest_chain.finality_target(target_hash, maybe_max_number).await?; self.longest_chain.finality_target(target_hash, maybe_max_number).await?;
if !self.is_relay_chain { if let IsDisputesAwareWithOverseer::Yes(ref selection) = self.selection {
return Ok(longest_chain_best) selection
.finality_target_with_longest_chain(
target_hash,
longest_chain_best,
maybe_max_number,
)
.await
} else {
Ok(longest_chain_best)
} }
self.selection
.finality_target_with_longest_chain(target_hash, longest_chain_best, maybe_max_number)
.await
} }
} }
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment