feat: Revisit Hash On-Init for AllBlocksHasher by ata-nas · Pull Request #2704 · hiero-ledger/hiero-block-node

ata-nas · 2026-05-05T14:05:19Z

Move the AllPreviousBlocksRootHashHasherSnapshot to internal protos
All blocks hasher general cleanup and hardening
Configuration property for optional hashing from history
Remove time-based AllPreviousBlocksRootHashHasherSnapshot persistence and make it block based (configurable)
Consolidate verification additions from feat(verification): add StateProof (indirect proof) verification (#2019) #2535 and fix(verification): disable allBlocksHasher by default and add missing docs for VerificationPlugin. #2666

Reviewer Notes

Related Issue(s)

codacy-production · 2026-05-05T14:06:58Z

Not up to standards ⛔

🔴 Issues 3 medium · 2 minor

Alerts:
⚠ 5 issues (≤ 0 issues of at least minor severity)

Results:
5 new issues

Category Results

CodeStyle 2 minor

Complexity 3 medium

View in Codacy

🟢 Metrics 0 complexity · 32 duplication

Metric Results

Complexity 0

Duplication 32

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer}
_{TIP This summary will be updated as you push new changes.}

ivannov

Looks to me that this PR captures whatever it is expected to capture.

I give my official unofficial +1, but I would leave other folks confirm

Nana-EC

Good start, did a first pass with some suggestions and questions.
Will need a second pass.
@jsync-swirlds and @AlfredoG87 should review next week

Nana-EC · 2026-05-09T01:47:02Z

+                        startOfBlockStateRootHash);
+            }
+        } catch (final IllegalStateException e) {
+            LOGGER.log(WARNING, "Block [{0}] has malformed proof structure: %s".formatted(e.getMessage()), e);


Looks like you're missing logic to insert the block number into the log.
Also probably don't need both message and the exception itself

This structure doesn't work correctly; the following should be better.

Suggested change

LOGGER.log(WARNING, "Block [{0}] has malformed proof structure: %s".formatted(e.getMessage()), e);

LOGGER.log(WARNING, "Block %d has malformed proof structure".formatted(blockNumber), e);

All of the new plugin was a straight up copy of the existing one. As things will change very drastically with the next PR, all of these will be addressed.

Nana-EC · 2026-05-09T01:47:33Z

+        MISSING_MANDATORY_FIELD,
+        /// This type indicates that the session was cancelled
+        CANCELLED,
+        /// This type indicates that an unknow error occurred


Suggested change

/// This type indicates that an unknow error occurred

/// This type indicates that an unknown error occurred

Thanks, I'll fix in the follow up.

Nana-EC · 2026-05-09T01:48:49Z

        /// This type indicates that the block is missing a mandatory item
        MISSING_MANDATORY_ITEM,
+        /// This type indicates that the block is missing a mandatory field (null)
+        MISSING_MANDATORY_FIELD,


Q: Is this a field within a proto message?

Yes, if a message is missing a mandatory field (no value provided).

Nana-EC · 2026-05-09T01:50:36Z

-        @Loggable @ConfigProperty(defaultValue = "10") int allBlocksHasherPersistenceInterval,
-        @Loggable @ConfigProperty(defaultValue = "/opt/hiero/block-node/verification/tss-parameters.bin") Path tssParametersFilePath) {}
+    @Loggable @ConfigProperty(defaultValue = "/opt/hiero/block-node/verification/rootHashOfAllPreviousBlocks.bin") Path allBlocksHasherFilePath,
+    @Loggable @ConfigProperty(defaultValue = "false") boolean allBlocksHasherEnabled,


Q: is this the logic to not use the history of block hashes tree in the block verification or is this just the calculation of the tree?

So, with the next PR for the new plugin, where we will drastically rework things, the all blocks hasher will simply no longer be a part of the verification process. As far as the enable/disable configuration, currently if enabled, we will attempt to initialize it and if successful, we will use for verification process. In the next PR, we can either remove it, or it will simply mean "try to initialize it and use it only to store new hashes, but will no longer be part of the verification process".

Nana-EC · 2026-05-09T01:55:25Z

    public static final byte[] ZERO_BLOCK_HASH = EMPTY_TREE_HASH;
    /// Expected size of a block hash in bytes.
    public static final int BLOCK_HASH_LENGTH = ZERO_BLOCK_HASH.length;
+    /// The initial value of the [#persistenceThresholdCounter], while this value is


Q: Do you mean where it's -1 persistence of the of allBlocksHash is disabled?

If we have successfully initialized the hasher, meaning it is enabled and working, on start we set this to 0. The sentinel value is used so that if -1, no persistence will be attempted.

This can and should probably change when we completely move the all blocks hasher out of verification.

Nana-EC · 2026-05-09T02:00:15Z

    public static final int BLOCK_HASH_LENGTH = ZERO_BLOCK_HASH.length;
+    /// The initial value of the [#persistenceThresholdCounter], while this value is
+    /// present, persistence is inactive. This is during init, on start go to 0 as default value.
+    private static final int UNINITIALIZED_PERSISTENCE_THRESHOLD = -1;


I don't see this getting set to 0 or anything else.
Also when used it checks if the counter is greater that it.
The counter that is compared to it also only checks for > meaning you only need one increment, so the threshold is always 1.
Is that the intention?

Also is this variable needed if there's also a variable for whether it's enabled? Is there ever a case where it's enabled but persistence is disabled? If not then just use the config value

The value is changed on start() if we are available.

Here is how the counter is used:
On each successful addition of a new hash, the counter is incremented assuming the all blocks hasher is available. If the threshold is passed, then we roll it back and persist. Subsequent calls continue to increment the counter until the threshold is passed and we repeat the process. The reason for the counter is that we want to configure an interval for persistence to avoid excessive disk usage.

Note that with upcoming PR, there will be only a single place where these updates will happen, so we will not worry about things racing. And in the future when the all blocks hasher is completely removed from verification plugin, we will simply have to report the newest hash and forget about the rest, from the perspective of the verification plugin that is.

And finally, to answer your last question, no, there are no cases where the all blocks hasher is enabled and persistence disable. There are cases where the all blocks hasher is enabled, but unavailable for any given reason.

Nana-EC · 2026-05-09T02:07:36Z

            return BlockUnparsed.PROTOBUF.parse(
                    rawData.toReadableSequentialData(), false, false, Codec.DEFAULT_MAX_DEPTH, MAX_BLOCK_SIZE_BYTES);
-        } catch (final ParseException e) {
+        } catch (final RuntimeException | ParseException e) {


Hmm, this swallows a more concerning error no? Currently if the block can't be parsed we swallow and log, but Runtime would catch more severe considerations.
Any culprits that you're trying to catch right now?

Might be a larger problem but if such an issue occurs should we be trying to clean up so that the BN can backfill it?

So, the thing is that we have defined the block accessor to return null if something happens during reading. There was a fairly recent change that made the same additions to implementations, but not here. I've made this addition so that we do not end up in a situation where we throw while trying to get an accessor. The caller will receive null if the block is not available or something happened when trying to read it.

Nana-EC · 2026-05-09T02:18:43Z

+    public void appendLatestHashToAllPreviousBlocksStreamingHasher(
+            @NonNull final byte[] blockHashBytes, final long blockNumber) {
+        if (isAvailable()) {
+            // @todo(2461) use blockNumber


yeah, unclear of why blockNumber isn't the only approach used here.
The the counter comparisons seem like proxies. I might need to circle back to this

This will be much improved once we move out the whole all blocks hasher out of verification. For now, (with an upcoming PR) the all blocks hasher will be extracted from the verification process and there will be a single point (thread) that will update this when appropriate. The counter is there to introduce an interval for persistence.

jsync-swirlds · 2026-05-09T02:46:11Z

+        // Must have at least 3 (signed block siblings) and remainder must be groups of 4 (gap blocks)
+        if (totalSiblings < 3 || (totalSiblings - 3) % 4 != 0) {
+            LOGGER.log(
+                    WARNING,
+                    "Block {0} state proof sibling count {1} is invalid (need >= 3, remainder must be multiple of 4)",
+                    blockNumber,
+                    totalSiblings);
+            return false;
+        }


Actually, the groups of 4 part might not be true.
We can check >= 3, but probably shouldn't check past that, because there are different ways to structure things past the first 3.

Interesting, thanks for the note. This is a straight up copy of existing logic. Once we have a follow up of this PR, we will be able to decouple hashing logic from proof verification logic and we can, piece by piece address these.

* Move the `AllPreviousBlocksRootHashHasherSnapshot` to internal protos * All blocks hasher general cleanup and hardening * Configuration property for optional hashing from history * Remove time-based `AllPreviousBlocksRootHashHasherSnapshot` persistence and make it block based (configurable) * Consolidate verification additions from #2535 and #2666 Signed-off-by: Atanas Atanasov <a.v.atanasov98@gmail.com>

codecov · 2026-05-11T08:17:06Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@             Coverage Diff              @@
##               main    #2704      +/-   ##
============================================
+ Coverage     81.46%   81.54%   +0.07%     
- Complexity     1508     1510       +2     
============================================
  Files           141      141              
  Lines          7154     7157       +3     
  Branches        756      756              
============================================
+ Hits           5828     5836       +8     
+ Misses         1018     1013       -5     
  Partials        308      308

Files with missing lines	Coverage Δ	Complexity Δ
...e/spi/blockmessaging/VerificationNotification.java	`69.23% <100.00%> (+9.23%)`	`3.00 <0.00> (ø)`
...block/node/spi/historicalblocks/BlockAccessor.java	`84.61% <100.00%> (ø)`	`1.00 <0.00> (ø)`

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ata-nas added this to the 0.34.0 milestone May 5, 2026

ata-nas self-assigned this May 5, 2026

ata-nas added the Verification Plugin Issue related to Verification Plugin label May 5, 2026

ata-nas linked an issue May 5, 2026 that may be closed by this pull request

Revisit Hash On-Init for AllBlocksHasher #2451

Open

ata-nas requested a review from jsync-swirlds May 5, 2026 14:05

ata-nas force-pushed the 2451-revisit-hash-on-init-for-allblockshasher branch 6 times, most recently from f9aa0cc to e0bd6d8 Compare May 7, 2026 10:15

ata-nas marked this pull request as ready for review May 7, 2026 10:19

ata-nas requested review from a team as code owners May 7, 2026 10:19

ata-nas requested review from AlfredoG87, Nana-EC, berryware, ivannov and rockysingh May 7, 2026 10:19

ata-nas force-pushed the 2451-revisit-hash-on-init-for-allblockshasher branch 3 times, most recently from a386e55 to a14186e Compare May 8, 2026 09:31

ivannov reviewed May 8, 2026

View reviewed changes

Nana-EC reviewed May 9, 2026

View reviewed changes

jsync-swirlds reviewed May 9, 2026

View reviewed changes

ata-nas force-pushed the 2451-revisit-hash-on-init-for-allblockshasher branch from a14186e to 29ca1a8 Compare May 11, 2026 08:07

	LOGGER.log(WARNING, "Block [{0}] has malformed proof structure: %s".formatted(e.getMessage()), e);
	LOGGER.log(WARNING, "Block %d has malformed proof structure".formatted(blockNumber), e);

	/// This type indicates that an unknow error occurred
	/// This type indicates that an unknown error occurred

Conversation

ata-nas commented May 5, 2026

Reviewer Notes

Related Issue(s)

Uh oh!

codacy-production Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Not up to standards ⛔

Uh oh!

ivannov left a comment

Choose a reason for hiding this comment

Uh oh!

Nana-EC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ata-nas May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented May 11, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codacy-production Bot commented May 5, 2026 •

edited

Loading

ata-nas May 11, 2026 •

edited

Loading