Restrict fork choice getters to finalized blocks (#1475)

## Issue Addressed

- Resolves #1451

## Proposed Changes

- Restricts the `contains_block` and `contains_block` so they only indicate a block is present if it descends from the finalized root. This helps to ensure that fork choice never points to a block that has been pruned from the database.
- Resolves #1451
- Before importing a block, double-check that its parent is known and a descendant of the finalized root.
- Split a big, monolithic block verification test into smaller tests. 

## Additional Notes

I suspect there would be a craftier way to do the `is_descendant_of_finalized` check, but we're a bit tight on time now and we can optimize later if it starts showing in benches.

## TODO

- [x] Tests
This commit is contained in:
Paul Hauner
2020-08-14 06:36:38 +00:00
parent b0a3731fff
commit 619ad106cf
5 changed files with 352 additions and 120 deletions

View File

@@ -48,6 +48,7 @@ use crate::{
},
metrics, BeaconChain, BeaconChainError, BeaconChainTypes, BeaconSnapshot,
};
use fork_choice::{ForkChoice, ForkChoiceStore};
use parking_lot::RwLockReadGuard;
use slog::{error, Logger};
use slot_clock::SlotClock;
@@ -62,7 +63,7 @@ use std::borrow::Cow;
use std::convert::TryFrom;
use std::fs;
use std::io::Write;
use store::{Error as DBError, HotStateSummary, StoreOp};
use store::{Error as DBError, HotColdDB, HotStateSummary, StoreOp};
use tree_hash::TreeHash;
use types::{
BeaconBlock, BeaconState, BeaconStateError, ChainSpec, CloneConfig, EthSpec, Hash256,
@@ -118,6 +119,13 @@ pub enum BlockError<T: EthSpec> {
block_slot: Slot,
finalized_slot: Slot,
},
/// The block conflicts with finalization, no need to propagate.
///
/// ## Peer scoring
///
/// It's unclear if this block is valid, but it conflicts with finality and shouldn't be
/// imported.
NotFinalizedDescendant { block_parent_root: Hash256 },
/// Block is already known, no need to re-import.
///
/// ## Peer scoring
@@ -397,6 +405,15 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
});
}
// Do not process a block that doesn't descend from the finalized root.
//
// We check this *before* we load the parent so that we can return a more detailed error.
let block = check_block_is_finalized_descendant::<T, _>(
block,
&chain.fork_choice.read(),
&chain.store,
)?;
let (mut parent, block) = load_parent(block, chain)?;
let block_root = get_block_root(&block);
@@ -779,6 +796,36 @@ fn check_block_against_finalized_slot<T: BeaconChainTypes>(
}
}
/// Returns `Ok(block)` if the block descends from the finalized root.
pub fn check_block_is_finalized_descendant<T: BeaconChainTypes, F: ForkChoiceStore<T::EthSpec>>(
block: SignedBeaconBlock<T::EthSpec>,
fork_choice: &ForkChoice<F, T::EthSpec>,
store: &HotColdDB<T::EthSpec, T::HotStore, T::ColdStore>,
) -> Result<SignedBeaconBlock<T::EthSpec>, BlockError<T::EthSpec>> {
if fork_choice.is_descendant_of_finalized(block.parent_root()) {
Ok(block)
} else {
// If fork choice does *not* consider the parent to be a descendant of the finalized block,
// then there are two more cases:
//
// 1. We have the parent stored in our database. Because fork-choice has confirmed the
// parent is *not* in our post-finalization DAG, all other blocks must be either
// pre-finalization or conflicting with finalization.
// 2. The parent is unknown to us, we probably want to download it since it might actually
// descend from the finalized root.
if store
.item_exists::<SignedBeaconBlock<T::EthSpec>>(&block.parent_root())
.map_err(|e| BlockError::BeaconChainError(e.into()))?
{
Err(BlockError::NotFinalizedDescendant {
block_parent_root: block.parent_root(),
})
} else {
Err(BlockError::ParentUnknown(Box::new(block)))
}
}
}
/// Performs simple, cheap checks to ensure that the block is relevant to be imported.
///
/// `Ok(block_root)` is returned if the block passes these checks and should progress with