feat: Block Verification Feature #414
Conversation
AlfredoG87
force-pushed
the
verification-handler
branch
2 times, most recently
from
50fbeb9 to
fa09df7
Compare
AlfredoG87
added
Block Node
AlfredoG87
force-pushed
the
verification-handler
branch
from
71fa05c to
f6a2236
Compare
Next steps: Add metrics to measure time to hash. Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
… added. and temporary stuff. Signed-off-by: Alfredo Gutierrez <[email protected]>
…ink I will create several implementations to the interfaces with some simple and synchronous and another one concurrent and asynchronous. Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
…ckerframework.checker.qual;) on module-info by accident Signed-off-by: Alfredo Gutierrez <[email protected]>
…me refactor around BlockVerificationService Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Small improvements in other tests as well. Improvements to the flow of finalizeVerification thanks to issues surfaced during unit testing Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
…tual/final configuration values Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
AlfredoG87
force-pushed
the
verification-handler
branch
from
932b207 to
bd9fb75
Compare
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
… on the chart deployment Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
| /** | ||
| * The Block failed verification, either due to an invalid signature or an invalid hash. | ||
| */ | ||
| SIGNATURE_INVALID |
There was a problem hiding this comment.
This implies that it's strictly signature issue, maybe add invalid hash enum or change name to something more ambiguous, as we are not sure at this point what is the root cause ?
| try { | ||
|
|
||
| if (!serviceStatus.isRunning()) { | ||
| LOGGER.log(ERROR, "Service is not running. Block item will not be processed further."); |
There was a problem hiding this comment.
Maybe wrap with else statement, otherwise won't it continue running down ?
| * A {@link StreamingTreeHasher} that computes the root hash of a perfect binary Merkle tree of {@link Bytes} leaves | ||
| * using a concurrent algorithm that hashes leaves in parallel and combines the resulting hashes in parallel. | ||
| * <p> | ||
| * <b>Important:</b> This class is not thread-safe, and client code must not make concurrent calls to |
There was a problem hiding this comment.
How can we be sure that this will be the case ? Do we wanna consider using some synchronization primitives ?
There was a problem hiding this comment.
@AlfredoG87 really good job!
This is my first pass through, I was focused mostly at looking at things on the surface level. Will need additional passes to get into the logic in depth, but the change is very big.
That being said, I comment you some nits/cleanup I found on the first pass.
I have not looked at the metrics and charts json changes as well as tests.
Here general comments:
-
I would not use
varever anywhere, even in test/loops etc. I think it makes things very unreadable and we should not be using that IMO. Not a blocker, but an opinion and a suggestion. -
I am sure I have missed places, but let's be adding the non-null annotation for the static code analysis.
-
Also I am sure I have missed some places, but lets always have a require non null checks on things that must not be null and we only do assignment (not calling a method on the target object). Lets fail early on these places. (Mostly in constructors or in methods where we need a non null object but we use it after executing a lot of logic before hand)
My focus on the next runs will be to get into and test the logic. Then tests (I presume there will be changes made, so I am hesitant to go to tests yet)
| | Environment Variable | Description | Default Value | | ||
| |:---------------------------------------|:-------------------------------------------------------------------------|--------------------:| | ||
| | PERSISTENCE_STORAGE_LIVE_ROOT_PATH | The root path for the live storage. | | | ||
| | PERSISTENCE_STORAGE_ARCHIVE_ROOT_PATH | The root path for the archive storage. | | |
There was a problem hiding this comment.
Let's stick with the old formatting of the md tables for now. I will make a discussion so that the team will be able to vote on what the formatting should be for the tables so we can all be on the same page.
| this.serviceStatus = serviceStatus; | ||
| this.notifier = notifier; | ||
| this.blockNodeContext = blockNodeContext; | ||
|
|
||
| streamMediator.subscribe(streamPersistenceHandler); | ||
| streamMediator.subscribe(streamVerificationHandler); | ||
| this.streamMediator = streamMediator; |
There was a problem hiding this comment.
we should add null checks here on all lines 81-87 as they are assignment and will be used later, lets fail early here.
| /** | ||
| * Constructs a new instance of {@link CompressionType}. | ||
| * | ||
| * @param minCompressionLevel the minimum compression level | ||
| * @param maxCompressionLevel the maximum compression level | ||
| */ | ||
| CompressionType(final int minCompressionLevel, final int maxCompressionLevel) { | ||
| this.minCompressionLevel = minCompressionLevel; | ||
| this.maxCompressionLevel = maxCompressionLevel; | ||
| } | ||
|
|
||
| /** | ||
| * This method verifies that the compression level is within the | ||
| * acceptable range for the given compression type. | ||
| * | ||
| * @param levelToCheck the compression level to check | ||
| * @throws IllegalArgumentException if the compression level is not within | ||
| * the acceptable range | ||
| */ |
There was a problem hiding this comment.
Thanks for the docs! I've missed that :)
| * | ||
| * @param <V> the type of the value to be written |
There was a problem hiding this comment.
Thanks for the docs again :)
| /** | ||
| * An enum representing the status of block verification. | ||
| */ | ||
| public enum BlockVerificationStatus { |
There was a problem hiding this comment.
Should we also add an UNVERIFIED option which would mean that this block is not yet verified? Or does the core logic and design do not support that?
| this.blockNumber = blockHeader.number(); | ||
| this.metricsService = metricsService; | ||
| this.signatureVerifier = signatureVerifier; | ||
| this.inputTreeHasher = inputTreeHasher; | ||
| this.outputTreeHasher = outputTreeHasher; | ||
|
|
||
| this.blockWorkStartTime = System.nanoTime(); |
There was a problem hiding this comment.
add require non null on 98-101 as they are assignment only, lets fail early here
| this.config = verificationConfig; | ||
| this.metricsService = metricsService; | ||
| this.signatureVerifier = signatureVerifier; | ||
| this.executorService = executorService; |
There was a problem hiding this comment.
lets add require non null check on 52-55 as they are assignment only, lets fail early here
| case ASYNC -> new BlockVerificationSessionAsync( | ||
| blockHeader, metricsService, signatureVerifier, executorService, hashCombineBatchSize); | ||
| case SYNC -> new BlockVerificationSessionSync(blockHeader, metricsService, signatureVerifier); |
There was a problem hiding this comment.
same comment as above for considering to migrate to static factory methods due to easier maintainability in the future
| * @param signature the signature to verify | ||
| * @return true if the signature is valid, false otherwise | ||
| */ | ||
| Boolean verifySignature(Bytes hash, Bytes signature); |
There was a problem hiding this comment.
could add the non-null annotation to the parameter for the static code analysis
| blockStream.millisecondsPerBlock=500 | ||
| blockStream.blockItemsBatchSize=1_000 |
There was a problem hiding this comment.
maybe drop these defaults, I think you changed these during development of the new feature, but by default they should be commented out?
Description:
New Classes:
verificationpackageverification.hasherpackageverification.servicepackage:verification.sessionpacakge:verification.signaturepackage:Other notable changes:
StreamVerificationHandlerImplas a new subscriber to theunverified ring buffer.Related issue(s):
Fixes #375
Fixes #374
Fixes #376
Fixes #173
Fixes #423
Fixes #424
Notes for reviewer:
For the scenarios below, I use a 501 blocks sample with up-to-date (Dec-18-2024) very high TPS (10K).
The default implementation will be
ASYNCthat not only is faster, but is alsonon-blockingStats with
SYNCimplementation:Stats with
ASYNCimplementation:Checklist