7311 add get headers from peer task #7781
Conversation
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…e available Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tantiation Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…en initializing PeerTaskFeatureToggle multiple times 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>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tem' into 7311-add-GetReceiptsFromPeerTask
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tem' into 7311-add-GetReceiptsFromPeerTask
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tem' into 7311-add-GetReceiptsFromPeerTask
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tem' into 7311-add-GetReceiptsFromPeerTask
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tem' into 7311-add-GetReceiptsFromPeerTask
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…peertask-system' into 7311-add-cli-feature-toggle-for-peertask-system
…tem' into 7311-add-GetReceiptsFromPeerTask
…' into 7311-add-GetReceiptsFromPeerTask
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…tem' into 7311-add-GetReceiptsFromPeerTask
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>
…PoS, remove new usages of currentProtocolSpecSupplier Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…add-GetHeadersFromPeerTask # Conflicts: # ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/checkpointsync/CheckpointDownloadBlockStep.java # ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/task/GetReceiptsFromPeerTaskTest.java # ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/FastSyncActionsTest.java
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>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda-Clerke <matilda.clerke@consensys.net>
…aderTest Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
Signed-off-by: Matilda-Clerke <matilda.clerke@consensys.net>
…ass for reuse in tests 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>
Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>
…and improve logging 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>
| @@ -84,6 +85,7 @@ protected MiningCoordinator createMiningCoordinator( | |||
| final ProtocolContext protocolContext, | |||
| final TransactionPool transactionPool, | |||
| final MiningConfiguration miningConfiguration, | |||
| final PeerTaskExecutor peerTaskExecutor, | |||
There was a problem hiding this comment.
Looks like we need are adding unused dependencies on peerTaskExecutor via the createMiningCoordinator method inherited from the BesuControllerBuilderInterface. Is there a way around that? Perhaps overloading the method and providing a default implementation to be used by subclasses that don't care about it?
There was a problem hiding this comment.
Yep, good idea
There was a problem hiding this comment.
Unfortunately, I can't really find a way around it. Overloading won't help because it's called from within BesuControllerBuilder, which can't make the distinction between which subclasses need the PeerTaskExecutor and which don't
...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/GetBlockFromPeerTask.java
Show resolved
Hide resolved
Signed-off-by: Matilda-Clerke <matilda.clerke@consensys.net>
| .scheduleServiceTask( | ||
| () -> { | ||
| GetHeadersFromPeerTask task = | ||
| new GetHeadersFromPeerTask( |
There was a problem hiding this comment.
This should be using a retry of peerCount like the task is using
| @@ -98,7 +101,7 @@ public <T> PeerTaskExecutorResult<T> execute(final PeerTask<T> peerTask) { | |||
| if (peer.isEmpty()) { | |||
| executorResult = | |||
| new PeerTaskExecutorResult<>( | |||
| Optional.empty(), PeerTaskExecutorResponseCode.NO_PEER_AVAILABLE); | |||
| Optional.empty(), PeerTaskExecutorResponseCode.NO_PEER_AVAILABLE, null); | |||
There was a problem hiding this comment.
Is this null going to cause an issue somewhere? Perhaps the peer should be Optional?
| protocolSchedule, | ||
| protocolContext, | ||
| ethContext, | ||
| peerTaskExecutor, |
There was a problem hiding this comment.
Adding the PeerTaskExecutor to the EthContext could be a nice home for this and save a lot of changes to wire through the PeerTaskExecutor through all the tasks.
| () -> { | ||
| GetHeadersFromPeerTask task = | ||
| new GetHeadersFromPeerTask( | ||
| Hash.wrap(peer.chainState().getBestBlock().getHash()), |
There was a problem hiding this comment.
Be good to update this to use the block hash constructor as it's not clear whether the block number or block hash is being used
| if (taskResult.responseCode() != PeerTaskExecutorResponseCode.SUCCESS | ||
| || taskResult.result().isEmpty()) { | ||
| LOG.info("Unable to download headers for range {}", range); | ||
| return CompletableFuture.failedFuture( | ||
| new RuntimeException("Unable to download headers for range " + range)); | ||
| } | ||
| LOG.info("Successfully downloaded headers"); | ||
| return CompletableFuture.completedFuture(taskResult.result().get()); |
There was a problem hiding this comment.
This seems like a common usage pattern that could go in the PeerTaskExecutor to simplify usage
| batchSize, | ||
| 0, | ||
| Direction.REVERSE, | ||
| Integer.MAX_VALUE, |
There was a problem hiding this comment.
The number of retries seems a bit high, using peerCount same as what is being done in the old code seems more sensible
| do { | ||
| if (isCancelled.get()) { | ||
| return CompletableFuture.failedFuture( | ||
| new InterruptedException("Pivot block confirmation has been cancelled")); |
There was a problem hiding this comment.
A CancellationException would make more sense.
| }); | ||
| } | ||
|
|
||
| private boolean waitForPeer() throws InterruptedException { |
There was a problem hiding this comment.
What thread does this block? I think the confirmPivotBlock is called before the sync pipeline, so this might not be on an executor thread.
| .scheduleServiceTask( | ||
| () -> { | ||
| GetHeadersFromPeerTask task = | ||
| new GetHeadersFromPeerTask( |
There was a problem hiding this comment.
This should use the same retries as the original task from MAX_QUERY_RETRIES_PER_PEER
| if (messageData == null) { | ||
| throw new InvalidPeerTaskResponseException("Response MessageData is null"); | ||
| } | ||
| return BlockHeadersMessage.readFrom(messageData).getHeaders(protocolSchedule); |
There was a problem hiding this comment.
This should validate that the headers form a chain like the AbstractGetHeadersFromPeerTask task does
| public Predicate<EthPeer> getPeerRequirementFilter() { | ||
| return (ethPeer) -> | ||
| ethPeer.getProtocolName().equals(getSubProtocol().getName()) | ||
| && (protocolSchedule.anyMatch((ps) -> ps.spec().isPoS()) |
There was a problem hiding this comment.
We should probably figure that out up front so we do not have to do that for each of the peers that we use, or use Suppliers.memoize?
| @@ -132,4 +132,8 @@ public Predicate<EthPeer> getPeerRequirementFilter() { | |||
| public boolean isSuccess(final Map<BlockHeader, List<TransactionReceipt>> result) { | |||
| return !result.isEmpty(); | |||
| } | |||
|
|
|||
| public Collection<BlockHeader> getBlockHeaders() { | |||
There was a problem hiding this comment.
This is only used in a test. Can this be solved differently in the test, so we can remove that here?
| private Boolean validateBlockHeaders(final EthPeer ethPeer, final List<BlockHeader> headers) { | ||
| boolean isValid; | ||
| if (headers.isEmpty()) { | ||
| if (blockIsRequired()) { |
There was a problem hiding this comment.
blockIsRequired always returns true and it is only used here. We should remove this!
| PeerTaskExecutorResult<List<BlockHeader>> taskResult = | ||
| peerTaskExecutor.executeAgainstPeer(task, peer); | ||
| if (taskResult.responseCode() == PeerTaskExecutorResponseCode.SUCCESS | ||
| && taskResult.result().isPresent()) { |
There was a problem hiding this comment.
Is it possible to only check one of these? Is it possible to have SUCCESS, but and empty result?
| GetHeadersFromPeerTask.Direction.FORWARD, | ||
| protocolSchedule); | ||
| PeerTaskExecutorResult<List<BlockHeader>> taskResult = | ||
| peerTaskExecutor.executeAgainstPeer(task, range.getSyncTarget()); |
There was a problem hiding this comment.
We should not execute against the sync target! I can see that the original implementation does that, but it is wrong!!!
| .getScheduler() | ||
| .scheduleServiceTask( | ||
| () -> { | ||
| LOG.info("Getting headers using peer task system"); |
| return CompletableFuture.failedFuture( | ||
| new RuntimeException("Unable to download headers for range " + range)); | ||
| } | ||
| LOG.info("Successfully downloaded headers"); |
| PeerTaskExecutorResult<List<BlockHeader>> taskResult = | ||
| peerTaskExecutor.executeAgainstPeer(task, range.getSyncTarget()); | ||
| if (taskResult.responseCode() != PeerTaskExecutorResponseCode.SUCCESS | ||
| || taskResult.result().isEmpty()) { |
There was a problem hiding this comment.
can we have SUCCESS with an empty result?
| taskResult.responseCode()); | ||
| return CompletableFuture.failedFuture(new NoAvailablePeersException()); | ||
| } else if (taskResult.responseCode() != PeerTaskExecutorResponseCode.SUCCESS | ||
| || taskResult.result().isEmpty()) { |
There was a problem hiding this comment.
can we have SUCCESS with an empty result?
| peerTaskExecutor.execute(task); | ||
| if (taskResult.responseCode() == PeerTaskExecutorResponseCode.NO_PEER_AVAILABLE | ||
| || taskResult.responseCode() | ||
| == PeerTaskExecutorResponseCode.PEER_DISCONNECTED) { |
There was a problem hiding this comment.
We should try another peer if the one we tried got disconnected ...
PR description
Add the GetHeadersFromPeerTask PeerTask and implement usages