-
Notifications
You must be signed in to change notification settings - Fork 997
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 WHISK fork only tests #3442
Conversation
Note:
|
@hwwhww seems circle CI is experiencing a caching problem again https://app.circleci.com/pipelines/github/ethereum/consensus-specs/9434/workflows/86a7977e-ac75-42f6-8d2e-a81982c949e4/jobs/70233
this method |
Oh my, it's so weird. I pushed a commit in my branch: ccd8026, and it seems to have installed with the correct commit but has some other errors. But after I pushed that commit to this PR's branch, it showed the same error. Is it possible because your repository has a different CircleCI setting? |
The reported errors were correct, there was a type issue with curdleproof's return values not caught in my local env due to caching. With nalinbhardwaj/curdleproofs.pie#21 merged, now all tests should pass :) EDIT: Circle CI version cache issue happening again. It's strange since the version bump is from a version that has
|
After debugging on a separate branch https://github.com/ethereum/consensus-specs/tree/whisk-tests-circleci results show that circle CI instances declare to install the correct commit of curdleproofs, but then a very old commit is actually used. I have not understood where this old version is coming from, but forcing an uninstall + reinstall seems to allow downstream steps to use the right version. I would suggest re-considering CircleCI as a platform to test the specs due to its non-determinism. Note this hack forces mantainers to duplicate the version declaration in both a Makefile command (for CircleCI) and regular requirements (for Github Actions) Lines 113 to 120 in db502dd
|
Updated the CRS with correct serialization. Tests should pass now. @hwwhww There's this error on CI which I can't reproduce locally. The error points to a spec variable not being present, however it's defined in the preset file. Any ideas?
Ref: https://github.com/ethereum/consensus-specs/actions/runs/6468128943/job/17559568960#step:8:106 |
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.
The PR looks good to me!
@hwwhww what do you think about merging it after fixing the CI issue?
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.
Hi @dapplion
I believe it was because these parameters are defined in Configurations
, so we use spec.config.*
to access them.
tests/core/pyspec/eth2spec/test/whisk/block_processing/test_process_shuffled_trackers.py
Outdated
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/whisk/block_processing/test_process_shuffled_trackers.py
Outdated
Show resolved
Hide resolved
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!
# Must be unique among the set `whisk_ks_initial + whisk_ks_final` | ||
def whisk_ks_final(i: int): | ||
return i + 10000000 |
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.
aha
def whisk_ks_initial(i: int): | ||
return i |
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.
perhaps add a TODO mark here
Description
Adds a baseline of tests covering the whisk feature introduced with #3342. In summary:
Scope / future work
Tests are only run for the whisk fork and minimal. The current crypto backend is significantly slower than BLS and KZG, so running more tests than that is prohibitively expensive at the moment.
Also to run tests from previous forks on Whisk, the vector generation has to be modified to accommodate the fact that the proposer is not known, and requires validator_size factorial G1 multiplications in an implementation without revealed shuffling data to figure out who is the proposer for a whisk round.
All those points can be addressed, but in a future PR if necessary