-
Notifications
You must be signed in to change notification settings - Fork 185
feat: Populate state proofs with real Merkle paths #22253
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
Signed-off-by: Matt Hess <[email protected]>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Signed-off-by: Matt Hess <[email protected]>
Signed-off-by: Matt Hess <[email protected]>
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #22253 +/- ##
=========================================
Coverage 74.67% 74.67%
- Complexity 23559 23573 +14
=========================================
Files 2520 2522 +2
Lines 95687 95768 +81
Branches 10173 10180 +7
=========================================
+ Hits 71451 71518 +67
- Misses 20445 20453 +8
- Partials 3791 3797 +6
... and 19 files with indirect coverage changes 🚀 New features to boost your workflow:
|
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 preferences |
Signed-off-by: Matt Hess <[email protected]>
Signed-off-by: Matt Hess <[email protected]>
Signed-off-by: Matt Hess <[email protected]>
Signed-off-by: Matt Hess <[email protected]>
timo0
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.
pending_proof.proto LGTM, thanks @mhess-swl
Signed-off-by: Matt Hess <[email protected]>
d0e40c4
Signed-off-by: Matt Hess <[email protected]>
Signed-off-by: Matt Hess <[email protected]>
Signed-off-by: Matt Hess <[email protected]>
Signed-off-by: Matt Hess <[email protected]>
Signed-off-by: Matt Hess <[email protected]>
Signed-off-by: Matt Hess <[email protected]>
Signed-off-by: Matt Hess <[email protected]>
| || Objects.equals(pendingProof.blockTimestamp(), Timestamp.DEFAULT) | ||
| || pendingProof.siblingHashesFromPrevBlockRoot().size() != 4) { | ||
| logger.warn( | ||
| "Pending proof metadata from {} is missing required fields (not considering remaining - {})", |
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 do we need to log warning here ?
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.
Logged this since the code doesn't try to reload any more blocks. And also because it logs a similar message just above.
| registered, | ||
| ("Unsigned block should be registered for every block between [%s, %s] –– block" | ||
| + " %s not registered") | ||
| .formatted(firstUnsignedBlockNum, signedBlockNum - 1, i)); |
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.
Have we validated these proofs work with the Verifiers given in Notion page ?
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.
ArrayStateProofVerifier or Option1DStateProofVerifier
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.
I talked with @edward-swirldslabs and @jsync-swirlds before the break–the verifiers actually need to be fixed. At the very least they need to take into account the single child prefix.
Also, the Option1DStateProofVerifier incorrectly expects the timestamp leaf to point to the siblings node instead of the parent node. I'll need to coordinate with @edward-swirldslabs to get the right algorithm.
| attempt.signatureFuture().thenAcceptAsync(signature -> { | ||
| finishProofWithSignature( | ||
| finalBlockRootHash, signature, attempt.verificationKey(), attempt.chainOfTrustProof()); | ||
| if (signature == null || Objects.equals(signature, Bytes.EMPTY)) { |
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.
Is it possible to have Bytes.EMPTY signature?
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.
I hope not. I did see one instance in local testing where the sig appeared to be empty, but wasn't able to confirm.
| // Block signatures on the current block will always produce a TssSignedBlockProof | ||
| proof = currentPendingBlock.proofBuilder().signedBlockProof(latestSignedBlockProof); | ||
| // Explicitly set empty per the protobuf spec | ||
| proof.siblingHashes(Collections.emptyList()); |
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.
As discussed these wont be used anymore right?
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.
I believe that's correct, yes. Will confirm today
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.
Removed in the latest commit
| proof = currentPendingBlock.proofBuilder().blockStateProof(stateProof); | ||
|
|
||
| // The first mp2 instance from the state proof has this block's sibling hashes | ||
| final var firstMp2Index = stateProof.paths().size() / 3; |
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.
What is 3 here. Defining a constant with a descriptive name will be helpful.
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.
I've defined a number of constants in the new code for any magic numbers :)
|
|
||
| // We can't verify the indirect proof until we have a signed block proof, so store the indirect proof for | ||
| // later verification and short-circuit the hints, history proofs (which require a signature) | ||
| indirectProofSeq.registerStateProof( |
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.
Does this actually run in CI or locally, because there are several indirect proofs? May be just Crypto check generates indirect proofs because TSS is enabled?
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.
Yes, it often runs on hapiTestRestart. See example failure here.
Signed-off-by: Matt Hess <[email protected]>
Signed-off-by: Matt Hess <[email protected]>
timo0
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.
pending_proof.proto LGTM :-)
Signed-off-by: Matt Hess <[email protected]>
Signed-off-by: Matt Hess <[email protected]>
Signed-off-by: Matt Hess <[email protected]>
This PR populates the merkle paths inside of a block's
StateProof, which must be generated any time a block is produced without an immediate signature.There are three merkle paths to generate for each block:
A full state proof of an unsigned block must also contain the merkle paths of each succeeding block that precedes the next signed block. That is, if a sequence of blocks A to N are unsigned, and block N+1 is signed, then a full proof for any single block X between [A, N] includes the merkle paths for X, and additionally must contain all the merkle paths for blocks X+1, X+2, ... X+N. The point is that the additional paths are necessary in order for any single state proof to provide all the hashes required to produce the hash of the signed block.
The state proof design requires the merkle paths to follow a depth-first ordering. Consequently, this code produces each set of paths using a pseudo pre-order traversal, resulting in the following array structure:
The first N-1 elements contain the paths representing the ordered timestamps of each block. The remainder of the array contains pairings of paths 2 and 3, with the final merkle path–representing the block's root hash–at the end of the array.
Testing for this feature has been done in three parts:
StateChangesValidatorclass to verify indirect proofs when encountered in the block streamDue to the relative (perceived?) rarity of blocks without a signature in our hapi test suites, it may be prudent to write a mechanism that forces the production of more unsigned blocks.
Closes #21209
EDIT: Design Changes
After discussion of which Merkle paths are necessary, we settled on three total paths for each state proof:
The production code and test examples have been updated accordingly.