Dankrad's merge review #2607
Dankrad's merge review #2607
Conversation
| @@ -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) | | |||
There was a problem hiding this comment.
With 16 bytes per gas and 30M limit we can already have almost 2MB transactions now. Why limit this?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
With rollups this is very relevant in practice, as this will be the maximum fraud proof size. So it's a pretty important quantity.
There was a problem hiding this comment.
This is being addressed here -- #2686
| @@ -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) | | |||
There was a problem hiding this comment.
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
| @@ -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) | | |||
There was a problem hiding this comment.
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
|
Thank you for the review! Suggestions have made their way into other PRs. Closing this one |
No description provided.