From 5fb0ab9d000944aee045fd5ea619e826f4b80a6f Mon Sep 17 00:00:00 2001 From: behzad nouri Date: Wed, 10 Nov 2021 21:19:03 +0000 Subject: [PATCH] removes redundant args from Shredder::try_recovery (#21226) Shredder::try_recovery is already taking a Vec as an argument. All the other arguments are embedded in the shreds, and are so redundant. --- core/benches/shredder.rs | 9 +-- ledger/src/blockstore.rs | 8 +-- ledger/src/shred.rs | 133 +++++++++++++-------------------------- ledger/tests/shred.rs | 15 +---- 4 files changed, 47 insertions(+), 118 deletions(-) diff --git a/core/benches/shredder.rs b/core/benches/shredder.rs index b7b41562b2..97f503c2df 100644 --- a/core/benches/shredder.rs +++ b/core/benches/shredder.rs @@ -148,14 +148,7 @@ fn bench_shredder_decoding(bencher: &mut Bencher) { true, // is_last_in_slot ); bencher.iter(|| { - Shredder::try_recovery( - coding_shreds[..].to_vec(), - symbol_count, - symbol_count, - 0, // first index - 1, // slot - ) - .unwrap(); + Shredder::try_recovery(coding_shreds[..].to_vec()).unwrap(); }) } diff --git a/ledger/src/blockstore.rs b/ledger/src/blockstore.rs index 0481323f9f..69ea35527c 100644 --- a/ledger/src/blockstore.rs +++ b/ledger/src/blockstore.rs @@ -725,13 +725,7 @@ impl Blockstore { code_cf, ); - if let Ok(mut result) = Shredder::try_recovery( - available_shreds, - erasure_meta.config.num_data(), - erasure_meta.config.num_coding(), - set_index as usize, - slot, - ) { + if let Ok(mut result) = Shredder::try_recovery(available_shreds) { Self::submit_metrics( slot, set_index, diff --git a/ledger/src/shred.rs b/ledger/src/shred.rs index 9d28439a66..fbbd340273 100644 --- a/ledger/src/shred.rs +++ b/ledger/src/shred.rs @@ -813,30 +813,51 @@ impl Shredder { pub fn try_recovery( shreds: Vec, - num_data: usize, - num_coding: usize, - first_index: usize, - slot: Slot, ) -> std::result::Result, reed_solomon_erasure::Error> { use reed_solomon_erasure::Error::InvalidIndex; Self::verify_consistent_shred_payload_sizes("try_recovery()", &shreds)?; - let fec_set_size = num_data + num_coding; - if num_coding == 0 || shreds.len() >= fec_set_size { + let (slot, fec_set_index) = match shreds.first() { + None => return Ok(Vec::default()), + Some(shred) => (shred.slot(), shred.common_header.fec_set_index), + }; + let (num_data_shreds, num_coding_shreds) = match shreds.iter().find(|shred| shred.is_code()) + { + None => return Ok(Vec::default()), + Some(shred) => ( + shred.coding_header.num_data_shreds, + shred.coding_header.num_coding_shreds, + ), + }; + debug_assert!(shreds.iter().all( + |shred| shred.slot() == slot && shred.common_header.fec_set_index == fec_set_index + )); + debug_assert!(shreds + .iter() + .filter(|shred| shred.is_code()) + .all( + |shred| shred.coding_header.num_data_shreds == num_data_shreds + && shred.coding_header.num_coding_shreds == num_coding_shreds + )); + let num_data_shreds = num_data_shreds as usize; + let num_coding_shreds = num_coding_shreds as usize; + let fec_set_index = fec_set_index as usize; + let fec_set_size = num_data_shreds + num_coding_shreds; + if num_coding_shreds == 0 || shreds.len() >= fec_set_size { return Ok(Vec::default()); } // Mask to exclude data shreds already received from the return value. - let mut mask = vec![false; num_data]; + let mut mask = vec![false; num_data_shreds]; let mut blocks = vec![None; fec_set_size]; for shred in shreds { - if (shred.index() as usize) < first_index { + if (shred.index() as usize) < fec_set_index { return Err(InvalidIndex); } let shred_is_data = shred.is_data(); - let offset = if shred_is_data { 0 } else { num_data }; - let index = offset + shred.index() as usize - first_index; + let offset = if shred_is_data { 0 } else { num_data_shreds }; + let index = offset + shred.index() as usize - fec_set_index; let mut block = shred.payload; if shred_is_data { - if index >= num_data { + if index >= num_data_shreds { return Err(InvalidIndex); } mask[index] = true; @@ -854,8 +875,8 @@ impl Shredder { }; blocks[index] = Some(block); } - Session::new(num_data, num_coding)?.decode_blocks(&mut blocks)?; - let data_shred_indices = first_index..first_index + num_data; + Session::new(num_data_shreds, num_coding_shreds)?.decode_blocks(&mut blocks)?; + let data_shred_indices = fec_set_index..fec_set_index + num_data_shreds; let recovered_data = mask .into_iter() .zip(blocks) @@ -1367,26 +1388,13 @@ pub mod tests { .collect::>(); // Test0: Try recovery/reassembly with only data shreds, but not all data shreds. Hint: should fail - assert_matches!( - Shredder::try_recovery( - data_shreds[..data_shreds.len() - 1].to_vec(), - num_data_shreds, - num_coding_shreds, - 0, - slot - ), - Err(reed_solomon_erasure::Error::TooFewShardsPresent) + assert_eq!( + Shredder::try_recovery(data_shreds[..data_shreds.len() - 1].to_vec(),), + Ok(Vec::default()) ); // Test1: Try recovery/reassembly with only data shreds. Hint: should work - let recovered_data = Shredder::try_recovery( - data_shreds[..].to_vec(), - num_data_shreds, - num_coding_shreds, - 0, - slot, - ) - .unwrap(); + let recovered_data = Shredder::try_recovery(data_shreds[..].to_vec()).unwrap(); assert!(recovered_data.is_empty()); // Test2: Try recovery/reassembly with missing data shreds + coding shreds. Hint: should work @@ -1396,14 +1404,7 @@ pub mod tests { .filter_map(|(i, b)| if i % 2 == 0 { Some(b.clone()) } else { None }) .collect(); - let mut recovered_data = Shredder::try_recovery( - shred_info.clone(), - num_data_shreds, - num_coding_shreds, - 0, - slot, - ) - .unwrap(); + let mut recovered_data = Shredder::try_recovery(shred_info.clone()).unwrap(); assert_eq!(recovered_data.len(), 2); // Data shreds 1 and 3 were missing let recovered_shred = recovered_data.remove(0); @@ -1443,14 +1444,7 @@ pub mod tests { .filter_map(|(i, b)| if i % 2 != 0 { Some(b.clone()) } else { None }) .collect(); - let recovered_data = Shredder::try_recovery( - shred_info.clone(), - num_data_shreds, - num_coding_shreds, - 0, - slot, - ) - .unwrap(); + let recovered_data = Shredder::try_recovery(shred_info.clone()).unwrap(); assert_eq!(recovered_data.len(), 3); // Data shreds 0, 2, 4 were missing for (i, recovered_shred) in recovered_data.into_iter().enumerate() { @@ -1500,7 +1494,6 @@ pub mod tests { let serialized_entries = bincode::serialize(&entries).unwrap(); let (data_shreds, coding_shreds, _) = shredder.entries_to_shreds(&keypair, &entries, true, 25); - let num_coding_shreds = coding_shreds.len(); // We should have 10 shreds now assert_eq!(data_shreds.len(), num_data_shreds); @@ -1516,14 +1509,7 @@ pub mod tests { .filter_map(|(i, b)| if i % 2 != 0 { Some(b.clone()) } else { None }) .collect(); - let recovered_data = Shredder::try_recovery( - shred_info.clone(), - num_data_shreds, - num_coding_shreds, - 25, - slot, - ) - .unwrap(); + let recovered_data = Shredder::try_recovery(shred_info.clone()).unwrap(); assert_eq!(recovered_data.len(), 3); // Data shreds 25, 27, 29 were missing for (i, recovered_shred) in recovered_data.into_iter().enumerate() { @@ -1547,33 +1533,8 @@ pub mod tests { assert_eq!(serialized_entries[..], result[..serialized_entries.len()]); // Test6: Try recovery/reassembly with incorrect slot. Hint: does not recover any shreds - let recovered_data = Shredder::try_recovery( - shred_info.clone(), - num_data_shreds, - num_coding_shreds, - 25, - slot + 1, - ) - .unwrap(); + let recovered_data = Shredder::try_recovery(shred_info.clone()).unwrap(); assert!(recovered_data.is_empty()); - - // Test7: Try recovery/reassembly with incorrect index. Hint: does not recover any shreds - assert_matches!( - Shredder::try_recovery( - shred_info.clone(), - num_data_shreds, - num_coding_shreds, - 15, - slot, - ), - Err(reed_solomon_erasure::Error::InvalidIndex) - ); - - // 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, slot), - Err(reed_solomon_erasure::Error::InvalidIndex) - ); } #[test] @@ -1619,7 +1580,6 @@ pub mod tests { let (data_shreds, coding_shreds, _) = shredder.entries_to_shreds(&keypair, &[entry], is_last_in_slot, next_shred_index); let num_data_shreds = data_shreds.len(); - let num_coding_shreds = coding_shreds.len(); let mut shreds = coding_shreds; shreds.extend(data_shreds.iter().cloned()); shreds.shuffle(&mut rng); @@ -1636,14 +1596,7 @@ pub mod tests { .filter(|shred| shred.is_data()) .map(|shred| shred.index()) .collect(); - let recovered_shreds = Shredder::try_recovery( - shreds, - num_data_shreds, - num_coding_shreds, - next_shred_index as usize, // first index - slot, - ) - .unwrap(); + let recovered_shreds = Shredder::try_recovery(shreds).unwrap(); assert_eq!( recovered_shreds, data_shreds diff --git a/ledger/tests/shred.rs b/ledger/tests/shred.rs index 9a47e64391..1d117f275b 100644 --- a/ledger/tests/shred.rs +++ b/ledger/tests/shred.rs @@ -72,14 +72,7 @@ fn test_multi_fec_block_coding() { .filter_map(|(i, b)| if i % 2 != 0 { Some(b.clone()) } else { None }) .collect(); - let recovered_data = Shredder::try_recovery( - shred_info.clone(), - MAX_DATA_SHREDS_PER_FEC_BLOCK as usize, - MAX_DATA_SHREDS_PER_FEC_BLOCK as usize, - shred_start_index, - slot, - ) - .unwrap(); + let recovered_data = Shredder::try_recovery(shred_info.clone()).unwrap(); for (i, recovered_shred) in recovered_data.into_iter().enumerate() { let index = shred_start_index + (i * 2); @@ -122,17 +115,13 @@ fn test_multi_fec_block_different_size_coding() { 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 = fec_data_shreds .iter() .step_by(2) .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, slot) - .unwrap(); + let recovered_data = Shredder::try_recovery(all_shreds).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