Use 16-bit random value in validator filter

[jtraglia]

See: https://docs.google.com/spreadsheets/d/18VeMVLO1jbP8xHkT4faGEb5SyrWQhQLcU-BtsL9pTss

This fixes an oversight in compute_proposer_index and get_next_sync_committee_indices.

Currently, we use an 8-bit random byte to perform an effective balance filter. This was used to penalize validators with effective balances less than 32 ETH. But now that we have raised the maximum effective balance, this mechanism plays a more important role in validator selection.

One might expect a validator with a greater effective balance to have a greater chance of being selected as proposer or sync committee participant, but no. This is because the random byte can only represent 256 "buckets" of effective balance. So for example, currently, a validator with 33 ETH has the same likelihood of being selected as a validator with 40 ETH, because these validators are in the same bucket.

The solution is to use a bigger random value to allow more precision. I propose we use a 16-bit random value instead which provides enough precision (~65k buckets). Originally I wanted to use 32-bit values but it would require more complex changes because there would be an integer overflow when multiplying the max values together.

[mkalinin]

The change looks good and makes sense to me! 👍
I think we need more tests to ensure implementation compliance

[rolfyone]

Sounds logical... good writeup

[jtraglia]

I think we need more tests to ensure implementation compliance

It shouldn't be necessary to add extra tests. There should be several sanity tests which will check this.

For example, test_proposer_after_inactive_index and test_high_proposer_index would most likely error if get_beacon_proposer_index were implemented incorrectly.

• consensus-specs/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py

  Lines 669 to 716 in 554189d

  	@with_all_phases
  	@spec_state_test
  	def test_proposer_after_inactive_index(spec, state):
  	# disable some low validator index to check after for
  	inactive_index = 10
  	state.validators[inactive_index].exit_epoch = spec.get_current_epoch(state)
  	# skip forward, get brand new proposers
  	next_epoch_via_block(spec, state)
  	next_epoch_via_block(spec, state)
  	while True:
  	proposer_index = spec.get_beacon_proposer_index(state)
  	if proposer_index > inactive_index:
  	# found a proposer that has a higher index than a disabled validator
  	yield 'pre', state
  	# test if the proposer can be recognized correctly after the inactive validator
  	signed_block = state_transition_and_sign_block(spec, state, build_empty_block_for_next_slot(spec, state))
  	yield 'blocks', [signed_block]
  	yield 'post', state
  	break
  	next_slot(spec, state)
  	@with_all_phases
  	@spec_state_test
  	def test_high_proposer_index(spec, state):
  	# disable a good amount of validators to make the active count lower, for a faster test
  	current_epoch = spec.get_current_epoch(state)
  	for i in range(len(state.validators) // 3):
  	state.validators[i].exit_epoch = current_epoch
  	# skip forward, get brand new proposers
  	state.slot = spec.SLOTS_PER_EPOCH * 2
  	block = build_empty_block_for_next_slot(spec, state)
  	state_transition_and_sign_block(spec, state, block)
  	active_count = len(spec.get_active_validator_indices(state, current_epoch))
  	while True:
  	proposer_index = spec.get_beacon_proposer_index(state)
  	if proposer_index >= active_count:
  	# found a proposer that has a higher index than the active validator count
  	yield 'pre', state
  	# test if the proposer can be recognized correctly, even while it has a high index.
  	signed_block = state_transition_and_sign_block(spec, state, build_empty_block_for_next_slot(spec, state))
  	yield 'blocks', [signed_block]
  	yield 'post', state
  	break
  	next_slot(spec, state)

[mkalinin]

The PR looks good to me and great work on the analysis! 👍

[ppopth]

Looking good to me

[ralexstokes]

noting we get the intended change here as precedence in python gives us this (i % 16) * 2

[ralexstokes]

looks good to me! nice catch :)

we should verify there is some difference in test output, just so it will be clear if a client does not conform, but otherwise I think it is ready to go

[jtraglia]

we should verify there is some difference in test output

Good call. I have a simple difference. In test_high_proposer_index, the state's slot will be different. This is because the following check will require more iterations.

consensus-specs/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py

Lines 706 to 708 in 98cf01d

	while True:
	proposer_index = spec.get_beacon_proposer_index(state)
	if proposer_index >= active_count:

See slot: 17 in the first test run (on dev) and slot: 20 on the second one (on this branch):