-
Notifications
You must be signed in to change notification settings - Fork 45
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: ensures consistent namespace hash format for all tree node parameters in VerifyInclusion function #176
Merged
staheri14
merged 26 commits into
master
from
verify-node-format-before-proof-verification
Apr 18, 2023
+43
−20
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
6b0ecaf
checks the exact size of the node
staheri14 a17fc23
revises tests descriptions
staheri14 14746d1
revises func description
staheri14 9cde761
refactors tests
staheri14 e0766e3
reorganizes helper functions and adds their descriptions
staheri14 422368f
adds more comments
staheri14 05017fc
resolves a gofumpt issue
staheri14 9b27e87
verifies root and proof.nodes validity at the beginning of verifyincl…
staheri14 ca7baf8
Merge branch 'master' into check-node-exact-length-in-hasher
staheri14 30abad6
Merge branch 'master' into verify-node-format-before-proof-verification
staheri14 7506e19
Merge branch 'check-node-exact-length-in-hasher' into verify-node-for…
staheri14 bbf7613
developes test for node format verification in the Verifyinclusion
staheri14 847c6ad
verifies that the node size is exactly the same as the expected size
staheri14 b021016
Merge branch 'check-node-exact-length-in-hasher' into verify-node-for…
staheri14 3e871b8
Merge branch 'master' into performs-arguments-consistency-check-to-ve…
staheri14 6bf186f
Merge branch 'master' into check-node-exact-length-in-hasher
staheri14 0844d82
explains that VerifyInclusion does not verify proof completeness
staheri14 f5427e5
adds a test case for node length > the expected size
staheri14 76b27b4
revises the error message
staheri14 dd6a640
revises test names
staheri14 428744d
Merge branch 'check-node-exact-length-in-hasher' into performs-argume…
staheri14 782b3f3
fixes gofumpt issues
staheri14 b5f617e
revises comments
staheri14 8628f5c
refactors tests
staheri14 91b96f7
Merge branch 'master' into verify-node-format-before-proof-verification
staheri14 bcd45c3
resolves test failure
staheri14 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
[non-blocking question]
similar to what we have discussed with needing to throw and error after the proof was verified if the namepsaces are not in order, do we need to do the same 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.
I believe the issue you are referring to is #97. Currently, if namespaces are not in order, the verification result is false without providing any error. However, we can choose to return a proper error message to indicate the root cause of verification failure, if this information would be useful for the caller. However, my current understanding is that returning an error from proof verification methods such as
VerifyInclusion
may not address the issue in question, as the problem is related to verifying that shares of a block are not out of order. To achieve this, the full node must construct NMTs from the block data and identify out-of-order leaves during tree construction.Does this address your question?
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.
ahh I See. consensus full nodes having the ability to generate otherwise valid proofs makes sense, but that's not possible, so there's no point.
do we need to enable non-consensus full nodes to have this ability if we want to protect against this in a trust minimized way then?
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 guess you are suggesting returning an error message from the
VerirfyInclusion
method to indicate whether leaves were out of order so that the light node can use that information to craft a bad encoding proof and propagate it in the network (or just consume it locally).Returning an error message is possible, but it may not provide a definitive conclusion, as in the current implementation of
VerirfyInclusion
(and other similar verification methods available), as soon as a violation is detected e.g., nodes are out of order, the verification stops and thefalse
result is returned. This means that the root does not get calculated from the given proof and leaves to see whether it matches the expected root (from the block header) or not. As such, even if an error gets returned, it would not be conclusive. If were to enable such feature, we need to modify the verification process to always calculate the root. Please let me know if this is a desired feature and I can track it in a separate GH issue.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 think we need more discussion around this and to walk through the entire flow. this convo is definitely not blocking merging.
I'm not entirely sure that we have to worry about it, since we this is the equivalent of 2/3s signing over an invalid data root. Typically, full nodes would simply reject that block, which is what would happen now, in our case though, this could result in light clients being tricked in very specific scenarios. It seems like something that we would want to fix to have trust minimized exclusion proofs. meaning that when a consensus or full node returns all the data in a namespace along with a prove of inclusion and completeness, then light clients can rely on the completeness without making an honest majority assumption or running a consensus node.
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 will make a note to discuss this further in our next call.