Skip to content
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 support for subtree root verification #260

Merged
merged 17 commits into from
Jul 16, 2024
Merged

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented May 29, 2024

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

    • Added subtree root computation functionality to the Namespaced Merkle Tree (NMT).
    • Introduced new validation methods for subtree root inclusion in NMT.
  • Tests

    • Added comprehensive tests for subtree root computation and verification in the Namespaced Merkle Tree.
    • Introduced helper functions for enhanced verification capabilities.
    • Added edge case handling for various scenarios in the NMT proof system.

@rach-id rach-id added the enhancement New feature or request label May 29, 2024
@rach-id rach-id self-assigned this May 29, 2024
Copy link

coderabbitai bot commented May 29, 2024

Walkthrough

The changes introduce methods to compute and verify inner nodes' roots in a Namespaced Merkle Tree (NMT). This includes adding methods for subtree root computation (ComputeSubtreeRoot), validation (VerifySubtreeRootInclusion), and accompanying utility functions. The updates also refactor related struct field names and enhance testing with new test cases for the added functionality.

Changes

Files Change Summary
nmt.go Added ComputeSubtreeRoot method, isPowerOfTwo function, and updated struct field names.
nmt_test.go Added exampleNMT2 and TestComputeSubtreeRoot functions for testing subtree root computation.
proof.go Added VerifySubtreeRootInclusion, ToLeafRanges, nextLeafRange, and other utility functions.
proof_test.go Added multiple test functions to verify new utility functions and subtree root inclusion.

Assessment against linked issues

Objective (Issue #256) Addressed Explanation
Support inner nodes proving/verification
Computation and validation of subtree roots

Poem

In the realm of code where bytes dance free,
A tree gains wisdom, roots we now see.
With leaves that whisper of proofs and might,
Inner nodes' secrets come to light.
Hail to the Merkle, strong and true,
Verified roots, a dream come through. 🌳🚀


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented May 29, 2024

Codecov Report

Attention: Patch coverage is 82.14286% with 20 lines in your changes missing coverage. Please review.

Project coverage is 67.84%. Comparing base (3539dc8) to head (680faca).

Files Patch % Lines
proof.go 82.22% 8 Missing and 8 partials ⚠️
nmt.go 81.81% 2 Missing and 2 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 9fb3c91 and 4daae42.
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: The isPowerOfTwo function is implemented correctly and efficiently.


647-675: The implementation of ComputeSubtreeRoot method 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 ComputeSubtreeRoot is well-covered by unit tests, as evidenced by multiple occurrences in nmt_test.go and proof_test.go.

  • nmt_test.go: Contains a dedicated test function TestComputeSubtreeRoot.
  • proof_test.go: Contains numerous calls to ComputeSubtreeRoot with 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 in subtreeRootThresholdToLeafRange.

The method subtreeRootThresholdToLeafRange correctly 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 subtreeRootThresholdToLeafRange

Likely invalid or redundant comment.


587-633: Validate assumptions in toLeafRanges method.

The toLeafRanges method 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 for isPowerOfTwo.

The test cases for isPowerOfTwo are 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 in TestComputeSubtreeRoot is comprehensive.

The test function TestComputeSubtreeRoot includes various scenarios, including valid ranges and expected errors. It's crucial to ensure that all potential error conditions in the ComputeSubtreeRoot method 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 TestComputeSubtreeRoot function includes tests for error scenarios, as indicated by the presence of multiple expectError: true entries.

  • : The TestComputeSubtreeRoot function 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.go

Length 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.go

Length of output: 156

proof_test.go (8)

6-6: The import of the fmt package is appropriate as it is used in the test functions for formatting strings.


1128-1156: The test function TestLargestPowerOfTwo is 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 the largestPowerOfTwo function.


1158-1180: The test function TestSubtreeRootThresholdToLeafRange effectively 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 function TestToLeafRanges is 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 function TestNextLeafRange correctly 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 function TestSafeIntToUint is crucial for ensuring that the conversion from int to uint handles 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 function TestMinInt effectively tests the utility function minInt with 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 function TestVerifySubtreeRootInclusion is 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.

proof.go Outdated Show resolved Hide resolved
@rach-id rach-id marked this pull request as draft May 29, 2024 21:45
@rach-id rach-id marked this pull request as ready for review May 29, 2024 21:46
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 4daae42 and 4ba85fe.
Files selected for processing (1)
  • proof_test.go (2 hunks)
Additional comments not posted (8)
proof_test.go (8)

6-6: Ensure the fmt import is used in the file.


1129-1157: Review the new test function TestLargestPowerOfTwo.

This test function correctly checks various scenarios for the largestPowerOfTwo function, including edge cases like the input being zero, which should correctly return an error.


1159-1181: Review the new test function TestSubtreeRootThresholdToLeafRange.

This test function adequately covers different values for subtreeRootThreshold, including negative values, ensuring robust error handling.


1183-1333: Review the new test function TestToLeafRanges.

This test function comprehensively tests the toLeafRanges function 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 function TestNextLeafRange.

This test function effectively checks the nextLeafRange function 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 function TestSafeIntToUint.

This test function correctly handles the conversion from int to uint, including the scenario where the input is negative, which should appropriately trigger an error.


1476-1516: Review the new test function TestMinInt.

This test function is well-implemented to check the minInt function with various integer inputs, ensuring that it returns the correct minimum value.


1518-1842: Review the new test function TestVerifySubtreeRootInclusion.

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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4ba85fe and 12b62f1.

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 in safeIntToUint.

Consider adding a check for extremely large integers that might cause unexpected behavior when converted to uint.

+ // Consider adding checks for upper bounds if applicable
proof_test.go (8)

6-6: Added import of "fmt" package.

This import is necessary for the new test functions that use fmt.Sprintf for dynamic test case naming.


1129-1157: New test function TestLargestPowerOfTwo added.

This function tests the largestPowerOfTwo utility function across various bounds. It properly handles both expected results and error conditions, ensuring comprehensive coverage.


1159-1181: New test function TestSubtreeRootThresholdToLeafRange added.

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 function TestToLeafRanges added.

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 function TestNextLeafRange added.

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 function TestSafeIntToUint added.

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 function TestMinInt added.

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 function TestVerifySubtreeRootInclusion added.

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.

proof.go Outdated Show resolved Hide resolved
proof.go Outdated Show resolved Hide resolved
// the range of the leaves matching that namespace ID in the tree
namespaceRanges map[string]leafRange
namespaceRanges map[string]LeafRange
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also exported so that we have a way downstream to find the leaf ranges based on a proof start -> end

@rach-id
Copy link
Member Author

rach-id commented May 30, 2024

Making this a draft because I want to finish the implementation downstream to be sure this PR contains all the changes needed

@rach-id rach-id marked this pull request as draft May 30, 2024 09:20
@rach-id rach-id marked this pull request as ready for review June 2, 2024 22:28
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 12b62f1 and a81b748.

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 tests

proof.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 updateNamespaceRanges method 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 of namespaceRanges is correct.

The initialization of namespaceRanges using make(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 of LeafRange is justified.

Based on the existing comment from rach-id, the export of LeafRange is 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 in ComputeSubtreeRoot.

The method ComputeSubtreeRoot performs several checks on the input parameters start and end. However, there are a few improvements and corrections needed:

  1. The error message in line 654 should use "strictly" instead of "stricly".
  2. The method uses safeIntToUint for converting start and end to 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 start and end.

- 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 tests

proof.go (2)

713-718: Handle edge cases in safeIntToUint.

+ // Consider adding checks for upper bounds if applicable

The safeIntToUint function 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 for VerifySubtreeRootInclusion.

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 TestVerifySubtreeRootInclusion in proof_test.go includes various test cases that validate different aspects of the VerifySubtreeRootInclusion method. 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 VerifySubtreeRootInclusion method 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 5

Length 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.go

Length 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 tests

nmt_test.go (2)

865-877: Add documentation for exampleNMT2.

The function exampleNMT2 lacks a docstring. It's important to document the purpose and behavior of this function, especially how it differs from exampleNMT and in what contexts it should be used.

Tools
golangci-lint

867-867: undefined: NamespacedMerkleTree


1193-1322: Ensure correct handling of edge cases in TestComputeSubtreeRoot.

Verification successful

The TestComputeSubtreeRoot function in nmt_test.go includes 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 the fmt import is utilized effectively.

The import of the fmt package is justified as it is used in various test functions for formatting strings.


1129-1157: Review the new test function TestLargestPowerOfTwo.

This test function correctly checks the behavior of the largestPowerOfTwo function 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 function TestToLeafRanges.

The function TestToLeafRanges effectively tests the ToLeafRanges function with a comprehensive set of inputs, ensuring that it handles various sizes and conditions correctly. The use of dynamic test generation with fmt.Sprintf for test names enhances readability and maintainability.


1314-1408: Review the new test function TestNextLeafRange.

TestNextLeafRange provides a thorough examination of the nextLeafRange function, 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 function TestSafeIntToUint.

The test function TestSafeIntToUint is well-designed to check the conversion from int to uint, 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 function TestMinInt.

TestMinInt effectively tests the minInt function 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 function TestVerifySubtreeRootInclusion.

This test function thoroughly evaluates the VerifySubtreeRootInclusion method 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 Show resolved Hide resolved
proof.go Show resolved Hide resolved
nmt_test.go Show resolved Hide resolved
proof.go Outdated
Comment on lines 517 to 549
// 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
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@staheri14 staheri14 Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The subtree range exactly matches one of the supplied subtree roots -> hence return that subtree root
  2. 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
  3. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@staheri14 staheri14 Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@rach-id rach-id Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@staheri14 staheri14 Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
			}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also added another comment which also applies to your new suggested code block.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a81b748 and a68a69f.

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 for VerifySubtreeRootInclusion Method

The VerifySubtreeRootInclusion method 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:

  1. 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.

  2. 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.

  3. 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.

  4. 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.

  5. 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.

  6. 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 of ToLeafRanges Function

The ToLeafRanges function 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:

  1. 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.

  2. 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 of nextLeafRange Function

The nextLeafRange function 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:

  1. Range Calculation Logic:
    The function uses the minInt and safeIntToUint functions 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.

  2. 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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a68a69f and 9d92a94.

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 Function minInt Approved

This function is implemented correctly and efficiently.

proof.go Outdated Show resolved Hide resolved
Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general, added some comments as well.

nmt.go Show resolved Hide resolved
nmt.go Show resolved Hide resolved
proof.go Outdated
Comment on lines 516 to 534
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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears to me that all these lines can be removed since the the same logic is covered after this if block.

Suggested change
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
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor

@staheri14 staheri14 Jun 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@staheri14 staheri14 Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 expect right==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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the changes discussed in this thread are here: 70bc2b0 , let's merge it now and follow up if needed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is something left to discuss, let's do it in separate issues 👍

nmt.go Show resolved Hide resolved
end: 16,
tree: n,
expectedRoot: func() []byte {
root, err := n.Root()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment on lines 1302 to 1307
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@staheri14 staheri14 Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

  • subtreeWidth is a power of two and cannot exceed the tree size for which the proof is generated, nor can it exceed the proof's end-start range.
  • The proof's start index must be a multiple of subtreeWidth.
  • subtreeRoots are the MMR picks of the tree starting from the proof's start index. Each node in subtreeRoots has a range of at most subtreeWidth.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also depends on: #260 (comment)

proof.go Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 9d92a94 and 12ec563.

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 tests

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 (5)
nmt.go (2)

593-600: Ensure LeafRange is used correctly.

The changes involve renaming leafRange to LeafRange. Ensure that the new type LeafRange is correctly defined and used in the rest of the codebase.


647-650: Document the inclusivity of the end parameter.

It would be beneficial to explicitly state whether the end parameter 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 minInt function 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.

nmt.go Show resolved Hide resolved
nmt.go Show resolved Hide resolved
nmt.go Show resolved Hide resolved
proof.go Show resolved Hide resolved
proof.go Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 9d92a94 and 12ec563.

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 tests

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 (3)
nmt.go (2)

673-678: LGTM!

The LeafRange type definition and comments are clear and well-documented.


667-669: Verify the nextSubtreeSize function usage.

Ensure that the nextSubtreeSize function 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 -->

nmt.go Show resolved Hide resolved
proof.go Outdated Show resolved Hide resolved
proof.go Outdated Show resolved Hide resolved
proof.go Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 12ec563 and 3ad98af.

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.

proof.go Show resolved Hide resolved
proof.go Show resolved Hide resolved
proof.go Show resolved Hide resolved
proof.go Show resolved Hide resolved
proof.go Show resolved Hide resolved
proof.go Outdated Show resolved Hide resolved
evan-forbes
evan-forbes previously approved these changes Jul 16, 2024
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's unblock this if possible! LGTM

proof.go Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 3ad98af and 70bc2b0.

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 of safeIntToUint.

Consider adding checks for upper bounds to prevent potential integer overflow issues, especially in environments where int and uint sizes 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 TestLargestPowerOfTwo correctly tests various boundary conditions for the function largestPowerOfTwo. 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 TestToLeafRanges is 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 compareRanges is used in the TestToLeafRanges function 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:

TestNextLeafRange tests 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 int to uint, which is a critical operation given that uint cannot represent negative numbers. The function safeIntToUint is 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:

TestMinInt effectively tests the minInt function, 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 TestVerifySubtreeRootInclusion function 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.

nmt_test.go Outdated Show resolved Hide resolved
nmt_test.go Show resolved Hide resolved
proof.go Show resolved Hide resolved
proof.go Show resolved Hide resolved
proof.go Show resolved Hide resolved
proof.go Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 70bc2b0 and a580bac.

Files selected for processing (1)
  • nmt_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • nmt_test.go

@rach-id rach-id enabled auto-merge (squash) July 16, 2024 13:47
Comment on lines +648 to +649
// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rach-id rach-id merged commit 1278ba2 into main Jul 16, 2024
7 of 8 checks passed
@rach-id rach-id deleted the verify-power-2 branch July 16, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support inner nodes proving/verification
6 participants