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.

Sign up for GitHub

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

Update block's blob_kzg_commitments size limit to MAX_BLOB_COMMITMENTS_PER_BLOCK (4096) #3338

Merged
merged 11 commits into from
May 24, 2023
Merged

Update block's blob_kzg_commitments size limit to MAX_BLOB_COMMITMENTS_PER_BLOCK (4096) #3338

Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
17dac8c
Update MAX_BLOBS_PER_BLOCK to a higher bound
g11tech Apr 25, 2023
5e43f43
update limit to 4844 friendly 16 blobs
g11tech Apr 28, 2023
1aad9b5
adding a fixed theoretical limit for commitments in a block
g11tech May 6, 2023
9f530a7
update max commitments per block limit to blobs per tx limit from eip…
g11tech May 7, 2023
a75292b
restore the 4844 max limit to 4
g11tech May 11, 2023
8ccc257
apply feedback
g11tech May 20, 2023
4458645
add check
g11tech May 22, 2023
feb1968
add comment
g11tech May 22, 2023
a05b689
Merge branch 'dev' into pr3338
hwwhww May 24, 2023
f0a4281
Add tests. Add validation in the p2p beacon block gossiping
hwwhww May 24, 2023
e9cc8dc
PR feedback of Danny + verify `MAX_BLOBS_PER_BLOCK` size in unittest
hwwhww May 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions specs/deneb/beacon-chain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@

This upgrade adds blobs to the beacon chain as part of Deneb. This is an extension of the Capella upgrade.

The blob transactions are packed into the execution payload by the EL/builder with their corresponding blobs being independently transmitted and are limited by `MAX_DATA_GAS_PER_BLOCK // DATA_GAS_PER_BLOB`. However the CL limit is independently defined by `MAX_BLOBS_PER_BLOCK`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I would put this reference to execution-layer constraints here.

Definitely don't need a reference to "builder", and there is no definition of EL in the consensus-layer specs at this point. If we want to reference that layer use ExecutionEngine


## Custom types

| Name | SSZ equivalent | Description |
Expand Down Expand Up @@ -68,7 +70,8 @@ This upgrade adds blobs to the beacon chain as part of Deneb. This is an extensi

| Name | Value |
| - | - |
| `MAX_BLOBS_PER_BLOCK` | `uint64(2**2)` (= 4) |
| `MAX_BLOB_COMMITMENTS_PER_BLOCK` | `uint64(2**12)` (= 4096) | hardfork independent fixed theoretical limit same as `LIMIT_BLOBS_PER_TX` (see EIP 4844) |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| `MAX_BLOB_COMMITMENTS_PER_BLOCK` | `uint64(2**12)` (= 4096) | hardfork independent fixed theoretical limit same as `LIMIT_BLOBS_PER_TX` (see EIP 4844) |
| `MAX_BLOB_COMMITMENTS_PER_BLOCK` | `uint64(2**12)` (= 4096) | hardfork independent fixed theoretical limit same as `LIMIT_BLOBS_PER_TX` (see EIP 4844) |

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think mentioning the EL constants in the consensus spec is a good idea, since I think we should make the EL and CL as independent to each other as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its more of an EIP constant imo

| `MAX_BLOBS_PER_BLOCK` | `uint64(2**2)` (= 4) | Maximum number of blobs in a single block limited by `MAX_BLOB_COMMITMENTS_PER_BLOCK` |

## Configuration

Expand Down Expand Up @@ -96,7 +99,7 @@ class BeaconBlockBody(Container):
# Execution
execution_payload: ExecutionPayload # [Modified in Deneb]
bls_to_execution_changes: List[SignedBLSToExecutionChange, MAX_BLS_TO_EXECUTION_CHANGES]
blob_kzg_commitments: List[KZGCommitment, MAX_BLOBS_PER_BLOCK] # [New in Deneb]
blob_kzg_commitments: List[KZGCommitment, MAX_BLOB_COMMITMENTS_PER_BLOCK] # [New in Deneb]
```

#### `ExecutionPayload`
Expand Down Expand Up @@ -251,6 +254,9 @@ def process_execution_payload(state: BeaconState, payload: ExecutionPayload, exe

```python
def process_blob_kzg_commitments(body: BeaconBlockBody) -> None:
# Verify commitments are under limit
assert len(body.blob_kzg_commitments) <= MAX_BLOBS_PER_BLOCK
# Verify commitments match with corresponding blob transactions
assert verify_kzg_commitments_against_transactions(body.execution_payload.transactions, body.blob_kzg_commitments)
```

Expand Down