RFC: Use MAX_BLOB_COMMITMENTS_PER_BLOCK as max length for BlindedBlobsBundle SSZ lists

[jimmygchen]

Context

In the below consensus PR, MAX_BLOB_COMMITMENTS_PER_BLOCK was introduced and used in the SSZ specification for blob_kzg_commitments in the deneb beacon block:

• Update block's blob_kzg_commitments size limit to MAX_BLOB_COMMITMENTS_PER_BLOCK (4096) consensus-specs#3338

The SSZ representation of KZG blobs commitments are currently different in BeaconBlockBody and BlindedBlobBundle:

BeaconBlockBody:

class BeaconBlockBody(Container):
    ...
    blob_kzg_commitments: List[KZGCommitment, MAX_BLOB_COMMITMENTS_PER_BLOCK]  # [New in Deneb:EIP4844]

BlindedBlobsBundle:

 class BlindedBlobsBundle(Container): 
     commitments: List[KZGCommitment, MAX_BLOBS_PER_BLOCK] 
     proofs: List[KZGProof, MAX_BLOBS_PER_BLOCK] 
     blob_roots: List[Root, MAX_BLOBS_PER_BLOCK] 

I'm interested in hearing people's thought about whether MAX_BLOBS_PER_BLOCK or MAX_BLOB_COMMITMENTS_PER_BLOCK should be used in the BlindedBlobsBundle SSZ specification. AFAIK this has no impact on serialisation or deserialisation, but will be used in computing the signing root for the BuilderBid.

I think we'd be okay either way, as this tree hash root isn't stored anywhere and only used for bid signatures, which means it's short lived. Using MAX_BLOB_COMMITMENTS_PER_BLOCK for max length gives us consistency with the beacon block representation, which means in some languages we could reuse the same type. On the other hand, using MAX_BLOBS_PER_BLOCK means it's probably more consistent with the rest of the API specs - maxItems: 6 is specified for all blob related lists, including blobs_sidecars.

I don't have a strong opinion here, keen to hear people's thoughts!

[realbigsean]

To me it makes sense to use MAX_BLOB_COMMITMENTS_PER_BLOCK. We wouldn't have to worry about updating it if we have a hardfork to update MAX_BLOBS_PER_BLOCK.

[jimmygchen]

I'm convinced to use MAX_BLOB_COMMITMENTS_PER_BLOCK as well - I think there isn't much upside to go with the alternative, and it isn't worth having to add extra logic / types to update the SSZ representation between forks on both the relay and CLs.

[ralexstokes]

makes sense!

one thing is that the limit of the List type does not influence the serialization or the hash tree root, it is only used when serializing or deserializing to understand if the data violates the type's size

that being said, I do think using this constant over the MAX_BLOBS_PER_BLOCK is more forward-compatible and will require less maintenance over time

[realbigsean]

This does end up impacting the hash tree root of List because max size is used in chunk_count in the spec: https://github.com/ethereum/consensus-specs/blob/dev/ssz/merkle-proofs.md?plain=1#L136

which is passed to get_power_of_two_ceil here: https://github.com/ethereum/consensus-specs/blob/dev/ssz/merkle-proofs.md?plain=1#L185

so in this case if MAX_BLOBS_PER_BLOCK is 9 or higher (or 4 or lower) the root changes

[ralexstokes]

This does end up impacting the hash tree root of List because max size is used in chunk_count in the spec: https://github.com/ethereum/consensus-specs/blob/dev/ssz/merkle-proofs.md?plain=1#L136

which is passed to get_power_of_two_ceil here: https://github.com/ethereum/consensus-specs/blob/dev/ssz/merkle-proofs.md?plain=1#L185

so in this case if MAX_BLOBS_PER_BLOCK is 9 or higher (or 4 or lower) the root changes

interesting @realbigsean , there may be a discrepancy in the specs, as I was referencing this: https://github.com/ethereum/consensus-specs/blob/dev/ssz/simple-serialize.md#merkleization
for the chunk_count where I think for this example would be indifferent to the size of the list

[realbigsean]

I think it's consistent cause N is the max size of the list (what we're updating) in List[B, N]

List[B, N] and Vector[B, N], where B is a basic type: (N * size_of(B) + 31) // 32 (dividing by chunk size, rounding up)