[MINOR] Eliminate redundant header validation #943
Conversation
smatthewenglish
force-pushed
the
eliminate-redundant-header-validation
branch
from
75bf8a5
to
84553d9
Compare
smatthewenglish
removed
the
work in progress
smatthewenglish
force-pushed
the
eliminate-redundant-header-validation
branch
from
6f4d2ea
to
e269709
Compare
smatthewenglish
force-pushed
the
eliminate-redundant-header-validation
branch
from
e269709
to
d6ed372
Compare
|
|
||
| // Import block | ||
| final PersistBlockTask<C> importTask = | ||
| PersistBlockTask.create( | ||
| protocolSchedule, protocolContext, block, HeaderValidationMode.FULL, metricsSystem); | ||
| protocolSchedule, protocolContext, block, HeaderValidationMode.NONE, metricsSystem); |
There was a problem hiding this comment.
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. :)
| importingBlocks.remove(block.getHash()); | ||
| if (t != null) { | ||
| .scheduleSyncWorkerTask( | ||
| () -> { |
There was a problem hiding this comment.
There's probably a few methods that should be extracted here to make it easier to read. I'd probably have the whole of this worker task as a private method (maybe validateAndProcessPendingBlock?)
| ethContext | ||
| .getScheduler() | ||
| .scheduleSyncWorkerTask(() -> broadcastBlock(block, parent)); | ||
| return ethContext |
There was a problem hiding this comment.
I'd be tempted to extract this blob that does the actual import into a separate method as well.
| .scheduleSyncWorkerTask(() -> broadcastBlock(block, parent)); | ||
| return ethContext | ||
| .getScheduler() | ||
| .scheduleSyncWorkerTask(importTask::run) |
There was a problem hiding this comment.
We're already in a worker thread so can remove scheduleSyncWorkerTask and just call run directly. It still returns a CompletableFuture<Block> so we'd still chain the whenComplete in the same way.
| if (blockHeaderValidator.validateHeader( | ||
| block.getHeader(), parent, protocolContext, HeaderValidationMode.FULL)) { | ||
| ethContext.getScheduler().scheduleSyncWorkerTask(() -> broadcastBlock(block, parent)); | ||
| return ethContext.getScheduler().scheduleSyncWorkerTask(() -> runImportTask(block)); |
There was a problem hiding this comment.
You don't need to defer this to a separate worker task since you're already in a worker thread.
| return ethContext.getScheduler().scheduleSyncWorkerTask(() -> runImportTask(block)); | |
| return runImportTask(block); |
| .whenComplete( | ||
| (r, t) -> { | ||
| (result, throwable) -> { |
There was a problem hiding this comment.
+1 to the better naming here. :)
…nt-header-validation
PR description
wrt block propagation -- currently we're validating headers twice: once to decide if we should propagate and once when we actually import.
changes here enforce that we validate just once, then propagate and import in parallel.
Fixed Issue(s)