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

Clarify public API #12

Closed
liamsi opened this issue Jan 11, 2021 · 4 comments
Closed

Clarify public API #12

liamsi opened this issue Jan 11, 2021 · 4 comments
Labels
enhancement New feature or request help wanted Extra attention is needed question Further information is requested

Comments

@liamsi
Copy link
Member

liamsi commented Jan 11, 2021

The current public facing API:
image

Should we:

  • add methods to the extended data square? For instance: serialization and deserialzation (e.g. from and to protobuf), (square/orig. data) Width
  • clarify the names of the existing Encode/Decode functions (e.g. by exposing the methods of the Codec interface and removing the existing Encode/Decode functions)
  • decouple the erasure coding step from the row/col roots step (that way the library can be used with greater flexibility)

related: #11

@liamsi
Copy link
Member Author

liamsi commented Jan 20, 2021

For LazyLedger, we need to modify the erasure coded shares before they are committed to as row / col roots. Namely, we need to prefix the orig. data and the parity shares with their namespaces respectively. This is already possible using the public API by computing the row / column roots externally. But if we want to use the rsmt2d for this instead, we need to make it possible to transform the shares before they are committed to internally.

See for example this test-code: https://github.com/lazyledger/lazyledger-core/blob/4256e2090ba71873ee4698353de7d6646db97907/p2p/ipld/plugin/plugin_test.go#L116-L129

@adlerjohn
Copy link
Member

Additional: #56 (comment)

@liamsi
Copy link
Member Author

liamsi commented Apr 30, 2021

Another (potentially last?) point to address with the is how the library handles proofs:

Currently, we extracted a tree interface with a Prove method:
https://github.com/lazyledger/rsmt2d/blob/5a692bff96defabeaf8c928510c1894255e97761/tree.go#L22-L23

@adlerjohn noticed:

so while looking into #53, I realized different Merkle trees have different prove function signatures:

  1. Regular Merkle tree returns a bunch of stuff: https://github.com/lazyledger/merkletree/blob/6901c4c3c75f7ab8a0f11707cb0eb4e477f9074f/tree.go
  2. NMT returns a proof struct: https://github.com/lazyledger/nmt/blob/ddcc72040149c115f83b2199eafabf3127ae12ac/nmt.go
    [ ...]
  3. Defer constructing proofs to the caller, which should know exactly which tree is used and can handle the specific return values. Note that as-is, there is sufficient data for the caller to construct Merkle proofs (all repaired shares are returned). Also note that within the repair function there may not be enough data for all Merkle proofs to be constructed; some proofs must come from when a share is downloaded. As such, the caller will need to handle attaching the appropriate proofs regardless. I prefer this approach.

I think the latter is the approach which we should aim for. This also means compute(Row|Column)Proof can be deleted, or, rather moved into the (only) test files they are used:
https://github.com/lazyledger/rsmt2d/blob/5a692bff96defabeaf8c928510c1894255e97761/datasquare.go#L222-L232

We should rename this test https://github.com/lazyledger/rsmt2d/blob/5a692bff96defabeaf8c928510c1894255e97761/datasquare_test.go#L115
to TestDefaultTreeProofs or similar, to document to the user how this can be used when having the tree.

We should also introduce a rsmt2d_test package for tests and only test the public APIs properly (instead of using the private methods and fields all over the place).

Test files that declare a package with the suffix "_test" will be compiled as a separate package, and then linked and run with the main test binary.

Note that if we move handling Proofs to the caller of the library, we should explicitly state that in the documentation somewhere too. Also, I think we need a doc.go that goes a bit more into detail than the Readme.

@liamsi
Copy link
Member Author

liamsi commented Apr 27, 2022

Closinf in favour of:
#82
#83
#84
#85

And other improvements that were already done:

#78
#77

@liamsi liamsi closed this as completed Apr 27, 2022
@liamsi liamsi moved this to Done in Celestia Node Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed question Further information is requested
Projects
No open projects
Archived in project
Development

No branches or pull requests

2 participants