From ae4d48730557444c21bb788a75654a100dcda97e Mon Sep 17 00:00:00 2001 From: rakita Date: Thu, 4 Jan 2024 00:05:47 +0100 Subject: [PATCH] fix(BlockchainTree): remove forked chain receipts/reverts (#5921) --- crates/blockchain-tree/src/blockchain_tree.rs | 144 ++++++++++++++---- crates/blockchain-tree/src/chain.rs | 10 +- .../bundle_state_with_receipts.rs | 12 +- 3 files changed, 135 insertions(+), 31 deletions(-) diff --git a/crates/blockchain-tree/src/blockchain_tree.rs b/crates/blockchain-tree/src/blockchain_tree.rs index 1a60342cd..c02e15d82 100644 --- a/crates/blockchain-tree/src/blockchain_tree.rs +++ b/crates/blockchain-tree/src/blockchain_tree.rs @@ -147,35 +147,33 @@ impl BlockchainTree { if block.number <= last_finalized_block { // check if block is canonical if self.is_block_hash_canonical(&block.hash)? { - return Ok(Some(BlockStatus::Valid)); + return Ok(Some(BlockStatus::Valid)) } // check if block is inside database if self.externals.provider_factory.provider()?.block_number(block.hash)?.is_some() { - return Ok(Some(BlockStatus::Valid)); + return Ok(Some(BlockStatus::Valid)) } return Err(BlockchainTreeError::PendingBlockIsFinalized { last_finalized: last_finalized_block, } - .into()); + .into()) } // check if block is part of canonical chain if self.is_block_hash_canonical(&block.hash)? { - return Ok(Some(BlockStatus::Valid)); + return Ok(Some(BlockStatus::Valid)) } // is block inside chain if let Some(status) = self.is_block_inside_chain(&block) { - return Ok(Some(status)); + return Ok(Some(status)) } // check if block is disconnected if let Some(block) = self.state.buffered_blocks.block(&block.hash) { - return Ok(Some(BlockStatus::Disconnected { - missing_ancestor: block.parent_num_hash(), - })); + return Ok(Some(BlockStatus::Disconnected { missing_ancestor: block.parent_num_hash() })) } Ok(None) @@ -266,7 +264,7 @@ impl BlockchainTree { // get canonical fork. let canonical_fork = self.canonical_fork(chain_id)?; - return Some(BundleStateData { state, parent_block_hashed, canonical_fork }); + return Some(BundleStateData { state, parent_block_hashed, canonical_fork }) } // check if there is canonical block @@ -276,7 +274,7 @@ impl BlockchainTree { canonical_fork: ForkBlock { number: canonical_number, hash: block_hash }, state: BundleStateWithReceipts::default(), parent_block_hashed: self.canonical_chain().inner().clone(), - }); + }) } None @@ -299,7 +297,7 @@ impl BlockchainTree { // 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) { // found parent in side tree, try to insert there - return self.try_insert_block_into_side_chain(block, chain_id, block_validation_kind); + return self.try_insert_block_into_side_chain(block, chain_id, block_validation_kind) } // if not found, check if the parent can be found inside canonical chain. @@ -307,7 +305,7 @@ impl BlockchainTree { .is_block_hash_canonical(&parent.hash) .map_err(|err| InsertBlockError::new(block.block.clone(), err.into()))? { - return self.try_append_canonical_chain(block, block_validation_kind); + return self.try_append_canonical_chain(block, block_validation_kind) } // this is another check to ensure that if the block points to a canonical block its block @@ -323,7 +321,7 @@ impl BlockchainTree { block_number: block.number, }, block.block, - )); + )) } } @@ -400,7 +398,7 @@ impl BlockchainTree { return Err(InsertBlockError::execution_error( BlockValidationError::BlockPreMerge { hash: block.hash }.into(), block.block, - )); + )) } let parent_header = provider @@ -563,7 +561,7 @@ impl BlockchainTree { } else { // if there is no fork block that point to other chains, break the loop. // it means that this fork joins to canonical block. - break; + break } } hashes @@ -584,9 +582,9 @@ impl BlockchainTree { // get fork block chain if let Some(fork_chain_id) = self.block_indices().get_blocks_chain_id(&fork.hash) { chain_id = fork_chain_id; - continue; + continue } - break; + break } (self.block_indices().canonical_hash(&fork.number) == Some(fork.hash)).then_some(fork) } @@ -693,7 +691,7 @@ impl BlockchainTree { pub fn buffer_block(&mut self, block: SealedBlockWithSenders) -> Result<(), InsertBlockError> { // validate block consensus rules if let Err(err) = self.validate_block(&block) { - return Err(InsertBlockError::consensus_error(err, block.block)); + return Err(InsertBlockError::consensus_error(err, block.block)) } self.state.buffered_blocks.insert_block(block); @@ -710,17 +708,17 @@ impl BlockchainTree { ?block, "Failed to validate total difficulty for block {}: {e:?}", block.header.hash ); - return Err(e); + return Err(e) } if let Err(e) = self.externals.consensus.validate_header(block) { error!(?block, "Failed to validate header {}: {e:?}", block.header.hash); - return Err(e); + return Err(e) } if let Err(e) = self.externals.consensus.validate_block(block) { error!(?block, "Failed to validate block {}: {e:?}", block.header.hash); - return Err(e); + return Err(e) } Ok(()) @@ -741,7 +739,7 @@ impl BlockchainTree { Some(BlockStatus::Valid) } else { Some(BlockStatus::Accepted) - }; + } } None } @@ -782,7 +780,7 @@ impl BlockchainTree { // validate block consensus rules if let Err(err) = self.validate_block(&block) { - return Err(InsertBlockError::consensus_error(err, block.block)); + return Err(InsertBlockError::consensus_error(err, block.block)) } Ok(InsertPayloadOk::Inserted( @@ -951,7 +949,7 @@ impl BlockchainTree { } if header.is_none() && self.is_block_hash_inside_chain(*hash) { - return Ok(None); + return Ok(None) } if header.is_none() { @@ -1006,9 +1004,9 @@ impl BlockchainTree { return Err(CanonicalError::from(BlockValidationError::BlockPreMerge { hash: *block_hash, }) - .into()); + .into()) } - return Ok(CanonicalOutcome::AlreadyCanonical { header }); + return Ok(CanonicalOutcome::AlreadyCanonical { header }) } let Some(chain_id) = self.block_indices().get_blocks_chain_id(block_hash) else { @@ -1016,7 +1014,7 @@ impl BlockchainTree { return Err(CanonicalError::from(BlockchainTreeError::BlockHashNotFoundInChain { block_hash: *block_hash, }) - .into()); + .into()) }; let chain = self.state.chains.remove(&chain_id).expect("To be present"); @@ -1180,7 +1178,7 @@ impl BlockchainTree { block_number: tip.number, block_hash: tip.hash, }, - )))); + )))) } recorder.record_relative(MakeCanonicalAction::RetrieveStateTrieUpdates); @@ -1206,7 +1204,7 @@ impl BlockchainTree { pub fn unwind(&mut self, unwind_to: BlockNumber) -> RethResult<()> { // nothing to be done if unwind_to is higher then the tip if self.block_indices().canonical_tip().number <= unwind_to { - return Ok(()); + return Ok(()) } // revert `N` blocks from current canonical chain and put them inside BlockchanTree let old_canon_chain = self.revert_canonical_from_database(unwind_to)?; @@ -1621,6 +1619,94 @@ mod tests { ); } + #[tokio::test] + async fn test_side_chain_fork() { + let data = BlockChainTestData::default_with_numbers(11, 12); + let (block1, exec1) = data.blocks[0].clone(); + let (block2, exec2) = data.blocks[1].clone(); + let genesis = data.genesis; + + // test pops execution results from vector, so order is from last to first. + let externals = setup_externals(vec![exec2.clone(), exec1.clone(), exec2, exec1]); + + // last finalized block would be number 9. + setup_genesis(&externals.provider_factory, genesis); + + // make tree + let config = BlockchainTreeConfig::new(1, 2, 3, 2); + let mut tree = BlockchainTree::new(externals, config, None).expect("failed to create tree"); + // genesis block 10 is already canonical + tree.make_canonical(&B256::ZERO).unwrap(); + + // make genesis block 10 as finalized + tree.finalize_block(10); + + assert_eq!( + tree.insert_block(block1.clone(), BlockValidationKind::Exhaustive).unwrap(), + InsertPayloadOk::Inserted(BlockStatus::Valid) + ); + + assert_eq!( + tree.insert_block(block2.clone(), BlockValidationKind::Exhaustive).unwrap(), + InsertPayloadOk::Inserted(BlockStatus::Valid) + ); + + // we have one chain that has two blocks. + // Trie state: + // b2 (pending block) + // | + // | + // b1 (pending block) + // / + // / + // g1 (canonical blocks) + // | + TreeTester::default() + .with_chain_num(1) + .with_block_to_chain(HashMap::from([(block1.hash, 0.into()), (block2.hash, 0.into())])) + .with_fork_to_child(HashMap::from([(block1.parent_hash, HashSet::from([block1.hash]))])) + .assert(&tree); + + let mut block2a = block2.clone(); + let block2a_hash = B256::new([0x34; 32]); + block2a.hash = block2a_hash; + + assert_eq!( + tree.insert_block(block2a.clone(), BlockValidationKind::Exhaustive).unwrap(), + InsertPayloadOk::Inserted(BlockStatus::Accepted) + ); + + // fork chain. + // Trie state: + // b2 b2a (pending blocks in tree) + // | / + // | / + // b1 + // / + // / + // g1 (canonical blocks) + // | + + TreeTester::default() + .with_chain_num(2) + .with_block_to_chain(HashMap::from([ + (block1.hash, 0.into()), + (block2.hash, 0.into()), + (block2a.hash, 1.into()), + ])) + .with_fork_to_child(HashMap::from([ + (block1.parent_hash, HashSet::from([block1.hash])), + (block2a.parent_hash, HashSet::from([block2a.hash])), + ])) + .assert(&tree); + // chain 0 has two blocks so receipts and reverts len is 2 + assert_eq!(tree.state.chains.get(&0.into()).unwrap().state().receipts().len(), 2); + assert_eq!(tree.state.chains.get(&0.into()).unwrap().state().state().reverts.len(), 2); + // chain 1 has one block so receipts and reverts len is 1 + assert_eq!(tree.state.chains.get(&1.into()).unwrap().state().receipts().len(), 1); + assert_eq!(tree.state.chains.get(&1.into()).unwrap().state().state().reverts.len(), 1); + } + #[tokio::test] async fn sanity_path() { let data = BlockChainTestData::default_with_numbers(11, 12); diff --git a/crates/blockchain-tree/src/chain.rs b/crates/blockchain-tree/src/chain.rs index 6d9128213..1af8f73cf 100644 --- a/crates/blockchain-tree/src/chain.rs +++ b/crates/blockchain-tree/src/chain.rs @@ -172,8 +172,16 @@ impl AppendableChain { externals, ) .map_err(|err| InsertBlockError::new(block.block.clone(), err.into()))?; + // extending will also optimize few things, mostly related to selfdestruct and wiping of + // storage. state.extend(block_state); + // remove all receipts and reverts (except the last one), as they belong to the chain we + // forked from and not the new chain we are creating. + let size = state.receipts().len(); + state.receipts_mut().drain(0..size - 1); + state.state_mut().take_n_reverts(size - 1); + // If all is okay, return new chain back. Present chain is not modified. Ok(Self { chain: Chain::from_block(block, state) }) } @@ -224,7 +232,7 @@ impl AppendableChain { return Err(ConsensusError::BodyStateRootDiff( GotExpected { got: state_root, expected: block.state_root }.into(), ) - .into()); + .into()) } } diff --git a/crates/storage/provider/src/bundle_state/bundle_state_with_receipts.rs b/crates/storage/provider/src/bundle_state/bundle_state_with_receipts.rs index 0408afd09..958ed5cbd 100644 --- a/crates/storage/provider/src/bundle_state/bundle_state_with_receipts.rs +++ b/crates/storage/provider/src/bundle_state/bundle_state_with_receipts.rs @@ -95,6 +95,11 @@ impl BundleStateWithReceipts { &self.bundle } + /// Returns mutable revm bundle state. + pub fn state_mut(&mut self) -> &mut BundleState { + &mut self.bundle + } + /// Set first block. pub fn set_first_block(&mut self, first_block: BlockNumber) { self.first_block = first_block; @@ -268,11 +273,16 @@ impl BundleStateWithReceipts { self.receipts.root_slow(self.block_number_to_index(block_number)?, chain_spec, timestamp) } - /// Return reference to receipts. + /// Returns reference to receipts. pub fn receipts(&self) -> &Receipts { &self.receipts } + /// Returns mutable reference to receipts. + pub fn receipts_mut(&mut self) -> &mut Receipts { + &mut self.receipts + } + /// Return all block receipts pub fn receipts_by_block(&self, block_number: BlockNumber) -> &[Option] { let Some(index) = self.block_number_to_index(block_number) else { return &[] };