Add SyncTransactionReceipt, decoder, and update LogsBloomFilter to utilise raw Bytes #9574
Conversation
…ilise raw Bytes Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
.../java/org/hyperledger/besu/ethereum/core/encoding/receipt/SyncTransactionReceiptDecoder.java
Outdated
Show resolved
Hide resolved
...a/org/hyperledger/besu/ethereum/core/encoding/receipt/SyncTransactionReceiptDecoderTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
| transactionTypeCode, | ||
| statusOrStateRoot, | ||
| cumulativeGas, | ||
| bloomFilter == null ? LogsBloomFilter.builder().insertRawLogs(logs).build() : bloomFilter, |
There was a problem hiding this comment.
I think when we are syncing we should not have to deal with the case that we have a legacy receipt without bloom filter. I think that it only us removing the bloom filter when we are storing receipts in a compacted form in the db.
There was a problem hiding this comment.
Hmm, I'll have to double check the PoC branch, but removing the bloom filter stuff should simplify this a bit.
There was a problem hiding this comment.
Unfortunately, we do need the bloom filters to calculate the receipts root
|
|
||
| @Test | ||
| public void testDecodeLegacyReceipt() { | ||
| final Hash stateRoot = Hash.fromHexStringLenient("01"); |
There was a problem hiding this comment.
01 should be the status then, not the state root.
A legacy receipt can contain the state root, so maybe we should add a test with a state root as well?
There was a problem hiding this comment.
In this case it's just using 01 as some test data. As it's used in the Hash class, it's always going to be 32 bytes long and the content doesn't actually matter, as long as it remains the same in the result
There was a problem hiding this comment.
I've changed these to a random 32 byte value instead, to make it clearer that this isn't a status
There's also no reason to write a separate test for receipt with a status instead of stateRoot as the code doesn't do anything different either way.
There was a problem hiding this comment.
Hmmm, state root should only be possible in legacy receipts. I will have another look at the code :-)
...a/org/hyperledger/besu/ethereum/core/encoding/receipt/SyncTransactionReceiptDecoderTest.java
Outdated
Show resolved
Hide resolved
| : transactionByteRlp; | ||
| Bytes cumulativeGasUsed = input.readBytes(); | ||
| List<List<Bytes>> logs = parseLogs(input); | ||
| Bytes bloomFilter = LogsBloomFilter.builder().insertRawLogs(logs).build(); |
There was a problem hiding this comment.
do we actually need the bloomFilter later when we received an eth69 receipt?
There was a problem hiding this comment.
Hmm, I wrote this class to mirror the functionality of the TransactionReceiptDecoder, which does give eth69 receipts a bloom filter in the same manner. This may be removed if we don't need bloom filters for syncing.
...a/org/hyperledger/besu/ethereum/core/encoding/receipt/SyncTransactionReceiptDecoderTest.java
Outdated
Show resolved
Hide resolved
| public Builder insertRawLogs(final Collection<List<Bytes>> logs) { | ||
| logs.forEach( | ||
| (bytesList) -> | ||
| insertRawLog(bytesList.getFirst(), bytesList.subList(1, bytesList.size() - 1))); |
There was a problem hiding this comment.
I think we could just call insertBytes for each log
There was a problem hiding this comment.
I thought so too initially, but it seems the data portion of the log is actually not included in the bloom filter. This is why you can see I'm using a subList on the second parameter.
The insertRawLog method also makes it clear and explicit exactly which Bytes is the address and which Bytes are log topics, which imo is preferable to just inserting them into the filter.
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
|
My comments have been addressed. Will leave to sync experts to approve |
|
Add sync On Jan 7, 2026 11:04 PM, Sally MacFarlane ***@***.***> wrote:macfarla left a comment (hyperledger/besu#9574)
My comments have been addressed. Will leave to sync experts to approve
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
4 participants
PR description
This PR adds SyncTransactionReceipt (similar to SyncBlockBody) to allow us to perform sync operations without fully parsing and duplicating the backing data. (PoC branch at #9416)