chore: Remove Previous Block Hash from BlockHeader #17862
Conversation
Signed-off-by: Thomas Moran <[email protected]>
thomas-swirlds-labs
requested review from
edward-swirldslabs,
timfn-hg,
OlegMazurov and
Neeharika-Sompalli
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17862 +/- ##
=========================================
Coverage 69.01% 69.02%
- Complexity 23036 23051 +15
=========================================
Files 2654 2655 +1
Lines 99652 99674 +22
Branches 10284 10289 +5
=========================================
+ Hits 68777 68796 +19
- Misses 26976 26985 +9
+ Partials 3899 3893 -6
... and 31 files with indirect coverage changes |
Signed-off-by: Thomas Moran <[email protected]>
Signed-off-by: Thomas Moran <[email protected]>
…der' into 17861-remove-block-hash-from-header
Signed-off-by: Thomas Moran <[email protected]>
| @@ -254,7 +254,6 @@ public void startRound(@NonNull final Round round, @NonNull final State state) { | |||
| worker = new BlockStreamManagerTask(); | |||
| final var header = BlockHeader.newBuilder() | |||
| .number(blockNumber) | |||
| .previousBlockHash(lastBlockHash) | |||
There was a problem hiding this comment.
We should also be able to change when we compute lastBlockHash. The whole point of this change is so we have more time to compute it as it is not needed till the end of the file.
There was a problem hiding this comment.
Agreed, we will do this in a small follow up PR 👍
Signed-off-by: Thomas Moran <[email protected]>
| * </li> | ||
| * <li>The block hash SHALL be the SHA-384 hash calculated for the root | ||
| * of this merkle tree.</li> | ||
| * </ul> | ||
| */ | ||
| bytes previous_block_root_hash = 2; |
There was a problem hiding this comment.
Just to confirm, in order to check the signature in this BlockProof for Block 4, we have to compute the root hash of the merkle tree for Block 4, merge it with the sibling hashes and validate that the signature matches the result?
- Where does the
previous_block_root_hashfit into this picture?
I think there is a problem in this new formulation. If this is the BlockProof for Block 4, the root hash for Block 3 needs to be part of the merkletree of block 4 and signed. The BlockProof for Block 4 is outside of the merkle tree contributing to the block root hash for block 4. We deleted the previous block root hash from the block header, but did not re-add it anywhere else inside the merkle tree.
We need to add a new BlockItem that is the root hash of the previous block so that it is a formal part of the merkle tree being signed.
There was a problem hiding this comment.
@jasperpotts, can you double check my thoughts here?
Description:
The previousBlockHash is no longer required in the Block Stream BlockHeader because it causes a timing problem that prevents optimal parallel thread pipelining in the consensus node and we do not want to hold up creating the block header until that hash is ready. It will continue to be available in the Block Proof however.
Related issue(s):
Fixes #17861, hashgraph/hedera-protobufs#468
Checklist