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: Check max blob size and return err if exceeds #2495

Closed
Tracked by #2287
renaynay opened this issue Jul 18, 2023 · 9 comments
Closed
Tracked by #2287

blob: Check max blob size and return err if exceeds #2495

renaynay opened this issue Jul 18, 2023 · 9 comments
Assignees

Comments

@renaynay
Copy link
Member

renaynay commented Jul 18, 2023

Blocked on core/app (cc @rootulp)

@rootulp
Copy link
Collaborator

rootulp commented Jul 31, 2023

@Wondertan Wondertan changed the title nodebuilder/blob: Check max blob size and return err if exceeds blob: Check max blob size and return err if exceeds Jul 31, 2023
@rootulp
Copy link
Collaborator

rootulp commented Jul 31, 2023

We're considering improving the behavior of CheckTx in celestia-app by rejecting blobs that are too large (see celestiaorg/celestia-app#2156). Once that is implemented, I don't think it will be necessary for celestia-node to perform any additional validation for max blob size b/c I expect it to be covered by CheckTx. @vgonkivs thoughts?

@Wondertan
Copy link
Member

I think if that's a stateless check, we should add it to our validation path. Specifically in the blob constructor on our side.

@rootulp
Copy link
Collaborator

rootulp commented Jul 31, 2023

The stateless check depends on a versioned constant (SquareSizeUpperBound) and a hard-coded constant (MaxBlockSizeBytes). We can add it to blob constructor on celestia-node side with the caveat that the versioned constant may change in the future.

@Wondertan
Copy link
Member

The version, in this case, should be the one binary was built with.

@rootulp
Copy link
Collaborator

rootulp commented Jul 31, 2023

The versioned constant is dependent on the app version which is currently v1 (see here) and likely will be incremented when there's a new major version of celestia-app.

@vgonkivs
Copy link
Member

Can we simply use LatestVersion constant even when It will be v2,v3...?

@rootulp
Copy link
Collaborator

rootulp commented Jul 31, 2023

I think celestia-node can use LatestVersion because I would expect celestia-app v1.x.x LatestVersion to be v1 and celestia-app v2.x.x's LatestVersion to be v2.

@rootulp
Copy link
Collaborator

rootulp commented Sep 7, 2023

Closing as won't do. We've improved the behavior of celestia-app's CheckTx in celestiaorg/celestia-app#2202 so txs with a total blob size that is too large will be rejected at the mempool level.

@rootulp rootulp closed this as not planned Won't fix, can't repro, duplicate, stale Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants