fix(tree): retain max(additional, max_reorg_depth) block hashes

[shekhirin]

The actual number of canonical hashes we need to retain in memory is the maximum of how many is required for reorg (64) and for EVM execution (256). So it's 256.

[codecov]

Codecov Report

Merging #4612 (f0a9486) into main (5be8ae4) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Files Changed	Coverage Δ
crates/interfaces/src/blockchain_tree/mod.rs	42.10% <ø> (ø)
crates/blockchain-tree/src/blockchain_tree.rs	83.71% <100.00%> (-0.10%)	⬇️
crates/blockchain-tree/src/config.rs	88.88% <100.00%> (+0.79%)	⬆️

... and 13 files with indirect coverage changes

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

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

Components	Coverage Δ
reth binary	32.17% <ø> (ø)
blockchain tree	83.73% <100.00%> (-0.02%)	⬇️
pipeline	88.53% <ø> (ø)
storage (db)	72.52% <ø> (ø)
trie	94.73% <ø> (ø)
txpool	50.28% <ø> (+0.46%)	⬆️
networking	77.14% <ø> (ø)
rpc	57.83% <ø> (-0.01%)	⬇️
consensus	63.09% <ø> (ø)
revm	28.38% <ø> (ø)
payload builder	8.35% <ø> (ø)
primitives	86.53% <ø> (-0.02%)	⬇️

[mattsse]

hmm, I get 64+256 but why the tree needs this I'm not convinced, because they should still be in the database and therefore available during execution?

[onbjerg]

seconding matt, if the block is not covered by max_reorg_depth, then it is present in the db

[rakita]

@mattsse @onbjerg It is inside db, but with this, it is cached in memory. In general, clients cache block hash values. if desired we can go without it.

[shekhirin]

@rakita I can't find the place where we check the cache in implementations of DatabaseRef::block_hash, is it used now?

[rakita]

@rakita I can't find the place where we check the cache in implementations of DatabaseRef::block_hash, is it used now?

We clone the hashmap, as it is small, and use it inside I think BundleStateDataProvider

[shekhirin]

@rakita I can't find the place where we check the cache in implementations of DatabaseRef::block_hash, is it used now?

We clone the hashmap, as it is small, and use it inside I think BundleStateDataProvider

ah, I was still looking at the outdated revm API 😄 I see it now, thanks.

[shekhirin]

@mattsse @onbjerg I added the comments explaining the need for 256, lmk if my understanding is wrong.

[mattsse]

okay, I get it now,

my issue was that it wasn't obvious to me where we're passing the canonical hashes to the state provider.

I think worth adding a note to this type as well

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

Lines 4 to 12 in d846199

	/// This keeps track of all 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.
	#[derive(Debug, Clone, Default)]
	pub(crate) struct CanonicalChain {
	/// All blocks of the canonical chain in order.
	chain: BTreeMap<BlockNumber, BlockHash>,
	}