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

Jump to bottom

fix gossip and tx size limits for the merge #2686

Merged
merged 4 commits into from Oct 28, 2021
Merged

fix gossip and tx size limits for the merge #2686

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e320da
fix gossip and tx size limits for the merge
djrtwo Oct 20, 2021
dae5b87
increase TX sizes to account for very high gas limits
djrtwo Oct 21, 2021
bda07e1
remove extraneous gossip condition
djrtwo Oct 26, 2021
d58b1c5
bump up MAX_CHUNK_SIZE at the merge as well
djrtwo Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions presets/mainnet/merge.yaml
View file
Open in desktop
Expand Up @@ -2,10 +2,10 @@

# Execution
# ---------------------------------------------------------------
# 2**30 (= 1,073,741,824)
MAX_BYTES_PER_TRANSACTION: 1073741824
# 2**20 (= 1,048,576)
MAX_BYTES_PER_TRANSACTION: 1048576
# 2**14 (= 16,384)
MAX_TRANSACTIONS_PER_PAYLOAD: 16384
MAX_TRANSACTIONS_PER_PAYLOAD: 1048576
# 2**8 (= 256)
BYTES_PER_LOGS_BLOOM: 256
# 2**10 (= 1,024)
Expand Down
6 changes: 3 additions & 3 deletions presets/minimal/merge.yaml
View file
Open in desktop
Expand Up @@ -2,10 +2,10 @@

# Execution
# ---------------------------------------------------------------
# 2**30 (= 1,073,741,824)
MAX_BYTES_PER_TRANSACTION: 1073741824
# 2**20 (= 1,048,576)
MAX_BYTES_PER_TRANSACTION: 1048576
# 2**14 (= 16,384)
MAX_TRANSACTIONS_PER_PAYLOAD: 16384
MAX_TRANSACTIONS_PER_PAYLOAD: 1048576
# 2**8 (= 256)
BYTES_PER_LOGS_BLOOM: 256
# 2**10 (= 1,024)
Expand Down
4 changes: 2 additions & 2 deletions specs/merge/beacon-chain.md
View file
Open in desktop
Expand Up @@ -59,8 +59,8 @@ This patch adds transaction execution to the beacon chain as part of the Merge f

| Name | Value |
| - | - |
| `MAX_BYTES_PER_TRANSACTION` | `uint64(2**20)` (= 1,048,576) |
| `MAX_TRANSACTIONS_PER_PAYLOAD` | `uint64(2**14)` (= 16,384) |
| `MAX_BYTES_PER_TRANSACTION` | `uint64(2**30)` (= 1,073,741,824) |
| `MAX_TRANSACTIONS_PER_PAYLOAD` | `uint64(2**20)` (= 1,048,576) |
| `BYTES_PER_LOGS_BLOOM` | `uint64(2**8)` (= 256) |
| `GAS_LIMIT_DENOMINATOR` | `uint64(2**10)` (= 1,024) |
| `MIN_GAS_LIMIT` | `uint64(5000)` (= 5,000) |
Expand Down
49 changes: 45 additions & 4 deletions specs/merge/p2p-interface.md
View file
Open in desktop
Expand Up @@ -14,6 +14,7 @@ Readers should understand the Phase 0 and Altair documents and use them as a bas

- [Warning](#warning)
- [Modifications in the Merge](#modifications-in-the-merge)
- [Configuration](#configuration)
- [The gossip domain: gossipsub](#the-gossip-domain-gossipsub)
- [Topics and messages](#topics-and-messages)
- [Global topics](#global-topics)
Expand All @@ -23,6 +24,9 @@ Readers should understand the Phase 0 and Altair documents and use them as a bas
- [Messages](#messages)
- [BeaconBlocksByRange v2](#beaconblocksbyrange-v2)
- [BeaconBlocksByRoot v2](#beaconblocksbyroot-v2)
- [Design decision rationale](#design-decision-rationale)
- [Gossipsub](#gossipsub)
- [Why was the max gossip message size increased at the Merge?](#why-was-the-max-gossip-message-size-increased-at-the-merge)

<!-- END doctoc generated TOC please keep comment here to allow auto update -->
<!-- /TOC -->
Expand All @@ -34,6 +38,14 @@ Refer to the note in the [validator guide](./validator.md) for further details.

# Modifications in the Merge

## Configuration

This section outlines modifications constants that are used in this spec.

| Name | Value | Description |
|---|---|---|
| `GOSSIP_MAX_SIZE_MERGE` | `10 * 2**20` (= 10,485,760, 10 MiB) | The maximum allowed size of uncompressed gossip messages starting at the Merge upgrade |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why should we have a new configurable setting (i.e. have a _MERGE suffix here) instead of just overwriting already existing one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is generally how we handle configuration value updates to be able to handle and reason about fork boundaries more simply

See discussion in config README -- https://github.com/ethereum/consensus-specs/tree/dev/configs#forking


## The gossip domain: gossipsub

Some gossip meshes are upgraded in the Merge to support upgraded types.
Expand All @@ -43,7 +55,12 @@ Some gossip meshes are upgraded in the Merge to support upgraded types.
Topics follow the same specification as in prior upgrades.
All topics remain stable except the beacon block topic which is updated with the modified type.

The specification around the creation, validation, and dissemination of messages has not changed from the Phase 0 and Altair documents.
The specification around the creation, validation, and dissemination of messages has not changed from the Phase 0 and Altair documents unless explicitly noted here.

Starting at the Merge upgrade, each gossipsub [message](https://github.com/libp2p/go-libp2p-pubsub/blob/master/pb/rpc.proto#L17-L24)
has a maximum size of `GOSSIP_MAX_SIZE_MERGE`.
Clients MUST reject (fail validation) messages that are over this size limit.
Likewise, clients MUST NOT emit or propagate messages larger than this limit.

The derivation of the `message-id` remains stable.

Expand Down Expand Up @@ -74,9 +91,6 @@ Alias `block = signed_beacon_block.message`, `execution_payload = block.body.exe
-- i.e. `execution_payload.timestamp == compute_timestamp_at_slot(state, block.slot)`.
- _[REJECT]_ Gas used is less than the gas limit --
i.e. `execution_payload.gas_used <= execution_payload.gas_limit`.
- _[REJECT]_ The execution payload transaction list data is within expected size limits,
the data MUST NOT be larger than the SSZ list-limit,
and a client MAY be more strict.

*Note*: Additional [gossip validations](https://github.com/ethereum/devp2p/blob/master/caps/eth.md#block-encoding-and-validity)
(see block "data validity" conditions) that rely more heavily on execution-layer state and logic are currently under consideration.
Expand Down Expand Up @@ -123,3 +137,30 @@ Per `context = compute_fork_digest(fork_version, genesis_validators_root)`:
| `GENESIS_FORK_VERSION` | `phase0.SignedBeaconBlock` |
| `ALTAIR_FORK_VERSION` | `altair.SignedBeaconBlock` |
| `MERGE_FORK_VERSION` | `merge.SignedBeaconBlock` |

# Design decision rationale

## Gossipsub

### Why was the max gossip message size increased at the Merge?

With the addition of `ExecutionPayload` to `BeaconBlock`s, there is a dynamic
field -- `transactions` -- which can validly exceed the `GOSSIP_MAX_SIZE` limit (1 MiB) put in place in
place at Phase 0. At the `GAS_LIMIT` (~30M) currently seen on mainnet in 2021, a single transaction
filled entirely with data at a cost of 16 gas per byte can create a valid
`ExecutionPayload` of ~2 MiB. Thus we need a size limit to at least account for
current mainnet conditions.

Geth currently has a [max gossip message size](https://github.com/ethereum/go-ethereum/blob/3ce9f6d96f38712f5d6756e97b59ccc20cc403b3/eth/protocols/eth/protocol.go#L49) of 10 MiB.
To support backward compatibility with this previously defined network limit,
we adopt `GOSSIP_MAX_SIZE_MERGE` of 10 MiB for maximum gossip sizes at the
point of the Merge and beyond. Note, that clients SHOULD still reject objects
that exceed their maximum theoretical bounds which in most cases is less than `GOSSIP_MAX_SIZE_MERGE`.

Note, that due to additional size induced by the `BeaconBlock` contents (e.g.
proposer signature, operations lists, etc) this does reduce the
theoretical max valid `ExecutionPayload` (and `transactions` list) size as
slightly lower than 10 MiB. Considering that `BeaconBlock` max size is on the
order of 128 KiB in the worst case and the current gas limit (~30M) bounds max blocksize to less
than 2 MiB today, this marginal difference in theoretical bounds will have zero
impact on network functionality and security.