From ecb491c3920e51a363c75ef8ce1b435f958b7693 Mon Sep 17 00:00:00 2001 From: Ashwin Sekar Date: Tue, 28 May 2024 11:24:07 -0400 Subject: [PATCH] blockstore: always send a coding shred for chained merkle root conflicts (#1353) * blockstore: always send a coding shred for chained merkle root conflicts * pr feedback: return ErasureMeta * fix format string indentation --- ledger/src/blockstore.rs | 109 +++++++++++++++++----------------- ledger/src/blockstore_meta.rs | 5 ++ 2 files changed, 59 insertions(+), 55 deletions(-) diff --git a/ledger/src/blockstore.rs b/ledger/src/blockstore.rs index 562d8f5cac4828..19173fe1feb9ff 100644 --- a/ledger/src/blockstore.rs +++ b/ledger/src/blockstore.rs @@ -498,11 +498,11 @@ impl Blockstore { /// /// Checks the map `erasure_metas`, if not present scans blockstore. Returns None /// if the previous consecutive erasure set is not present in either. - fn previous_erasure_set( - &self, + fn previous_erasure_set<'a>( + &'a self, erasure_set: ErasureSetId, - erasure_metas: &BTreeMap>, - ) -> Result> { + erasure_metas: &'a BTreeMap>, + ) -> Result)>> { let (slot, fec_set_index) = erasure_set.store_key(); // Check the previous entry from the in memory map to see if it is the consecutive @@ -513,13 +513,15 @@ impl Blockstore { Bound::Excluded(erasure_set), )) .next_back(); - let candidate_erasure_set = candidate_erasure_entry + let candidate_erasure_set_and_meta = candidate_erasure_entry .filter(|(_, candidate_erasure_meta)| { candidate_erasure_meta.as_ref().next_fec_set_index() == Some(fec_set_index) }) - .map(|(candidate_erasure_set, _)| *candidate_erasure_set); - if candidate_erasure_set.is_some() { - return Ok(candidate_erasure_set); + .map(|(erasure_set, erasure_meta)| { + (*erasure_set, Cow::Borrowed(erasure_meta.as_ref())) + }); + if candidate_erasure_set_and_meta.is_some() { + return Ok(candidate_erasure_set_and_meta); } // Consecutive set was not found in memory, scan blockstore for a potential candidate @@ -549,7 +551,10 @@ impl Blockstore { return Err(BlockstoreError::InvalidErasureConfig); }; if next_fec_set_index == fec_set_index { - return Ok(Some(candidate_erasure_set)); + return Ok(Some(( + candidate_erasure_set, + Cow::Owned(candidate_erasure_meta), + ))); } Ok(None) } @@ -1168,7 +1173,6 @@ impl Blockstore { shred, &just_inserted_shreds, &erasure_metas, - &merkle_root_metas, &mut duplicate_shreds, ); } @@ -1887,7 +1891,6 @@ impl Blockstore { shred: &Shred, just_inserted_shreds: &HashMap, erasure_metas: &BTreeMap>, - merkle_root_metas: &HashMap>, duplicate_shreds: &mut Vec, ) -> bool { let slot = shred.slot(); @@ -1905,7 +1908,7 @@ impl Blockstore { // If a shred from the previous fec set has already been inserted, check the chaining. // Since we cannot compute the previous fec set index, we check the in memory map, otherwise // check the previous key from blockstore to see if it is consecutive with our current set. - let Some(prev_erasure_set) = self + let Some((prev_erasure_set, prev_erasure_meta)) = self .previous_erasure_set(erasure_set, erasure_metas) .expect("Expect database operations to succeed") else { @@ -1915,37 +1918,22 @@ impl Blockstore { return true; }; - let Some(prev_merkle_root_meta) = merkle_root_metas - .get(&prev_erasure_set) - .map(WorkingEntry::as_ref) - .map(Cow::Borrowed) - .or_else(|| { - self.merkle_root_meta(prev_erasure_set) - .unwrap() - .map(Cow::Owned) - }) - else { - warn!( - "The merkle root meta for the previous erasure set {prev_erasure_set:?} does not \ - exist. This should only happen if you have recently upgraded from a version < \ - v1.18.13. Skipping the backwards chained merkle root for {erasure_set:?}" - ); - return true; - }; let prev_shred_id = ShredId::new( slot, - prev_merkle_root_meta.first_received_shred_index(), - prev_merkle_root_meta.first_received_shred_type(), + prev_erasure_meta + .first_received_coding_shred_index() + .expect("First received coding index must fit in u32"), + ShredType::Code, ); let Some(prev_shred) = Self::get_shred_from_just_inserted_or_db(self, just_inserted_shreds, prev_shred_id) .map(Cow::into_owned) else { - error!( - "Shred {prev_shred_id:?} indicated by merkle root meta {prev_merkle_root_meta:?} \ - is missing from blockstore. This should only happen in extreme cases where \ - blockstore cleanup has caught up to the root. Skipping the backwards chained \ - merkle root consistency check" + warn!( + "Shred {prev_shred_id:?} indicated by the erasure meta {prev_erasure_meta:?} \ + is missing from blockstore. This can happen if you have recently upgraded \ + from a version < v1.18.13, or if blockstore cleanup has caught up to the root. \ + Skipping the backwards chained merkle root consistency check" ); return true; }; @@ -1955,12 +1943,10 @@ impl Blockstore { if !self.check_chaining(merkle_root, chained_merkle_root) { warn!( "Received conflicting chained merkle roots for slot: {slot}, shred {:?} type {:?} \ - chains to merkle root {chained_merkle_root:?}, however previous fec set shred \ - {prev_erasure_set:?} type {:?} has merkle root {merkle_root:?}. Reporting as \ - duplicate", + chains to merkle root {chained_merkle_root:?}, however previous fec set coding \ + shred {prev_erasure_set:?} has merkle root {merkle_root:?}. Reporting as duplicate", shred.erasure_set(), shred.shred_type(), - prev_merkle_root_meta.first_received_shred_type(), ); if !self.has_duplicate_shreds_in_slot(shred.slot()) { @@ -11322,8 +11308,9 @@ pub mod tests { assert_eq!( blockstore .previous_erasure_set(erasure_set, &erasure_metas) - .unwrap(), - Some(erasure_set_prev) + .unwrap() + .map(|(erasure_set, erasure_meta)| (erasure_set, erasure_meta.into_owned())), + Some((erasure_set_prev, erasure_meta_prev)) ); // Erasure meta only contains the older, blockstore has the previous fec set @@ -11334,8 +11321,9 @@ pub mod tests { assert_eq!( blockstore .previous_erasure_set(erasure_set, &erasure_metas) - .unwrap(), - Some(erasure_set_prev) + .unwrap() + .map(|(erasure_set, erasure_meta)| (erasure_set, erasure_meta.into_owned())), + Some((erasure_set_prev, erasure_meta_prev)) ); // Both contain the previous fec set @@ -11343,23 +11331,26 @@ pub mod tests { assert_eq!( blockstore .previous_erasure_set(erasure_set, &erasure_metas) - .unwrap(), - Some(erasure_set_prev) + .unwrap() + .map(|(erasure_set, erasure_meta)| (erasure_set, erasure_meta.into_owned())), + Some((erasure_set_prev, erasure_meta_prev)) ); // Works even if the previous fec set has index 0 assert_eq!( blockstore .previous_erasure_set(erasure_set_prev, &erasure_metas) - .unwrap(), - Some(erasure_set_0) + .unwrap() + .map(|(erasure_set, erasure_meta)| (erasure_set, erasure_meta.into_owned())), + Some((erasure_set_0, erasure_meta_0)) ); erasure_metas.remove(&erasure_set_0); assert_eq!( blockstore .previous_erasure_set(erasure_set_prev, &erasure_metas) - .unwrap(), - Some(erasure_set_0) + .unwrap() + .map(|(erasure_set, erasure_meta)| (erasure_set, erasure_meta.into_owned())), + Some((erasure_set_0, erasure_meta_0)) ); // Does not cross slot boundary @@ -11764,7 +11755,7 @@ pub mod tests { #[test] fn test_chained_merkle_root_upgrade_inconsistency_backwards() { - // Insert a coding shred (without a merkle meta) then inconsistent shreds from the next FEC set + // Insert a coding shred (with an old erasure meta and no merkle root meta) then inconsistent shreds from the next FEC set let ledger_path = get_tmp_ledger_path_auto_delete!(); let blockstore = Blockstore::open(ledger_path.path()).unwrap(); @@ -11773,15 +11764,23 @@ pub mod tests { let fec_set_index = 0; let (data_shreds, coding_shreds, leader_schedule) = setup_erasure_shreds_with_index(slot, parent_slot, 10, fec_set_index); - let coding_shred_previous = coding_shreds[0].clone(); + let coding_shred_previous = coding_shreds[1].clone(); let next_fec_set_index = fec_set_index + data_shreds.len() as u32; assert!(blockstore .insert_shred_return_duplicate(coding_shred_previous.clone(), &leader_schedule,) .is_empty()); - // Remove the merkle root meta in order to simulate this blockstore originating from - // an older version. + // Set the first received coding shred index to 0 and remove merkle root meta to simulate this insertion coming from an + // older version. + let mut erasure_meta = blockstore + .erasure_meta(coding_shred_previous.erasure_set()) + .unwrap() + .unwrap(); + erasure_meta.clear_first_received_coding_shred_index(); + blockstore + .put_erasure_meta(coding_shred_previous.erasure_set(), &erasure_meta) + .unwrap(); let mut write_batch = blockstore.db.batch().unwrap(); blockstore .db @@ -11794,7 +11793,7 @@ pub mod tests { .is_none()); // Add an incorrectly chained merkle from the next set. Although incorrectly chained - // we skip the duplicate check as the merkle root meta is missing. + // we skip the duplicate check as the first received coding shred index shred is missing let merkle_root = Hash::new_unique(); assert!(merkle_root != coding_shred_previous.merkle_root().unwrap()); let (data_shreds, coding_shreds, leader_schedule) = diff --git a/ledger/src/blockstore_meta.rs b/ledger/src/blockstore_meta.rs index 788bfcbb29e6e8..002d4970dd5403 100644 --- a/ledger/src/blockstore_meta.rs +++ b/ledger/src/blockstore_meta.rs @@ -423,6 +423,11 @@ impl ErasureMeta { StillNeed(num_needed) } } + + #[cfg(test)] + pub(crate) fn clear_first_received_coding_shred_index(&mut self) { + self.first_received_coding_index = 0; + } } impl MerkleRootMeta {