rpc: retry latest block via cache in block_with_senders

[tcoratger]

Should close #11859

[emhane]

isn't it a better option to change the block_hash field of CacheAction::GetBlockWithSenders to a BlockId? the EthCacheService also stores a provider, so can fetch the latest block hash when it processes the cache action.

this would makes more sense, since the EthCacheService has a queue of cache action events that it processes. the delay between sending the request and processing is only trivially solved by a retry to fetch the hash in this rpc method impl, if solved at all.

reth/crates/rpc/rpc-eth-types/src/cache/mod.rs

Lines 512 to 515 in cb60482

	GetBlockWithSenders {
	block_hash: B256,
	response_tx: BlockWithSendersResponseSender,
	},

[tcoratger]

@emhane I get your point, it seems like a good idea to me. I think that in the issue opened by @mattsse he mentioned the fact that retrying was the easiest solution to perform and that would solve most reorg issues but not all:

imo simply retrying once is the best solution here and should guard against most reorg related issues.

But I am totally open if it needs to be modified. Maybe I am wrong but I have the following remarks:

• In the CacheAction everything is fetches by hashes and this will be the unique field fetched by ID, is this not a problem in order to have some kind of similar logic for everything and not something different everywhere?
• If we do that, it requires also modifying the BlockLruCache to have a block ID instead of the hash here

reth/crates/rpc/rpc-eth-types/src/cache/mod.rs

Lines 49 to 54 in cfd066c

	type BlockLruCache<L> = MultiConsumerLruCache<
	B256,
	Arc<SealedBlockWithSenders>,
	L,
	Either<BlockWithSendersResponseSender, BlockTransactionsResponseSender>,
	>;

so maybe it requires a bit more changes.

Maybe doing the block hash to block id transformation directly in the lookup here as pointed out in the issue description

reth/crates/rpc/rpc-eth-types/src/cache/mod.rs

Line 356 in cfd066c

CacheAction::GetBlockWithSenders { block_hash, response_tx } => {

could be an intermediate solution? But if the block hash to block ID switch is the best solution given everything, more than happy to modify it.

[mattsse]

yeah I think we could use a new command in the cache that looks up by blockid, this way we delay the tag -> hash conversion as late as possible and can prevent a miss due to a reorg

[mattsse]

modifying the BlockLruCache to have a block ID

we can't do that, only hashes map blocks correctly

[tcoratger]

@mattsse Let me know if this is what you had in mind, I may have done it a little differently than you thought:

• I realized that simply changing the block hash to block id in the CacheAction could be negative for other use cases where we already had the block hash before. This would have amounted to:
  • Transforming the block ID into a block hash in a function
  • Then transforming it back into a block ID to enter it in the GetBlockWithSenders
  • Finally transforming again to a block hash at the beginning of the lookup
  • Which generally produced a useless round trip.
• I favored another technique where I simply create a new specific instance for this use case without touching the rest.

[mattsse]

actually using the cache with BlockId isn't trivial, I'd like to limit this pr to just the retry which should be fine

[tcoratger]

actually using the cache with BlockId isn't trivial, I'd like to limit this pr to just the retry which should be fine

@mattsse Fine just reverted the changes to have only the retry here