diff --git a/client/consensus/common/src/longest_chain.rs b/client/consensus/common/src/longest_chain.rs index b1f7f94f9eb28..7ec91a5ad87e9 100644 --- a/client/consensus/common/src/longest_chain.rs +++ b/client/consensus/common/src/longest_chain.rs @@ -91,11 +91,12 @@ where &self, target_hash: Block::Hash, maybe_max_number: Option>, - ) -> Result, ConsensusError> { + ) -> Result { let import_lock = self.backend.get_import_lock(); self.backend .blockchain() .best_containing(target_hash, maybe_max_number, import_lock) + .map(|maybe_hash| maybe_hash.unwrap_or(target_hash)) .map_err(|e| ConsensusError::ChainLookup(e.to_string()).into()) } } diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index f27a530ed2f40..c79698902e975 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -1165,7 +1165,7 @@ where debug!(target: "afg", "Finding best chain containing block {:?} with number limit {:?}", block, limit); let result = match select_chain.finality_target(block, None).await { - Ok(Some(best_hash)) => { + Ok(best_hash) => { let best_header = client .header(BlockId::Hash(best_hash))? .expect("Header known to exist after `finality_target` call; qed"); @@ -1223,10 +1223,6 @@ where }) .or_else(|| Some((target_header.hash(), *target_header.number()))) }, - Ok(None) => { - debug!(target: "afg", "Encountered error finding best chain containing {:?}: couldn't find target block", block); - None - }, Err(e) => { debug!(target: "afg", "Encountered error finding best chain containing {:?}: {:?}", block, e); None diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index f663bfe94afdf..1c4d1b4e97b88 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -118,10 +118,10 @@ where ) .await } else { - Ok(Some(pending_change.canon_hash)) + Ok(pending_change.canon_hash) }; - if let Ok(Some(hash)) = effective_block_hash { + if let Ok(hash) = effective_block_hash { if let Ok(Some(header)) = self.inner.header(BlockId::Hash(hash)) { if *header.number() == pending_change.effective_number() { out.push((header.hash(), *header.number())); diff --git a/client/service/test/src/client/mod.rs b/client/service/test/src/client/mod.rs index 295e941f7ceb1..d82a839936d7d 100644 --- a/client/service/test/src/client/mod.rs +++ b/client/service/test/src/client/mod.rs @@ -481,25 +481,7 @@ fn best_containing_with_genesis_block() { assert_eq!( genesis_hash.clone(), - block_on(longest_chain_select.finality_target(genesis_hash.clone(), None)) - .unwrap() - .unwrap(), - ); -} - -#[test] -fn best_containing_with_hash_not_found() { - // block tree: - // G - - let (client, longest_chain_select) = TestClientBuilder::new().build_with_longest_chain(); - - let uninserted_block = client.new_block(Default::default()).unwrap().build().unwrap().block; - - assert_eq!( - None, - block_on(longest_chain_select.finality_target(uninserted_block.hash().clone(), None)) - .unwrap(), + block_on(longest_chain_select.finality_target(genesis_hash.clone(), None)).unwrap(), ); } @@ -675,22 +657,10 @@ fn best_containing_on_longest_chain_with_single_chain_3_blocks() { assert_eq!( a2.hash(), - block_on(longest_chain_select.finality_target(genesis_hash, None)) - .unwrap() - .unwrap() - ); - assert_eq!( - a2.hash(), - block_on(longest_chain_select.finality_target(a1.hash(), None)) - .unwrap() - .unwrap() - ); - assert_eq!( - a2.hash(), - block_on(longest_chain_select.finality_target(a2.hash(), None)) - .unwrap() - .unwrap() + block_on(longest_chain_select.finality_target(genesis_hash, None)).unwrap() ); + assert_eq!(a2.hash(), block_on(longest_chain_select.finality_target(a1.hash(), None)).unwrap()); + assert_eq!(a2.hash(), block_on(longest_chain_select.finality_target(a2.hash(), None)).unwrap()); } #[test] @@ -819,343 +789,101 @@ fn best_containing_on_longest_chain_with_multiple_forks() { assert!(leaves.contains(&d2.hash())); assert_eq!(leaves.len(), 4); + let finality_target = |target_hash, number| { + block_on(longest_chain_select.finality_target(target_hash, number)).unwrap() + }; + // search without restriction - assert_eq!( - a5.hash(), - block_on(longest_chain_select.finality_target(genesis_hash, None)) - .unwrap() - .unwrap() - ); - assert_eq!( - a5.hash(), - block_on(longest_chain_select.finality_target(a1.hash(), None)) - .unwrap() - .unwrap() - ); - assert_eq!( - a5.hash(), - block_on(longest_chain_select.finality_target(a2.hash(), None)) - .unwrap() - .unwrap() - ); - assert_eq!( - a5.hash(), - block_on(longest_chain_select.finality_target(a3.hash(), None)) - .unwrap() - .unwrap() - ); - assert_eq!( - a5.hash(), - block_on(longest_chain_select.finality_target(a4.hash(), None)) - .unwrap() - .unwrap() - ); - assert_eq!( - a5.hash(), - block_on(longest_chain_select.finality_target(a5.hash(), None)) - .unwrap() - .unwrap() - ); - assert_eq!( - b4.hash(), - block_on(longest_chain_select.finality_target(b2.hash(), None)) - .unwrap() - .unwrap() - ); - assert_eq!( - b4.hash(), - block_on(longest_chain_select.finality_target(b3.hash(), None)) - .unwrap() - .unwrap() - ); - assert_eq!( - b4.hash(), - block_on(longest_chain_select.finality_target(b4.hash(), None)) - .unwrap() - .unwrap() - ); - assert_eq!( - c3.hash(), - block_on(longest_chain_select.finality_target(c3.hash(), None)) - .unwrap() - .unwrap() - ); - assert_eq!( - d2.hash(), - block_on(longest_chain_select.finality_target(d2.hash(), None)) - .unwrap() - .unwrap() - ); + assert_eq!(a5.hash(), finality_target(genesis_hash, None)); + assert_eq!(a5.hash(), finality_target(a1.hash(), None)); + assert_eq!(a5.hash(), finality_target(a2.hash(), None)); + assert_eq!(a5.hash(), finality_target(a3.hash(), None)); + assert_eq!(a5.hash(), finality_target(a4.hash(), None)); + assert_eq!(a5.hash(), finality_target(a5.hash(), None)); + assert_eq!(b4.hash(), finality_target(b2.hash(), None)); + assert_eq!(b4.hash(), finality_target(b3.hash(), None)); + assert_eq!(b4.hash(), finality_target(b4.hash(), None)); + assert_eq!(c3.hash(), finality_target(c3.hash(), None)); + assert_eq!(d2.hash(), finality_target(d2.hash(), None)); // search only blocks with number <= 5. equivalent to without restriction for this scenario - assert_eq!( - a5.hash(), - block_on(longest_chain_select.finality_target(genesis_hash, Some(5))) - .unwrap() - .unwrap() - ); - assert_eq!( - a5.hash(), - block_on(longest_chain_select.finality_target(a1.hash(), Some(5))) - .unwrap() - .unwrap() - ); - assert_eq!( - a5.hash(), - block_on(longest_chain_select.finality_target(a2.hash(), Some(5))) - .unwrap() - .unwrap() - ); - assert_eq!( - a5.hash(), - block_on(longest_chain_select.finality_target(a3.hash(), Some(5))) - .unwrap() - .unwrap() - ); - assert_eq!( - a5.hash(), - block_on(longest_chain_select.finality_target(a4.hash(), Some(5))) - .unwrap() - .unwrap() - ); - assert_eq!( - a5.hash(), - block_on(longest_chain_select.finality_target(a5.hash(), Some(5))) - .unwrap() - .unwrap() - ); - assert_eq!( - b4.hash(), - block_on(longest_chain_select.finality_target(b2.hash(), Some(5))) - .unwrap() - .unwrap() - ); - assert_eq!( - b4.hash(), - block_on(longest_chain_select.finality_target(b3.hash(), Some(5))) - .unwrap() - .unwrap() - ); - assert_eq!( - b4.hash(), - block_on(longest_chain_select.finality_target(b4.hash(), Some(5))) - .unwrap() - .unwrap() - ); - assert_eq!( - c3.hash(), - block_on(longest_chain_select.finality_target(c3.hash(), Some(5))) - .unwrap() - .unwrap() - ); - assert_eq!( - d2.hash(), - block_on(longest_chain_select.finality_target(d2.hash(), Some(5))) - .unwrap() - .unwrap() - ); + assert_eq!(a5.hash(), finality_target(genesis_hash, Some(5))); + assert_eq!(a5.hash(), finality_target(a1.hash(), Some(5))); + assert_eq!(a5.hash(), finality_target(a2.hash(), Some(5))); + assert_eq!(a5.hash(), finality_target(a3.hash(), Some(5))); + assert_eq!(a5.hash(), finality_target(a4.hash(), Some(5))); + assert_eq!(a5.hash(), finality_target(a5.hash(), Some(5))); + assert_eq!(b4.hash(), finality_target(b2.hash(), Some(5))); + assert_eq!(b4.hash(), finality_target(b3.hash(), Some(5))); + assert_eq!(b4.hash(), finality_target(b4.hash(), Some(5))); + assert_eq!(c3.hash(), finality_target(c3.hash(), Some(5))); + assert_eq!(d2.hash(), finality_target(d2.hash(), Some(5))); // search only blocks with number <= 4 - assert_eq!( - a4.hash(), - block_on(longest_chain_select.finality_target(genesis_hash, Some(4))) - .unwrap() - .unwrap() - ); - assert_eq!( - a4.hash(), - block_on(longest_chain_select.finality_target(a1.hash(), Some(4))) - .unwrap() - .unwrap() - ); - assert_eq!( - a4.hash(), - block_on(longest_chain_select.finality_target(a2.hash(), Some(4))) - .unwrap() - .unwrap() - ); - assert_eq!( - a4.hash(), - block_on(longest_chain_select.finality_target(a3.hash(), Some(4))) - .unwrap() - .unwrap() - ); - assert_eq!( - a4.hash(), - block_on(longest_chain_select.finality_target(a4.hash(), Some(4))) - .unwrap() - .unwrap() - ); - assert_eq!(None, block_on(longest_chain_select.finality_target(a5.hash(), Some(4))).unwrap()); - assert_eq!( - b4.hash(), - block_on(longest_chain_select.finality_target(b2.hash(), Some(4))) - .unwrap() - .unwrap() - ); - assert_eq!( - b4.hash(), - block_on(longest_chain_select.finality_target(b3.hash(), Some(4))) - .unwrap() - .unwrap() - ); - assert_eq!( - b4.hash(), - block_on(longest_chain_select.finality_target(b4.hash(), Some(4))) - .unwrap() - .unwrap() - ); - assert_eq!( - c3.hash(), - block_on(longest_chain_select.finality_target(c3.hash(), Some(4))) - .unwrap() - .unwrap() - ); - assert_eq!( - d2.hash(), - block_on(longest_chain_select.finality_target(d2.hash(), Some(4))) - .unwrap() - .unwrap() - ); + assert_eq!(a4.hash(), finality_target(genesis_hash, Some(4))); + assert_eq!(a4.hash(), finality_target(a1.hash(), Some(4))); + assert_eq!(a4.hash(), finality_target(a2.hash(), Some(4))); + assert_eq!(a4.hash(), finality_target(a3.hash(), Some(4))); + assert_eq!(a4.hash(), finality_target(a4.hash(), Some(4))); + assert_eq!(a5.hash(), finality_target(a5.hash(), Some(4))); + assert_eq!(b4.hash(), finality_target(b2.hash(), Some(4))); + assert_eq!(b4.hash(), finality_target(b3.hash(), Some(4))); + assert_eq!(b4.hash(), finality_target(b4.hash(), Some(4))); + assert_eq!(c3.hash(), finality_target(c3.hash(), Some(4))); + assert_eq!(d2.hash(), finality_target(d2.hash(), Some(4))); // search only blocks with number <= 3 - assert_eq!( - a3.hash(), - block_on(longest_chain_select.finality_target(genesis_hash, Some(3))) - .unwrap() - .unwrap() - ); - assert_eq!( - a3.hash(), - block_on(longest_chain_select.finality_target(a1.hash(), Some(3))) - .unwrap() - .unwrap() - ); - assert_eq!( - a3.hash(), - block_on(longest_chain_select.finality_target(a2.hash(), Some(3))) - .unwrap() - .unwrap() - ); - assert_eq!( - a3.hash(), - block_on(longest_chain_select.finality_target(a3.hash(), Some(3))) - .unwrap() - .unwrap() - ); - assert_eq!(None, block_on(longest_chain_select.finality_target(a4.hash(), Some(3))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(a5.hash(), Some(3))).unwrap()); - assert_eq!( - b3.hash(), - block_on(longest_chain_select.finality_target(b2.hash(), Some(3))) - .unwrap() - .unwrap() - ); - assert_eq!( - b3.hash(), - block_on(longest_chain_select.finality_target(b3.hash(), Some(3))) - .unwrap() - .unwrap() - ); - assert_eq!(None, block_on(longest_chain_select.finality_target(b4.hash(), Some(3))).unwrap()); - assert_eq!( - c3.hash(), - block_on(longest_chain_select.finality_target(c3.hash(), Some(3))) - .unwrap() - .unwrap() - ); - assert_eq!( - d2.hash(), - block_on(longest_chain_select.finality_target(d2.hash(), Some(3))) - .unwrap() - .unwrap() - ); + assert_eq!(a3.hash(), finality_target(genesis_hash, Some(3))); + assert_eq!(a3.hash(), finality_target(a1.hash(), Some(3))); + assert_eq!(a3.hash(), finality_target(a2.hash(), Some(3))); + assert_eq!(a3.hash(), finality_target(a3.hash(), Some(3))); + assert_eq!(a4.hash(), finality_target(a4.hash(), Some(3))); + assert_eq!(a5.hash(), finality_target(a5.hash(), Some(3))); + assert_eq!(b3.hash(), finality_target(b2.hash(), Some(3))); + assert_eq!(b3.hash(), finality_target(b3.hash(), Some(3))); + assert_eq!(b4.hash(), finality_target(b4.hash(), Some(3))); + assert_eq!(c3.hash(), finality_target(c3.hash(), Some(3))); + assert_eq!(d2.hash(), finality_target(d2.hash(), Some(3))); // search only blocks with number <= 2 - assert_eq!( - a2.hash(), - block_on(longest_chain_select.finality_target(genesis_hash, Some(2))) - .unwrap() - .unwrap() - ); - assert_eq!( - a2.hash(), - block_on(longest_chain_select.finality_target(a1.hash(), Some(2))) - .unwrap() - .unwrap() - ); - assert_eq!( - a2.hash(), - block_on(longest_chain_select.finality_target(a2.hash(), Some(2))) - .unwrap() - .unwrap() - ); - assert_eq!(None, block_on(longest_chain_select.finality_target(a3.hash(), Some(2))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(a4.hash(), Some(2))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(a5.hash(), Some(2))).unwrap()); - assert_eq!( - b2.hash(), - block_on(longest_chain_select.finality_target(b2.hash(), Some(2))) - .unwrap() - .unwrap() - ); - assert_eq!(None, block_on(longest_chain_select.finality_target(b3.hash(), Some(2))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(b4.hash(), Some(2))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(c3.hash(), Some(2))).unwrap()); - assert_eq!( - d2.hash(), - block_on(longest_chain_select.finality_target(d2.hash(), Some(2))) - .unwrap() - .unwrap() - ); + assert_eq!(a2.hash(), finality_target(genesis_hash, Some(2))); + assert_eq!(a2.hash(), finality_target(a1.hash(), Some(2))); + assert_eq!(a2.hash(), finality_target(a2.hash(), Some(2))); + assert_eq!(a3.hash(), finality_target(a3.hash(), Some(2))); + assert_eq!(a4.hash(), finality_target(a4.hash(), Some(2))); + assert_eq!(a5.hash(), finality_target(a5.hash(), Some(2))); + assert_eq!(b2.hash(), finality_target(b2.hash(), Some(2))); + assert_eq!(b3.hash(), finality_target(b3.hash(), Some(2))); + assert_eq!(b4.hash(), finality_target(b4.hash(), Some(2))); + assert_eq!(c3.hash(), finality_target(c3.hash(), Some(2))); + assert_eq!(d2.hash(), finality_target(d2.hash(), Some(2))); // search only blocks with number <= 1 - assert_eq!( - a1.hash(), - block_on(longest_chain_select.finality_target(genesis_hash, Some(1))) - .unwrap() - .unwrap() - ); - assert_eq!( - a1.hash(), - block_on(longest_chain_select.finality_target(a1.hash(), Some(1))) - .unwrap() - .unwrap() - ); - assert_eq!(None, block_on(longest_chain_select.finality_target(a2.hash(), Some(1))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(a3.hash(), Some(1))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(a4.hash(), Some(1))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(a5.hash(), Some(1))).unwrap()); - - assert_eq!(None, block_on(longest_chain_select.finality_target(b2.hash(), Some(1))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(b3.hash(), Some(1))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(b4.hash(), Some(1))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(c3.hash(), Some(1))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(d2.hash(), Some(1))).unwrap()); + assert_eq!(a1.hash(), finality_target(genesis_hash, Some(1))); + assert_eq!(a1.hash(), finality_target(a1.hash(), Some(1))); + assert_eq!(a2.hash(), finality_target(a2.hash(), Some(1))); + assert_eq!(a3.hash(), finality_target(a3.hash(), Some(1))); + assert_eq!(a4.hash(), finality_target(a4.hash(), Some(1))); + assert_eq!(a5.hash(), finality_target(a5.hash(), Some(1))); + + assert_eq!(b2.hash(), finality_target(b2.hash(), Some(1))); + assert_eq!(b3.hash(), finality_target(b3.hash(), Some(1))); + assert_eq!(b4.hash(), finality_target(b4.hash(), Some(1))); + assert_eq!(c3.hash(), finality_target(c3.hash(), Some(1))); + assert_eq!(d2.hash(), finality_target(d2.hash(), Some(1))); // search only blocks with number <= 0 - assert_eq!( - genesis_hash, - block_on(longest_chain_select.finality_target(genesis_hash, Some(0))) - .unwrap() - .unwrap() - ); - assert_eq!(None, block_on(longest_chain_select.finality_target(a1.hash(), Some(0))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(a2.hash(), Some(0))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(a3.hash(), Some(0))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(a4.hash(), Some(0))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(a5.hash(), Some(0))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(b2.hash(), Some(0))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(b3.hash(), Some(0))).unwrap()); - assert_eq!(None, block_on(longest_chain_select.finality_target(b4.hash(), Some(0))).unwrap()); - assert_eq!( - None, - block_on(longest_chain_select.finality_target(c3.hash().clone(), Some(0))).unwrap(), - ); - assert_eq!( - None, - block_on(longest_chain_select.finality_target(d2.hash().clone(), Some(0))).unwrap(), - ); + assert_eq!(genesis_hash, finality_target(genesis_hash, Some(0))); + assert_eq!(a1.hash(), finality_target(a1.hash(), Some(0))); + assert_eq!(a2.hash(), finality_target(a2.hash(), Some(0))); + assert_eq!(a3.hash(), finality_target(a3.hash(), Some(0))); + assert_eq!(a4.hash(), finality_target(a4.hash(), Some(0))); + assert_eq!(a5.hash(), finality_target(a5.hash(), Some(0))); + assert_eq!(b2.hash(), finality_target(b2.hash(), Some(0))); + assert_eq!(b3.hash(), finality_target(b3.hash(), Some(0))); + assert_eq!(b4.hash(), finality_target(b4.hash(), Some(0))); + assert_eq!(c3.hash(), finality_target(c3.hash(), Some(0))); + assert_eq!(d2.hash(), finality_target(d2.hash(), Some(0))); } #[test] @@ -1177,9 +905,7 @@ fn best_containing_on_longest_chain_with_max_depth_higher_than_best() { assert_eq!( a2.hash(), - block_on(longest_chain_select.finality_target(genesis_hash, Some(10))) - .unwrap() - .unwrap(), + block_on(longest_chain_select.finality_target(genesis_hash, Some(10))).unwrap(), ); } @@ -2085,12 +1811,7 @@ fn cleans_up_closed_notification_sinks_on_block_import() { // NOTE: we need to build the client here instead of using the client // provided by test_runtime_client otherwise we can't access the private // `import_notification_sinks` and `finality_notification_sinks` fields. - let mut client = new_in_mem::< - _, - substrate_test_runtime_client::runtime::Block, - _, - substrate_test_runtime_client::runtime::RuntimeApi, - >( + let mut client = new_in_mem::<_, Block, _, RuntimeApi>( substrate_test_runtime_client::new_native_executor(), &substrate_test_runtime_client::GenesisParameters::default().genesis_storage(), None, @@ -2108,8 +1829,8 @@ fn cleans_up_closed_notification_sinks_on_block_import() { in_mem::Backend, sc_executor::NativeElseWasmExecutor, >, - substrate_test_runtime_client::runtime::Block, - substrate_test_runtime_client::runtime::RuntimeApi, + Block, + RuntimeApi, >; let import_notif1 = client.import_notification_stream(); diff --git a/primitives/consensus/common/src/select_chain.rs b/primitives/consensus/common/src/select_chain.rs index 5408fc86b7bd4..fd8b06ecf8abb 100644 --- a/primitives/consensus/common/src/select_chain.rs +++ b/primitives/consensus/common/src/select_chain.rs @@ -50,7 +50,7 @@ pub trait SelectChain: Sync + Send + Clone { &self, target_hash: ::Hash, _maybe_max_number: Option>, - ) -> Result::Hash>, Error> { - Ok(Some(target_hash)) + ) -> Result<::Hash, Error> { + Ok(target_hash) } }