Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

blockstore: always send a coding shred for chained merkle root conflicts #1353

Merged
merged 3 commits into from
May 28, 2024
Merged

blockstore: always send a coding shred for chained merkle root conflicts #1353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f84437a
blockstore: always send a coding shred for chained merkle root conflicts
AshwinSekar May 14, 2024
73dfefc
pr feedback: return ErasureMeta
AshwinSekar May 17, 2024
1f28b82
fix format string indentation
AshwinSekar May 22, 2024
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
109 changes: 54 additions & 55 deletions ledger/src/blockstore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ErasureSetId, WorkingEntry<ErasureMeta>>,
) -> Result<Option<ErasureSetId>> {
erasure_metas: &'a BTreeMap<ErasureSetId, WorkingEntry<ErasureMeta>>,
) -> Result<Option<(ErasureSetId, Cow<ErasureMeta>)>> {
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
Expand All @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -1168,7 +1173,6 @@ impl Blockstore {
shred,
&just_inserted_shreds,
&erasure_metas,
&merkle_root_metas,
&mut duplicate_shreds,
);
}
Expand Down Expand Up @@ -1887,7 +1891,6 @@ impl Blockstore {
shred: &Shred,
just_inserted_shreds: &HashMap<ShredId, Shred>,
erasure_metas: &BTreeMap<ErasureSetId, WorkingEntry<ErasureMeta>>,
merkle_root_metas: &HashMap<ErasureSetId, WorkingEntry<MerkleRootMeta>>,
duplicate_shreds: &mut Vec<PossibleDuplicateShred>,
) -> bool {
let slot = shred.slot();
Expand All @@ -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 {
Expand All @@ -1915,37 +1918,22 @@ impl Blockstore {
return true;
};

let Some(prev_merkle_root_meta) = merkle_root_metas
Copy link
Author

Choose a reason for hiding this comment

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

This is safe because self.previous_erasure_set() on line 1896 must have found an ErasureMeta for prev_erasure_set in order to reach here.

Copy link

Choose a reason for hiding this comment

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

Shouldn't fn previous_erasure_set actually return that ErasureMeta instead of looking it up again here?

Copy link
Author

Choose a reason for hiding this comment

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

yeah good point, I didn't do this last time because we only needed the merkle root.
Updated it to return the meta as well.

.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!(
Copy link
Author

Choose a reason for hiding this comment

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

We enter this if either:

  • Blockstore cleanup has caught up to root and prev_shred has been cleaned up
  • prev_erasure_meta was inserted with an older version and prev_erasure_meta.first_received_coding_shred_index() points to shred 0 which we do not have.

"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;
};
Expand All @@ -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()) {
Expand Down Expand Up @@ -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
Expand All @@ -11334,32 +11321,36 @@ 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
erasure_metas.insert(erasure_set_prev, WorkingEntry::Clean(erasure_meta_prev));
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
Expand Down Expand Up @@ -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();

Expand All @@ -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
Expand All @@ -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) =
Expand Down
5 changes: 5 additions & 0 deletions ledger/src/blockstore_meta.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading