-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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 EIP: Upgrade block proposer election to Whisk #7441
Conversation
Co-authored-by: George Kadianakis <desnacked@riseup.net>
Co-authored-by: George Kadianakis <desnacked@riseup.net>
Co-authored-by: George Kadianakis <desnacked@riseup.net>
✅ All reviewers have approved. |
The commit 78ce5b8 (as a parent of 48a6dc1) contains errors. |
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.
Please fix the issue with the title
Co-authored-by: Andrew B Coathup <28278242+abcoathup@users.noreply.github.com>
|
||
The beacon chain currently elects the next 32 block proposers at the beginning of each epoch. The results of this election are public and everyone gets to learn the identity of those future block proposers. | ||
|
||
This information leak enables attackers to launch DoS attacks against each proposer sequentially in an attempt to disable Ethereum. |
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.
there was an EIP which described how proposers and/or users can collude to avoid paying higher base fee and pocket more tips leading to irregular block size, will see which was that . this EIP can potentially address that scenario as well
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.
With whisk, each proposer is aware of its future proposer slots much more in advance. So if there's collusion and proposers want to reveal such information to selected parties, Whisk makes the attack you describe easier.
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.
may be then we can add this in security considerations ?
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.
I don't think it's a significant change into the status quo security in this regard. With RANDAO based shuffling actors can still collude if they have proposer slots in a row
### Consensus layer | ||
|
||
The protocol can be summarized in the following concurrent steps: | ||
|
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.
- Proposals are now divided into shuffling phases of `WHISK_EPOCHS_PER_SHUFFLING_PHASE` epochs |
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.
I'm not sure if this sentence is necessary. TBH this mini summary of the protocol is not necessary either. It does not provide any specification and is only a general overview for readers. Should it be removed altogether?
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.
so the flow of someone looking into this feature would be (or atleast how we want it to be) : Read EIP => Read consensus specs => get into client implementations if someone wants to follow through on this. So mini summary helps making that jump
keeping that in mind, what kind of mini summary would you deem good to fit here. else if we have that somewhere in consensus specs (fork.md
?) then we can refer it here i guess
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.
I don't think such summary would fit anywhere in the specs. Since it's derived information it mostly fits here or in a blog / etheresearch post introduction.
Co-authored-by: g11tech <develop@g11tech.io>
Co-authored-by: g11tech <develop@g11tech.io>
Co-authored-by: g11tech <develop@g11tech.io>
Co-authored-by: g11tech <develop@g11tech.io>
|
||
The full specification of the proposed change can be found in [`/_features/whisk/beacon-chain.md`](https://github.com/ethereum/consensus-specs/blob/a39abe388bc2d1abd5b4fd62fd18aed497956b30/specs/_features/whisk/beacon-chain.md). In summary: | ||
|
||
- Update `BeaconState` with fields needed to track validator trackers, commitments, and the two rounds of candidate election. |
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.
may be we don't need this detail (how to update beacon state) as its defined in the full specification
already refered and could do away with this
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.
Would be good define some guidelines on what this section is expected to include or not? Other editors can chip-in and @djrtwo
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, discussions for how to best summarize EIP ideas can be done separately i guess.
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.
All Reviewers Have Approved; Performing Automatic Merge...
@Pandapip1 is there anything pending from your side? |
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.
All Reviewers Have Approved; Performing Automatic Merge...
Head branch was pushed to by a user without write access
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.
All Reviewers Have Approved; Performing Automatic Merge...
When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md
We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met: