Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

[MINOR] Eliminate redundant header validation #943

Original file line number Diff line number Diff line change
Expand Up @@ -109,32 +109,6 @@ private void setupListeners() {
.subscribe(EthPV62.NEW_BLOCK_HASHES, this::handleNewBlockHashesFromNetwork);
}

private void validateAndBroadcastBlock(final Block block) {
final ProtocolSpec<C> protocolSpec =
protocolSchedule.getByBlockNumber(block.getHeader().getNumber());
final BlockHeaderValidator<C> blockHeaderValidator = protocolSpec.getBlockHeaderValidator();
final BlockHeader parent =
protocolContext
.getBlockchain()
.getBlockHeader(block.getHeader().getParentHash())
.orElseThrow(
() ->
new IllegalArgumentException(
"Incapable of retrieving header from non-existent parent of "
+ block.getHeader().getNumber()
+ "."));
if (blockHeaderValidator.validateHeader(
block.getHeader(), parent, protocolContext, HeaderValidationMode.FULL)) {
final UInt256 totalDifficulty =
protocolContext
.getBlockchain()
.getTotalDifficultyByHash(parent.getHash())
.get()
.plus(block.getHeader().getDifficulty());
blockBroadcaster.propagate(block, totalDifficulty);
}
}

private void onBlockAdded(final BlockAddedEvent blockAddedEvent, final Blockchain blockchain) {
// Check to see if any of our pending blocks are now ready for import
final Block newBlock = blockAddedEvent.getBlock();
Expand Down Expand Up @@ -263,6 +237,16 @@ private CompletableFuture<Block> processAnnouncedBlock(
return getBlockTask.run().thenCompose((r) -> importOrSavePendingBlock(r.getResult()));
}

private void broadcastBlock(final Block block, final BlockHeader parent) {
final UInt256 totalDifficulty =
protocolContext
.getBlockchain()
.getTotalDifficultyByHash(parent.getHash())
.get()
.plus(block.getHeader().getDifficulty());
blockBroadcaster.propagate(block, totalDifficulty);
}

@VisibleForTesting
CompletableFuture<Block> importOrSavePendingBlock(final Block block) {
// Synchronize to avoid race condition where block import event fires after the
Expand Down Expand Up @@ -292,19 +276,54 @@ CompletableFuture<Block> importOrSavePendingBlock(final Block block) {
return CompletableFuture.completedFuture(block);
}

ethContext.getScheduler().scheduleSyncWorkerTask(() -> validateAndBroadcastBlock(block));
final BlockHeader parent =
protocolContext
.getBlockchain()
.getBlockHeader(block.getHeader().getParentHash())
.orElseThrow(
() ->
new IllegalArgumentException(
"Incapable of retrieving header from non-existent parent of "
+ block.getHeader().getNumber()
+ "."));

// Import block
final PersistBlockTask<C> importTask =
PersistBlockTask.create(
protocolSchedule, protocolContext, block, HeaderValidationMode.FULL, metricsSystem);
final ProtocolSpec<C> protocolSpec =
protocolSchedule.getByBlockNumber(block.getHeader().getNumber());
final BlockHeaderValidator<C> blockHeaderValidator = protocolSpec.getBlockHeaderValidator();
return ethContext
.getScheduler()
.scheduleSyncWorkerTask(importTask::run)
.scheduleSyncWorkerTask(
() -> validateAndProcessPendingBlock(blockHeaderValidator, block, parent));
}

private CompletableFuture<Block> validateAndProcessPendingBlock(
final BlockHeaderValidator<C> blockHeaderValidator,
final Block block,
final BlockHeader parent) {
if (blockHeaderValidator.validateHeader(
block.getHeader(), parent, protocolContext, HeaderValidationMode.FULL)) {
ethContext.getScheduler().scheduleSyncWorkerTask(() -> broadcastBlock(block, parent));
return ethContext.getScheduler().scheduleSyncWorkerTask(() -> runImportTask(block));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to defer this to a separate worker task since you're already in a worker thread.

Suggested change
return ethContext.getScheduler().scheduleSyncWorkerTask(() -> runImportTask(block));
return runImportTask(block);

} else {
importingBlocks.remove(block.getHash());
LOG.warn(
"Failed to import announced block {} ({}).",
block.getHeader().getNumber(),
block.getHash());
return CompletableFuture.completedFuture(block);
}
}

private CompletableFuture<Block> runImportTask(final Block block) {
final PersistBlockTask<C> importTask =
PersistBlockTask.create(
protocolSchedule, protocolContext, block, HeaderValidationMode.NONE, metricsSystem);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we create this inside the if(validate) block so it's only created if it will actually be used? Mostly just so that the first thing you come across isn't HeaderValidationMode.NODE which is a bit scary. :)

return importTask
.run()
.whenComplete(
(r, t) -> {
(result, throwable) -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to the better naming here. :)

importingBlocks.remove(block.getHash());
if (t != null) {
if (throwable != null) {
LOG.warn(
"Failed to import announced block {} ({}).",
block.getHeader().getNumber(),
Expand Down