Make test not fragile with improved arrangement

Author: ryoqun

core/src/validator.rs

@@ -158,7 +158,6 @@ pub struct ValidatorConfig {
     pub validator_exit: Arc<RwLock<Exit>>,
     pub no_wait_for_vote_to_start_leader: bool,
     pub accounts_shrink_ratio: AccountShrinkThreshold,
-    pub no_hard_fork_blockstore_root_reconcilation_for_local_cluster_test: bool,
 }
 
 impl Default for ValidatorConfig {
@@ -218,7 +217,6 @@ impl Default for ValidatorConfig {
             no_wait_for_vote_to_start_leader: true,
             accounts_shrink_ratio: AccountShrinkThreshold::default(),
             accounts_db_config: None,
-            no_hard_fork_blockstore_root_reconcilation_for_local_cluster_test: false,
         }
     }
 }
@@ -1292,20 +1290,18 @@ fn new_banks_from_ledger(
     if let Some(hard_fork_restart_slot) =
         maybe_cluster_restart_with_hard_fork(config, bank_forks.root_bank().slot())
     {
-        if !config.no_hard_fork_blockstore_root_reconcilation_for_local_cluster_test {
-            reconcile_blockstore_roots_with_external_source(
-                ExternalRootSource::HardFork(hard_fork_restart_slot),
-                &blockstore,
-                &mut last_blockstore_root,
-            )
-            .unwrap_or_else(|err| {
-                error!(
-                    "Failed to reconcile blockstore according to hard fork: {:?}",
-                    err
-                );
-                abort()
-            });
-        }
+        reconcile_blockstore_roots_with_external_source(
+            ExternalRootSource::HardFork(hard_fork_restart_slot),
+            &blockstore,
+            &mut last_blockstore_root,
+        )
+        .unwrap_or_else(|err| {
+            error!(
+                "Failed to reconcile blockstore according to hard fork: {:?}",
+                err
+            );
+            abort()
+        });
     }
     let tower = post_process_restored_tower(
         restored_tower,

ledger/src/blockstore.rs

@@ -1740,6 +1740,13 @@ impl Blockstore {
         self.meta_cf.put_bytes(slot, bytes)
     }
 
+    /// Manually update the meta for a slot.
+    /// Can interfere with automatic meta update and potentially break chaining.
+    /// Dangerous. Use with care.
+    pub fn put_meta(&self, slot: Slot, meta: &SlotMeta) -> Result<()> {
+        self.put_meta_bytes(slot, &bincode::serialize(meta)?)
+    }
+
     // Given a start and end entry index, find all the missing
     // indexes in the ledger in the range [start_index, end_index)
     // for the slot with the specified slot

ledger/src/blockstore_meta.rs

@@ -187,7 +187,11 @@ impl SlotMeta {
     }
 
     pub fn is_parent_set(&self) -> bool {
-        self.parent_slot != std::u64::MAX
+        self.parent_slot != Slot::MAX
+    }
+
+    pub fn unset_parent(&mut self) {
+        self.parent_slot = Slot::MAX;
     }
 
     pub fn clear_unconfirmed_slot(&mut self) {

local-cluster/src/validator_configs.rs

@@ -59,8 +59,6 @@ pub fn safe_clone_config(config: &ValidatorConfig) -> ValidatorConfig {
         no_wait_for_vote_to_start_leader: config.no_wait_for_vote_to_start_leader,
         accounts_shrink_ratio: config.accounts_shrink_ratio,
         accounts_db_config: config.accounts_db_config.clone(),
-        no_hard_fork_blockstore_root_reconcilation_for_local_cluster_test: config
-            .no_hard_fork_blockstore_root_reconcilation_for_local_cluster_test,
     }
 }
 

local-cluster/tests/local_cluster.rs

@@ -3159,9 +3159,9 @@ fn open_blockstore(ledger_path: &Path) -> Blockstore {
         })
 }
 
-fn purge_slots(blockstore: &Blockstore, start_slot: Slot, slot_count: Slot) {
-    blockstore.purge_from_next_slots(start_slot, start_slot + slot_count);
-    blockstore.purge_slots(start_slot, start_slot + slot_count, PurgeType::Exact);
+fn purge_slots_with_count(blockstore: &Blockstore, start_slot: Slot, slot_count: Slot) {
+    blockstore.purge_from_next_slots(start_slot, start_slot + slot_count - 1);
+    blockstore.purge_slots(start_slot, start_slot + slot_count - 1, PurgeType::Exact);
 }
 
 fn copy_blocks(end_slot: Slot, source: &Blockstore, dest: &Blockstore) {
@@ -3358,7 +3358,7 @@ fn do_test_optimistic_confirmation_violation_with_or_without_tower(with_tower: b
         remove_tower(&val_c_ledger_path, &validator_b_pubkey);
 
         let blockstore = open_blockstore(&val_c_ledger_path);
-        purge_slots(&blockstore, base_slot + 1, truncated_slots);
+        purge_slots_with_count(&blockstore, base_slot + 1, truncated_slots);
     }
     info!("Create validator A's ledger");
     {
@@ -3372,7 +3372,7 @@ fn do_test_optimistic_confirmation_violation_with_or_without_tower(with_tower: b
         copy_blocks(b_last_vote, &b_blockstore, &a_blockstore);
 
         // Purge uneccessary slots
-        purge_slots(&a_blockstore, next_slot_on_a + 1, truncated_slots);
+        purge_slots_with_count(&a_blockstore, next_slot_on_a + 1, truncated_slots);
     }
 
     // Step 3:
@@ -3404,7 +3404,7 @@ fn do_test_optimistic_confirmation_violation_with_or_without_tower(with_tower: b
     let validator_a_info = cluster.exit_node(&validator_a_pubkey);
     {
         let blockstore = open_blockstore(&val_a_ledger_path);
-        purge_slots(&blockstore, next_slot_on_a + 1, truncated_slots);
+        purge_slots_with_count(&blockstore, next_slot_on_a + 1, truncated_slots);
         if !with_tower {
             info!("Removing tower!");
             remove_tower(&val_a_ledger_path, &validator_a_pubkey);
@@ -3415,7 +3415,7 @@ fn do_test_optimistic_confirmation_violation_with_or_without_tower(with_tower: b
             // validator C yet.
             // Then it will be stuck on 27 unable to switch because C doesn't
             // have enough stake to generate a switching proof
-            purge_slots(&blockstore, next_slot_on_a, truncated_slots);
+            purge_slots_with_count(&blockstore, next_slot_on_a, truncated_slots);
         } else {
             info!("Not removing tower!");
         }
@@ -3563,7 +3563,7 @@ fn do_test_future_tower(cluster_mode: ClusterMode) {
             purged_slot_before_restart,
         );
         let blockstore = open_blockstore(&val_a_ledger_path);
-        purge_slots(&blockstore, purged_slot_before_restart, 100);
+        purge_slots_with_count(&blockstore, purged_slot_before_restart, 100);
     }
 
     cluster.restart_node(
@@ -3794,25 +3794,25 @@ fn create_simple_snapshot_config(ledger_path: &Path) -> SnapshotConfig {
     }
 }
 
-fn create_snapshot_to_hard_fork(ledger_path: &Path, snapshot_slot: Slot, hardforks: Vec<Slot>) {
+fn create_snapshot_to_hard_fork(
+    blockstore: &Blockstore,
+    snapshot_slot: Slot,
+    hard_forks: Vec<Slot>,
+) {
     let process_options = ProcessOptions {
         dev_halt_at_slot: Some(snapshot_slot),
-        new_hard_forks: Some(hardforks),
+        new_hard_forks: Some(hard_forks),
         poh_verify: false,
         ..ProcessOptions::default()
     };
+    let ledger_path = blockstore.ledger_path();
     let genesis_config = open_genesis_config(ledger_path, u64::max_value());
-    let blockstore =
-        Blockstore::open_with_access_type(ledger_path, AccessType::PrimaryOnly, None, true)
-            .unwrap_or_else(|e| {
-                panic!("Failed to open ledger at {:?}, err: {}", ledger_path, e);
-            });
     let snapshot_config = Some(create_simple_snapshot_config(ledger_path));
     let (accounts_package_sender, _) = channel();
     let (bank_forks, ..) = bank_forks_utils::load(
         &genesis_config,
-        &blockstore,
-        vec![blockstore.ledger_path().join("accounts")],
+        blockstore,
+        vec![ledger_path.join("accounts")],
         None,
         snapshot_config.as_ref(),
         process_options,
@@ -3840,7 +3840,9 @@ fn create_snapshot_to_hard_fork(ledger_path: &Path, snapshot_slot: Slot, hardfor
     );
 }
 
-fn do_test_hard_fork_with_or_without_gap_in_roots(with_gap: bool) {
+#[test]
+#[serial]
+fn test_hard_fork_with_gap_in_roots() {
     solana_logger::setup_with_default(RUST_LOG_FILTER);
 
     // First set up the cluster with 2 nodes
@@ -3865,7 +3867,6 @@ fn do_test_hard_fork_with_or_without_gap_in_roots(with_gap: bool) {
 
     let validator_config = ValidatorConfig {
         snapshot_config: Some(LocalCluster::create_dummy_load_only_snapshot_config()),
-        no_hard_fork_blockstore_root_reconcilation_for_local_cluster_test: with_gap,
         ..ValidatorConfig::default()
     };
     let mut config = ClusterConfig {
@@ -3915,7 +3916,23 @@ fn do_test_hard_fork_with_or_without_gap_in_roots(with_gap: bool) {
 
     // create hard-forked snapshot only for validator a, emulating the manual cluster restart
     // procedure with `solana-ledger-tool create-snapshot`
-    create_snapshot_to_hard_fork(&val_a_ledger_path, hard_fork_slot, vec![hard_fork_slot]);
+    let genesis_slot = 0;
+    {
+        let blockstore_a = Blockstore::open(&val_a_ledger_path).unwrap();
+        create_snapshot_to_hard_fork(&blockstore_a, hard_fork_slot, vec![hard_fork_slot]);
+
+        // Intentionally make solana-validator unbootable by replaying blocks from the genesis to
+        // ensure the hard-forked snapshot is used always.  Otherwise, we couldn't create a gap
+        // in the ledger roots column family reliably.
+        // There was a bug which caused the hard-forked snapshot at an unrooted slot to forget
+        // to root some slots (thus, creating a gap in roots, which shouldn't happen).
+        purge_slots_with_count(&blockstore_a, genesis_slot, 1);
+
+        let next_slot = genesis_slot + 1;
+        let mut meta = blockstore_a.meta(next_slot).unwrap().unwrap();
+        meta.unset_parent();
+        blockstore_a.put_meta(next_slot, &meta).unwrap();
+    }
 
     // strictly speaking, new_hard_forks isn't needed for validator a.
     // but when snapshot loading isn't working, you might see:
@@ -3958,48 +3975,25 @@ fn do_test_hard_fork_with_or_without_gap_in_roots(with_gap: bool) {
     let blockstore_a = Blockstore::open(&val_a_ledger_path).unwrap();
     let blockstore_b = Blockstore::open(&val_b_ledger_path).unwrap();
 
-    // collect all slots
-    let slots_a = AncestorIterator::new(common_last_vote, &blockstore_a).collect::<Vec<_>>();
-    let roots_a = blockstore_a
+    // collect all slot/root parents
+    let mut slots_a = AncestorIterator::new(common_last_vote, &blockstore_a).collect::<Vec<_>>();
+    let mut roots_a = blockstore_a
         .reversed_rooted_slot_iterator(common_root)
         .unwrap()
         .collect::<Vec<_>>();
+    // artifically restore the forcibly purged genesis only for the validator A just for the sake of
+    // following the final assertion.
+    slots_a.push(genesis_slot);
+    roots_a.push(genesis_slot);
+
     let slots_b = AncestorIterator::new(common_last_vote, &blockstore_b).collect::<Vec<_>>();
     let roots_b = blockstore_b
         .reversed_rooted_slot_iterator(common_root)
         .unwrap()
         .collect::<Vec<_>>();
 
-    // compare all slots
-    if !with_gap {
-        assert_eq!((slots_a, roots_a), (slots_b, roots_b));
-    } else {
-        // rough way to detect a gap with mostly similar slots...
-        assert_eq!(
-            (slots_a.first().unwrap(), roots_a.first().unwrap()),
-            (slots_b.first().unwrap(), roots_b.first().unwrap())
-        );
-        assert_eq!(
-            (slots_a.last().unwrap(), roots_a.last().unwrap()),
-            (slots_b.last().unwrap(), roots_b.last().unwrap())
-        );
-        assert_ne!((slots_a, roots_a), (slots_b, roots_b));
-    }
-}
-
-// the following two tests are rather fragile, depending on successful loading of the hard-forked snapshot.
-// so, make sure they aren't broken by testing (expected) failures as well with those conditioned
-// two actual test code invocations.
-#[test]
-#[serial]
-fn test_hard_fork_without_gap_in_roots() {
-    do_test_hard_fork_with_or_without_gap_in_roots(false);
-}
-
-#[test]
-#[serial]
-fn test_hard_fork_with_gap_in_roots() {
-    do_test_hard_fork_with_or_without_gap_in_roots(true);
+    // compare them all!
+    assert_eq!((slots_a, roots_a), (slots_b, roots_b));
 }
 
 fn run_test_load_program_accounts_partition(scan_commitment: CommitmentConfig) {