-
Notifications
You must be signed in to change notification settings - Fork 300
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
Add kzg reference tests support #7571
Conversation
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.
LGTM. Just a few suggestions.
import tech.pegasys.teku.spec.config.SpecConfigDeneb; | ||
|
||
public abstract class KzgTestExecutor implements TestExecutor { | ||
// We don't support config reloading and all tests are currently for mainnet |
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.
Is this something we want to support in the future? If it is, it would be good to have a ticket for it linked 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.
- We may need it not only here, Stefan is already following it for another PR Support Deneb fork choice reference tests #7541
- There are no kzg reference tests other than for
mainnet
at the moment, if they are added we start to fail and will notice it - Currently config type is part of the directory name and extracting it looks ugly, so I removed it and harcoded "mainnet". It's not an issue until minimal are added (see №2)
actualVerificationResult = | ||
CKZG4844 | ||
.getInstance() | ||
.verifyBlobKzgProofBatch(List.of(blob), List.of(commitment), List.of(proof)); |
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.
Could you please add a comment that we are using the batch verify because we don't have a "non-batch" implementation? Just to avoid confusion.
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.
It's in KzgTests
, I will duplicate it 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.
Added 28b053c
PR Description
Fixed Issue(s)
Fixes #6969
Documentation
doc-change-required
label to this PR if updates are required.Changelog