From e1eaa7ffa194f70f044970689b11b27c9d68cb66 Mon Sep 17 00:00:00 2001
From: Etan Kissling <etan@nimbus.team>
Date: Wed, 14 Aug 2024 22:55:05 +0200
Subject: [PATCH] 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.
---
 specs/bellatrix/beacon-chain.md | 11 +++++++++--
 specs/deneb/beacon-chain.md     |  4 ++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/specs/bellatrix/beacon-chain.md b/specs/bellatrix/beacon-chain.md
index 6f67be96a..51d570fe2 100644
--- a/specs/bellatrix/beacon-chain.md
+++ b/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
 ```
 
diff --git a/specs/deneb/beacon-chain.md b/specs/deneb/beacon-chain.md
index 0f6a8fc07..1b7a322c2 100644
--- a/specs/deneb/beacon-chain.md
+++ b/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