-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Changes from 6 commits
0581373
0b2f604
3247bcf
2eab6bf
fc45220
bee8fa1
73df193
7ec5efb
dd5e6f8
0a90b58
289d814
6b5513b
212a314
53a9221
ec1ee74
7a82763
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -24,13 +24,14 @@ | |||||||
- [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) | ||||||||
- [Block processing](#block-processing) | ||||||||
- [Execution engine](#execution-engine) | ||||||||
- [Request data](#request-data) | ||||||||
- [Modified `NewPayloadRequest`](#modified-newpayloadrequest) | ||||||||
- [Engine APIs](#engine-apis) | ||||||||
- [Modified `notify_new_payload`](#modified-notify_new_payload) | ||||||||
- [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 --> | ||||||||
|
@@ -158,64 +159,41 @@ 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 | ||||||||
|
||||||||
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). | ||||||||
### Execution engine | ||||||||
|
||||||||
```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) | ||||||||
] | ||||||||
``` | ||||||||
#### Request data | ||||||||
|
||||||||
#### `verify_kzg_commitments_against_transactions` | ||||||||
##### Modified `NewPayloadRequest` | ||||||||
|
||||||||
```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] | ||||||||
@dataclass | ||||||||
class NewPayloadRequest(object): | ||||||||
execution_payload: ExecutionPayload | ||||||||
versioned_hashes: Sequence[VersionedHash] | ||||||||
``` | ||||||||
|
||||||||
## Beacon chain state transition function | ||||||||
#### Engine APIs | ||||||||
|
||||||||
### Block processing | ||||||||
hwwhww marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
#### Modified `notify_new_payload` | ||||||||
|
||||||||
```python | ||||||||
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 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] | ||||||||
def notify_new_payload(self: ExecutionEngine, new_payload_request: NewPayloadRequest) -> bool: | ||||||||
""" | ||||||||
Return ``True`` if and only if ``new_payload_request`` is valid with respect to ``self.execution_state``. | ||||||||
""" | ||||||||
... | ||||||||
``` | ||||||||
|
||||||||
#### 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 | ||||||||
|
@@ -224,7 +202,11 @@ 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) | ||||||||
# [Modified in Deneb] | ||||||||
versioned_hashes = [kzg_commitment_to_versioned_hash(commitment) for commitment in body.blob_kzg_commitments] | ||||||||
assert execution_engine.notify_new_payload( | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. another strategy is to keep In fact if we went that path, I would also break out the verification of the assert execution_engine.is_valid_block_hash(payload)
assert execution_engine.is_valid_versioned_hashes(versioned_hashes, payload.transactions)
assert execution_engine.notify_new_payload(payload) and i would maybe keep it in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc: @mkalinin There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really like this strategy as making these checks explicit brings more clarity to the spec. Can't see anything that could be broken by doing it this way, I was thinking about optimistic sync tests but they shouldn't be affected afaics There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But in client implementations, the validation will still use the one Engine API call response, correct? Aren't different API abstractions make it more difficult to test? e.g., we already yield I'd like to confirm if clients indeed prefer parsing this themselves. 🤔 /cc @terencechain There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my view: we would just like it as a single call with length check before the call (check should come before versioned hashes computation but suggested here for the conversation context)
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given this is a specification to show complete state transition validation and mutation logic and not an implementation guide that knows or cares about the engine api (Engine API is just one architecture of a valid full client), I very much like explicitly calling out the various groupings of validations instead of just hiding it behind one. That said, at this point in the process, if it is going to cause too much additional overhead (e.g. with changing test formats), I'm not going to push too hard for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree the explicitness is good, but CL is just the caller and has no control over executing the validation. I support @ppopth's proposal of renaming the API name or creating a new API to replace the old one in Deneb. Another way is to make it clear in the abstract API: def verify_and_notify_new_payload(self: ExecutionEngine,
new_payload_request: NewPayloadRequest) -> bool:
"""
Return ``True`` if and only if ``new_payload_request`` is valid with respect to ``self.execution_state``.
"""
assert self.is_valid_block_hash(new_payload_request.execution_payload)
assert self.is_valid_versioned_hashes(new_payload_request)
assert self.notify_new_payload(new_payload_request.execution_payload) And we stick to using the single Note: we override this API in Does it make sense to rename the Engine API side as well? Edited: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It will be changed in this PR #3338 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||
NewPayloadRequest(execution_payload=payload, versioned_hashes=versioned_hashes) | ||||||||
) | ||||||||
|
||||||||
# Cache execution payload header | ||||||||
state.latest_execution_payload_header = ExecutionPayloadHeader( | ||||||||
|
@@ -247,13 +229,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. | ||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the ... imply here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to imply that this is a pseudo-code-like function and is client implementation dependent. 😅