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

leader: do not perform blockstore check in next_leader_slot #2445

Conversation

AshwinSekar
Copy link

Problem

next_leader_slot will skip over any group of 4 leader slots if there is a shred present for the first leader slot. This can lead to accidentally skipping leader slots where only the first block has been produced in cases of reset to an earlier slot.

Summary of Changes

Instead remove the blockstore check entirely. In poh_recorder, if we've ticked past our last slot, recompute the next leader slot's tick heights to be used instead. This builds on #2416 which performs the blockstore check in poh_recorder

@AshwinSekar AshwinSekar force-pushed the next-leader-slot-no-blockstore branch 2 times, most recently from 64a36b2 to 35af3ef Compare August 6, 2024 19:15
@AshwinSekar AshwinSekar force-pushed the next-leader-slot-no-blockstore branch from 35af3ef to 14b6aab Compare August 6, 2024 21:41
@jstarry
Copy link

jstarry commented Aug 7, 2024

I don't think the problem you stated is true, is it? next_leader_slot uses a flat_map to iterate over each of their leader slots, not just the first slot of each leader slot span

@AshwinSekar
Copy link
Author

I don't think the problem you stated is true, is it? next_leader_slot uses a flat_map to iterate over each of their leader slots, not just the first slot of each leader slot span

You're correct, we evaluate each slot, not just the first one.
I suppose even in a case like this, If we've only produced the 2nd leader block:

1 - 2 (have shreds) - 3 - 4

And then reset to slot 0, next_leader_slot will return Some((1, 1)). However we will still produce slot 3 and 4 after we've ticked through 2 (even if we don't reset to 1), since reached_leader_slot does not care if you've ticked past last_leader_tick_height.

It seems then that this change is not needed.

Here's the code in question:

let mut schedule = (epoch..=max_epoch)
.map(|epoch| self.get_epoch_schedule_else_compute(epoch, bank))
.while_some()
.zip(epoch..)
.flat_map(|(leader_schedule, k)| {
let offset = if k == epoch { start_index as usize } else { 0 };
let num_slots = bank.get_slots_in_epoch(k) as usize;
let first_slot = bank.epoch_schedule().get_first_slot_in_epoch(k);
leader_schedule
.get_indices(pubkey, offset)
.take_while(move |i| *i < num_slots)
.map(move |i| i as Slot + first_slot)
})
.skip_while(|slot| {
// Skip slots we already have shreds for
blockstore
.map(|bs| bs.has_existing_shreds_for_slot(*slot))
.unwrap_or(false)
});
let first_slot = schedule.next()?;
let max_slot = first_slot.saturating_add(max_slot_range);
let last_slot = schedule
.take_while(|slot| *slot < max_slot)
.zip(first_slot + 1..)
.take_while(|(a, b)| a == b)
.map(|(s, _)| s)
.last()
.unwrap_or(first_slot);
Some((first_slot, last_slot))

@AshwinSekar AshwinSekar closed this Aug 7, 2024
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