From e1eaa7ffa194f70f044970689b11b27c9d68cb66 Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Wed, 14 Aug 2024 22:55:05 +0200 Subject: [PATCH 1/4] 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 6f67be96a7..51d570fe2d 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 0f6a8fc076..1b7a322c2f 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 From a6095bf498243dfd2619a996c5c9f9fcbf79cfdc Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Mon, 21 Oct 2024 13:00:10 +0200 Subject: [PATCH 2/4] Update specs/deneb/beacon-chain.md Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com> --- specs/deneb/beacon-chain.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/specs/deneb/beacon-chain.md b/specs/deneb/beacon-chain.md index 1b7a322c2f..98a4b05bda 100644 --- a/specs/deneb/beacon-chain.md +++ b/specs/deneb/beacon-chain.md @@ -293,9 +293,8 @@ 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 + parent_beacon_block_root = new_payload_request.parent_beacon_block_root # [Modified in Deneb:EIP4788] if b'' in execution_payload.transactions: return False From 44d5a1b49bffbddc79f9a49ca4245c7ef0e7c693 Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Mon, 21 Oct 2024 13:01:30 +0200 Subject: [PATCH 3/4] Sync changes to Electra --- specs/electra/beacon-chain.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/specs/electra/beacon-chain.md b/specs/electra/beacon-chain.md index 0c56491907..89575d671c 100644 --- a/specs/electra/beacon-chain.md +++ b/specs/electra/beacon-chain.md @@ -1015,6 +1015,9 @@ def verify_and_notify_new_payload(self: ExecutionEngine, parent_beacon_block_root = new_payload_request.parent_beacon_block_root execution_requests_list = get_execution_requests_list(new_payload_request.execution_requests) # [New in Electra] + if b'' in execution_payload.transactions: + return False + if not self.is_valid_block_hash(execution_payload, parent_beacon_block_root): return False From 0f964b04d288433fdf096e223031bd3e73a09807 Mon Sep 17 00:00:00 2001 From: Justin Traglia <95511699+jtraglia@users.noreply.github.com> Date: Mon, 21 Oct 2024 08:43:25 -0500 Subject: [PATCH 4/4] Change "modified" to "new" --- specs/deneb/beacon-chain.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/deneb/beacon-chain.md b/specs/deneb/beacon-chain.md index 98a4b05bda..1025af7b61 100644 --- a/specs/deneb/beacon-chain.md +++ b/specs/deneb/beacon-chain.md @@ -294,7 +294,7 @@ 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``. """ execution_payload = new_payload_request.execution_payload - parent_beacon_block_root = new_payload_request.parent_beacon_block_root # [Modified in Deneb:EIP4788] + parent_beacon_block_root = new_payload_request.parent_beacon_block_root # [New in Deneb:EIP4788] if b'' in execution_payload.transactions: return False