celestia-node icon indicating copy to clipboard operation
celestia-node copied to clipboard

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

Open rootulp opened this issue 1 year ago • 5 comments

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.

rootulp avatar May 02 '24 11:05 rootulp

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

jcstein avatar May 02 '24 13:05 jcstein

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 avatar May 02 '24 17:05 rootulp

@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.

renaynay avatar May 06 '24 16:05 renaynay

@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.

rootulp avatar May 06 '24 17:05 rootulp

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 avatar May 09 '24 14:05 renaynay

Hey @rootulp -- is this still relevant? Aren't we hard capping the max blob size anyway ?

renaynay avatar Oct 29 '24 15:10 renaynay

I think it is, at least based on CIP-28: "Transaction size is limited to 2MiB by setting the versioned parameter MaxTxSize to 2097152"

So I think this should at least be changed to match 2MiB, because it's currently:

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

Otherwise I think this issue is mostly complete with CIP-28

jcstein avatar Oct 29 '24 16:10 jcstein

I think we should remove the blob size check in celestia-node all together because it isn't necessary. The check was added in this PR but DefaultMaxBytes is not appropriate to use because that constant applies to something else:

// DefaultMaxBytes is the default value for the governance modifiable
// maximum number of bytes allowed in a valid block.
DefaultMaxBytes = DefaultGovMaxSquareSize * DefaultGovMaxSquareSize * share.ContinuationSparseShareContentSize

There is currently no defined max blob size limit. It is inferred based on other parameters.

rootulp avatar Nov 12 '24 20:11 rootulp