add base merge p2p spec

[djrtwo]

[wait for #2530 to integrate merge p2p into slightly modified build process]

todo:

• consider any gossip validations for pre-merge execution payload (e.g. force empty)
• consider any validations for execution payload correctness -- probably no beyond just being well-formed and the existing dos resistance validations. actually computing the correctness of block will result in much higher gossip times and increased orphan and attestation head incorrect rates

[protolambda]

I think we can introduce additional gossip topic validation steps, to filter bad execution-payloads, without requiring any execution-layer storage:

• [reject] Check gas_used <= gas_limit
• [reject] Check gas_limit <= GAS_SANITY_LIMIT
• [reject] timestamp != genesis_time + slot * SECONDS_PER_SLOT
• [reject] block_hash != parent_hash
• [reject] Check transactions data size

And if we want to track the execution chain history (or maybe just a cache of the latest hashes and numbers?)

• [ignore] Check parent_hash is known
• [reject] Check block_number == previous_block_number + 1

[djrtwo]

yeah, good call. also checking what other low-cost validations geth does today

[djrtwo]

The current devp2p gossip validations can be found here -- https://github.com/ethereum/devp2p/blob/master/caps/eth.md#block-encoding-and-validity

All cheap checks should be replicated here as much as is possible. Ideally, we do not have to add a validateHeader call to the execution-engine. I would argue it's even okay for a "cheap" condition or two to be dropped if it is too difficult for the consensus layer to validate. The proposer signature (like the proof of work) is the most important initial validation. All others are nice-to-have

[djrtwo]

Check parent_hash is known

comes trivially from knowing and fully processing the beacon_block parent. Will keep that embedded in that validation.

[reject] Check block_number == previous_block_number + 1

going to avoid adding state/cache here for now

[mkalinin]

We should consider rebasing the Merge spec with London as well as with Altair to incorporate new checks introduced in London into state transition and p2p interface.

consider any gossip validations for pre-merge execution payload (e.g. force empty)

To get this done properly clients would need to check if the payload refers to a terminal PoW block which might be too heavy to be included in a block gossip checks.

consider any validations for execution payload correctness -- probably no beyond just being well-formed and the existing dos resistance validations

Clients won't be able to fully check the block format without a corresponding request to the execution-layer, this external dependency might hinder fast block propagation. Another potential argument against introducing such a request would be the willingness of supplanting the existing format of execution block with the one that is beacon chain friendly in the future which would allow more checks on the consensus-layer side. Also, receiving malformed block with correct proposer's signature looks more like a result of software bug rather than DoS attempt and proposer will eventually be punished by not receiving proposer's reward for it. I think we should aim for as many checks as we can do with the data that is accessible to consensus-layer without introduction of external dependency.

Some of the checks are meaningful only if is_merge_complete(state) returns true which requires querying over pre-block state. Is this state accessible to the block gossip validator in current implementations? Even if it is it might make sense to introduce this kind of checks in the fork next to the merge where after a clean up of transition logic these sanity checks may unconditionally be applied to every block.

[mkalinin]

Some of the checks are meaningful only if is_merge_complete(state) returns true which requires querying over pre-block state. Is this state accessible to the block gossip validator in current implementations? Even if it is it might make sense to introduce this kind of checks in the fork next to the merge where after a clean up of transition logic these sanity checks may unconditionally be applied to every block.

A thought. Looks like parent beacon block is available to the block gossip validator and instead of is_merge_complete(state) it may use parent_block.body.execution_payload != ExecutionPayload() condition to trigger additional checks that involve parent block's payload.

[djrtwo]

We should consider rebasing the Merge spec with London as well as with Altair

absolutely agreed! Let's handle this in a separate PR to ease review

To get this done properly clients would need to check if the payload refers to a terminal PoW block which might be too heavy to be included in a block gossip checks.

Right because it is wrt total_difficultly which would require a call to the execution-engine

I think we should aim for as many checks as we can do with the data that is accessible to consensus-layer without introduction of external dependency.

By default, I agree. Most checks beyond the PoW (or proposer signature) are "nice to have" in a sense. Let's do a full evaluation of https://github.com/ethereum/devp2p/blob/master/caps/eth.md#block-encoding-and-validity and make a call on the right balance

Conditional on parent execution_payload seems reasonable to me

[djrtwo]

@mkalinin It's actually more expected for the client to have state than to have the full parent_block on hand. thus I think the is_merge_complete and other merge/execution related predicates are appropriate here.

[djrtwo]

@protolambda @mkalinin I added some of the simple execution payload validations (conditional upon merge status and execution payload status) that proto suggested, and deferred anything that was stateful or required too much logic from the execution-layer directly.

We can debate/discuss more beyond this initial PR

ready for final review

[mkalinin]

Looks good to me! 👍
I don't see value in checking that block_hash doesn't equal to parent_hash.

[djrtwo]

I don't see value in checking that block_hash doesn't equal to parent_hash

the only reason is it's trivial to check so "why not"

leaving in for now. we can debate in another context. certainly not critical either way