diff --git a/substrate/core/consensus/common/src/import_queue.rs b/substrate/core/consensus/common/src/import_queue.rs index 997b30ed6f..1952869187 100644 --- a/substrate/core/consensus/common/src/import_queue.rs +++ b/substrate/core/consensus/common/src/import_queue.rs @@ -358,17 +358,18 @@ impl BlockImporter { let success = self.justification_import.as_ref().map(|justification_import| { justification_import.import_justification(hash, number, justification) .map_err(|e| { - debug!("Justification import failed with {:?} for hash: {:?} number: {:?} coming from node: {:?}", e, hash, number, who); + debug!(target: "sync", "Justification import failed with {:?} for hash: {:?} number: {:?} coming from node: {:?}", e, hash, number, who); e }).is_ok() }).unwrap_or(false); + if let Some(link) = self.link.as_ref() { link.justification_imported(who, &hash, number, success); } } fn handle_import_blocks(&mut self, origin: BlockOrigin, blocks: Vec>) { - trace!(target:"sync", "Scheduling {} blocks for import", blocks.len()); + trace!(target: "sync", "Scheduling {} blocks for import", blocks.len()); self.worker_sender .send(BlockImportWorkerMsg::ImportBlocks(origin, blocks)) .expect("1. This is holding a sender to the worker, 2. the worker should not quit while a sender is still held; qed"); @@ -423,7 +424,7 @@ impl> BlockImportWorker { _ => Default::default(), }; - trace!(target:"sync", "Starting import of {} blocks {}", count, blocks_range); + trace!(target: "sync", "Starting import of {} blocks {}", count, blocks_range); let mut results = vec![]; diff --git a/substrate/core/finality-grandpa/src/authorities.rs b/substrate/core/finality-grandpa/src/authorities.rs index 38bcec2347..49917ae1c2 100644 --- a/substrate/core/finality-grandpa/src/authorities.rs +++ b/substrate/core/finality-grandpa/src/authorities.rs @@ -351,14 +351,18 @@ where /// authority set change (without triggering it), ensuring that if there are /// multiple changes in the same branch, finalizing this block won't /// finalize past multiple transitions (i.e. transitions must be finalized - /// in-order). The given function `is_descendent_of` should return `true` if - /// the second hash (target) is a descendent of the first hash (base). + /// in-order). Returns `Some(true)` if the block being finalized enacts a + /// change that can be immediately applied, `Some(false)` if the block being + /// finalized enacts a change but it cannot be applied yet since there are + /// other dependent changes, and `None` if no change is enacted. The given + /// function `is_descendent_of` should return `true` if the second hash + /// (target) is a descendent of the first hash (base). pub fn enacts_standard_change( &self, finalized_hash: H, finalized_number: N, is_descendent_of: &F, - ) -> Result> + ) -> Result, fork_tree::Error> where F: Fn(&H, &H) -> Result, E: std::error::Error, { @@ -659,21 +663,51 @@ mod tests { delay_kind: DelayKind::Finalized, }; + let change_b = PendingChange { + next_authorities: set_a.clone(), + delay: 10, + canon_height: 20, + canon_hash: "hash_b", + delay_kind: DelayKind::Finalized, + }; + authorities.add_pending_change(change_a.clone(), &static_is_descendent_of(false)).unwrap(); + authorities.add_pending_change(change_b.clone(), &static_is_descendent_of(true)).unwrap(); let is_descendent_of = is_descendent_of(|base, hash| match (*base, *hash) { - ("hash_a", "hash_b") => true, + ("hash_a", "hash_d") => true, + ("hash_a", "hash_e") => true, + ("hash_b", "hash_d") => true, + ("hash_b", "hash_e") => true, ("hash_a", "hash_c") => false, + ("hash_b", "hash_c") => false, _ => unreachable!(), }); // "hash_c" won't finalize the existing change since it isn't a descendent - assert!(!authorities.enacts_standard_change("hash_c", 15, &is_descendent_of).unwrap()); - // "hash_b" at depth 14 won't work either - assert!(!authorities.enacts_standard_change("hash_b", 14, &is_descendent_of).unwrap()); + assert_eq!( + authorities.enacts_standard_change("hash_c", 15, &is_descendent_of).unwrap(), + None, + ); + + // "hash_d" at depth 14 won't work either + assert_eq!( + authorities.enacts_standard_change("hash_d", 14, &is_descendent_of).unwrap(), + None, + ); // but it should work at depth 15 (change height + depth) - assert!(authorities.enacts_standard_change("hash_b", 15, &is_descendent_of).unwrap()); + assert_eq!( + authorities.enacts_standard_change("hash_d", 15, &is_descendent_of).unwrap(), + Some(true), + ); + + // finalizing "hash_e" at depth 20 will trigger change at "hash_b", but + // it can't be applied yet since "hash_a" must be applied first + assert_eq!( + authorities.enacts_standard_change("hash_e", 30, &is_descendent_of).unwrap(), + Some(false), + ); } #[test] @@ -709,7 +743,10 @@ mod tests { // there's an effective change triggered at block 15 but not a standard one. // so this should do nothing. - assert!(!authorities.enacts_standard_change("hash_c", 15, &static_is_descendent_of(true)).unwrap()); + assert_eq!( + authorities.enacts_standard_change("hash_c", 15, &static_is_descendent_of(true)).unwrap(), + None, + ); // throw a standard change into the mix to prove that it's discarded // for being on the same fork. diff --git a/substrate/core/finality-grandpa/src/import.rs b/substrate/core/finality-grandpa/src/import.rs index 9ff3486b86..0c59bd60fe 100644 --- a/substrate/core/finality-grandpa/src/import.rs +++ b/substrate/core/finality-grandpa/src/import.rs @@ -116,7 +116,7 @@ impl, RA, PRA> JustificationImport } enum AppliedChanges { - Standard, + Standard(bool), // true if the change is ready to be applied (i.e. it's a root) Forced(NewAuthoritySet), None, } @@ -124,7 +124,7 @@ enum AppliedChanges { impl AppliedChanges { fn needs_justification(&self) -> bool { match *self { - AppliedChanges::Standard => true, + AppliedChanges::Standard(_) => true, AppliedChanges::Forced(_) | AppliedChanges::None => false, } } @@ -345,8 +345,8 @@ impl, RA, PRA> GrandpaBlockImport, RA, PRA> GrandpaBlockImport Some(new), - AppliedChanges::Standard => None, // the change isn't actually applied yet. + AppliedChanges::Standard(_) => None, // the change isn't actually applied yet. AppliedChanges::None => None, }; @@ -405,7 +405,7 @@ impl, RA, PRA> BlockImport let pending_changes = self.make_authorities_changes(&mut block, hash)?; // we don't want to finalize on `inner.import_block` - let justification = block.justification.take(); + let mut justification = block.justification.take(); let enacts_consensus_change = new_authorities.is_some(); let import_result = self.inner.import_block(block, new_authorities); @@ -435,23 +435,34 @@ impl, RA, PRA> BlockImport } let needs_justification = applied_changes.needs_justification(); - if let AppliedChanges::Forced(new) = applied_changes { - // NOTE: when we do a force change we are "discrediting" the old set so we - // ignore any justifications from them. this block may contain a justification - // which should be checked and imported below against the new authority - // triggered by this forced change. the new grandpa voter will start at the - // last median finalized block (which is before the block that enacts the - // change), full nodes syncing the chain will not be able to successfully - // import justifications for those blocks since their local authority set view - // is still of the set before the forced change was enacted, still after #1867 - // they should import the block and discard the justification, and they will - // then request a justification from sync if it's necessary (which they should - // then be able to successfully validate). - let _ = self.send_voter_commands.unbounded_send(VoterCommand::ChangeAuthorities(new)); - // we must clear all pending justifications requests, presumably they won't be - // finalized hence why this forced changes was triggered - imported_aux.clear_justification_requests = true; + match applied_changes { + AppliedChanges::Forced(new) => { + // NOTE: when we do a force change we are "discrediting" the old set so we + // ignore any justifications from them. this block may contain a justification + // which should be checked and imported below against the new authority + // triggered by this forced change. the new grandpa voter will start at the + // last median finalized block (which is before the block that enacts the + // change), full nodes syncing the chain will not be able to successfully + // import justifications for those blocks since their local authority set view + // is still of the set before the forced change was enacted, still after #1867 + // they should import the block and discard the justification, and they will + // then request a justification from sync if it's necessary (which they should + // then be able to successfully validate). + let _ = self.send_voter_commands.unbounded_send(VoterCommand::ChangeAuthorities(new)); + + // we must clear all pending justifications requests, presumably they won't be + // finalized hence why this forced changes was triggered + imported_aux.clear_justification_requests = true; + }, + AppliedChanges::Standard(false) => { + // we can't apply this change yet since there are other dependent changes that we + // need to apply first, drop any justification that might have been provided with + // the block to make sure we request them from `sync` which will ensure they'll be + // applied in-order. + justification.take(); + }, + _ => {}, } if !needs_justification && !enacts_consensus_change { @@ -481,6 +492,7 @@ impl, RA, PRA> BlockImport if enacts_consensus_change { self.consensus_changes.lock().note_change((number, hash)); } + imported_aux.needs_justification = true; } } diff --git a/substrate/core/network/src/sync.rs b/substrate/core/network/src/sync.rs index 14222798ef..f1464cbed9 100644 --- a/substrate/core/network/src/sync.rs +++ b/substrate/core/network/src/sync.rs @@ -98,6 +98,7 @@ struct PendingJustifications { pending_requests: VecDeque>, peer_requests: HashMap>, previous_requests: HashMap, Vec<(NodeIndex, Instant)>>, + importing_requests: HashSet>, } impl PendingJustifications { @@ -107,6 +108,7 @@ impl PendingJustifications { pending_requests: VecDeque::new(), peer_requests: HashMap::new(), previous_requests: HashMap::new(), + importing_requests: HashSet::new(), } } @@ -243,6 +245,16 @@ impl PendingJustifications { /// Queues a retry in case the import failed. fn justification_import_result(&mut self, hash: B::Hash, number: NumberFor, success: bool) { let request = (hash, number); + + if !self.importing_requests.remove(&request) { + debug!(target: "sync", "Got justification import result for unknown justification {:?} {:?} request.", + request.0, + request.1, + ); + + return; + }; + if success { if self.justifications.finalize_root(&request.0).is_none() { warn!(target: "sync", "Imported justification for {:?} {:?} which isn't a root in the tree: {:?}", @@ -250,6 +262,7 @@ impl PendingJustifications { request.1, self.justifications.roots().collect::>(), ); + return; }; @@ -278,6 +291,7 @@ impl PendingJustifications { if let Some(request) = self.peer_requests.remove(&who) { if let Some(justification) = justification { import_queue.import_justification(who.clone(), request.0, request.1, justification); + self.importing_requests.insert(request); return } @@ -285,6 +299,7 @@ impl PendingJustifications { .entry(request) .or_insert(Vec::new()) .push((who, Instant::now())); + self.pending_requests.push_front(request); } } @@ -299,7 +314,11 @@ impl PendingJustifications { ) -> Result<(), fork_tree::Error> where F: Fn(&B::Hash, &B::Hash) -> Result { - use std::collections::HashSet; + if self.importing_requests.contains(&(*best_finalized_hash, best_finalized_number)) { + // we imported this justification ourselves, so we should get back a response + // from the import queue through `justification_import_result` + return Ok(()); + } self.justifications.finalize(best_finalized_hash, best_finalized_number, &is_descendent_of)?; diff --git a/substrate/core/util/fork-tree/src/lib.rs b/substrate/core/util/fork-tree/src/lib.rs index 71c83f57ca..f194ac8915 100644 --- a/substrate/core/util/fork-tree/src/lib.rs +++ b/substrate/core/util/fork-tree/src/lib.rs @@ -228,17 +228,19 @@ impl ForkTree where /// Checks if any node in the tree is finalized by either finalizing the /// node itself or a child node that's not in the tree, guaranteeing that /// the node being finalized isn't a descendent of any of the node's - /// children. The given `predicate` is checked on the prospective finalized - /// root and must pass for finalization to occur. The given function - /// `is_descendent_of` should return `true` if the second hash (target) is a - /// descendent of the first hash (base). + /// children. Returns `Some(true)` if the node being finalized is a root, + /// `Some(false)` if the node being finalized is not a root, and `None` if + /// no node in the tree is finalized. The given `predicate` is checked on + /// the prospective finalized root and must pass for finalization to occur. + /// The given function `is_descendent_of` should return `true` if the second + /// hash (target) is a descendent of the first hash (base). pub fn finalizes_any_with_descendent_if( &self, hash: &H, number: N, is_descendent_of: &F, predicate: P, - ) -> Result> + ) -> Result, Error> where E: std::error::Error, F: Fn(&H, &H) -> Result, P: Fn(&V) -> bool, @@ -253,20 +255,20 @@ impl ForkTree where // tree, if we find a valid node that passes the predicate then we must // ensure that we're not finalizing past any of its child nodes. for node in self.node_iter() { - if node.hash == *hash || is_descendent_of(&node.hash, hash)? { - if predicate(&node.data) { + if predicate(&node.data) { + if node.hash == *hash || is_descendent_of(&node.hash, hash)? { for node in node.children.iter() { if node.number <= number && is_descendent_of(&node.hash, &hash)? { return Err(Error::UnfinalizedAncestor); } } - return Ok(true); + return Ok(Some(self.roots.iter().any(|root| root.hash == node.hash))); } } } - Ok(false) + Ok(None) } /// Finalize a root in the tree by either finalizing the node itself or a @@ -298,8 +300,8 @@ impl ForkTree where // we're not finalizing past any children node. let mut position = None; for (i, root) in self.roots.iter().enumerate() { - if root.hash == *hash || is_descendent_of(&root.hash, hash)? { - if predicate(&root.data) { + if predicate(&root.data) { + if root.hash == *hash || is_descendent_of(&root.hash, hash)? { for node in root.children.iter() { if node.number <= number && is_descendent_of(&node.hash, &hash)? { return Err(Error::UnfinalizedAncestor); @@ -692,7 +694,19 @@ mod test { &is_descendent_of, |c| c.effective <= 2, ), - Ok(false), + Ok(None), + ); + + // finalizing "D" will finalize a block from the tree, but it can't be applied yet + // since it is not a root change + assert_eq!( + tree.finalizes_any_with_descendent_if( + &"D", + 10, + &is_descendent_of, + |c| c.effective == 10, + ), + Ok(Some(false)), ); // finalizing "B" doesn't finalize "A0" since the predicate doesn't pass, @@ -720,7 +734,7 @@ mod test { &is_descendent_of, |c| c.effective <= 5, ), - Ok(true), + Ok(Some(true)), ); assert_eq!( @@ -757,7 +771,7 @@ mod test { &is_descendent_of, |c| c.effective <= 100, ), - Ok(true), + Ok(Some(true)), ); assert_eq!( @@ -788,4 +802,76 @@ mod test { ], ); } + + #[test] + fn minimizes_calls_to_is_descendent_of() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + let n_is_descendent_of_calls = AtomicUsize::new(0); + + let is_descendent_of = |_: &&str, _: &&str| -> Result { + n_is_descendent_of_calls.fetch_add(1, Ordering::SeqCst); + Ok(true) + }; + + { + // Deep tree where we want to call `finalizes_any_with_descendent_if`. The + // search for the node should first check the predicate (which is cheaper) and + // only then call `is_descendent_of` + let mut tree = ForkTree::new(); + let letters = vec!["A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K"]; + + for (i, letter) in letters.iter().enumerate() { + tree.import::<_, TestError>(*letter, i, i, &|_, _| Ok(true)).unwrap(); + } + + // "L" is a descendent of "K", but the predicate will only pass for "K", + // therefore only one call to `is_descendent_of` should be made + assert_eq!( + tree.finalizes_any_with_descendent_if( + &"L", + 11, + &is_descendent_of, + |i| *i == 10, + ), + Ok(Some(false)), + ); + + assert_eq!( + n_is_descendent_of_calls.load(Ordering::SeqCst), + 1, + ); + } + + n_is_descendent_of_calls.store(0, Ordering::SeqCst); + + { + // Multiple roots in the tree where we want to call `finalize_with_descendent_if`. + // The search for the root node should first check the predicate (which is cheaper) + // and only then call `is_descendent_of` + let mut tree = ForkTree::new(); + let letters = vec!["A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K"]; + + for (i, letter) in letters.iter().enumerate() { + tree.import::<_, TestError>(*letter, i, i, &|_, _| Ok(false)).unwrap(); + } + + // "L" is a descendent of "K", but the predicate will only pass for "K", + // therefore only one call to `is_descendent_of` should be made + assert_eq!( + tree.finalize_with_descendent_if( + &"L", + 11, + &is_descendent_of, + |i| *i == 10, + ), + Ok(FinalizationResult::Changed(Some(10))), + ); + + assert_eq!( + n_is_descendent_of_calls.load(Ordering::SeqCst), + 1, + ); + } + } }