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

Dankrad's merge review #2607

Closed
wants to merge 2 commits into from
Closed

Dankrad's merge review #2607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5423bbc
Some small fixes
dankrad Sep 17, 2021
c18e8aa
Remove arbitrary condition
dankrad Sep 17, 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
2 changes: 1 addition & 1 deletion specs/merge/beacon-chain.md
View file
Open in desktop
Expand Up @@ -59,7 +59,7 @@ This patch adds transaction execution to the beacon chain as part of the Merge f

| Name | Value |
| - | - |
| `MAX_BYTES_PER_OPAQUE_TRANSACTION` | `uint64(2**20)` (= 1,048,576) |
| `MAX_BYTES_PER_OPAQUE_TRANSACTION` | `uint64(2**24)` (= 1,048,576) |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With 16 bytes per gas and 30M limit we can already have almost 2MB transactions now. Why limit this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think because the consensus cannot validate these TXs nor the gas limit and this puts a safe(r) limit on sanity check sizes of blocks coming off the wire.

cc @protolambda who I think will have the deeper reasoning available

Copy link
Contributor

Choose a reason for hiding this comment

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

Because this number multiplies with MAX_TRANSACTIONS_PER_PAYLOAD to be an insanely large block. The consensus-layer does not have sane-limit validations on the size of the block as it comes off the wire so a limit is introduced here in conjunction with the MAX_TRANSACTIONS_PER_PAYLOAD.

At the current values, this can still be a 16GB block. This is why the MAY is available in the p2p validation in your other comment. A 16GB block is a dos block... but so is a 1GB block.... is a 50MB block? Not sure but sane limits can be encoded into the p2p validations (static or dynamic depending on the client's choice of anti-dos measures).

Also that comment is MAY in relation to stricter size limits. Maybe we should be a bit clearer to not imply that this means random validations can be applied

Copy link
Collaborator

Choose a reason for hiding this comment

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

This also closely relates to EIP-3860, as discussed on ACD (cc @holiman): https://eips.ethereum.org/EIPS/eip-3860

Generally we should put a validation condition in place that checks if the sum of the block is not causing a DoS just by its size. Req-Resp and Gossipsub payloads in eth2 are all currently limited to 1 MB, we should increase that if valid block instances can actually be larger, to follow consensus. And then within the EVM execution the same limits apply, where EIP-3860 is also concerned with.

That said, if we handle DoS properly then I'm not opposed to set the capacity to 2**24 just for future compatibility, if the block size needs to grow without changing the SSZ type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't buy the argument that this prevents DOS blocks, as this would, as @djrtwo noticed, still allow 16 GB blocks. The right way to do prevent the DOS attack is to limit the execution payload size. For example, any execution payload over GAS_LIMIT // 16 in size is obviously invalid and can be rejected.

I don't see an argument why there needs to be any further restriction for transaction. If there is a transaction that needs 1.8 MB of data and pays for the gas I don't see a reason to exclude it, since we are accepting 2 transactions of 0.9 MB each. I can't see any DOS vector this prevents. And you need to be able to process the full block size either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

p2p spec is much simpler to change/upgrade over time. Thus @dankrad's argument is to put the dos protections there and leave this more open to theoretical maximums

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, and also I think Eth1 can already have blocks larger than 1 MB so I think we should already upgrade P2P to at least support Eth1 current max (almost 2 MB)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It also worth considering that the size of ETH protocol messages is limited to 10MB in geth https://github.com/ethereum/go-ethereum/blob/master/eth/protocols/eth/protocol.go#L49

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With rollups this is very relevant in practice, as this will be the maximum fraud proof size. So it's a pretty important quantity.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is being addressed here -- #2686

| `MAX_TRANSACTIONS_PER_PAYLOAD` | `uint64(2**14)` (= 16,384) |
| `BYTES_PER_LOGS_BLOOM` | `uint64(2**8)` (= 256) |
| `GAS_LIMIT_DENOMINATOR` | `uint64(2**10)` (= 1,024) |
Expand Down
28 changes: 7 additions & 21 deletions specs/merge/fork-choice.md
View file
Open in desktop
Expand Up @@ -43,25 +43,13 @@ The body of this function is implementation dependent.
The Consensus API may be used to implement this with an external execution engine.

```python
def set_head(self: ExecutionEngine, block_hash: Hash32) -> bool:
"""
Returns True if the ``block_hash`` was successfully set as head of the execution payload chain.
"""
...
```

#### `finalize_block`
HEAD_TYPE_FINALIZED_HEAD = uint64(0)
HEAD_TYPE_SAFE_HEAD = uint64(1)
djrtwo marked this conversation as resolved.
Show resolved Hide resolved
HEAD_TYPE_UNSAFE_HEAD = uint64(2)

Applies finality to the execution state: it irreversibly persists the chain of all execution payloads
and corresponding state, up to and including `block_hash`.

The body of this function is implementation dependent.
The Consensus API may be used to implement this with an external execution engine.

```python
def finalize_block(self: ExecutionEngine, block_hash: Hash32) -> bool:
def set_head(self: ExecutionEngine, block_hash: Hash32, head_type: unit64) -> bool:
djrtwo marked this conversation as resolved.
Show resolved Hide resolved
"""
Returns True if the data up to and including ``block_hash`` was successfully finalized.
Returns True if the ``block_hash`` was successfully set as head of the execution payload chain.
"""
...
```
Expand All @@ -71,16 +59,14 @@ def finalize_block(self: ExecutionEngine, block_hash: Hash32) -> bool:
### `TransitionStore`

```python
@dataclass
dankrad marked this conversation as resolved.
Show resolved Hide resolved
class TransitionStore(object):
class TransitionStore(Container):
terminal_total_difficulty: uint256
```

### `PowBlock`

```python
@dataclass
djrtwo marked this conversation as resolved.
Show resolved Hide resolved
class PowBlock(object):
class PowBlock(Container):
block_hash: Hash32
parent_hash: Hash32
total_difficulty: uint256
Expand Down
2 changes: 0 additions & 2 deletions specs/merge/p2p-interface.md
View file
Open in desktop
Expand Up @@ -78,8 +78,6 @@ Alias `block = signed_beacon_block.message`, `execution_payload = block.body.exe
-- i.e. `execution_payload.timestamp == compute_time_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 block hash is not equal to the parent hash --
djrtwo marked this conversation as resolved.
Show resolved Hide resolved
i.e. `execution_payload.block_hash != execution_payload.parent_hash`.
- _[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.
Expand Down