feat: add support for subtree root verification #260
Conversation
WalkthroughThe changes introduce methods to compute and verify inner nodes' roots in a Namespaced Merkle Tree (NMT). This includes adding methods for subtree root computation ( Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #260 +/- ##
==========================================
+ Coverage 66.43% 67.84% +1.40%
==========================================
Files 6 6
Lines 1028 1132 +104
==========================================
+ Hits 683 768 +85
- Misses 328 337 +9
- Partials 17 27 +10 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- nmt.go (1 hunks)
- nmt_test.go (2 hunks)
- proof.go (3 hunks)
- proof_test.go (2 hunks)
Additional Context Used
GitHub Check Runs (1)
codecov/patch success (10)
nmt.go: [warning] 663-663: nmt.go#L663
Added line #L663 was not covered by tests
nmt.go: [warning] 667-667: nmt.go#L667
Added line #L667 was not covered by tests
proof.go: [warning] 550-550: proof.go#L550
Added line #L550 was not covered by tests
proof.go: [warning] 554-554: proof.go#L554
Added line #L554 was not covered by tests
proof.go: [warning] 559-559: proof.go#L559
Added line #L559 was not covered by tests
proof.go: [warning] 563-563: proof.go#L563
Added line #L563 was not covered by tests
proof.go: [warning] 575-575: proof.go#L575
Added line #L575 was not covered by tests
proof.go: [warning] 580-580: proof.go#L580
Added line #L580 was not covered by tests
proof.go: [warning] 621-621: proof.go#L621
Added line #L621 was not covered by tests
proof.go: [warning] 626-626: proof.go#L626
Added line #L626 was not covered by tests
Additional comments not posted (14)
nmt.go (2)
677-680: TheisPowerOfTwofunction is implemented correctly and efficiently.
647-675: The implementation ofComputeSubtreeRootmethod looks robust with appropriate checks for tree size, range validity, and subtree correctness. However, ensure that the new logic is covered by unit tests, especially for the error paths.Verification successful
The method
ComputeSubtreeRootis well-covered by unit tests, as evidenced by multiple occurrences innmt_test.goandproof_test.go.
nmt_test.go: Contains a dedicated test functionTestComputeSubtreeRoot.proof_test.go: Contains numerous calls toComputeSubtreeRootwith various parameters.This confirms that the new logic is adequately tested.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the new method `ComputeSubtreeRoot` is covered by unit tests. # Test: Search for the method usage in test files. Expect: At least one occurrence. rg --type python $'ComputeSubtreeRoot'Length of output: 68
Script:
#!/bin/bash # Description: Verify if the new method `ComputeSubtreeRoot` is covered by unit tests. # Test: Search for the method usage in all files. Expect: At least one occurrence. rg 'ComputeSubtreeRoot'Length of output: 2341
proof.go (2)
635-644: Check for negative values insubtreeRootThresholdToLeafRange.The method
subtreeRootThresholdToLeafRangecorrectly handles negative inputs by returning an error. However, it would be beneficial to prevent such calls at the source by ensuring that the input values are validated before being passed to this method.+ // Ensure non-negative values before calling subtreeRootThresholdToLeafRangeLikely invalid or redundant comment.
587-633: Validate assumptions intoLeafRangesmethod.The
toLeafRangesmethod assumes that the tree's number of leaves is a power of two. This assumption is critical for the correct operation of the method. Please ensure that this assumption is always validated before this method is called.nmt_test.go (2)
1193-1218: Review the test cases forisPowerOfTwo.The test cases for
isPowerOfTwoare comprehensive, covering a range of inputs including edge cases like zero and negative numbers. This ensures robust testing of the function's behavior across different scenarios.
1220-1349: Ensure error handling inTestComputeSubtreeRootis comprehensive.The test function
TestComputeSubtreeRootincludes various scenarios, including valid ranges and expected errors. It's crucial to ensure that all potential error conditions in theComputeSubtreeRootmethod are covered by these tests. This verification script checks if all documented error scenarios are indeed tested.Verification successful
The verification script confirms that the
TestComputeSubtreeRootfunction includes tests for error scenarios, as indicated by the presence of multipleexpectError: trueentries.
- : The
TestComputeSubtreeRootfunction comprehensively handles error scenarios.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all error scenarios in `TestComputeSubtreeRoot` are covered by tests. # Test: Search for error handling tests. Expect: Tests that cover all documented error scenarios. rg --type python $'expectError: true' nmt_test.goLength of output: 81
Script:
#!/bin/bash # Description: Verify that all error scenarios in `TestComputeSubtreeRoot` are covered by tests. # Test: Search for error handling tests. Expect: Tests that cover all documented error scenarios. rg --type go 'expectError: true' nmt_test.goLength of output: 156
proof_test.go (8)
6-6: The import of thefmtpackage is appropriate as it is used in the test functions for formatting strings.
1128-1156: The test functionTestLargestPowerOfTwois well-structured and covers a variety of cases, including edge cases like the smallest and largest bounds, and an error scenario when the bound is zero. This thorough testing ensures the robustness of thelargestPowerOfTwofunction.
1158-1180: The test functionTestSubtreeRootThresholdToLeafRangeeffectively checks the conversion from a subtree root threshold to a leaf range. It includes a negative test case which is good for ensuring error handling is correct.
1182-1332: The functionTestToLeafRangesis comprehensive, testing various scenarios to ensure that the leaf ranges are calculated correctly based on different inputs. This includes tests for different sizes of proof ranges and thresholds, and also tests for error conditions such as negative indices.
1346-1437: The functionTestNextLeafRangecorrectly tests the calculation of the next leaf range based on the current range and the maximum leaf range. It includes tests for typical cases as well as edge cases where the current range is at the boundary of the tree. The inclusion of error scenarios where the current end is less than the current start is crucial for robust error handling.
1439-1473: The functionTestSafeIntToUintis crucial for ensuring that the conversion frominttouinthandles negative numbers correctly, as negative integers cannot be represented as unsigned integers. The test cases cover typical conversions and the specific case where the input is negative, ensuring that an appropriate error is returned.
1475-1515: The functionTestMinInteffectively tests the utility functionminIntwith a variety of input values, including positive, negative, and zero values. This ensures that the function correctly identifies the minimum integer, which is essential for its use in calculations involving indices and sizes in the NMT.
1517-1841: The functionTestVerifySubtreeRootInclusionis a comprehensive test for the verification of subtree root inclusions within a Namespaced Merkle Tree. It tests various scenarios including valid proofs for different ranges and invalid conditions such as incorrect subtree root thresholds and malformed roots. This thorough testing is crucial for ensuring the reliability of subtree root verifications in the NMT.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- proof_test.go (2 hunks)
Additional comments not posted (8)
proof_test.go (8)
6-6: Ensure thefmtimport is used in the file.
1129-1157: Review the new test functionTestLargestPowerOfTwo.This test function correctly checks various scenarios for the
largestPowerOfTwofunction, including edge cases like the input being zero, which should correctly return an error.
1159-1181: Review the new test functionTestSubtreeRootThresholdToLeafRange.This test function adequately covers different values for
subtreeRootThreshold, including negative values, ensuring robust error handling.
1183-1333: Review the new test functionTestToLeafRanges.This test function comprehensively tests the
toLeafRangesfunction with a variety of input ranges and thresholds. It includes tests for edge cases and ensures that the function handles errors appropriately.
1347-1438: Review the new test functionTestNextLeafRange.This test function effectively checks the
nextLeafRangefunction across a range of scenarios, ensuring that the function behaves as expected even with edge cases like zero and negative ranges.
1440-1474: Review the new test functionTestSafeIntToUint.This test function correctly handles the conversion from
inttouint, including the scenario where the input is negative, which should appropriately trigger an error.
1476-1516: Review the new test functionTestMinInt.This test function is well-implemented to check the
minIntfunction with various integer inputs, ensuring that it returns the correct minimum value.
1518-1842: Review the new test functionTestVerifySubtreeRootInclusion.This test function is crucial for verifying the inclusion of subtree roots. It is well-structured and tests a variety of scenarios, including edge cases and error conditions, to ensure robustness.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- nmt.go (4 hunks)
- proof.go (3 hunks)
- proof_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- nmt.go
Additional comments not posted (9)
proof.go (1)
722-727: Handle edge cases insafeIntToUint.Consider adding a check for extremely large integers that might cause unexpected behavior when converted to uint.
+ // Consider adding checks for upper bounds if applicableproof_test.go (8)
6-6: Added import of "fmt" package.This import is necessary for the new test functions that use
fmt.Sprintffor dynamic test case naming.
1129-1157: New test functionTestLargestPowerOfTwoadded.This function tests the
largestPowerOfTwoutility function across various bounds. It properly handles both expected results and error conditions, ensuring comprehensive coverage.
1159-1181: New test functionTestSubtreeRootThresholdToLeafRangeadded.This function tests the conversion from a subtree root threshold to a leaf range. It includes tests for both valid and error scenarios, which is good for robustness.
1183-1333: New test functionTestToLeafRangesadded.This function tests the conversion of proof parameters into leaf ranges. It includes a variety of scenarios to ensure the function handles different edge cases and errors correctly.
1347-1438: New test functionTestNextLeafRangeadded.This function tests the calculation of the next leaf range based on current parameters and maximum leaf range. It includes tests for both typical and boundary conditions, ensuring the function's reliability.
1440-1474: New test functionTestSafeIntToUintadded.This function tests the conversion of an integer to an unsigned integer, handling the error scenario where the integer is negative. The tests are well-structured to cover both normal and exceptional cases.
1476-1516: New test functionTestMinIntadded.This function tests finding the minimum of two integers. It covers a range of inputs including positive, negative, and zero values, ensuring the function behaves as expected across these cases.
1518-1842: New test functionTestVerifySubtreeRootInclusionadded.This function tests the verification of subtree root inclusions within a Namespaced Merkle Tree. It includes a variety of test cases to ensure the function handles different scenarios correctly, including error handling for invalid inputs.
| // the range of the leaves matching that namespace ID in the tree | ||
| namespaceRanges map[string]leafRange | ||
| namespaceRanges map[string]LeafRange |
There was a problem hiding this comment.
This exports the LeafRange to be used downstream
proof.go
Outdated
| // - Go back to the loop condition. | ||
| // | ||
| // Note: This method is Celestia specific. | ||
| func ToLeafRanges(proofStart, proofEnd, subtreeRootThreshold int) ([]LeafRange, error) { |
There was a problem hiding this comment.
This is also exported so that we have a way downstream to find the leaf ranges based on a proof start -> end
|
Making this a draft because I want to finish the implementation downstream to be sure this PR contains all the changes needed |
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- nmt.go (5 hunks)
- nmt_test.go (2 hunks)
- proof.go (3 hunks)
- proof_test.go (2 hunks)
Additional context used
GitHub Check: codecov/patch
nmt.go
[warning] 658-658: nmt.go#L658
Added line #L658 was not covered by tests
[warning] 662-662: nmt.go#L662
Added line #L662 was not covered by testsproof.go
[warning] 554-554: proof.go#L554
Added line #L554 was not covered by tests
[warning] 558-558: proof.go#L558
Added line #L558 was not covered by tests
[warning] 563-563: proof.go#L563
Added line #L563 was not covered by tests
[warning] 567-567: proof.go#L567
Added line #L567 was not covered by tests
[warning] 579-579: proof.go#L579
Added line #L579 was not covered by tests
[warning] 584-584: proof.go#L584
Added line #L584 was not covered by tests
[warning] 627-627: proof.go#L627
Added line #L627 was not covered by tests
golangci-lint
nmt_test.go
853-853: undefined: NamespacedMerkleTree
867-867: undefined: NamespacedMerkleTree
63-63: undefined: New
74-74: undefined: MinNamespace
75-75: undefined: MaxNamespace
117-117: undefined: New
132-132: undefined: sum
133-133: undefined: sum
136-136: undefined: sum
155-155: undefined: New
559-559: undefined: NmtHasher
562-562: undefined: NewNmtHasher
670-670: undefined: Proof
821-821: undefined: NamespacedMerkleTree
910-910: undefined: ErrInvalidNodeLen
911-911: undefined: ErrUnorderedSiblings
912-912: undefined: ErrUnorderedSiblings
914-914: undefined: ErrUnorderedSiblings
916-916: undefined: ErrInvalidRange
917-917: undefined: ErrInvalidRange
918-918: undefined: ErrInvalidRange
952-952: undefined: ErrInvalidNodeLen
990-990: undefined: ErrInvalidNodeLen
1171-1171: undefined: NewNmtHasher
Additional comments not posted (15)
nmt.go (4)
593-600: Review the logic for updating namespace ranges.The logic for updating namespace ranges in the
updateNamespaceRangesmethod appears to be correct. It efficiently handles the addition of new leaves to the tree and updates the ranges accordingly. Good use of checking the existence of the namespace key before deciding to create a new range or extend an existing one.
154-154: Initialization ofnamespaceRangesis correct.The initialization of
namespaceRangesusingmake(map[string]LeafRange)is appropriate and ensures that the map is ready to store namespace ID ranges as leaves are added to the tree.
112-112: Export ofLeafRangeis justified.Based on the existing comment from
rach-id, the export ofLeafRangeis necessary for downstream usage. This aligns with the PR's objectives to enhance the library's functionality for broader use cases.
647-670: Ensure proper validation and error handling inComputeSubtreeRoot.The method
ComputeSubtreeRootperforms several checks on the input parametersstartandend. However, there are a few improvements and corrections needed:
- The error message in line 654 should use "strictly" instead of "stricly".
- The method uses
safeIntToUintfor convertingstartandendto unsigned integers, which is good for safety. However, the coverage for these conversions is missing in the tests.Consider adding unit tests to cover these scenarios to ensure robustness, especially for boundary values of
startandend.- return nil, fmt.Errorf("end %d should be stricly bigger than start %d", end, start) + return nil, fmt.Errorf("end %d should be strictly bigger than start %d", end, start)Tools
GitHub Check: codecov/patch
[warning] 658-658: nmt.go#L658
Added line #L658 was not covered by tests
[warning] 662-662: nmt.go#L662
Added line #L662 was not covered by testsproof.go (2)
713-718: Handle edge cases insafeIntToUint.+ // Consider adding checks for upper bounds if applicableThe
safeIntToUintfunction converts an integer to an unsigned integer and checks for negative values. Consider adding a check for extremely large integers that might cause unexpected behavior when converted to uint.Likely invalid or redundant comment.
468-589: Ensure assumptions are documented and validated forVerifySubtreeRootInclusion.This method assumes that the tree's number of leaves is a power of two and that subtree roots are created according to specific rules. It's crucial to ensure these assumptions are validated in the implementation or documented clearly to avoid misuse.
Verification successful
The test function
TestVerifySubtreeRootInclusioninproof_test.goincludes various test cases that validate different aspects of theVerifySubtreeRootInclusionmethod. These test cases cover scenarios such as invalid subtree root widths, invalid number of subtree roots, invalid root formats, invalid proof ranges, and invalid proof nodes. This indicates that the assumptions and edge cases are being tested.
- The test cases ensure that the subtree roots are created according to the specified rules.
- The test cases also cover scenarios where the number of leaves is a power of two, as well as invalid cases.
Thus, the assumptions documented in the
VerifySubtreeRootInclusionmethod are validated through these comprehensive test cases.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the assumptions stated in the comments are respected throughout the codebase. # Test: Search for usages of `VerifySubtreeRootInclusion` and check if assumptions are validated. ast-grep --lang go --pattern $'func $_($_, $_, $_, $_) { $$$ VerifySubtreeRootInclusion($_, $_, $_, $_) $$$ }'Length of output: 117
Script:
#!/bin/bash # Description: Search for all occurrences of `VerifySubtreeRootInclusion` in the codebase and inspect the context. # Find all occurrences of `VerifySubtreeRootInclusion` in the codebase rg 'VerifySubtreeRootInclusion' -A 5 -B 5Length of output: 2263
Script:
#!/bin/bash # Description: Extract the full `TestVerifySubtreeRootInclusion` function for detailed inspection. # Extract the `TestVerifySubtreeRootInclusion` function from `proof_test.go` ast-grep --lang go --pattern $'func TestVerifySubtreeRootInclusion(t *testing.T) { $$$ }' proof_test.goLength of output: 14820
Tools
GitHub Check: codecov/patch
[warning] 554-554: proof.go#L554
Added line #L554 was not covered by tests
[warning] 558-558: proof.go#L558
Added line #L558 was not covered by tests
[warning] 563-563: proof.go#L563
Added line #L563 was not covered by tests
[warning] 567-567: proof.go#L567
Added line #L567 was not covered by tests
[warning] 579-579: proof.go#L579
Added line #L579 was not covered by tests
[warning] 584-584: proof.go#L584
Added line #L584 was not covered by testsnmt_test.go (2)
865-877: Add documentation forexampleNMT2.The function
exampleNMT2lacks a docstring. It's important to document the purpose and behavior of this function, especially how it differs fromexampleNMTand in what contexts it should be used.Tools
golangci-lint
867-867: undefined: NamespacedMerkleTree
1193-1322: Ensure correct handling of edge cases inTestComputeSubtreeRoot.Verification successful
The
TestComputeSubtreeRootfunction innmt_test.goincludes several test cases designed to handle various edge cases. Here are the key points:
- Valid Ranges: Tests with valid ranges (e.g.,
[0, 16),[0, 8),[8, 16), etc.) ensure that the subtree roots are correctly computed.- Invalid Ranges: Tests with invalid ranges (e.g.,
[-1, 4),[4, 4),[5, 4)) expect errors, verifying that the function handles these edge cases properly.- Non-Power-of-2 Leaf Counts: A test case with a tree whose leaves are not a power of 2 (e.g.,
[0, 1, 2, 3, 4]) expects an error, ensuring the function correctly identifies this condition.The test cases appear comprehensive and cover a wide range of edge cases, ensuring robust handling of different scenarios.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of edge cases in `TestComputeSubtreeRoot`. # Test: Search for the function usage. Expect: Proper handling of edge cases. rg --type python $'TestComputeSubtreeRoot'Length of output: 72
Script:
#!/bin/bash # Description: Verify the handling of edge cases in `TestComputeSubtreeRoot`. # Test: Search for the function usage. Expect: Proper handling of edge cases. rg --type go 'TestComputeSubtreeRoot'Length of output: 93
proof_test.go (7)
6-6: Ensure thefmtimport is utilized effectively.The import of the
fmtpackage is justified as it is used in various test functions for formatting strings.
1129-1157: Review the new test functionTestLargestPowerOfTwo.This test function correctly checks the behavior of the
largestPowerOfTwofunction across a range of inputs, including edge cases like the smallest and largest inputs, and an error scenario when the input is zero.
1159-1300: Review the new test functionTestToLeafRanges.The function
TestToLeafRangeseffectively tests theToLeafRangesfunction with a comprehensive set of inputs, ensuring that it handles various sizes and conditions correctly. The use of dynamic test generation withfmt.Sprintffor test names enhances readability and maintainability.
1314-1408: Review the new test functionTestNextLeafRange.
TestNextLeafRangeprovides a thorough examination of thenextLeafRangefunction, covering a wide range of scenarios including valid ranges, edge cases, and error conditions. The structured approach in defining test cases ensures that all critical paths are covered.
1410-1443: Review the new test functionTestSafeIntToUint.The test function
TestSafeIntToUintis well-designed to check the conversion frominttouint, including an error scenario for negative integers. The detailed assertions ensure that both the return value and error states are correctly verified.
1446-1485: Review the new test functionTestMinInt.
TestMinInteffectively tests theminIntfunction with various combinations of positive, negative, and zero values. The test cases are well-structured and cover all necessary scenarios to ensure the function behaves as expected under different conditions.
1488-1814: Review the new test functionTestVerifySubtreeRootInclusion.This test function thoroughly evaluates the
VerifySubtreeRootInclusionmethod across a range of scenarios, including valid proofs, proofs with incorrect subtree roots, and proofs with invalid parameters. The comprehensive testing ensures robust verification of subtree root inclusions in the NMT.
proof.go
Outdated
| // reached a leaf | ||
| if end-start == 1 { | ||
| // if the leaf index falls within the proof range, pop and return a | ||
| // leaf | ||
| if proof.Start() <= start && start < proof.End() { | ||
| // this check should always be the case. | ||
| // however, it is added to avoid nil pointer exceptions | ||
| if len(ranges) != 0 { | ||
| // advance the list of ranges | ||
| ranges = ranges[1:] | ||
| } | ||
| // advance leafHashes | ||
| return popIfNonEmpty(&subtreeRoots), nil | ||
| } | ||
|
|
||
| // if the leaf index is outside the proof range, pop and return a | ||
| // proof node (which in this case is a leaf) if present, else return | ||
| // nil because leaf doesn't exist | ||
| return popIfNonEmpty(&proof.nodes), nil | ||
| } | ||
|
|
||
| // if the current range does not overlap with the proof range, pop and | ||
| // return a proof node if present, else return nil because subtree | ||
| // doesn't exist | ||
| if end <= proof.Start() || start >= proof.End() { | ||
| return popIfNonEmpty(&proof.nodes), nil | ||
| } | ||
|
|
||
| if len(ranges) != 0 && ranges[0].Start == start && ranges[0].End == end { | ||
| ranges = ranges[1:] | ||
| return popIfNonEmpty(&subtreeRoots), nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Note:
This could be refactored to be this:
// if the current range has a subtree root, then we use it
if len(ranges) != 0 && ranges[0].Start == start && ranges[0].End == end {
ranges = ranges[1:]
return popIfNonEmpty(&subtreeRoots), nil
}
// reached a leaf
if end-start == 1 {
// if the leaf index is outside the proof range, pop and return a
// proof node (which in this case is a leaf) if present, else return
// nil because leaf doesn't exist
return popIfNonEmpty(&proof.nodes), nil
}
// if the current range does not overlap with the proof range, pop and
// return a proof node if present, else return nil because subtree
// doesn't exist
if end <= proof.Start() || start >= proof.End() {
return popIfNonEmpty(&proof.nodes), nil
}
and I tried testing it and it works fine. However, I would like a second confirmation that nothing will break with this refactor
There was a problem hiding this comment.
Looks correct to me, too. Would still be good to closely look into the case if end-start == 1 and why both related code blocks are equivalent in all (edge) cases.
There was a problem hiding this comment.
I'd even say you can revise the refactored version further (as suggested in this comment as well), and it should work:
// if the current range has a subtree root, then we use it
if len(ranges) != 0 && ranges[0].Start == start && ranges[0].End == end {
ranges = ranges[1:]
return popIfNonEmpty(&subtreeRoots), nil
}
// if the current range does not overlap with the proof range, pop and
// return a proof node if present, else return nil because subtree
// doesn't exist
if end <= proof.Start() || start >= proof.End() {
return popIfNonEmpty(&proof.nodes), nil
}
Rationale: When traversing the tree from top (root) to bottom (leaves), we split the range covered by the root to half, and for each half i.e., subtree there can be 3 cases:
- The subtree range exactly matches one of the supplied subtree roots -> hence return that subtree root
- The subtree range overlaps with the proof range but does not exactly match the range of any of the subtree roots yet -> keep splitting that subtree
- The subtree range is entirely outside of the proof range -> there needs to be a node for that in the proof, hence pop and return it
Further clarification: In the original version as well as your refactored version, the if statement of end-start == 1 was there to cover the case 1 of above. Since in the original version, the func would accept leaf hashes (not subtree roots), and the range covered by each leaf hash by definition is 1 i.e., end-start=1.
There was a problem hiding this comment.
Nevertheless, even if you want to keep your refactored version with the additional if end-start == 1 { block in it, I won't oppose it, though; one thing you could do is to refactor it slightly differently:
// if the current range has a subtree root, then we use it
if len(ranges) != 0 && ranges[0].Start == start && ranges[0].End == end {
ranges = ranges[1:]
return popIfNonEmpty(&subtreeRoots), nil
}
// if the current range does not overlap with the proof range, pop and
// return a proof node if present, else return nil because subtree
// doesn't exist
if end <= proof.Start() || start >= proof.End() {
return popIfNonEmpty(&proof.nodes), nil
}
// reached a leaf
if end-start == 1 {
// if the leaf index is outside the proof range, pop and return a
// proof node (which in this case is a leaf) if present, else return
// nil because leaf doesn't exist
return popIfNonEmpty(&proof.nodes), nil
}
In this new version, I doubt we can find any input that can make that if end-start == 1 block to be executed at all. All the inputs will enter either the first if or the second one, but never the third (the second if covers the third one already).
There was a problem hiding this comment.
One more point, in the refactored version you shared, i.e, this if specifically
// if the current range has a subtree root, then we use it
if len(ranges) != 0 && ranges[0].Start == start && ranges[0].End == end {
ranges = ranges[1:]
return popIfNonEmpty(&subtreeRoots), nil
}
I think it needs to be revised. First we need to check whether the proof range proof.Start(),proof.End() contains the current range i.e., start, end, and if it was the case then check if if len(ranges) != 0 && ranges[0].Start == start && ranges[0].End == end , and if this was the case, we return a subtreeRoot, otherwise, this is an error and an error needs to be returned.
There was a problem hiding this comment.
@staheri14 Why is it an error? if I understand correctly, if we have for example the proof for the range [0, 8) in a [0,15] tree.
We could be at level [4,8), for example, and this range is contained in the proof range, however, there is no subtree root for that, given the subtree root width is 1.
Separately, I removed the whole block:
if end-start == 1 {
// if the leaf index falls within the proof range, pop and return a
// leaf
if proof.Start() <= start && start < proof.End() {
// this check should always be the case.
// however, it is added to avoid nil pointer exceptions
if len(ranges) != 0 {
// advance the list of ranges
ranges = ranges[1:]
}
// advance leafHashes
return popIfNonEmpty(&subtreeRoots), nil
}
// if the leaf index is outside the proof range, pop and return a
// proof node (which in this case is a leaf) if present, else return
// nil because leaf doesn't exist
return popIfNonEmpty(&proof.nodes), nil
}and ran the all combinations test and it works fine. So probably we can remove it no problem. And we end up with:
computeRoot = func(start, end int) ([]byte, error) {
if len(ranges) != 0 && ranges[0].Start == start && ranges[0].End == end {
ranges = ranges[1:]
return popIfNonEmpty(&subtreeRoots), nil
}
// if the current range does not overlap with the proof range, pop and
// return a proof node if present, else return nil because subtree
// doesn't exist
if end <= proof.Start() || start >= proof.End() {
return popIfNonEmpty(&proof.nodes), nil
}
// Recursively get left and right subtree
k := getSplitPoint(end - start)
left, err := computeRoot(start, start+k)
if err != nil {
return nil, fmt.Errorf("failed to compute subtree root [%d, %d): %w", start, start+k, err)
}
right, err := computeRoot(start+k, end)
if err != nil {
return nil, fmt.Errorf("failed to compute subtree root [%d, %d): %w", start+k, end, err)
}
// only right leaf/subtree can be non-existent
if right == nil {
return left, nil
}
hash, err := nth.HashNode(left, right)
if err != nil {
return nil, fmt.Errorf("failed to hash node: %w", err)
}
return hash, nil
}Also, we can remove the power of 2 constraint on the tree size.
if you agree, I'll go ahead and commit these changes.
There was a problem hiding this comment.
@staheri14 Why is it an error? if I understand correctly, if we have for example the proof for the range [0, 8) in a [0,15] tree.
We could be at level [4,8), for example, and this range is contained in the proof range, however, there is no subtree root for that, given the subtree root width is 1.
To clarify my comments: in your example, if we are at range [0,8) that is a subrange of the proof range but there is no subtree root left i.e., len(ranges) == 0, then that is an error and we should return an error. Right now, I think it is not captured in the code. Essentially, my suggestion (in pseudocode) would be: (I've additionally included another if statement to capture the case where there is a missing range in the subtreeRoots, I also included some inline comments about where we should return error, but it is not the case currently)
if the current range i.e., [start,End) falls within the proof range {
if len(ranges) == 0 {
// there should be some range available, but none exists, hence return err here
}
if ranges[0].Start == start && ranges[0].End == end {
ranges = ranges[1:]
// below, i.e., when we `popIfNonEmpty`, I also think that we should return an error as there is no subtree root available (but must exist), unless we expect proof range i.e., proof.end exceeds the tree size which I think would not be the case at all
return popIfNonEmpty(&subtreeRoots), nil
}
if end-start == 1 {
// we reached a leaf, there is nothing further to go down, however, there has been no matching range in the subtreeRoots, this is an error and we should return an error here
}
}There was a problem hiding this comment.
I also added another comment which also applies to your new suggested code block.
| } | ||
|
|
||
| func TestVerifySubtreeRootInclusion(t *testing.T) { | ||
| tree := exampleNMT(1, true, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15) |
There was a problem hiding this comment.
Can we add in a test for a tree that isn't a power of two?
As this is all celestia specific, should we test with a subtreeWidth that follows adr13?
There was a problem hiding this comment.
Can we add in a test for a tree that isn't a power of two?
The algorithm assumes a perfect tree... It won't work with a tree that isn't a power of 2
As this is all celestia specific, should we test with a subtreeWidth that follows adr13?
that's already the case if I understand correctly. we're using subtree width inspired from adr13
There was a problem hiding this comment.
It won't work
As in it panics or errors?
There was a problem hiding this comment.
Definitely no panics. But can either error, or return a false positive/negative.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- proof.go (3 hunks)
Additional context used
GitHub Check: codecov/patch
proof.go
[warning] 552-552: proof.go#L552
Added line #L552 was not covered by tests
[warning] 556-556: proof.go#L556
Added line #L556 was not covered by tests
[warning] 561-561: proof.go#L561
Added line #L561 was not covered by tests
[warning] 565-565: proof.go#L565
Added line #L565 was not covered by tests
[warning] 577-577: proof.go#L577
Added line #L577 was not covered by tests
[warning] 582-582: proof.go#L582
Added line #L582 was not covered by tests
[warning] 625-625: proof.go#L625
Added line #L625 was not covered by tests
Additional comments not posted (3)
proof.go (3)
468-587: Review and Recommendations forVerifySubtreeRootInclusionMethodThe
VerifySubtreeRootInclusionmethod is a critical addition designed for verifying the inclusion of subtree roots within a Namespaced Merkle Tree (NMT). It's essential for the Celestia project as per the specifications in ADR-013. Here are some observations and recommendations:
Validation of Proof Range:
The method correctly checks for the validity of the proof range (lines 478-481). This is a crucial check to ensure that the proof operations are conducted within valid bounds.Validation of Root and Nodes:
The method validates the format of the root and each node in the proof (lines 483-491). This ensures that all components involved in the verification process conform to the expected hash format, which is vital for the integrity of the proof verification.Validation of Subtree Roots:
Each subtree root is validated against the NMT hasher's format (lines 493-497). This step is essential to ensure that the subtree roots, which are critical for the proof, are in the correct format.Leaf Ranges Calculation:
The method calculates the leaf ranges for the given proof start and end, and subtree width (lines 500-504). This calculation is pivotal for aligning the subtree roots with the corresponding leaf indices.Matching of Ranges and Roots:
It checks if the number of calculated leaf ranges matches the number of provided subtree roots (lines 506-510). This check is crucial for ensuring that there is a one-to-one correspondence between the ranges and the roots, which is necessary for a valid proof.Recursive Root Computation:
The method uses a recursive approach to compute the root of the tree based on the subtree roots and the proof nodes (lines 512-584). This recursive computation is central to the method's functionality, allowing it to build the tree root from the bottom up and verify it against the provided root.Recommendations:
- Ensure comprehensive testing, especially for edge cases and error handling scenarios, to maintain the robustness and reliability of this method.
- Consider adding more inline comments explaining the logic behind key operations, especially the recursive root computation, to improve maintainability and ease of understanding for future developers.
Overall, the method is well-structured and adheres to the specifications. However, thorough testing and enhanced documentation would be beneficial.
Tools
GitHub Check: codecov/patch
[warning] 552-552: proof.go#L552
Added line #L552 was not covered by tests
[warning] 556-556: proof.go#L556
Added line #L556 was not covered by tests
[warning] 561-561: proof.go#L561
Added line #L561 was not covered by tests
[warning] 565-565: proof.go#L565
Added line #L565 was not covered by tests
[warning] 577-577: proof.go#L577
Added line #L577 was not covered by tests
[warning] 582-582: proof.go#L582
Added line #L582 was not covered by tests
589-632: Review ofToLeafRangesFunctionThe
ToLeafRangesfunction is designed to calculate leaf ranges based on the provided subtree width. This function is crucial for determining the segments of the tree that correspond to the subtree roots. Here are some observations:
Input Validation:
The function performs essential input validation (lines 609-616), ensuring that the start is not negative, the end is greater than the start, and the subtree width is positive. These checks are vital for preventing erroneous calculations and potential runtime errors.Range Calculation:
The function calculates the leaf ranges in a loop (lines 618-630), which iteratively determines the range for each subtree root until the entire range is covered. This approach is efficient and aligns with the method's purpose.Recommendation:
- Enhance the error messages to provide more context about the nature of the input errors, which can aid in debugging and error resolution.
Overall, the function is well-implemented and serves its purpose effectively. Enhancing the clarity of error messages would further improve its usability.
Tools
GitHub Check: codecov/patch
[warning] 625-625: proof.go#L625
Added line #L625 was not covered by tests
634-652: Review ofnextLeafRangeFunctionThe
nextLeafRangefunction calculates the range for a subtree root within a specified segment of the tree. This function is a key component in determining the structure of the tree based on the subtree roots. Observations:
Range Calculation Logic:
The function uses theminIntandsafeIntToUintfunctions to determine the minimum range that can be covered by a subtree root (lines 641-644). It then calculates the largest power of two within this range (lines 647-650). This logic is crucial for aligning the subtree roots with the powers of two, which is a requirement in many tree-based data structures.Error Handling:
The function has robust error handling, which ensures that any issues during the range calculation are properly reported and can be handled by the caller (lines 645, 649).Recommendation:
- The function is well-designed and effectively implements the required functionality. Ensuring comprehensive test coverage, particularly for edge cases, would be beneficial to validate the range calculation logic under various scenarios.
Overall, the function performs its intended task effectively and adheres to the requirements.
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- proof.go (3 hunks)
Additional context used
GitHub Check: codecov/patch
proof.go
[warning] 552-552: proof.go#L552
Added line #L552 was not covered by tests
[warning] 556-556: proof.go#L556
Added line #L556 was not covered by tests
[warning] 561-561: proof.go#L561
Added line #L561 was not covered by tests
[warning] 565-565: proof.go#L565
Added line #L565 was not covered by tests
[warning] 577-577: proof.go#L577
Added line #L577 was not covered by tests
[warning] 582-582: proof.go#L582
Added line #L582 was not covered by tests
[warning] 626-626: proof.go#L626
Added line #L626 was not covered by tests
Additional comments not posted (1)
proof.go (1)
719-724: Simple Utility FunctionminIntApprovedThis function is implemented correctly and efficiently.
proof.go
Outdated
| if end-start == 1 { | ||
| // if the leaf index falls within the proof range, pop and return a | ||
| // leaf | ||
| if proof.Start() <= start && start < proof.End() { | ||
| // this check should always be the case. | ||
| // however, it is added to avoid nil pointer exceptions | ||
| if len(ranges) != 0 { | ||
| // advance the list of ranges | ||
| ranges = ranges[1:] | ||
| } | ||
| // advance leafHashes | ||
| return popIfNonEmpty(&subtreeRoots), nil | ||
| } | ||
|
|
||
| // if the leaf index is outside the proof range, pop and return a | ||
| // proof node (which in this case is a leaf) if present, else return | ||
| // nil because leaf doesn't exist | ||
| return popIfNonEmpty(&proof.nodes), nil | ||
| } |
There was a problem hiding this comment.
It appears to me that all these lines can be removed since the the same logic is covered after this if block.
| if end-start == 1 { | |
| // if the leaf index falls within the proof range, pop and return a | |
| // leaf | |
| if proof.Start() <= start && start < proof.End() { | |
| // this check should always be the case. | |
| // however, it is added to avoid nil pointer exceptions | |
| if len(ranges) != 0 { | |
| // advance the list of ranges | |
| ranges = ranges[1:] | |
| } | |
| // advance leafHashes | |
| return popIfNonEmpty(&subtreeRoots), nil | |
| } | |
| // if the leaf index is outside the proof range, pop and return a | |
| // proof node (which in this case is a leaf) if present, else return | |
| // nil because leaf doesn't exist | |
| return popIfNonEmpty(&proof.nodes), nil | |
| } |
There was a problem hiding this comment.
this is related to #260 (comment) and #260 (comment) and #263.
I am not fully convinced all edge cases are caught if we refactor this code to be similar to what the above comments suggest. If you can confirm that, I'll remove
There was a problem hiding this comment.
I have added my comment here with the rationale of why it works #260 (comment)
On the paper and theoretically, I don't see why it wouldn't cover all corner cases. However, it is best to test it comprehensively to confirm.
| } | ||
|
|
||
| // only right leaf/subtree can be non-existent | ||
| if right == nil { |
There was a problem hiding this comment.
This if statement makes me think that you are considering a non-full tree, right?
If this is the case, then we need to also clarify how a proof looks like for a non-full tree. Ideally, the proof.nodes should contain a node for such an empty right subtree, where the value of the node would be a nested hash of an empty leaf (depending on the level of the node), right?
If this if statement ever gets executed, then I suggest providing a test case that can make this if statement to be executed (demonstrating that this is a possible condition).
Lastly, if we are covering a non-full tree, then I think there is no need to assume that the tree size is a power of two. We could demonstrate it (that the func can handle other sizes of tree) by a test case as well.
There was a problem hiding this comment.
check this: #260 (comment)
That test case goes over trees that are not a power of 2 and tests all combinations and everything passes. However, I am not sure all edge cases are covered/the test really is testing all combinations.
If we are sure that we can handle all type of trees, then we can remove the comment. Otherwise, we can look into removing this part of code
There was a problem hiding this comment.
As I mentioned previously,
This if statement makes me think that you are considering a non-full tree, right?
If this is the case, then we need to also clarify how a proof looks like for a non-full tree. Ideally, the proof.nodes should contain a node for such an empty right subtree, where the value of the node would be a nested hash of an empty leaf (depending on the level of the node), right?
If the aim is to cover cases that the rightmost side of the tree is empty, then we first need to decide on how a proof for such a tree looks like. There are two cases here:
-
We want to allow proofs for non-full trees:
To me, even when the tree is not full, still the hash of nil values should be returned hence the can never be a nil node (i.e., nil left or right subtree), there should always be a hash value (hence we should never expectright==nil). -
But if we do not want to cover non-full trees, then, again there would not be any empty right or left subtree root.
The last point: Nevertheless, the right in this if statement is relative, and does not necessarily mean the right most part of the tree, rather it is a right subtree of any node in the tree.
There was a problem hiding this comment.
@staheri14
Concerning this:
Ideally, the proof.nodes should contain a node for such an empty right subtree, where the value of the node would be a nested hash of an empty leaf (depending on the level of the node), right?
Why is that?
we need to also clarify how a proof looks like for a non-full tree
the same way it is for an RFC 6962, no? the inner nodes required to compute the root. Are there any special cases that you think would need to be specified?
To me, even when the tree is not full, still the hash of nil values should be returned hence the can never be a nil node (i.e., nil left or right subtree), there should always be a hash value (hence we should never expect right==nil).
That's not how it works currently. In non-full trees, we return a nil when we encounter a non-existing right node. Do you mean we should change this behaviour also for the current implementation?
To sum-up, I removed the:
if end-start == 1 {
// if the leaf index falls within the proof range, pop and return a
// leaf
if proof.Start() <= start && start < proof.End() {
// this check should always be the case.
// however, it is added to avoid nil pointer exceptions
if len(ranges) != 0 {
// advance the list of ranges
ranges = ranges[1:]
}
// advance leafHashes
return popIfNonEmpty(&subtreeRoots), nil
}
// if the leaf index is outside the proof range, pop and return a
// proof node (which in this case is a leaf) if present, else return
// nil because leaf doesn't exist
return popIfNonEmpty(&proof.nodes), nil
}
block entirely and everything seems to work.
What else is blocking to the merging of this PR? so that we get ahead and audit the change as it's needed in Celestia-node.
There was a problem hiding this comment.
Some of the changes discussed in this thread are here: 70bc2b0 , let's merge it now and follow up if needed
There was a problem hiding this comment.
If there is something left to discuss, let's do it in separate issues 👍
| end: 16, | ||
| tree: n, | ||
| expectedRoot: func() []byte { | ||
| root, err := n.Root() |
There was a problem hiding this comment.
This updates the state of the original tree n, can we create another instance of the tree instead similar to other test cases i.e.,
exampleNMT2(1, true, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15).Root()
There was a problem hiding this comment.
what's the problem with updating the original tree with a root in a test? that would make the other tests not recalculate it which is good, no?
nmt_test.go
Outdated
| start: 0, | ||
| end: 16, | ||
| tree: func() *NamespacedMerkleTree { | ||
| return exampleNMT2(1, true, 0, 1, 2, 3, 4) // tree leaves are not a power of 2 | ||
| }(), | ||
| expectError: true, |
There was a problem hiding this comment.
The reason that this test case errors out is that the the range of the proof is larger than the size of the tree, hence, the comment tree leaves are not a power of 2 does not indicate why an error is expected, right?
To my understanding, the correct behaviour of this specific function i.e, ComputeSubtreeRoot does not require the tree to be power of two.
There was a problem hiding this comment.
depends on: #260 (comment)
| // Warning: This method is Celestia specific! Using it without verifying | ||
| // the following assumptions, can return unexpected errors, false positive/negatives: | ||
| // - The subtree roots are created according to the ADR-013 | ||
| // https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md |
There was a problem hiding this comment.
It would be easier to understand this function if we didn't need to refer to ADR13 (we could still keep a reference to it but not requiring people to fully read it) and instead explained the relationship between the inputs of this func based on that ADR. From my pov, below are the requirements the function needs to specify (though, you may need to extend them):
subtreeWidthis a power of two and cannot exceed the tree size for which the proof is generated, nor can it exceed the proof'send-startrange.- The proof's
startindex must be a multiple ofsubtreeWidth. subtreeRootsare the MMR picks of the tree starting from the proof'sstartindex. Each node insubtreeRootshas a range of at mostsubtreeWidth.
There was a problem hiding this comment.
also depends on: #260 (comment)
There was a problem hiding this comment.
Actionable comments posted: 13
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- nmt.go (5 hunks)
- proof.go (3 hunks)
Additional context used
GitHub Check: codecov/patch
nmt.go
[warning] 659-659: nmt.go#L659
Added line #L659 was not covered by tests
[warning] 663-663: nmt.go#L663
Added line #L663 was not covered by testsproof.go
[warning] 552-552: proof.go#L552
Added line #L552 was not covered by tests
[warning] 556-556: proof.go#L556
Added line #L556 was not covered by tests
[warning] 561-561: proof.go#L561
Added line #L561 was not covered by tests
[warning] 565-565: proof.go#L565
Added line #L565 was not covered by tests
[warning] 577-577: proof.go#L577
Added line #L577 was not covered by tests
[warning] 582-582: proof.go#L582
Added line #L582 was not covered by tests
[warning] 626-626: proof.go#L626
Added line #L626 was not covered by tests
Additional comments not posted (5)
nmt.go (2)
593-600: EnsureLeafRangeis used correctly.The changes involve renaming
leafRangetoLeafRange. Ensure that the new typeLeafRangeis correctly defined and used in the rest of the codebase.
647-650: Document the inclusivity of theendparameter.It would be beneficial to explicitly state whether the
endparameter is inclusive or exclusive in the function documentation.- // The provided range, defined by start and end, is end-exclusive. + // The provided range, defined by start (inclusive) and end (exclusive), is end-exclusive.proof.go (3)
719-724: LGTM!The
minIntfunction is straightforward and does not require changes.
478-481: Combine validation checks for better readability.The validation checks for the proof range can be combined to improve readability.
- if proof.Start() < 0 || proof.Start() >= proof.End() { + if proof.Start() < 0 || proof.Start() >= proof.End() { + return false, fmt.Errorf("proof range [proof.start=%d, proof.end=%d) is not valid: %w", proof.Start(), proof.End(), ErrInvalidRange) + }Likely invalid or redundant comment.
500-504: Add context to error message.The error message should include more context about the failure.
- return false, fmt.Errorf("failed to compute leaf ranges: %w", err) + return false, fmt.Errorf("failed to compute leaf ranges for proof range [%d, %d): %w", proof.Start(), proof.End(), err)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- nmt.go (5 hunks)
- proof.go (3 hunks)
Additional context used
GitHub Check: codecov/patch
nmt.go
[warning] 659-659: nmt.go#L659
Added line #L659 was not covered by tests
[warning] 663-663: nmt.go#L663
Added line #L663 was not covered by testsproof.go
[warning] 552-552: proof.go#L552
Added line #L552 was not covered by tests
[warning] 556-556: proof.go#L556
Added line #L556 was not covered by tests
[warning] 561-561: proof.go#L561
Added line #L561 was not covered by tests
[warning] 565-565: proof.go#L565
Added line #L565 was not covered by tests
[warning] 577-577: proof.go#L577
Added line #L577 was not covered by tests
[warning] 582-582: proof.go#L582
Added line #L582 was not covered by tests
[warning] 626-626: proof.go#L626
Added line #L626 was not covered by tests
Additional comments not posted (3)
nmt.go (2)
673-678: LGTM!The
LeafRangetype definition and comments are clear and well-documented.
667-669: Verify thenextSubtreeSizefunction usage.Ensure that the
nextSubtreeSizefunction is adequately tested to confirm its behavior aligns with the expected logic for subtree size calculation.</blockquote></details> <details> <summary>proof.go (1)</summary><blockquote> `12-12`: **Import statement for `pb` package is correct.** The import statement for the `pb` package looks good. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 9
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- proof.go (3 hunks)
Additional context used
GitHub Check: codecov/patch
proof.go
[warning] 552-552: proof.go#L552
Added line #L552 was not covered by tests
[warning] 556-556: proof.go#L556
Added line #L556 was not covered by tests
[warning] 561-561: proof.go#L561
Added line #L561 was not covered by tests
[warning] 565-565: proof.go#L565
Added line #L565 was not covered by tests
[warning] 577-577: proof.go#L577
Added line #L577 was not covered by tests
[warning] 582-582: proof.go#L582
Added line #L582 was not covered by tests
[warning] 626-626: proof.go#L626
Added line #L626 was not covered by tests
Additional comments not posted (1)
proof.go (1)
644-649: Combine validation checks for better readability.The validation checks for the input parameters can be combined to improve readability.
- minimum := minInt(currentLeafRange, subtreeWidth) - uMinimum, err := safeIntToUint(minimum) - if err != nil { - return LeafRange{}, fmt.Errorf("failed to convert subtree root range to Uint %w", err) + uMinimum, err := safeIntToUint(minInt(currentLeafRange, subtreeWidth)) + if err != nil { + return LeafRange{}, fmt.Errorf("failed to convert subtree root range to Uint: %w", err) }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- nmt_test.go (2 hunks)
- proof.go (3 hunks)
- proof_test.go (2 hunks)
Additional comments not posted (8)
proof.go (1)
697-702: Enhance Robustness ofsafeIntToUint.Consider adding checks for upper bounds to prevent potential integer overflow issues, especially in environments where
intanduintsizes may differ.+ if val < 0 || val > int(^uint(0)>>1) { + return 0, fmt.Errorf("cannot convert int %d to uint", val) }Likely invalid or redundant comment.
proof_test.go (7)
1129-1157: Review of TestLargestPowerOfTwo:The function
TestLargestPowerOfTwocorrectly tests various boundary conditions for the functionlargestPowerOfTwo. It includes a test for the error condition when the input is zero, which is expected to throw an error. The structure of the test cases is clear and covers a good range of inputs.
1159-1300: Review of TestToLeafRanges:The function
TestToLeafRangesis well-structured and tests the conversion of proof start and end indices into leaf ranges based on a given subtree width. The dynamic generation of expected ranges using functions within test cases is a good practice for complex scenarios. The test also correctly handles error conditions when input values are negative or zero, which should not be valid for subtree widths and indices.
1302-1312: Review of compareRanges function:The helper function
compareRangesis used in theTestToLeafRangesfunction to compare expected and actual results. It checks both the length and individual elements of the slice, which is appropriate for ensuring that the entire range of leaves is correctly calculated. This function is simple and effective for its intended use.
1314-1408: Review of TestNextLeafRange:
TestNextLeafRangetests the calculation of the next leaf range given a current range and a maximum leaf range. It includes a variety of test cases that cover different scenarios, including edge cases where the current end is less than the current start, which correctly triggers an error. The function handles these cases well and the tests are comprehensive.
1410-1443: Review of TestSafeIntToUint:This test function checks the conversion from
inttouint, which is a critical operation given thatuintcannot represent negative numbers. The functionsafeIntToUintis tested against various inputs, including negative numbers, to ensure it returns an error as expected. The test cases are well-structured and effectively validate the functionality.
1446-1485: Review of TestMinInt:
TestMinInteffectively tests theminIntfunction, which calculates the minimum of two integers. The test cases cover a variety of inputs, including negative and zero values. The function is simple and the tests confirm that it behaves as expected across different scenarios.
1488-1830: Review of TestVerifySubtreeRootInclusion:The
TestVerifySubtreeRootInclusionfunction tests the verification of subtree root inclusion within a larger tree. It uses a variety of subtree widths and configurations to ensure robust testing. The function handles both valid and error scenarios effectively. The tests are comprehensive and cover scenarios with different numbers of subtree roots and various proof configurations. This is crucial for ensuring the integrity of the verification process in the NMT.
| // Also, it requires the start and end range to correctly reference an inner node. | ||
| // The provided range, defined by start and end, is end-exclusive. |
There was a problem hiding this comment.
It's not clear here how the start and end could reference an inner node. Shouldn't it be leaves?
I would just say [start, end) as it's more succinct.
Overview
Closes #256
I added this change here so that we have a reference implementation of the algorithm that we will implement in Solidity.
Also, adds a method to generate the subtree roots, which didn't exist before and will be needed during proof generation in Celestia-node.
The codecov missing coverage complaints are for conditions that are checked twice in different contexts. So there is no way to bypass the first check to arrive at the second check. So, I guess they're fine.
Summary by CodeRabbit
New Features
Tests