diff --git a/core/src/broadcast_stage/standard_broadcast_run.rs b/core/src/broadcast_stage/standard_broadcast_run.rs index 7a0dbdfa56..877a234bd8 100644 --- a/core/src/broadcast_stage/standard_broadcast_run.rs +++ b/core/src/broadcast_stage/standard_broadcast_run.rs @@ -596,7 +596,7 @@ mod test { .expect("Expected a shred that signals an interrupt"); // Validate the shred - assert_eq!(shred.parent(), Some(parent)); + assert_eq!(shred.parent().unwrap(), parent); assert_eq!(shred.slot(), slot); assert_eq!(shred.index(), next_shred_index); assert!(shred.is_data()); diff --git a/core/src/window_service.rs b/core/src/window_service.rs index 919faaab13..822dc4047c 100644 --- a/core/src/window_service.rs +++ b/core/src/window_service.rs @@ -164,8 +164,8 @@ fn verify_shred_slot(shred: &Shred, root: u64) -> bool { match shred.shred_type() { // Only data shreds have parent information ShredType::Data => match shred.parent() { - Some(parent) => blockstore::verify_shred_slots(shred.slot(), parent, root), - None => false, + Ok(parent) => blockstore::verify_shred_slots(shred.slot(), parent, root), + Err(_) => false, }, // Filter out outdated coding shreds ShredType::Code => shred.slot() >= root, diff --git a/ledger/src/blockstore.rs b/ledger/src/blockstore.rs index 8495024d2f..f2dedb9ece 100644 --- a/ledger/src/blockstore.rs +++ b/ledger/src/blockstore.rs @@ -13,10 +13,7 @@ use { erasure::ErasureConfig, leader_schedule_cache::LeaderScheduleCache, next_slots_iterator::NextSlotsIterator, - shred::{ - Result as ShredResult, Shred, ShredType, Shredder, MAX_DATA_SHREDS_PER_FEC_BLOCK, - SHRED_PAYLOAD_SIZE, - }, + shred::{Result as ShredResult, Shred, ShredType, Shredder, SHRED_PAYLOAD_SIZE}, }, bincode::deserialize, log::*, @@ -1187,7 +1184,9 @@ impl Blockstore { &self.db, slot_meta_working_set, slot, - shred.parent().ok_or(InsertDataShredError::InvalidShred)?, + shred + .parent() + .map_err(|_| InsertDataShredError::InvalidShred)?, ); let slot_meta = &mut slot_meta_entry.new_slot_meta.borrow_mut(); @@ -1244,16 +1243,7 @@ impl Blockstore { } fn should_insert_coding_shred(shred: &Shred, last_root: &RwLock) -> bool { - let shred_index = shred.index(); - let fec_set_index = shred.common_header.fec_set_index; - let num_coding_shreds = shred.coding_header.num_coding_shreds as u32; - shred.is_code() - && shred_index >= fec_set_index - && shred_index - fec_set_index < num_coding_shreds - && num_coding_shreds != 0 - && num_coding_shreds <= 8 * MAX_DATA_SHREDS_PER_FEC_BLOCK - && num_coding_shreds - 1 <= u32::MAX - fec_set_index - && shred.slot() > *last_root.read().unwrap() + shred.is_code() && shred.sanitize() && shred.slot() > *last_root.read().unwrap() } fn insert_coding_shred( @@ -1267,7 +1257,7 @@ impl Blockstore { // Assert guaranteed by integrity checks on the shred that happen before // `insert_coding_shred` is called - assert!(shred.is_code() && shred_index >= shred.common_header.fec_set_index as u64); + assert!(shred.is_code() && shred.sanitize()); // Commit step: commit all changes to the mutable structures at once, or none at all. // We don't want only a subset of these changes going through. @@ -5712,7 +5702,9 @@ pub mod tests { coding.clone(), ); coding_shred.common_header.fec_set_index = std::u32::MAX - 1; + coding_shred.coding_header.num_data_shreds = 2; coding_shred.coding_header.num_coding_shreds = 3; + coding_shred.coding_header.position = 1; coding_shred.common_header.index = std::u32::MAX - 1; assert!(!Blockstore::should_insert_coding_shred( &coding_shred, diff --git a/ledger/src/shred.rs b/ledger/src/shred.rs index 48ab6efec7..eaaf2020d7 100644 --- a/ledger/src/shred.rs +++ b/ledger/src/shred.rs @@ -161,6 +161,9 @@ pub enum ShredError { "invalid parent offset; parent_offset {parent_offset} must be larger than slot {slot}" )] InvalidParentOffset { slot: Slot, parent_offset: u16 }, + + #[error("invalid payload")] + InvalidPayload, } pub type Result = std::result::Result; @@ -340,41 +343,32 @@ impl Shred { let common_header: ShredCommonHeader = Self::deserialize_obj(&mut start, SIZE_OF_COMMON_SHRED_HEADER, &payload)?; - let slot = common_header.slot; // Shreds should be padded out to SHRED_PAYLOAD_SIZE // so that erasure generation/recovery works correctly // But only the data_header.size is stored in blockstore. payload.resize(SHRED_PAYLOAD_SIZE, 0); - let shred = match common_header.shred_type { + let (data_header, coding_header) = match common_header.shred_type { ShredType::Code => { let coding_header: CodingShredHeader = Self::deserialize_obj(&mut start, SIZE_OF_CODING_SHRED_HEADER, &payload)?; - Self { - common_header, - data_header: DataShredHeader::default(), - coding_header, - payload, - } + (DataShredHeader::default(), coding_header) } ShredType::Data => { let data_header: DataShredHeader = Self::deserialize_obj(&mut start, SIZE_OF_DATA_SHRED_HEADER, &payload)?; - if u64::from(data_header.parent_offset) > common_header.slot { - return Err(ShredError::InvalidParentOffset { - slot, - parent_offset: data_header.parent_offset, - }); - } - Self { - common_header, - data_header, - coding_header: CodingShredHeader::default(), - payload, - } + (data_header, CodingShredHeader::default()) } }; - - Ok(shred) + let shred = Self { + common_header, + data_header, + coding_header, + payload, + }; + shred + .sanitize() + .then(|| shred) + .ok_or(ShredError::InvalidPayload) } pub fn new_empty_coding( @@ -448,13 +442,24 @@ impl Shred { self.common_header.slot } - pub fn parent(&self) -> Option { + pub fn parent(&self) -> Result { match self.shred_type() { ShredType::Data => { - let parent_offset = Slot::try_from(self.data_header.parent_offset); - self.slot().checked_sub(parent_offset.ok()?) + let slot = self.slot(); + let parent_offset = Slot::from(self.data_header.parent_offset); + if parent_offset == 0 && slot != 0 { + return Err(ShredError::InvalidParentOffset { + slot, + parent_offset: 0, + }); + } + slot.checked_sub(parent_offset) + .ok_or(ShredError::InvalidParentOffset { + slot, + parent_offset: self.data_header.parent_offset, + }) } - ShredType::Code => None, + ShredType::Code => Err(ShredError::InvalidShredType), } } @@ -462,19 +467,48 @@ impl Shred { self.common_header.index } + pub(crate) fn fec_set_index(&self) -> u32 { + self.common_header.fec_set_index + } + + // Returns true if the shred passes sanity checks. + pub(crate) fn sanitize(&self) -> bool { + self.erasure_block_index().is_some() + && match self.shred_type() { + ShredType::Data => { + self.parent().is_ok() + && usize::from(self.data_header.size) <= self.payload.len() + } + ShredType::Code => { + u32::from(self.coding_header.num_coding_shreds) + <= 8 * MAX_DATA_SHREDS_PER_FEC_BLOCK + } + } + } + pub fn version(&self) -> u16 { self.common_header.version } // Returns the block index within the erasure coding set. fn erasure_block_index(&self) -> Option { - let fec_set_index = self.common_header.fec_set_index; - let index = self.index().checked_sub(fec_set_index)? as usize; + let index = self.index().checked_sub(self.fec_set_index())?; + let index = usize::try_from(index).ok()?; match self.shred_type() { ShredType::Data => Some(index), ShredType::Code => { - let num_data_shreds = self.coding_header.num_data_shreds as usize; - let num_coding_shreds = self.coding_header.num_coding_shreds as usize; + // TODO should use first_coding_index once position field is + // populated. + // Assert that the last shred index in the erasure set does not + // overshoot u32. + self.fec_set_index().checked_add(u32::from( + self.coding_header + .num_data_shreds + .max(self.coding_header.num_coding_shreds) + .checked_sub(1)?, + ))?; + let num_data_shreds = usize::from(self.coding_header.num_data_shreds); + let num_coding_shreds = usize::from(self.coding_header.num_coding_shreds); let fec_set_size = num_data_shreds.checked_add(num_coding_shreds)?; let index = index.checked_add(num_data_shreds)?; (index < fec_set_size).then(|| index) @@ -1108,7 +1142,7 @@ pub fn verify_test_data_shred( assert!(shred.is_data()); assert_eq!(shred.index(), index); assert_eq!(shred.slot(), slot); - assert_eq!(shred.parent(), Some(parent)); + assert_eq!(shred.parent().unwrap(), parent); assert_eq!(verify, shred.verify(pk)); if is_last_in_slot { assert!(shred.last_in_slot()); @@ -1845,12 +1879,13 @@ pub mod tests { shred.copy_to_packet(&mut packet); let shred_res = Shred::new_from_serialized_shred(packet.data.to_vec()); assert_matches!( - shred_res, + shred.parent(), Err(ShredError::InvalidParentOffset { slot: 10, parent_offset: 1000 }) ); + assert_matches!(shred_res, Err(ShredError::InvalidPayload)); } #[test]