feat: add path generation for nmt subroots #621
Conversation
Codecov Report
@@ Coverage Diff @@
## main #621 +/- ##
==========================================
+ Coverage 29.89% 38.46% +8.57%
==========================================
Files 16 26 +10
Lines 2054 2740 +686
==========================================
+ Hits 614 1054 +440
- Misses 1380 1600 +220
- Partials 60 86 +26
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Co-authored-by: Rootul Patel <rootulp@gmail.com>
evan-forbes
requested review from
rootulp and
adlerjohn
and removed request for
adlerjohn
…iaorg/celestia-app into evan/nmt-subroot-path-gen
| } | ||
| } | ||
|
|
||
| // canClimbRight uses the current position to calculate the direction of the next |
There was a problem hiding this comment.
I don't understand this part. You can only climb in one direction, right? It is when you're going down that you have right or left path, no?
There was a problem hiding this comment.
Like you mentioned, with climb terminology, one could think a child node climbs up to a parent node (i.e. only one direction). But another way of thinking about this method: isLeftChild().
Maybe this comment helps? https://github.com/celestiaorg/celestia-app/pull/621/files/d2d645b024464bcbcfd7114b42e9e154f77729cf#r948000052
There was a problem hiding this comment.
// Depth Position
// 0 0
// / \
// / \
// 1 0 1
// /\ /\
// 2 0 1 2 3
// /\ /\ /\ /\
// 3 0 1 2 3 4 5 6 7
What canClimbRight() does is, for (depth=3, position=5), check if we can climb from 5 to 2. In this case, canClimbRight() will return false.
For (depth=3, position=6), it can climb right to 3.
I guess we need to explain it with an example in the docs.
| } | ||
| } | ||
|
|
||
| // canClimbRight uses the current position to calculate the direction of the next |
There was a problem hiding this comment.
Like you mentioned, with climb terminology, one could think a child node climbs up to a parent node (i.e. only one direction). But another way of thinking about this method: isLeftChild().
Maybe this comment helps? https://github.com/celestiaorg/celestia-app/pull/621/files/d2d645b024464bcbcfd7114b42e9e154f77729cf#r948000052
| } | ||
|
|
||
| // climb is a state transition function to simulate climbing a balanced binary | ||
| // tree, using the current node as input and the next highest node as output. |
There was a problem hiding this comment.
Probably would be better to add to the docs that we expect canClimbRight() as a precondition.
There was a problem hiding this comment.
I don't think being able to climb right is a technically a precondition? or perhaps I'm just not clear on what you mean by this? the implementation that uses this code only climbs if it can, but is that different?
pkg/inclusion/paths.go
Outdated
| // climb. Returns true if the next climb is right (if the position (index) is | ||
| // even). | ||
| func (c coord) canClimbRight() bool { | ||
| return c.position%2 == 0 |
There was a problem hiding this comment.
Shouldn't we also check if depth != 0?
There was a problem hiding this comment.
you're right, we probably should just to avoid potential leaky abstractions (although I hope others do not use coord for anything 😅) as we're relying on the implementation to do this for us atm
| // genSubTreeRootPath calculates the path to a given subtree root of a node, given the | ||
| // depth and position of the node. note: the root of the tree is depth 0. | ||
| // The following nolint can be removed after this function is used. | ||
| //nolint:unused,deadcode |
There was a problem hiding this comment.
Shouldn't we add a test for this one? Seems standalone and can be tested easily.
There was a problem hiding this comment.
yes, we should! cherry picked this from the implementation 9cf9f54
| } | ||
| } | ||
|
|
||
| // canClimbRight uses the current position to calculate the direction of the next |
There was a problem hiding this comment.
// Depth Position
// 0 0
// / \
// / \
// 1 0 1
// /\ /\
// 2 0 1 2 3
// /\ /\ /\ /\
// 3 0 1 2 3 4 5 6 7
What canClimbRight() does is, for (depth=3, position=5), check if we can climb from 5 to 2. In this case, canClimbRight() will return false.
For (depth=3, position=6), it can climb right to 3.
I guess we need to explain it with an example in the docs.
pkg/inclusion/paths.go
Outdated
| //nolint:unused,deadcode | ||
| func genSubTreeRootPath(depth int, pos uint) []bool { | ||
| path := make([]bool, depth) | ||
| for i := depth; i >= 0; i-- { |
There was a problem hiding this comment.
Shouldn't the path to the root be the empty set?
| for i := depth; i >= 0; i-- { | |
| for i := depth; i > 0; i-- { |
There was a problem hiding this comment.
good catch! the implementation changed as this was one of the bugs
9cf9f54
there's a test specifically for this now too
|
thanks everyone for the great feedback on this! I'll try to incorporate all of them today, I've still been debugging the last e2e on non-interactives defaults |
Description
This PR adds code that generates paths to nmt sub tree roots. While we also have code for this funtionality in the nmt repo, that code returns the paths that could include multiple rows, with out providing info as to which paths belonged to which row (afaict). Changing that was difficult, and requires encoding rsmt2d logic (rows) into nmt, so I decided to rewrite the functionality here given the simplicity of it.
part of #381