-
Notifications
You must be signed in to change notification settings - Fork 80
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
revert!: rsmt2d#277 and rsmt2d#287 #295
Conversation
This reverts commit 5a03c15.
…tiaorg#277)" This reverts commit bb5e119.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #295 +/- ##
==========================================
+ Coverage 80.89% 82.24% +1.34%
==========================================
Files 8 7 -1
Lines 869 614 -255
==========================================
- Hits 703 505 -198
+ Misses 119 66 -53
+ Partials 47 43 -4 ☔ View full report in Codecov by Sentry. |
Mind providing a link to |
Sure! I updated the original post. |
Has |
Thank you for sharing the links. You're correct; the Additionally, I'm eager to hear your perspective on the challenges associated with "Celestia-app needs to be careful to register all the appropriate trees once (and only once) before they are used ". What do you see as the main concerns with this requirement? Nevertheless, I am going to approve and leave the final decision up to you. |
While attempting to bump celestia-app to the v0.12.0-rc2, I noticed that the `RegisterTree` design leaks an implementation detail to celestia-app: the registering and managing of `treeName`s. Celestia-app has two categories of of trees: 1. erasured namespaced merkle tree in [nmt_wrapper.go](https://github.com/celestiaorg/celestia-app/blob/main/pkg/wrapper/nmt_wrapper.go) 2. EDS subtree root cacher [nmt_caching.go](https://github.com/celestiaorg/celestia-app/blob/main/pkg/inclusion/nmt_caching.go) Each of those categories has trees based on square size and NMT options. Celestia-app needs to be careful to register all the appropriate trees once (and only once) before they are used (via `Compute` or `Import`). I'd like to explore a less breaking option to get celestia-node the original desired feature which was celestiaorg/rsmt2d#275. In the meantime, I think we should revert the two big breaking changes so that main can remain release-able. Revert celestiaorg/rsmt2d#277 Revert celestiaorg/rsmt2d#287 Closes celestiaorg/rsmt2d#295 because no longer relevant if we merge this.
While attempting to bump celestia-app to the v0.12.0-rc2, I noticed that the
RegisterTree
design leaks an implementation detail to celestia-app: the registering and managing oftreeName
s. Celestia-app has two categories of of trees:Each of those categories has trees based on square size and NMT options. Celestia-app needs to be careful to register all the appropriate trees once (and only once) before they are used (via
Compute
orImport
). I'd like to explore a less breaking option to get celestia-node the original desired feature which was #275. In the meantime, I think we should revert the two big breaking changes so that main can remain release-able.Revert #277
Revert #287
Closes #295 because no longer relevant if we merge this.