Synchronously check all transactions to have non-zero length

[etan-status]

As part of newPayload block hash verification, the transactionsRoot is computed by the EL. Because Merkle-Patricia Tries cannot contain [] entries, MPT implementations typically treat setting a key to [] as deleting the entry for the key. This means that if a CL receives a block with transactions containing one or more zero-length transactions, that such transactions will effectively be skipped when computing the transactionsRoot. Note that transactions are opaque to the CL and zero-length transactions are not filtered out before newPayload.

# https://eips.ethereum.org/EIPS/eip-2718
def compute_trie_root_from_indexed_data(data):
    """
    Computes the root hash of `patriciaTrie(rlp(Index) => Data)` for a data array.
    """
    t = HexaryTrie(db={})
    for i, obj in enumerate(data):
        k = encode(i, big_endian_int)
        t.set(k, obj)  # Implicitly skipped if `obj == b''` (invalid RLP)
    return t.root_hash

In any case, the blockHash validation may still succeed, resulting in a potential SYNCING/ACCEPTED result to newPayload by spec.

Note, however, that there is an effective hash collision if a payload is modified by appending one or more zero-length transactions to the end of transactions list: In the trivial case, a block with zero transactions has the same transactionsRoot (and blockHash) as one of a block with one [] transaction (as that one is skipped).

This means that the same blockHash can refer to a valid block (without extra [] transactions added), but also can refer to an invalid block. Because forkchoiceUpdated refers to blocks by blockHash, outcome may be nondeterministic and implementation dependent. If forkchoiceUpdated deems the blockHash to refer to a VALID object (obtained from a src that does not have the extra [] transactions, e.g., devp2p), then this could result in honest attestations to a CL beacon block with invalid [] transactions in its ExecutionPayload, risking finalizing it.

The problem can be avoided by returning INVALID in newPayload if there are any zero-length transactions entries, preventing optimistic import of such blocks by the CL.

[etan-status]

• Synchronously check all transactions to have non-zero length consensus-specs#3885

[mkalinin]

Why MAY in the second case? Can we change it to MUST as likely ELs run this validation either before or during the block hash computations, or not?

Suggested change

	1. Client software **MUST** validate that all `transactions` have non-zero length (at least 1 byte). Client software **MUST** run this validation in all cases even if this branch or any other branches of the block tree are in an active sync process. Client software **MAY** employ stricter checks and validate that all `transactions` are fully valid.
	1. Client software **MUST** validate that all `transactions` are encoded correctly and have non-zero length (at least 1 byte). Client software **MUST** run this validation in all cases even if this branch or any other branches of the block tree are in an active sync process.

[etan-status]

For the purpose of computing a unique block hash that's not affected by hash collisions, it's fine if a transaction is malformed as long as it is not empty.

MAY wording enables CL implementations to also perform the checks in situation where the EL is unavailable (maintenance etc); making this a MUST requires knowledge of individual transaction types.

[mkalinin]

MAY introduces ambiguity as transactions encoding may be run synchronously or may be not depending on the state of the node and EL client implementation

[etan-status]

It's the same ambuity that's already present with steps (4) and (5). But arguably, the stricter transaction validation check is already covered by those. As in, if a transaction doesn't parse, it will fail in step (4). And step (5) already allows deferring validation until fcU. Will remove it from here. Only the empty-check has to be done always and must be synchronous, as in, there has to be an INVALID on newPayload if a tx is empty, so that there is no chance that allows the CL to optimistically import it and request an ambiguous fcu lateron.

4. Client software MUST validate the payload if it extends the canonical chain and requisite data for the validation is locally available. The validation process is specified in the Payload validation section.

5. Client software MAY NOT validate the payload if the payload doesn't belong to the canonical chain.

[mkalinin]

This “ambiguity" is necessary to handle different EL client designs

[mkalinin]

It might make sense to have a Hive test covering the new check introduced by this PR, cc @marioevz