Fix a bug in fork pruning (#1507)

Extracted from https://github.com/sigp/lighthouse/pull/1380 because merging #1380 proves to be contentious.

Co-authored-by: Michael Sproul <michael@sigmaprime.io>
This commit is contained in:
Adam Szkoda 2020-08-12 07:00:00 +00:00
parent 61367efa64
commit 8a1a4051cf
3 changed files with 29 additions and 18 deletions

View File

@ -202,11 +202,6 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> Migrate<E, Hot, Cold>
old_finalized_block_hash: SignedBeaconBlockHash,
new_finalized_block_hash: SignedBeaconBlockHash,
) {
if let Err(e) = process_finalization(self.db.clone(), state_root, &new_finalized_state) {
// This migrator is only used for testing, so we just log to stderr without a logger.
eprintln!("Migration error: {:?}", e);
}
if let Err(e) = Self::prune_abandoned_forks(
self.db.clone(),
head_tracker,
@ -216,6 +211,11 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> Migrate<E, Hot, Cold>
) {
eprintln!("Pruning error: {:?}", e);
}
if let Err(e) = process_finalization(self.db.clone(), state_root, &new_finalized_state) {
// This migrator is only used for testing, so we just log to stderr without a logger.
eprintln!("Migration error: {:?}", e);
}
}
}
@ -325,6 +325,17 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
new_finalized_slot,
)) = rx.recv()
{
match Self::prune_abandoned_forks(
db.clone(),
head_tracker,
old_finalized_block_hash,
new_finalized_block_hash,
new_finalized_slot,
) {
Ok(()) => {}
Err(e) => warn!(log, "Block pruning failed: {:?}", e),
}
match process_finalization(db.clone(), state_root, &state) {
Ok(()) => {}
Err(Error::HotColdDBError(HotColdDBError::FreezeSlotUnaligned(slot))) => {
@ -342,17 +353,6 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> BackgroundMigrator<E, Ho
);
}
};
match Self::prune_abandoned_forks(
db.clone(),
head_tracker,
old_finalized_block_hash,
new_finalized_block_hash,
new_finalized_slot,
) {
Ok(()) => {}
Err(e) => warn!(log, "Block pruning failed: {:?}", e),
}
}
});

View File

@ -743,7 +743,7 @@ fn prunes_abandoned_fork_between_two_finalized_checkpoints() {
let (stray_blocks, stray_states, _, stray_head, _) = harness.add_stray_blocks(
harness.get_head_state(),
slot,
slots_per_epoch - 1,
slots_per_epoch - 3,
&faulty_validators,
);

View File

@ -208,6 +208,13 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> HotColdDB<E, Hot, Cold>
}
/// Fetch a state from the store.
///
/// If `slot` is provided then it will be used as a hint as to which database should
/// be checked. Importantly, if the slot hint is provided and indicates a slot that lies
/// in the freezer database, then only the freezer database will be accessed and `Ok(None)`
/// will be returned if the provided `state_root` doesn't match the state root of the
/// frozen state at `slot`. Consequently, if a state from a non-canonical chain is desired, it's
/// best to set `slot` to `None`, or call `load_hot_state` directly.
pub fn get_state(
&self,
state_root: &Hash256,
@ -217,7 +224,11 @@ impl<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> HotColdDB<E, Hot, Cold>
if let Some(slot) = slot {
if slot < self.get_split_slot() {
self.load_cold_state_by_slot(slot).map(Some)
// Although we could avoid a DB lookup by shooting straight for the
// frozen state using `load_cold_state_by_slot`, that would be incorrect
// in the case where the caller provides a `state_root` that's off the canonical
// chain. This way we avoid returning a state that doesn't match `state_root`.
self.load_cold_state(state_root)
} else {
self.load_hot_state(state_root)
}