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

slots: incrementally backoff claiming slots if finality lags behind #7186

Merged
45 commits merged into from
Nov 11, 2020
Merged
Show file tree
Hide file tree
Changes from 43 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
250fbeb
babe: backoff authoring blocks when finality lags
octol Sep 16, 2020
d531efa
babe: move backoff authoring params to default constructor
octol Sep 22, 2020
b060e9b
babe: deduplicate the test a bit
octol Sep 22, 2020
0f95c44
babe: set backoff constants in service
octol Sep 23, 2020
df1951c
babe: use better names for backoff authoring block parameters
octol Sep 23, 2020
c17ca03
babe: remove last unwrap
octol Sep 23, 2020
2892da0
babe: slight style tweak
octol Sep 23, 2020
01f1ac3
babe: fix comment
octol Sep 23, 2020
9794ffa
slots: move backoff block authorship logic to SimpleSlotWorker
octol Sep 28, 2020
d2ee780
aura: append SlotInfo in on_slot
octol Sep 30, 2020
026a8d0
slots: use the correct types for parameters
octol Sep 30, 2020
f428a35
Merge branch 'master' into jon/incremental-backoff-on-finality
octol Oct 14, 2020
6903837
slots: fix review comments
octol Oct 23, 2020
4a6b8b7
Merge branch 'master' into jon/incremental-backoff-on-finality
octol Oct 23, 2020
92b506a
aura: add missing backoff authoring blocks parameters
octol Oct 23, 2020
cda5150
slots: add comments for default values
octol Oct 27, 2020
62f97dc
slots: add additional checks in test
octol Oct 27, 2020
7fa2084
Merge remote-tracking branch 'upstream/master' into jon/incremental-b…
octol Oct 28, 2020
af8d30b
slots: update implementation for new master
octol Oct 28, 2020
76b745a
slots: revert the change to SlotInfo
octol Oct 28, 2020
dafd2e5
Merge remote-tracking branch 'upstream/master' into jon/incremental-b…
octol Oct 28, 2020
dc04a1f
Merge remote-tracking branch 'upstream/master' into jon/incremental-b…
octol Oct 29, 2020
cb6d24c
Fix review comments
octol Oct 30, 2020
55827c3
slots: rework unit tests for backing off claiming slots
octol Nov 2, 2020
83538f0
slots: add test for asymptotic behaviour for slot claims
octol Nov 3, 2020
c87145d
Merge remote-tracking branch 'upstream/master' into jon/incremental-b…
octol Nov 3, 2020
0435142
slots: address review comments
octol Nov 4, 2020
38e61e5
slots: add test for max_interval
octol Nov 4, 2020
1228aef
slots: add assertion for intervals between between claimed slots
octol Nov 4, 2020
f7b8c76
slots: remove rustfmt directive
octol Nov 4, 2020
9790f60
slots: another attempt at explaining authoring_rate
octol Nov 4, 2020
f95225a
Merge remote-tracking branch 'upstream/master' into jon/incremental-b…
octol Nov 4, 2020
461d5f9
slots: up unfinalized_slack to 50 by default
octol Nov 6, 2020
796baf7
slots: add tests for time to reach max_interval
octol Nov 6, 2020
0d11df5
slots: fix typo in comments
octol Nov 6, 2020
9a968c6
Apply suggestions from code review
octol Nov 10, 2020
a607bc4
slots: additional tweaks to comments and info calls
octol Nov 10, 2020
bc32a7c
slots: rename to BackoffAuthoringOnFinalizedHeadLagging
octol Nov 10, 2020
eb45ed7
slots: make the backing off strategy generic
octol Nov 10, 2020
5a40da8
Merge remote-tracking branch 'upstream/master' into jon/incremental-b…
octol Nov 10, 2020
4f44b62
Apply suggestions from code review
octol Nov 10, 2020
1423649
slots: implement backoff trait for () for simplicity
octol Nov 10, 2020
44de2cf
slots: move logging inside backing off function to make it more specific
octol Nov 11, 2020
90d0ad3
Merge remote-tracking branch 'upstream/master' into jon/incremental-b…
octol Nov 11, 2020
b6435ff
aura: add missing function parameter
octol Nov 11, 2020
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
2 changes: 2 additions & 0 deletions Cargo.lock

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

4 changes: 3 additions & 1 deletion bin/node-template/node/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ pub fn new_full(config: Configuration) -> Result<TaskManager, ServiceError> {

let role = config.role.clone();
let force_authoring = config.force_authoring;
let backoff_authoring_blocks: Option<()> = None;
let name = config.network.node_name.clone();
let enable_grandpa = !config.disable_grandpa;
let prometheus_registry = config.prometheus_registry().cloned();
Expand Down Expand Up @@ -155,7 +156,7 @@ pub fn new_full(config: Configuration) -> Result<TaskManager, ServiceError> {
let can_author_with =
sp_consensus::CanAuthorWithNativeVersion::new(client.executor().clone());

let aura = sc_consensus_aura::start_aura::<_, _, _, _, _, AuraPair, _, _, _>(
let aura = sc_consensus_aura::start_aura::<_, _, _, _, _, AuraPair, _, _, _,_>(
sc_consensus_aura::slot_duration(&*client)?,
client.clone(),
select_chain,
Expand All @@ -164,6 +165,7 @@ pub fn new_full(config: Configuration) -> Result<TaskManager, ServiceError> {
network.clone(),
inherent_data_providers.clone(),
force_authoring,
backoff_authoring_blocks,
keystore_container.sync_keystore(),
can_author_with,
)?;
Expand Down
1 change: 1 addition & 0 deletions bin/node/cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ sc-chain-spec = { version = "2.0.0", path = "../../../client/chain-spec" }
sc-consensus = { version = "0.8.0", path = "../../../client/consensus/common" }
sc-transaction-pool = { version = "2.0.0", path = "../../../client/transaction-pool" }
sc-network = { version = "0.8.0", path = "../../../client/network" }
sc-consensus-slots = { version = "0.8.0", path = "../../../client/consensus/slots" }
sc-consensus-babe = { version = "0.8.0", path = "../../../client/consensus/babe" }
grandpa = { version = "0.8.0", package = "sc-finality-grandpa", path = "../../../client/finality-grandpa" }
sc-client-db = { version = "0.8.0", default-features = false, path = "../../../client/db" }
Expand Down
3 changes: 3 additions & 0 deletions bin/node/cli/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,8 @@ pub fn new_full_base(

let role = config.role.clone();
let force_authoring = config.force_authoring;
let backoff_authoring_blocks =
Some(sc_consensus_slots::BackoffAuthoringOnFinalizedHeadLagging::default());
let name = config.network.node_name.clone();
let enable_grandpa = !config.disable_grandpa;
let prometheus_registry = config.prometheus_registry().cloned();
Expand Down Expand Up @@ -249,6 +251,7 @@ pub fn new_full_base(
sync_oracle: network.clone(),
inherent_data_providers: inherent_data_providers.clone(),
force_authoring,
backoff_authoring_blocks,
babe_link,
can_author_with,
};
Expand Down
39 changes: 31 additions & 8 deletions client/consensus/aura/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ use sp_core::crypto::Public;
use sp_application_crypto::{AppKey, AppPublic};
use sp_runtime::{
generic::{BlockId, OpaqueDigestItemId},
Justification,
traits::NumberFor, Justification,
};
use sp_runtime::traits::{Block as BlockT, Header, DigestItemFor, Zero, Member};
use sp_api::ProvideRuntimeApi;
Expand All @@ -73,6 +73,7 @@ use sc_telemetry::{telemetry, CONSENSUS_TRACE, CONSENSUS_DEBUG, CONSENSUS_INFO};

use sc_consensus_slots::{
CheckedHeader, SlotInfo, SlotCompatible, StorageChanges, check_equivocation,
BackoffAuthoringBlocksStrategy,
};

use sp_api::ApiExt;
Expand Down Expand Up @@ -138,7 +139,7 @@ impl SlotCompatible for AuraSlotCompatible {
}

/// Start the aura worker. The returned future should be run in a futures executor.
pub fn start_aura<B, C, SC, E, I, P, SO, CAW, Error>(
pub fn start_aura<B, C, SC, E, I, P, SO, CAW, BS, Error>(
slot_duration: SlotDuration,
client: Arc<C>,
select_chain: SC,
Expand All @@ -147,11 +148,12 @@ pub fn start_aura<B, C, SC, E, I, P, SO, CAW, Error>(
sync_oracle: SO,
inherent_data_providers: InherentDataProviders,
force_authoring: bool,
backoff_authoring_blocks: Option<BS>,
keystore: SyncCryptoStorePtr,
can_author_with: CAW,
) -> Result<impl Future<Output = ()>, sp_consensus::Error> where
B: BlockT,
C: ProvideRuntimeApi<B> + BlockOf + ProvideCache<B> + AuxStore + Send + Sync,
C: ProvideRuntimeApi<B> + BlockOf + ProvideCache<B> + AuxStore + HeaderBackend<B> + Send + Sync,
C::Api: AuraApi<B, AuthorityId<P>>,
SC: SelectChain<B>,
E: Environment<B, Error = Error> + Send + Sync + 'static,
Expand All @@ -163,6 +165,7 @@ pub fn start_aura<B, C, SC, E, I, P, SO, CAW, Error>(
Error: std::error::Error + Send + From<sp_consensus::Error> + 'static,
SO: SyncOracle + Send + Sync + Clone,
CAW: CanAuthorWith<B> + Send,
BS: BackoffAuthoringBlocksStrategy<NumberFor<B>> + Send + 'static,
{
let worker = AuraWorker {
client,
Expand All @@ -171,6 +174,7 @@ pub fn start_aura<B, C, SC, E, I, P, SO, CAW, Error>(
keystore,
sync_oracle: sync_oracle.clone(),
force_authoring,
backoff_authoring_blocks,
_key_type: PhantomData::<P>,
};
register_aura_inherent_data_provider(
Expand All @@ -188,20 +192,22 @@ pub fn start_aura<B, C, SC, E, I, P, SO, CAW, Error>(
))
}

struct AuraWorker<C, E, I, P, SO> {
struct AuraWorker<C, E, I, P, SO, BS> {
client: Arc<C>,
block_import: Arc<Mutex<I>>,
env: E,
keystore: SyncCryptoStorePtr,
sync_oracle: SO,
force_authoring: bool,
backoff_authoring_blocks: Option<BS>,
_key_type: PhantomData<P>,
}

impl<B, C, E, I, P, Error, SO> sc_consensus_slots::SimpleSlotWorker<B> for AuraWorker<C, E, I, P, SO>
impl<B, C, E, I, P, Error, SO, BS> sc_consensus_slots::SimpleSlotWorker<B>
for AuraWorker<C, E, I, P, SO, BS>
where
B: BlockT,
C: ProvideRuntimeApi<B> + BlockOf + ProvideCache<B> + Sync,
C: ProvideRuntimeApi<B> + BlockOf + ProvideCache<B> + HeaderBackend<B> + Sync,
C::Api: AuraApi<B, AuthorityId<P>>,
E: Environment<B, Error = Error>,
E::Proposer: Proposer<B, Error = Error, Transaction = sp_api::TransactionFor<C, B>>,
Expand All @@ -210,6 +216,7 @@ where
P::Public: AppPublic + Public + Member + Encode + Decode + Hash,
P::Signature: TryFrom<Vec<u8>> + Member + Encode + Decode + Hash + Debug,
SO: SyncOracle + Send + Clone,
BS: BackoffAuthoringBlocksStrategy<NumberFor<B>> + Send + 'static,
Error: std::error::Error + Send + From<sp_consensus::Error> + 'static,
{
type BlockImport = I;
Expand Down Expand Up @@ -316,6 +323,20 @@ where
self.force_authoring
}

fn should_backoff(&self, slot_number: u64, chain_head: &B::Header) -> bool {
if let Some(ref strategy) = self.backoff_authoring_blocks {
if let Ok(chain_head_slot) = find_pre_digest::<B, P>(chain_head) {
return strategy.should_backoff(
*chain_head.number(),
chain_head_slot,
self.client.info().finalized_number,
slot_number,
);
}
}
false
}

fn sync_oracle(&mut self) -> &mut Self::SyncOracle {
&mut self.sync_oracle
}
Expand Down Expand Up @@ -863,7 +884,7 @@ mod tests {
use sp_keyring::sr25519::Keyring;
use sc_client_api::BlockchainEvents;
use sp_consensus_aura::sr25519::AuthorityPair;
use sc_consensus_slots::SimpleSlotWorker;
use sc_consensus_slots::{SimpleSlotWorker, BackoffAuthoringOnFinalizedHeadLagging};
use std::task::Poll;
use sc_block_builder::BlockBuilderProvider;
use sp_runtime::traits::Header as _;
Expand Down Expand Up @@ -1012,7 +1033,7 @@ mod tests {
&inherent_data_providers, slot_duration.get()
).expect("Registers aura inherent data provider");

aura_futures.push(start_aura::<_, _, _, _, _, AuthorityPair, _, _, _>(
aura_futures.push(start_aura::<_, _, _, _, _, AuthorityPair, _, _, _, _>(
slot_duration,
client.clone(),
select_chain,
Expand All @@ -1021,6 +1042,7 @@ mod tests {
DummyOracle,
inherent_data_providers,
false,
Some(BackoffAuthoringOnFinalizedHeadLagging::default()),
keystore,
sp_consensus::AlwaysCanAuthor,
).expect("Starts aura"));
Expand Down Expand Up @@ -1081,6 +1103,7 @@ mod tests {
keystore: keystore.into(),
sync_oracle: DummyOracle.clone(),
force_authoring: false,
backoff_authoring_blocks: Some(BackoffAuthoringOnFinalizedHeadLagging::default()),
_key_type: PhantomData::<AuthorityPair>,
};

Expand Down
38 changes: 33 additions & 5 deletions client/consensus/babe/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ use log::{debug, info, log, trace, warn};
use prometheus_endpoint::Registry;
use sc_consensus_slots::{
SlotInfo, SlotCompatible, StorageChanges, CheckedHeader, check_equivocation,
BackoffAuthoringBlocksStrategy,
};
use sc_consensus_epochs::{
descendent_query, SharedEpochChanges, EpochChangesFor, Epoch as EpochT, ViableEpochDescriptor,
Expand Down Expand Up @@ -354,7 +355,7 @@ impl std::ops::Deref for Config {
}

/// Parameters for BABE.
pub struct BabeParams<B: BlockT, C, E, I, SO, SC, CAW> {
pub struct BabeParams<B: BlockT, C, E, I, SO, SC, CAW, BS> {
/// The keystore that manages the keys of the node.
pub keystore: SyncCryptoStorePtr,

Expand All @@ -381,6 +382,9 @@ pub struct BabeParams<B: BlockT, C, E, I, SO, SC, CAW> {
/// Force authoring of blocks even if we are offline
pub force_authoring: bool,

/// Strategy and parameters for backing off block production.
pub backoff_authoring_blocks: Option<BS>,

/// The source of timestamps for relative slots
pub babe_link: BabeLink<B>,

Expand All @@ -389,7 +393,7 @@ pub struct BabeParams<B: BlockT, C, E, I, SO, SC, CAW> {
}

/// Start the babe worker.
pub fn start_babe<B, C, SC, E, I, SO, CAW, Error>(BabeParams {
pub fn start_babe<B, C, SC, E, I, SO, CAW, BS, Error>(BabeParams {
keystore,
client,
select_chain,
Expand All @@ -398,9 +402,10 @@ pub fn start_babe<B, C, SC, E, I, SO, CAW, Error>(BabeParams {
sync_oracle,
inherent_data_providers,
force_authoring,
backoff_authoring_blocks,
babe_link,
can_author_with,
}: BabeParams<B, C, E, I, SO, SC, CAW>) -> Result<
}: BabeParams<B, C, E, I, SO, SC, CAW, BS>) -> Result<
BabeWorker<B>,
sp_consensus::Error,
> where
Expand All @@ -416,6 +421,7 @@ pub fn start_babe<B, C, SC, E, I, SO, CAW, Error>(BabeParams {
Error: std::error::Error + Send + From<ConsensusError> + From<I::Error> + 'static,
SO: SyncOracle + Send + Sync + Clone + 'static,
CAW: CanAuthorWith<B> + Send + 'static,
BS: BackoffAuthoringBlocksStrategy<NumberFor<B>> + Send + 'static,
{
let config = babe_link.config;
let slot_notification_sinks = Arc::new(Mutex::new(Vec::new()));
Expand All @@ -426,6 +432,7 @@ pub fn start_babe<B, C, SC, E, I, SO, CAW, Error>(BabeParams {
env,
sync_oracle: sync_oracle.clone(),
force_authoring,
backoff_authoring_blocks,
keystore,
epoch_changes: babe_link.epoch_changes.clone(),
slot_notification_sinks: slot_notification_sinks.clone(),
Expand Down Expand Up @@ -490,19 +497,22 @@ impl<B: BlockT> futures::Future for BabeWorker<B> {
/// Slot notification sinks.
type SlotNotificationSinks<B> = Arc<Mutex<Vec<Sender<(u64, ViableEpochDescriptor<<B as BlockT>::Hash, NumberFor<B>, Epoch>)>>>>;

struct BabeSlotWorker<B: BlockT, C, E, I, SO> {
struct BabeSlotWorker<B: BlockT, C, E, I, SO, BS> {
client: Arc<C>,
block_import: Arc<Mutex<I>>,
env: E,
sync_oracle: SO,
force_authoring: bool,
backoff_authoring_blocks: Option<BS>,
keystore: SyncCryptoStorePtr,
epoch_changes: SharedEpochChanges<B, Epoch>,
slot_notification_sinks: SlotNotificationSinks<B>,
config: Config,
}

impl<B, C, E, I, Error, SO> sc_consensus_slots::SimpleSlotWorker<B> for BabeSlotWorker<B, C, E, I, SO> where
impl<B, C, E, I, Error, SO, BS> sc_consensus_slots::SimpleSlotWorker<B>
for BabeSlotWorker<B, C, E, I, SO, BS>
where
B: BlockT,
C: ProvideRuntimeApi<B> +
ProvideCache<B> +
Expand All @@ -513,6 +523,7 @@ impl<B, C, E, I, Error, SO> sc_consensus_slots::SimpleSlotWorker<B> for BabeSlot
E::Proposer: Proposer<B, Error = Error, Transaction = sp_api::TransactionFor<C, B>>,
I: BlockImport<B, Transaction = sp_api::TransactionFor<C, B>> + Send + Sync + 'static,
SO: SyncOracle + Send + Clone,
BS: BackoffAuthoringBlocksStrategy<NumberFor<B>>,
Error: std::error::Error + Send + From<ConsensusError> + From<I::Error> + 'static,
{
type EpochData = ViableEpochDescriptor<B::Hash, NumberFor<B>, Epoch>;
Expand Down Expand Up @@ -657,6 +668,23 @@ impl<B, C, E, I, Error, SO> sc_consensus_slots::SimpleSlotWorker<B> for BabeSlot
self.force_authoring
}

fn should_backoff(&self, slot_number: u64, chain_head: &B::Header) -> bool {
if let Some(ref strategy) = self.backoff_authoring_blocks {
if let Ok(chain_head_slot) = find_pre_digest::<B>(chain_head)
.map(|digest| digest.slot_number())
{
return strategy.should_backoff(
*chain_head.number(),
chain_head_slot,
self.client.info().finalized_number,
slot_number,
self.logging_target(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not the most elegant maybe, is there a cleaner way @bkchr ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah not that nice, but yeah 🤷

@andresilva WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

The other option would be to pass the logging target to the strategy itself when creating it but I think that's as nice as this. The reason we use the same logging target here as block production is to make sure these logging messages don't go unnoticed (i.e. people would forget to add some "slots" target). In this case the message is being logged under info so it should always be printed regardless of the target we provide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you're saying might as well drop the logging_target altogether?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either keep the changes you made or drop it altogether. I don't have a strict opinion about this, I guess we can keep it as is.

);
}
}
false
}

fn sync_oracle(&mut self) -> &mut Self::SyncOracle {
&mut self.sync_oracle
}
Expand Down
2 changes: 2 additions & 0 deletions client/consensus/babe/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ use sp_consensus_babe::{
make_transcript,
make_transcript_data,
};
use sc_consensus_slots::BackoffAuthoringOnFinalizedHeadLagging;
use sc_block_builder::{BlockBuilder, BlockBuilderProvider};
use sp_consensus::{
NoNetwork as DummyOracle, Proposal, RecordProof, AlwaysCanAuthor,
Expand Down Expand Up @@ -434,6 +435,7 @@ fn run_one_test(
sync_oracle: DummyOracle,
inherent_data_providers: data.inherent_data_providers.clone(),
force_authoring: false,
backoff_authoring_blocks: Some(BackoffAuthoringOnFinalizedHeadLagging::default()),
babe_link: data.link.clone(),
keystore,
can_author_with: sp_consensus::AlwaysCanAuthor,
Expand Down
1 change: 1 addition & 0 deletions client/consensus/slots/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ sc-client-api = { version = "2.0.0", path = "../../api" }
sp-core = { version = "2.0.0", path = "../../../primitives/core" }
sp-trie = { version = "2.0.0", path = "../../../primitives/trie" }
sp-application-crypto = { version = "2.0.0", path = "../../../primitives/application-crypto" }
sp-arithmetic = { version = "2.0.0", path = "../../../primitives/arithmetic" }
sp-blockchain = { version = "2.0.0", path = "../../../primitives/blockchain" }
sp-consensus-slots = { version = "0.8.0", path = "../../../primitives/consensus/slots" }
sp-runtime = { version = "2.0.0", path = "../../../primitives/runtime" }
Expand Down
Loading