From c90af3cd631cbba3b51d042ac0ef16dfd5458e59 Mon Sep 17 00:00:00 2001 From: behzad nouri Date: Tue, 13 Jul 2021 22:32:59 +0000 Subject: [PATCH] removes id from push_lowest_slot args (#18645) push_lowest_slot cannot sign the new crds-value unless the id (pubkey) argument passed-in is the same pubkey as in ClusterInfo::keypair(), in which case the id argument is redundant: https://github.com/solana-labs/solana/blob/bb41cf346/gossip/src/cluster_info.rs#L824-L845 Additionally, the lookup is done with self.id(), but insert is done with the id argument, which is logically a bug. --- core/src/cluster_slots_service.rs | 25 +++++++++++++------------ gossip/src/cluster_info.rs | 24 ++++++++++++------------ gossip/src/crds_value.rs | 2 +- 3 files changed, 26 insertions(+), 25 deletions(-) diff --git a/core/src/cluster_slots_service.rs b/core/src/cluster_slots_service.rs index 5b455fa5f1..88da52ca33 100644 --- a/core/src/cluster_slots_service.rs +++ b/core/src/cluster_slots_service.rs @@ -4,7 +4,7 @@ use solana_gossip::cluster_info::ClusterInfo; use solana_ledger::blockstore::Blockstore; use solana_measure::measure::Measure; use solana_runtime::bank_forks::BankForks; -use solana_sdk::{clock::Slot, pubkey::Pubkey}; +use solana_sdk::clock::Slot; use std::{ sync::{ atomic::{AtomicBool, Ordering}, @@ -43,8 +43,7 @@ impl ClusterSlotsService { cluster_slots_update_receiver: ClusterSlotsUpdateReceiver, exit: Arc, ) -> Self { - let id = cluster_info.id(); - Self::initialize_lowest_slot(id, &blockstore, &cluster_info); + Self::initialize_lowest_slot(&blockstore, &cluster_info); Self::initialize_epoch_slots(&bank_forks, &cluster_info); let t_cluster_slots_service = Builder::new() .name("solana-cluster-slots-service".to_string()) @@ -93,10 +92,9 @@ impl ClusterSlotsService { } }; let new_root = bank_forks.read().unwrap().root(); - let id = cluster_info.id(); let mut lowest_slot_elapsed = Measure::start("lowest_slot_elapsed"); let lowest_slot = blockstore.lowest_slot(); - Self::update_lowest_slot(&id, lowest_slot, &cluster_info); + Self::update_lowest_slot(lowest_slot, &cluster_info); lowest_slot_elapsed.stop(); let mut process_cluster_slots_updates_elapsed = Measure::start("process_cluster_slots_updates_elapsed"); @@ -151,16 +149,16 @@ impl ClusterSlotsService { } } - fn initialize_lowest_slot(id: Pubkey, blockstore: &Blockstore, cluster_info: &ClusterInfo) { + fn initialize_lowest_slot(blockstore: &Blockstore, cluster_info: &ClusterInfo) { // Safe to set into gossip because by this time, the leader schedule cache should // also be updated with the latest root (done in blockstore_processor) and thus // will provide a schedule to window_service for any incoming shreds up to the // last_confirmed_epoch. - cluster_info.push_lowest_slot(id, blockstore.lowest_slot()); + cluster_info.push_lowest_slot(blockstore.lowest_slot()); } - fn update_lowest_slot(id: &Pubkey, lowest_slot: Slot, cluster_info: &ClusterInfo) { - cluster_info.push_lowest_slot(*id, lowest_slot); + fn update_lowest_slot(lowest_slot: Slot, cluster_info: &ClusterInfo) { + cluster_info.push_lowest_slot(lowest_slot); } fn initialize_epoch_slots(bank_forks: &RwLock, cluster_info: &ClusterInfo) { @@ -179,15 +177,18 @@ impl ClusterSlotsService { #[cfg(test)] mod test { - use super::*; - use solana_gossip::{cluster_info::Node, crds_value::CrdsValueLabel}; + use { + super::*, + solana_gossip::{cluster_info::Node, crds_value::CrdsValueLabel}, + solana_sdk::pubkey::Pubkey, + }; #[test] pub fn test_update_lowest_slot() { let pubkey = Pubkey::new_unique(); let node_info = Node::new_localhost_with_pubkey(&pubkey); let cluster_info = ClusterInfo::new_with_invalid_keypair(node_info.info); - ClusterSlotsService::update_lowest_slot(&pubkey, 5, &cluster_info); + ClusterSlotsService::update_lowest_slot(5, &cluster_info); cluster_info.flush_push_queue(); let lowest = { let label = CrdsValueLabel::LowestSlot(pubkey); diff --git a/gossip/src/cluster_info.rs b/gossip/src/cluster_info.rs index 352c098e65..6731266744 100644 --- a/gossip/src/cluster_info.rs +++ b/gossip/src/cluster_info.rs @@ -821,20 +821,20 @@ impl ClusterInfo { ) } - pub fn push_lowest_slot(&self, id: Pubkey, min: Slot) { - let now = timestamp(); - let last = self - .gossip - .read() - .unwrap() - .crds - .get(&CrdsValueLabel::LowestSlot(self.id())) - .and_then(|x| x.value.lowest_slot()) - .map(|x| x.lowest) - .unwrap_or(0); + pub fn push_lowest_slot(&self, min: Slot) { + let self_pubkey = self.id(); + let last = { + let gossip = self.gossip.read().unwrap(); + gossip + .crds + .get_lowest_slot(self_pubkey) + .map(|x| x.lowest) + .unwrap_or_default() + }; if min > last { + let now = timestamp(); let entry = CrdsValue::new_signed( - CrdsData::LowestSlot(0, LowestSlot::new(id, min, now)), + CrdsData::LowestSlot(0, LowestSlot::new(self_pubkey, min, now)), &self.keypair(), ); self.local_message_pending_push_queue diff --git a/gossip/src/crds_value.rs b/gossip/src/crds_value.rs index 5e58b8d215..1976d2781e 100644 --- a/gossip/src/crds_value.rs +++ b/gossip/src/crds_value.rs @@ -83,7 +83,7 @@ impl Signable for CrdsValue { pub enum CrdsData { ContactInfo(ContactInfo), Vote(VoteIndex, Vote), - LowestSlot(u8, LowestSlot), + LowestSlot(/*DEPRECATED:*/ u8, LowestSlot), SnapshotHashes(SnapshotHash), AccountsHashes(SnapshotHash), EpochSlots(EpochSlotsIndex, EpochSlots),