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

UnmarshalJSON is limited to the default Tree #275

Closed
Wondertan opened this issue Dec 11, 2023 · 1 comment · Fixed by #277
Closed

UnmarshalJSON is limited to the default Tree #275

Wondertan opened this issue Dec 11, 2023 · 1 comment · Fixed by #277
Labels
bug Something isn't working

Comments

@Wondertan
Copy link
Member

Wondertan commented Dec 11, 2023

Problem

When we deserialize the EDS we hardcode the default tree. This is problematic because computed Row and Col roots of the deserialized eds will be utterly different if another custom tree is used.

Solutions

  1. Have a global tree constructor var/registry that UnmarshalJSON would use instead
    • Users would then register their trees by name
      • The name would be serialized in the json and deserialization would then get the proper tree by the name
    • This is exactly what happens with the codec.
  2. UnmarshalEDS func that accepts the tree.
@Wondertan Wondertan added the bug Something isn't working label Dec 11, 2023
@Wondertan
Copy link
Member Author

The first solution is bulletproof and requires minimum work outside of rsmt2d, as well the same approach is used for Codec, so we should go with it, imo

rootulp added a commit that referenced this issue Feb 7, 2024
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
#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.
0xchainlover pushed a commit to celestia-org/rsmt2d that referenced this issue Aug 1, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant