From 76c9831ce45c2f8cc58f598b350a0b256d950e5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 8 May 2020 14:53:19 +0200 Subject: [PATCH 1/3] babe: don't repeatedly lookup keys in authorship rpc function Expose a new function `claim_slot_using_keypars` in Babe so that the `babe_epochAuthorship` can lookup authorship for all slots in the epoch without repeatedly looking up keys in the keystore. Time to run the `babe_epochAuthorship` RPC call goes from 7s to 25ms on a local dev chain on my machine. --- client/consensus/babe/rpc/src/lib.rs | 13 +++++- client/consensus/babe/src/authorship.rs | 58 ++++++++++++++----------- 2 files changed, 44 insertions(+), 27 deletions(-) diff --git a/client/consensus/babe/rpc/src/lib.rs b/client/consensus/babe/rpc/src/lib.rs index fa5421761cea8..c9538bbd1ec31 100644 --- a/client/consensus/babe/rpc/src/lib.rs +++ b/client/consensus/babe/rpc/src/lib.rs @@ -116,9 +116,20 @@ impl BabeApi for BabeRPCHandler let mut claims: HashMap = HashMap::new(); + let key_pairs = { + let keystore = keystore.read(); + epoch.authorities.iter().enumerate() + .flat_map(|(i, a)| { + keystore.key_pair::(&a.0).ok().map(|kp| (kp, i)) + }) + .collect::>() + }; + for slot_number in epoch_start..epoch_end { let epoch = epoch_data(&shared_epoch, &client, &babe_config, slot_number, &select_chain)?; - if let Some((claim, key)) = authorship::claim_slot(slot_number, &epoch, &keystore) { + if let Some((claim, key)) = + authorship::claim_slot_using_key_pairs(slot_number, &epoch, &key_pairs) + { match claim { PreDigest::Primary { .. } => { claims.entry(key.public()).or_default().primary.push(slot_number); diff --git a/client/consensus/babe/src/authorship.rs b/client/consensus/babe/src/authorship.rs index 1810f9f5bef11..7982e69d123fa 100644 --- a/client/consensus/babe/src/authorship.rs +++ b/client/consensus/babe/src/authorship.rs @@ -124,7 +124,7 @@ pub(super) fn secondary_slot_author( fn claim_secondary_slot( slot_number: SlotNumber, epoch: &Epoch, - keystore: &KeyStorePtr, + key_pairs: &Vec<(AuthorityPair, usize)>, author_secondary_vrf: bool, ) -> Option<(PreDigest, AuthorityPair)> { let Epoch { authorities, randomness, epoch_index, .. } = epoch; @@ -139,14 +139,7 @@ fn claim_secondary_slot( *randomness, )?; - let keystore = keystore.read(); - - for (pair, authority_index) in authorities.iter() - .enumerate() - .flat_map(|(i, a)| { - keystore.key_pair::(&a.0).ok().map(|kp| (kp, i)) - }) - { + for (pair, authority_index) in key_pairs { if pair.public() == *expected_author { let pre_digest = if author_secondary_vrf { let transcript = super::authorship::make_transcript( @@ -161,16 +154,16 @@ fn claim_secondary_slot( slot_number, vrf_output: VRFOutput(s.0.to_output()), vrf_proof: VRFProof(s.1), - authority_index: authority_index as u32, + authority_index: *authority_index as u32, }) } else { PreDigest::SecondaryPlain(SecondaryPlainPreDigest { slot_number, - authority_index: authority_index as u32, + authority_index: *authority_index as u32, }) }; - return Some((pre_digest, pair)); + return Some((pre_digest, pair.clone())); } } @@ -186,7 +179,26 @@ pub fn claim_slot( epoch: &Epoch, keystore: &KeyStorePtr, ) -> Option<(PreDigest, AuthorityPair)> { - claim_primary_slot(slot_number, epoch, epoch.config.c, keystore) + let key_pairs = { + let keystore = keystore.read(); + epoch.authorities.iter() + .enumerate() + .flat_map(|(i, a)| { + keystore.key_pair::(&a.0).ok().map(|kp| (kp, i)) + }) + .collect::>() + }; + claim_slot_using_key_pairs(slot_number, epoch, &key_pairs) +} + +/// Like `claim_slot`, but allows passing an explicit set of key pairs. Useful if we intend +/// to make repeated calls for different slots using the same key pairs. +pub fn claim_slot_using_key_pairs( + slot_number: SlotNumber, + epoch: &Epoch, + key_pairs: &Vec<(AuthorityPair, usize)>, +) -> Option<(PreDigest, AuthorityPair)> { + claim_primary_slot(slot_number, epoch, epoch.config.c, &key_pairs) .or_else(|| { if epoch.config.allowed_slots.is_secondary_plain_slots_allowed() || epoch.config.allowed_slots.is_secondary_vrf_slots_allowed() @@ -194,7 +206,7 @@ pub fn claim_slot( claim_secondary_slot( slot_number, &epoch, - keystore, + &key_pairs, epoch.config.allowed_slots.is_secondary_vrf_slots_allowed(), ) } else { @@ -216,39 +228,33 @@ fn claim_primary_slot( slot_number: SlotNumber, epoch: &Epoch, c: (u64, u64), - keystore: &KeyStorePtr, + key_pairs: &Vec<(AuthorityPair, usize)>, ) -> Option<(PreDigest, AuthorityPair)> { let Epoch { authorities, randomness, epoch_index, .. } = epoch; - let keystore = keystore.read(); - for (pair, authority_index) in authorities.iter() - .enumerate() - .flat_map(|(i, a)| { - keystore.key_pair::(&a.0).ok().map(|kp| (kp, i)) - }) - { + for (pair, authority_index) in key_pairs { let transcript = super::authorship::make_transcript(randomness, slot_number, *epoch_index); // Compute the threshold we will use. // // We already checked that authorities contains `key.public()`, so it can't // be empty. Therefore, this division in `calculate_threshold` is safe. - let threshold = super::authorship::calculate_primary_threshold(c, authorities, authority_index); + let threshold = super::authorship::calculate_primary_threshold(c, authorities, *authority_index); - let pre_digest = get_keypair(&pair) + let pre_digest = get_keypair(pair) .vrf_sign_after_check(transcript, |inout| super::authorship::check_primary_threshold(inout, threshold)) .map(|s| { PreDigest::Primary(PrimaryPreDigest { slot_number, vrf_output: VRFOutput(s.0.to_output()), vrf_proof: VRFProof(s.1), - authority_index: authority_index as u32, + authority_index: *authority_index as u32, }) }); // early exit on first successful claim if let Some(pre_digest) = pre_digest { - return Some((pre_digest, pair)); + return Some((pre_digest, pair.clone())); } } From d0dc0b8db5e4aa13d51d81e0085e09bebe198690 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Sun, 10 May 2020 13:21:14 +0200 Subject: [PATCH 2/3] babe: pass reference to slice instead of ref to Vec --- client/consensus/babe/src/authorship.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/client/consensus/babe/src/authorship.rs b/client/consensus/babe/src/authorship.rs index 7982e69d123fa..584501110b75f 100644 --- a/client/consensus/babe/src/authorship.rs +++ b/client/consensus/babe/src/authorship.rs @@ -124,7 +124,7 @@ pub(super) fn secondary_slot_author( fn claim_secondary_slot( slot_number: SlotNumber, epoch: &Epoch, - key_pairs: &Vec<(AuthorityPair, usize)>, + key_pairs: &[(AuthorityPair, usize)], author_secondary_vrf: bool, ) -> Option<(PreDigest, AuthorityPair)> { let Epoch { authorities, randomness, epoch_index, .. } = epoch; @@ -196,7 +196,7 @@ pub fn claim_slot( pub fn claim_slot_using_key_pairs( slot_number: SlotNumber, epoch: &Epoch, - key_pairs: &Vec<(AuthorityPair, usize)>, + key_pairs: &[(AuthorityPair, usize)], ) -> Option<(PreDigest, AuthorityPair)> { claim_primary_slot(slot_number, epoch, epoch.config.c, &key_pairs) .or_else(|| { @@ -228,7 +228,7 @@ fn claim_primary_slot( slot_number: SlotNumber, epoch: &Epoch, c: (u64, u64), - key_pairs: &Vec<(AuthorityPair, usize)>, + key_pairs: &[(AuthorityPair, usize)], ) -> Option<(PreDigest, AuthorityPair)> { let Epoch { authorities, randomness, epoch_index, .. } = epoch; From b2d264f5f10ded47ad99454d2160c8283c44fdfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Sun, 10 May 2020 23:28:12 +0200 Subject: [PATCH 3/3] babe: fix bunch of clippy warnings --- client/consensus/babe/rpc/src/lib.rs | 2 +- client/consensus/babe/src/aux_schema.rs | 2 +- client/consensus/babe/src/lib.rs | 19 +++++++++---------- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/client/consensus/babe/rpc/src/lib.rs b/client/consensus/babe/rpc/src/lib.rs index c9538bbd1ec31..aa7e1c0e1e21c 100644 --- a/client/consensus/babe/rpc/src/lib.rs +++ b/client/consensus/babe/rpc/src/lib.rs @@ -174,7 +174,7 @@ pub enum Error { impl From for jsonrpc_core::Error { fn from(error: Error) -> Self { jsonrpc_core::Error { - message: format!("{}", error).into(), + message: format!("{}", error), code: jsonrpc_core::ErrorCode::ServerError(1234), data: None, } diff --git a/client/consensus/babe/src/aux_schema.rs b/client/consensus/babe/src/aux_schema.rs index 2a3f23981dc30..4f26568d83343 100644 --- a/client/consensus/babe/src/aux_schema.rs +++ b/client/consensus/babe/src/aux_schema.rs @@ -112,7 +112,7 @@ pub(crate) fn write_epoch_changes( /// Write the cumulative chain-weight of a block ot aux storage. pub(crate) fn write_block_weight( block_hash: H, - block_weight: &BabeBlockWeight, + block_weight: BabeBlockWeight, write_aux: F, ) -> R where F: FnOnce(&[(Vec, &[u8])]) -> R, diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 678a33cbcc7a6..d6498c119b361 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -78,7 +78,6 @@ use std::{ collections::HashMap, sync::Arc, u64, pin::Pin, time::{Instant, Duration}, any::Any, borrow::Cow }; -use sp_consensus_babe; use sp_consensus::{ImportResult, CanAuthorWith}; use sp_consensus::import_queue::{ BoxJustificationImport, BoxFinalityProofImport, @@ -186,7 +185,7 @@ impl Epoch { start_slot: slot_number, duration: genesis_config.epoch_length, authorities: genesis_config.genesis_authorities.clone(), - randomness: genesis_config.randomness.clone(), + randomness: genesis_config.randomness, config: BabeEpochConfiguration { c: genesis_config.c, allowed_slots: genesis_config.allowed_slots, @@ -399,7 +398,7 @@ pub fn start_babe(BabeParams { register_babe_inherent_data_provider(&inherent_data_providers, config.slot_duration())?; sc_consensus_uncles::register_uncles_inherent_data_provider( - client.clone(), + client, select_chain.clone(), &inherent_data_providers, )?; @@ -494,7 +493,7 @@ impl sc_consensus_slots::SimpleSlotWorker for BabeWork &self.keystore, ); - if let Some(_) = s { + if s.is_some() { debug!(target: "babe", "Claimed slot {}", slot_number); } @@ -796,7 +795,7 @@ impl Verifier for BabeVerifier where // FIXME #1019 in the future, alter this queue to allow deferring of headers let v_params = verification::VerificationParams { header: header.clone(), - pre_digest: Some(pre_digest.clone()), + pre_digest: Some(pre_digest), slot_now: slot_now + 1, epoch: viable_epoch.as_ref(), }; @@ -952,7 +951,7 @@ impl BlockImport for BabeBlockImport>, ) -> Result { let hash = block.post_hash(); - let number = block.header.number().clone(); + let number = *block.header.number(); // early exit if block already in chain, otherwise the check for // epoch changes will error when trying to re-import an epoch change @@ -1133,7 +1132,7 @@ impl BlockImport for BabeBlockImport BlockImport for BabeBlockImport BlockImport for BabeBlockImport( register_babe_inherent_data_provider(&inherent_data_providers, babe_link.config.slot_duration)?; let verifier = BabeVerifier { - client: client.clone(), + client, inherent_data_providers, config: babe_link.config, epoch_changes: babe_link.epoch_changes,