-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Delay beefy worker initialization while network is on major sync #10705
Changes from 6 commits
f6eaef1
b7e28d4
ddea83b
befe3d2
e88d416
44ff51b
e104fd1
31b04cb
ecdd381
8aca7a1
65071ae
6ef365f
e3395c6
16efe0f
0d2a0c7
3675f3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -22,6 +22,7 @@ use codec::{Codec, Decode, Encode}; | |||||
| use futures::{future, FutureExt, StreamExt}; | ||||||
| use log::{debug, error, info, log_enabled, trace, warn}; | ||||||
| use parking_lot::Mutex; | ||||||
| use std::task::Poll; | ||||||
|
|
||||||
| use sc_client_api::{Backend, FinalityNotification, FinalityNotifications}; | ||||||
| use sc_network_gossip::GossipEngine; | ||||||
|
|
@@ -47,10 +48,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, | ||||||
| { | ||||||
|
|
@@ -63,14 +64,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>, | ||||||
|
|
@@ -91,24 +94,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, | ||||||
|
|
@@ -119,6 +125,7 @@ where | |||||
| gossip_validator, | ||||||
| min_block_delta, | ||||||
| metrics, | ||||||
| sync_oracle, | ||||||
| } = worker_params; | ||||||
|
|
||||||
| BeefyWorker { | ||||||
|
|
@@ -136,17 +143,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 { | ||||||
|
|
@@ -400,6 +409,11 @@ where | |||||
| )); | ||||||
|
|
||||||
| loop { | ||||||
| if self.sync_oracle.is_major_syncing() { | ||||||
| debug!(target: "beefy", "Skipping initialization due to sync."); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||
| wait_for_major_syncing(self.sync_oracle.clone()).await; | ||||||
| } | ||||||
|
|
||||||
| let engine = self.gossip_engine.clone(); | ||||||
| let gossip_engine = future::poll_fn(|cx| engine.lock().poll_unpin(cx)); | ||||||
|
|
||||||
|
|
@@ -430,6 +444,19 @@ where | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Returns a future that waits until major sync is done | ||||||
| /// before completing | ||||||
| fn wait_for_major_syncing<SO: SyncOracle + Send + Sync + Clone + 'static>( | ||||||
| mut sync_oracle: SO, | ||||||
| ) -> impl future::Future<Output = ()> { | ||||||
| return future::poll_fn(move |_cx| { | ||||||
| if sync_oracle.is_major_syncing() { | ||||||
| Poll::Pending | ||||||
| } else { | ||||||
| Poll::Ready(()) | ||||||
| } | ||||||
| }) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this function has no way of knowing when the sync is over, i suggest adding it to the sync_oracle itself
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if it's rescheduled for polling with |
||||||
| } | ||||||
| /// Extract the MMR root hash from a digest in the given header, if it exists. | ||||||
| fn find_mmr_root_digest<B, Id>(header: &B::Header) -> Option<MmrRootHash> | ||||||
| where | ||||||
|
|
||||||
There was a problem hiding this comment.
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...