Use stable sync committee indices when processing block rewards #2394
Conversation
| @@ -277,6 +277,9 @@ def get_sync_committee_indices(state: BeaconState, epoch: Epoch) -> Sequence[Val | |||
| """ | |||
| Return the sequence of sync committee indices (which may include duplicate indices) | |||
| for a given ``state`` and ``epoch``. | |||
|
|
|||
| Note: This function is not stable during a sync committee period as | |||
| a validator's effective balance may change enough to affect the sampling. | |||
| """ | |||
| MAX_RANDOM_BYTE = 2**8 - 1 | |||
| base_epoch = Epoch((max(epoch // EPOCHS_PER_SYNC_COMMITTEE_PERIOD, 1) - 1) * EPOCHS_PER_SYNC_COMMITTEE_PERIOD) | |||
There was a problem hiding this comment.
If the function can now only be legally called at the start of a period, why not formalize that in an assert and then simplify the base_epoch computation?
There was a problem hiding this comment.
ah, the complication is that the fork epoch from phase 0 to altair may not be aligned to an EPOCHS_PER_SYNC_COMMITTEE_PERIOD boundary
we either round the fork epoch we determine up to the EPOCHS_PER_SYNC_COMMITTEE_PERIOD boundary or leave this as is
There was a problem hiding this comment.
looks good. agree with @vbuterin's comments
Also, we should add a test for this case. Seems very important it's triggered in the consensus tests.
One way to do it would be to just manually create a sync_committee in state (e.g. just indicies 1..N) and then move forward into the period. That way, the dynamic calculation will certainly not be the same as the value in state but value in state should win.
Alternatively we can set many validator balances to be on the cusp of an effective balance change at the start of the period. Then run a few empty epochs to inflict penalties to alter effective balances enough to change the shuffling
Co-authored-by: vbuterin <v@buterin.com>
| if proposer_index == index: | ||
| reward += compute_sync_committee_proposer_reward( | ||
| spec, | ||
| pre_state, | ||
| committee, | ||
| committee_bits, | ||
| ) |
There was a problem hiding this comment.
coincidentally, it seems the proposer was in the sync committee for all of the other tests using this code...
if we want to check every validator (or at least the case where the proposer is outside the sync committee) then we need to unindent this conditional one level
Addresses a bug surfaced in #2392 thanks to @michaelsproul.
It is possible that the
effective_balances of the active validator set change enough during a sync committee period so that the shuffling computed at the period boundary (epoch % EPOCHS_PER_SYNC_COMMITTEE_PERIOD == 0) changes.This implies
get_sync_committee_indicesis not stable across a sync committee period and so should not be relied upon while computing the members of the sync committee throughout such a period. This function is currently used during the processing of sync committee per-block rewards so this PR uses the (fixed) sync committee set in theBeaconState, rather than computing on-demand for each block.