feat: implements input validation functions for HashNode and HashLeaf to prevent panics #113
Conversation
Codecov Report
@@ Coverage Diff @@
## master #113 +/- ##
==========================================
+ Coverage 96.27% 96.31% +0.04%
==========================================
Files 6 6
Lines 429 461 +32
==========================================
+ Hits 413 444 +31
- Misses 10 11 +1
Partials 6 6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
staheri14
changed the title
staheri14
changed the title
hasher.go
Outdated
| // namespaced hash values. | ||
| func (n *Hasher) validateSiblingsNamespaceOrder(left, right []byte) (err error) { | ||
| // each NMT node has two namespace IDs for the min and max | ||
| totalNameSpaceLen := 2 * n.NamespaceLen |
There was a problem hiding this comment.
[uber nit] in some scenarios the "S" in namespace is capitalized and in other scenarios it isn't. For codebase consistency, would it help to consolidate on no capitalization per https://en.wikipedia.org/wiki/Namespace
| totalNameSpaceLen := 2 * n.NamespaceLen | |
| totalNamespaceLen := 2 * n.NamespaceLen |
There was a problem hiding this comment.
Resolved b1f17fb.
hasher_test.go
Outdated
| { // mismatched namespace size | ||
| "invalid node: mismatching namespace sizes", |
There was a problem hiding this comment.
| { // mismatched namespace size | |
| "invalid node: mismatching namespace sizes", | |
| { | |
| "invalid node: length", |
There was a problem hiding this comment.
Addressed faf4e50
hasher.go
Outdated
| @@ -14,6 +16,11 @@ const ( | |||
|
|
|||
| var _ hash.Hash = (*Hasher)(nil) | |||
|
|
|||
| var ( | |||
| ErrUnorderedSiblings = errors.New("NMT sibling nodes should be ordered lexicographically by namespace IDs") | |||
| ErrInvalidNodeLen = errors.New("Invalid NMT node size") | |||
There was a problem hiding this comment.
[nit][optional]
https://github.com/golang/go/wiki/CodeReviewComments#error-strings
| ErrInvalidNodeLen = errors.New("Invalid NMT node size") | |
| ErrInvalidNodeLen = errors.New("invalid NMT node size") |
There was a problem hiding this comment.
Addressed faf4e50
There was a problem hiding this comment.
It is important to note that returning an error from this method is not possible as it would violate the interface it is intended to fulfill i.e., NodeHasher and Hash.
The validation utility functions developed in this PR must be further integrated into both the tree construction and proof verification process to detect invalid inputs and notify the caller of any errors.
I'm wondering about the first statement. We might need to break the api, and that's ok imo. In fact, I assumed that we would have to to close #109. It would require significantly changes downstream, but I'm not sure that's avoidable even if we don't break the API.
I guess I just have questions over how we actually plan to call these ahead of time in order to avoid panics, but we can discuss that synchronously if that would be faster 🙂. Ideally, we would incorporate this into the code so that this was the default, since the panics here are purely to handle errors. We're still going to have to handle those errors downstream in order to write safe code, so might as well handle it here imo
otherwise LGTM, I like this change
I agree that some of the APIs will inevitably have to change. However, my aim was to keep the |
Overview
An incremental PR toward #109
Next: The validation utility functions developed in this PR must be further integrated into both the tree construction and proof verification process to detect invalid inputs and notify the caller of any errors. This will close #109 and is in line with the objectives outlined in issues #99 and #97. I will address this in a subsequent pull request.
Checklist