search

celestiaorg / celestia-app

PoS application for the consensus portion of the Celestia network. Built using celestia-core (fork of CometBFT) and the cosmos-sdk
https://celestia.org
Apache License 2.0
328 stars 261 forks source link

test: prepare proposal block data size #3584

Closed rootulp closed 1 week ago

rootulp commented 1 week ago

Closes https://github.com/celestiaorg/celestia-app/issues/3573

Note: we could add an explicit conditional at the end of prepare proposal like: 

    if getSize(txs) > req.BlockDataSize {
        panic("the size of the proposal block data exceeds the block data size")
    }

but I'm not sure that actually gets us much. Instead of panic'ing in celestia-core (like it does currently), it'll panic in celestia-app. Since this is a change to prepare proposal which has already been audited, I'm hesitant to add the conditional unless reviewers feel strongly that it is helpful.

coderabbitai[bot] commented 1 week ago

Walkthrough

The changes in prepare_proposal_test.go focus on renaming the variable accnts to accounts throughout the file. This involves updating all instances of the variable in function calls and array slices related to account information. The primary goal is to improve code readability and maintain consistency in naming conventions.

Changes

File Change Summary
app/test/prepare_proposal_test.go Renamed variable accnts to accounts throughout the file for consistency

Sequence Diagram(s)

(No changes made to control flow or interactions between components, so diagram generation was skipped.)

Assessment against linked issues

Objective Addressed Explanation
Unit test to ensure prepare proposal does not exceed BlockDataSize in ResponsePrepareProposal.BlockData.Txs (#3573) ❌ No new unit tests were added to address this specific requirement.

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 - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
Tips ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): > :bangbang: **IMPORTANT** > Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged. - 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](https://docs.coderabbit.ai/guides/configure-coderabbit) 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](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.
cmwaters commented 1 week ago

Is this some form of defensive programming in the instance that we modify the algorithm such that there is a possibility that bytes are added and that therefore this test would catch it?

rootulp commented 1 week ago

Is this some form of defensive programming in the instance that we modify the algorithm such that there is a possibility that bytes are added and that therefore this test would catch it?

Unfortunately I don't think this test would actually catch it because it doesn't fuzz inputs into prepare proposal. If we wanted to be defensive then we would have to add the explicit check in prepare proposal (see PR description).

This PR is motivated by the last audit finding. It communicates the "intention" of prepare proposal but doesn't actually enforce any new constraints.

cmwaters commented 1 week ago

Yeah, I'm a little confused by it's existence. Are you saying it's there just to communicate to people that we don't expect PrepareProposal to somehow increase the bytes returned?

rootulp commented 1 week ago

Yea exactly

rootulp commented 1 week ago

Discussed with @cmwaters offline and we decided this informational audit finding should just be acknowledged and the unit test doesn't add much value.