Use parent_root for finalized chain check

[paulhauner]

Summary

Use block.parent_root instead of hash_tree_root(block) to determine if the block descends from the finalized root.

This has two outcomes:

1. Removes a single iteration from the check (probably trivial).
2. Allows for storing the block in store.blocks only if it is valid.

Reasoning

I will make an argument that the following two checks are equal:

Check 1 (existing)

assert get_ancestor(store, hash_tree_root(block), finalized_slot) == store.finalized_checkpoint.root

Check 2 (proposed)

assert get_ancestor(store, block.parent_root, finalized_slot) == store.finalized_checkpoint.root

Argument

In get_ancestor, there are effectively two paths:

1. if block.slot > slot, recurse using block.parent_root.
2. Return root.

Considering we've established that prior to both checks we have assert block.slot > finalized_slot, the only possible path in get_ancestor is (1). This recursion in path 1 is equivalent to check 2.

Why am I raising this?

Optimization

Firstly, this removes a redundant block lookup. However, it's not significant and shouldn't be a driving factor, IMO.

Ergonomics

My motivation is that it allows for calling store.blocks[hash_tree_root(block)] = block only once the block is valid. This is important for implementations (like Lighthouse) because it means we don't need to roll back on an invalid block.

I'd like to implement this for Lighthouse and similar to #1880 it would nice to get feedback from spec authors and link to this PR in our code.

Spec bug

Seeing as the spec doesn't implement a rollback, I also believe that this PR fixes a bug that we can call: "Fork choice store is corrupted if an invalid block is suppplied to on_block".

Consider we have some signed_block with an invalid signature and it is the first block to be added to Store after genesis. In on_block that block will be added to store.blocks. Then, the invalid signature will cause on_block to terminate prior to adding the respective state to store.block_states.

Consider that there are now two objects in store.blocks:

• The genesis block (genesis_block)
• The invalid block (invalid_block)

But only one object in store.block_states:

• The genesis block state

Now consider the call stack the next time get_head is called:

1. get_head(store)
2. get_filtered_block_store(store)
3. filter_block_tree(store, tree_hash_root(genesis_block_root), {})
4. filter_block_tree(store, tree_hash_root(invalid_block), {})

During (4) we will run:

head_state = store.block_states[tree_hash_root(invalid_block)]

This will raise an exception.

[terencechain]

Thanks @paulhauner for raising this. Prysm implements the same approach. I'm also curious to hear feedbacks with this approach. My apology, I should have brought this up earlier. It totally slipped my mind

https://github.com/prysmaticlabs/prysm/blob/master/beacon-chain/blockchain/process_block_helpers.go#L63

[djrtwo]

As discussed out of band, agreed on the modification here, but also want to add a note to spec that signals that any failed assertion in a handler should result in a no-op to the store.

Doing a quick review to make sure there aren't any other issues like this