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.

Sign up for GitHub

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

Jump to bottom

Pass versioned hashes to EL for validation when processing block #7219

Merged
merged 8 commits into from
Jun 2, 2023
Merged

Pass versioned hashes to EL for validation when processing block #7219

merged 8 commits into from
Jun 2, 2023

Conversation

StefanBratanov
Copy link
Contributor

PR Description

  • Removed KZGCommitmentsProcessor since process_blob_kzg_commitments in spec no longer required
  • Introduced computeNewPayloadRequest in BlockProcessor and logic is different in Bellatrix and Deneb as per spec
  • Removed unused methods in MiscHelpersDeneb

Fixed Issue(s)

fixes #7203

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

zilm13
zilm13 reviewed Jun 2, 2023
View reviewed changes
Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

I see that spec now contains method
verify_and_notify_new_payload which has 1 or 2 verification pre-checks (2 for Deneb) before calling EL. I think we should add it too or I missed its implementation

...src/test/java/tech/pegasys/teku/spec/logic/versions/deneb/block/BlockProcessorDenebTest.java Outdated
assertThat(newPayloadRequest.getVersionedHashes().get())
.hasSize(3)
.allMatch(
versionedHash -> versionedHash.isVersion(SpecConfigDeneb.VERSIONED_HASH_VERSION_KZG));
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also add a check that hashes match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@StefanBratanov
Copy link
Contributor Author

I see that spec now contains method verify_and_notify_new_payload which has 1 or 2 verification pre-checks (2 for Deneb) before calling EL. I think we should add it too or I missed its implementation

My understanding of the spec is that verify_and_notify_new_payload is essentially an abstraction of the EL communication. In case of Teku, is_valid_block_hash, is_valid_versioned_hashes and notify_new_payload are done by EL (newPayloadV3), since we don't have the information to do these verficiations.

@zilm13
Copy link
Contributor

zilm13 commented Jun 2, 2023

Sorry, my misunderstanding, rechecked spec again and you are right, everything should be done on el side

@StefanBratanov StefanBratanov enabled auto-merge (squash) June 2, 2023 09:47
@StefanBratanov StefanBratanov merged commit eec830d into Consensys:master Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zilm13 zilm13 zilm13 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use engine_newPayloadV3 to pass versioned_hashes to EL for validation
2 participants
@StefanBratanov @zilm13