Implement compute-optimised Merkle tree #5
Conversation
- might call it with value and path roles mixed up (as happened here)
fix digestLeaf method signature
This reverts commit d1eb1ea.
Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>
|
TODO: investigate why this case is not being hit by test. https://coveralls.io/builds/32535000/source?filename=proofs.go#L80 Edit: done. |
|
It's probably a good idea to run sth like |
|
Looks like the DeepSparseMerkleSubTree.AddBranches needs to be reimplemented and cannot simply call the updateWithSideNodes function in an optimised tree, as this does more than simply add branches from the proof. |
|
I think this is ready to be merged. |
|
Speedtest to update 10,000 keys in a fresh tree: This confers to up to a ~10x speed-up. |
proofs.go
Outdated
| // BitMask, in the case of a compact proof, is a bit mask of the sidenodes | ||
| // of the proof where an on-bit indicates that the sidenode at the bit's | ||
| // index is a placeholder. This is only set if the proof is compact. | ||
| BitMask []byte | ||
|
|
||
| // NumSideNodes, in the case of a compact proof, indicates the number of | ||
| // sidenodes in the proof when decompacted. This is only set if the proof is compact. | ||
| NumSideNodes int |
There was a problem hiding this comment.
I don't fully understand how that would make the API fiddly and complicated (imho it could make it clearer). Let's not bother with this now. Happy to look into it after this gets merged.
|
Ok, I'm going to split compact proofs to a different type. |
|
Done |
| type keyAlreadyEmptyError struct{} | ||
|
|
||
| func (e *keyAlreadyEmptyError) Error() string { | ||
| return "key already empty" | ||
| } |
There was a problem hiding this comment.
How is this error useful if not public? If it is public users can switch over it and handle it accordingly.
Ah OK, I see: this is only used internally (and switched over).
This can be simplified to var keyAlreadyEmptyError = errors.New("key already empty") , then you can do
if err == keyAlreadyEmptyError instead of if _, ok := err.(*keyAlreadyEmptyError); ok {.
Or, use the errors.Is function. See "Errors in Go 1.13" in https://blog.golang.org/go1.13-errors
Also, name should be errKeyAlreadyEmpty.
| value, err := smt.ms.Get(valueHash) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
These Gets won't err in tests because the in-memory store never errs. In the future we might want to test these too.
There was a problem hiding this comment.
Sure, but if someone changes the in-memory store implementation in the future then this ensures that any errors returned are caught in tests.
smt.go
Outdated
| if err != nil { | ||
| return SparseMerkleProof{}, err | ||
| } |
There was a problem hiding this comment.
Why is this err checked here? That is confusing, as it would occur several lines up in:
sideNodes, leafHash, leafData, err := smt.sideNodesForRoot(path, root)
There was a problem hiding this comment.
I'm not sure why I did that. Fixed.
| // BadProofError is returned when an invalid Merkle proof is supplied. | ||
| type BadProofError struct{} | ||
|
|
||
| func (e *BadProofError) Error() string { | ||
| return "bad proof" | ||
| } |
There was a problem hiding this comment.
Similar to the other place, this is equivalent to a sentinel error:
| // BadProofError is returned when an invalid Merkle proof is supplied. | |
| type BadProofError struct{} | |
| func (e *BadProofError) Error() string { | |
| return "bad proof" | |
| } | |
| // ErrBadProof is returned when an invalid Merkle proof is supplied. | |
| var ErrBadProof = errors.New("bad proof") |
Also, it is more common / go-idiomatic to have a Err prefix.
There was a problem hiding this comment.
Isn't it better to have a special type for the error, so that users of the API can differentiate between error types?
There was a problem hiding this comment.
In go it is very common to use sentinel values for cases like this (this custom error is just returns a string with no further fields or context). See for instance the io package: https://golang.org/pkg/io/#pkg-variables
The custom error doesn't provide any additional value compared to a sentinel error (not to say that in other cases it could make more sense to use a custom error).
Also, worth looking at: https://blog.golang.org/go1.13-errors
WIP
With whitespaces changes hidden: https://github.com/lazyledger/smt/pull/5/files?diff=unified&w=1