From 291f8e1410915e93d42672a92fb5f591a38cd27c Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Thu, 5 Dec 2024 06:16:11 +0100 Subject: [PATCH] Synchronously check all `transactions` to have non-zero length (#573) * 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. * Allow `INVALID` on invalid transaction. * Remove redundant validity check --- src/engine/paris.md | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/engine/paris.md b/src/engine/paris.md index ee914074f..a4e3b8bc3 100644 --- a/src/engine/paris.md +++ b/src/engine/paris.md @@ -161,26 +161,30 @@ The payload build process is specified as follows: #### Specification -1. Client software **MUST** validate `blockHash` value as being equivalent to `Keccak256(RLP(ExecutionBlockHeader))`, where `ExecutionBlockHeader` is the execution layer block header (the former PoW block header structure). Fields of this object are set to the corresponding payload values and constant values according to the Block structure section of [EIP-3675](https://eips.ethereum.org/EIPS/eip-3675#block-structure), extended with the corresponding section of [EIP-4399](https://eips.ethereum.org/EIPS/eip-4399#block-structure). 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. +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. -2. Client software **MAY** initiate a sync process if requisite data for payload validation is missing. Sync process is specified in the [Sync](#sync) section. +2. Client software **MUST** validate `blockHash` value as being equivalent to `Keccak256(RLP(ExecutionBlockHeader))`, where `ExecutionBlockHeader` is the execution layer block header (the former PoW block header structure). Fields of this object are set to the corresponding payload values and constant values according to the Block structure section of [EIP-3675](https://eips.ethereum.org/EIPS/eip-3675#block-structure), extended with the corresponding section of [EIP-4399](https://eips.ethereum.org/EIPS/eip-4399#block-structure). 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. -3. 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](#payload-validation) section. +3. Client software **MAY** initiate a sync process if requisite data for payload validation is missing. Sync process is specified in the [Sync](#sync) section. -4. Client software **MAY NOT** validate the payload if the payload doesn't belong to the canonical chain. +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](#payload-validation) section. -5. Client software **MUST** respond to this method call in the following way: +5. Client software **MAY NOT** validate the payload if the payload doesn't belong to the canonical chain. + +6. Client software **MUST** respond to this method call in the following way: + * `{status: INVALID, latestValidHash: null, validationError: errorMessage | null}` if `transactions` contains zero length or invalid entries * `{status: INVALID_BLOCK_HASH, latestValidHash: null, validationError: errorMessage | null}` if the `blockHash` validation has failed * `{status: INVALID, latestValidHash: 0x0000000000000000000000000000000000000000000000000000000000000000, validationError: errorMessage | null}` if terminal block conditions are not satisfied * `{status: SYNCING, latestValidHash: null, validationError: null}` if requisite data for the payload's acceptance or validation is missing * with the payload status obtained from the [Payload validation](#payload-validation) process if the payload has been fully validated while processing the call * `{status: ACCEPTED, latestValidHash: null, validationError: null}` if the following conditions are met: + - all `transactions` have non-zero length - the `blockHash` of the payload is valid - the payload doesn't extend the canonical chain - the payload hasn't been fully validated - ancestors of a payload are known and comprise a well-formed chain. -6. If any of the above fails due to errors unrelated to the normal processing flow of the method, client software **MUST** respond with an error object. +7. If any of the above fails due to errors unrelated to the normal processing flow of the method, client software **MUST** respond with an error object. ### engine_forkchoiceUpdatedV1