From e08139f949b6ea43d7cda0122b04da45169eea27 Mon Sep 17 00:00:00 2001 From: behzad nouri Date: Sat, 11 Dec 2021 14:47:20 +0000 Subject: [PATCH] uses Option for SlotMeta.last_index (#21775) SlotMeta.last_index may be unknown and current code is using u64::MAX to indicate that: https://github.com/solana-labs/solana/blob/6c108c8fc/ledger/src/blockstore_meta.rs#L169-L174 This lacks type-safety and can introduce bugs if not always checked for Several instances of slot_meta.last_index + 1 are also subject to overflow. This commit updates the type to Option. Backward compatibility is maintained by customizing serde serialize/deserialize implementations. --- core/src/repair_generic_traversal.rs | 4 +- ledger/src/blockstore.rs | 75 ++++++++++++++-------------- ledger/src/blockstore_meta.rs | 54 ++++++++++++-------- 3 files changed, 73 insertions(+), 60 deletions(-) diff --git a/core/src/repair_generic_traversal.rs b/core/src/repair_generic_traversal.rs index 8f35f67498..b5d7866782 100644 --- a/core/src/repair_generic_traversal.rs +++ b/core/src/repair_generic_traversal.rs @@ -57,7 +57,7 @@ pub fn get_unknown_last_index( .entry(slot) .or_insert_with(|| blockstore.meta(slot).unwrap()); if let Some(slot_meta) = slot_meta { - if slot_meta.known_last_index().is_none() { + if slot_meta.last_index.is_none() { let shred_index = blockstore.get_index(slot).unwrap(); let num_processed_shreds = if let Some(shred_index) = shred_index { shred_index.data().num_shreds() as u64 @@ -123,7 +123,7 @@ pub fn get_closest_completion( if slot_meta.is_full() { continue; } - if let Some(last_index) = slot_meta.known_last_index() { + if let Some(last_index) = slot_meta.last_index { let shred_index = blockstore.get_index(slot).unwrap(); let dist = if let Some(shred_index) = shred_index { let shred_count = shred_index.data().num_shreds() as u64; diff --git a/ledger/src/blockstore.rs b/ledger/src/blockstore.rs index 3b372c56cf..e89e02091b 100644 --- a/ledger/src/blockstore.rs +++ b/ledger/src/blockstore.rs @@ -1338,14 +1338,14 @@ impl Blockstore { // Check that we do not receive shred_index >= than the last_index // for the slot let last_index = slot_meta.last_index; - if shred_index >= last_index { + if last_index.map(|ix| shred_index >= ix).unwrap_or_default() { let leader_pubkey = leader_schedule .and_then(|leader_schedule| leader_schedule.slot_leader_at(slot, None)); let ending_shred: Cow> = self.get_data_shred_from_just_inserted_or_db( just_inserted_data_shreds, slot, - last_index, + last_index.unwrap(), ); if self @@ -1364,7 +1364,7 @@ impl Blockstore { ( "error", format!( - "Leader {:?}, slot {}: received index {} >= slot.last_index {}, shred_source: {:?}", + "Leader {:?}, slot {}: received index {} >= slot.last_index {:?}, shred_source: {:?}", leader_pubkey, slot, shred_index, last_index, shred_source ), String @@ -1509,7 +1509,14 @@ impl Blockstore { i64 ), ("slot", slot_meta.slot, i64), - ("last_index", slot_meta.last_index, i64), + ( + "last_index", + slot_meta + .last_index + .and_then(|ix| i64::try_from(ix).ok()) + .unwrap_or(-1), + i64 + ), ("num_repaired", num_repaired, i64), ("num_recovered", num_recovered, i64), ); @@ -1541,7 +1548,8 @@ impl Blockstore { .collect() } - pub fn get_data_shreds( + #[cfg(test)] + fn get_data_shreds( &self, slot: Slot, from_index: u64, @@ -3170,20 +3178,11 @@ fn update_slot_meta( slot_meta.first_shred_timestamp = timestamp() - slot_time_elapsed; } slot_meta.consumed = new_consumed; - slot_meta.last_index = { - // If the last index in the slot hasn't been set before, then - // set it to this shred index - if slot_meta.last_index == std::u64::MAX { - if is_last_in_slot { - u64::from(index) - } else { - std::u64::MAX - } - } else { - slot_meta.last_index - } - }; - + // If the last index in the slot hasn't been set before, then + // set it to this shred index + if is_last_in_slot && slot_meta.last_index.is_none() { + slot_meta.last_index = Some(u64::from(index)); + } update_completed_data_indexes( is_last_in_slot || is_last_in_data, index, @@ -4021,7 +4020,7 @@ pub mod tests { let num_shreds = shreds_per_slot[i as usize]; assert_eq!(meta.consumed, num_shreds); assert_eq!(meta.received, num_shreds); - assert_eq!(meta.last_index, num_shreds - 1); + assert_eq!(meta.last_index, Some(num_shreds - 1)); if i == num_slots - 1 { assert!(meta.next_slots.is_empty()); } else { @@ -4246,7 +4245,7 @@ pub mod tests { assert_eq!(meta.consumed, num_shreds); assert_eq!(meta.received, num_shreds); assert_eq!(meta.parent_slot, 0); - assert_eq!(meta.last_index, num_shreds - 1); + assert_eq!(meta.last_index, Some(num_shreds - 1)); assert!(meta.next_slots.is_empty()); assert!(meta.is_connected); } @@ -4271,7 +4270,7 @@ pub mod tests { .meta(0) .unwrap() .expect("Expected metadata object to exist"); - assert_eq!(meta.last_index, num_shreds - 1); + assert_eq!(meta.last_index, Some(num_shreds - 1)); if i != 0 { assert_eq!(result.len(), 0); assert!(meta.consumed == 0 && meta.received == num_shreds as u64); @@ -4449,9 +4448,9 @@ pub mod tests { } assert_eq!(meta.consumed, 0); if num_shreds % 2 == 0 { - assert_eq!(meta.last_index, num_shreds - 1); + assert_eq!(meta.last_index, Some(num_shreds - 1)); } else { - assert_eq!(meta.last_index, std::u64::MAX); + assert_eq!(meta.last_index, None); } blockstore.insert_shreds(even_shreds, None, false).unwrap(); @@ -4465,7 +4464,7 @@ pub mod tests { assert_eq!(meta.received, num_shreds); assert_eq!(meta.consumed, num_shreds); assert_eq!(meta.parent_slot, parent_slot); - assert_eq!(meta.last_index, num_shreds - 1); + assert_eq!(meta.last_index, Some(num_shreds - 1)); } } @@ -4719,7 +4718,7 @@ pub mod tests { // Slot 1 is not trunk because slot 0 hasn't been inserted yet assert!(!s1.is_connected); assert_eq!(s1.parent_slot, 0); - assert_eq!(s1.last_index, shreds_per_slot as u64 - 1); + assert_eq!(s1.last_index, Some(shreds_per_slot as u64 - 1)); // 2) Write to the second slot let shreds2 = shreds @@ -4731,7 +4730,7 @@ pub mod tests { // Slot 2 is not trunk because slot 0 hasn't been inserted yet assert!(!s2.is_connected); assert_eq!(s2.parent_slot, 1); - assert_eq!(s2.last_index, shreds_per_slot as u64 - 1); + assert_eq!(s2.last_index, Some(shreds_per_slot as u64 - 1)); // Check the first slot again, it should chain to the second slot, // but still isn't part of the trunk @@ -4739,7 +4738,7 @@ pub mod tests { assert_eq!(s1.next_slots, vec![2]); assert!(!s1.is_connected); assert_eq!(s1.parent_slot, 0); - assert_eq!(s1.last_index, shreds_per_slot as u64 - 1); + assert_eq!(s1.last_index, Some(shreds_per_slot as u64 - 1)); // 3) Write to the zeroth slot, check that every slot // is now part of the trunk @@ -4755,7 +4754,7 @@ pub mod tests { } else { assert_eq!(s.parent_slot, i - 1); } - assert_eq!(s.last_index, shreds_per_slot as u64 - 1); + assert_eq!(s.last_index, Some(shreds_per_slot as u64 - 1)); assert!(s.is_connected); } } @@ -4836,7 +4835,7 @@ pub mod tests { } else { assert_eq!(s.parent_slot, i - 1); } - assert_eq!(s.last_index, shreds_per_slot as u64 - 1); + assert_eq!(s.last_index, Some(shreds_per_slot as u64 - 1)); assert!(s.is_connected); } } @@ -4885,7 +4884,7 @@ pub mod tests { assert_eq!(s.parent_slot, i - 1); } - assert_eq!(s.last_index, shreds_per_slot as u64 - 1); + assert_eq!(s.last_index, Some(shreds_per_slot as u64 - 1)); // Other than slot 0, no slots should be part of the trunk if i != 0 { @@ -4921,7 +4920,7 @@ pub mod tests { assert_eq!(s.parent_slot, i - 1); } - assert_eq!(s.last_index, shreds_per_slot as u64 - 1); + assert_eq!(s.last_index, Some(shreds_per_slot as u64 - 1)); } } } @@ -5175,7 +5174,7 @@ pub mod tests { let meta = blockstore.meta(i).unwrap().unwrap(); assert_eq!(meta.received, 1); - assert_eq!(meta.last_index, 0); + assert_eq!(meta.last_index, Some(0)); if i != 0 { assert_eq!(meta.parent_slot, i - 1); assert_eq!(meta.consumed, 1); @@ -5484,7 +5483,7 @@ pub mod tests { // Trying to insert a shred with index > the "is_last" shred should fail if shred8.is_data() { - shred8.set_slot(slot_meta.last_index + 1); + shred8.set_slot(slot_meta.last_index.unwrap() + 1); } else { panic!("Shred in unexpected format") } @@ -5748,7 +5747,7 @@ pub mod tests { assert_eq!(slot_meta.consumed, num_shreds); assert_eq!(slot_meta.received, num_shreds); - assert_eq!(slot_meta.last_index, num_shreds - 1); + assert_eq!(slot_meta.last_index, Some(num_shreds - 1)); assert!(slot_meta.is_full()); let (shreds, _) = make_slot_entries(0, 0, 22); @@ -5757,7 +5756,7 @@ pub mod tests { assert_eq!(slot_meta.consumed, num_shreds); assert_eq!(slot_meta.received, num_shreds); - assert_eq!(slot_meta.last_index, num_shreds - 1); + assert_eq!(slot_meta.last_index, Some(num_shreds - 1)); assert!(slot_meta.is_full()); assert!(blockstore.has_duplicate_shreds_in_slot(0)); @@ -8444,7 +8443,7 @@ pub mod tests { assert_eq!(meta.consumed, 0); assert_eq!(meta.received, last_index + 1); assert_eq!(meta.parent_slot, 0); - assert_eq!(meta.last_index, last_index); + assert_eq!(meta.last_index, Some(last_index)); assert!(!blockstore.is_full(0)); } @@ -8460,7 +8459,7 @@ pub mod tests { assert_eq!(meta.consumed, num_shreds); assert_eq!(meta.received, num_shreds); assert_eq!(meta.parent_slot, 0); - assert_eq!(meta.last_index, num_shreds - 1); + assert_eq!(meta.last_index, Some(num_shreds - 1)); assert!(blockstore.is_full(0)); assert!(!blockstore.is_dead(0)); } diff --git a/ledger/src/blockstore_meta.rs b/ledger/src/blockstore_meta.rs index 6e6ed8a573..be5864595b 100644 --- a/ledger/src/blockstore_meta.rs +++ b/ledger/src/blockstore_meta.rs @@ -3,7 +3,7 @@ use { erasure::ErasureConfig, shred::{Shred, ShredType}, }, - serde::{Deserialize, Serialize}, + serde::{Deserialize, Deserializer, Serialize, Serializer}, solana_sdk::{clock::Slot, hash::Hash}, std::{ collections::BTreeSet, @@ -27,8 +27,10 @@ pub struct SlotMeta { // The timestamp of the first time a shred was added for this slot pub first_shred_timestamp: u64, // The index of the shred that is flagged as the last shred for this slot. - pub last_index: u64, + #[serde(with = "serde_compat")] + pub last_index: Option, // The slot height of the block this one derives from. + // TODO use Option instead. pub parent_slot: Slot, // The list of slots, each of which contains a block that derives // from this one. @@ -40,6 +42,27 @@ pub struct SlotMeta { pub completed_data_indexes: BTreeSet, } +// Serde implementation of serialize and deserialize for Option +// where None is represented as u64::MAX; for backward compatibility. +mod serde_compat { + use super::*; + + pub(super) fn serialize(val: &Option, serializer: S) -> Result + where + S: Serializer, + { + val.unwrap_or(u64::MAX).serialize(serializer) + } + + pub(super) fn deserialize<'de, D>(deserializer: D) -> Result, D::Error> + where + D: Deserializer<'de>, + { + let val = u64::deserialize(deserializer)?; + Ok((val != u64::MAX).then(|| val)) + } +} + #[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq)] /// Index recording presence/absence of shreds pub struct Index { @@ -168,38 +191,30 @@ impl ShredIndex { impl SlotMeta { pub fn is_full(&self) -> bool { - // last_index is std::u64::MAX when it has no information about how + // last_index is None when it has no information about how // many shreds will fill this slot. // Note: A full slot with zero shreds is not possible. - if self.last_index == std::u64::MAX { - return false; - } - // Should never happen - if self.consumed > self.last_index + 1 { + if self + .last_index + .map(|ix| self.consumed > ix + 1) + .unwrap_or_default() + { datapoint_error!( "blockstore_error", ( "error", format!( - "Observed a slot meta with consumed: {} > meta.last_index + 1: {}", + "Observed a slot meta with consumed: {} > meta.last_index + 1: {:?}", self.consumed, - self.last_index + 1 + self.last_index.map(|ix| ix + 1), ), String ) ); } - self.consumed == self.last_index + 1 - } - - pub fn known_last_index(&self) -> Option { - if self.last_index == std::u64::MAX { - None - } else { - Some(self.last_index) - } + Some(self.consumed) == self.last_index.map(|ix| ix + 1) } pub fn is_parent_set(&self) -> bool { @@ -217,7 +232,6 @@ impl SlotMeta { slot, parent_slot, is_connected: slot == 0, - last_index: std::u64::MAX, ..SlotMeta::default() } }