Fix bad assumption when checking finalized descendant (#1629)

## Issue Addressed

- Resolves #1616

## Proposed Changes

Fixes a bug where we are unable to read the finalized block from fork choice.

## Detail

I had made an assumption that the finalized block always has a parent root of `None`:

e5fc6bab48/consensus/fork_choice/src/fork_choice.rs (L749-L752)

This was a faulty assumption, we don't set parent *roots* to `None`. Instead we *sometimes* set parent *indices* to `None`, depending if this pruning condition is satisfied: 

e5fc6bab48/consensus/proto_array/src/proto_array.rs (L229-L232) 

The bug manifested itself like this:

1. We attempt to get the finalized block from fork choice
1. We try to check that the block is descendant of the finalized block (note: they're the same block).
1. We expect the parent root to be `None`, but it's actually the parent root of the finalized root.
1. We therefore end up checking if the parent of the finalized root is a descendant of itself. (note: it's an *ancestor* not a *descendant*).
1. We therefore declare that the finalized block is not a descendant of (or eq to) the finalized block. Bad.

## Additional Info

In reflection, I made a poor assumption in the quest to obtain a probably negligible performance gain. The performance gain wasn't worth the risk and we got burnt.
This commit is contained in:
Paul Hauner
2020-09-18 05:14:31 +00:00
parent 49ab414594
commit a17f74896a
3 changed files with 42 additions and 12 deletions

View File

@@ -745,12 +745,11 @@ where
/// Returns a `ProtoBlock` if the block is known **and** a descendant of the finalized root.
pub fn get_block(&self, block_root: &Hash256) -> Option<ProtoBlock> {
self.proto_array.get_block(block_root).filter(|block| {
// If available, use the parent_root to perform the lookup since it will involve one
// less lookup. This involves making the assumption that the finalized block will
// always have `block.parent_root` of `None`.
self.is_descendant_of_finalized(block.parent_root.unwrap_or(block.root))
})
if self.is_descendant_of_finalized(*block_root) {
self.proto_array.get_block(block_root)
} else {
None
}
}
/// Return `true` if `block_root` is equal to the finalized root, or a known descendant of it.