-
Notifications
You must be signed in to change notification settings - Fork 311
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
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 <[email protected]>
Co-authored-by: Rootul Patel <[email protected]>
Co-authored-by: Rootul Patel <[email protected]>
Co-authored-by: Rootul Patel <[email protected]>
…iaorg/celestia-app into evan/nmt-subroot-path-gen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first pass, looks good. But, I will still need to give it more time to understand.
} | ||
} | ||
|
||
// canClimbRight uses the current position to calculate the direction of the next |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Great work 🚀
} | ||
|
||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably would be better to add to the docs that we expect canClimbRight()
as a precondition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we also check if depth != 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we add a test for this one? Seems standalone and can be tested easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great work
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