Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

removes first_coding_index from erasure recovery code #16646

Merged
behzadnouri merged 1 commit into from
Apr 23, 2021
Merged

removes first_coding_index from erasure recovery code #16646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions core/benches/shredder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
})
Expand Down
4 changes: 1 addition & 3 deletions core/src/cluster_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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));
Expand Down
14 changes: 5 additions & 9 deletions ledger/src/blockstore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
Expand Down
19 changes: 9 additions & 10 deletions ledger/src/blockstore_meta.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Copy Markdown
Contributor

@carllin carllin Apr 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm since these are stored/fetched from Rocks, is this going to cause compatibility issues if we try to fetch old ErasureMeta's?

Copy link
Copy Markdown
Contributor Author

@behzadnouri behzadnouri Apr 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right. fixed

/// 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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
28 changes: 3 additions & 25 deletions ledger/src/shred.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand All @@ -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,
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -1453,7 +1439,6 @@ pub mod tests {
num_data_shreds,
num_coding_shreds,
0,
0,
slot
),
Err(reed_solomon_erasure::Error::TooFewShardsPresent)
Expand All @@ -1465,7 +1450,6 @@ pub mod tests {
num_data_shreds,
num_coding_shreds,
0,
0,
slot,
)
.unwrap();
Expand All @@ -1483,7 +1467,6 @@ pub mod tests {
num_data_shreds,
num_coding_shreds,
0,
0,
slot,
)
.unwrap();
Expand Down Expand Up @@ -1531,7 +1514,6 @@ pub mod tests {
num_data_shreds,
num_coding_shreds,
0,
0,
slot,
)
.unwrap();
Expand Down Expand Up @@ -1604,7 +1586,6 @@ pub mod tests {
num_data_shreds,
num_coding_shreds,
25,
25,
slot,
)
.unwrap();
Expand Down Expand Up @@ -1636,7 +1617,6 @@ pub mod tests {
num_data_shreds,
num_coding_shreds,
25,
25,
slot + 1,
)
.unwrap();
Expand All @@ -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)
);
}
Expand Down Expand Up @@ -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();
Expand Down
16 changes: 4 additions & 12 deletions ledger/tests/shred.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down