Implement wrapper for rsmt2d.Tree #25
Conversation
Codecov Report
@@ Coverage Diff @@
## master #25 +/- ##
==========================================
- Coverage 94.82% 92.40% -2.43%
==========================================
Files 7 8 +1
Lines 309 329 +20
==========================================
+ Hits 293 304 +11
- Misses 11 19 +8
- Partials 5 6 +1
Continue to review full report at Codecov.
|
| adjustedMessageSize = shareSize - DefaultNamespaceIDLen | ||
| ) | ||
|
|
||
| func TestPushErasuredNamespacedMerkleTree(t *testing.T) { |
There was a problem hiding this comment.
I wasn't sure what else I could test, so any suggestions are greatly appreciated.
There was a problem hiding this comment.
I think there is not much to test as the wrapper is really simple. And that is good. Well done!
The only thing I'd suggest is to also cover the non-happy paths in unit tests (e.g. the few edge cases where this can panic/error).
| @@ -5,4 +5,5 @@ go 1.14 | |||
| require ( | |||
| github.com/google/gofuzz v1.2.0 | |||
| github.com/lazyledger/merkletree v0.0.0-20201214195110-6901c4c3c75f | |||
| github.com/lazyledger/rsmt2d v0.0.0-20210121155026-23bf69f65287 | |||
There was a problem hiding this comment.
IMO, the NMT should not need to know about rsmt2d. That is why I think the wrapper should live in ll-core. There all the dependencies need to come together anyways. And introducing the dependency here might cause some headaches in the future.
There was a problem hiding this comment.
So what's the plan here, is the logic in this PR going to be moved instead of lazyledger-core?
There was a problem hiding this comment.
yeah, that makes a lot of sense!
| "github.com/lazyledger/rsmt2d" | ||
| ) | ||
|
|
||
| var paritySharesNamespaceID = namespace.ID{0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF} |
There was a problem hiding this comment.
Note that the tree already knows about this internally: https://github.com/lazyledger/nmt/blob/e0a317aa2d0ff84fcf1478527e4c90e6a13245d7/hasher.go#L77
| // if the data is erasure data use the parity ns | ||
| default: | ||
| copy(nsID, paritySharesNamespaceID) | ||
| } |
There was a problem hiding this comment.
Couldn't the caller do this instead? They already know which shares are parity shares. Then they could simply prefix the parity shares with namespace.ID{0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}. It would simplify the wrapper a bit.
On another note, I think we should change the method signature for Push from Push(nid, data []byte) (currently) to Push(data []byte) for the underlying nmt anyways. I think I've mentioned this in some convos but I never got around opening a small issue explaining the rationale for that 😬 Will do in a sec.
There was a problem hiding this comment.
+1 for changing the signature
We will need the wrapper to be aware of the parity share, as it will have to be passed to rsmt2d.RepairExtendedDataSquare
| adjustedMessageSize = shareSize - DefaultNamespaceIDLen | ||
| ) | ||
|
|
||
| func TestPushErasuredNamespacedMerkleTree(t *testing.T) { |
There was a problem hiding this comment.
I think there is not much to test as the wrapper is really simple. And that is good. Well done!
The only thing I'd suggest is to also cover the non-happy paths in unit tests (e.g. the few edge cases where this can panic/error).
|
I'm going to close this PR for now. As mentioned in the comments, it makes sense to keep any new logic in lazyledger-core. I think we will still need a wrapper, because we need to pass a |
This PR creates a wrapper for
NamespacedMerkleTreethat conforms to thersmt2d.Treeinterface, which will enableRetrieveBlockDatato properly repair the extended data square. It will also help close #172.ref #15
closes: #24