gaspersic/solana
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

uses Option<u64> for SlotMeta.last_index (backport #21775) (#22915)

Browse Source 
* uses Option<u64> 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<u64>. Backward compatibility is
maintained by customizing serde serialize/deserialize implementations.

(cherry picked from commit e08139f949)

# Conflicts:
#	core/src/repair_generic_traversal.rs
#	ledger/src/blockstore.rs
#	ledger/src/blockstore_meta.rs

* removes mergify merge conflicts

Co-authored-by: behzad nouri <behzadnouri@gmail.com>
This commit is contained in:
mergify[bot]
2022-02-03 20:47:58 +00:00
committed by GitHub
parent 6e46511cc8
commit f32c33dd80
2 changed files with 72 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

77
ledger/src/blockstore.rs
View File

@ -46,7 +46,7 @@ use {
cell::RefCell, cell::RefCell,
cmp, cmp,
collections::{hash_map::Entry as HashMapEntry, BTreeMap, BTreeSet, HashMap, HashSet}, collections::{hash_map::Entry as HashMapEntry, BTreeMap, BTreeSet, HashMap, HashSet},
convert::TryInto, convert::{TryFrom, TryInto},
fs, fs,
io::{Error as IoError, ErrorKind}, io::{Error as IoError, ErrorKind},
path::{Path, PathBuf}, path::{Path, PathBuf},
@ -1345,14 +1345,14 @@ impl Blockstore {
// Check that we do not receive shred_index >= than the last_index // Check that we do not receive shred_index >= than the last_index
// for the slot // for the slot
let last_index = slot_meta.last_index; 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 let leader_pubkey = leader_schedule
.and_then(|leader_schedule| leader_schedule.slot_leader_at(slot, None)); .and_then(|leader_schedule| leader_schedule.slot_leader_at(slot, None));
let ending_shred: Cow<Vec<u8>> = self.get_data_shred_from_just_inserted_or_db( let ending_shred: Cow<Vec<u8>> = self.get_data_shred_from_just_inserted_or_db(
just_inserted_data_shreds, just_inserted_data_shreds,
slot, slot,
last_index, last_index.unwrap(),
); );
if self if self
@ -1371,7 +1371,7 @@ impl Blockstore {
( (
"error", "error",
format!( 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 leader_pubkey, slot, shred_index, last_index, shred_source
), ),
String String
@ -1516,7 +1516,14 @@ impl Blockstore {
i64 i64
), ),
("slot", slot_meta.slot, 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_repaired", num_repaired, i64),
("num_recovered", num_recovered, i64), ("num_recovered", num_recovered, i64),
); );
@ -1548,7 +1555,8 @@ impl Blockstore {
.collect() .collect()
} }
pub fn get_data_shreds( #[cfg(test)]
fn get_data_shreds(
&self, &self,
slot: Slot, slot: Slot,
from_index: u64, from_index: u64,
@ -3157,20 +3165,11 @@ fn update_slot_meta(
slot_meta.first_shred_timestamp = timestamp() - slot_time_elapsed; slot_meta.first_shred_timestamp = timestamp() - slot_time_elapsed;
} }
slot_meta.consumed = new_consumed; slot_meta.consumed = new_consumed;
slot_meta.last_index = { // If the last index in the slot hasn't been set before, then
// If the last index in the slot hasn't been set before, then // set it to this shred index
// set it to this shred index if is_last_in_slot && slot_meta.last_index.is_none() {
if slot_meta.last_index == std::u64::MAX { slot_meta.last_index = Some(u64::from(index));
if is_last_in_slot { }
u64::from(index)
} else {
std::u64::MAX
}
} else {
slot_meta.last_index
}
};
update_completed_data_indexes( update_completed_data_indexes(
is_last_in_slot || is_last_in_data, is_last_in_slot || is_last_in_data,
index, index,
@ -3968,7 +3967,7 @@ pub mod tests {
let num_shreds = shreds_per_slot[i as usize]; let num_shreds = shreds_per_slot[i as usize];
assert_eq!(meta.consumed, num_shreds); assert_eq!(meta.consumed, num_shreds);
assert_eq!(meta.received, 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 { if i == num_slots - 1 {
assert!(meta.next_slots.is_empty()); assert!(meta.next_slots.is_empty());
} else { } else {
@ -4201,7 +4200,7 @@ pub mod tests {
assert_eq!(meta.consumed, num_shreds); assert_eq!(meta.consumed, num_shreds);
assert_eq!(meta.received, num_shreds); assert_eq!(meta.received, num_shreds);
assert_eq!(meta.parent_slot, 0); 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.next_slots.is_empty());
assert!(meta.is_connected); assert!(meta.is_connected);
@ -4230,7 +4229,7 @@ pub mod tests {
.meta(0) .meta(0)
.unwrap() .unwrap()
.expect("Expected metadata object to exist"); .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 { if i != 0 {
assert_eq!(result.len(), 0); assert_eq!(result.len(), 0);
assert!(meta.consumed == 0 && meta.received == num_shreds as u64); assert!(meta.consumed == 0 && meta.received == num_shreds as u64);
@ -4424,9 +4423,9 @@ pub mod tests {
} }
assert_eq!(meta.consumed, 0); assert_eq!(meta.consumed, 0);
if num_shreds % 2 == 0 { if num_shreds % 2 == 0 {
assert_eq!(meta.last_index, num_shreds - 1); assert_eq!(meta.last_index, Some(num_shreds - 1));
} else { } else {
assert_eq!(meta.last_index, std::u64::MAX); assert_eq!(meta.last_index, None);
} }
blockstore.insert_shreds(even_shreds, None, false).unwrap(); blockstore.insert_shreds(even_shreds, None, false).unwrap();
@ -4440,7 +4439,7 @@ pub mod tests {
assert_eq!(meta.received, num_shreds); assert_eq!(meta.received, num_shreds);
assert_eq!(meta.consumed, num_shreds); assert_eq!(meta.consumed, num_shreds);
assert_eq!(meta.parent_slot, parent_slot); assert_eq!(meta.parent_slot, parent_slot);
assert_eq!(meta.last_index, num_shreds - 1); assert_eq!(meta.last_index, Some(num_shreds - 1));
} }
} }
@ -4695,7 +4694,7 @@ pub mod tests {
// Slot 1 is not trunk because slot 0 hasn't been inserted yet // Slot 1 is not trunk because slot 0 hasn't been inserted yet
assert!(!s1.is_connected); assert!(!s1.is_connected);
assert_eq!(s1.parent_slot, 0); 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 // 2) Write to the second slot
let shreds2 = shreds let shreds2 = shreds
@ -4707,7 +4706,7 @@ pub mod tests {
// Slot 2 is not trunk because slot 0 hasn't been inserted yet // Slot 2 is not trunk because slot 0 hasn't been inserted yet
assert!(!s2.is_connected); assert!(!s2.is_connected);
assert_eq!(s2.parent_slot, 1); 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, // Check the first slot again, it should chain to the second slot,
// but still isn't part of the trunk // but still isn't part of the trunk
@ -4715,7 +4714,7 @@ pub mod tests {
assert_eq!(s1.next_slots, vec![2]); assert_eq!(s1.next_slots, vec![2]);
assert!(!s1.is_connected); assert!(!s1.is_connected);
assert_eq!(s1.parent_slot, 0); 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 // 3) Write to the zeroth slot, check that every slot
// is now part of the trunk // is now part of the trunk
@ -4731,7 +4730,7 @@ pub mod tests {
} else { } else {
assert_eq!(s.parent_slot, i - 1); 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); assert!(s.is_connected);
} }
} }
@ -4814,7 +4813,7 @@ pub mod tests {
} else { } else {
assert_eq!(s.parent_slot, i - 1); 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); assert!(s.is_connected);
} }
} }
@ -4866,7 +4865,7 @@ pub mod tests {
assert_eq!(s.parent_slot, i - 1); 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 // Other than slot 0, no slots should be part of the trunk
if i != 0 { if i != 0 {
@ -4902,7 +4901,7 @@ pub mod tests {
assert_eq!(s.parent_slot, i - 1); 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));
} }
} }
} }
@ -5169,7 +5168,7 @@ pub mod tests {
let meta = blockstore.meta(i).unwrap().unwrap(); let meta = blockstore.meta(i).unwrap().unwrap();
assert_eq!(meta.received, 1); assert_eq!(meta.received, 1);
assert_eq!(meta.last_index, 0); assert_eq!(meta.last_index, Some(0));
if i != 0 { if i != 0 {
assert_eq!(meta.parent_slot, i - 1); assert_eq!(meta.parent_slot, i - 1);
assert_eq!(meta.consumed, 1); assert_eq!(meta.consumed, 1);
@ -5492,7 +5491,7 @@ pub mod tests {
// Trying to insert a shred with index > the "is_last" shred should fail // Trying to insert a shred with index > the "is_last" shred should fail
if shred8.is_data() { if shred8.is_data() {
shred8.set_slot(slot_meta.last_index + 1); shred8.set_slot(slot_meta.last_index.unwrap() + 1);
} else { } else {
panic!("Shred in unexpected format") panic!("Shred in unexpected format")
} }
@ -5771,7 +5770,7 @@ pub mod tests {
assert_eq!(slot_meta.consumed, num_shreds); assert_eq!(slot_meta.consumed, num_shreds);
assert_eq!(slot_meta.received, 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!(slot_meta.is_full());
let (shreds, _) = make_slot_entries(0, 0, 22); let (shreds, _) = make_slot_entries(0, 0, 22);
@ -5780,7 +5779,7 @@ pub mod tests {
assert_eq!(slot_meta.consumed, num_shreds); assert_eq!(slot_meta.consumed, num_shreds);
assert_eq!(slot_meta.received, 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!(slot_meta.is_full());
assert!(blockstore.has_duplicate_shreds_in_slot(0)); assert!(blockstore.has_duplicate_shreds_in_slot(0));
@ -8543,7 +8542,7 @@ pub mod tests {
assert_eq!(meta.consumed, 0); assert_eq!(meta.consumed, 0);
assert_eq!(meta.received, last_index + 1); assert_eq!(meta.received, last_index + 1);
assert_eq!(meta.parent_slot, 0); 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)); assert!(!blockstore.is_full(0));
} }
@ -8559,7 +8558,7 @@ pub mod tests {
assert_eq!(meta.consumed, num_shreds); assert_eq!(meta.consumed, num_shreds);
assert_eq!(meta.received, num_shreds); assert_eq!(meta.received, num_shreds);
assert_eq!(meta.parent_slot, 0); 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_full(0));
assert!(!blockstore.is_dead(0)); assert!(!blockstore.is_dead(0));
} }

46
ledger/src/blockstore_meta.rs
View File

@ -3,7 +3,7 @@ use {
erasure::ErasureConfig, erasure::ErasureConfig,
shred::{Shred, ShredType}, shred::{Shred, ShredType},
}, },
serde::{Deserialize, Serialize}, serde::{Deserialize, Deserializer, Serialize, Serializer},
solana_sdk::{clock::Slot, hash::Hash}, solana_sdk::{clock::Slot, hash::Hash},
std::{ std::{
collections::BTreeSet, collections::BTreeSet,
@ -27,8 +27,10 @@ pub struct SlotMeta {
// The timestamp of the first time a shred was added for this slot // The timestamp of the first time a shred was added for this slot
pub first_shred_timestamp: u64, pub first_shred_timestamp: u64,
// The index of the shred that is flagged as the last shred for this slot. // 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<u64>,
// The slot height of the block this one derives from. // The slot height of the block this one derives from.
// TODO use Option<Slot> instead.
pub parent_slot: Slot, pub parent_slot: Slot,
// The list of slots, each of which contains a block that derives // The list of slots, each of which contains a block that derives
// from this one. // from this one.
@ -40,6 +42,27 @@ pub struct SlotMeta {
pub completed_data_indexes: BTreeSet<u32>, pub completed_data_indexes: BTreeSet<u32>,
} }
// Serde implementation of serialize and deserialize for Option<u64>
// where None is represented as u64::MAX; for backward compatibility.
mod serde_compat {
use super::*;
pub(super) fn serialize<S>(val: &Option<u64>, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
val.unwrap_or(u64::MAX).serialize(serializer)
}
pub(super) fn deserialize<'de, D>(deserializer: D) -> Result<Option<u64>, D::Error>
where
D: Deserializer<'de>,
{
let val = u64::deserialize(deserializer)?;
Ok((val != u64::MAX).then(|| val))
}
}
#[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq)] #[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq)]
/// Index recording presence/absence of shreds /// Index recording presence/absence of shreds
pub struct Index { pub struct Index {
@ -168,30 +191,30 @@ impl ShredIndex {
impl SlotMeta { impl SlotMeta {
pub fn is_full(&self) -> bool { 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. // many shreds will fill this slot.
// Note: A full slot with zero shreds is not possible. // Note: A full slot with zero shreds is not possible.
if self.last_index == std::u64::MAX {
return false;
}
// Should never happen // 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!( datapoint_error!(
"blockstore_error", "blockstore_error",
( (
"error", "error",
format!( format!(
"Observed a slot meta with consumed: {} > meta.last_index + 1: {}", "Observed a slot meta with consumed: {} > meta.last_index + 1: {:?}",
self.consumed, self.consumed,
self.last_index + 1 self.last_index.map(|ix| ix + 1),
), ),
String String
) )
); );
} }
self.consumed == self.last_index + 1 Some(self.consumed) == self.last_index.map(|ix| ix + 1)
} }
pub fn is_parent_set(&self) -> bool { pub fn is_parent_set(&self) -> bool {
@ -209,7 +232,6 @@ impl SlotMeta {
slot, slot,
parent_slot, parent_slot,
is_connected: slot == 0, is_connected: slot == 0,
last_index: std::u64::MAX,
..SlotMeta::default() ..SlotMeta::default()
} }
} }