Use integer division instead of floating-point division #16
Conversation
proofs.go
Outdated
| @@ -71,7 +70,7 @@ func (proof *SparseCompactMerkleProof) sanityCheck(th *treeHasher) bool { | |||
|
|
|||
| // Compact proofs: check that the length of the bit mask is as expected | |||
| // according to NumSideNodes. | |||
| len(proof.BitMask) != int(math.Ceil(float64(proof.NumSideNodes)/float64(8))) || | |||
| len(proof.BitMask) != (proof.NumSideNodes+8-1)/8 || | |||
There was a problem hiding this comment.
What happens if NumSideNodes is MaxInt and do we need to worry about this?
There was a problem hiding this comment.
There's already a sanity check (which should be called before trying to verify a proof): https://github.com/lazyledger/smt/blob/042131b944f99aafc1de3d9a34b982780878c418/proofs.go#L24-L25
There was a problem hiding this comment.
Hmm this does seem a bit dangerous as this is only safe because the above sanity check happens to be called before the the sanity check in this pull request. If someone refactors the code in the future, this could be overlooked and cause bugs.
There was a problem hiding this comment.
I agree, but 1) it's safer than float division and 2) if it's an immediate concern we can add a test for max int. The true solution though is at the type level: define a new SaneSparseMerkleProof type that is only for internal functions, for which the sanity check is guaranteed to have been done. That way correctness can be guaranteed by the compiler at the type level, without having to rely on potentially-fragile unit tests. This should be done regardless since there are other places in the code that make assumptions on sanity checks having been done.
There was a problem hiding this comment.
Fixed in bce1912, which introduces a method that is guaranteed to not overflow and does not depend on other checks.
There was a problem hiding this comment.
Hmm introducing an extra function (or an extra test) now seems to overcomplicate things. I'd rather keep the original, which is better for readability.
There was a problem hiding this comment.
Still not comfortable with floating point math in this context, but looking into the behavior, it looks like there won't be an issue so long as the dividend is under 2**50 (2**53 minus 2**3 = 8 to handle the division). Which should never happen due to the sanity checks.
So, I'm okay with keeping the floating point division here.
Instead of rounding up by converting to float then back to int, do rounding-up integer division directly.
Based on https://stackoverflow.com/a/17974.