Use `engine_newPayloadV3` to pass `versioned_hashes` to EL for validation #3359

hwwhww · 2023-05-15T16:37:59Z

Replace #3345
Require ethereum/EIPs#6985 and ethereum/execution-apis#407

Changes

beacon chain
- new wrappers
  - Since we are extending the Engine API request, I added abstract dataclass NewPayloadRequest and the ExecutionEngine.verify_and_notify_new_payload wrappers in Bellatrix so that we can add new arguments cleanly
  - To pass versioned_hash to EL, we change the arguments of process_execution_payload in Bellatrix. Now we pass body: BeaconBlockBody instead of payload: ExecutionPayload.
- Removed process_blob_kzg_commitments
validator.md: Removed validate_blobs_and_kzg_commitments

TODO

Check if CL clients are happy with it
There is an "Optionally sanity-check that the KZG commitments match the versioned hashes in the transactions" in the validator guide. Can we just remove it?
Fix tests
Update operation test format: it will be annoying that we have to pass a BeaconBody to process_execution_payload.
[optional] Move old Deneb sanity tests to block_processing (operations) tests

terencechain

+1, I think we can just remove verify_kzg_commitments_against_transactions

mkalinin · 2023-05-16T11:29:31Z

specs/deneb/validator.md

@@ -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?


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

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

Btw, my comment was about validate_blobs_and_kzg_commitments function in general

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

djrtwo · 2023-05-18T15:48:20Z

specs/deneb/beacon-chain.md

-    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(


another strategy is to keep notify_new_payload() as the same and to insert another function execution_engine.verify_versioned_hashes(versioned_hashes, payload.transactions). This would be divergent from the engine API but is very explicit.

In fact if we went that path, I would also break out the verification of the block_hash -- e.g.

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 process_blob_kzg_commitments along with the length check discussed in #3338

cc: @mkalinin

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

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 execution_valid in test vectors, if we want to add what's proposed in #3359 (comment) + add execution_engine.is_valid_versioned_hashes() helper in specs, we have to yield an is_valid_versioned_hashes field in test vectors.

I'd like to confirm if clients indeed prefer parsing this themselves. 🤔

/cc @terencechain

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

assert execution_engine.notify_new_payload(

asset len(body.blob_kzg_commitments) <= MAX_BLOBS_PER_BLOCK

assert execution_engine.notify_new_payload(

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.

@g11tech @ppopth

The extra length check is unnecessary since the SSZ container has defined field blob_kzg_commitments: List[KZGCommitment, MAX_BLOBS_PER_BLOCK].

@djrtwo @mkalinin

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 execution_valid return value of notify_new_payload_and_verify_versioned_hashes in testing.

Note: we override this API in setup.py, so the content doesn't matter for pyspec itself.

Does it make sense to rename the Engine API side as well?

Edited: verify_and_notify_new_payload approach https://github.com/ethereum/consensus-specs/compare/engine-versioned-hashes-explicit

The extra length check is unnecessary since the SSZ container has defined field blob_kzg_commitments: List[KZGCommitment, MAX_BLOBS_PER_BLOCK].

It will be changed in this PR #3338

@ppopth @g11tech
Ah...! but this PR will get merged first, so #3338 will have to address it. 😅

djrtwo · 2023-05-18T15:50:06Z

tests/core/pyspec/eth2spec/test/deneb/sanity/test_blocks.py



 @with_deneb_and_later
 @spec_state_test
-def test_invalid_incorrect_transaction_length_1_byte(spec, state):
+def test_incorrect_transaction_length_1_byte(spec, state):


Should we do a version of this that has executionengine return invalid?

Essentially, the execution_valid result is fully up to what we set in the testing script. So I think only the "the input is correct, but execution_valid returns False" is valuable here. I added test case test_invalid_correct_input__execution_invalid for it.

specs/deneb/beacon-chain.md

hwwhww · 2023-05-23T15:57:30Z

Things to check

@mkalinin and @ralexstokes suggested remove the whole validate_blobs_and_kzg_commitments (https://github.com/ethereum/consensus-specs/pull/3359/files/058137327a4cb9163c4202e1ed3bd4d886d745cf#r1195022061)
My approach to making the validation clear (Use engine_newPayloadV3 to pass versioned_hashes to EL for validation #3359 (comment)): https://github.com/ethereum/consensus-specs/compare/engine-versioned-hashes-explicit, if it's good, we can cherry-pick it.

djrtwo · 2023-05-23T19:34:21Z

@hwwhww I like the approach of having the higher level function encapsulate and make explicit the three high level things the call is performing.

Why deneb instead of a Bellatrix modification? This won't affect test vectors and brings the explicit nature of the block-hash verification to the prior spec as well.

I'm fine to do this here or in a subsequent cleanup

djrtwo · 2023-05-23T20:06:27Z

As for the removal of validate_blobs_and_kzg_commitments, that looks fine. This looks like strictly duplicate specification work on building.

But note that this does not preclude the CL from following the on_block flow which does the is_data_available/validate_blobs call. This ensures that any non-local build flow still ultimately does the verifications to incorporate the block into your local view

…-hashes

hwwhww · 2023-05-24T03:56:55Z

@djrtwo

But note that this does not preclude the CL from following the on_block flow which does the is_data_available/validate_blobs call. This ensures that any non-local build flow still ultimately does the verifications to incorporate the block into your local view

Would you like to add a note in the validator guide?

djrtwo

a couple of tiny things.

approved!

djrtwo · 2023-05-24T03:56:35Z

specs/bellatrix/beacon-chain.md

+    """
+    assert self.is_valid_block_hash(new_payload_request.execution_payload)
+    assert self.notify_new_payload(new_payload_request.execution_payload)
+    ...


What does the ... imply here?

I wanted to imply that this is a pseudo-code-like function and is client implementation dependent. 😅

djrtwo · 2023-05-24T03:57:52Z

specs/deneb/beacon-chain.md

+    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)  # [Modified in Deneb]


Thank you 🙏

It's really nice to see this validation land explicitly in the spec, but at the same time to hide this behind one function that we can call out as the direct mapping for the engine api!

specs/deneb/beacon-chain.md

Co-authored-by: Danny Ryan <dannyjryan@gmail.com>

Use new engine_newPayloadV3

0581373

hwwhww added the Deneb was called: eip-4844 label May 15, 2023

hwwhww requested review from mkalinin and djrtwo May 15, 2023 16:50

terencechain approved these changes May 15, 2023

View reviewed changes

mkalinin reviewed May 16, 2023

View reviewed changes

Fix tests

0b2f604

hwwhww force-pushed the engine-versioned-hashes branch from d163bef to 0b2f604 Compare May 16, 2023 16:25

hwwhww marked this pull request as ready for review May 16, 2023 16:33

g11tech approved these changes May 17, 2023

View reviewed changes

djrtwo mentioned this pull request May 18, 2023

Consensus-layer Call 109 ethereum/pm#783

Closed

djrtwo reviewed May 18, 2023

View reviewed changes

djrtwo mentioned this pull request May 18, 2023

Update block's blob_kzg_commitments size limit to MAX_BLOB_COMMITMENTS_PER_BLOCK (4096) #3338

Merged

ppopth reviewed May 22, 2023

View reviewed changes

specs/deneb/beacon-chain.md Outdated Show resolved Hide resolved

ppopth reviewed May 22, 2023

View reviewed changes

specs/deneb/beacon-chain.md Outdated Show resolved Hide resolved

hwwhww added 4 commits May 23, 2023 15:16

PR feedback from @ppopth

3247bcf

Merge branch 'dev' into engine-versioned-hashes

2eab6bf

Move old Deneb sanity tests to block_processing (operations) tests

fc45220

Add test_invalid_correct_input__execution_invalid

bee8fa1

ppopth reviewed May 23, 2023

View reviewed changes

specs/deneb/beacon-chain.md Show resolved Hide resolved

hwwhww added 2 commits May 23, 2023 23:37

Use verify_and_notify_new_payload approach

73df193

Add ### Block processing back

7ec5efb

Add make_function_abstract to make it more general

dd5e6f8

djrtwo mentioned this pull request May 23, 2023

v1.4.0-alpha.0 release #3380

Merged

8 tasks

hwwhww added 3 commits May 24, 2023 10:40

Merge branch 'engine-versioned-hashes-explicit' into engine-versioned…

0a90b58

…-hashes

Move verify_and_notify_new_payload to Bellatrix

289d814

Merge branch 'dev' into engine-versioned-hashes

6b5513b

hwwhww added 2 commits May 24, 2023 11:27

Fix ToC and remove validate_blobs_and_kzg_commitments

212a314

Fix ToC

53a9221

djrtwo approved these changes May 24, 2023

View reviewed changes

hwwhww and others added 2 commits May 24, 2023 12:17

Fix typo

ec1ee74

Co-authored-by: Danny Ryan <dannyjryan@gmail.com>

Ensure verify_and_notify_new_payload returns bool

7a82763

hwwhww merged commit 2225fd3 into dev May 24, 2023

hwwhww deleted the engine-versioned-hashes branch May 24, 2023 07:34

hwwhww mentioned this pull request May 25, 2023

RLP-encoded blob transaction #3345

Closed

g11tech mentioned this pull request May 29, 2023

feat: change the blob txs encoding assumption to be opaque rlp ChainSafe/lodestar#5569

Merged

1 task

StefanBratanov mentioned this pull request May 31, 2023

Use engine_newPayloadV3 to pass versioned_hashes to EL for validation Consensys/teku#7203

Closed

realbigsean mentioned this pull request Jun 5, 2023

Deneb Devnet 6 sigp/lighthouse#4374

Closed

8 tasks

tersec mentioned this pull request Jun 8, 2023

4844-devnet-6 specs status-im/nimbus-eth2#5041

Closed

20 tasks

tersec added a commit to status-im/nimbus-eth2 that referenced this pull request Jun 9, 2023

https://github.com/ethereum/consensus-specs/pull/3359

36982d8

tersec added a commit to status-im/nimbus-eth2 that referenced this pull request Jun 10, 2023

ethereum/consensus-specs#3359 (#5047)

9e14d90

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `engine_newPayloadV3` to pass `versioned_hashes` to EL for validation #3359

Use `engine_newPayloadV3` to pass `versioned_hashes` to EL for validation #3359

hwwhww commented May 15, 2023 •

edited

Loading

terencechain left a comment

mkalinin May 16, 2023

g11tech May 16, 2023

mkalinin May 17, 2023

ralexstokes May 17, 2023

djrtwo May 18, 2023 •

edited

Loading

djrtwo May 18, 2023

mkalinin May 19, 2023 •

edited

Loading

hwwhww May 19, 2023

g11tech May 20, 2023 •

edited

Loading

djrtwo May 22, 2023

hwwhww May 23, 2023 •

edited

Loading

hwwhww May 23, 2023 •

edited

Loading

ppopth May 23, 2023

hwwhww May 23, 2023 •

edited

Loading

djrtwo May 18, 2023

hwwhww May 23, 2023

hwwhww commented May 23, 2023 •

edited

Loading

djrtwo commented May 23, 2023

djrtwo commented May 23, 2023

hwwhww commented May 24, 2023

djrtwo left a comment

djrtwo May 24, 2023

hwwhww May 24, 2023 •

edited

Loading

djrtwo May 24, 2023

	assert execution_engine.notify_new_payload(
	asset len(body.blob_kzg_commitments) <= MAX_BLOBS_PER_BLOCK
	assert execution_engine.notify_new_payload(

Use engine_newPayloadV3 to pass versioned_hashes to EL for validation #3359

Use engine_newPayloadV3 to pass versioned_hashes to EL for validation #3359

Conversation

hwwhww commented May 15, 2023 • edited Loading

Changes

TODO

terencechain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djrtwo May 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkalinin May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g11tech May 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww May 23, 2023 • edited Loading

Choose a reason for hiding this comment

hwwhww May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww commented May 23, 2023 • edited Loading

djrtwo commented May 23, 2023

djrtwo commented May 23, 2023

hwwhww commented May 24, 2023

djrtwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use `engine_newPayloadV3` to pass `versioned_hashes` to EL for validation #3359

Use `engine_newPayloadV3` to pass `versioned_hashes` to EL for validation #3359

hwwhww commented May 15, 2023 •

edited

Loading

djrtwo May 18, 2023 •

edited

Loading

mkalinin May 19, 2023 •

edited

Loading

g11tech May 20, 2023 •

edited

Loading

hwwhww May 23, 2023 •

edited

Loading

hwwhww May 23, 2023 •

edited

Loading

hwwhww May 23, 2023 •

edited

Loading

hwwhww commented May 23, 2023 •

edited

Loading

hwwhww May 24, 2023 •

edited

Loading