celestiaorg / rsmt2d

Go implementation of two dimensional Reed-Solomon merkle tree data availability scheme.
Apache License 2.0
162 stars 80 forks source link

Clarify public API #12

Closed liamsi closed 2 years ago

liamsi commented 3 years ago

The current public facing API: image

Should we:

related: #11

liamsi commented 3 years ago

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 commented 3 years ago

Additional: https://github.com/lazyledger/rsmt2d/pull/56#discussion_r622810103

liamsi commented 3 years ago

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 https://github.com/lazyledger/rsmt2d/issues/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 commented 2 years ago

Closinf in favour of: https://github.com/celestiaorg/rsmt2d/issues/82 https://github.com/celestiaorg/rsmt2d/issues/83 https://github.com/celestiaorg/rsmt2d/issues/84 https://github.com/celestiaorg/rsmt2d/issues/85

And other improvements that were already done:

https://github.com/celestiaorg/rsmt2d/issues/78 https://github.com/celestiaorg/rsmt2d/issues/77