Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Delay beefy worker initialization while network is on major sync #10705

Closed
Closed
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions client/beefy/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ sp-blockchain = { version = "4.0.0-dev", path = "../../primitives/blockchain" }
sp-core = { version = "4.1.0-dev", path = "../../primitives/core" }
sp-keystore = { version = "0.10.0", path = "../../primitives/keystore" }
sp-runtime = { version = "4.1.0-dev", path = "../../primitives/runtime" }
sp-consensus = { version = "0.10.0-dev", path = "../../primitives/consensus/common" }

sc-chain-spec = { version = "4.0.0-dev", path = "../../client/chain-spec" }
sc-utils = { version = "4.0.0-dev", path = "../utils" }
Expand Down
13 changes: 10 additions & 3 deletions client/beefy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use sc_network_gossip::{GossipEngine, Network as GossipNetwork};

use sp_api::ProvideRuntimeApi;
use sp_blockchain::HeaderBackend;
pub use sp_consensus::SyncOracle;
use sp_keystore::SyncCryptoStorePtr;
use sp_runtime::traits::Block;

Expand Down Expand Up @@ -106,13 +107,14 @@ where
}

/// BEEFY gadget initialization parameters.
pub struct BeefyParams<B, BE, C, N>
pub struct BeefyParams<B, BE, C, N, SO>
where
B: Block,
BE: Backend<B>,
C: Client<B, BE>,
C::Api: BeefyApi<B>,
N: GossipNetwork<B> + Clone + Send + 'static,
SO: SyncOracle + Send + Sync + Clone + 'static,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would these two ever be provided by different objects? If not, I think it would make sense to simply squash them together, they're both mandatory anyway...

{
/// BEEFY client
pub client: Arc<C>,
Expand All @@ -132,18 +134,21 @@ where
pub prometheus_registry: Option<Registry>,
/// Chain specific GRANDPA protocol name. See [`beefy_protocol_name::standard_name`].
pub protocol_name: std::borrow::Cow<'static, str>,
/// A handle to the sync oracle
pub sync_oracle: SO,
}

/// Start the BEEFY gadget.
///
/// This is a thin shim around running and awaiting a BEEFY worker.
pub async fn start_beefy_gadget<B, BE, C, N>(beefy_params: BeefyParams<B, BE, C, N>)
pub async fn start_beefy_gadget<B, BE, C, N, SO>(beefy_params: BeefyParams<B, BE, C, N, SO>)
where
B: Block,
BE: Backend<B>,
C: Client<B, BE>,
C::Api: BeefyApi<B>,
N: GossipNetwork<B> + Clone + Send + 'static,
SO: SyncOracle + Send + Sync + Clone + 'static,
{
let BeefyParams {
client,
Expand All @@ -155,6 +160,7 @@ where
min_block_delta,
prometheus_registry,
protocol_name,
sync_oracle,
} = beefy_params;

let gossip_validator = Arc::new(gossip::GossipValidator::new());
Expand Down Expand Up @@ -184,9 +190,10 @@ where
gossip_validator,
min_block_delta,
metrics,
sync_oracle,
};

let worker = worker::BeefyWorker::<_, _, _>::new(worker_params);
let worker = worker::BeefyWorker::<_, _, _, _>::new(worker_params);

worker.run().await
}
25 changes: 19 additions & 6 deletions client/beefy/src/worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,10 @@ use crate::{
metric_inc, metric_set,
metrics::Metrics,
notification::{BeefyBestBlockSender, BeefySignedCommitmentSender},
round, Client,
round, Client, SyncOracle,
};

pub(crate) struct WorkerParams<B, BE, C>
pub(crate) struct WorkerParams<B, BE, C, SO>
where
B: Block,
{
Expand All @@ -63,14 +63,16 @@ where
pub gossip_validator: Arc<GossipValidator<B>>,
pub min_block_delta: u32,
pub metrics: Option<Metrics>,
pub sync_oracle: SO,
}

/// A BEEFY worker plays the BEEFY protocol
pub(crate) struct BeefyWorker<B, C, BE>
pub(crate) struct BeefyWorker<B, C, BE, SO>
where
B: Block,
BE: Backend<B>,
C: Client<B, BE>,
SO: SyncOracle + Send + Sync + Clone + 'static,
{
client: Arc<C>,
backend: Arc<BE>,
Expand All @@ -91,24 +93,27 @@ where
beefy_best_block_sender: BeefyBestBlockSender<B>,
/// Validator set id for the last signed commitment
last_signed_id: u64,
/// Handle to the sync oracle
sync_oracle: SO,
// keep rustc happy
_backend: PhantomData<BE>,
}

impl<B, C, BE> BeefyWorker<B, C, BE>
impl<B, C, BE, SO> BeefyWorker<B, C, BE, SO>
where
B: Block + Codec,
BE: Backend<B>,
C: Client<B, BE>,
C::Api: BeefyApi<B>,
SO: SyncOracle + Send + Sync + Clone + 'static,
{
/// Return a new BEEFY worker instance.
///
/// Note that a BEEFY worker is only fully functional if a corresponding
/// BEEFY pallet has been deployed on-chain.
///
/// The BEEFY pallet is needed in order to keep track of the BEEFY authority set.
pub(crate) fn new(worker_params: WorkerParams<B, BE, C>) -> Self {
pub(crate) fn new(worker_params: WorkerParams<B, BE, C, SO>) -> Self {
let WorkerParams {
client,
backend,
Expand All @@ -119,6 +124,7 @@ where
gossip_validator,
min_block_delta,
metrics,
sync_oracle,
} = worker_params;

BeefyWorker {
Expand All @@ -136,17 +142,19 @@ where
best_beefy_block: None,
last_signed_id: 0,
beefy_best_block_sender,
sync_oracle,
_backend: PhantomData,
}
}
}

impl<B, C, BE> BeefyWorker<B, C, BE>
impl<B, C, BE, SO> BeefyWorker<B, C, BE, SO>
where
B: Block,
BE: Backend<B>,
C: Client<B, BE>,
C::Api: BeefyApi<B>,
SO: SyncOracle + Send + Sync + Clone + 'static,
{
/// Return `true`, if we should vote on block `number`
fn should_vote_on(&self, number: NumberFor<B>) -> bool {
Expand Down Expand Up @@ -400,6 +408,11 @@ where
));

loop {
if self.sync_oracle.is_major_syncing() {
debug!(target: "beefy", "Skipping initialization due to sync.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not necessarily initialization, it is the main loop of the worker. I would rephrase to smth like:

Suggested change
debug!(target: "beefy", "Skipping initialization due to sync.");
debug!(target: "beefy", "Waiting for major sync to complete.");

continue
}

acatangiu marked this conversation as resolved.
Show resolved Hide resolved
let engine = self.gossip_engine.clone();
let gossip_engine = future::poll_fn(|cx| engine.lock().poll_unpin(cx));

Expand Down