check blob versioned hashes when no EL is connected #6501
Conversation
When no EL is conencted, we have to at the very least ensure that the data in the beacon block is consistent with the execution payload. We already do this for the block hash, but also have to do it for the `blob_kzg_commitments`. To validate that they are linked with the execution payload, we have to RLP decode all EIP-4844 blob transactions and compare their blob versioned hashes with the hashed commitments.
|
Manually tested with a log on successful verification on sepolia. Meaningful automated tests need a sufficiently mature EL implementation. |
beacon_chain/spec/helpers_el.nim
Outdated
| if txBytes.len == 0 or txBytes[0] != TxEip4844.byte: | ||
| continue # Only blob transactions may have blobs | ||
| let tx = ? txBytes.readExecutionTransaction() | ||
| for vHash in tx.versionedHashes: |
There was a problem hiding this comment.
Is the intent for this loop to return err("No blob transaction allowed before Deneb") if and only if tx.versionedHashes.len > 0/txBytes.readExecutionTransaction().versionedHashes.len > 0? The loop as such is pointless and vHash isn't ever referenced.
There was a problem hiding this comment.
yep, can be replaced with a check on length > 0, done so — 289ce0b
beacon_chain/spec/helpers_el.nim
Outdated
| template transactions: untyped = blck.body.execution_payload.transactions | ||
|
|
||
| for txBytes in transactions: | ||
| if txBytes.len == 0 or txBytes[0] != TxEip4844.byte: |
There was a problem hiding this comment.
Is it sufficient to check whether either of these conditions holds, and if so, reject the transactions in general?
To the extent that it's true that is_valid_versioned_hashes probably should not run the other checks (0-length tx, etc) and is a special-purpose thing, another way of enforcing its only applying to Deneb and later is_valid_versioned_hashes only taking deneb.BeaconBlock and electra.BeaconBlock -- that would at least clarify that this function isn't really suited to Bellatrix/Capella. Otherwise about half of the code in this function is working around its not-very-useful promise to function on pre-Deneb blocks.
The entire Bellatrix/Capella section is kind of wonky, and the pre-Bellatrix section does nothing at all (in that case, appropriately).
There was a problem hiding this comment.
Thanks to the union type, there is no real type information we can use as to when what tx type becomes available. So, as far as this validation logic is concerned, there could be type 3 tx also pre Deneb. The goal is only to validate blob versioned hashes (and nothing else), so from Deneb onward it extracts the versioned hashes and validates them against `blob_kzg_commitments), and pre Deneb it just checks that there is no TX inside that has versioned hashes (in case that a type 3 tx is still entered into the list -- remember it's union so thanks to that we don't have typing). Pre Bellatrix we also don't have a payload so there's no RLP data to begin with and the check passes implicitly.
The txBytes.len == 0 or txBytes[0] != TxEip4844.byte check is actually not needed, but it ensures that the janky nimbus-eth1 code is only run on blob transactions (and everything else is assumed to not contain versioned hashes).
There was a problem hiding this comment.
As written, the function is as defensively written as possible and ensures to only inspect blob transactions, without any validity assumptions or EL spec enforcements beyond what's strictly necessary to check the blob versioned hashes.
| # https://github.com/ethereum/execution-apis/blob/main/src/engine/experimental/blob-extension.md#specification | ||
| # "This validation MUST be instantly run in all cases even during active | ||
| # sync process." | ||
| let blobsRes = signedBlock.message.is_valid_versioned_hashes |
There was a problem hiding this comment.
Regarding https://github.com/status-im/nimbus-eth2/pull/6501/files#r1724809115 is_valid_versioned_hashes being called only here, the caller is defined to only matter in the case of Deneb or newer, so if the Bellatrix/Capella logic ever runs, that's already effectively a bug.
There was a problem hiding this comment.
Pre-Deneb consensus specs do not require this check.
There was a problem hiding this comment.
ok, have added the deneb check at the caller and added a comment that describes the case that used to be handled here.
There was a problem hiding this comment.
it's probably alright, as the transactions are covered by the block_hash (and we have the extra check that filters the [] case). so, if there happens to be a type 3 transaction pre deneb, it would not be possible to build a valid history on top of that. and because there are no blob_kzg_commitments in the beacon block, there's nothing that can get out of sync. so, post deneb only is good enough.
When no EL is conencted, we have to at the very least ensure that the data in the beacon block is consistent with the execution payload. We already do this for the block hash, but also have to do it for the
blob_kzg_commitments. To validate that they are linked with the execution payload, we have to RLP decode all EIP-4844 blob transactions and compare their blob versioned hashes with the hashed commitments.