-
Notifications
You must be signed in to change notification settings - Fork 16
feat(verification): Changes to Support 0.69.0 CN Protobuf and Produced blocks
#1993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
jsync-swirlds
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still working through this, but there are a few comments below to start with.
...ode/verification/src/main/java/org/hiero/block/node/verification/AllBlocksHasherHandler.java
Show resolved
Hide resolved
...ode/verification/src/main/java/org/hiero/block/node/verification/AllBlocksHasherHandler.java
Outdated
Show resolved
Hide resolved
...ode/verification/src/main/java/org/hiero/block/node/verification/AllBlocksHasherHandler.java
Outdated
Show resolved
Hide resolved
...ode/verification/src/main/java/org/hiero/block/node/verification/AllBlocksHasherHandler.java
Outdated
Show resolved
Hide resolved
jsync-swirlds
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small items and an API design concern.
block-node/app/src/testFixtures/java/org/hiero/block/node/app/fixtures/blocks/BlockUtils.java
Outdated
Show resolved
Hide resolved
.../verification/src/main/java/org/hiero/block/node/verification/VerificationServicePlugin.java
Outdated
Show resolved
Hide resolved
block-node/verification/src/main/proto/root_hash_of_all_previous_blocks_hasher.proto
Show resolved
Hide resolved
...ation/src/main/java/org/hiero/block/node/verification/session/HapiVersionSessionFactory.java
Show resolved
Hide resolved
.../src/main/java/org/hiero/block/node/verification/session/impl/ExtendedMerkleTreeSession.java
Show resolved
Hide resolved
common/src/main/java/org/hiero/block/common/hasher/HashingUtilities.java
Outdated
Show resolved
Hide resolved
...sts/simulator/src/main/java/org/hiero/block/simulator/generator/CraftBlockStreamManager.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
… since is on the construction, it should fail under the right exception at the class that is instantiating it, and either force recovery or fail, leaving this decision to the instantiating class. Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
…by checking hasSignedBlockProof and verifying there is a single one. Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
…ashes when starting from genesis. - Changed the Simulator to always produce deterministic blocks, so the rootHashOfAllBlockHashesTree always matches between runs, this is needed for E2E Testing scenarios. - Improved Flaky test by giving a tiny bit more time. - VerificationSession interface now has 2 methods, a void processBlockItems and a finalizeVerification that returns the VerificationNotification result, making use of the end_of_block message that is now received and not relaying on block_proofs alone. Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
…kStreamingHasher into class: AllBlocksHasherHandler that handles initialization and lifecycle managing. Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
… edge case. Made needed changes in classes to fix bugs found during testing phase. Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
…d it to used the intermeadiate hashes instead of all the previous hasher to buildd the StreamingHasher, also fixed the StreamingHasherm to accept leafCount to be able to resume correct state from saved snapshot. Made the saving of the hasher snapshot independent and saving automatically every X amount of time. Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
5d493e2 to
e12d575
Compare
...sts/simulator/src/main/java/org/hiero/block/simulator/generator/CraftBlockStreamManager.java
Outdated
Show resolved
Hide resolved
...sts/simulator/src/main/java/org/hiero/block/simulator/generator/CraftBlockStreamManager.java
Outdated
Show resolved
Hide resolved
...sts/simulator/src/main/java/org/hiero/block/simulator/generator/CraftBlockStreamManager.java
Outdated
Show resolved
Hide resolved
...sts/simulator/src/main/java/org/hiero/block/simulator/generator/CraftBlockStreamManager.java
Outdated
Show resolved
Hide resolved
...sts/simulator/src/main/java/org/hiero/block/simulator/generator/CraftBlockStreamManager.java
Outdated
Show resolved
Hide resolved
…Hash and the rootOfAllPreviousBlockHash when is created as opposed to when the block is finalized, also, removed the finalizeBlock method, and rely on the blockItems, so far the BlockItemsMessgae comes with endOfBlock, so it's been used internally and returned to the previous design of the VerificationSession with fewer changes Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
…UnparsedBlockItem, to avoid accidental use Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
- Moved .proto to PBJ Default location /src/main/proto, outside of the java classes Signed-off-by: Alfredo Gutierrez Grajeda <[email protected]>
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1993 +/- ##
============================================
+ Coverage 78.99% 79.76% +0.76%
- Complexity 1241 1296 +55
============================================
Files 130 131 +1
Lines 5952 6168 +216
Branches 646 664 +18
============================================
+ Hits 4702 4920 +218
+ Misses 955 945 -10
- Partials 295 303 +8 ... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
| @Loggable @ConfigProperty(defaultValue = "/opt/hiero/block-node/verification/rootHashOfAllPreviousBlocks.bin") | ||
| Path allBlocksHasherFilePath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just collapse this to one line; it's a little long, but not so long that it's a problem (in my opinion).
Not strictly necessary, however.
| // footer | ||
| Bytes rootOfAllPreviousBlockHashes = rootHashOfAllBlockHashesTree != null | ||
| ? rootHashOfAllBlockHashesTree | ||
| : this.blockFooter.rootHashOfAllBlockHashesTree(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: this. is not necessary here.
More significantly, we should detect if blockFooter is null immediately prior to this line and fail the verification rather than throwing a NPE or continuing the process.
| public static <T> T getSingle(List<T> list, Predicate<T> predicate) { | ||
| List<T> filtered = list.stream().filter(predicate).toList(); | ||
|
|
||
| if (filtered.size() != 1) { | ||
| throw new IllegalStateException(String.format( | ||
| "Expected exactly 1 element matching predicate [%s], but found %d.", predicate, filtered.size())); | ||
| } | ||
|
|
||
| return filtered.get(0); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of expensive (particularly for large blocks).
Might be better (particularly as it's only used for block proof) to just take the last item, and if it's not the expected type return null.
For the short term (until we support multiple block proofs, which has to come fairly quickly, possibly as soon as next sprint) that should be sufficient.
| LOGGER.log(TRACE, "Persisting all blocks hasher with {0} leaves snapshot to {1}", new Object[] { | ||
| hasher.leafCount(), hasherFilePath | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the new Object[] here?
| LOGGER.log(TRACE, "Persisting all blocks hasher with {0} leaves snapshot to {1}", new Object[] { | |
| hasher.leafCount(), hasherFilePath | |
| }); | |
| LOGGER.log(TRACE, "Persisting all blocks hasher with {0} leaves snapshot to {1}", hasher.leafCount(), hasherFilePath); |
| "Cannot rebuild hasher from store: requires a single contiguous range starting from genesis. " | ||
| + "Found {0} range(s) starting at block {1}. Will fall back to block footer values.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This performs a string catenation every time it's called.
we should replace this with a constant to avoid that.
| "Cannot rebuild hasher from store: requires a single contiguous range starting from genesis. " | |
| + "Found {0} range(s) starting at block {1}. Will fall back to block footer values.", | |
| private final static String HASHER_REQUIRES_CONTINUOUS_RANGE_MESSAGE = | |
| "Cannot rebuild hasher from store: requires a single contiguous range starting from genesis. " | |
| + "Found {0} range(s) starting at block {1}. Will fall back to block footer values."; |
jsync-swirlds
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple items to clean up.
Could be done in a follow-up if desired.
Reviewer Notes
0.69.0version of protobufRelated Issue(s)
#1906