Synchronously check all transactions to have non-zero length

[etan-status]

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.

# 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.

[jtraglia]

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.

[ppopth]

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.

[jtraglia]

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?

[etan-status]

Yep, verify_and_notify_new_payload is a mock implementation for the minimal EL checks that have to be done.

• Synchronously check all transactions to have non-zero length execution-apis#573

This is the matching spec that describes the additional check for zero length transactions.

[jtraglia]

LGTM 👍 thanks Etan!

[ppopth]

Since verify_and_notify_new_payload is an EL function rather than the CL, I don't think what is specified in the consensus-specs has to be exactly the same as what is done in the EL. I think we have this function here only to roughly show what is verified.

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?

[etan-status]

verify_and_notify_new_payload represents all necessary checks that must be done at the bare minimum, even when the EL has not synced any state. The empty transaction check is one of them.

Note that certain CL implementations support syncing without a connected EL for maintenance use cases, and if engine_newPayload is skipped, then the CL itself has to perform these checks. It is very useful for such implementation to have an exhaustive list of checks that must be done, i.e., to have verify_and_notify_new_payload.

[jtraglia]

I've decided to push this to a later release. We need more folks to review this.

[jtraglia]

Had a meeting with @hwwhww and we agreed to merge this one & include it in the upcoming release.