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.

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;

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<Vec<u8>> = 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
-        // 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(
         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));
     }

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<u64>,
     // The slot height of the block this one derives from.
+    // TODO use Option<Slot> 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<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)]
 /// 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
+        Some(self.consumed) == self.last_index.map(|ix| ix + 1)
-    }
-
-    pub fn known_last_index(&self) -> Option<u64> {
-        if self.last_index == std::u64::MAX {
-            None
-        } else {
-            Some(self.last_index)
-        }
     }
 
     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()
         }
     }