diff --git a/bin/node-template/node/src/service.rs b/bin/node-template/node/src/service.rs index 355e24c95620b..36be0b1a938bd 100644 --- a/bin/node-template/node/src/service.rs +++ b/bin/node-template/node/src/service.rs @@ -37,6 +37,10 @@ pub(crate) type FullClient = type FullBackend = sc_service::TFullBackend; type FullSelectChain = sc_consensus::LongestChain; +/// 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 { 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/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 487c6c48f0061..5fb5e77c06012 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -54,6 +54,10 @@ type FullGrandpaBlockImport = /// The transaction pool type defintion. pub type TransactionPool = sc_transaction_pool::FullPool; +/// 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/client/consensus/grandpa/src/communication/gossip.rs b/client/consensus/grandpa/src/communication/gossip.rs index 2c0fe3d8571e5..c4c885d48e9b9 100644 --- a/client/consensus/grandpa/src/communication/gossip.rs +++ b/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/client/consensus/grandpa/src/communication/tests.rs b/client/consensus/grandpa/src/communication/tests.rs index eee9fddcea2d3..504fde74be603 100644 --- a/client/consensus/grandpa/src/communication/tests.rs +++ b/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/client/consensus/grandpa/src/environment.rs b/client/consensus/grandpa/src/environment.rs index eeb9ff4468291..d3e2beb84e79c 100644 --- a/client/consensus/grandpa/src/environment.rs +++ b/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( + client: &Client, + justification_period: u32, + number: NumberFor, + enacts_change: bool, +) -> bool +where + Block: BlockT, + BE: BackendT, + Client: ClientForGrandpa, +{ + 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( client: Arc, authority_set: &SharedAuthoritySet>, - justification_period: Option>, + justification_generation_period: Option, hash: Block::Hash, number: NumberFor, justification_or_commit: JustificationOrCommit, @@ -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/client/consensus/grandpa/src/import.rs b/client/consensus/grandpa/src/import.rs index cd13f832ce6dc..760cb2da0484d 100644 --- a/client/consensus/grandpa/src/import.rs +++ b/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 { inner: Arc, + justification_import_period: u32, select_chain: SC, authority_set: SharedAuthoritySet>, send_voter_commands: TracingUnboundedSender>>, @@ -74,6 +75,7 @@ impl 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 GrandpaBlockImport { pub(crate) fn new( inner: Arc, + justification_import_period: u32, select_chain: SC, authority_set: SharedAuthoritySet>, send_voter_commands: TracingUnboundedSender>>, @@ -733,6 +749,7 @@ impl GrandpaBlockImport justification, }; - let result = finalize_block( + let result = environment::finalize_block( self.inner.clone(), &self.authority_set, None, diff --git a/client/consensus/grandpa/src/lib.rs b/client/consensus/grandpa/src/lib.rs index c888340f70a34..ff0412aeb314c 100644 --- a/client/consensus/grandpa/src/lib.rs +++ b/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( client: Arc, + justification_import_period: u32, genesis_authorities_provider: &dyn GenesisAuthoritySetProvider, select_chain: SC, telemetry: Option, @@ -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 { /// given static authorities. pub fn block_import_with_authority_set_hard_forks( client: Arc, + justification_import_period: u32, genesis_authorities_provider: &dyn GenesisAuthoritySetProvider, select_chain: SC, authority_set_hard_forks: Vec>, @@ -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/client/consensus/grandpa/src/tests.rs b/client/consensus/grandpa/src/tests.rs index 4fbeed71a1bf5..0175f7d1b473c 100644 --- a/client/consensus/grandpa/src/tests.rs +++ b/client/consensus/grandpa/src/tests.rs @@ -69,6 +69,8 @@ type GrandpaBlockImport = crate::GrandpaBlockImport< LongestChain, >; +const JUSTIFICATION_IMPORT_PERIOD: u32 = 32; + #[derive(Default)] struct GrandpaTestNet { peers: Vec, @@ -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]