diff --git a/substrate/bin/node-template/node/src/service.rs b/substrate/bin/node-template/node/src/service.rs
index 355e24c95620b0cc7c59127aafc6bcfaf7de17e4..36be0b1a938bd0940be4ebab7ddfdb9ce33e2f4e 100644
--- a/substrate/bin/node-template/node/src/service.rs
+++ b/substrate/bin/node-template/node/src/service.rs
@@ -37,6 +37,10 @@ pub(crate) type FullClient =
type FullBackend = sc_service::TFullBackend<Block>;
type FullSelectChain = sc_consensus::LongestChain<FullBackend, Block>;
+/// The minimum period of blocks on which justifications will be
+/// imported and generated.
+const GRANDPA_JUSTIFICATION_PERIOD: u32 = 512;
+
#[allow(clippy::type_complexity)]
pub fn new_partial(
config: &Configuration,
@@ -97,6 +101,7 @@ pub fn new_partial(
let (grandpa_block_import, grandpa_link) = sc_consensus_grandpa::block_import(
client.clone(),
+ GRANDPA_JUSTIFICATION_PERIOD,
&client,
select_chain.clone(),
telemetry.as_ref().map(|x| x.handle()),
@@ -290,7 +295,7 @@ pub fn new_full(config: Configuration) -> Result<TaskManager, ServiceError> {
let grandpa_config = sc_consensus_grandpa::Config {
// FIXME #1578 make this available through chainspec
gossip_duration: Duration::from_millis(333),
- justification_period: 512,
+ justification_generation_period: GRANDPA_JUSTIFICATION_PERIOD,
name: Some(name),
observer_enabled: false,
keystore,
diff --git a/substrate/bin/node/cli/src/service.rs b/substrate/bin/node/cli/src/service.rs
index 487c6c48f0061ef86d5d5060a82327c160b7384a..5fb5e77c0601271be34da38374ca3c8a9efb76b6 100644
--- a/substrate/bin/node/cli/src/service.rs
+++ b/substrate/bin/node/cli/src/service.rs
@@ -54,6 +54,10 @@ type FullGrandpaBlockImport =
/// The transaction pool type defintion.
pub type TransactionPool = sc_transaction_pool::FullPool<Block, FullClient>;
+/// The minimum period of blocks on which justifications will be
+/// imported and generated.
+const GRANDPA_JUSTIFICATION_PERIOD: u32 = 512;
+
/// Fetch the nonce of the given `account` from the chain state.
///
/// Note: Should only be used for tests.
@@ -193,6 +197,7 @@ pub fn new_partial(
let (grandpa_block_import, grandpa_link) = grandpa::block_import(
client.clone(),
+ GRANDPA_JUSTIFICATION_PERIOD,
&(client.clone() as Arc<_>),
select_chain.clone(),
telemetry.as_ref().map(|x| x.handle()),
@@ -524,7 +529,7 @@ pub fn new_full_base(
let grandpa_config = grandpa::Config {
// FIXME #1578 make this available through chainspec
gossip_duration: std::time::Duration::from_millis(333),
- justification_period: 512,
+ justification_generation_period: GRANDPA_JUSTIFICATION_PERIOD,
name: Some(name),
observer_enabled: false,
keystore,
diff --git a/substrate/client/consensus/grandpa/src/communication/gossip.rs b/substrate/client/consensus/grandpa/src/communication/gossip.rs
index 2c0fe3d8571e52e47eeda4ad26de9902a6ce02e4..c4c885d48e9b969f4becd208a3b057299470fd45 100644
--- a/substrate/client/consensus/grandpa/src/communication/gossip.rs
+++ b/substrate/client/consensus/grandpa/src/communication/gossip.rs
@@ -1700,7 +1700,7 @@ mod tests {
fn config() -> crate::Config {
crate::Config {
gossip_duration: Duration::from_millis(10),
- justification_period: 256,
+ justification_generation_period: 256,
keystore: None,
name: None,
local_role: Role::Authority,
diff --git a/substrate/client/consensus/grandpa/src/communication/tests.rs b/substrate/client/consensus/grandpa/src/communication/tests.rs
index eee9fddcea2d350dde9f89b9133f9f40d93b85b2..504fde74be6033f22d794db7c9338090a81af44f 100644
--- a/substrate/client/consensus/grandpa/src/communication/tests.rs
+++ b/substrate/client/consensus/grandpa/src/communication/tests.rs
@@ -244,7 +244,7 @@ impl Tester {
fn config() -> crate::Config {
crate::Config {
gossip_duration: std::time::Duration::from_millis(10),
- justification_period: 256,
+ justification_generation_period: 256,
keystore: None,
name: None,
local_role: Role::Authority,
diff --git a/substrate/client/consensus/grandpa/src/environment.rs b/substrate/client/consensus/grandpa/src/environment.rs
index eeb9ff4468291551f749196cd9924bb3444f5447..d3e2beb84e79c101f7618421c16f7797123f55fa 100644
--- a/substrate/client/consensus/grandpa/src/environment.rs
+++ b/substrate/client/consensus/grandpa/src/environment.rs
@@ -1102,7 +1102,7 @@ where
finalize_block(
self.client.clone(),
&self.authority_set,
- Some(self.config.justification_period.into()),
+ Some(self.config.justification_generation_period),
hash,
number,
(round, commit).into(),
@@ -1315,6 +1315,38 @@ where
.or_else(|| Some((target_header.hash(), *target_header.number()))))
}
+/// Whether we should process a justification for the given block.
+///
+/// This can be used to decide whether to import a justification (when
+/// importing a block), or whether to generate a justification from a
+/// commit (when validating). Justifications for blocks that change the
+/// authority set will always be processed, otherwise we'll only process
+/// justifications if the last one was `justification_period` blocks ago.
+pub(crate) fn should_process_justification<BE, Block, Client>(
+ client: &Client,
+ justification_period: u32,
+ number: NumberFor<Block>,
+ enacts_change: bool,
+) -> bool
+where
+ Block: BlockT,
+ BE: BackendT<Block>,
+ Client: ClientForGrandpa<Block, BE>,
+{
+ if enacts_change {
+ return true
+ }
+
+ let last_finalized_number = client.info().finalized_number;
+
+ // keep the first justification before reaching the justification period
+ if last_finalized_number.is_zero() {
+ return true
+ }
+
+ last_finalized_number / justification_period.into() != number / justification_period.into()
+}
+
/// Finalize the given block and apply any authority set changes. If an
/// authority set change is enacted then a justification is created (if not
/// given) and stored with the block when finalizing it.
@@ -1322,7 +1354,7 @@ where
pub(crate) fn finalize_block<BE, Block, Client>(
client: Arc<Client>,
authority_set: &SharedAuthoritySet<Block::Hash, NumberFor<Block>>,
- justification_period: Option<NumberFor<Block>>,
+ justification_generation_period: Option<u32>,
hash: Block::Hash,
number: NumberFor<Block>,
justification_or_commit: JustificationOrCommit<Block>,
@@ -1393,22 +1425,13 @@ where
let (justification_required, justification) = match justification_or_commit {
JustificationOrCommit::Justification(justification) => (true, justification),
JustificationOrCommit::Commit((round_number, commit)) => {
- let mut justification_required =
- // justification is always required when block that enacts new authorities
- // set is finalized
- status.new_set_block.is_some();
-
- // justification is required every N blocks to be able to prove blocks
- // finalization to remote nodes
- if !justification_required {
- if let Some(justification_period) = justification_period {
- let last_finalized_number = client.info().finalized_number;
- justification_required = (!last_finalized_number.is_zero() ||
- number - last_finalized_number == justification_period) &&
- (last_finalized_number / justification_period !=
- number / justification_period);
- }
- }
+ let enacts_change = status.new_set_block.is_some();
+
+ let justification_required = justification_generation_period
+ .map(|period| {
+ should_process_justification(&*client, period, number, enacts_change)
+ })
+ .unwrap_or(enacts_change);
let justification =
GrandpaJustification::from_commit(&client, round_number, commit)?;
diff --git a/substrate/client/consensus/grandpa/src/import.rs b/substrate/client/consensus/grandpa/src/import.rs
index cd13f832ce6dc55f6d38a1157d8ba77cca5a44c5..760cb2da0484d99793c008c14c110f39d9610ee8 100644
--- a/substrate/client/consensus/grandpa/src/import.rs
+++ b/substrate/client/consensus/grandpa/src/import.rs
@@ -41,7 +41,7 @@ use sp_runtime::{
use crate::{
authorities::{AuthoritySet, DelayKind, PendingChange, SharedAuthoritySet},
- environment::finalize_block,
+ environment,
justification::GrandpaJustification,
notification::GrandpaJustificationSender,
AuthoritySetChanges, ClientForGrandpa, CommandOrError, Error, NewAuthoritySet, VoterCommand,
@@ -59,6 +59,7 @@ use crate::{
/// object.
pub struct GrandpaBlockImport<Backend, Block: BlockT, Client, SC> {
inner: Arc<Client>,
+ justification_import_period: u32,
select_chain: SC,
authority_set: SharedAuthoritySet<Block::Hash, NumberFor<Block>>,
send_voter_commands: TracingUnboundedSender<VoterCommand<Block::Hash, NumberFor<Block>>>,
@@ -74,6 +75,7 @@ impl<Backend, Block: BlockT, Client, SC: Clone> Clone
fn clone(&self) -> Self {
GrandpaBlockImport {
inner: self.inner.clone(),
+ justification_import_period: self.justification_import_period,
select_chain: self.select_chain.clone(),
authority_set: self.authority_set.clone(),
send_voter_commands: self.send_voter_commands.clone(),
@@ -648,26 +650,39 @@ where
match grandpa_justification {
Some(justification) => {
- let import_res = self.import_justification(
- hash,
+ if environment::should_process_justification(
+ &*self.inner,
+ self.justification_import_period,
number,
- (GRANDPA_ENGINE_ID, justification),
needs_justification,
- initial_sync,
- );
+ ) {
+ let import_res = self.import_justification(
+ hash,
+ number,
+ (GRANDPA_ENGINE_ID, justification),
+ needs_justification,
+ initial_sync,
+ );
- import_res.unwrap_or_else(|err| {
- if needs_justification {
- debug!(
- target: LOG_TARGET,
- "Requesting justification from peers due to imported block #{} that enacts authority set change with invalid justification: {}",
- number,
- err
- );
- imported_aux.bad_justification = true;
- imported_aux.needs_justification = true;
- }
- });
+ import_res.unwrap_or_else(|err| {
+ if needs_justification {
+ debug!(
+ target: LOG_TARGET,
+ "Requesting justification from peers due to imported block #{} that enacts authority set change with invalid justification: {}",
+ number,
+ err
+ );
+ imported_aux.bad_justification = true;
+ imported_aux.needs_justification = true;
+ }
+ });
+ } else {
+ debug!(
+ target: LOG_TARGET,
+ "Ignoring unnecessary justification for block #{}",
+ number,
+ );
+ }
},
None =>
if needs_justification {
@@ -695,6 +710,7 @@ where
impl<Backend, Block: BlockT, Client, SC> GrandpaBlockImport<Backend, Block, Client, SC> {
pub(crate) fn new(
inner: Arc<Client>,
+ justification_import_period: u32,
select_chain: SC,
authority_set: SharedAuthoritySet<Block::Hash, NumberFor<Block>>,
send_voter_commands: TracingUnboundedSender<VoterCommand<Block::Hash, NumberFor<Block>>>,
@@ -733,6 +749,7 @@ impl<Backend, Block: BlockT, Client, SC> GrandpaBlockImport<Backend, Block, Clie
GrandpaBlockImport {
inner,
+ justification_import_period,
select_chain,
authority_set,
send_voter_commands,
@@ -783,7 +800,7 @@ where
Ok(justification) => justification,
};
- let result = finalize_block(
+ let result = environment::finalize_block(
self.inner.clone(),
&self.authority_set,
None,
diff --git a/substrate/client/consensus/grandpa/src/lib.rs b/substrate/client/consensus/grandpa/src/lib.rs
index c888340f70a341bab43c9e4a738f12921e65f036..ff0412aeb314c70b58095aa90df62d3899b4cbd4 100644
--- a/substrate/client/consensus/grandpa/src/lib.rs
+++ b/substrate/client/consensus/grandpa/src/lib.rs
@@ -215,10 +215,10 @@ impl Clone for SharedVoterState {
pub struct Config {
/// The expected duration for a message to be gossiped across the network.
pub gossip_duration: Duration,
- /// Justification generation period (in blocks). GRANDPA will try to generate justifications
- /// at least every justification_period blocks. There are some other events which might cause
- /// justification generation.
- pub justification_period: u32,
+ /// Justification generation period (in blocks). GRANDPA will try to generate
+ /// justifications at least every justification_generation_period blocks. There
+ /// are some other events which might cause justification generation.
+ pub justification_generation_period: u32,
/// Whether the GRANDPA observer protocol is live on the network and thereby
/// a full-node not running as a validator is running the GRANDPA observer
/// protocol (we will only issue catch-up requests to authorities when the
@@ -495,8 +495,16 @@ where
/// Make block importer and link half necessary to tie the background voter
/// to it.
+///
+/// The `justification_import_period` sets the minimum period on which
+/// justifications will be imported. When importing a block, if it includes a
+/// justification it will only be processed if it fits within this period,
+/// otherwise it will be ignored (and won't be validated). This is to avoid
+/// slowing down sync by a peer serving us unnecessary justifications which
+/// aren't trivial to validate.
pub fn block_import<BE, Block: BlockT, Client, SC>(
client: Arc<Client>,
+ justification_import_period: u32,
genesis_authorities_provider: &dyn GenesisAuthoritySetProvider<Block>,
select_chain: SC,
telemetry: Option<TelemetryHandle>,
@@ -508,6 +516,7 @@ where
{
block_import_with_authority_set_hard_forks(
client,
+ justification_import_period,
genesis_authorities_provider,
select_chain,
Default::default(),
@@ -540,6 +549,7 @@ pub struct AuthoritySetHardFork<Block: BlockT> {
/// given static authorities.
pub fn block_import_with_authority_set_hard_forks<BE, Block: BlockT, Client, SC>(
client: Arc<Client>,
+ justification_import_period: u32,
genesis_authorities_provider: &dyn GenesisAuthoritySetProvider<Block>,
select_chain: SC,
authority_set_hard_forks: Vec<AuthoritySetHardFork<Block>>,
@@ -599,6 +609,7 @@ where
Ok((
GrandpaBlockImport::new(
client.clone(),
+ justification_import_period,
select_chain.clone(),
persistent_data.authority_set.clone(),
voter_commands_tx,
diff --git a/substrate/client/consensus/grandpa/src/tests.rs b/substrate/client/consensus/grandpa/src/tests.rs
index 4fbeed71a1bf566e1fabbba23be3e5edc09f6ce3..0175f7d1b473cb8be88da088df4792099f015a31 100644
--- a/substrate/client/consensus/grandpa/src/tests.rs
+++ b/substrate/client/consensus/grandpa/src/tests.rs
@@ -69,6 +69,8 @@ type GrandpaBlockImport = crate::GrandpaBlockImport<
LongestChain<substrate_test_runtime_client::Backend, Block>,
>;
+const JUSTIFICATION_IMPORT_PERIOD: u32 = 32;
+
#[derive(Default)]
struct GrandpaTestNet {
peers: Vec<GrandpaPeer>,
@@ -126,6 +128,7 @@ impl TestNetFactory for GrandpaTestNet {
let (client, backend) = (client.as_client(), client.as_backend());
let (import, link) = block_import(
client.clone(),
+ JUSTIFICATION_IMPORT_PERIOD,
&self.test_config,
LongestChain::new(backend.clone()),
None,
@@ -318,7 +321,7 @@ fn initialize_grandpa(
let grandpa_params = GrandpaParams {
config: Config {
gossip_duration: TEST_GOSSIP_DURATION,
- justification_period: 32,
+ justification_generation_period: 32,
keystore: Some(keystore),
name: Some(format!("peer#{}", peer_id)),
local_role: Role::Authority,
@@ -446,11 +449,14 @@ async fn finalize_3_voters_no_observers() {
let net = Arc::new(Mutex::new(net));
run_to_completion(20, net.clone(), peers).await;
- // normally there's no justification for finalized blocks
- assert!(
- net.lock().peer(0).client().justifications(hashof20).unwrap().is_none(),
- "Extra justification for block#1",
- );
+ // all peers should have stored the justification for the best finalized block #20
+ for peer_id in 0..3 {
+ let client = net.lock().peers[peer_id].client().as_client();
+ let justification =
+ crate::aux_schema::best_justification::<_, Block>(&*client).unwrap().unwrap();
+
+ assert_eq!(justification.justification.commit.target_number, 20);
+ }
}
#[tokio::test]
@@ -470,7 +476,7 @@ async fn finalize_3_voters_1_full_observer() {
let grandpa_params = GrandpaParams {
config: Config {
gossip_duration: TEST_GOSSIP_DURATION,
- justification_period: 32,
+ justification_generation_period: 32,
keystore: None,
name: Some(format!("peer#{}", peer_id)),
local_role: Role::Authority,
@@ -565,7 +571,7 @@ async fn transition_3_voters_twice_1_full_observer() {
let grandpa_params = GrandpaParams {
config: Config {
gossip_duration: TEST_GOSSIP_DURATION,
- justification_period: 32,
+ justification_generation_period: 32,
keystore: Some(keystore),
name: Some(format!("peer#{}", peer_id)),
local_role: Role::Authority,
@@ -695,8 +701,8 @@ async fn justification_is_generated_periodically() {
let net = Arc::new(Mutex::new(net));
run_to_completion(32, net.clone(), peers).await;
- // when block#32 (justification_period) is finalized, justification
- // is required => generated
+ // when block#32 (justification_generation_period) is finalized,
+ // justification is required => generated
for i in 0..3 {
assert!(net.lock().peer(i).client().justifications(hashof32).unwrap().is_some());
}
@@ -993,7 +999,7 @@ async fn voter_persists_its_votes() {
let bob_network = {
let config = Config {
gossip_duration: TEST_GOSSIP_DURATION,
- justification_period: 32,
+ justification_generation_period: 32,
keystore: Some(bob_keystore.clone()),
name: Some(format!("peer#{}", 1)),
local_role: Role::Authority,
@@ -1035,7 +1041,7 @@ async fn voter_persists_its_votes() {
let grandpa_params = GrandpaParams {
config: Config {
gossip_duration: TEST_GOSSIP_DURATION,
- justification_period: 32,
+ justification_generation_period: 32,
keystore: Some(keystore),
name: Some(format!("peer#{}", 0)),
local_role: Role::Authority,
@@ -1081,7 +1087,7 @@ async fn voter_persists_its_votes() {
let grandpa_params = GrandpaParams {
config: Config {
gossip_duration: TEST_GOSSIP_DURATION,
- justification_period: 32,
+ justification_generation_period: 32,
keystore: Some(keystore),
name: Some(format!("peer#{}", 0)),
local_role: Role::Authority,
@@ -1246,7 +1252,7 @@ async fn finalize_3_voters_1_light_observer() {
let observer = observer::run_grandpa_observer(
Config {
gossip_duration: TEST_GOSSIP_DURATION,
- justification_period: 32,
+ justification_generation_period: 32,
keystore: None,
name: Some("observer".to_string()),
local_role: Role::Full,
@@ -1294,7 +1300,7 @@ async fn voter_catches_up_to_latest_round_when_behind() {
let grandpa_params = GrandpaParams {
config: Config {
gossip_duration: TEST_GOSSIP_DURATION,
- justification_period: 32,
+ justification_generation_period: 32,
keystore,
name: Some(format!("peer#{}", peer_id)),
local_role: Role::Authority,
@@ -1409,7 +1415,7 @@ where
let config = Config {
gossip_duration: TEST_GOSSIP_DURATION,
- justification_period: 32,
+ justification_generation_period: 32,
keystore,
name: None,
local_role: Role::Authority,
@@ -1903,24 +1909,19 @@ async fn imports_justification_for_regular_blocks_on_import() {
let client = net.peer(0).client().clone();
let (mut block_import, ..) = net.make_block_import(client.clone());
-
let full_client = client.as_client();
- let builder = full_client
- .new_block_at(full_client.chain_info().genesis_hash, Default::default(), false)
- .unwrap();
- let block = builder.build().unwrap().block;
- let block_hash = block.hash();
+ // create a new block (without importing it)
+ let generate_block = |parent| {
+ let builder = full_client.new_block_at(parent, Default::default(), false).unwrap();
+ builder.build().unwrap().block
+ };
// create a valid justification, with one precommit targeting the block
- let justification = {
- let round = 1;
+ let make_justification = |round, hash, number| {
let set_id = 0;
- let precommit = finality_grandpa::Precommit {
- target_hash: block_hash,
- target_number: *block.header.number(),
- };
+ let precommit = finality_grandpa::Precommit { target_hash: hash, target_number: number };
let msg = finality_grandpa::Message::Precommit(precommit.clone());
let encoded = sp_consensus_grandpa::localized_payload(round, set_id, &msg);
@@ -1933,33 +1934,59 @@ async fn imports_justification_for_regular_blocks_on_import() {
};
let commit = finality_grandpa::Commit {
- target_hash: block_hash,
- target_number: *block.header.number(),
+ target_hash: hash,
+ target_number: number,
precommits: vec![precommit],
};
GrandpaJustification::from_commit(&full_client, round, commit).unwrap()
};
- // we import the block with justification attached
- let mut import = BlockImportParams::new(BlockOrigin::File, block.header);
- import.justifications = Some((GRANDPA_ENGINE_ID, justification.encode()).into());
- import.body = Some(block.extrinsics);
- import.fork_choice = Some(ForkChoiceStrategy::LongestChain);
+ let mut generate_and_import_block_with_justification = |parent| {
+ // we import the block with justification attached
+ let block = generate_block(parent);
+ let block_hash = block.hash();
+ let justification = make_justification(1, block_hash, *block.header.number());
- assert_eq!(
- block_import.import_block(import).await.unwrap(),
- ImportResult::Imported(ImportedAux {
- needs_justification: false,
- clear_justification_requests: false,
- bad_justification: false,
- is_new_best: true,
- ..Default::default()
- }),
- );
+ let mut import = BlockImportParams::new(BlockOrigin::File, block.header);
+ import.justifications = Some((GRANDPA_ENGINE_ID, justification.encode()).into());
+ import.body = Some(block.extrinsics);
+ import.fork_choice = Some(ForkChoiceStrategy::LongestChain);
+
+ assert_eq!(
+ // NOTE: we use `block_on` here because async closures are
+ // unsupported and it doesn't matter if we block in a test
+ futures::executor::block_on(block_import.import_block(import)).unwrap(),
+ ImportResult::Imported(ImportedAux {
+ needs_justification: false,
+ clear_justification_requests: false,
+ bad_justification: false,
+ is_new_best: true,
+ ..Default::default()
+ }),
+ );
+
+ block_hash
+ };
+
+ let block1 =
+ generate_and_import_block_with_justification(full_client.chain_info().genesis_hash);
// the justification should be imported and available from the client
- assert!(client.justifications(block_hash).unwrap().is_some());
+ assert!(client.justifications(block1).unwrap().is_some());
+
+ // subsequent justifications should be ignored and not imported
+ let mut parent = block1;
+ for _ in 2..JUSTIFICATION_IMPORT_PERIOD {
+ parent = generate_and_import_block_with_justification(parent);
+ assert!(client.justifications(parent).unwrap().is_none());
+ }
+
+ let block32 = generate_and_import_block_with_justification(parent);
+
+ // until we reach a block in the next justification import period, at
+ // which point we should import it
+ assert!(client.justifications(block32).unwrap().is_some());
}
#[tokio::test]