From 18508e2f03bce2bc3ba90afea7953385cf579cdd Mon Sep 17 00:00:00 2001
From: Paul Hauner <paul@paulhauner.com>
Date: Fri, 18 Sep 2020 03:03:35 +0000
Subject: [PATCH] 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`:

https://github.com/sigp/lighthouse/blob/e5fc6bab485fa54d7e518b325f4eb9201ba5c6a1/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:

https://github.com/sigp/lighthouse/blob/e5fc6bab485fa54d7e518b325f4eb9201ba5c6a1/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.
---
 .../beacon_chain/src/block_verification.rs    | 15 ++++++----
 consensus/fork_choice/src/fork_choice.rs      | 11 ++++----
 consensus/fork_choice/tests/tests.rs          | 28 +++++++++++++++++++
 3 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs
index 9f25186edaa..c6d8ab02b59 100644
--- a/beacon_node/beacon_chain/src/block_verification.rs
+++ b/beacon_node/beacon_chain/src/block_verification.rs
@@ -980,14 +980,17 @@ fn load_parent<T: BeaconChainTypes>(
         // exist in fork choice but not in the database yet. In such a case we simply
         // indicate that we don't yet know the parent.
         let root = block.parent_root();
-        let parent_block = if let Some(block) = chain
+        let parent_block = chain
             .get_block(&block.parent_root())
             .map_err(BlockError::BeaconChainError)?
-        {
-            block
-        } else {
-            return Err(BlockError::ParentUnknown(Box::new(block)));
-        };
+            .ok_or_else(|| {
+                // Return a `MissingBeaconBlock` error instead of a `ParentUnknown` error since
+                // we've already checked fork choice for this block.
+                //
+                // It's an internal error if the block exists in fork choice but not in the
+                // database.
+                BlockError::from(BeaconChainError::MissingBeaconBlock(block.parent_root()))
+            })?;
 
         // Load the parent blocks state from the database, returning an error if it is not found.
         // It is an error because if we know the parent block we should also know the parent state.
diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs
index 87818ac8435..804771330fb 100644
--- a/consensus/fork_choice/src/fork_choice.rs
+++ b/consensus/fork_choice/src/fork_choice.rs
@@ -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.
diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs
index d8b71243b40..ffa9cbe6bd4 100644
--- a/consensus/fork_choice/tests/tests.rs
+++ b/consensus/fork_choice/tests/tests.rs
@@ -377,6 +377,26 @@ impl ForkChoiceTest {
 
         self
     }
+
+    /// Check to ensure that we can read the finalized block. This is a regression test.
+    pub fn check_finalized_block_is_accessible(self) -> Self {
+        self.harness
+            .chain
+            .fork_choice
+            .write()
+            .get_block(
+                &self
+                    .harness
+                    .chain
+                    .head_info()
+                    .unwrap()
+                    .finalized_checkpoint
+                    .root,
+            )
+            .unwrap();
+
+        self
+    }
 }
 
 fn is_safe_to_update(slot: Slot) -> bool {
@@ -879,3 +899,11 @@ fn valid_attestation_skip_across_epoch() {
             |result| result.unwrap(),
         );
 }
+
+#[test]
+fn can_read_finalized_block() {
+    ForkChoiceTest::new()
+        .apply_blocks_while(|_, state| state.finalized_checkpoint.epoch == 0)
+        .apply_blocks(1)
+        .check_finalized_block_is_accessible();
+}