refactor(engine, tree): connect buffered blocks on pruner finish

[shekhirin]

Two main changes:

1. Separate removing chains from blockchain tree and connecting buffered blocks to blockchain tree in restore_canonical_hashes, because we only need the buffered blocks part when pruner finishes.
2. Rename restore_canonical_hashes to connect_buffered_blocks_to_canonical_hashes and restore_canonical_hashes_and_finalize to connect_buffered_blocks_to_canonical_hashes_and_finalize accordingly.

[codecov]

Codecov Report

Merging #4613 (fcb3608) into main (d846199) will decrease coverage by 0.01%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

Files Changed	Coverage Δ
crates/consensus/beacon/src/engine/hooks/mod.rs	65.21% <ø> (ø)
crates/interfaces/src/blockchain_tree/mod.rs	42.10% <ø> (ø)
crates/blockchain-tree/src/blockchain_tree.rs	83.80% <100.00%> (+0.35%)	⬆️
crates/blockchain-tree/src/shareable.rs	56.52% <100.00%> (+0.31%)	⬆️
crates/consensus/beacon/src/engine/hooks/prune.rs	86.25% <100.00%> (ø)
crates/consensus/beacon/src/engine/mod.rs	74.42% <100.00%> (ø)
crates/storage/provider/src/providers/mod.rs	19.52% <100.00%> (ø)

... and 9 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	16.91% <0.00%> (-0.02%)	⬇️
unit-tests	63.24% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
reth binary	32.17% <ø> (ø)
blockchain tree	83.75% <100.00%> (+0.16%)	⬆️
pipeline	88.52% <ø> (-0.02%)	⬇️
storage (db)	73.47% <100.00%> (ø)
trie	94.73% <ø> (ø)
txpool	49.91% <ø> (-0.02%)	⬇️
networking	77.20% <ø> (-0.02%)	⬇️
rpc	57.55% <ø> (-0.04%)	⬇️
consensus	62.58% <100.00%> (ø)
revm	19.66% <ø> (ø)
payload builder	9.06% <ø> (ø)
primitives	86.44% <ø> (ø)

[mattsse]

what does this fix exactly?

[mattsse]

I don't understand why we need this?

why do we need to store additional canonical hashes intended for execution in the tree?

[shekhirin]

from my other PR, if I'm not mistaken:

reth/crates/blockchain-tree/src/config.rs

Lines 78 to 82 in 9fde694

	/// It is calculated as the maximum of `max_reorg_depth` (which is the number of blocks required
	/// for the deepest reorg possible according to the consensus protocol) and
	/// `num_of_additional_canonical_block_hashes` (which is the number of block hashes needed to
	/// satisfy the `BLOCKHASH` opcode in the EVM. See [`crate::BundleStateDataRef::block_hash`] and
	/// [`crate::AppendableChain::new_canonical_head_fork`] where it's used).

[shekhirin]

@mattsse when we finish the pruner, we need to connect the blocks buffered during the pruning. We've already been doing it, but also doing this

reth/crates/blockchain-tree/src/blockchain_tree.rs

Lines 775 to 783 in 458e3b9

	let (mut remove_chains, _) =
	self.block_indices.update_block_hashes(last_canonical_hashes.clone());
	// remove all chains that got discarded
	while let Some(chain_id) = remove_chains.first() {
	if let Some(chain) = self.chains.remove(chain_id) {
	remove_chains.extend(self.block_indices.remove_chain(&chain));
	}
	}

which is unnecessary, because canonical hashes in database didn't change while the pruner was running.

In case of scenario when the pipeline run is finished, we need to check the canonical hashes in database because they might've been changed:

reth/crates/consensus/beacon/src/engine/mod.rs

Lines 1326 to 1343 in 11f5f3f

	/// Attempt to restore the tree with the given block hash.
	///
	/// This is invoked after a full pipeline to update the tree with the most recent canonical
	/// hashes.
	///
	/// If the given block is missing from the database, this will return `false`. Otherwise, `true`
	/// is returned: the database contains the hash and the tree was updated.
	fn update_tree_on_finished_pipeline(&mut self, block_hash: H256) -> Result<bool, Error> {
	let synced_to_finalized = match self.blockchain.block_number(block_hash)? {
	Some(number) => {
	// Attempt to restore the tree.
	self.blockchain.restore_canonical_hashes_and_finalize(number)?;
	true
	}
	None => false,
	};
	Ok(synced_to_finalized)
	}

---

It wasn't a bug, we just did an unnecessary work and used one function for both cases (pipeline & pruner) which might be confusing.

[mattsse]

lgmt, I like this naming better

[rakita]

lgtm