From 150997ed13ce4ec82da477697534d06e55da671b Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Mon, 27 May 2024 13:52:40 +0200 Subject: [PATCH] chore(docs): clarify tree canonical chain docs --- crates/blockchain-tree/src/block_indices.rs | 20 ++---- crates/blockchain-tree/src/blockchain_tree.rs | 63 ++++++++++--------- crates/blockchain-tree/src/canonical_chain.rs | 28 +++------ crates/blockchain-tree/src/state.rs | 4 +- 4 files changed, 49 insertions(+), 66 deletions(-) diff --git a/crates/blockchain-tree/src/block_indices.rs b/crates/blockchain-tree/src/block_indices.rs index 373b419b3753..875b6fbe1010 100644 --- a/crates/blockchain-tree/src/block_indices.rs +++ b/crates/blockchain-tree/src/block_indices.rs @@ -17,9 +17,9 @@ use std::collections::{btree_map, hash_map, BTreeMap, BTreeSet, HashMap, HashSet pub struct BlockIndices { /// Last finalized block. last_finalized_block: BlockNumber, - /// Canonical chain. Contains N number (depends on `finalization_depth`) of blocks. - /// These blocks are found in fork_to_child but not inside `blocks_to_chain` or - /// `number_to_block` as those are chain specific indices. + /// Non-finalized canonical chain. Contains N number (depends on `finalization_depth`) of + /// blocks. These blocks are found in fork_to_child but not inside `blocks_to_chain` or + /// `number_to_block` as those are sidechain specific indices. canonical_chain: CanonicalChain, /// Index needed when discarding the chain, so we can remove connected chains from tree. /// @@ -101,14 +101,6 @@ impl BlockIndices { (canonical_tip.number + 1, pending_blocks) } - /// Returns the block number of the canonical block with the given hash. - /// - /// Returns `None` if no block could be found in the canonical chain. - #[inline] - pub(crate) fn get_canonical_block_number(&self, block_hash: &BlockHash) -> Option { - self.canonical_chain.get_canonical_block_number(self.last_finalized_block, block_hash) - } - /// Last finalized block pub fn last_finalized_block(&self) -> BlockNumber { self.last_finalized_block @@ -138,8 +130,8 @@ impl BlockIndices { self.fork_to_child.entry(first.parent_hash).or_default().insert_if_absent(first.hash()); } - /// Get the chain ID the block belongs to - pub(crate) fn get_blocks_chain_id(&self, block: &BlockHash) -> Option { + /// Get the [BlockchainId] the given block belongs to if it exists. + pub(crate) fn get_block_chain_id(&self, block: &BlockHash) -> Option { self.blocks_to_chain.get(block).cloned() } @@ -370,7 +362,7 @@ impl BlockIndices { /// Returns the block number of the canonical block with the given hash. #[inline] - pub fn canonical_number(&self, block_hash: BlockHash) -> Option { + pub fn canonical_number(&self, block_hash: &BlockHash) -> Option { self.canonical_chain.canonical_number(block_hash) } diff --git a/crates/blockchain-tree/src/blockchain_tree.rs b/crates/blockchain-tree/src/blockchain_tree.rs index 3b51ee6fae31..db9102314975 100644 --- a/crates/blockchain-tree/src/blockchain_tree.rs +++ b/crates/blockchain-tree/src/blockchain_tree.rs @@ -215,7 +215,7 @@ where } // is block inside chain - if let Some(attachment) = self.is_block_inside_chain(&block) { + if let Some(attachment) = self.is_block_inside_sidechain(&block) { return Ok(Some(BlockStatus::Valid(attachment))) } @@ -260,7 +260,7 @@ where } /// Returns true if the block is included in a side-chain. - fn is_block_hash_inside_chain(&self, block_hash: BlockHash) -> bool { + fn is_block_hash_inside_sidechain(&self, block_hash: BlockHash) -> bool { self.block_by_hash(block_hash).is_some() } @@ -287,7 +287,7 @@ where let canonical_chain = self.state.block_indices.canonical_chain(); // if it is part of the chain - if let Some(chain_id) = self.block_indices().get_blocks_chain_id(&block_hash) { + if let Some(chain_id) = self.block_indices().get_block_chain_id(&block_hash) { trace!(target: "blockchain_tree", ?block_hash, "Constructing post state data based on non-canonical chain"); // get block state let Some(chain) = self.state.chains.get(&chain_id) else { @@ -318,7 +318,7 @@ where } // check if there is canonical block - if let Some(canonical_number) = canonical_chain.canonical_number(block_hash) { + if let Some(canonical_number) = canonical_chain.canonical_number(&block_hash) { trace!(target: "blockchain_tree", %block_hash, "Constructing post state data based on canonical chain"); return Some(BundleStateData { canonical_fork: ForkBlock { number: canonical_number, hash: block_hash }, @@ -345,7 +345,7 @@ where let parent = block.parent_num_hash(); // check if block parent can be found in any side chain. - if let Some(chain_id) = self.block_indices().get_blocks_chain_id(&parent.hash) { + if let Some(chain_id) = self.block_indices().get_block_chain_id(&parent.hash) { // found parent in side tree, try to insert there return self.try_insert_block_into_side_chain(block, chain_id, block_validation_kind) } @@ -358,7 +358,7 @@ where // this is another check to ensure that if the block points to a canonical block its block // is valid if let Some(canonical_parent_number) = - self.block_indices().canonical_number(block.parent_hash) + self.block_indices().canonical_number(&block.parent_hash) { // we found the parent block in canonical chain if canonical_parent_number != parent.number { @@ -458,7 +458,7 @@ where /// Try inserting a block into the given side chain. /// - /// WARNING: This expects a valid side chain id, see [BlockIndices::get_blocks_chain_id] + /// WARNING: This expects a valid side chain id, see [BlockIndices::get_block_chain_id] #[instrument(level = "trace", skip_all, target = "blockchain_tree")] fn try_insert_block_into_side_chain( &mut self, @@ -557,8 +557,7 @@ where } let fork_block = chain.fork_block(); - if let Some(next_chain_id) = self.block_indices().get_blocks_chain_id(&fork_block.hash) - { + if let Some(next_chain_id) = self.block_indices().get_block_chain_id(&fork_block.hash) { chain_id = next_chain_id; } else { // if there is no fork block that point to other chains, break the loop. @@ -582,7 +581,7 @@ where // chain fork block fork = self.state.chains.get(&chain_id)?.fork_block(); // get fork block chain - if let Some(fork_chain_id) = self.block_indices().get_blocks_chain_id(&fork.hash) { + if let Some(fork_chain_id) = self.block_indices().get_block_chain_id(&fork.hash) { chain_id = fork_chain_id; continue } @@ -608,7 +607,7 @@ where while let Some(block) = dependent_block.pop_back() { // Get chain of dependent block. - let Some(chain_id) = self.block_indices().get_blocks_chain_id(&block) else { + let Some(chain_id) = self.block_indices().get_block_chain_id(&block) else { debug!(target: "blockchain_tree", ?block, "Block not in tree"); return Default::default(); }; @@ -735,15 +734,15 @@ where Ok(()) } - /// Check if block is found inside chain and its attachment. + /// Check if block is found inside a sidechain and its attachment. /// /// if it is canonical or extends the canonical chain, return [BlockAttachment::Canonical] /// if it does not extend the canonical chain, return [BlockAttachment::HistoricalFork] /// if the block is not in the tree or its chain id is not valid, return None #[track_caller] - fn is_block_inside_chain(&self, block: &BlockNumHash) -> Option { + fn is_block_inside_sidechain(&self, block: &BlockNumHash) -> Option { // check if block known and is already in the tree - if let Some(chain_id) = self.block_indices().get_blocks_chain_id(&block.hash) { + if let Some(chain_id) = self.block_indices().get_block_chain_id(&block.hash) { // find the canonical fork of this chain let Some(canonical_fork) = self.canonical_fork(chain_id) else { debug!(target: "blockchain_tree", chain_id=?chain_id, block=?block.hash, "Chain id not valid"); @@ -981,22 +980,25 @@ where /// /// Returns `Ok(None)` if the block hash is not canonical (block hash does not exist, or is /// included in a sidechain). + /// + /// Note: this does not distinguish between a block that is finalized and a block that is not + /// finalized yet, only whether it is part of the canonical chain or not. pub fn find_canonical_header( &self, hash: &BlockHash, ) -> Result, ProviderError> { // if the indices show that the block hash is not canonical, it's either in a sidechain or - // canonical, but in the db. If it is in a sidechain, it is not canonical. If it is not in - // the db, then it is not canonical. + // canonical, but in the db. If it is in a sidechain, it is not canonical. If it is missing + // in the db, then it is also not canonical. let provider = self.externals.provider_factory.provider()?; let mut header = None; - if let Some(num) = self.block_indices().get_canonical_block_number(hash) { + if let Some(num) = self.block_indices().canonical_number(hash) { header = provider.header_by_number(num)?; } - if header.is_none() && self.is_block_hash_inside_chain(*hash) { + if header.is_none() && self.is_block_hash_inside_sidechain(*hash) { return Ok(None) } @@ -1008,6 +1010,9 @@ where } /// Determines whether or not a block is canonical, checking the db if necessary. + /// + /// Note: this does not distinguish between a block that is finalized and a block that is not + /// finalized yet, only whether it is part of the canonical chain or not. pub fn is_block_hash_canonical(&self, hash: &BlockHash) -> Result { self.find_canonical_header(hash).map(|header| header.is_some()) } @@ -1062,7 +1067,7 @@ where return Ok(CanonicalOutcome::AlreadyCanonical { header }) } - let Some(chain_id) = self.block_indices().get_blocks_chain_id(&block_hash) else { + let Some(chain_id) = self.block_indices().get_block_chain_id(&block_hash) else { debug!(target: "blockchain_tree", ?block_hash, "Block hash not found in block indices"); return Err(CanonicalError::from(BlockchainTreeError::BlockHashNotFoundInChain { block_hash, @@ -1085,7 +1090,7 @@ where let mut chains_to_promote = vec![canonical]; // loop while fork blocks are found in Tree. - while let Some(chain_id) = self.block_indices().get_blocks_chain_id(&fork_block.hash) { + while let Some(chain_id) = self.block_indices().get_block_chain_id(&fork_block.hash) { // canonical chain is lower part of the chain. let Some(canonical) = self.remove_and_split_chain(chain_id, ChainSplitTarget::Number(fork_block.number)) @@ -1200,7 +1205,7 @@ where .unwrap_or_default() .into_iter() .for_each(|child| { - if let Some(chain_id) = self.block_indices().get_blocks_chain_id(&child) { + if let Some(chain_id) = self.block_indices().get_block_chain_id(&child) { if let Some(chain) = self.state.chains.get_mut(&chain_id) { chain.clear_trie_updates(); } @@ -1307,8 +1312,8 @@ where // This should only happen when an optimistic sync target was re-orged. // // Static files generally contain finalized data. The blockchain tree only deals - // with unfinalized data. The only scenario where canonical reverts go past the highest - // static file is when an optimistic sync occured and unfinalized data was written to + // with non-finalized data. The only scenario where canonical reverts go past the highest + // static file is when an optimistic sync occurred and non-finalized data was written to // static files. if self .externals @@ -1780,7 +1785,7 @@ mod tests { ); let block3a_chain_id = - tree.state.block_indices.get_blocks_chain_id(&block3a.hash()).unwrap(); + tree.state.block_indices.get_block_chain_id(&block3a.hash()).unwrap(); assert_eq!( tree.all_chain_hashes(block3a_chain_id), BTreeMap::from([ @@ -1820,7 +1825,7 @@ mod tests { tree.insert_block(block1.clone(), BlockValidationKind::Exhaustive).unwrap(), InsertPayloadOk::Inserted(BlockStatus::Valid(BlockAttachment::Canonical)) ); - let block1_chain_id = tree.state.block_indices.get_blocks_chain_id(&block1.hash()).unwrap(); + let block1_chain_id = tree.state.block_indices.get_block_chain_id(&block1.hash()).unwrap(); let block1_chain = tree.state.chains.get(&block1_chain_id).unwrap(); assert!(block1_chain.trie_updates().is_some()); @@ -1828,7 +1833,7 @@ mod tests { tree.insert_block(block2.clone(), BlockValidationKind::Exhaustive).unwrap(), InsertPayloadOk::Inserted(BlockStatus::Valid(BlockAttachment::Canonical)) ); - let block2_chain_id = tree.state.block_indices.get_blocks_chain_id(&block2.hash()).unwrap(); + let block2_chain_id = tree.state.block_indices.get_block_chain_id(&block2.hash()).unwrap(); let block2_chain = tree.state.chains.get(&block2_chain_id).unwrap(); assert!(block2_chain.trie_updates().is_none()); @@ -1841,7 +1846,7 @@ mod tests { tree.insert_block(block3.clone(), BlockValidationKind::Exhaustive).unwrap(), InsertPayloadOk::Inserted(BlockStatus::Valid(BlockAttachment::Canonical)) ); - let block3_chain_id = tree.state.block_indices.get_blocks_chain_id(&block3.hash()).unwrap(); + let block3_chain_id = tree.state.block_indices.get_block_chain_id(&block3.hash()).unwrap(); let block3_chain = tree.state.chains.get(&block3_chain_id).unwrap(); assert!(block3_chain.trie_updates().is_some()); @@ -1854,7 +1859,7 @@ mod tests { tree.insert_block(block4.clone(), BlockValidationKind::Exhaustive).unwrap(), InsertPayloadOk::Inserted(BlockStatus::Valid(BlockAttachment::Canonical)) ); - let block4_chain_id = tree.state.block_indices.get_blocks_chain_id(&block4.hash()).unwrap(); + let block4_chain_id = tree.state.block_indices.get_block_chain_id(&block4.hash()).unwrap(); let block4_chain = tree.state.chains.get(&block4_chain_id).unwrap(); assert!(block4_chain.trie_updates().is_some()); @@ -1863,7 +1868,7 @@ mod tests { InsertPayloadOk::Inserted(BlockStatus::Valid(BlockAttachment::Canonical)) ); - let block5_chain_id = tree.state.block_indices.get_blocks_chain_id(&block5.hash()).unwrap(); + let block5_chain_id = tree.state.block_indices.get_block_chain_id(&block5.hash()).unwrap(); let block5_chain = tree.state.chains.get(&block5_chain_id).unwrap(); assert!(block5_chain.trie_updates().is_none()); diff --git a/crates/blockchain-tree/src/canonical_chain.rs b/crates/blockchain-tree/src/canonical_chain.rs index e641e455a78b..0aca1bf945d7 100644 --- a/crates/blockchain-tree/src/canonical_chain.rs +++ b/crates/blockchain-tree/src/canonical_chain.rs @@ -1,13 +1,13 @@ use reth_primitives::{BlockHash, BlockNumHash, BlockNumber}; use std::collections::BTreeMap; -/// This keeps track of all blocks of the canonical chain. +/// This keeps track of (non-finalized) blocks of the canonical chain. /// /// This is a wrapper type around an ordered set of block numbers and hashes that belong to the -/// canonical chain. +/// canonical chain that is not yet finalized. #[derive(Debug, Clone, Default)] pub(crate) struct CanonicalChain { - /// All blocks of the canonical chain in order. + /// All blocks of the canonical chain in order of their block number. chain: BTreeMap, } @@ -22,18 +22,18 @@ impl CanonicalChain { self.chain = chain; } - /// Returns the block hash of the canonical block with the given number. + /// Returns the block hash of the (non-finalized) canonical block with the given number. #[inline] pub(crate) fn canonical_hash(&self, number: &BlockNumber) -> Option { self.chain.get(number).cloned() } - /// Returns the block number of the canonical block with the given hash. + /// Returns the block number of the (non-finalized) canonical block with the given hash. #[inline] - pub(crate) fn canonical_number(&self, block_hash: BlockHash) -> Option { + pub(crate) fn canonical_number(&self, block_hash: &BlockHash) -> Option { self.chain.iter().find_map( |(number, hash)| { - if *hash == block_hash { + if hash == block_hash { Some(*number) } else { None @@ -42,20 +42,6 @@ impl CanonicalChain { ) } - /// Returns the block number of the canonical block with the given hash. - /// - /// Returns `None` if no block could be found in the canonical chain. - #[inline] - pub(crate) fn get_canonical_block_number( - &self, - last_finalized_block: BlockNumber, - block_hash: &BlockHash, - ) -> Option { - self.chain - .range(last_finalized_block..) - .find_map(|(num, &h)| (h == *block_hash).then_some(*num)) - } - /// Extends all items from the given iterator to the chain. #[inline] pub(crate) fn extend(&mut self, blocks: impl Iterator) { diff --git a/crates/blockchain-tree/src/state.rs b/crates/blockchain-tree/src/state.rs index 75b6b4a91934..f02890654c3c 100644 --- a/crates/blockchain-tree/src/state.rs +++ b/crates/blockchain-tree/src/state.rs @@ -68,7 +68,7 @@ impl TreeState { &self, block_hash: BlockHash, ) -> Option<&SealedBlockWithSenders> { - let id = self.block_indices.get_blocks_chain_id(&block_hash)?; + let id = self.block_indices.get_block_chain_id(&block_hash)?; let chain = self.chains.get(&id)?; chain.block_with_senders(block_hash) } @@ -77,7 +77,7 @@ impl TreeState { /// /// Caution: This will not return blocks from the canonical chain. pub(crate) fn receipts_by_block_hash(&self, block_hash: BlockHash) -> Option> { - let id = self.block_indices.get_blocks_chain_id(&block_hash)?; + let id = self.block_indices.get_block_chain_id(&block_hash)?; let chain = self.chains.get(&id)?; chain.receipts_by_block_hash(block_hash) }