From 15bc86ea434145a17a0ec1aa3253b2d994607a35 Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Wed, 14 Aug 2024 15:36:07 +0200 Subject: [PATCH 1/2] 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. --- 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 495e1db1..3f00a379 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 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 From 180eb4cb841d960e4747f8b296cd020130d7d84b Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Wed, 14 Aug 2024 19:30:49 +0200 Subject: [PATCH 2/2] Allow `INVALID` on invalid transaction. --- src/engine/paris.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/engine/paris.md b/src/engine/paris.md index 3f00a379..030df203 100644 --- a/src/engine/paris.md +++ b/src/engine/paris.md @@ -161,7 +161,7 @@ The payload build process is specified as follows: #### Specification -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. +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. 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. @@ -172,7 +172,7 @@ The payload build process is specified as follows: 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 entries + * `{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