Disallow Voting For The Current Eth1Data in State

[nisdas]

It has been observed in mainnet that during certain voting periods, clients choose to vote on an already included eth1data. This ends up making the voting period redundant, as clients voting for an already included eth1data and its accompanying deposit count and root does not allow newer deposits to be included.

This ends up with deposits taking longer than expected(an additional voting period) to being included. @ajsutton has also brought up the fact that currently in the spec: https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/validator.md#eth1-data

    default_vote = votes_to_consider[len(votes_to_consider) - 1] if any(votes_to_consider) else state_eth1_data

We default to voting for the state_eth1_data if there are no other viable eth1data votes. We should instead disallow state_eth1_data votes and mark them as invalid when each block proposer is tallying the current votes in the state. This is so as to prefer strictly progressing eth1_data rather than allowing block proposers to vote for an already included eth1_data tuple which is in the state. This will prevent a whole voting period from being wasted and deposits from being delayed to be included, which has been show to happen in certain voting periods in mainnet.

There is limited downside to including a change such as this due to the fact this will only disallow voting for an already included on eth1_blockhash , so this will always lead to progressive eth1_data votes from each period to the next.

[seishun]

Is this the change you have in mind?

diff --git a/specs/phase0/validator.md b/specs/phase0/validator.md
index 14a5ecbb..43b86488 100644
--- a/specs/phase0/validator.md
+++ b/specs/phase0/validator.md
@@ -349,6 +349,7 @@ def get_eth1_vote(state: BeaconState, eth1_chain: Sequence[Eth1Block]) -> Eth1Da             is_candidate_block(block, period_start)
             # Ensure cannot move back to earlier deposit contract states
             and get_eth1_data(block).deposit_count >= state.eth1_data.deposit_count
+            and get_eth1_data(block) != state.eth1_data
         )
     ]

[nisdas]

That is part of it, also we would need to change this condition

    # Default vote on latest eth1 block data in the period range unless eth1 chain is not live
    # Non-substantive casting for linter
    state_eth1_data: Eth1Data = state.eth1_data
    default_vote = votes_to_consider[len(votes_to_consider) - 1] if any(votes_to_consider) else state_eth1_data

Make default_vote , a randomized value in that case. Or some other tuple rather than the current state_eth1_data

[seishun]

Is that a necessary change? Other validators won't consider a vote for state_eth1_data valid anyway.

[ajsutton]

Personally I like using state_eth1_data as the "I have no vote" option rather than a random one. As long as other validators don't consider it the most popular option and follow along there shouldn't be any need for random votes.

[nisdas]

Is that a necessary change? Other validators won't consider a vote for state_eth1_data valid anyway.

True, although if we are ignoring the vote anyway when tallying it, it does make it odd to have that as our default vote. But I don't feel too strongly on it, also as @ajsutton mentioned:

Personally I like using state_eth1_data as the "I have no vote" option rather than a random one. As long as other validators don't consider it the most popular option and follow along there shouldn't be any need for random votes.

That makes sense too, we could have state_eth1_data simply designate the throw away vote from the point of a proposer.

[ajsutton]

That makes sense too, we could have state_eth1_data simply designate the throw away vote from the point of a proposer

One nice advantage of this is that you are then very clearly able to tell the difference between "I have no vote" and "I'm voting for a chain that you just don't have access to" which a random block root might be. Admittedly it's exceptionally unlikely to be anything but a random block root but it's very easy to pick voting for the current eth1 data when querying the beacon chain without needing to also query an eth1 chain.

[ajsutton]

So to circle back on this - there is an interesting case I'd missed previously. The state eth1_data is updated as soon as the 50% threshold is reached, so the logic for excluding the current eth1_data from the votes to consider is actually a bit more complex - you ignore it until until it has reached 50%. At that point either we've already voted in a new eth1 block or there's no chance it will happen this voting period (we've had 50% default votes).

This makes me very much on the fence about which approach is better. Random is probably simpler now, but having tested our caching I do find it unfortunate that you can't tell the difference between "my cache is missing a block" and "that validator doesn't have an eth1 endpoint configured".

[mkalinin]

Could zeroed Eth1Data() be used to indicate the absence of connection to eth1 endpoint?

[ajsutton]

Could zeroed Eth1Data() be used to indicate the absence of connection to eth1 endpoint?

Yeah an all zero hash is probably the best possible answer. Easy to tell it's the default vote and simple to exclude it from the votes to consider/follow.