Fix log index in transaction receipt

[Wetitpig]

PR description

Assign log index to transaction receipt according to position in block instead of in transaction.

Fixed Issue(s)

fixes #6204

[github-actions]

• I thought about documentation and added the doc-change-required label to this PR if updates are required.
• I thought about the changelog and included a changelog update if required.
• If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

[shemnon]

As a "fix it now" patch this seems like a reasonable solution. However there will be performance implications with large blocks. I think a longer term solution would be to add the log index into the TransactionLocation and save that when calculating the location. The data can be written so the value may be missing, and when it is we use this logic. Creating a seamless transition. We also could add a DB migration to re-build the transaction locations in the background.

[Wetitpig]

How to initiate re-building transaction locations?
Do I just add a similar section to migrateVariables?

besu/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/storage/keyvalue/KeyValueStoragePrefixedKeyBlockchainStorage.java

Lines 152 to 167 in be5cc68

	get(VARIABLES_PREFIX, CHAIN_HEAD_HASH.getBytes())
	.map(this::bytesToHash)
	.ifPresent(
	bch ->
	variablesStorage
	.getChainHead()
	.ifPresentOrElse(
	vch -> {
	if (!vch.equals(bch)) {
	logInconsistencyAndFail(CHAIN_HEAD_HASH, bch, vch);
	}
	},
	() -> {
	variablesUpdater.setChainHead(bch);
	LOG.info("Migrated key {} to variables storage", CHAIN_HEAD_HASH);
	}));

[shemnon]

@fab-10

[fab-10]

@Wetitpig migrateVariables was much easier to implement, since it only had to move few keys around, and did it synchronously, while for this re-build an asynchronous tasks is needed, with all the consequences of making it not hurting the overall Besu performance.

My proposal for this is to:

1. first support saving the location for newly imported blocks
2. then for already imported blocks, that miss this data, calculate it and save on demand

Have not yet looking into the code, to understand how much it, but would like to hear what you think.

[Wetitpig]

Given that most transaction receipts may not be queried for multiple times I am not sure if it is worth to save the logIndexOffset for each transaction.
Probably I am going to try removing parallel() and see how much the performance impact really is.

[fab-10]

if calculating that data at runtime is good enough for the moment, we can postpone the work to persist that data until more performance is needed.

[fab-10]

LGTM, just a check on the use of parallel stream or not for the sum

[fab-10]

since usually we only have few hundreds txs in a block, have you any number to confirm that doing the sum in parallel is faster than doing it serially?

[Wetitpig]

Not really any numbers there. I am just thinking that it does not hurt to map and sum the numbers in parallel.

[fab-10]

just thinking that a quick JMH test could be enough to spot the difference in performance, between serial/parallel

[Wetitpig]

I have no idea how to do a JMH benchmark, but I tried the query with time curl and deducting the user and sys time

query {
	blocks (from: 18676591 to: 18676640) {
		transactions { gasUsed }
	}
}

Every time GraphQL transverses into a transaction it would first obtain the receipt for the amount of gas used, thus running the code for obtaining log indices in the receipt.

So with 8235 transactions in these 50 blocks the results are as follows:

.parallel()	Time averaged over 10 queries
with	8.5605 s
without	8.6308 s

So it may not be worth the hassle to parallelise

[fab-10]

Ok, indeed for small lists there should not be a big difference.
Just for reference, an example of a JMH test in Besu is RLPBench

[fab-10]

looks better now!

[fab-10]

nice opt