removes position field in coding-shred-header

CodingShredHeader.position is equal to
  ShredCommonHeader.index - ShredCommonHeader.fec_set_index
and is so redundant. The extra position field can add bugs if not
consistent with index and fec_set_index.

core/src/retransmit_stage.rs

@@ -787,7 +787,7 @@ mod tests {
         assert_eq!(check_if_already_received(&packet, &shreds_received), None);
         assert_eq!(check_if_already_received(&packet, &shreds_received), None);
 
-        let shred = Shred::new_empty_coding(slot, index, 0, 1, 1, 0, version);
+        let shred = Shred::new_empty_coding(slot, index, 0, 1, 1, version);
         shred.copy_to_packet(&mut packet);
         // Coding at (1, 5) passes
         assert_eq!(
@@ -797,7 +797,7 @@ mod tests {
         // then blocked
         assert_eq!(check_if_already_received(&packet, &shreds_received), None);
 
-        let shred = Shred::new_empty_coding(slot, index, 2, 1, 1, 0, version);
+        let shred = Shred::new_empty_coding(slot, index, 2, 1, 1, version);
         shred.copy_to_packet(&mut packet);
         // 2nd unique coding at (1, 5) passes
         assert_eq!(
@@ -807,7 +807,7 @@ mod tests {
         // same again is blocked
         assert_eq!(check_if_already_received(&packet, &shreds_received), None);
 
-        let shred = Shred::new_empty_coding(slot, index, 3, 1, 1, 0, version);
+        let shred = Shred::new_empty_coding(slot, index, 3, 1, 1, version);
         shred.copy_to_packet(&mut packet);
         // Another unique coding at (1, 5) always blocked
         assert_eq!(check_if_already_received(&packet, &shreds_received), None);

core/src/window_service.rs

@@ -682,7 +682,7 @@ mod test {
         );
 
         // If it's a coding shred, test that slot >= root
-        let (common, coding) = Shredder::new_coding_shred_header(5, 5, 5, 6, 6, 0, 0);
+        let (common, coding) = Shredder::new_coding_shred_header(5, 5, 5, 6, 6, 0);
         let mut coding_shred =
             Shred::new_empty_from_header(common, DataShredHeader::default(), coding);
         Shredder::sign_shred(&leader_keypair, &mut coding_shred);
@@ -767,7 +767,7 @@ mod test {
         use crate::serve_repair::RepairType;
         use std::net::{IpAddr, Ipv4Addr};
         solana_logger::setup();
-        let (common, coding) = Shredder::new_coding_shred_header(5, 5, 5, 6, 6, 0, 0);
+        let (common, coding) = Shredder::new_coding_shred_header(5, 5, 5, 6, 6, 0);
         let shred = Shred::new_empty_from_header(common, DataShredHeader::default(), coding);
         let mut shreds = vec![shred.clone(), shred.clone(), shred];
         let _from_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 8080);

ledger/src/blockstore.rs

@@ -12,7 +12,7 @@ use crate::{
     erasure::ErasureConfig,
     leader_schedule_cache::LeaderScheduleCache,
     next_slots_iterator::NextSlotsIterator,
-    shred::{Result as ShredResult, Shred, Shredder},
+    shred::{Result as ShredResult, Shred, Shredder, MAX_DATA_SHREDS_PER_FEC_BLOCK},
 };
 pub use crate::{blockstore_db::BlockstoreError, blockstore_meta::SlotMeta};
 use bincode::deserialize;
@@ -1213,21 +1213,16 @@ impl Blockstore {
     }
 
     fn should_insert_coding_shred(shred: &Shred, last_root: &RwLock<u64>) -> bool {
-        let slot = shred.slot();
         let shred_index = shred.index();
-
-        if shred.is_data() || shred_index < u32::from(shred.coding_header.position) {
-            return false;
-        }
-
-        let set_index = shred.common_header.fec_set_index;
-
-        !(shred.coding_header.num_coding_shreds == 0
-            || shred.coding_header.position >= shred.coding_header.num_coding_shreds
-            || std::u32::MAX - set_index < u32::from(shred.coding_header.num_coding_shreds) - 1
-            || slot <= *last_root.read().unwrap()
-            || shred.coding_header.num_coding_shreds as u32
-                > (8 * crate::shred::MAX_DATA_SHREDS_PER_FEC_BLOCK))
+        let fec_set_index = shred.common_header.fec_set_index;
+        let num_coding_shreds = shred.coding_header.num_coding_shreds as u32;
+        shred.is_code()
+            && shred_index >= fec_set_index
+            && shred_index - fec_set_index < num_coding_shreds
+            && num_coding_shreds != 0
+            && num_coding_shreds <= 8 * MAX_DATA_SHREDS_PER_FEC_BLOCK
+            && num_coding_shreds - 1 <= u32::MAX - fec_set_index
+            && shred.slot() > *last_root.read().unwrap()
     }
 
     fn insert_coding_shred(
@@ -1241,7 +1236,7 @@ impl Blockstore {
 
         // Assert guaranteed by integrity checks on the shred that happen before
         // `insert_coding_shred` is called
-        assert!(shred.is_code() && shred_index >= u64::from(shred.coding_header.position));
+        assert!(shred.is_code() && shred_index >= shred.common_header.fec_set_index as u64);
 
         // Commit step: commit all changes to the mutable structures at once, or none at all.
         // We don't want only a subset of these changes going through.
@@ -5468,7 +5463,7 @@ pub mod tests {
             let blockstore = Blockstore::open(&blockstore_path).unwrap();
 
             let slot = 1;
-            let (shred, coding) = Shredder::new_coding_shred_header(slot, 11, 11, 11, 11, 10, 0);
+            let (shred, coding) = Shredder::new_coding_shred_header(slot, 11, 11, 11, 11, 0);
             let coding_shred =
                 Shred::new_empty_from_header(shred, DataShredHeader::default(), coding);
 
@@ -5514,8 +5509,7 @@ pub mod tests {
             let last_root = RwLock::new(0);
 
             let slot = 1;
-            let (mut shred, coding) =
-                Shredder::new_coding_shred_header(slot, 11, 11, 11, 11, 10, 0);
+            let (mut shred, coding) = Shredder::new_coding_shred_header(slot, 11, 11, 11, 11, 0);
             let coding_shred = Shred::new_empty_from_header(
                 shred.clone(),
                 DataShredHeader::default(),
@@ -5564,7 +5558,7 @@ pub mod tests {
                     DataShredHeader::default(),
                     coding.clone(),
                 );
-                let index = coding_shred.coding_header.position - 1;
+                let index = coding_shred.index() - coding_shred.common_header.fec_set_index - 1;
                 coding_shred.set_index(index as u32);
 
                 assert!(!Blockstore::should_insert_coding_shred(
@@ -5594,7 +5588,9 @@ pub mod tests {
                     DataShredHeader::default(),
                     coding.clone(),
                 );
-                coding_shred.coding_header.num_coding_shreds = coding_shred.coding_header.position;
+                let num_coding_shreds =
+                    coding_shred.common_header.index - coding_shred.common_header.fec_set_index;
+                coding_shred.coding_header.num_coding_shreds = num_coding_shreds as u16;
                 assert!(!Blockstore::should_insert_coding_shred(
                     &coding_shred,
                     &last_root
@@ -5612,7 +5608,6 @@ pub mod tests {
                 coding_shred.common_header.fec_set_index = std::u32::MAX - 1;
                 coding_shred.coding_header.num_coding_shreds = 3;
                 coding_shred.common_header.index = std::u32::MAX - 1;
-                coding_shred.coding_header.position = 0;
                 assert!(!Blockstore::should_insert_coding_shred(
                     &coding_shred,
                     &last_root

ledger/src/shred.rs

@@ -200,7 +200,8 @@ pub struct DataShredHeader {
 pub struct CodingShredHeader {
     pub num_data_shreds: u16,
     pub num_coding_shreds: u16,
-    pub position: u16,
+    #[serde(rename = "position")]
+    __unused: u16,
 }
 
 #[derive(Clone, Debug, PartialEq)]
@@ -360,7 +361,6 @@ impl Shred {
         fec_set_index: u32,
         num_data: usize,
         num_code: usize,
-        position: usize,
         version: u16,
     ) -> Self {
         let (header, coding_header) = Shredder::new_coding_shred_header(
@@ -369,7 +369,6 @@ impl Shred {
             fec_set_index,
             num_data,
             num_code,
-            position,
             version,
         );
         Shred::new_empty_from_header(header, DataShredHeader::default(), coding_header)
@@ -735,7 +734,6 @@ impl Shredder {
         fec_set_index: u32,
         num_data: usize,
         num_code: usize,
-        position: usize,
         version: u16,
     ) -> (ShredCommonHeader, CodingShredHeader) {
         let header = ShredCommonHeader {
@@ -751,7 +749,7 @@ impl Shredder {
             CodingShredHeader {
                 num_data_shreds: num_data as u16,
                 num_coding_shreds: num_code as u16,
-                position: position as u16,
+                ..CodingShredHeader::default()
             },
         )
     }
@@ -797,7 +795,6 @@ impl Shredder {
                     fec_set_index,
                     num_data,
                     num_coding,
-                    i, // position
                     version,
                 );
                 shred.payload[SIZE_OF_CODING_SHRED_HEADERS..].copy_from_slice(parity);
@@ -1923,7 +1920,7 @@ pub mod tests {
         );
         assert_eq!(stats.index_overrun, 4);
 
-        let shred = Shred::new_empty_coding(8, 2, 10, 30, 4, 7, 200);
+        let shred = Shred::new_empty_coding(8, 2, 10, 30, 4, 200);
         shred.copy_to_packet(&mut packet);
         assert_eq!(
             Some((8, 2, false)),
@@ -1935,8 +1932,7 @@ pub mod tests {
         assert_eq!(None, get_shred_slot_index_type(&packet, &mut stats));
         assert_eq!(1, stats.index_out_of_bounds);
 
-        let (mut header, coding_header) =
-            Shredder::new_coding_shred_header(8, 2, 10, 30, 4, 7, 200);
+        let (mut header, coding_header) = Shredder::new_coding_shred_header(8, 2, 10, 30, 4, 200);
         header.shred_type = ShredType(u8::MAX);
         let shred = Shred::new_empty_from_header(header, DataShredHeader::default(), coding_header);
         shred.copy_to_packet(&mut packet);