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

blob.NewBlob will reject > 2 MB blobs even if the square size is increased #3356

Open
rootulp opened this issue May 2, 2024 · 5 comments
Open
Labels
area:blob area:state Related to fetching state and state execution external Issues created by non node team members v0.15.0 Intended for v0.15.0 release

Comments

@rootulp
Copy link
Collaborator

rootulp commented May 2, 2024

Context

celestia-node enforces a max blob size of 1.97 (1974272 bytes) here.

Problem

If governance votes to allow 8MB blocks via bumping:

  • consensus.block.MaxBytes to 8MiB
  • blob.GovMaxSquareSize to 128

then celestia-node will reject blobs that could theoretically fit inside a block.

Question

Is the max blob size check here necessary?

  • If no, can we remove it and rely on error handling from consensus nodes?
  • If yes, can it be made aware of the governance params and infer a max blob size based on them?

Tangent: I think it's quite complex to determine what is the max possible blob size that is submittable on Celestia b/c it depends on a number of parameters that can be changed up or down. We may consider adding an explicit constraint on the max blob size (i.e. 1 MiB) to make checks like this stateless.

@github-actions github-actions bot added needs:triage external Issues created by non node team members labels May 2, 2024
@jcstein
Copy link
Member

jcstein commented May 2, 2024

Isn't this max blob size bigger than consensus.block.MaxBytes | 1.88MiB set in celestia-app mainnet parameters?

@rootulp
Copy link
Collaborator Author

rootulp commented May 2, 2024

1974272 bytes == 1.88 MiB but the params in the specs should be updated to use the exact byte value to make that clearer.

rootulp added a commit to celestiaorg/celestia-app that referenced this issue May 2, 2024
@renaynay renaynay added area:state Related to fetching state and state execution area:blob and removed needs:triage labels May 6, 2024
@renaynay
Copy link
Member

renaynay commented May 6, 2024

@rootulp if governance votes to increase or decrease the Max blob size, would that const (DefaultMaxBytes) get updated in the app version? If so, node just needs to import the upgrade and it will automatically adjust to the new value (which is why we point to the app const to begin with).

Please confirm if this is the case. If not, then we can decide how to proceed.

@rootulp
Copy link
Collaborator Author

rootulp commented May 6, 2024

@rootulp if governance votes to increase or decrease the Max blob size, would that const (DefaultMaxBytes) get updated in the app version?

No it would not. DefaultMaxBytes is the initial value (i.e. the default) for the param. Governance can modify it without a code change to DefaultMaxBytes in celestia-app.

@renaynay
Copy link
Member

renaynay commented May 9, 2024

Okay good to know @rootulp , we will include a fix for this in the following release (v0.15.0) as this doesn't seem pressing for now.

@renaynay renaynay added the v0.15.0 Intended for v0.15.0 release label May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:blob area:state Related to fetching state and state execution external Issues created by non node team members v0.15.0 Intended for v0.15.0 release
Projects
None yet
Development

No branches or pull requests

3 participants