From d9c259a2313f620b4b64b7cec0d55dd4234fbcd9 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Thu, 27 Jan 2022 00:48:00 +0800 Subject: [PATCH] Set the correct root in block commitment cache initialization (#22750) * Set the correct root in block commitment cache initialization * clean up test * bump --- core/src/validator.rs | 2 +- local-cluster/tests/local_cluster.rs | 51 ++++++++++++---------------- replica-node/src/replica_node.rs | 2 +- runtime/src/commitment.rs | 4 +-- 4 files changed, 26 insertions(+), 33 deletions(-) diff --git a/core/src/validator.rs b/core/src/validator.rs index f1eee5326..da28ded37 100644 --- a/core/src/validator.rs +++ b/core/src/validator.rs @@ -538,7 +538,7 @@ impl Validator { cluster_info.restore_contact_info(ledger_path, config.contact_save_interval); let cluster_info = Arc::new(cluster_info); let mut block_commitment_cache = BlockCommitmentCache::default(); - block_commitment_cache.initialize_slots(bank.slot()); + block_commitment_cache.initialize_slots(bank.slot(), bank_forks.read().unwrap().root()); let block_commitment_cache = Arc::new(RwLock::new(block_commitment_cache)); let optimistically_confirmed_bank = diff --git a/local-cluster/tests/local_cluster.rs b/local-cluster/tests/local_cluster.rs index bb552d3c5..67732708a 100644 --- a/local-cluster/tests/local_cluster.rs +++ b/local-cluster/tests/local_cluster.rs @@ -1800,13 +1800,10 @@ fn test_validator_saves_tower() { let file_tower_storage = FileTowerStorage::new(ledger_path.clone()); // Wait for some votes to be generated - let mut last_replayed_root; loop { if let Ok(slot) = validator_client.get_slot_with_commitment(CommitmentConfig::processed()) { trace!("current slot: {}", slot); if slot > 2 { - // this will be the root next time a validator starts - last_replayed_root = slot; break; } } @@ -1818,35 +1815,31 @@ fn test_validator_saves_tower() { let tower1 = Tower::restore(&file_tower_storage, &validator_id).unwrap(); trace!("tower1: {:?}", tower1); assert_eq!(tower1.root(), 0); + assert!(tower1.last_voted_slot().is_some()); // Restart the validator and wait for a new root cluster.restart_node(&validator_id, validator_info, SocketAddrSpace::Unspecified); let validator_client = cluster.get_validator_client(&validator_id).unwrap(); - // Wait for the first root - loop { + // Wait for the first new root + let last_replayed_root = loop { #[allow(deprecated)] // This test depends on knowing the immediate root, without any delay from the commitment // service, so the deprecated CommitmentConfig::root() is retained if let Ok(root) = validator_client.get_slot_with_commitment(CommitmentConfig::root()) { trace!("current root: {}", root); - if root > last_replayed_root + 1 { - last_replayed_root = root; - break; + if root > 0 { + break root; } } sleep(Duration::from_millis(50)); - } + }; // Stop validator, and check saved tower - let recent_slot = validator_client - .get_slot_with_commitment(CommitmentConfig::processed()) - .unwrap(); let validator_info = cluster.exit_node(&validator_id); let tower2 = Tower::restore(&file_tower_storage, &validator_id).unwrap(); trace!("tower2: {:?}", tower2); assert_eq!(tower2.root(), last_replayed_root); - last_replayed_root = recent_slot; // Rollback saved tower to `tower1` to simulate a validator starting from a newer snapshot // without having to wait for that snapshot to be generated in this test @@ -1858,7 +1851,7 @@ fn test_validator_saves_tower() { let validator_client = cluster.get_validator_client(&validator_id).unwrap(); // Wait for a new root, demonstrating the validator was able to make progress from the older `tower1` - loop { + let new_root = loop { #[allow(deprecated)] // This test depends on knowing the immediate root, without any delay from the commitment // service, so the deprecated CommitmentConfig::root() is retained @@ -1869,17 +1862,18 @@ fn test_validator_saves_tower() { last_replayed_root ); if root > last_replayed_root { - break; + break root; } } sleep(Duration::from_millis(50)); - } + }; // Check the new root is reflected in the saved tower state let mut validator_info = cluster.exit_node(&validator_id); let tower3 = Tower::restore(&file_tower_storage, &validator_id).unwrap(); trace!("tower3: {:?}", tower3); - assert!(tower3.root() > last_replayed_root); + let tower3_root = tower3.root(); + assert!(tower3_root >= new_root); // Remove the tower file entirely and allow the validator to start without a tower. It will // rebuild tower from its vote account contents @@ -1889,26 +1883,25 @@ fn test_validator_saves_tower() { cluster.restart_node(&validator_id, validator_info, SocketAddrSpace::Unspecified); let validator_client = cluster.get_validator_client(&validator_id).unwrap(); - // Wait for a couple more slots to pass so another vote occurs - let current_slot = validator_client - .get_slot_with_commitment(CommitmentConfig::processed()) - .unwrap(); - loop { - if let Ok(slot) = validator_client.get_slot_with_commitment(CommitmentConfig::processed()) { - trace!("current_slot: {}, slot: {}", current_slot, slot); - if slot > current_slot + 1 { - break; + // Wait for another new root + let new_root = loop { + #[allow(deprecated)] + // This test depends on knowing the immediate root, without any delay from the commitment + // service, so the deprecated CommitmentConfig::root() is retained + if let Ok(root) = validator_client.get_slot_with_commitment(CommitmentConfig::root()) { + trace!("current root: {}, last tower root: {}", root, tower3_root); + if root > tower3_root { + break root; } } sleep(Duration::from_millis(50)); - } + }; cluster.close_preserve_ledgers(); let tower4 = Tower::restore(&file_tower_storage, &validator_id).unwrap(); trace!("tower4: {:?}", tower4); - // should tower4 advance 1 slot compared to tower3???? - assert_eq!(tower4.root(), tower3.root() + 1); + assert!(tower4.root() >= new_root); } fn save_tower(tower_path: &Path, tower: &Tower, node_keypair: &Keypair) { diff --git a/replica-node/src/replica_node.rs b/replica-node/src/replica_node.rs index 6e05ca1af..7f2a55f04 100644 --- a/replica-node/src/replica_node.rs +++ b/replica-node/src/replica_node.rs @@ -146,7 +146,7 @@ fn initialize_from_snapshot( OptimisticallyConfirmedBank::locked_from_bank_forks_root(&bank_forks); let mut block_commitment_cache = BlockCommitmentCache::default(); - block_commitment_cache.initialize_slots(bank0_slot); + block_commitment_cache.initialize_slots(bank0_slot, bank0_slot); let block_commitment_cache = Arc::new(RwLock::new(block_commitment_cache)); ReplicaBankInfo { diff --git a/runtime/src/commitment.rs b/runtime/src/commitment.rs index cb420e05b..192bdbd28 100644 --- a/runtime/src/commitment.rs +++ b/runtime/src/commitment.rs @@ -193,9 +193,9 @@ impl BlockCommitmentCache { self.commitment_slots.highest_confirmed_root = root; } - pub fn initialize_slots(&mut self, slot: Slot) { + pub fn initialize_slots(&mut self, slot: Slot, root: Slot) { self.commitment_slots.slot = slot; - self.commitment_slots.root = slot; + self.commitment_slots.root = root; } pub fn set_all_slots(&mut self, slot: Slot, root: Slot) {