removes shred wire layout specs from sigverify

behzadnouri · behzadnouri · commit 64c57ed72b7b · 2022-05-24T14:20:46.000-04:00
sigverify_shres relies on wire layout specs of shreds: https://github.com/solana-labs/solana/blob/0376ab41a/ledger/src/sigverify_shreds.rs#L39-L46 https://github.com/solana-labs/solana/blob/0376ab41a/ledger/src/sigverify_shreds.rs#L298-L305 In preparation of solana-labs#25237 which adds a new shred variant with different layout and signed message, this commit removes shred layout specification from sigverify and instead encapsulate that in shred module.
diff --git a/core/src/repair_response.rs b/core/src/repair_response.rs
@@ -91,20 +91,17 @@ mod test {
             .iter()
             .cloned()
             .collect();
-        let rv = verify_shred_cpu(&packet, &leader_slots);
-        assert_eq!(rv, Some(1));
+        assert!(verify_shred_cpu(&packet, &leader_slots));
 
         let wrong_keypair = Keypair::new();
         let leader_slots = [(slot, wrong_keypair.pubkey().to_bytes())]
             .iter()
             .cloned()
             .collect();
-        let rv = verify_shred_cpu(&packet, &leader_slots);
-        assert_eq!(rv, Some(0));
+        assert!(!verify_shred_cpu(&packet, &leader_slots));
 
         let leader_slots = HashMap::new();
-        let rv = verify_shred_cpu(&packet, &leader_slots);
-        assert_eq!(rv, None);
+        assert!(!verify_shred_cpu(&packet, &leader_slots));
     }
 
     #[test]
diff --git a/core/src/sigverify_shreds.rs b/core/src/sigverify_shreds.rs
@@ -2,8 +2,7 @@
 use {
     crate::{sigverify, sigverify_stage::SigVerifier},
     solana_ledger::{
-        leader_schedule_cache::LeaderScheduleCache, shred::Shred,
-        sigverify_shreds::verify_shreds_gpu,
+        leader_schedule_cache::LeaderScheduleCache, shred, sigverify_shreds::verify_shreds_gpu,
     },
     solana_perf::{self, packet::PacketBatch, recycler_cache::RecyclerCache},
     solana_runtime::bank_forks::BankForks,
@@ -32,10 +31,12 @@ impl ShredSigVerifier {
             recycler_cache: RecyclerCache::warmed(),
         }
     }
-    fn read_slots(batches: &[PacketBatch]) -> HashSet<u64> {
+    fn read_slots(batches: &[PacketBatch]) -> HashSet<Slot> {
         batches
             .iter()
-            .flat_map(|batch| batch.iter().filter_map(Shred::get_slot_from_packet))
+            .flat_map(PacketBatch::iter)
+            .map(shred::layout::get_shred)
+            .filter_map(shred::layout::get_slot)
             .collect()
     }
 }
diff --git a/ledger/src/blockstore.rs b/ledger/src/blockstore.rs
@@ -1856,7 +1856,7 @@ impl Blockstore {
             let upper_index = cmp::min(current_index, end_index);
             // the tick that will be used to figure out the timeout for this hole
             let data = db_iterator.value().expect("couldn't read value");
-            let reference_tick = u64::from(Shred::reference_tick_from_data(data).unwrap());
+            let reference_tick = u64::from(shred::layout::get_reference_tick(data).unwrap());
             if ticks_since_first_insert < reference_tick + MAX_TURBINE_DELAY_IN_TICKS {
                 // The higher index holes have not timed out yet
                 break 'outer;
diff --git a/ledger/src/shred.rs b/ledger/src/shred.rs
@@ -56,7 +56,7 @@ use {
     num_enum::{IntoPrimitive, TryFromPrimitive},
     serde::{Deserialize, Serialize},
     solana_entry::entry::{create_ticks, Entry},
-    solana_perf::packet::Packet,
+    solana_perf::packet::{deserialize_from_with_limit, Packet},
     solana_sdk::{
         clock::Slot,
         hash::{hashv, Hash},
@@ -315,7 +315,7 @@ impl Shred {
     }
 
     pub fn new_from_serialized_shred(shred: Vec<u8>) -> Result<Self, Error> {
-        Ok(match Self::shred_type_from_payload(&shred)? {
+        Ok(match layout::get_shred_type(&shred)? {
             ShredType::Code => Self::from(ShredCode::from_payload(shred)?),
             ShredType::Data => Self::from(ShredData::from_payload(shred)?),
         })
@@ -381,7 +381,7 @@ impl Shred {
 
     // Possibly zero pads bytes stored in blockstore.
     pub(crate) fn resize_stored_shred(shred: Vec<u8>) -> Result<Vec<u8>, Error> {
-        match Self::shred_type_from_payload(&shred)? {
+        match layout::get_shred_type(&shred)? {
             ShredType::Code => ShredCode::resize_stored_shred(shred),
             ShredType::Data => ShredData::resize_stored_shred(shred),
         }
@@ -430,16 +430,6 @@ impl Shred {
         self.common_header().shred_type
     }
 
-    fn shred_type_from_payload(shred: &[u8]) -> Result<ShredType, Error> {
-        match shred.get(OFFSET_OF_SHRED_TYPE) {
-            None => Err(Error::InvalidPayloadSize(shred.len())),
-            Some(shred_type) => match ShredType::try_from(*shred_type) {
-                Err(_) => Err(Error::InvalidShredType),
-                Ok(shred_type) => Ok(shred_type),
-            },
-        }
-    }
-
     pub fn is_data(&self) -> bool {
         self.shred_type() == ShredType::Data
     }
@@ -477,25 +467,6 @@ impl Shred {
         }
     }
 
-    // Get slot from a shred packet with partial deserialize
-    pub fn get_slot_from_packet(p: &Packet) -> Option<Slot> {
-        let slot_start = OFFSET_OF_SHRED_SLOT;
-        let slot_end = slot_start + SIZE_OF_SHRED_SLOT;
-        p.deserialize_slice(slot_start..slot_end).ok()
-    }
-
-    pub(crate) fn reference_tick_from_data(data: &[u8]) -> Result<u8, Error> {
-        const SHRED_FLAGS_OFFSET: usize = SIZE_OF_COMMON_SHRED_HEADER + std::mem::size_of::<u16>();
-        if Self::shred_type_from_payload(data)? != ShredType::Data {
-            return Err(Error::InvalidShredType);
-        }
-        let flags = match data.get(SHRED_FLAGS_OFFSET) {
-            None => return Err(Error::InvalidPayloadSize(data.len())),
-            Some(flags) => flags,
-        };
-        Ok(flags & ShredFlags::SHRED_TICK_REFERENCE_MASK.bits())
-    }
-
     pub fn verify(&self, pubkey: &Pubkey) -> bool {
         let message = self.signed_payload();
         self.signature().verify(pubkey.as_ref(), message)
@@ -524,6 +495,73 @@ impl Shred {
     }
 }
 
+// Helper methods to extract pieces of the shred from the payload
+// without deserializing the entire payload.
+pub mod layout {
+    use {super::*, std::ops::Range};
+
+    fn get_shred_size(packet: &Packet) -> usize {
+        if packet.meta.repair() {
+            packet.meta.size.saturating_sub(SIZE_OF_NONCE)
+        } else {
+            packet.meta.size
+        }
+    }
+
+    pub fn get_shred(packet: &Packet) -> &[u8] {
+        &packet.data[..get_shred_size(packet)]
+    }
+
+    pub(crate) fn get_signature(shred: &[u8]) -> Option<Signature> {
+        Some(Signature::new(shred.get(..SIZE_OF_SIGNATURE)?))
+    }
+
+    pub(crate) const fn get_signature_range() -> Range<usize> {
+        0..SIZE_OF_SIGNATURE
+    }
+
+    pub(super) fn get_shred_type(shred: &[u8]) -> Result<ShredType, Error> {
+        match shred.get(OFFSET_OF_SHRED_TYPE) {
+            None => Err(Error::InvalidPayloadSize(shred.len())),
+            Some(shred_type) => match ShredType::try_from(*shred_type) {
+                Err(_) => Err(Error::InvalidShredType),
+                Ok(shred_type) => Ok(shred_type),
+            },
+        }
+    }
+
+    pub fn get_slot(shred: &[u8]) -> Option<Slot> {
+        deserialize_from_with_limit(shred.get(OFFSET_OF_SHRED_SLOT..)?).ok()
+    }
+
+    pub(super) fn get_index(shred: &[u8]) -> Option<u32> {
+        deserialize_from_with_limit(shred.get(OFFSET_OF_SHRED_INDEX..)?).ok()
+    }
+
+    // Returns chunk of the payload which is signed.
+    pub(crate) fn get_signed_message(shred: &[u8]) -> Option<&[u8]> {
+        shred.get(SIZE_OF_SIGNATURE..)
+    }
+
+    // Returns slice range of the packet payload which is signed.
+    pub(crate) fn get_signed_message_range(packet: &Packet) -> Range<usize> {
+        SIZE_OF_SIGNATURE..get_shred_size(packet)
+    }
+
+    pub(crate) fn get_reference_tick(shred: &[u8]) -> Result<u8, Error> {
+        const SIZE_OF_PARENT_OFFSET: usize = std::mem::size_of::<u16>();
+        const OFFSET_OF_SHRED_FLAGS: usize = SIZE_OF_COMMON_SHRED_HEADER + SIZE_OF_PARENT_OFFSET;
+        if get_shred_type(shred)? != ShredType::Data {
+            return Err(Error::InvalidShredType);
+        }
+        let flags = match shred.get(OFFSET_OF_SHRED_FLAGS) {
+            None => return Err(Error::InvalidPayloadSize(shred.len())),
+            Some(flags) => flags,
+        };
+        Ok(flags & ShredFlags::SHRED_TICK_REFERENCE_MASK.bits())
+    }
+}
+
 impl From<ShredCode> for Shred {
     fn from(shred: ShredCode) -> Self {
         Self::ShredCode(shred)
@@ -538,50 +576,39 @@ impl From<ShredData> for Shred {
 
 // Get slot, index, and type from a packet with partial deserialize
 pub fn get_shred_slot_index_type(
-    p: &Packet,
+    packet: &Packet,
     stats: &mut ShredFetchStats,
 ) -> Option<(Slot, u32, ShredType)> {
-    let index_start = OFFSET_OF_SHRED_INDEX;
-    let index_end = index_start + SIZE_OF_SHRED_INDEX;
-    let slot_start = OFFSET_OF_SHRED_SLOT;
-    let slot_end = slot_start + SIZE_OF_SHRED_SLOT;
-
-    debug_assert!(index_end > slot_end);
-    debug_assert!(index_end > OFFSET_OF_SHRED_TYPE);
-
-    if index_end > p.meta.size {
+    let shred = layout::get_shred(packet);
+    if OFFSET_OF_SHRED_INDEX + SIZE_OF_SHRED_INDEX > shred.len() {
         stats.index_overrun += 1;
         return None;
     }
-
-    let index = match p.deserialize_slice(index_start..index_end) {
-        Ok(x) => x,
-        Err(_e) => {
-            stats.index_bad_deserialize += 1;
+    let shred_type = match layout::get_shred_type(shred) {
+        Ok(shred_type) => shred_type,
+        Err(_) => {
+            stats.bad_shred_type += 1;
             return None;
         }
     };
-
-    if index >= MAX_DATA_SHREDS_PER_SLOT as u32 {
-        stats.index_out_of_bounds += 1;
-        return None;
-    }
-
-    let slot = match p.deserialize_slice(slot_start..slot_end) {
-        Ok(x) => x,
-        Err(_e) => {
+    let slot = match layout::get_slot(shred) {
+        Some(slot) => slot,
+        None => {
             stats.slot_bad_deserialize += 1;
             return None;
         }
     };
-
-    let shred_type = match ShredType::try_from(p.data[OFFSET_OF_SHRED_TYPE]) {
-        Err(_) => {
-            stats.bad_shred_type += 1;
+    let index = match layout::get_index(shred) {
+        Some(index) => index,
+        None => {
+            stats.index_bad_deserialize += 1;
             return None;
         }
-        Ok(shred_type) => shred_type,
     };
+    if index >= MAX_DATA_SHREDS_PER_SLOT as u32 {
+        stats.index_out_of_bounds += 1;
+        return None;
+    }
     Some((slot, index, shred_type))
 }
 
@@ -898,9 +925,9 @@ mod tests {
         assert_eq!(shred, Shred::new_from_serialized_shred(payload).unwrap());
         assert_eq!(
             shred.reference_tick(),
-            Shred::reference_tick_from_data(&packet.data).unwrap()
+            layout::get_reference_tick(&packet.data).unwrap()
         );
-        assert_eq!(Shred::get_slot_from_packet(&packet), Some(shred.slot()));
+        assert_eq!(layout::get_slot(&packet.data), Some(shred.slot()));
         assert_eq!(
             get_shred_slot_index_type(&packet, &mut ShredFetchStats::default()),
             Some((shred.slot(), shred.index(), shred.shred_type()))
@@ -939,9 +966,9 @@ mod tests {
         assert_eq!(shred, Shred::new_from_serialized_shred(payload).unwrap());
         assert_eq!(
             shred.reference_tick(),
-            Shred::reference_tick_from_data(&packet.data).unwrap()
+            layout::get_reference_tick(&packet.data).unwrap()
         );
-        assert_eq!(Shred::get_slot_from_packet(&packet), Some(shred.slot()));
+        assert_eq!(layout::get_slot(&packet.data), Some(shred.slot()));
         assert_eq!(
             get_shred_slot_index_type(&packet, &mut ShredFetchStats::default()),
             Some((shred.slot(), shred.index(), shred.shred_type()))
@@ -985,7 +1012,7 @@ mod tests {
         packet.meta.size = payload.len();
         assert_eq!(shred.bytes_to_store(), payload);
         assert_eq!(shred, Shred::new_from_serialized_shred(payload).unwrap());
-        assert_eq!(Shred::get_slot_from_packet(&packet), Some(shred.slot()));
+        assert_eq!(layout::get_slot(&packet.data), Some(shred.slot()));
         assert_eq!(
             get_shred_slot_index_type(&packet, &mut ShredFetchStats::default()),
             Some((shred.slot(), shred.index(), shred.shred_type()))
@@ -1024,7 +1051,7 @@ mod tests {
             assert_eq!(shred.last_in_slot(), is_last_in_slot);
             assert_eq!(shred.reference_tick(), reference_tick.min(63u8));
             assert_eq!(
-                Shred::reference_tick_from_data(shred.payload()).unwrap(),
+                layout::get_reference_tick(shred.payload()).unwrap(),
                 reference_tick.min(63u8),
             );
         }
diff --git a/ledger/src/shredder.rs b/ledger/src/shredder.rs
@@ -353,7 +353,8 @@ mod tests {
     use {
         super::*,
         crate::shred::{
-            max_entries_per_n_shred, max_ticks_per_n_shreds, verify_test_data_shred, ShredType,
+            self, max_entries_per_n_shred, max_ticks_per_n_shreds, verify_test_data_shred,
+            ShredType,
         },
         bincode::serialized_size,
         matches::assert_matches,
@@ -519,7 +520,7 @@ mod tests {
         );
         data_shreds.iter().for_each(|s| {
             assert_eq!(s.reference_tick(), 5);
-            assert_eq!(Shred::reference_tick_from_data(s.payload()).unwrap(), 5);
+            assert_eq!(shred::layout::get_reference_tick(s.payload()).unwrap(), 5);
         });
 
         let deserialized_shred =
@@ -555,7 +556,7 @@ mod tests {
                 ShredFlags::SHRED_TICK_REFERENCE_MASK.bits()
             );
             assert_eq!(
-                Shred::reference_tick_from_data(s.payload()).unwrap(),
+                shred::layout::get_reference_tick(s.payload()).unwrap(),
                 ShredFlags::SHRED_TICK_REFERENCE_MASK.bits()
             );
         });
diff --git a/ledger/src/sigverify_shreds.rs b/ledger/src/sigverify_shreds.rs

Original file line number	Diff line number	Diff line change
`@@ -2,8 +2,7 @@`
`2`	`2`	`use {`
`3`	`3`	`crate::{sigverify, sigverify_stage::SigVerifier},`
`4`	`4`	`solana_ledger::{`
`5`		`- leader_schedule_cache::LeaderScheduleCache, shred::Shred,`
`6`		`- sigverify_shreds::verify_shreds_gpu,`
	`5`	`+ leader_schedule_cache::LeaderScheduleCache, shred, sigverify_shreds::verify_shreds_gpu,`
`7`	`6`	`},`
`8`	`7`	`solana_perf::{self, packet::PacketBatch, recycler_cache::RecyclerCache},`
`9`	`8`	`solana_runtime::bank_forks::BankForks,`
`@@ -32,10 +31,12 @@ impl ShredSigVerifier {`
`32`	`31`	`recycler_cache: RecyclerCache::warmed(),`
`33`	`32`	`}`
`34`	`33`	`}`
`35`		`- fn read_slots(batches: &[PacketBatch]) -> HashSet<u64> {`
	`34`	`+ fn read_slots(batches: &[PacketBatch]) -> HashSet<Slot> {`
`36`	`35`	`batches`
`37`	`36`	`.iter()`
`38`		`- .flat_map(\|batch\| batch.iter().filter_map(Shred::get_slot_from_packet))`
	`37`	`+ .flat_map(PacketBatch::iter)`
	`38`	`+ .map(shred::layout::get_shred)`
	`39`	`+ .filter_map(shred::layout::get_slot)`
`39`	`40`	`.collect()`
`40`	`41`	`}`
`41`	`42`	`}`