Assert array items in BlobsBundleV1 to be of same length

[g11tech]

UPDATE:
This PR proposed two things

i. Arrays are of the same length.
ii. The length is limited by MAX_BLOBS_PER_BLOCK .

Consensus to keep (i) and let CL handle (ii) in process block ops (where the MAX_BLOBS_PER_BLOCK check has already been placed)

---

In consensus specs, the blob limit it can handle is defined by MAX_BLOBS_PER_BLOCK and this check in apis will make the EL aware of this limit in CL and have mitigations (like perform min(MAX_DATA_GAS_PER_BLOCK / DATA_GAS_PER_BLOB,MAX_BLOBS_PER_BLOCK) to get actual blob limit for the block to return in execution apis

[Inphi]

This might be a bit redundant given that the MAX_DATA_GAS_PER_BLOCK block validity condition already asserts this.

There may be at most MAX_DATA_GAS_PER_BLOCK // DATA_GAS_PER_BLOB total blob commitments in a valid block.

[g11tech]

yes you are right @Inphi ! may be we also remove MAX_BLOBS_PER_BLOCK from consensus specs and not have independent variables lying around. (so consensus-specs (and configs/presets) also use the same reference MAX_DATA_GAS_PER_BLOCK // DATA_GAS_PER_BLOB)

[mkalinin]

yes you are right @Inphi ! may be we also remove MAX_BLOBS_PER_BLOCK from consensus specs and not have independent variables lying around. (so consensus-specs (and configs/presets) also use the same reference MAX_DATA_GAS_PER_BLOCK // DATA_GAS_PER_BLOB)

It does seem to me that we should increase MAX_BLOBS_PER_BLOCK up to a sane but large enough number that would allow us to effectively limit a number of blobs on EL side without any changes to CL configuration.

[g11tech]

yes you are right @Inphi ! may be we also remove MAX_BLOBS_PER_BLOCK from consensus specs and not have independent variables lying around. (so consensus-specs (and configs/presets) also use the same reference MAX_DATA_GAS_PER_BLOCK // DATA_GAS_PER_BLOB)

It does seem to me that we should increase MAX_BLOBS_PER_BLOCK up to a sane but large enough number that would allow us to effectively limit a number of blobs on EL side without any changes to CL configuration.

makes sense to me if we plan to keep raising the data gas limit which we will, fixed high static limit is good for the CLs as well as for making EL checks

How about 128? or 1024, effectively sharding limit

[mkalinin]

1024 sounds good to me, I think we should open a PR into CL specs repo and have a discussion there

[g11tech]

opened a PR here: ethereum/consensus-specs#3338

But seems like this PR addition should make sense here to quantify the length

cc @Inphi

[g11tech]

@mkalinin @Inphi

do you think this clarification still makes sense for number of items to be <= MAX_BLOBS_PER_BLOCK or should this PR be closed?

[Inphi]

It's confusing to note that the limit here should be 1024 whereas in reality it'll always be MAX_BLOBS_PER_BLOCK. Setting this to 1024 also makes execution-apis kinda dependent on the CL. Maybe that's fine given that execution APIs is sorta inbetween. But overall I'm weakly for closing this PR.

[mkalinin]

Specifying this limitation in the terms used by EL, i.e. MAX_DATA_GAS_PER_BLOCK / DATA_GAS_PER_BLOB makes more sense to me in this case.

The question is whether we want to specify it at all? I can find it useful if we plan to check this in Hive, having a statement in the spec helps to avoid missing a check for it. cc @marioevz

[g11tech]

the reason i find it useful is for the EL: it has to be technically min of (MAX_DATA_GAS_PER_BLOCK / DATA_GAS_PER_BLOB,MAX_BLOBS_PER_BLOCK)

since MAX_DATA_GAS_PER_BLOCK / DATA_GAS_PER_BLOB is already well defined check for the EL, but MAX_BLOBS_PER_BLOCK doesn't find a mention in any EL related docs

And we actually do not mention a value for it just a reminder for the api to be aware of this, will remove it from description

[ppopth]

the reason i find it useful is for the EL: it has to be technically min of (MAX_DATA_GAS_PER_BLOCK / DATA_GAS_PER_BLOB,MAX_BLOBS_PER_BLOCK)

Is it possible to just assert that MAX_DATA_GAS_PER_BLOCK / DATA_GAS_PER_BLOB is not greater than MAX_BLOBS_PER_BLOCK instead of doing the min of the two?

The benefit of this is that MAX_BLOBS_PER_BLOCK will be used only for assertion and not used anywhere else and the EL logic can rely on only the EL terms of MAX_DATA_GAS_PER_BLOCK and DATA_GAS_PER_BLOB.

I had suggested this in ethereum/consensus-specs#3338 (comment)

[g11tech]

used

I think it will work, but EL will anyway need to track: MAX_BLOBS_PER_BLOCK as a hardfork param (like how it tracks MAX_DATA_GAS_PER_BLOCK etc) and asset while validating the block

So then here we wouldn't need to refer to MAX_BLOBS_PER_BLOCK but this constant should then find reference in the EIP itself in the block validity section

[mkalinin]

@ppopth @g11tech what's the status of this PR? There are two checks proposed:
i. Arrays are of the same length.
ii. The length is limited by MAX_BLOBS_PER_BLOCK .

From my perspective we should keep (i) and remove (ii) as EL limits a number of blobs by the data gas limitation and CL enforces this check on block processing and before sending a block and blobs to the wire.

[g11tech]

@ppopth @g11tech what's the status of this PR? There are two checks proposed: i. Arrays are of the same length. ii. The length is limited by MAX_BLOBS_PER_BLOCK .

From my perspective we should keep (i) and remove (ii) as EL limits a number of blobs by the data gas limitation and CL enforces this check on block processing and before sending a block and blobs to the wire.

ok sounds good to me to retain i) and let CL handle MAX_BLOBS_PER_BLOCK

[dankrad]

lgtm