fix: off by one slice bug #254

rach-id · 2023-11-02T10:45:15Z

Overview

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

Summary by CodeRabbit

New Features
- Enhanced data slicing functionality to handle invalid range inputs and out-of-bounds scenarios.
Bug Fixes
- Fixed an issue where data elements were incorrectly assigned during slicing.
Tests
- Added new tests to validate the improved slicing functionality and error handling.

coderabbitai · 2023-11-02T10:45:21Z

Walkthrough

The changes primarily focus on enhancing the slice function in the DAVerifier library. The function now includes checks for invalid and out-of-bounds range inputs, and returns an error code along with the sliced data. The updates also introduce new test functions in the DAVerifierTest contract to validate the improved slice function.

Changes

File Path	Change Summary
src/lib/verifier/DAVerifier.sol	Updated the `slice` function to handle invalid and out-of-bounds range inputs. Introduced new error codes `InvalidRange` and `OutOfBoundsRange`. Modified the `verifySharesProof` and `verifySharesToRowsProof` functions to handle the new error codes.
src/lib/verifier/test/DAVerifier.t.sol	Added new test functions `testValidSlice`, `testInvalidSliceBeginEnd`, and `testOutOfBoundsSlice` to validate the updated `slice` function in the `DAVerifier` contract.

Poem

🐇 Hopping through the code, with a twinkle in our eye, 🌟

Fixing bugs and testing stuff, under the autumn sky. 🍂

On this day in history, the magic was quite clear, ✨

As we sliced and diced the data, with nothing left to fear. 📊

So here's to all the coders, working night and day, 💻

Your efforts are appreciated, in every single way! 🎉

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

If you reply to a review comment from CodeRabbit, the bot will automatically respond.
To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
Note: Review comments are made on code diffs or files, not on the PR overview.
Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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 a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai help to get help.
@coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
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/coderabbit-overrides.v2.json

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 9dade12 and 6f061b4.

Files selected for processing (1)

src/lib/verifier/DAVerifier.sol (1 hunks)

Additional comments: 1

src/lib/verifier/DAVerifier.sol (1)
238-244: The fix for the off-by-one error looks good. It correctly adjusts the index for the out array to ensure that the elements from _data are correctly sliced and assigned to the out array. However, it would be good to add a check to ensure that _begin is less than or equal to _end to avoid potential issues.
+ if (_begin > _end) {
+     revert("Invalid range: _begin is greater than _end");
+ }

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 6f061b4 and 0b9a380.

Files selected for processing (1)

src/lib/verifier/DAVerifier.sol (1 hunks)

Files skipped from review due to trivial changes (1)

src/lib/verifier/DAVerifier.sol

src/lib/verifier/DAVerifier.sol

coderabbitai

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 0b9a380 and e2daa12.

Files selected for processing (1)

src/lib/verifier/DAVerifier.sol (3 hunks)

Additional comments: 2

src/lib/verifier/DAVerifier.sol (2)

64-70: The addition of the InvalidRange error code is a good practice for handling invalid range inputs in the slice function.

109-121: The slice function is now being used to extract a subset of shares from _sharesProof.data. The error handling for the slice function is correctly implemented.

src/lib/verifier/DAVerifier.sol

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between e2daa12 and 39d7c7d.

Files selected for processing (1)

src/lib/verifier/DAVerifier.sol (3 hunks)

Additional comments: 3

src/lib/verifier/DAVerifier.sol (3)
64-71: The addition of new error codes InvalidRange and OutOfBoundsRange is a good practice for better error handling and debugging.

111-123: The error handling in the verifySharesProof function has been updated to check for errors when slicing the data. This is a good practice as it ensures that the function behaves correctly even when it encounters unexpected input or state.

240-259: The slice function has been updated to return a tuple of bytes[] and ErrorCodes. This is a good practice as it allows the function to communicate more information about its execution, specifically any errors that occurred. The function now checks if the provided range is valid and within the bounds of the _data array, which improves the robustness of the function.
- function slice(bytes[] memory _data, uint256 _begin, uint256 _end) internal pure returns (bytes[] memory)
+ function slice(bytes[] memory _data, uint256 _begin, uint256 _end) internal pure returns (bytes[] memory, ErrorCodes)

adlerjohn · 2023-11-06T22:11:02Z

src/lib/verifier/DAVerifier.sol

        bytes[] memory out = new bytes[](_end-_begin);
        for (uint256 i = _begin; i < _end; i++) {
-            out[i] = _data[i];
+            out[i - _begin] = _data[i];


Can you add a test that hits this codepath and would have caught the bug?

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 39d7c7d and a042ac6.

Files selected for processing (1)

src/lib/verifier/test/DAVerifier.t.sol (1 hunks)

Additional comments: 3

src/lib/verifier/test/DAVerifier.t.sol (3)

197-209: The test testValidSlice is well written and covers the expected behavior of the slice function when provided with valid input. It checks both the returned slice and the error code.

211-221: The test testInvalidSliceBeginEnd correctly checks the behavior of the slice function when _begin is greater than _end. It verifies that the appropriate error code is returned.

223-235: The test testOutOfBoundsSlice checks the behavior of the slice function when the slice range is out of bounds. It verifies that the appropriate error code is returned in both cases.

evan-forbes

lgtm but I'll defer to others for solidity things

fix: off by one slice bug

6f061b4

rach-id added bug Something isn't working contracts labels Nov 2, 2023

rach-id self-assigned this Nov 2, 2023

rach-id requested a review from adlerjohn as a code owner November 2, 2023 10:45

coderabbitai bot reviewed Nov 2, 2023

View reviewed changes

fix: add _begin and _end checks

0b9a380

coderabbitai bot reviewed Nov 2, 2023

View reviewed changes

evan-forbes previously approved these changes Nov 2, 2023

View reviewed changes

src/lib/verifier/DAVerifier.sol Show resolved Hide resolved

adlerjohn reviewed Nov 2, 2023

View reviewed changes

src/lib/verifier/DAVerifier.sol Outdated Show resolved Hide resolved

src/lib/verifier/DAVerifier.sol Show resolved Hide resolved

chore: add an error instead of reverting

e2daa12

rach-id dismissed evan-forbes’s stale review via e2daa12 November 2, 2023 17:08

coderabbitai bot reviewed Nov 2, 2023

View reviewed changes

src/lib/verifier/DAVerifier.sol Outdated Show resolved Hide resolved

chore: add another check

39d7c7d

coderabbitai bot reviewed Nov 2, 2023

View reviewed changes

adlerjohn reviewed Nov 6, 2023

View reviewed changes

test: add slice tests

a042ac6

coderabbitai bot reviewed Nov 7, 2023

View reviewed changes

evan-forbes approved these changes Nov 7, 2023

View reviewed changes

rach-id requested a review from adlerjohn November 7, 2023 10:12

adlerjohn approved these changes Nov 8, 2023

View reviewed changes

rach-id merged commit 5b34595 into celestiaorg:master Nov 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: off by one slice bug #254

fix: off by one slice bug #254

rach-id commented Nov 2, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 2, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

adlerjohn Nov 6, 2023

rach-id Nov 7, 2023

coderabbitai bot left a comment

evan-forbes left a comment

fix: off by one slice bug #254

fix: off by one slice bug #254

Conversation

rach-id commented Nov 2, 2023 • edited by coderabbitai bot Loading

Overview

Checklist

Summary by CodeRabbit

coderabbitai bot commented Nov 2, 2023 • edited Loading

Walkthrough

Changes

Poem

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

adlerjohn Nov 6, 2023

Choose a reason for hiding this comment

rach-id Nov 7, 2023

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

rach-id commented Nov 2, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 2, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)