Synchronously check all transactions to have non-zero length#3885
Conversation
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.
Reject blocks with zero length transactions early when no EL connected. - ethereum/consensus-specs#3885
Reject blocks with zero length transactions early when no EL connected. - ethereum/consensus-specs#3885
f29161c to
c746890
Compare
| if b'' in execution_payload.transactions: | ||
| return False |
There was a problem hiding this comment.
I think I understand the implications of the PR and agree that zero-length transactions should be rejected, but I'm not certain that verify_and_notify_new_payload is best location for such a check. I feel like notify_new_payload via the Engine API should return false if it sees this.
There was a problem hiding this comment.
I think notify_new_payload is not an Engine API thing anymore. In fact, verify_and_notify_new_payload is an Engine API. AFAIU, the reason that we have the logic of verify_and_notify_new_payload in the consensus-specs is because we want to show what the function does, but it doesn't mean that it's an CL thing.
So I think putting it in verify_and_notify_new_payload already makes sense because it's a "verify" thing.
There was a problem hiding this comment.
Hmm okay, well in that case I guess it doesn't hurt to add. Let's do it!
@etan-status could you merge in dev & make similar changes to electra?
There was a problem hiding this comment.
Yep, verify_and_notify_new_payload is a mock implementation for the minimal EL checks that have to be done.
This is the matching spec that describes the additional check for zero length transactions.
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
|
Since If this check is not relevant to the CL, I don't think we should include it in the spec since it will distract CL researchers. Do you think this check is relevant to the CL? I feel that this issue is related to MPT and it's very opaque to the CL and the CL researchers have to dig deeply why we need this check which I think they don't need to. What do you think? |
|
Note that certain CL implementations support syncing without a connected EL for maintenance use cases, and if |
|
I've decided to push this to a later release. We need more folks to review this. |
|
Had a meeting with @hwwhww and we agreed to merge this one & include it in the upcoming release. |
As part of
newPayloadblock hash verification, thetransactionsRootis 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 withtransactionscontaining one or more zero-length transactions, that such transactions will effectively be skipped when computing thetransactionsRoot. Note thattransactionsare opaque to the CL and zero-length transactions are not filtered out beforenewPayload.In any case, the
blockHashvalidation may still succeed, resulting in a potentialSYNCING/ACCEPTEDresult tonewPayloadby 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
transactionslist: In the trivial case, a block with zero transactions has the sametransactionsRoot(andblockHash) as one of a block with one[]transaction (as that one is skipped).This means that the same
blockHashcan refer to a valid block (without extra[]transactions added), but also can refer to an invalid block. BecauseforkchoiceUpdatedrefers to blocks byblockHash, outcome may be nondeterministic and implementation dependent. IfforkchoiceUpdateddeems theblockHashto refer to aVALIDobject (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 itsExecutionPayload, risking finalizing it.The problem can be avoided by returning
INVALIDinnewPayloadif there are any zero-lengthtransactionsentries, preventing optimistic import of such blocks by the CL.