Skip to content

Commit

Permalink
Synchronously check all transactions to have non-zero length (#573)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
etan-status authored Dec 5, 2024
1 parent 9707339 commit 291f8e1
Showing 1 changed file with 10 additions and 6 deletions.
16 changes: 10 additions & 6 deletions src/engine/paris.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit 291f8e1

Please sign in to comment.