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::get_sigs_for_addr2: ensure lowest_slot >= first_available_block #33556

Merged
merged 3 commits into from
Oct 6, 2023
Merged

Blockstore::get_sigs_for_addr2: ensure lowest_slot >= first_available_block #33556

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a0bbea7
Set empty lowest_slot to first_available_block and remove check in loop
Oct 6, 2023
162511a
Ensure get_transaction_status on_with_counter returns slots >= first_…
Oct 6, 2023
a85792b
Actually cleanup ledger
Oct 6, 2023
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
22 changes: 9 additions & 13 deletions ledger/src/blockstore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2325,14 +2325,15 @@ impl Blockstore {
confirmed_unrooted_slots: &HashSet<Slot>,
) -> Result<(Option<(Slot, TransactionStatusMeta)>, u64)> {
let mut counter = 0;
let (lock, lowest_available_slot) = self.ensure_lowest_cleanup_slot();
let (lock, _) = self.ensure_lowest_cleanup_slot();
let first_available_block = self.get_first_available_block()?;

for transaction_status_cf_primary_index in 0..=1 {
let index_iterator = self.transaction_status_cf.iter(IteratorMode::From(
(
transaction_status_cf_primary_index,
signature,
lowest_available_slot,
first_available_block,
),
IteratorDirection::Forward,
))?;
Expand Down Expand Up @@ -2630,16 +2631,17 @@ impl Blockstore {
};
get_before_slot_timer.stop();

let first_available_block = self.get_first_available_block()?;
// Generate a HashSet of signatures that should be excluded from the results based on
// `until` signature
let mut get_until_slot_timer = Measure::start("get_until_slot_timer");
let (lowest_slot, until_excluded_signatures) = match until {
None => (0, HashSet::new()),
None => (first_available_block, HashSet::new()),
Some(until) => {
let transaction_status =
self.get_transaction_status(until, &confirmed_unrooted_slots)?;
match transaction_status {
None => (0, HashSet::new()),
None => (first_available_block, HashSet::new()),
Some((slot, _)) => {
let mut slot_signatures = self.get_sorted_block_signatures(slot)?;
if let Some(pos) = slot_signatures.iter().position(|&x| x == until) {
Expand All @@ -2654,7 +2656,6 @@ impl Blockstore {
get_until_slot_timer.stop();

// Fetch the list of signatures that affect the given address
let first_available_block = self.get_first_available_block()?;
let mut address_signatures = vec![];

// Get signatures in `slot`
Expand Down Expand Up @@ -2695,10 +2696,7 @@ impl Blockstore {
if slot == next_max_slot || slot < lowest_slot {
break;
}
if i == starting_primary_index
&& key_address == address
&& slot >= first_available_block
{
if i == starting_primary_index && key_address == address {
if self.is_root(slot) || confirmed_unrooted_slots.contains(&slot) {
address_signatures.push((slot, signature));
}
Expand Down Expand Up @@ -2733,10 +2731,7 @@ impl Blockstore {
if slot < lowest_slot {
break;
}
if i == next_primary_index
&& key_address == address
&& slot >= first_available_block
{
if i == next_primary_index && key_address == address {
if self.is_root(slot) || confirmed_unrooted_slots.contains(&slot) {
address_signatures.push((slot, signature));
}
Expand Down Expand Up @@ -8017,6 +8012,7 @@ pub mod tests {

if simulate_ledger_cleanup_service {
*blockstore.lowest_cleanup_slot.write().unwrap() = lowest_cleanup_slot;
blockstore.purge_slots(0, lowest_cleanup_slot, PurgeType::CompactionFilter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Did unit test fail without this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because Blockstore::first_available_block() depends on the rooted-slot iterator, and this "simulation" wasn't adjusting the root list, just writing a new lowest_cleanup_slot. This change seemed defensible to me, since this is exactly what the LedgerCleanupService does, plus some extra data reporting.

}

let are_missing = check_for_missing();
Expand Down