From 0c20c827b12683f59bda66dcfa0d36892a78242a Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 31 Jan 2023 19:15:14 +0100 Subject: [PATCH 01/13] Finalization target should be chosed as some ancestor of SelectChain::best_chain --- client/consensus/common/src/longest_chain.rs | 64 ++++++- client/service/test/src/client/mod.rs | 189 +++++++++++++------ primitives/blockchain/src/backend.rs | 68 ++----- 3 files changed, 198 insertions(+), 123 deletions(-) diff --git a/client/consensus/common/src/longest_chain.rs b/client/consensus/common/src/longest_chain.rs index fab2c3a4c06fd..4d7657153326b 100644 --- a/client/consensus/common/src/longest_chain.rs +++ b/client/consensus/common/src/longest_chain.rs @@ -21,7 +21,7 @@ use sc_client_api::backend; use sp_blockchain::{Backend, HeaderBackend}; use sp_consensus::{Error as ConsensusError, SelectChain}; -use sp_runtime::traits::{Block as BlockT, NumberFor}; +use sp_runtime::traits::{Block as BlockT, Header, NumberFor}; use std::{marker::PhantomData, sync::Arc}; /// Implement Longest Chain Select implementation @@ -48,15 +48,19 @@ where LongestChain { backend, _phantom: Default::default() } } - fn best_block_header(&self) -> sp_blockchain::Result<::Header> { + fn best_hash(&self) -> sp_blockchain::Result<::Hash> { let info = self.backend.blockchain().info(); let import_lock = self.backend.get_import_lock(); let best_hash = self .backend .blockchain() - .best_containing(info.best_hash, None, import_lock)? + .best_containing(info.best_hash, import_lock)? .unwrap_or(info.best_hash); + Ok(best_hash) + } + fn best_header(&self) -> sp_blockchain::Result<::Header> { + let best_hash = self.best_hash()?; Ok(self .backend .blockchain() @@ -64,6 +68,51 @@ where .expect("given block hash was fetched from block in db; qed")) } + /// If `maybe_max_block_number` is `Some(max_block_number)` + /// the search is limited to block `numbers <= max_block_number`. + /// in other words as if there were no blocks greater `max_block_number`. + fn finality_target( + &self, + target_hash: Block::Hash, + maybe_max_number: Option>, + ) -> sp_blockchain::Result { + use sp_blockchain::Error::{MissingHeader, NotInFinalizedChain}; + let blockchain = self.backend.blockchain(); + + let mut current_head = self.best_header()?; + let mut best_hash = current_head.hash(); + + let target_header = blockchain + .header(target_hash)? + .ok_or_else(|| MissingHeader(target_hash.to_string()))?; + let target_number = *target_header.number(); + + if let Some(max_number) = maybe_max_number { + if max_number < target_number { + return Err(NotInFinalizedChain) + } + + while current_head.number() > &max_number { + best_hash = *current_head.parent_hash(); + current_head = blockchain + .header(best_hash)? + .ok_or_else(|| MissingHeader(best_hash.to_string()))?; + } + } + + while current_head.hash() != target_hash { + if *current_head.number() < target_number { + return Err(NotInFinalizedChain) + } + let current_hash = *current_head.parent_hash(); + current_head = blockchain + .header(current_hash)? + .ok_or_else(|| MissingHeader(current_hash.to_string()))?; + } + + return Ok(best_hash) + } + fn leaves(&self) -> Result::Hash>, sp_blockchain::Error> { self.backend.blockchain().leaves() } @@ -80,8 +129,7 @@ where } async fn best_chain(&self) -> Result<::Header, ConsensusError> { - LongestChain::best_block_header(self) - .map_err(|e| ConsensusError::ChainLookup(e.to_string())) + LongestChain::best_header(self).map_err(|e| ConsensusError::ChainLookup(e.to_string())) } async fn finality_target( @@ -89,11 +137,7 @@ where target_hash: Block::Hash, maybe_max_number: Option>, ) -> 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)) + LongestChain::finality_target(self, target_hash, maybe_max_number) .map_err(|e| ConsensusError::ChainLookup(e.to_string())) } } diff --git a/client/service/test/src/client/mod.rs b/client/service/test/src/client/mod.rs index 97c22a1cb509e..0f99749c56517 100644 --- a/client/service/test/src/client/mod.rs +++ b/client/service/test/src/client/mod.rs @@ -731,8 +731,13 @@ 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() + let error_hash = Hash::from([0xff; 32]); + // the quite improbable error_hash is retuned on error + let finality_target = |target_hash, number| match block_on( + longest_chain_select.finality_target(target_hash, number), + ) { + Ok(hash) => hash, + Err(_) => error_hash, }; // search without restriction @@ -742,11 +747,11 @@ fn best_containing_on_longest_chain_with_multiple_forks() { 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)); + assert_eq!(error_hash, finality_target(b2.hash(), None)); + assert_eq!(error_hash, finality_target(b3.hash(), None)); + assert_eq!(error_hash, finality_target(b4.hash(), None)); + assert_eq!(error_hash, finality_target(c3.hash(), None)); + assert_eq!(error_hash, finality_target(d2.hash(), None)); // search only blocks with number <= 5. equivalent to without restriction for this scenario assert_eq!(a5.hash(), finality_target(genesis_hash, Some(5))); @@ -755,11 +760,11 @@ fn best_containing_on_longest_chain_with_multiple_forks() { 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))); + assert_eq!(error_hash, finality_target(b2.hash(), Some(5))); + assert_eq!(error_hash, finality_target(b3.hash(), Some(5))); + assert_eq!(error_hash, finality_target(b4.hash(), Some(5))); + assert_eq!(error_hash, finality_target(c3.hash(), Some(5))); + assert_eq!(error_hash, finality_target(d2.hash(), Some(5))); // search only blocks with number <= 4 assert_eq!(a4.hash(), finality_target(genesis_hash, Some(4))); @@ -767,65 +772,64 @@ fn best_containing_on_longest_chain_with_multiple_forks() { 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))); + assert_eq!(error_hash, finality_target(a5.hash(), Some(4))); + assert_eq!(error_hash, finality_target(b2.hash(), Some(4))); + assert_eq!(error_hash, finality_target(b3.hash(), Some(4))); + assert_eq!(error_hash, finality_target(b4.hash(), Some(4))); + assert_eq!(error_hash, finality_target(c3.hash(), Some(4))); + assert_eq!(error_hash, finality_target(d2.hash(), Some(4))); // search only blocks with number <= 3 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))); + assert_eq!(error_hash, finality_target(a4.hash(), Some(3))); + assert_eq!(error_hash, finality_target(a5.hash(), Some(3))); + assert_eq!(error_hash, finality_target(b2.hash(), Some(3))); + assert_eq!(error_hash, finality_target(b3.hash(), Some(3))); + assert_eq!(error_hash, finality_target(b4.hash(), Some(3))); + assert_eq!(error_hash, finality_target(c3.hash(), Some(3))); + assert_eq!(error_hash, finality_target(d2.hash(), Some(3))); // search only blocks with number <= 2 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))); + assert_eq!(error_hash, finality_target(a3.hash(), Some(2))); + assert_eq!(error_hash, finality_target(a4.hash(), Some(2))); + assert_eq!(error_hash, finality_target(a5.hash(), Some(2))); + assert_eq!(error_hash, finality_target(b2.hash(), Some(2))); + assert_eq!(error_hash, finality_target(b3.hash(), Some(2))); + assert_eq!(error_hash, finality_target(b4.hash(), Some(2))); + assert_eq!(error_hash, finality_target(c3.hash(), Some(2))); + assert_eq!(error_hash, finality_target(d2.hash(), Some(2))); // search only blocks with number <= 1 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))); + assert_eq!(error_hash, finality_target(a2.hash(), Some(1))); + assert_eq!(error_hash, finality_target(a3.hash(), Some(1))); + assert_eq!(error_hash, finality_target(a4.hash(), Some(1))); + assert_eq!(error_hash, finality_target(a5.hash(), Some(1))); + assert_eq!(error_hash, finality_target(b2.hash(), Some(1))); + assert_eq!(error_hash, finality_target(b3.hash(), Some(1))); + assert_eq!(error_hash, finality_target(b4.hash(), Some(1))); + assert_eq!(error_hash, finality_target(c3.hash(), Some(1))); + assert_eq!(error_hash, finality_target(d2.hash(), Some(1))); // search only blocks with number <= 0 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))); + assert_eq!(error_hash, finality_target(a1.hash(), Some(0))); + assert_eq!(error_hash, finality_target(a2.hash(), Some(0))); + assert_eq!(error_hash, finality_target(a3.hash(), Some(0))); + assert_eq!(error_hash, finality_target(a4.hash(), Some(0))); + assert_eq!(error_hash, finality_target(a5.hash(), Some(0))); + assert_eq!(error_hash, finality_target(b2.hash(), Some(0))); + assert_eq!(error_hash, finality_target(b3.hash(), Some(0))); + assert_eq!(error_hash, finality_target(b4.hash(), Some(0))); + assert_eq!(error_hash, finality_target(c3.hash(), Some(0))); + assert_eq!(error_hash, finality_target(d2.hash(), Some(0))); } #[test] @@ -851,6 +855,77 @@ fn best_containing_on_longest_chain_with_max_depth_higher_than_best() { ); } +#[test] +fn finality_target_with_best_not_on_longest_chain() { + // block tree: + // G -> A1 -> A2 -> A3 -> A4 + // -> B2 -> B3 + // ^best + + let (mut client, longest_chain_select) = TestClientBuilder::new().build_with_longest_chain(); + + // G -> A1 + let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; + block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap(); + + // A1 -> A2 + let a2 = client.new_block(Default::default()).unwrap().build().unwrap().block; + block_on(client.import(BlockOrigin::Own, a2.clone())).unwrap(); + + // A2 -> A3 + let a3 = client.new_block(Default::default()).unwrap().build().unwrap().block; + block_on(client.import(BlockOrigin::Own, a3.clone())).unwrap(); + + // A3 -> A4 + let a4 = client.new_block(Default::default()).unwrap().build().unwrap().block; + block_on(client.import(BlockOrigin::Own, a4.clone())).unwrap(); + + // A3 -> A5 + let a5 = client.new_block(Default::default()).unwrap().build().unwrap().block; + block_on(client.import(BlockOrigin::Own, a5.clone())).unwrap(); + + // A1 -> B2 + let mut builder = client + .new_block_at(&BlockId::Hash(a1.hash()), Default::default(), false) + .unwrap(); + // this push is required as otherwise B2 has the same hash as A2 and won't get imported + builder + .push_transfer(Transfer { + from: AccountKeyring::Alice.into(), + to: AccountKeyring::Ferdie.into(), + amount: 41, + nonce: 0, + }) + .unwrap(); + let b2 = builder.build().unwrap().block; + block_on(client.import(BlockOrigin::Own, b2.clone())).unwrap(); + + // B2 -> B3 + let b3 = client + .new_block_at(&BlockId::Hash(b2.hash()), Default::default(), false) + .unwrap() + .build() + .unwrap() + .block; + block_on(client.import_as_best(BlockOrigin::Own, b3.clone())).unwrap(); + + // B3 -> B4 + let b4 = client + .new_block_at(&BlockId::Hash(b3.hash()), Default::default(), false) + .unwrap() + .build() + .unwrap() + .block; + block_on(client.import(BlockOrigin::Own, b4.clone())).unwrap(); + + let genesis_hash = client.chain_info().genesis_hash; + + assert_eq!( + b4.hash(), + block_on(longest_chain_select.finality_target(genesis_hash, Some(10))).unwrap(), + ); +} + #[test] fn import_with_justification() { // block tree: @@ -995,7 +1070,7 @@ fn finalizing_diverged_block_should_trigger_reorg() { .block; block_on(client.import(BlockOrigin::Own, b2.clone())).unwrap(); - // A2 is the current best since it's the longest chain + // A2 is the current best since it's the (first) longest chain assert_eq!(client.chain_info().best_hash, a2.hash()); // we finalize block B1 which is on a different branch from current best @@ -1012,8 +1087,7 @@ fn finalizing_diverged_block_should_trigger_reorg() { // `SelectChain` should report B2 as best block though assert_eq!(block_on(select_chain.best_chain()).unwrap().hash(), b2.hash()); - // after we build B3 on top of B2 and import it - // it should be the new best block, + // after we build B3 on top of B2 and import it, it should be the new best block let b3 = client .new_block_at(&BlockId::Hash(b2.hash()), Default::default(), false) .unwrap() @@ -1022,6 +1096,9 @@ fn finalizing_diverged_block_should_trigger_reorg() { .block; block_on(client.import(BlockOrigin::Own, b3.clone())).unwrap(); + // `SelectChain` should report B3 as best block though + assert_eq!(block_on(select_chain.best_chain()).unwrap().hash(), b3.hash()); + assert_eq!(client.chain_info().best_hash, b3.hash()); ClientExt::finalize_block(&client, b3.hash(), None).unwrap(); diff --git a/primitives/blockchain/src/backend.rs b/primitives/blockchain/src/backend.rs index ec9c8ac0d5780..bb324b54c45f0 100644 --- a/primitives/blockchain/src/backend.rs +++ b/primitives/blockchain/src/backend.rs @@ -184,7 +184,7 @@ pub trait Backend: fn children(&self, parent_hash: Block::Hash) -> Result>; /// Get the most recent block hash of the best (longest) chains - /// that contain block with the given `target_hash`. + /// that contain block with the given `base_hash`. /// /// The search space is always limited to blocks which are in the finalized /// chain or descendents of it. @@ -192,87 +192,42 @@ pub trait Backend: /// If `maybe_max_block_number` is `Some(max_block_number)` /// the search is limited to block `numbers <= max_block_number`. /// in other words as if there were no blocks greater `max_block_number`. + /// /// Returns `Ok(None)` if `target_hash` is not found in search space. - /// TODO: document time complexity of this, see [#1444](https://github.com/paritytech/substrate/issues/1444) + // TODO: document time complexity of this, see [#1444](https://github.com/paritytech/substrate/issues/1444) fn best_containing( &self, - target_hash: Block::Hash, - maybe_max_number: Option>, + base_hash: Block::Hash, import_lock: &RwLock<()>, ) -> Result> { let target_header = { - match self.header(target_hash)? { + match self.header(base_hash)? { Some(x) => x, // target not in blockchain None => return Ok(None), } }; - if let Some(max_number) = maybe_max_number { - // target outside search range - if target_header.number() > &max_number { - return Ok(None) - } - } - let leaves = { // ensure no blocks are imported during this code block. // an import could trigger a reorg which could change the canonical chain. // we depend on the canonical chain staying the same during this code block. let _import_guard = import_lock.read(); - let info = self.info(); - - // this can be `None` if the best chain is shorter than the target header. - let maybe_canon_hash = self.hash(*target_header.number())?; - - if maybe_canon_hash.as_ref() == Some(&target_hash) { - // if a `max_number` is given we try to fetch the block at the - // given depth, if it doesn't exist or `max_number` is not - // provided, we continue to search from all leaves below. - if let Some(max_number) = maybe_max_number { - if let Some(header) = self.hash(max_number)? { - return Ok(Some(header)) - } - } - } else if info.finalized_number >= *target_header.number() { - // header is on a dead fork. + if info.finalized_number > *target_header.number() { + // `target_header` is on a dead fork. return Ok(None) } - self.leaves()? }; // for each chain. longest chain first. shortest last for leaf_hash in leaves { - // start at the leaf let mut current_hash = leaf_hash; - - // if search is not restricted then the leaf is the best - let mut best_hash = leaf_hash; - - // go backwards entering the search space - // waiting until we are <= max_number - if let Some(max_number) = maybe_max_number { - loop { - let current_header = self - .header(current_hash)? - .ok_or_else(|| Error::MissingHeader(current_hash.to_string()))?; - - if current_header.number() <= &max_number { - best_hash = current_header.hash(); - break - } - - current_hash = *current_header.parent_hash(); - } - } - // go backwards through the chain (via parent links) loop { - // until we find target - if current_hash == target_hash { - return Ok(Some(best_hash)) + if current_hash == base_hash { + return Ok(Some(leaf_hash)) } let current_header = self @@ -293,9 +248,8 @@ pub trait Backend: // // FIXME #1558 only issue this warning when not on a dead fork warn!( - "Block {:?} exists in chain but not found when following all \ - leaves backwards. Number limit = {:?}", - target_hash, maybe_max_number, + "Block {:?} exists in chain but not found when following all leaves backwards", + base_hash, ); Ok(None) From 37237178d44d4b8c68b8e6dee65c2dd9c1ce07cd Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 31 Jan 2023 19:49:35 +0100 Subject: [PATCH 02/13] More test assertions --- client/service/test/src/client/mod.rs | 52 ++++++++++++++++----------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/client/service/test/src/client/mod.rs b/client/service/test/src/client/mod.rs index 0f99749c56517..58700045c387b 100644 --- a/client/service/test/src/client/mod.rs +++ b/client/service/test/src/client/mod.rs @@ -581,7 +581,7 @@ fn uncles_with_multiple_forks() { } #[test] -fn best_containing_on_longest_chain_with_single_chain_3_blocks() { +fn finality_target_on_longest_chain_with_single_chain_3_blocks() { // block tree: // G -> A1 -> A2 @@ -606,7 +606,7 @@ fn best_containing_on_longest_chain_with_single_chain_3_blocks() { } #[test] -fn best_containing_on_longest_chain_with_multiple_forks() { +fn finality_target_on_longest_chain_with_multiple_forks() { // block tree: // G -> A1 -> A2 -> A3 -> A4 -> A5 // A1 -> B2 -> B3 -> B4 @@ -731,8 +731,8 @@ fn best_containing_on_longest_chain_with_multiple_forks() { assert!(leaves.contains(&d2.hash())); assert_eq!(leaves.len(), 4); + // A quite improbable hash is retuned on error let error_hash = Hash::from([0xff; 32]); - // the quite improbable error_hash is retuned on error let finality_target = |target_hash, number| match block_on( longest_chain_select.finality_target(target_hash, number), ) { @@ -833,11 +833,11 @@ fn best_containing_on_longest_chain_with_multiple_forks() { } #[test] -fn best_containing_on_longest_chain_with_max_depth_higher_than_best() { +fn finality_target_on_longest_chain_with_max_depth_higher_than_best() { // block tree: // G -> A1 -> A2 - let (mut client, longest_chain_select) = TestClientBuilder::new().build_with_longest_chain(); + let (mut client, chain_select) = TestClientBuilder::new().build_with_longest_chain(); // G -> A1 let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; @@ -849,20 +849,18 @@ fn best_containing_on_longest_chain_with_max_depth_higher_than_best() { let genesis_hash = client.chain_info().genesis_hash; - assert_eq!( - a2.hash(), - block_on(longest_chain_select.finality_target(genesis_hash, Some(10))).unwrap(), - ); + assert_eq!(a2.hash(), block_on(chain_select.finality_target(genesis_hash, Some(10))).unwrap(),); } #[test] fn finality_target_with_best_not_on_longest_chain() { // block tree: - // G -> A1 -> A2 -> A3 -> A4 - // -> B2 -> B3 + // G -> A1 -> A2 -> A3 -> A4 -> A5 + // -> B2 -> (B3) -> B4 // ^best - let (mut client, longest_chain_select) = TestClientBuilder::new().build_with_longest_chain(); + let (mut client, chain_select) = TestClientBuilder::new().build_with_longest_chain(); + let genesis_hash = client.chain_info().genesis_hash; // G -> A1 let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block; @@ -900,6 +898,13 @@ fn finality_target_with_best_not_on_longest_chain() { let b2 = builder.build().unwrap().block; block_on(client.import(BlockOrigin::Own, b2.clone())).unwrap(); + assert_eq!(a5.hash(), block_on(chain_select.finality_target(genesis_hash, None)).unwrap()); + assert_eq!(a5.hash(), block_on(chain_select.finality_target(a1.hash(), None)).unwrap()); + assert_eq!(a5.hash(), block_on(chain_select.finality_target(a2.hash(), None)).unwrap()); + assert_eq!(a5.hash(), block_on(chain_select.finality_target(a3.hash(), None)).unwrap()); + assert_eq!(a5.hash(), block_on(chain_select.finality_target(a4.hash(), None)).unwrap()); + assert_eq!(a5.hash(), block_on(chain_select.finality_target(a5.hash(), None)).unwrap()); + // B2 -> B3 let b3 = client .new_block_at(&BlockId::Hash(b2.hash()), Default::default(), false) @@ -916,14 +921,21 @@ fn finality_target_with_best_not_on_longest_chain() { .build() .unwrap() .block; - block_on(client.import(BlockOrigin::Own, b4.clone())).unwrap(); - - let genesis_hash = client.chain_info().genesis_hash; - - assert_eq!( - b4.hash(), - block_on(longest_chain_select.finality_target(genesis_hash, Some(10))).unwrap(), - ); + let (header, extrinsics) = b4.clone().deconstruct(); + let mut import_params = BlockImportParams::new(BlockOrigin::Own, header); + import_params.body = Some(extrinsics); + import_params.fork_choice = Some(ForkChoiceStrategy::Custom(false)); + block_on(client.import_block(import_params, Default::default())).unwrap(); + + // double check that B3 is still the best... + assert_eq!(client.info().best_hash, b3.hash()); + + assert_eq!(b4.hash(), block_on(chain_select.finality_target(genesis_hash, None)).unwrap()); + assert_eq!(b4.hash(), block_on(chain_select.finality_target(a1.hash(), None)).unwrap()); + assert!(block_on(chain_select.finality_target(a2.hash(), None)).is_err()); + assert_eq!(b4.hash(), block_on(chain_select.finality_target(b2.hash(), None)).unwrap()); + assert_eq!(b4.hash(), block_on(chain_select.finality_target(b3.hash(), None)).unwrap()); + assert_eq!(b4.hash(), block_on(chain_select.finality_target(b4.hash(), None)).unwrap()); } #[test] From a6ee614f68de3f2d39b7103ae4ced053992f3ec2 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 1 Feb 2023 09:40:05 +0100 Subject: [PATCH 03/13] Improve docs --- client/consensus/common/src/longest_chain.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client/consensus/common/src/longest_chain.rs b/client/consensus/common/src/longest_chain.rs index 4d7657153326b..5d9d145bb132b 100644 --- a/client/consensus/common/src/longest_chain.rs +++ b/client/consensus/common/src/longest_chain.rs @@ -68,6 +68,8 @@ where .expect("given block hash was fetched from block in db; qed")) } + /// Returns the highest descendant of the "best" block. + /// /// If `maybe_max_block_number` is `Some(max_block_number)` /// the search is limited to block `numbers <= max_block_number`. /// in other words as if there were no blocks greater `max_block_number`. From 865a0979e1c22d497ec0038ff719ccd34bad0c45 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 1 Feb 2023 10:06:37 +0100 Subject: [PATCH 04/13] Removed stale docs --- primitives/blockchain/src/backend.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/primitives/blockchain/src/backend.rs b/primitives/blockchain/src/backend.rs index bb324b54c45f0..c68385369ef77 100644 --- a/primitives/blockchain/src/backend.rs +++ b/primitives/blockchain/src/backend.rs @@ -189,10 +189,6 @@ pub trait Backend: /// The search space is always limited to blocks which are in the finalized /// chain or descendents of it. /// - /// If `maybe_max_block_number` is `Some(max_block_number)` - /// the search is limited to block `numbers <= max_block_number`. - /// in other words as if there were no blocks greater `max_block_number`. - /// /// Returns `Ok(None)` if `target_hash` is not found in search space. // TODO: document time complexity of this, see [#1444](https://github.com/paritytech/substrate/issues/1444) fn best_containing( From 7f1b5f5b0506d272198424b169946a9b64db1b59 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 1 Feb 2023 10:11:54 +0100 Subject: [PATCH 05/13] Rename 'target' to 'base' in lookup method --- primitives/blockchain/src/backend.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/primitives/blockchain/src/backend.rs b/primitives/blockchain/src/backend.rs index c68385369ef77..8329da2714c74 100644 --- a/primitives/blockchain/src/backend.rs +++ b/primitives/blockchain/src/backend.rs @@ -183,25 +183,21 @@ pub trait Backend: /// Return hashes of all blocks that are children of the block with `parent_hash`. fn children(&self, parent_hash: Block::Hash) -> Result>; - /// Get the most recent block hash of the best (longest) chains - /// that contain block with the given `base_hash`. + /// Get the most recent block hash of the longest chain that contains + /// a block with the given `base_hash`. /// /// The search space is always limited to blocks which are in the finalized /// chain or descendents of it. /// - /// Returns `Ok(None)` if `target_hash` is not found in search space. + /// Returns `Ok(None)` if `base_hash` is not found in search space. // TODO: document time complexity of this, see [#1444](https://github.com/paritytech/substrate/issues/1444) fn best_containing( &self, base_hash: Block::Hash, import_lock: &RwLock<()>, ) -> Result> { - let target_header = { - match self.header(base_hash)? { - Some(x) => x, - // target not in blockchain - None => return Ok(None), - } + let Some(base_header) = self.header(base_hash)? else { + return Ok(None) }; let leaves = { @@ -210,8 +206,8 @@ pub trait Backend: // we depend on the canonical chain staying the same during this code block. let _import_guard = import_lock.read(); let info = self.info(); - if info.finalized_number > *target_header.number() { - // `target_header` is on a dead fork. + if info.finalized_number > *base_header.number() { + // `base_header` is on a dead fork. return Ok(None) } self.leaves()? @@ -231,7 +227,7 @@ pub trait Backend: .ok_or_else(|| Error::MissingHeader(current_hash.to_string()))?; // stop search in this chain once we go below the target's block number - if current_header.number() < target_header.number() { + if current_header.number() < base_header.number() { break } From 28128cf447537555c7c3b5904adedc6bd0036820 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Fri, 3 Feb 2023 12:42:55 +0100 Subject: [PATCH 06/13] Fix typo --- client/service/test/src/client/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/service/test/src/client/mod.rs b/client/service/test/src/client/mod.rs index 58700045c387b..12b92afc458b4 100644 --- a/client/service/test/src/client/mod.rs +++ b/client/service/test/src/client/mod.rs @@ -731,7 +731,7 @@ fn finality_target_on_longest_chain_with_multiple_forks() { assert!(leaves.contains(&d2.hash())); assert_eq!(leaves.len(), 4); - // A quite improbable hash is retuned on error + // On error we return a quite improbable hash let error_hash = Hash::from([0xff; 32]); let finality_target = |target_hash, number| match block_on( longest_chain_select.finality_target(target_hash, number), From 2ebf7e5d3ec1e1454eb90c2bc6efc4e6e0f8f602 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 7 Feb 2023 17:50:02 +0100 Subject: [PATCH 07/13] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> --- client/consensus/common/src/longest_chain.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/consensus/common/src/longest_chain.rs b/client/consensus/common/src/longest_chain.rs index 5d9d145bb132b..3f8965a55de64 100644 --- a/client/consensus/common/src/longest_chain.rs +++ b/client/consensus/common/src/longest_chain.rs @@ -68,11 +68,11 @@ where .expect("given block hash was fetched from block in db; qed")) } - /// Returns the highest descendant of the "best" block. + /// Returns the highest descendant of the given target block. /// /// If `maybe_max_block_number` is `Some(max_block_number)` /// the search is limited to block `numbers <= max_block_number`. - /// in other words as if there were no blocks greater `max_block_number`. + /// in other words as if there were no blocks greater than `max_block_number`. fn finality_target( &self, target_hash: Block::Hash, From 5226f9d7af8c26f924a0f87de9f680e37367e78f Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 7 Feb 2023 17:58:25 +0100 Subject: [PATCH 08/13] Rename 'target_hash' to 'base_hash' in 'SelectChain::finality_target()' --- client/consensus/common/src/longest_chain.rs | 12 ++++++------ client/finality-grandpa/src/tests.rs | 2 +- primitives/consensus/common/src/select_chain.rs | 8 ++++---- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/client/consensus/common/src/longest_chain.rs b/client/consensus/common/src/longest_chain.rs index 3f8965a55de64..90b8603efd6f2 100644 --- a/client/consensus/common/src/longest_chain.rs +++ b/client/consensus/common/src/longest_chain.rs @@ -75,7 +75,7 @@ where /// in other words as if there were no blocks greater than `max_block_number`. fn finality_target( &self, - target_hash: Block::Hash, + base_hash: Block::Hash, maybe_max_number: Option>, ) -> sp_blockchain::Result { use sp_blockchain::Error::{MissingHeader, NotInFinalizedChain}; @@ -85,8 +85,8 @@ where let mut best_hash = current_head.hash(); let target_header = blockchain - .header(target_hash)? - .ok_or_else(|| MissingHeader(target_hash.to_string()))?; + .header(base_hash)? + .ok_or_else(|| MissingHeader(base_hash.to_string()))?; let target_number = *target_header.number(); if let Some(max_number) = maybe_max_number { @@ -102,7 +102,7 @@ where } } - while current_head.hash() != target_hash { + while current_head.hash() != base_hash { if *current_head.number() < target_number { return Err(NotInFinalizedChain) } @@ -136,10 +136,10 @@ where async fn finality_target( &self, - target_hash: Block::Hash, + base_hash: Block::Hash, maybe_max_number: Option>, ) -> Result { - LongestChain::finality_target(self, target_hash, maybe_max_number) + LongestChain::finality_target(self, base_hash, maybe_max_number) .map_err(|e| ConsensusError::ChainLookup(e.to_string())) } } diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 07ea4649148fb..62ef4709e1909 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -237,7 +237,7 @@ impl SelectChain for MockSelectChain { async fn finality_target( &self, - _target_hash: Hash, + _base_hash: Hash, _maybe_max_number: Option>, ) -> Result { Ok(self.finality_target.lock().take().unwrap()) diff --git a/primitives/consensus/common/src/select_chain.rs b/primitives/consensus/common/src/select_chain.rs index f366cd34c51ea..5beab6705046d 100644 --- a/primitives/consensus/common/src/select_chain.rs +++ b/primitives/consensus/common/src/select_chain.rs @@ -43,14 +43,14 @@ pub trait SelectChain: Sync + Send + Clone { /// finalize. async fn best_chain(&self) -> Result<::Header, Error>; - /// Get the best descendent of `target_hash` that we should attempt to - /// finalize next, if any. It is valid to return the given `target_hash` + /// Get the best descendent of `base_hash` that we should attempt to + /// finalize next, if any. It is valid to return the given `base_hash` /// itself if no better descendent exists. async fn finality_target( &self, - target_hash: ::Hash, + base_hash: ::Hash, _maybe_max_number: Option>, ) -> Result<::Hash, Error> { - Ok(target_hash) + Ok(base_hash) } } From a21a3115cf4e4c39cbb38d3a33af21722f74298d Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 8 Feb 2023 15:10:29 +0100 Subject: [PATCH 09/13] Apply suggestions from code review Co-authored-by: Anton --- client/consensus/common/src/longest_chain.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/consensus/common/src/longest_chain.rs b/client/consensus/common/src/longest_chain.rs index 90b8603efd6f2..100010abf0733 100644 --- a/client/consensus/common/src/longest_chain.rs +++ b/client/consensus/common/src/longest_chain.rs @@ -70,8 +70,8 @@ where /// Returns the highest descendant of the given target block. /// - /// If `maybe_max_block_number` is `Some(max_block_number)` - /// the search is limited to block `numbers <= max_block_number`. + /// If `maybe_max_number` is `Some(max_block_number)` + /// the search is limited to block `number <= max_block_number`. /// in other words as if there were no blocks greater than `max_block_number`. fn finality_target( &self, From 5512c05deda6ecea1b3178fac8b1f6c0c81b2bb5 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 8 Feb 2023 15:11:42 +0100 Subject: [PATCH 10/13] Docs improvement --- client/consensus/common/src/longest_chain.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/client/consensus/common/src/longest_chain.rs b/client/consensus/common/src/longest_chain.rs index 100010abf0733..bd5931b3b0a0d 100644 --- a/client/consensus/common/src/longest_chain.rs +++ b/client/consensus/common/src/longest_chain.rs @@ -68,7 +68,11 @@ where .expect("given block hash was fetched from block in db; qed")) } - /// Returns the highest descendant of the given target block. + /// Returns the highest descendant of the given block that is + /// a valid candidate to be finalized. + /// + /// In this implementation being valid means that is a descendant of the + /// best chain according to the `best_header` method. /// /// If `maybe_max_number` is `Some(max_block_number)` /// the search is limited to block `number <= max_block_number`. From 85aa43c773956270515a88fbf912dab39bee246f Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 8 Feb 2023 15:15:49 +0100 Subject: [PATCH 11/13] Doc fix --- client/consensus/common/src/longest_chain.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/client/consensus/common/src/longest_chain.rs b/client/consensus/common/src/longest_chain.rs index bd5931b3b0a0d..23be2e5ff9944 100644 --- a/client/consensus/common/src/longest_chain.rs +++ b/client/consensus/common/src/longest_chain.rs @@ -68,15 +68,15 @@ where .expect("given block hash was fetched from block in db; qed")) } - /// Returns the highest descendant of the given block that is - /// a valid candidate to be finalized. + /// Returns the highest descendant of the given block that is a valid + /// candidate to be finalized. /// - /// In this implementation being valid means that is a descendant of the - /// best chain according to the `best_header` method. + /// In this context, being a valid target means being an ancestor of + /// the best chain according to the `best_header` method. /// - /// If `maybe_max_number` is `Some(max_block_number)` - /// the search is limited to block `number <= max_block_number`. - /// in other words as if there were no blocks greater than `max_block_number`. + /// If `maybe_max_number` is `Some(max_block_number)` the search is + /// limited to block `number <= max_block_number`. In other words + /// as if there were no blocks greater than `max_block_number`. fn finality_target( &self, base_hash: Block::Hash, From 013f303a646d3be9e83806341bcd0c3de6ad76fa Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Sat, 11 Feb 2023 16:30:54 +0100 Subject: [PATCH 12/13] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- client/consensus/common/src/longest_chain.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/client/consensus/common/src/longest_chain.rs b/client/consensus/common/src/longest_chain.rs index 23be2e5ff9944..2d61b27c9ecd9 100644 --- a/client/consensus/common/src/longest_chain.rs +++ b/client/consensus/common/src/longest_chain.rs @@ -102,7 +102,7 @@ where best_hash = *current_head.parent_hash(); current_head = blockchain .header(best_hash)? - .ok_or_else(|| MissingHeader(best_hash.to_string()))?; + .ok_or_else(|| MissingHeader(format!("{best_hash:?}")))?; } } @@ -113,10 +113,10 @@ where let current_hash = *current_head.parent_hash(); current_head = blockchain .header(current_hash)? - .ok_or_else(|| MissingHeader(current_hash.to_string()))?; + .ok_or_else(|| MissingHeader(format!("{best_hash:?}")))?; } - return Ok(best_hash) + Ok(best_hash) } fn leaves(&self) -> Result::Hash>, sp_blockchain::Error> { From 89ea0b898975c149b208f62138a8950d342d33e9 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Sat, 11 Feb 2023 17:03:43 +0100 Subject: [PATCH 13/13] Apply more code suggestions --- client/consensus/common/src/longest_chain.rs | 24 +++++++++++++------- primitives/blockchain/src/backend.rs | 2 +- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/client/consensus/common/src/longest_chain.rs b/client/consensus/common/src/longest_chain.rs index 2d61b27c9ecd9..ece2486209ff3 100644 --- a/client/consensus/common/src/longest_chain.rs +++ b/client/consensus/common/src/longest_chain.rs @@ -54,7 +54,7 @@ where let best_hash = self .backend .blockchain() - .best_containing(info.best_hash, import_lock)? + .longest_containing(info.best_hash, import_lock)? .unwrap_or(info.best_hash); Ok(best_hash) } @@ -82,20 +82,24 @@ where base_hash: Block::Hash, maybe_max_number: Option>, ) -> sp_blockchain::Result { - use sp_blockchain::Error::{MissingHeader, NotInFinalizedChain}; + use sp_blockchain::Error::{Application, MissingHeader}; let blockchain = self.backend.blockchain(); let mut current_head = self.best_header()?; let mut best_hash = current_head.hash(); - let target_header = blockchain + let base_header = blockchain .header(base_hash)? .ok_or_else(|| MissingHeader(base_hash.to_string()))?; - let target_number = *target_header.number(); + let base_number = *base_header.number(); if let Some(max_number) = maybe_max_number { - if max_number < target_number { - return Err(NotInFinalizedChain) + if max_number < base_number { + let msg = format!( + "Requested a finality target using max number {} below the base number {}", + max_number, base_number + ); + return Err(Application(msg.into())) } while current_head.number() > &max_number { @@ -107,8 +111,12 @@ where } while current_head.hash() != base_hash { - if *current_head.number() < target_number { - return Err(NotInFinalizedChain) + if *current_head.number() < base_number { + let msg = format!( + "Requested a finality target using a base {:?} not in the best chain {:?}", + base_hash, best_hash, + ); + return Err(Application(msg.into())) } let current_hash = *current_head.parent_hash(); current_head = blockchain diff --git a/primitives/blockchain/src/backend.rs b/primitives/blockchain/src/backend.rs index 8329da2714c74..7339d4c1a6804 100644 --- a/primitives/blockchain/src/backend.rs +++ b/primitives/blockchain/src/backend.rs @@ -191,7 +191,7 @@ pub trait Backend: /// /// Returns `Ok(None)` if `base_hash` is not found in search space. // TODO: document time complexity of this, see [#1444](https://github.com/paritytech/substrate/issues/1444) - fn best_containing( + fn longest_containing( &self, base_hash: Block::Hash, import_lock: &RwLock<()>,