Eth1 data voting exception

[paulhauner]

Part of the description for Eth1 Data voting is as follows:

Let get_eth1_data(distance: uint64) -> Eth1Data be the (subjective) function that returns the Eth1 data at distance distance relative to the Eth1 head at the start of the current Eth1 voting period. Let previous_eth1_distance be the distance relative to the Eth1 block corresponding to state.eth1_data.block_hash at the start of the current Eth1 voting period. An honest block proposer sets block.eth1_data = get_eth1_vote(state, previous_eth1_distance) where ...

This text uses the state variable twice:

1. state.eth1_data.block_hash when determining previous_eth1_distance.
2. get_eth1_vote(state, previous_eth1_distance) when setting block.eth1_data.

I claim that these two state variables must be distinct, otherwise, in the best-case voting scenario, an exception is raised (see "Empty all_eth1_data exception") .

I think it comes down to the following line being unclear (imo):

Let previous_eth1_distance be the distance relative to the Eth1 block corresponding to state.eth1_data.block_hash at the start of the current Eth1 voting period.

And to prevent the exception, it should become:

Let prev_state be the state at the start of the current Eth1 voting period. Let previous_eth1_distance be the distance between the Eth1 head at the start of the current Eth1 voting period and the Eth1 block corresponding to prev_state.eth1_data.block_hash

However, what I suggested is still somewhat overwhelming to read. It's my opinion that the second paragraph of Eth1 Data should really be executable code instead of English.

Empty all_eth1_data exception

This describes an exception that is raised when using the state at the current slot (i.e., the state for the block being produced) as inputs to both get_eth1_vote(..) and previous_eth1_distance:

1. The first block proposer in the eth1 voting period chooses get_eth1_data(ETH1_FOLLOW_DISTANCE).
2. All subsequent block proposers select the same eth1 block.
3. After ~SLOTS_PER_ETH1_VOTING_PERIOD / 2 slots, state.eth1_data is set to get_eth1_data(ETH1_FOLLOW_DISTANCE) (as per process_eth1_data).
4. Next time get_eth1_vote is called, len(all_eth1_data) == 0 because previous_eth1_distance == ETH1_FOLLOW_DISTANCE.
5. The lambda in the max function raises a ValueError because v is not in all_eth1_data.

[paulhauner]

This was an accidental submission, please give me some time finish it (and perhaps realize it's invalid).

[paulhauner]

Alrighty, I'm finished with this and would still like to raise it as an issue.

[djrtwo]

You are correct. The current language was assuming that eth1data wasn't updated in state until the end of the period. Because we opportunistically update eth1data in the middle of the voting period if enough votes, we can't rely on it being stable.

I agree that some more executable code and some more consensus tests would be useful, but for now I'm going to expedite the change in language through to get it out immediately with v0.9.1.

[djrtwo]

addressed via clarification in the text in #1469
Closing for now. We can address further executability and testing in subsequent issues/prs