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.

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

[WIP] Introduce consensus code for Whisk (SSLE) with Curdleproofs #3205

Closed
wants to merge 22 commits into from

Conversation

nalinbhardwaj
Copy link
Contributor

asn-d6 and others added 21 commits January 12, 2023 06:09
@asn-d6: As discussed, I fixed a few bugs along the way but likely also introduced some bugs :)
This PR changes the Whisk tracker format to be of the form `(r * pubkey, r * BLS_GT_GENERATOR)` instead of `(r * k * BLS_G1_GENERATOR, r * BLS_G1_GENERATOR)`. This allows for non-interactive tracker registrations from validator pubkeys, removing ~50 lines the code. It also significantly reduces the amount of state overhead. This PR also removes permutation commitments, though those can be easily readded if deemed necessary.
@asn-d6: Readded a consistency check for `IsValidWhiskOpeningProof` (involving `pubkey` instead of `k_commitment`).
This needs its own ethresearch post, and some additional analysis to see if we can do the shuffle ZKP in the allowed
timeframe.

This reverts commit 8517aca.
Ditch the Feistel logic and instead have each shuffler pick the row they shuffle using their RANDAO reveal.
This commit further simplifies 0faef30 by removing the entire squareshuffle.

The latest version of https://eprint.iacr.org/2022/560 proposes that each shuffler picks random indices from the entire
candidate set instead of organizing validators into a square.


assert FINALIZED_ROOT_INDEX == get_generalized_index(BeaconState, 'finalized_checkpoint', 'root')
assert NEXT_SYNC_COMMITTEE_INDEX == get_generalized_index(BeaconState, 'next_sync_committee')
Copy link
Collaborator

@dapplion dapplion Mar 5, 2023

Choose a reason for hiding this comment

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

I don't think you want to commit this files. @hwwhww aren't this gitignored or what may have happened? Same for tests/core/pyspec/eth2spec/merge/minimal.py

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, it should be ignored. The spec name stub "merge" was very outdated. I guess @nalinbhardwaj didn't clean the old stub folder before pulling from the dev branch.

from eth2spec.utils import bls

from .exceptions import SkippedTest
from .helpers.constants import (
PHASE0, ALTAIR, BELLATRIX, CAPELLA, EIP4844,
PHASE0, ALTAIR, BELLATRIX, CAPELLA, EIP4844, WHISK, SHARDING,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
PHASE0, ALTAIR, BELLATRIX, CAPELLA, EIP4844, WHISK, SHARDING,
PHASE0, ALTAIR, BELLATRIX, CAPELLA, EIP4844, WHISK,


# The forks that pytest can run with.
ALL_PHASES = (
# Formal forks
PHASE0, ALTAIR, BELLATRIX, CAPELLA,
# Experimental patches
EIP4844,
EIP4844, CUSTODY_GAME, WHISK,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
EIP4844, CUSTODY_GAME, WHISK,
EIP4844, WHISK,

@@ -81,7 +77,6 @@ MIN_PER_EPOCH_CHURN_LIMIT: 4
# 2**16 (= 65,536)
CHURN_LIMIT_QUOTIENT: 65536


Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary polluting diff styling configs/mainnet.yaml and configs/minimal.yaml, should be reverted for a cleaner PR.

@dapplion dapplion mentioned this pull request Mar 5, 2023
@dapplion
Copy link
Collaborator

dapplion commented Mar 19, 2023

Some notes regarding implementation:

  • Instead of extending the validator class, add two new lists in the state. That reduces the additional hashing load from +4 hashes / val record to +2 hashes / val record. It also prevents adding 1 extra depth to the validator type and break light-client uses. It also simplifies implementation, as clients are used to different states but the Validator type has remained constant since phase0 and is heavily used.
  • Add whisk_opening_proof in the block body, not in the BeaconBlock type. I see no reason to not add the property in the body, and similarly to comment above, adding new properties outside the body adds implementation complexity for no clear gain.
  • Use strict snake case, Lodestar does not expect capital letters after _ otherwise it would break tooling.

See suggested diff here nalinbhardwaj/consensus-specs@whisk-curdleproofs...dapplion:consensus-specs:whisk-curdleproofs-review


If you want @nalinbhardwaj I can help rebase this PR into current structure, as now features are in a different directory than when this PR was created.

@asn-d6
Copy link
Contributor

asn-d6 commented Mar 24, 2023

Some notes regarding implementation:

* Instead of extending the validator class, add two new lists in the state. That reduces the additional hashing load from +4 hashes / val record to +2 hashes / val record. It also prevents adding 1 extra depth to the validator type and break light-client uses. It also simplifies implementation, as clients are used to different states but the Validator type has remained constant since phase0 and is heavily used.

* Add `whisk_opening_proof` in the block body, not in the BeaconBlock type. I see no reason to not add the property in the body, and similarly to comment above, adding new properties outside the body adds implementation complexity for no clear gain.

* Use strict snake case, Lodestar does not expect capital letters after `_` otherwise it would break tooling.

See suggested diff here nalinbhardwaj/consensus-specs@whisk-curdleproofs...dapplion:consensus-specs:whisk-curdleproofs-review

If you want @nalinbhardwaj I can help rebase this PR into current structure, as now features are in a different directory than when this PR was created.

Hey @dapplion . Thanks for reviewing the code. That was invaluable.

I think all your suggestions are great with regards to better utilizing the state and the beacon blocks.

I don't have a strong opinion about the naming, but I kinda liked having capital letters to signify the group elements (instead of field elements). Instead of changing the spec, isn't this something that can be changed on the lodestar side?

With regards to incorporating your diff: Would you like to rebase this PR, and then do a PR on top of it, so that we are also able to review your changes? I think most of your changes are great, but perhaps I'd like to comment on a line or two.

@dapplion
Copy link
Collaborator

dapplion commented Mar 27, 2023

@asn-d6 Opened PR against this branch, so it's in @nalinbhardwaj repo

Feel free to rebase first if you want and I can rebase my diff too

@hwwhww
Copy link
Contributor

hwwhww commented Apr 21, 2023

Hey, @nalinbhardwaj @asn-d6

Could you move the feature specs to specs/_features/whisk?

I suppose it needs more proper reviews before merging, but I can at least add a CI job to filter out the basic bugs. :)

@hwwhww
Copy link
Contributor

hwwhww commented Jun 8, 2023

Closing via #3342

Thank you @nalinbhardwaj @asn-d6 @JustinDrake @dapplion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants