search

rollkit / celestia-da

Celestia Implementation of Modular Data Availability Interface
Apache License 2.0
18 stars 14 forks source link

celestia-da: account for padding shares #70

Closed tuxcanfly closed 9 months ago

tuxcanfly commented 10 months ago

Overview

This PR fixes an issue with the MaxBlobSize API call. The current size doesn't account for padding shares.

Checklist

Summary by CodeRabbit

coderabbitai[bot] commented 10 months ago

Walkthrough

The update introduces new constants for commit sizes and block encoding overhead in celestia.go, refining the calculation of DefaultMaxBytes and thus the MaxBlobSize. This adjustment ensures more accurate data handling capacities. Correspondingly, celestia_test.go sees a modification where a test now directly references DefaultMaxBytes instead of an outdated constant, aligning the test assertions with the updated data size calculations.

Changes

Files Change Summary
celestia/celestia.go,
celestia/.../celestia_test.go		 Added constants for commit sizes and block encoding overhead. Updated DefaultMaxBytes calculation.
Updated test assertion to use DefaultMaxBytes directly, removing outdated constant reference.

"In the realm of code, where bytes do flow,
A rabbit hopped, making changes glow.
Constants new, and sizes true,
🌟 In Celestia's skies, data anew."

  • 🐰 CodeRabbit

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: - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit-tests for this file.` - 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 tests 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 from git and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit tests.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` 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 a review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@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. - The JSON schema for the configuration file is available [here](https://coderabbit.ai/integrations/coderabbit-overrides.v2.json). - 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` ### CodeRabbit Discord Community Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback.
codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (6afe388) 74.05% compared to head (8aa63af) 74.05%.

:exclamation: Current head 8aa63af differs from pull request most recent head 1a20032. Consider uploading reports for the commit 1a20032 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #70 +/- ## ======================================= Coverage 74.05% 74.05% ======================================= Files 2 2 Lines 158 158 ======================================= Hits 117 117 Misses 28 28 Partials 13 13 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tuxcanfly commented 10 months ago

OK, I dug pretty deep into this. I believe this is not the right fix because we're doing tx accounting overhead to the blob limit.

With some debugging I found instead that the issue is that transactions are accepted into the mempool, but they're not added to the block because of improper validation, I added some trace fmts to prepare_proposal.go:

app.App.PrepareProposal: FilterTxs len(txs): 1
app.App.PrepareProposal: square.Build len(txs): 0

It seems that square.Build has stricter conditions that everything else. It also silently ignores transactions it deems invalid. This causes the tx to stay in mempool and times out the DA submit transaction, leading to a cascade of errors. I believe the proper fix is in celestia-app/go-square, but until then we can mitigate this by hardcoding the blob limit that it accepts.

tuxcanfly commented 9 months ago

Some more logging indicates that there is a worst case padding which might be missing from mempool inclusion checks:

7:00PM DBG inserted new (maybe) valid transaction height=3 num_txs=1 priority=2000 size=1962824 tx=DB4489C41BDFA0F3C47CC5F0354BD4B9CC62AF52EE13D9E17D99880E54F35411
in mempool.v1.TxMempool.ReapMaxBytesMaxGas
(maxGas: -1 >= 0 && totalGas: 0 + w.gasWanted: 16754612 > maxGas: -1) || (maxBytes: 1973430 >= 0 && totalBytes: 0 + txBytes: 1962828 > maxBytes: 1973430
state.BlockExecutor.CreateProposalBlock: ReapMaxBytesMaxGas txs: 1
in app.App.PrepareProposal
app.App.PrepareProposal: FilterTxs txs: 1
in square.newElement: len(blob.Data): 1962441; numShares: 4072
in square.Builder.AppendBlobTX loop: blobElements[idx].NumShares: 4072; blobElements[idx].PfbIndex: 0, blobElements[idx].BlobIndex: 0; blobElements[idx].MaxPadding: 63
in square.Builder.AppendBlobTx: pfbShareDiff: 1; maxBlobShareCount: 4135
in square.Builder.canFit: b.currentSize: 0; shareNum: 4136; b.maxCapacity: 4096
app.App.PrepareProposal: square.Build txs: 0