diff --git a/Cargo.lock b/Cargo.lock index 0f39783f6d4f7..73987c741b173 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10074,7 +10074,6 @@ dependencies = [ "futures-timer", "linked-hash-map", "log", - "num-traits", "parity-scale-codec", "parking_lot 0.12.1", "sc-block-builder", diff --git a/bin/node-template/node/src/service.rs b/bin/node-template/node/src/service.rs index 745b14073d151..068ef8c0f26cc 100644 --- a/bin/node-template/node/src/service.rs +++ b/bin/node-template/node/src/service.rs @@ -35,6 +35,11 @@ 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, ) -> Result< @@ -94,6 +99,7 @@ pub fn new_partial( let (grandpa_block_import, grandpa_link) = sc_consensus_grandpa::block_import( client.clone(), + GRANDPA_JUSTIFICATION_PERIOD, &(client.clone() as Arc<_>), select_chain.clone(), telemetry.as_ref().map(|x| x.handle()), @@ -275,7 +281,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 8fc44c7c5eddf..d3c467afc91a2 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -53,6 +53,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. @@ -192,6 +196,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()), @@ -528,7 +533,7 @@ pub fn new_full_base( let 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 eb88382989175..53f09c81bb127 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 67820a59cc943..112bd5145d4a8 100644 --- a/client/consensus/grandpa/src/environment.rs +++ b/client/consensus/grandpa/src/environment.rs @@ -1094,7 +1094,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(), @@ -1307,6 +1307,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. @@ -1314,7 +1346,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, @@ -1385,22 +1417,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 c11e873eca738..08417a3a53ac6 100644 --- a/client/consensus/grandpa/src/lib.rs +++ b/client/consensus/grandpa/src/lib.rs @@ -214,10 +214,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 c46e249be485c..726ae89ea2205 100644 --- a/client/consensus/grandpa/src/tests.rs +++ b/client/consensus/grandpa/src/tests.rs @@ -68,6 +68,8 @@ type GrandpaBlockImport = crate::GrandpaBlockImport< LongestChain, >; +const JUSTIFICATION_IMPORT_PERIOD: u32 = 32; + #[derive(Default)] struct GrandpaTestNet { peers: Vec, @@ -125,6 +127,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, @@ -317,7 +320,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, @@ -442,11 +445,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] @@ -466,7 +472,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, @@ -558,7 +564,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, @@ -685,8 +691,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()); } @@ -983,7 +989,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, @@ -1025,7 +1031,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, @@ -1068,7 +1074,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, @@ -1230,7 +1236,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, @@ -1278,7 +1284,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, @@ -1390,7 +1396,7 @@ where let config = Config { gossip_duration: TEST_GOSSIP_DURATION, - justification_period: 32, + justification_generation_period: 32, keystore, name: None, local_role: Role::Authority, @@ -1883,24 +1889,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); @@ -1913,33 +1914,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] diff --git a/client/transaction-pool/Cargo.toml b/client/transaction-pool/Cargo.toml index 6338f8127aa1c..47bd65c1c968f 100644 --- a/client/transaction-pool/Cargo.toml +++ b/client/transaction-pool/Cargo.toml @@ -19,7 +19,6 @@ futures = "0.3.21" futures-timer = "3.0.2" linked-hash-map = "0.5.4" log = "0.4.17" -num-traits = "0.2.8" parking_lot = "0.12.1" serde = { version = "1.0.136", features = ["derive"] } thiserror = "1.0.30" diff --git a/client/transaction-pool/api/src/lib.rs b/client/transaction-pool/api/src/lib.rs index 428d0aed62efe..f11f83bb17deb 100644 --- a/client/transaction-pool/api/src/lib.rs +++ b/client/transaction-pool/api/src/lib.rs @@ -305,6 +305,20 @@ pub enum ChainEvent { }, } +impl ChainEvent { + /// Returns the block hash associated to the event. + pub fn hash(&self) -> B::Hash { + match self { + Self::NewBestBlock { hash, .. } | Self::Finalized { hash, .. } => *hash, + } + } + + /// Is `self == Self::Finalized`? + pub fn is_finalized(&self) -> bool { + matches!(self, Self::Finalized { .. }) + } +} + /// Trait for transaction pool maintenance. #[async_trait] pub trait MaintainedTransactionPool: TransactionPool { diff --git a/client/transaction-pool/src/enactment_state.rs b/client/transaction-pool/src/enactment_state.rs index ed6b3d186694f..85c572c127e84 100644 --- a/client/transaction-pool/src/enactment_state.rs +++ b/client/transaction-pool/src/enactment_state.rs @@ -19,10 +19,9 @@ //! Substrate transaction pool implementation. use crate::LOG_TARGET; -use num_traits::CheckedSub; use sc_transaction_pool_api::ChainEvent; use sp_blockchain::TreeRoute; -use sp_runtime::traits::{Block as BlockT, NumberFor}; +use sp_runtime::traits::{Block as BlockT, NumberFor, Saturating}; /// The threshold since the last update where we will skip any maintenance for blocks. /// @@ -101,17 +100,16 @@ where TreeRouteF: Fn(Block::Hash, Block::Hash) -> Result, String>, BlockNumberF: Fn(Block::Hash) -> Result>, String>, { - let (new_hash, current_hash, finalized) = match event { - ChainEvent::NewBestBlock { hash, .. } => (*hash, self.recent_best_block, false), - ChainEvent::Finalized { hash, .. } => (*hash, self.recent_finalized_block, true), - }; + let new_hash = event.hash(); + let finalized = event.is_finalized(); // do not proceed with txpool maintain if block distance is to high - let skip_maintenance = match (hash_to_number(new_hash), hash_to_number(current_hash)) { - (Ok(Some(new)), Ok(Some(current))) => - new.checked_sub(¤t) > Some(SKIP_MAINTENANCE_THRESHOLD.into()), - _ => true, - }; + let skip_maintenance = + match (hash_to_number(new_hash), hash_to_number(self.recent_best_block)) { + (Ok(Some(new)), Ok(Some(current))) => + new.saturating_sub(current) > SKIP_MAINTENANCE_THRESHOLD.into(), + _ => true, + }; if skip_maintenance { log::debug!(target: LOG_TARGET, "skip maintain: tree_route would be too long"); @@ -131,10 +129,10 @@ where log::debug!( target: LOG_TARGET, - "resolve hash:{:?} finalized:{:?} tree_route:{:?} best_block:{:?} finalized_block:{:?}", - new_hash, - finalized, - tree_route, + "resolve hash: {new_hash:?} finalized: {finalized:?} \ + tree_route: (common {:?}, last {:?}) best_block: {:?} finalized_block:{:?}", + tree_route.common_block(), + tree_route.last(), self.recent_best_block, self.recent_finalized_block );