removes first_coding_index from erasure recovery code (solana-labs#16646

) first_coding_index is the same as the set_index and is so redundant: https://github.com/solana-labs/solana/blob/37b8587d4/ledger/src/blockstore_meta.rs#L49-L60
brooksprumo · Apr 23, 2021 · 0319414 · 0319414
1 parent f59fe41
commit 0319414
Show file tree

Hide file tree

Showing 6 changed files with 24 additions and 62 deletions.
diff --git a/core/benches/shredder.rs b/core/benches/shredder.rs
@@ -152,9 +152,8 @@ fn bench_shredder_decoding(bencher: &mut Bencher) {
             coding_shreds[..].to_vec(),
             symbol_count,
             symbol_count,
-            0,
-            0,
-            1,
+            0, // first index
+            1, // slot
         )
         .unwrap();
     })

diff --git a/core/src/cluster_info.rs b/core/src/cluster_info.rs
@@ -3516,7 +3516,6 @@ mod tests {
     use itertools::izip;
     use rand::{seq::SliceRandom, SeedableRng};
     use rand_chacha::ChaChaRng;
-    use serial_test::serial;
     use solana_ledger::shred::Shredder;
     use solana_sdk::signature::{Keypair, Signer};
     use solana_vote_program::{vote_instruction, vote_state::Vote};
@@ -4787,8 +4786,7 @@ mod tests {
     }
 
     #[test]
-    #[serial]
-    #[ignore]
+    #[ignore] // TODO: debug why this is flaky on buildkite!
     fn test_pull_request_time_pruning() {
         let node = Node::new_localhost();
         let cluster_info = Arc::new(ClusterInfo::new_with_invalid_keypair(node.info));

diff --git a/ledger/src/blockstore.rs b/ledger/src/blockstore.rs
@@ -573,8 +573,7 @@ impl Blockstore {
         prev_inserted_codes: &mut HashMap<(u64, u64), Shred>,
         code_cf: &LedgerColumn<cf::ShredCode>,
     ) {
-        (erasure_meta.first_coding_index
-            ..erasure_meta.first_coding_index + erasure_meta.config.num_coding() as u64)
+        (erasure_meta.set_index..erasure_meta.set_index + erasure_meta.config.num_coding() as u64)
             .for_each(|i| {
                 if let Some(shred) = prev_inserted_codes
                     .remove(&(slot, i))
@@ -645,7 +644,6 @@ impl Blockstore {
             erasure_meta.config.num_data(),
             erasure_meta.config.num_coding(),
             set_index as usize,
-            erasure_meta.first_coding_index as usize,
             slot,
         ) {
             Self::submit_metrics(
@@ -1069,12 +1067,10 @@ impl Blockstore {
         );
 
         let erasure_meta = erasure_metas.entry((slot, set_index)).or_insert_with(|| {
-            let first_coding_index =
-                u64::from(shred.index()) - u64::from(shred.coding_header.position);
             self.erasure_meta_cf
                 .get((slot, set_index))
                 .expect("Expect database get to succeed")
-                .unwrap_or_else(|| ErasureMeta::new(set_index, first_coding_index, &erasure_config))
+                .unwrap_or_else(|| ErasureMeta::new(set_index, erasure_config))
         });
 
         if erasure_config != erasure_meta.config {
@@ -1128,10 +1124,10 @@ impl Blockstore {
     ) -> Option<Vec<u8>> {
         // Search for the shred which set the initial erasure config, either inserted,
         // or in the current batch in just_received_coding_shreds.
-        let coding_start = erasure_meta.first_coding_index;
-        let coding_end = coding_start + erasure_meta.config.num_coding() as u64;
+        let coding_indices = erasure_meta.set_index
+            ..erasure_meta.set_index + erasure_meta.config.num_coding() as u64;
         let mut conflicting_shred = None;
-        for coding_index in coding_start..coding_end {
+        for coding_index in coding_indices {
             let maybe_shred = self.get_coding_shred(slot, coding_index);
             if let Ok(Some(shred_data)) = maybe_shred {
                 let potential_shred = Shred::new_from_serialized_shred(shred_data).unwrap();

diff --git a/ledger/src/blockstore_meta.rs b/ledger/src/blockstore_meta.rs
@@ -51,8 +51,9 @@ pub struct ShredIndex {
 pub struct ErasureMeta {
     /// Which erasure set in the slot this is
     pub set_index: u64,
-    /// First coding index in the FEC set
-    pub first_coding_index: u64,
+    /// Deprecated field.
+    #[serde(rename = "first_coding_index")]
+    __unused: u64,
     /// Size of shards in this erasure set
     pub size: usize,
     /// Erasure configuration for this erasure set
@@ -184,21 +185,19 @@ impl SlotMeta {
 }
 
 impl ErasureMeta {
-    pub fn new(set_index: u64, first_coding_index: u64, config: &ErasureConfig) -> ErasureMeta {
+    pub fn new(set_index: u64, config: ErasureConfig) -> ErasureMeta {
         ErasureMeta {
             set_index,
-            first_coding_index,
-            size: 0,
-            config: *config,
+            config,
+            ..Self::default()
         }
     }
 
     pub fn status(&self, index: &Index) -> ErasureMetaStatus {
         use ErasureMetaStatus::*;
 
-        let num_coding = index.coding().present_in_bounds(
-            self.first_coding_index..self.first_coding_index + self.config.num_coding() as u64,
-        );
+        let coding_indices = self.set_index..self.set_index + self.config.num_coding() as u64;
+        let num_coding = index.coding().present_in_bounds(coding_indices);
         let num_data = index
             .data()
             .present_in_bounds(self.set_index..self.set_index + self.config.num_data() as u64);
@@ -263,7 +262,7 @@ mod test {
         let set_index = 0;
         let erasure_config = ErasureConfig::default();
 
-        let mut e_meta = ErasureMeta::new(set_index, set_index, &erasure_config);
+        let mut e_meta = ErasureMeta::new(set_index, erasure_config);
         let mut rng = thread_rng();
         let mut index = Index::new(0);
         e_meta.size = 1;

diff --git a/ledger/src/shred.rs b/ledger/src/shred.rs
@@ -845,7 +845,6 @@ impl Shredder {
         num_data: usize,
         num_coding: usize,
         first_index: usize,
-        first_code_index: usize,
         slot: Slot,
     ) -> std::result::Result<Vec<Shred>, reed_solomon_erasure::Error> {
         Self::verify_consistent_shred_payload_sizes(&"try_recovery()", &shreds)?;
@@ -859,8 +858,8 @@ impl Shredder {
             let mut shred_bufs: Vec<Vec<u8>> = shreds
                 .into_iter()
                 .flat_map(|shred| {
-                    let index =
-                        Self::get_shred_index(&shred, num_data, first_index, first_code_index);
+                    let offset = if shred.is_data() { 0 } else { num_data };
+                    let index = offset + shred.index() as usize;
                     let mut blocks = Self::fill_in_missing_shreds(
                         num_data,
                         num_coding,
@@ -960,19 +959,6 @@ impl Shredder {
         Ok(Self::reassemble_payload(num_data, data_shred_bufs))
     }
 
-    fn get_shred_index(
-        shred: &Shred,
-        num_data: usize,
-        first_data_index: usize,
-        first_code_index: usize,
-    ) -> usize {
-        if shred.is_data() {
-            shred.index() as usize
-        } else {
-            shred.index() as usize + num_data + first_data_index - first_code_index
-        }
-    }
-
     fn reassemble_payload(num_data: usize, data_shred_bufs: Vec<&Vec<u8>>) -> Vec<u8> {
         let valid_data_len = SHRED_PAYLOAD_SIZE - SIZE_OF_CODING_SHRED_HEADERS;
         data_shred_bufs[..num_data]
@@ -1453,7 +1439,6 @@ pub mod tests {
                 num_data_shreds,
                 num_coding_shreds,
                 0,
-                0,
                 slot
             ),
             Err(reed_solomon_erasure::Error::TooFewShardsPresent)
@@ -1465,7 +1450,6 @@ pub mod tests {
             num_data_shreds,
             num_coding_shreds,
             0,
-            0,
             slot,
         )
         .unwrap();
@@ -1483,7 +1467,6 @@ pub mod tests {
             num_data_shreds,
             num_coding_shreds,
             0,
-            0,
             slot,
         )
         .unwrap();
@@ -1531,7 +1514,6 @@ pub mod tests {
             num_data_shreds,
             num_coding_shreds,
             0,
-            0,
             slot,
         )
         .unwrap();
@@ -1604,7 +1586,6 @@ pub mod tests {
             num_data_shreds,
             num_coding_shreds,
             25,
-            25,
             slot,
         )
         .unwrap();
@@ -1636,7 +1617,6 @@ pub mod tests {
             num_data_shreds,
             num_coding_shreds,
             25,
-            25,
             slot + 1,
         )
         .unwrap();
@@ -1649,15 +1629,14 @@ pub mod tests {
                 num_data_shreds,
                 num_coding_shreds,
                 15,
-                15,
                 slot,
             ),
             Err(reed_solomon_erasure::Error::TooFewShardsPresent)
         );
 
         // Test8: Try recovery/reassembly with incorrect index. Hint: does not recover any shreds
         assert_matches!(
-            Shredder::try_recovery(shred_info, num_data_shreds, num_coding_shreds, 35, 35, slot,),
+            Shredder::try_recovery(shred_info, num_data_shreds, num_coding_shreds, 35, slot),
             Err(reed_solomon_erasure::Error::TooFewShardsPresent)
         );
     }
@@ -1721,7 +1700,6 @@ pub mod tests {
             num_data_shreds,
             num_coding_shreds,
             next_shred_index as usize, // first index
-            next_shred_index as usize, // first code index
             slot,
         )
         .unwrap();

diff --git a/ledger/tests/shred.rs b/ledger/tests/shred.rs
@@ -76,7 +76,6 @@ fn test_multi_fec_block_coding() {
             MAX_DATA_SHREDS_PER_FEC_BLOCK as usize,
             MAX_DATA_SHREDS_PER_FEC_BLOCK as usize,
             shred_start_index,
-            shred_start_index,
             slot,
         )
         .unwrap();
@@ -121,6 +120,7 @@ fn test_multi_fec_block_different_size_coding() {
     for (fec_data_shreds, fec_coding_shreds) in fec_data.values().zip(fec_coding.values()) {
         let first_data_index = fec_data_shreds.first().unwrap().index() as usize;
         let first_code_index = fec_coding_shreds.first().unwrap().index() as usize;
+        assert_eq!(first_data_index, first_code_index);
         let num_data = fec_data_shreds.len();
         let num_coding = fec_coding_shreds.len();
         let all_shreds: Vec<Shred> = fec_data_shreds
@@ -129,17 +129,9 @@ fn test_multi_fec_block_different_size_coding() {
             .chain(fec_coding_shreds.iter().step_by(2))
             .cloned()
             .collect();
-
-        let recovered_data = Shredder::try_recovery(
-            all_shreds,
-            num_data,
-            num_coding,
-            first_data_index,
-            first_code_index,
-            slot,
-        )
-        .unwrap();
-
+        let recovered_data =
+            Shredder::try_recovery(all_shreds, num_data, num_coding, first_data_index, slot)
+                .unwrap();
         // Necessary in order to ensure the last shred in the slot
         // is part of the recovered set, and that the below `index`
         // calcuation in the loop is correct