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.

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

Remove explicit assert in Blockstore::get_slot_entries_in_block() #1258

Merged
merged 1 commit into from
May 9, 2024

Conversation

steviez
Copy link

@steviez steviez commented May 9, 2024

Problem

This is a private function and the callers should NOT be calling with an empty completed range vector. Regardless, it is safer to just return an empty Entry vector should an empty CompletedRanges be provided as input.

I was initially worried about something downstream not handling an empty Vec<Entry> gracefully. However, I found the concern to invalid for the two current callers of get_slot_entries_in_block():

agave/ledger/src/blockstore.rs

Lines 3597 to 3605 in 8a55087

pub fn get_entries_in_data_block(
&self,
slot: Slot,
start_index: u32,
end_index: u32,
slot_meta: Option<&SlotMeta>,
) -> Result<Vec<Entry>> {
self.get_slot_entries_in_block(slot, vec![(start_index, end_index)], slot_meta)
}

  • The sole consumer of Blockstore::get_entries_in_block() iterates over the the entries, so no concerns with empty Vec

agave/ledger/src/blockstore.rs

Lines 3371 to 3397 in 8a55087

pub fn get_slot_entries_with_shred_info(
&self,
slot: Slot,
start_index: u64,
allow_dead_slots: bool,
) -> Result<(Vec<Entry>, u64, bool)> {
let (completed_ranges, slot_meta) = self.get_completed_ranges(slot, start_index)?;
// Check if the slot is dead *after* fetching completed ranges to avoid a race
// where a slot is marked dead by another thread before the completed range query finishes.
// This should be sufficient because full slots will never be marked dead from another thread,
// this can only happen during entry processing during replay stage.
if self.is_dead(slot) && !allow_dead_slots {
return Err(BlockstoreError::DeadSlot);
} else if completed_ranges.is_empty() {
return Ok((vec![], 0, false));
}
let slot_meta = slot_meta.unwrap();
let num_shreds = completed_ranges
.last()
.map(|(_, end_index)| u64::from(*end_index) - start_index + 1)
.unwrap_or(0);
let entries = self.get_slot_entries_in_block(slot, completed_ranges, Some(&slot_meta))?;
Ok((entries, num_shreds, slot_meta.is_full()))
}

  • Blockstore::get_slot_entries_with_shred_info() checks if completed_ranges is empty before calling Blockstore::get_slot_entries_in_block()

This is (long overdue) followup to this comment: solana-labs#34768 (comment)

This is a private function and the callers should NOT be calling with an
empty completed range vector. Regardless, it is safer to just return an
empty Entry vector should an empty CompletedRanges be provided as input.
@steviez steviez merged commit c84c478 into anza-xyz:master May 9, 2024
38 checks passed
@steviez steviez deleted the bstore_avoid_assert branch May 9, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants