Subrootpaths

[mattdf]

This PR adds a function GetSubrootPaths to generate the minimal set of paths that define the set of subroots in the Namespace Merkle Tree which span a set of shares.

This is required in order to verify a PayForMessage transaction when accepting a new block that you didn't produce, and for fraud proofs.

[codecov]

Codecov Report

Merging #49 (fee9337) into master (1d72cff) will increase coverage by 1.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   94.44%   95.56%   +1.12%     
==========================================
  Files           6        7       +1     
  Lines         288      361      +73     
==========================================
+ Hits          272      345      +73     
  Misses         11       11              
  Partials        5        5              

Impacted Files	Coverage Δ
subrootpaths.go	100.00% <100.00%> (ø)

---

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 1d72cff...fee9337. Read the comment docs.

[Wondertan]

Concerned about disposition of the code. Is it ok that we bring here notions of rsmt2d that nmt wasn't aware before? Would it make sense to keep this code together with the NMTWrapper(adapter for rsmt2d tree) or move it to rsmt2d instead?

cc @adlerjohn

[liamsi]

Is it ok that we bring here notions of rsmt2d that nmt wasn't aware before?

Which rsmt2d notions are you referring to specifically? OK, I see: the code talks about shares, and rows and square sizes. I agree that this should probably be moved out of the nmt lib. That said, we are currently moved out that wrapper from core to node and are thinking of moving it into a separate repo (including the ipld plugin code etc). Let's review it here first.

I think what would be great to have some sorts of integration test (not in this repo but in the app probably) that wires this together with the app to basically show-case the first part of this:

This is required in order to verify a PayForMessage transaction when accepting a new block that you didn't produce, and for fraud proofs.

[adlerjohn]

Actually, you're right @Wondertan that this doesn't really belong in this repo because it has knowledge of multiple rows and a square, which is something the NMT shouldn't know about. Targeting rsmt2d makes more sense, but also isn't perfect because the rsmt2d shouldn't be aware of messages. That being said, it's just returning subtree roots, which isn't necessarily only for messages, it just happens to be applicable to messages. Can we move this PR to target rsmt2d instead, @mattdf?

[liamsi]

I'm against putting this into rsmt2d either. The right place would be together / close to this code: https://github.com/celestiaorg/celestia-core/blob/c2df2c4ce0942b8e974fd2b530355f2a58139926/pkg/wrapper/nmt_wrapper.go#L23

But as this is also about to be moved, I'd simply keep the pure functions here and focus on actually making sure that they can be used as intended.

[evan-forbes]

As @Wondertan stated, this code is not nmt specific, as it encodes details relevant to rsmt2d and to the non interactive defaults set forth in the spec that are only relevant to celestia. It will also be used by anyone who is attempting to verify the inclusion of messages or create message inclusion fraud proofs, so I agree with @liamsi that the best place for this code is currently in the celestia-core's pkg/ directory. At least until we move it to it's own repo. We haven't decided where we will actually check for inclusion of the message, see ADR008, but it will need to be either celestia-core or celestia-app, and then also rechecked by celestia-node.

[mattdf]

Regarding where to put this, this function effectively computes pathsets for "square" organized merkle trees, so it should be part of some merkle-tree related package.

The functionality is fairly low level from a merkle-tree perspective, an implementation of whatever *mt lib is used should likely provide a typed interface to the user that uses this internally rather than exposing it directly, so not sure that celestia-core or celestia-app is the right level to include this at.

[evan-forbes]

The functionality is fairly low level from a merkle-tree perspective, an implementation of whatever *mt lib is used should likely provide a typed interface to the user that uses this internally rather than exposing it directly, so not sure that celestia-core or celestia-app is the right level to include this at.

that makes sense, perhaps rsmt2d is the best place then.

[liamsi]

Left some nits. I really think we need some kind of integration test that shows why this is necessary right now (by actually using GetSubrootPaths in some other context than unit tests).