Skip to content

docs: Add clear clarification around previousBlockHash source in RecordFileItem pseudocode#24508

Open
rockysingh wants to merge 1 commit intohiero-ledger:mainfrom
rockysingh:fix/record-file-item-pseudocode-previousBlockHash
Open

docs: Add clear clarification around previousBlockHash source in RecordFileItem pseudocode#24508
rockysingh wants to merge 1 commit intohiero-ledger:mainfrom
rockysingh:fix/record-file-item-pseudocode-previousBlockHash

Conversation

@rockysingh
Copy link
Copy Markdown

Summary

  • Renamed previousBlockHash to previousRecordFileHash in the reconstructRecordFile pseudocode to avoid confusion with BlockFooter.previous_block_root_hash (which is the block stream merkle tree root hash — a completely different value)
  • Added explicit variable bindings showing where all procedure parameters come from (record_file_contents, BlockProof.SignedRecordFileProof)
  • Added a NOTE explaining that start_object_running_hash has different semantics per version:
    • v2: Stores the hash of the previous record file (not a running hash)
    • v5/v6: Stores the actual running hash of all RecordStreamItems before this file

Context

The pseudocode referenced a previousBlockHash parameter without documenting its source. This was misleading because it could be mistaken for BlockFooter.previous_block_root_hash. In reality, the value comes from record_file_contents.start_object_running_hash.hash, which for v2 files holds the previous record file's content hash — a semantic overload of a field that means something different for v5/v6.

This is a documentation-only change — no proto fields or message definitions are modified.

@rockysingh rockysingh requested review from a team as code owners March 25, 2026 18:43
@trunk-io
Copy link
Copy Markdown

trunk-io Bot commented Mar 25, 2026

✨ Submitted to Merge by @jsync-swirlds. It will be added to the merge queue once all branch protection rules pass and there are no merge conflicts with the target branch. See more details here.

@lfdt-bot
Copy link
Copy Markdown

lfdt-bot commented Mar 25, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@rockysingh rockysingh changed the title docs: Clarify previousBlockHash source in RecordFileItem pseudocode docs: Add clear clarification around previousBlockHash source in RecordFileItem pseudocode Mar 25, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

Impacted file tree graph

@@             Coverage Diff              @@
##               main   #24508      +/-   ##
============================================
+ Coverage     78.21%   78.22%   +0.01%     
+ Complexity    11679    11676       -3     
============================================
  Files          2486     2486              
  Lines         94619    94619              
  Branches      10211    10210       -1     
============================================
+ Hits          74004    74015      +11     
+ Misses        16871    16862       -9     
+ Partials       3744     3742       -2     

see 13 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@jasperpotts jasperpotts left a comment

Choose a reason for hiding this comment

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

Good catch

Copy link
Copy Markdown
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

@rockysingh rockysingh force-pushed the fix/record-file-item-pseudocode-previousBlockHash branch from ff5bc8a to 8e8e01f Compare March 25, 2026 23:45
The reconstructRecordFile pseudocode referenced a previousBlockHash
parameter without documenting where it comes from. This was confusing
because it could be mistaken for BlockFooter.previous_block_root_hash,
which is a completely different value (the block stream merkle tree
root hash of the previous block).

The fix:
- Renamed the parameter to previousRecordFileHash for clarity
- Added explicit variable bindings showing all parameters come from
  record_file_contents and BlockProof.SignedRecordFileProof
- Added a NOTE explaining that start_object_running_hash has different
  semantics per version: for v2 it holds the previous record file hash,
  while for v5/v6 it holds the actual running hash of RecordStreamItems

Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
@rockysingh rockysingh force-pushed the fix/record-file-item-pseudocode-previousBlockHash branch from 8e8e01f to 3d45dad Compare March 25, 2026 23:45
@rockysingh
Copy link
Copy Markdown
Author

/trunk merge

@trunk-io
Copy link
Copy Markdown

trunk-io Bot commented Mar 25, 2026

An error occurred while submitting your PR to the queue: Only users that are a part of this repo's Trunk organization or have write permissions to the repo can submit a PR to the queue

@jsync-swirlds
Copy link
Copy Markdown
Contributor

/trunk merge

* recordStreamFile):
* WRITE version number (4 bytes, int)
* IF version == 2:
* WRITE HAPI major version (4 bytes int)
Copy link
Copy Markdown

@xin-hedera xin-hedera Mar 26, 2026

Choose a reason for hiding this comment

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

When creating WRB, do we read the HAPI version number from V2 file and write it into record_file_contents.hapi_proto_version.major?

If so and before it's too late, we should instead write it to record_file_contents.hapi_proto_version.minor and update the pseudo code.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good catch — you're right that the single int in V2 files (value 3) semantically represents HAPI version 0.3.0, not 3.0.0. We do currently read it and store it in hapi_proto_version.major.

However, after looking into it, I don't think we should change it:

  1. The round-trip is lossless — we read the int into major, and the pseudocode reconstructs V2 by writing hapi_proto_version.major back as the single int. The original V2 binary is reproduced byte-for-byte, and signature verification works correctly.

  2. Changing it would break existing wrapped blocks — storing 3 in minor instead of major changes the protobuf serialization of SemanticVersion (field 1 vs field 2), which changes the RecordStreamFile bytes, which changes the block hash for every V2-era wrapped block (~blocks 0 to ~1.8M). All existing hash registries, Merkle trees, and jumpstart files would be invalidated and everything would need to be re-wrapped and re-validated.

  3. No practical impact — V2 blocks are from 2019 (the start of mainnet). No downstream consumer is making decisions based on hapi_proto_version from these blocks.

So the cost of fixing (re-wrapping millions of blocks, invalidating all state files) outweighs the cosmetic benefit. The pseudocode and code are internally consistent as-is.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@xin-hedera do you agree we shouldn't change this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants