Verify inclusion proofs with multiple leaves #58
Conversation
Codecov Report
@@ Coverage Diff @@
## master #58 +/- ##
==========================================
+ Coverage 95.58% 95.60% +0.02%
==========================================
Files 7 7
Lines 362 364 +2
==========================================
+ Hits 346 348 +2
Misses 11 11
Partials 5 5
Continue to review full report at Codecov.
|
proof.go
Outdated
| func (proof Proof) VerifyInclusion(h hash.Hash, nid namespace.ID, data []byte, root []byte) bool { | ||
| func (proof Proof) VerifyInclusion(h hash.Hash, nid namespace.ID, data [][]byte, root []byte) bool { |
There was a problem hiding this comment.
This api is arbitrarily restricting proofs to the same namespace. While that fits our use case for the rest of the repos, the api could be simplified to match tree.Push() where the namespace is included in the data. This would simplify some other things as well, but I felt that was outside the scope of this PR.
There was a problem hiding this comment.
Can you document this public function? I don't actually know what [][]byte is just by looking at it.
|
Can you note in the PR description that this is a breaking API change? |
Co-authored-by: John Adler <[email protected]>
| @@ -8,7 +8,7 @@ import ( | |||
|
|
|||
| "github.com/celestiaorg/nmt" | |||
| "github.com/celestiaorg/nmt/namespace" | |||
| "github.com/google/gofuzz" | |||
| fuzz "github.com/google/gofuzz" | |||
There was a problem hiding this comment.
We should switch to the go-internal fuzzer soon.
| // and the provided proof to regenerate and compare the root. Note that the leaf | ||
| // data should not contain the prefixed namespace, unlike the tree.Push method, | ||
| // which takes prefixed data. All leaves implicitly have the same namespace ID: `nid`. | ||
| func (proof Proof) VerifyInclusion(h hash.Hash, nid namespace.ID, leaves [][]byte, root []byte) bool { |
There was a problem hiding this comment.
An additional convenience method VerifyNamespaceRange or sth alike does not make sense?
There was a problem hiding this comment.
I can see that working as well, tbh I just didn't think about that and was just following the underlying library's API.
I want to merge to wrap this up, but I also don't want to lose this comment, so #59
building on #57, we also need to be able to verify inclusion proofs for arbitrary ranges of leaves.
Note: this PR is API breaking and will require another version bump