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

Create MiscHelpersEip4844 #6474

Merged
merged 10 commits into from
Nov 22, 2022
Merged

Create MiscHelpersEip4844 #6474

merged 10 commits into from
Nov 22, 2022

Conversation

zilm13
Copy link
Contributor

@zilm13 zilm13 commented Nov 18, 2022 •
edited
Loading

PR Description

Fixed Issue(s)

Fixes #6391

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.

github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 18, 2022
View reviewed changes
...um/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/eip4844/util/KZGUtilEip4844.java
public class KZGUtilEip4844 {

public static void verifyAggregateKZGProof(
final List<Blob> blobs,

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'blobs' is never used.
...um/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/eip4844/util/KZGUtilEip4844.java

public static void verifyAggregateKZGProof(
final List<Blob> blobs,
final List<KZGCommitment> expectedKZGCommitments,

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'expectedKZGCommitments' is never used.
...um/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/eip4844/util/KZGUtilEip4844.java
public static void verifyAggregateKZGProof(
final List<Blob> blobs,
final List<KZGCommitment> expectedKZGCommitments,
final KZGProof kzgProof) {

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'kzgProof' is never used.
@zilm13 zilm13 marked this pull request as ready for review November 21, 2022 19:43
@tbenr
Copy link
Contributor

tbenr commented Nov 22, 2022

As per out-of-band discussion, we agreed to move back from SSZ to the "magic" when decoding transaction to get the versioned hash. We gain in runtime performance and reduced maintenance (we don't need to deal with potential changes in next fork. Eventual changes in new forks will be implemented with a new miscHelper).
We lose some clarity but the "magic" is well confined in one method. We can have nice comment to increase clarity.

tbenr
tbenr reviewed Nov 22, 2022
View reviewed changes
Copy link
Contributor

@tbenr tbenr left a comment

Choose a reason for hiding this comment

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

looks good. Just a couple of comments

ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/capella/SpecLogicCapella.java Outdated Show resolved Hide resolved
infrastructure/crypto/build.gradle Outdated Show resolved Hide resolved
tbenr
tbenr approved these changes Nov 22, 2022
View reviewed changes
Copy link
Contributor

@tbenr tbenr left a comment

Choose a reason for hiding this comment

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

LGTM

@zilm13 zilm13 merged commit bb0e75b into Consensys:master Nov 22, 2022
@zilm13 zilm13 deleted the eip4844-mischelpers branch November 22, 2022 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajsutton ajsutton ajsutton left review comments

@tbenr tbenr tbenr 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.

Implement MiscHelpersEip4844
3 participants
@zilm13 @tbenr @ajsutton