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

Improve beacon proposer selection logic #1371

Closed
wants to merge 89 commits into from
Closed

Improve beacon proposer selection logic #1371

wants to merge 89 commits into from

Conversation

JustinDrake
Copy link
Collaborator

@JustinDrake JustinDrake commented Aug 20, 2019

Sample proposer from the full set of active validators (as opposed to just the first committee) and make proposer slashing per-slot (as opposed to per-epoch).

  • improved liveness: The beacon chain can progress with just one validator (as opposed to SLOTS_PER_EPOCHS, an unintended design flaw). Improved liveness with low validator count may be useful for testing, fuzzing, and private consortium beacon chains.
  • reduced complexity: Avoid calling get_committee_count, get_start_shard and get_crosslink_committee. Reduce get_beacon_proposer_index line count by 20% (from 15 lines to 12 lines).
  • improved fairness: Ensure that the probability of being selected as a proposer is proportional to the effective balance. The current design gradually breaks fairness as the number of validators decreases.
  • more natural design: Block proposing is natively per-slot, not per-epoch. Block proposing is orthogonal from the notion of committees, and per-slot slashing is more natural than per-epoch slashing.

Edit: I've abstracted away compute_proposer_index for reuse in phase 1.

JustinDrake and others added 30 commits August 1, 2019 17:03
Co-Authored-By: Carl Beekhuizen <carl@ethereum.org>
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
Starting on phase 1 misc beacon changes
@djrtwo
Copy link
Contributor

djrtwo commented Sep 2, 2019

This needs adjustments to the validator guide (being part of a committee is no longer a warning of potential proposal) and I want to run a couple of tests on the min-validator requirement of the chain now.

I'm a little resistant to the change due to the spec freeze especially since we don't really have a criteria for evaluating if something like this should go in other than "it's better".

@terencechain
Copy link
Contributor

I'm a little resistant to the change due to the spec freeze especially since we don't really have a criteria for evaluating if something like this should go in other than "it's better".

Why not put this under Phase 1 miscellaneous beacon chain changes?

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

Updated the v-guide accordingly.
Confirmed that a lot more of the tests pass with less than SLOTS_PER_EPOCH vals, but haven't fully confirmed the low val count. Too many of our tests assume there is a committee available at a slot to make attestations (which is a sane way to test in general). Have on my todo to add some tests for handling empty operations and things.

Last thing here: I don't think we can do the filter in get_beacon_proposer_index by slashed. That would remove the predictable property within an epoch

@djrtwo
Copy link
Contributor

djrtwo commented Sep 20, 2019

@JustinDrake

Bump on my above comment

Last thing here: I don't think we can do the filter in get_beacon_proposer_index by slashed. That would remove the predictable property within an epoch

Looking for an agreement and then I'll merge this

@JustinDrake
Copy link
Collaborator Author

Confirmed that a lot more of the tests pass with less than SLOTS_PER_EPOCH vals

👍

I don't think we can do the filter in get_beacon_proposer_index by slashed. That would remove the predictable property within an epoch

Oh right :)

On that note get_active_validator_indices(state, epoch) can return a different value depending on when called. It's temping to add assert_invariant(epoch <= get_current_epoch(state) < compute_activation_exit_epoch(epoch)) 😂

@djrtwo
Copy link
Contributor

djrtwo commented Sep 22, 2019

Closing in favor of #1413

@djrtwo djrtwo closed this Sep 22, 2019
@djrtwo djrtwo deleted the proposers branch May 20, 2020 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
post-freeze (substantive) Substantive consensus change non-critical for long-lived cross-client testnets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants