Remove impossible branch in forkchoice

[potuz]

Replace an impossible branch in forkchoice with an assert of this impossibility.

The message's slot being >= than the block slot is asserted in validate_on_attestation.k

[kevaundray]

Seems like the == case was/is not impossible.

To separate the two cases:

• message.slot < block.slot was impossible because of

  consensus-specs/specs/gloas/fork-choice.md

  Line 562 in 7e33b9f

  assert block_slot <= attestation.data.slot

• Returning false on message.slot == block.slot was a bug?

[fradamt]

Like Kev said, the message.slot == block.slot case is possible. However, returning False there was correct. Consider this case:

• Slot S: Block B proposed
• Slot S: Validator attests to B (we assert data.index=0 since same slot, so message.payload_present=False)
• Fork-choice runs, comparing EMPTY vs FULL for block B

With current code:

• is_supporting_vote(store, (B, EMPTY), message) → False
• is_supporting_vote(store, (B, FULL), message) → False

With this, in both cases we return node.payload_status==PAYLOAD_STATUS_EMPTY, since message.payload_present is False. Therefore:

• is_supporting_vote(store, (B, EMPTY), message) → True
• is_supporting_vote(store, (B, FULL), message) → False

The first case is wrong, this vote should not support (B, EMPTY), only (B, PENDING)

This would be correct:

      assert message.slot >= block.slot                                                                                 
      if message.slot == block.slot:                                                                                                                                                                                                                                   
          return False 

[potuz]

Now I'm not sure if the PR is even worth merging. The assert was mostly to show that we can't really be dealing with attestations from the future. But the fact that we still need to deal with same slot attestations and return a consistent value on both branches for empty and full, perhaps as it's currently implemented ends up being more readable than with this extra assert.

[jihoonsong]

This is not the first PR that tries to refactor this code to improve readability. (See this for another example.) I think we can improve it by separating message.slot < block.slot and message.slot == block.slot, each of which represents two different cases. Taking suggestions in PRs into account, options that I can come up with are:

1. Option 1.

# `block` should be part of the chain that the `message` supports.
assert message.slot >= block.slot
if message.slot == block.slot:
	# Same slot attestation cannot support `PAYLOAD_STATUS_FULL` or `PAYLOAD_STATUS_EMPTY`.
	return False 

2. Option 2.

if message.slot < block.slot:
	# `block` should be part of the chain that the `message` supports.
	return False
if message.slot == block.slot:
	# Same slot attestation cannot support `PAYLOAD_STATUS_FULL` or `PAYLOAD_STATUS_EMPTY`.
	return False 

Comments are auxiliary so we can remove it.