Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use engine_newPayloadV3 to pass versioned_hashes to EL for validation #3359

Merged
merged 16 commits into from
May 24, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ def get_pow_chain_head() -> PowBlock:

class NoopExecutionEngine(ExecutionEngine):

def notify_new_payload(self: ExecutionEngine, execution_payload: ExecutionPayload) -> bool:
def notify_new_payload(self: ExecutionEngine, new_payload_request: NewPayloadRequest) -> bool:
return True

def notify_forkchoice_updated(self: ExecutionEngine,
Expand Down
10 changes: 6 additions & 4 deletions specs/_features/eip6110/beacon-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -178,12 +178,11 @@ def process_block(state: BeaconState, block: BeaconBlock) -> None:
process_block_header(state, block)
if is_execution_enabled(state, block.body):
process_withdrawals(state, block.body.execution_payload)
process_execution_payload(state, block.body.execution_payload, EXECUTION_ENGINE) # [Modified in EIP6110]
process_execution_payload(state, block.body, EXECUTION_ENGINE) # [Modified in EIP6110]
process_randao(state, block.body)
process_eth1_data(state, block.body)
process_operations(state, block.body) # [Modified in EIP6110]
process_sync_aggregate(state, block.body.sync_aggregate)
process_blob_kzg_commitments(block.body)
```

#### Modified `process_operations`
Expand Down Expand Up @@ -238,7 +237,9 @@ def process_deposit_receipt(state: BeaconState, deposit_receipt: DepositReceipt)
*Note*: The function `process_execution_payload` is modified to use the new `ExecutionPayloadHeader` type.

```python
def process_execution_payload(state: BeaconState, payload: ExecutionPayload, execution_engine: ExecutionEngine) -> None:
def process_execution_payload(state: BeaconState, body: BeaconBlockBody, execution_engine: ExecutionEngine) -> None:
payload = body.execution_payload

# Verify consistency of the parent hash with respect to the previous execution payload header
if is_merge_transition_complete(state):
assert payload.parent_hash == state.latest_execution_payload_header.block_hash
Expand All @@ -247,7 +248,8 @@ def process_execution_payload(state: BeaconState, payload: ExecutionPayload, exe
# Verify timestamp
assert payload.timestamp == compute_timestamp_at_slot(state, state.slot)
# Verify the execution payload is valid
assert execution_engine.notify_new_payload(payload)
versioned_hashes = [kzg_commitment_to_versioned_hash(commitment) for commitment in body.blob_kzg_commitments]
assert execution_engine.notify_new_payload(NewPayloadRequest(execution_payload=payload))
# Cache execution payload header
state.latest_execution_payload_header = ExecutionPayloadHeader(
parent_hash=payload.parent_hash,
Expand Down
25 changes: 21 additions & 4 deletions specs/bellatrix/beacon-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
- [Modified `slash_validator`](#modified-slash_validator)
- [Beacon chain state transition function](#beacon-chain-state-transition-function)
- [Execution engine](#execution-engine)
- [Request data](#request-data)
- [`NewPayloadRequest`](#newpayloadrequest)
- [Engine APIs](#engine-apis)
- [`notify_new_payload`](#notify_new_payload)
- [Block processing](#block-processing)
- [Execution payload](#execution-payload)
Expand Down Expand Up @@ -300,6 +303,18 @@ def slash_validator(state: BeaconState,

### Execution engine

#### Request data

##### `NewPayloadRequest`

```python
@dataclass
class NewPayloadRequest(object):
execution_payload: ExecutionPayload
```

#### Engine APIs

The implementation-dependent `ExecutionEngine` protocol encapsulates the execution sub-system logic via:

* a state object `self.execution_state` of type `ExecutionState`
Expand All @@ -313,7 +328,7 @@ The Engine API may be used to implement this and similarly defined functions via
#### `notify_new_payload`

```python
def notify_new_payload(self: ExecutionEngine, execution_payload: ExecutionPayload) -> bool:
def notify_new_payload(self: ExecutionEngine, new_payload_request: NewPayloadRequest) -> bool:
"""
Return ``True`` if and only if ``execution_payload`` is valid with respect to ``self.execution_state``.
"""
Expand All @@ -328,7 +343,7 @@ def notify_new_payload(self: ExecutionEngine, execution_payload: ExecutionPayloa
def process_block(state: BeaconState, block: BeaconBlock) -> None:
process_block_header(state, block)
if is_execution_enabled(state, block.body):
process_execution_payload(state, block.body.execution_payload, EXECUTION_ENGINE) # [New in Bellatrix]
process_execution_payload(state, block.body, EXECUTION_ENGINE) # [New in Bellatrix]
process_randao(state, block.body)
process_eth1_data(state, block.body)
process_operations(state, block.body)
Expand All @@ -340,7 +355,9 @@ def process_block(state: BeaconState, block: BeaconBlock) -> None:
##### `process_execution_payload`

```python
def process_execution_payload(state: BeaconState, payload: ExecutionPayload, execution_engine: ExecutionEngine) -> None:
def process_execution_payload(state: BeaconState, body: BeaconBlockBody, execution_engine: ExecutionEngine) -> None:
payload = body.execution_payload

# Verify consistency of the parent hash with respect to the previous execution payload header
if is_merge_transition_complete(state):
assert payload.parent_hash == state.latest_execution_payload_header.block_hash
Expand All @@ -349,7 +366,7 @@ def process_execution_payload(state: BeaconState, payload: ExecutionPayload, exe
# Verify timestamp
assert payload.timestamp == compute_timestamp_at_slot(state, state.slot)
# Verify the execution payload is valid
assert execution_engine.notify_new_payload(payload)
assert execution_engine.notify_new_payload(NewPayloadRequest(execution_payload=payload))
# Cache execution payload header
state.latest_execution_payload_header = ExecutionPayloadHeader(
parent_hash=payload.parent_hash,
Expand Down
8 changes: 5 additions & 3 deletions specs/capella/beacon-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ def process_block(state: BeaconState, block: BeaconBlock) -> None:
process_block_header(state, block)
if is_execution_enabled(state, block.body):
process_withdrawals(state, block.body.execution_payload) # [New in Capella]
process_execution_payload(state, block.body.execution_payload, EXECUTION_ENGINE) # [Modified in Capella]
process_execution_payload(state, block.body, EXECUTION_ENGINE) # [Modified in Capella]
process_randao(state, block.body)
process_eth1_data(state, block.body)
process_operations(state, block.body) # [Modified in Capella]
Expand Down Expand Up @@ -407,7 +407,9 @@ def process_withdrawals(state: BeaconState, payload: ExecutionPayload) -> None:
*Note*: The function `process_execution_payload` is modified to use the new `ExecutionPayloadHeader` type.

```python
def process_execution_payload(state: BeaconState, payload: ExecutionPayload, execution_engine: ExecutionEngine) -> None:
def process_execution_payload(state: BeaconState, body: BeaconBlockBody, execution_engine: ExecutionEngine) -> None:
payload = body.execution_payload

# Verify consistency of the parent hash with respect to the previous execution payload header
if is_merge_transition_complete(state):
assert payload.parent_hash == state.latest_execution_payload_header.block_hash
Expand All @@ -416,7 +418,7 @@ def process_execution_payload(state: BeaconState, payload: ExecutionPayload, exe
# Verify timestamp
assert payload.timestamp == compute_timestamp_at_slot(state, state.slot)
# Verify the execution payload is valid
assert execution_engine.notify_new_payload(payload)
assert execution_engine.notify_new_payload(NewPayloadRequest(execution_payload=payload))
# Cache execution payload header
state.latest_execution_payload_header = ExecutionPayloadHeader(
parent_hash=payload.parent_hash,
Expand Down
71 changes: 29 additions & 42 deletions specs/deneb/beacon-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,15 @@
- [Helper functions](#helper-functions)
- [Misc](#misc)
- [`kzg_commitment_to_versioned_hash`](#kzg_commitment_to_versioned_hash)
- [`tx_peek_blob_versioned_hashes`](#tx_peek_blob_versioned_hashes)
- [`verify_kzg_commitments_against_transactions`](#verify_kzg_commitments_against_transactions)
- [Beacon chain state transition function](#beacon-chain-state-transition-function)
- [Execution engine](#execution-engine)
- [Request data](#request-data)
- [Modified `NewPayloadRequest`](#modified-newpayloadrequest)
- [Engine APIs](#engine-apis)
- [Modified `notify_new_payload`](#modified-notify_new_payload)
- [Block processing](#block-processing)
- [Execution payload](#execution-payload)
- [`process_execution_payload`](#process_execution_payload)
- [Blob KZG commitments](#blob-kzg-commitments)
- [Testing](#testing)

<!-- END doctoc generated TOC please keep comment here to allow auto update -->
Expand Down Expand Up @@ -158,64 +160,55 @@ def kzg_commitment_to_versioned_hash(kzg_commitment: KZGCommitment) -> Versioned
return VERSIONED_HASH_VERSION_KZG + hash(kzg_commitment)[1:]
```

#### `tx_peek_blob_versioned_hashes`
## Beacon chain state transition function

### Execution engine

#### Request data

This function retrieves the hashes from the `SignedBlobTransaction` as defined in Deneb, using SSZ offsets.
Offsets are little-endian `uint32` values, as defined in the [SSZ specification](../../ssz/simple-serialize.md).
See [the full details of `blob_versioned_hashes` offset calculation](https://gist.github.com/protolambda/23bd106b66f6d4bb854ce46044aa3ca3).
##### Modified `NewPayloadRequest`

```python
def tx_peek_blob_versioned_hashes(opaque_tx: Transaction) -> Sequence[VersionedHash]:
assert opaque_tx[0] == BLOB_TX_TYPE
message_offset = 1 + uint32.decode_bytes(opaque_tx[1:5])
# field offset: 32 + 8 + 32 + 32 + 8 + 4 + 32 + 4 + 4 + 32 = 188
blob_versioned_hashes_offset = (
message_offset
+ uint32.decode_bytes(opaque_tx[(message_offset + 188):(message_offset + 192)])
)
# `VersionedHash` is a 32-byte object
assert (len(opaque_tx) - blob_versioned_hashes_offset) % 32 == 0
return [
VersionedHash(opaque_tx[x:(x + 32)])
for x in range(blob_versioned_hashes_offset, len(opaque_tx), 32)
]
@dataclass
class NewPayloadRequest(object):
execution_payload: ExecutionPayload
versioned_hashes: Sequence[VersionedHash]
```

#### `verify_kzg_commitments_against_transactions`
#### Engine APIs

#### Modified `notify_new_payload`

```python
def verify_kzg_commitments_against_transactions(transactions: Sequence[Transaction],
kzg_commitments: Sequence[KZGCommitment]) -> bool:
all_versioned_hashes: List[VersionedHash] = []
for tx in transactions:
if tx[0] == BLOB_TX_TYPE:
all_versioned_hashes += tx_peek_blob_versioned_hashes(tx)
return all_versioned_hashes == [kzg_commitment_to_versioned_hash(commitment) for commitment in kzg_commitments]
def notify_new_payload(self: ExecutionEngine, new_payload_request: NewPayloadRequest) -> bool:
"""
Return ``True`` if and only if ``execution_payload`` is valid with respect to ``self.execution_state``.
hwwhww marked this conversation as resolved.
Show resolved Hide resolved
"""
...
```

## Beacon chain state transition function

### Block processing
hwwhww marked this conversation as resolved.
Show resolved Hide resolved

```python
def process_block(state: BeaconState, block: BeaconBlock) -> None:
hwwhww marked this conversation as resolved.
Show resolved Hide resolved
process_block_header(state, block)
if is_execution_enabled(state, block.body):
process_withdrawals(state, block.body.execution_payload)
process_execution_payload(state, block.body.execution_payload, EXECUTION_ENGINE) # [Modified in Deneb]
process_execution_payload(state, block.body, EXECUTION_ENGINE) # [Modified in Deneb]
process_randao(state, block.body)
process_eth1_data(state, block.body)
process_operations(state, block.body)
process_sync_aggregate(state, block.body.sync_aggregate)
process_blob_kzg_commitments(block.body) # [New in Deneb]
```

#### Execution payload

##### `process_execution_payload`

```python
def process_execution_payload(state: BeaconState, payload: ExecutionPayload, execution_engine: ExecutionEngine) -> None:
def process_execution_payload(state: BeaconState, body: BeaconBlockBody, execution_engine: ExecutionEngine) -> None:
payload = body.execution_payload

# Verify consistency of the parent hash with respect to the previous execution payload header
if is_merge_transition_complete(state):
assert payload.parent_hash == state.latest_execution_payload_header.block_hash
Expand All @@ -224,7 +217,8 @@ def process_execution_payload(state: BeaconState, payload: ExecutionPayload, exe
# Verify timestamp
assert payload.timestamp == compute_timestamp_at_slot(state, state.slot)
# Verify the execution payload is valid
assert execution_engine.notify_new_payload(payload)
versioned_hashes = [kzg_commitment_to_versioned_hash(commitment) for commitment in body.blob_kzg_commitments]
assert execution_engine.notify_new_payload(NewPayloadRequest(execution_payload=payload, versioned_hashes=versioned_hashes))

# Cache execution payload header
state.latest_execution_payload_header = ExecutionPayloadHeader(
Expand All @@ -247,13 +241,6 @@ def process_execution_payload(state: BeaconState, payload: ExecutionPayload, exe
)
```

#### Blob KZG commitments

```python
def process_blob_kzg_commitments(body: BeaconBlockBody) -> None:
assert verify_kzg_commitments_against_transactions(body.execution_payload.transactions, body.blob_kzg_commitments)
```

## Testing

*Note*: The function `initialize_beacon_state_from_eth1` is modified for pure Deneb testing only.
Expand Down
4 changes: 2 additions & 2 deletions specs/deneb/validator.md
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ def validate_blobs_and_kzg_commitments(execution_payload: ExecutionPayload,
blobs: Sequence[Blob],
blob_kzg_commitments: Sequence[KZGCommitment],
blob_kzg_proofs: Sequence[KZGProof]) -> None:
# Optionally sanity-check that the KZG commitments match the versioned hashes in the transactions
assert verify_kzg_commitments_against_transactions(execution_payload.transactions, blob_kzg_commitments)
# TODO: can we just remove it?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this sanity check does make sense on CL side. If a CL client was given with inconsistent blobs data and a payload what can be done to overcome this situation? I think that EL should be checking this during the build process, otherwise, this inconsistency will likely result in a missed proposal. Even if the build process would be triggered once again there is a chance that the outcome will remain the same

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this can't be done anymore unless CL implements RLP, and also as mentioned above the proposal with eventually fail if this is inconsistent via new_payload EL calls. So better to just remove them

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, my comment was about validate_blobs_and_kzg_commitments function in general

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I agree with @mkalinin here, im not sure what value this adds

the assumption is that the engine API is trusted so it seems a bit redundant to have the CL check what should be "good" data from the EL

# assert verify_kzg_commitments_against_transactions(execution_payload.transactions, blob_kzg_commitments)

# Optionally sanity-check that the KZG commitments match the blobs (as produced by the execution engine)
assert len(blob_kzg_commitments) == len(blobs) == len(blob_kzg_proofs)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@ def run_execution_payload_processing(spec, state, execution_payload, valid=True,
called_new_block = False

class TestEngine(spec.NoopExecutionEngine):
def notify_new_payload(self, payload) -> bool:
def notify_new_payload(self, new_payload_request) -> bool:
nonlocal called_new_block, execution_valid
called_new_block = True
assert payload == execution_payload
assert new_payload_request.execution_payload == execution_payload
return execution_valid

if not valid:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ def process_and_sign_block_without_header_validations(spec, state, block):
)
if is_post_bellatrix(spec):
if spec.is_execution_enabled(state, block.body):
spec.process_execution_payload(state, block.body.execution_payload, spec.EXECUTION_ENGINE)
spec.process_execution_payload(state, block.body, spec.EXECUTION_ENGINE)

# Perform rest of process_block transitions
spec.process_randao(state, block.body)
Expand Down