From d16d4056e9e2cbcfc31b1d3aa5ab49be6c3c2c72 Mon Sep 17 00:00:00 2001 From: Adam Szkoda Date: Tue, 11 Aug 2020 12:17:42 +0200 Subject: [PATCH 1/3] Reproduce the bug A bug in a test case hid a bug in the code: prunes_abandoned_fork_between_two_finalized_checkpoints: contrary to its name, the abandoned fork isn't actually wholly contained between two finalized checkpoints in terms of slot values. --- beacon_node/beacon_chain/tests/store_tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index 6ab2908bcec..bafef37882f 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -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, ); From 4dd911cd3a3ed128b2e820ec7c8cefa78cd1856a Mon Sep 17 00:00:00 2001 From: Adam Szkoda Date: Tue, 11 Aug 2020 12:19:24 +0200 Subject: [PATCH 2/3] Fix the bug Pruning stopped short of pruning the entire abandoned chain. It only pruned the head. This has to do with get_state() returning unexpected states based on slots for states that that lie below the "split point". Fix by doing pruning on the hot database (as it should be from the start) and not touching the cold database. --- beacon_node/beacon_chain/src/migrate.rs | 32 ++++++++++++------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/beacon_node/beacon_chain/src/migrate.rs b/beacon_node/beacon_chain/src/migrate.rs index 59079344c92..83eae0ae71d 100644 --- a/beacon_node/beacon_chain/src/migrate.rs +++ b/beacon_node/beacon_chain/src/migrate.rs @@ -202,11 +202,6 @@ impl, Cold: ItemStore> Migrate 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, Cold: ItemStore> Migrate ) { 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, Cold: ItemStore> BackgroundMigrator {} + 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, Cold: ItemStore> BackgroundMigrator {} - Err(e) => warn!(log, "Block pruning failed: {:?}", e), - } } }); From 59509607f321004043f1ef1d24c2a6c24babb751 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 30 Jul 2020 10:17:09 +1000 Subject: [PATCH 3/3] Fix non-canonical chain access in DB `get_state` --- beacon_node/store/src/hot_cold_store.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index 48917af7357..7bad9ef1567 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -208,6 +208,13 @@ impl, Cold: ItemStore> HotColdDB } /// 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, Cold: ItemStore> HotColdDB 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) }