Skip to content
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

feat: add unverified blocks root #715

Merged

Conversation

ata-nas
Copy link
Contributor

@ata-nas ata-nas commented Feb 25, 2025

Reviewer Notes

  • add a new unverified blocks root
  • persistence will first persist blocks under the unverfied root
  • after successful persistence under unverified && successful verification, persistence should move the block to live
  • due to unavaliable critical infrastructure, a method call has to be made to persistence to move the block

Related Issue(s)

Closes #582

@ata-nas ata-nas linked an issue Feb 25, 2025 that may be closed by this pull request
@ata-nas ata-nas self-assigned this Feb 25, 2025
@ata-nas ata-nas added the Block Node Issues/PR related to the Block Node. label Feb 25, 2025
@ata-nas ata-nas force-pushed the 582-feat-add-unverified-marker-for-persisted-block-files branch 4 times, most recently from d6231a3 to b271aa3 Compare February 27, 2025 12:47
@ata-nas ata-nas added this to the 0.6.0 milestone Feb 27, 2025
@ata-nas ata-nas marked this pull request as ready for review February 27, 2025 19:05
@ata-nas ata-nas requested review from a team as code owners February 27, 2025 19:05
@ata-nas
Copy link
Contributor Author

ata-nas commented Feb 27, 2025

General note:
Due to lack of some critical infrastructure we currently lack, the only point where we can currently be sure that a block is persisted (in unverified root) && verified so then it can be moved, is in the AckHandler. Ideally in the future when we are able to use the messaging service, we will no longer need to pass a ref of the persistence handler to the ack handler in order to call the move logic. This is the easiest temporary solution which unfortunately brings coupling and some debt, but there is little we could do for now. Open to all kinds of suggestions.

@ata-nas ata-nas force-pushed the 582-feat-add-unverified-marker-for-persisted-block-files branch from 69521c7 to cae09c2 Compare February 28, 2025 13:50
@ata-nas ata-nas force-pushed the 582-feat-add-unverified-marker-for-persisted-block-files branch 3 times, most recently from 1927c9c to f100092 Compare March 4, 2025 11:41
AlfredoG87
AlfredoG87 previously approved these changes Mar 4, 2025
Copy link
Contributor

@AlfredoG87 AlfredoG87 left a comment

Choose a reason for hiding this comment

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

LGTM 💯

jsync-swirlds
jsync-swirlds previously approved these changes Mar 5, 2025
jsync-swirlds
jsync-swirlds previously approved these changes Mar 5, 2025
@ata-nas ata-nas force-pushed the 582-feat-add-unverified-marker-for-persisted-block-files branch from 322c86a to eb6ae49 Compare March 6, 2025 08:33
@ata-nas ata-nas force-pushed the 582-feat-add-unverified-marker-for-persisted-block-files branch from eb6ae49 to 4846f68 Compare March 6, 2025 08:49
- add a new unverified blocks root
- persistence will first persist blocks under the unverfied root
- after successful persistence under unverified && successful verification, persistence should move the block to live
- due to unavaliable critical infrastructure, a method call has to be made to persistence to move the block

Signed-off-by: Atanas Atanasov <[email protected]>
@ata-nas ata-nas force-pushed the 582-feat-add-unverified-marker-for-persisted-block-files branch from 4846f68 to 16544bf Compare March 6, 2025 08:50
georgi-l95
georgi-l95 previously approved these changes Mar 6, 2025
…ll overwrite existing unverified block file

Signed-off-by: Atanas Atanasov <[email protected]>
@ata-nas ata-nas merged commit 37d3f73 into main Mar 6, 2025
16 checks passed
@ata-nas ata-nas deleted the 582-feat-add-unverified-marker-for-persisted-block-files branch March 6, 2025 10:22
Copy link

codecov bot commented Mar 6, 2025

Codecov Report

Attention: Patch coverage is 84.09091% with 14 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...va/com/hedera/block/server/ack/AckHandlerImpl.java 35.71% 9 Missing ⚠️
...ence/storage/archive/LocalGroupZipArchiveTask.java 0.00% 3 Missing ⚠️
...nce/storage/path/BlockAsLocalFilePathResolver.java 94.73% 0 Missing and 1 partial ⚠️
...ersistence/storage/path/NoOpBlockPathResolver.java 50.00% 1 Missing ⚠️
@@             Coverage Diff              @@
##               main     #715      +/-   ##
============================================
- Coverage     84.29%   84.21%   -0.08%     
- Complexity      632      634       +2     
============================================
  Files           129      130       +1     
  Lines          2903     2946      +43     
  Branches        206      206              
============================================
+ Hits           2447     2481      +34     
- Misses          387      397      +10     
+ Partials         69       68       -1     
Files with missing lines Coverage Δ Complexity Δ
...er/config/ServerMappedConfigSourceInitializer.java 100.00% <ø> (ø) 2.00 <0.00> (ø)
...server/persistence/PersistenceInjectionModule.java 62.06% <ø> (ø) 9.00 <0.00> (ø)
...rver/persistence/StreamPersistenceHandlerImpl.java 71.26% <100.00%> (+8.02%) 10.00 <2.00> (+2.00)
.../persistence/storage/PersistenceStorageConfig.java 100.00% <100.00%> (ø) 1.00 <0.00> (ø)
...ence/storage/archive/BlockAsLocalFileArchiver.java 64.00% <ø> (ø) 8.00 <0.00> (ø)
...ver/persistence/storage/path/ArchiveBlockPath.java 100.00% <100.00%> (ø) 1.00 <1.00> (ø)
...server/persistence/storage/path/LiveBlockPath.java 100.00% <100.00%> (ø) 1.00 <1.00> (ø)
.../persistence/storage/path/UnverifiedBlockPath.java 100.00% <100.00%> (ø) 1.00 <1.00> (?)
...rsistence/storage/read/BlockAsLocalFileReader.java 68.00% <ø> (ø) 5.00 <0.00> (ø)
...stence/storage/remove/BlockAsLocalFileRemover.java 100.00% <100.00%> (ø) 3.00 <0.00> (-1.00)
... and 7 more
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ata-nas
Copy link
Contributor Author

ata-nas commented Mar 6, 2025

@jsync-swirlds @georgi-l95 @AlfredoG87 as a last minute cleanup: 0c9dd45

What I did in this change was to simply remove some redundant (and in a sense wrong, but not wrong in a "this should not happen way", but rather in a "we do not even need to do this, it is redundant" way) logic which would delete if exists the unverified block file. The reason for this is that unverified block files should be rewritable, the writer is agnostic to the decision making as to should/shouldn't it rewrite the contents of an existing unverified block file. It simply always does. Whoever created the writer task is responsible for potential race conditions and/or undesired behavior if two tasks that act upon a single unverified block file (with the same block number) exist and run in parallel. Also, I added a test to assert that the rewrite happens. Simply, whenever we use Files.newOutputStream(path) it uses default options which are write, create, truncate - this means that if a file does not exist it will be created. If it exists, it will be truncated to 0 bytes and then it will be written the new data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Block Node Issues/PR related to the Block Node.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add unverified blocks root
4 participants