feat: compute leaf hashes in the Push method

[staheri14]

Overview

Closes #108 and #143

Checklist

• New and updated code has appropriate documentation
• New and updated code has new and/or updated testing
• Required CI checks are passing
• Visual proof for any user facing features like CLI or documentation updates
• Linked issues closed with keywords

[codecov]

Codecov Report

Merging #172 (5b53987) into master (1884bfb) will decrease coverage by 0.43%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
- Coverage   96.19%   95.76%   -0.43%     
==========================================
  Files           5        5              
  Lines         525      519       -6     
==========================================
- Hits          505      497       -8     
- Misses         12       13       +1     
- Partials        8        9       +1     

Impacted Files	Coverage Δ
nmt.go	97.94% <66.66%> (-1.08%)	⬇️
hasher.go	97.54% <100.00%> (+0.08%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[evan-forbes]

only two questions, otherwise LGTM this makes sense.

iirc, we have some benchmarks in this repo, have we compared this change against the old yet?

[evan-forbes]

why not bubble the error here if there ever is one?

[staheri14]

There are two reasons why the error is not being bubbled up:

• In line 410, the data validity is checked through the validateAndExtractNamespace function call. As a result, the HashLeaf function call cannot produce an error. To handle this scenario, the MustHashLeaf function is introduced, which assumes that the data passed to it has already been validated. This approach is similar to the current situation and avoids any unnecessary error handling.

• If we were to use HashLeaf and bubble up the error, we would need to introduce an if block in our code i.e., the Push method. However, this if block would not be covered in our tests, leading to a reduction in test coverage percentage. Therefore, I have opted not to bubble up the error in this situation.

Please let me know if this addresses your question.

[evan-forbes]

that makes sense, but documenting it in the code for posterity would be a good addition imo. we could also just ignore the error if its impossible (and we have comments), but in general I think its always best to bubble even if we don't strictly need to. Its one less thing the reader or future contributors have to think about.

I'm not blocking on it though! will defer to the author 🙂

[staheri14]

Fair point, replaced MustHashLeaf with HashLeaf and bubbled up the error, please see d60367b
With this change, the code coverage percentage decreases below the set threshold, and there is no way to develop a test to cover the line that captures the returned error. In such situations, what should we do? Should we still merge the PR?

[evan-forbes]

Yeah, 100% merge imo

[rootulp]

LGTM

[staheri14]

cc: @evan-forbes
Please find the benchmark comparison below. It's worth noting that the change introduced by this PR doesn't affect the ComputeRoot() benchmark test result. This is because the time measurement captures the total time taken to push N items and then compute the root. Before this change, the items were hashed inside the Root() method, whereas now they are hashed within the Push() method. As a result, the total number of hashes remains unchanged, and therefore the performance for this particular benchmark test does not improve.

goos: darwin
goarch: arm64
pkg: github.com/celestiaorg/nmt
                          │   master.txt   │              hash-and-push.txt               │
                          │   sec/op    │   sec/op     vs base               │
ComputeRoot/64-leaves-10    36.28µ ± 1%   38.10µ ± 0%  +5.00% (p=0.000 n=10)
ComputeRoot/128-leaves-10   72.23µ ± 0%   75.29µ ± 0%  +4.24% (p=0.000 n=10)
ComputeRoot/256-leaves-10   146.4µ ± 1%   152.7µ ± 1%  +4.30% (p=0.000 n=10)
geomean                     72.66µ        75.94µ       +4.51%

[staheri14]

@rootulp Can you please review the recent changes 🙏🏼