Fork choice: avoid redundant call to get_ancestor

[paulhauner]

Summary

Fork choice on_block contains a redundant call to get_ancestor in the best case scenario.

Detail

Consider the following best-case scenario for the beacon chain:

• It is slot n and n % SLOTS_PER_EPOCH == 0.
• The transition from slot n - 1 to n caused both the finalized and justified checkpoints to increase.
• There are no forks in the chain.

First we will run the following:

    # Update justified checkpoint
    if state.current_justified_checkpoint.epoch > store.justified_checkpoint.epoch:
        if state.current_justified_checkpoint.epoch > store.best_justified_checkpoint.epoch:
            store.best_justified_checkpoint = state.current_justified_checkpoint
        if should_update_justified_checkpoint(store, state.current_justified_checkpoint):
            store.justified_checkpoint = state.current_justified_checkpoint

We will execute the last line in the above block, therefore store.justified_checkpoint == state.current_justified_checkpoint.

Next, we face if state.finalized_checkpoint.epoch > store.finalized_checkpoint.epoch (code block omitted for brevity). This condition will be true, so we will then be faced with:

        # Update justified if new justified is later than store justified
        # or if store justified is not in chain with finalized checkpoint
        if (
            state.current_justified_checkpoint.epoch > store.justified_checkpoint.epoch
            or get_ancestor(store, store.justified_checkpoint.root, finalized_slot) != store.finalized_checkpoint.root
        ):
            store.justified_checkpoint = state.current_justified_checkpoint

We know the left-hand-side of the or is false (we already set these two values to be equal), so we will always hit the right-hand-side (get_ancestor). The body of the if statement is a no-op, so the result of get_ancestor is redundant.

It's also worth noting that this get_ancestor call is particularly onerous seeing as it is based upon store.justified_checkpoint and we can't utilize the state.block_roots from the block that we are currently processing. (i.e., we must do a state read or a block-by-block reverse iteration).

Why am I raising this?

I know that suggesting runtime optimizations to the spec is generally discouraged, but:

1. Fork choice already sets a precedence of optimizing around calls to get_ancestor.
2. It is a non-substantive change.
3. I plan to implement this optimization and I'd like to be able to reference some rationale and perhaps feedback from the authors of the specification.

(1) and (2) are arguments to merge this PR, however I'm not particularly concerned whether or not it gets merged (i.e., not dying on this hill). (3) is the primary driving force at the moment, if you'd be willing to provide some comment :)

[djrtwo]

Yeah, agreed on analysis @paulhauner.

I just made another commit to further separate to the two paths if justified is different both for clarity in presentation and to avoid the call to get_ancestor in even more cases