Cleanup, refactor and add trace logs to DownloadSyncReceiptsStep

[fab-10]

PR description

Summary of the changes in the PR

• Moved the filters about empty blocks and dedup before the request iterations, so they are done once
• Moved the validations of the response in the validate method that seems more appropriate for them
• In case of no peers or timeout add a wait before retrying, to avoid a very tight loop that results in CPU spikes, since I saw that happening in the logs especially for small networks, where few peers are responding and they could be already busy.
• Added a max number of retries in case of failure
• Added many trace logs, with task id and iteration id, so it is possible to correlate the logs
• Removed some dev logs that were executed even if TRACE not enabled, that could have an impact on memory and cpu
• Removed the now unneeded abstraction
• As bonus of the retry mechanism is that Besu now exit normally if interrupted during the sync, while before you had to kill it

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

• Checked out our contribution guidelines?
• Considered documentation and added the doc-change-required label to this PR if updates are required.
• Considered the changelog and included an update if required.
• For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

• spotless: ./gradlew spotlessApply
• unit tests: ./gradlew build
• acceptance tests: ./gradlew acceptanceTest
• integration tests: ./gradlew integrationTest
• reference tests: ./gradlew ethereum:referenceTests:referenceTests
• hive tests: Engine or other RPCs modified?

[Copilot]

Pull request overview

Refactors receipt-download flow for fast sync by deduplicating requests up-front, adding trace-level correlation logs, and adding retry/backoff behavior; updates the peer task and related tests to match the new request/response shape.

Changes:

• Refactored DownloadSyncReceiptsStep to request receipts by SyncBlock, dedup by receipts-root, and emit structured trace logs.
• Updated GetSyncReceiptsFromPeerTask to return ordered receipt lists and moved validation into validateResult.
• Added/updated unit tests and test utilities; adjusted BlockDataGenerator to support configurable transaction counts.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/DownloadSyncReceiptsStep.java	New receipt download loop with dedup, correlation logs, and retry/backoff.
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/task/GetSyncReceiptsFromPeerTask.java	Changes task input/output shape and strengthens validation logic.
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/messages/GetReceiptsMessage.java	Changes hashes() return type to List<Hash>.
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java	Improves invalid-response logging and exception messaging.
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/AbstractDownloadReceiptsStep.java	Removes the old generic receipt-download abstraction.
ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/FastSyncChainDownloaderTest.java	Adjusts mocks to match new receipts task output type.
ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/checkpointsync/CheckPointSyncChainDownloaderTest.java	Adjusts mocks to match new receipts task output type.
ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/DownloadSyncReceiptsStepTest.java	Adds unit tests for receipts download/combination behavior.
ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/task/GetSyncReceiptsFromPeerTaskTest.java	Adds unit tests for request construction, parsing, and validation.
ethereum/eth/src/test-support/java/org/hyperledger/besu/ethereum/eth/core/Utils.java	Adds test-support helpers for receipts and block conversions/comparisons.
ethereum/core/src/test-support/java/org/hyperledger/besu/ethereum/core/BlockDataGenerator.java	Adds configurable transaction count and honors blockOptionsSupplier in block().

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[macfarla]

LGTM. couple of comments from claude that I think are worth reviewing.
2. resolvedBlocks.clear() on subList — At line ~740 in the new DownloadSyncReceiptsStep, resolvedBlocks is a subList view, and resolvedBlocks.clear() mutates the backing blocksToRequest list (removing the first N elements). This is correct but subtle — a comment would help future readers understand this isn't a no-op.
5. Ordering assumption in response matching — The new code assumes receipts come back in the same order as requested blocks (blocksToRequest.get(i) matched with blocksReceipts.get(i)). The old code matched by receipt root hash, which
was order-independent. If a peer returns receipts in a different order than requested, this would silently associate wrong receipts with wrong blocks. However, per the Eth protocol spec, Receipts responses should be in the same
order as the GetReceipts request, so this should be safe. The validateResult() check via receiptsRootMatches would catch any mismatch.
6. prepareRequest returns ArrayList (mutable) — The collect to ArrayList is intentional since downloadReceipts mutates the list via resolvedBlocks.clear(). This coupling between the two methods is a bit fragile — if someone
refactored prepareRequest to return an unmodifiable list, downloadReceipts would break at runtime.
7. receiptsRootMatches uses .getBytes().equals() — Hash extends Bytes32 which extends Bytes, so calculatedReceiptsRoot.getBytes().equals(blockHeader.getReceiptsRoot().getBytes()) could be simplified to just .equals() on the Hash
objects directly. The current approach works but is unnecessarily verbose.

[fab-10]

LGTM. couple of comments from claude that I think are worth reviewing.
2. resolvedBlocks.clear() on subList — At line ~740 in the new DownloadSyncReceiptsStep, resolvedBlocks is a subList view, and resolvedBlocks.clear() mutates the backing blocksToRequest list (removing the first N elements). This is correct but subtle — a comment would help future readers understand this isn't a no-op.

added a comment to explain what is happening

5. Ordering assumption in response matching — The new code assumes receipts come back in the same order as requested blocks (blocksToRequest.get(i) matched with blocksReceipts.get(i)). The old code matched by receipt root hash, which was order-independent. If a peer returns receipts in a different order than requested, this would silently associate wrong receipts with wrong blocks. However, per the Eth protocol spec, Receipts responses should be in the same order as the GetReceipts request, so this should be safe. The validateResult() check via receiptsRootMatches would catch any mismatch.

The protocol enforces the order, so the response matching has been simplified.

6. prepareRequest returns ArrayList (mutable) — The collect to ArrayList is intentional since downloadReceipts mutates the list via resolvedBlocks.clear(). This coupling between the two methods is a bit fragile — if someone refactored prepareRequest to return an unmodifiable list, downloadReceipts would break at runtime.

This is to avoid an extra allocation and work, and since the code is in private methods a single class, I think we could take this shortcut, and there will always be unit tests to surface any issue in case of refactoring

7. receiptsRootMatches uses .getBytes().equals() — Hash extends Bytes32 which extends Bytes, so calculatedReceiptsRoot.getBytes().equals(blockHeader.getReceiptsRoot().getBytes()) could be simplified to just .equals() on the Hash objects directly. The current approach works but is unnecessarily verbose.

I know but Hash::equals is now deprecated.