From 805cfde5bcfd7a5443405d062e6926b7020340b2 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 17 Sep 2020 15:08:02 +1000 Subject: [PATCH 1/4] Fix bad assumption when checking finalized desc. --- consensus/fork_choice/src/fork_choice.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) 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 { - 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. From 3957b037322d8298a3d77d60b2620c5e0a7d8d8a Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 18 Sep 2020 08:40:13 +1000 Subject: [PATCH 2/4] Add regression test --- consensus/fork_choice/tests/tests.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) 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(); +} From 0de48efe97a9937dc14407b280e9997232d826ae Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 18 Sep 2020 08:42:20 +1000 Subject: [PATCH 3/4] Return missing block error --- beacon_node/beacon_chain/src/block_verification.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 9f25186edaa..bd56b2cd33b 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -980,14 +980,12 @@ fn load_parent( // 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(|| { + 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. From 121e973aa28e8f20465d008bf46d146aa169692a Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 18 Sep 2020 08:46:24 +1000 Subject: [PATCH 4/4] Add comment --- beacon_node/beacon_chain/src/block_verification.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index bd56b2cd33b..c6d8ab02b59 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -984,6 +984,11 @@ fn load_parent( .get_block(&block.parent_root()) .map_err(BlockError::BeaconChainError)? .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())) })?;