Synchronously check all transactions to have non-zero length

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`.

```python
# 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.

Author: etan-status

specs/bellatrix/beacon-chain.md

@@ -355,10 +355,17 @@ def verify_and_notify_new_payload(self: ExecutionEngine,
     """
     Return ``True`` if and only if ``new_payload_request`` is valid with respect to ``self.execution_state``.
     """
-    if not self.is_valid_block_hash(new_payload_request.execution_payload):
+    execution_payload = new_payload_request.execution_payload
+
+    if b'' in execution_payload.transactions:
         return False
-    if not self.notify_new_payload(new_payload_request.execution_payload):
+
+    if not self.is_valid_block_hash(execution_payload):
         return False
+
+    if not self.notify_new_payload(execution_payload):
+        return False
+
     return True
 ```
 

specs/deneb/beacon-chain.md

@@ -293,9 +293,13 @@ def verify_and_notify_new_payload(self: ExecutionEngine,
     """
     Return ``True`` if and only if ``new_payload_request`` is valid with respect to ``self.execution_state``.
     """
+    # [Modified in Deneb:EIP4788]
     execution_payload = new_payload_request.execution_payload
     parent_beacon_block_root = new_payload_request.parent_beacon_block_root
 
+    if b'' in execution_payload.transactions:
+        return False
+
     # [Modified in Deneb:EIP4788]
     if not self.is_valid_block_hash(execution_payload, parent_beacon_block_root):
         return False