Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reuse hash #40

Merged
merged 3 commits into from
Aug 17, 2021
Merged

Reuse hash #40

merged 3 commits into from
Aug 17, 2021

Conversation

liamsi
Copy link
Member

@liamsi liamsi commented May 23, 2021

After reviewing celestiaorg/celestia-core#351 I've realized that #38 introduced an unnecessary alloc per leaf and inner node.

This PR reverts the changes of #38, caches the root s.t. it only gets computed when necessary but keeps that test that aims to reproduce the issue in #37. (draft as I want to confirm this really does not re-introduce the issue in test withing ll-core, too celestiaorg/celestia-core@1522021)

@codecov
Copy link

codecov bot commented May 23, 2021

Codecov Report

Merging #40 (945e12f) into master (a25c390) will increase coverage by 1.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   93.77%   94.78%   +1.01%     
==========================================
  Files           7        7              
  Lines         305      307       +2     
==========================================
+ Hits          286      291       +5     
+ Misses         14       11       -3     
  Partials        5        5              
Impacted Files Coverage Δ
proof.go 97.05% <ø> (ø)
hasher.go 100.00% <100.00%> (+5.66%) ⬆️
nmt.go 93.10% <100.00%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a25c390...945e12f. Read the comment docs.

@Wondertan
Copy link
Member

Wondertan commented May 23, 2021

It is so cool that you immediately realized how to apply here the change @jsign proposed. Love this!

@liamsi
Copy link
Member Author

liamsi commented May 24, 2021

Thanks :-) We were already doing the suggested change (of celestiaorg/celestia-core#351) here. Only very recently (#38) we changed this for (#37). But yeah reviewing that code made me realize that our "bugfix" (quotes as the bug only happened during unit tests), wasn't the smartest one and we should have kept the existing code in favour of the fix here.
In general there is plenty of room for optimizations in this library (nmt).

Two low hanging fruits are:

  • have one function call that combines Push and Root (takes all leaves and immediately returns the hash)
  • get rid of the nebulous labs code for proofs (remove the dependency entirely)

@liamsi liamsi marked this pull request as ready for review August 6, 2021 10:03
@liamsi
Copy link
Member Author

liamsi commented Aug 6, 2021

IMO, the test in #38 is probably enough confirmation that this does not re-introduce the problem pre #38/#37.

@adlerjohn
Copy link
Member

Is this branch up-to-date against master? It's hard to tell from the force push.

@liamsi
Copy link
Member Author

liamsi commented Aug 7, 2021

Yes, the force push was to rebase on master.

@liamsi liamsi merged commit 02cdbfd into master Aug 17, 2021
@liamsi liamsi deleted the ismail/reuse_hash branch August 17, 2021 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants