From 178e699c7d9a9f399040e290943dd13873772c68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lech=20G=C5=82owiak?= Date: Mon, 26 Aug 2024 00:30:08 +0200 Subject: [PATCH] Skip slot before creating inherent data providers during major sync (#5344) # Description Moves `create_inherent_data_provider` after checking if major sync is in progress. ## Integration Change is internal to sc-consensus-slots. It should be no-op unless someone is using fork of this SDK. ## Review Notes Motivation for this change is to avoid calling `create_inherent_data_providers` if it's result is going to be discarded anyway during major sync. This has potential to speed up node operations during major sync by not calling possibly expensive `create_inherent_data_provider`. TODO: labels T0-node D0-simple TODO: there is no tests for `Slots`, should I add one for this case? # Checklist * [x] My PR includes a detailed description as outlined in the "Description" and its two subsections above. * [x] My PR follows the [labeling requirements](CONTRIBUTING.md#Process) of this project (at minimum one label for `T` required) * External contributors: ask maintainers to put the right label on your PR. * [ ] I have made corresponding changes to the documentation (if applicable) * [ ] I have added tests that prove my fix is effective or that my feature works (if applicable) --- prdoc/pr_5344.prdoc | 10 ++++++++++ substrate/client/consensus/slots/src/lib.rs | 13 ++++++------- substrate/client/consensus/slots/src/slots.rs | 17 +++++++++++++---- 3 files changed, 29 insertions(+), 11 deletions(-) create mode 100644 prdoc/pr_5344.prdoc diff --git a/prdoc/pr_5344.prdoc b/prdoc/pr_5344.prdoc new file mode 100644 index 000000000000..9f83c113686d --- /dev/null +++ b/prdoc/pr_5344.prdoc @@ -0,0 +1,10 @@ +title: Fix storage weight reclaim bug. + +doc: + - audience: Node Dev + description: | + Improvement in slot worker loop that will not call create inherent data providers if the major sync is in progress. Before it was called every slot and the results were discarded during major sync. + +crates: + - name: sc-consensus-slots + bump: minor diff --git a/substrate/client/consensus/slots/src/lib.rs b/substrate/client/consensus/slots/src/lib.rs index 7cdf90877dff..06e0756fc968 100644 --- a/substrate/client/consensus/slots/src/lib.rs +++ b/substrate/client/consensus/slots/src/lib.rs @@ -517,16 +517,15 @@ pub async fn start_slot_worker( CIDP: CreateInherentDataProviders + Send + 'static, CIDP::InherentDataProviders: InherentDataProviderExt + Send, { - let mut slots = Slots::new(slot_duration.as_duration(), create_inherent_data_providers, client); + let mut slots = Slots::new( + slot_duration.as_duration(), + create_inherent_data_providers, + client, + sync_oracle, + ); loop { let slot_info = slots.next_slot().await; - - if sync_oracle.is_major_syncing() { - debug!(target: LOG_TARGET, "Skipping proposal slot due to sync."); - continue - } - let _ = worker.on_slot(slot_info).await; } } diff --git a/substrate/client/consensus/slots/src/slots.rs b/substrate/client/consensus/slots/src/slots.rs index 203764310601..c0b412e8ad5b 100644 --- a/substrate/client/consensus/slots/src/slots.rs +++ b/substrate/client/consensus/slots/src/slots.rs @@ -21,7 +21,7 @@ //! This is used instead of `futures_timer::Interval` because it was unreliable. use super::{InherentDataProviderExt, Slot, LOG_TARGET}; -use sp_consensus::SelectChain; +use sp_consensus::{SelectChain, SyncOracle}; use sp_inherents::{CreateInherentDataProviders, InherentDataProvider}; use sp_runtime::traits::{Block as BlockT, Header as HeaderT}; @@ -87,21 +87,23 @@ impl SlotInfo { } /// A stream that returns every time there is a new slot. -pub(crate) struct Slots { +pub(crate) struct Slots { last_slot: Slot, slot_duration: Duration, until_next_slot: Option, create_inherent_data_providers: IDP, select_chain: SC, + sync_oracle: SO, _phantom: std::marker::PhantomData, } -impl Slots { +impl Slots { /// Create a new `Slots` stream. pub fn new( slot_duration: Duration, create_inherent_data_providers: IDP, select_chain: SC, + sync_oracle: SO, ) -> Self { Slots { last_slot: 0.into(), @@ -109,17 +111,19 @@ impl Slots { until_next_slot: None, create_inherent_data_providers, select_chain, + sync_oracle, _phantom: Default::default(), } } } -impl Slots +impl Slots where Block: BlockT, SC: SelectChain, IDP: CreateInherentDataProviders + 'static, IDP::InherentDataProviders: crate::InherentDataProviderExt, + SO: SyncOracle, { /// Returns a future that fires when the next slot starts. pub async fn next_slot(&mut self) -> SlotInfo { @@ -138,6 +142,11 @@ where let wait_dur = time_until_next_slot(self.slot_duration); self.until_next_slot = Some(Delay::new(wait_dur)); + if self.sync_oracle.is_major_syncing() { + log::debug!(target: LOG_TARGET, "Skipping slot: major sync is in progress."); + continue; + } + let chain_head = match self.select_chain.best_chain().await { Ok(x) => x, Err(e) => {