Fix get logs patch 2

[matkt]

Signed-off-by: Karim TAAM karim.t2am@gmail.com

PR description

LogBloom could be invalid in the cache after a reorg. With this PR, there is an update of the cache every time a reorg takes place

Test performed

• Sync on goerli (reorg etc.)

Changelog

• I thought about the changelog and included a changelog update if required.

[ajsutton]

nit: Feels a bit weird to use .map and then .isPresent here. Would be worth considering using .ifPresent:

Suggested change

	final Optional<Long> ancestorBlockNumber =
	commonAncestorBlockHeader.map(ProcessableBlockHeader::getNumber);
	if (ancestorBlockNumber.isPresent()) {
	// walk through the blocks from the common ancestor to the received block in order to
	// reload the cache in case of reorg
	for (long number = ancestorBlockNumber.get() + 1;
	number < blockHeader.getNumber();
	number++) {
	Optional<BlockHeader> ancestorBlockHeader = blockchain.getBlockHeader(number);
	if (ancestorBlockHeader.isPresent()) {
	cacheSingleBlock(ancestorBlockHeader.get(), cacheFile);
	}
	}
	}
	commonAncestorBlockHeader
	.map(ProcessableBlockHeader::getNumber)
	.ifPresent(ancestorBlockNumber -> {
	// walk through the blocks from the common ancestor to the received block in order to
	// reload the cache in case of reorg
	for (long number = ancestorBlockNumber.get() + 1;
	number < blockHeader.getNumber();
	number++) {
	blockchain.getBlockHeader(number).ifPresent(
	header -> cacheSingleBlock(ancestorBlockHeader.get(), cacheFile));
	}
	});

[ajsutton]

Also, this isn't removing logs from the cache correctly when a reorg makes the chain shorter. It needs to delete all cache entries between the new chain head number and the old one if the new head number is less.

[ajsutton]

Actually, if this index is only used as the initial filter, you wouldn't need to remove the later blocks when a reorg makes the chain shorter - you'd potentially check the blocks in this range when you didn't need to but it would just be treated as a false positive and sorted out once you're pulling logs out of the actual blocks.

In that case I think this should work. Will deploy it and test it out.

[matkt]

in fact in case of a shorter reorg the cache will be truncated the first time cacheSingleBlock is used. this is why I did not add anything else here.

besu/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/query/cache/TransactionLogBloomCacher.java

Line 184 in be89cd9

final long validCacheSize = offset + BLOOM_BITS_LENGTH;

[matkt]

I always use ifpresent but in this case cacheSingleBlock can throw an exception and there is a catch here. if I use ifPresent I should put 2 catch with the same implementation. this is the main reason for using isPresent here

https://github.com/matkt/besu/blob/931f3c2a3c56fa489b62a0a872aa54edc412bce4/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/query/cache/TransactionLogBloomCacher.java#L174

[ajsutton]

LGTM. I'd strongly recommend updating the cache version again though - those who skip release to release still only reindex once, but anyone who updated to the snapshot reindexes again to fix the log cache. So helps some people with no down side.

[matkt]

LGTM. I'd strongly recommend updating the cache version again though - those who skip release to release still only reindex once, but anyone who updated to the snapshot reindexes again to fix the log cache. So helps some people with no down side.

ok you convinced me I will update the version

[AbdelStark]

Looks good to me.