Skip to content

Commit

Permalink
Round up correctly when truncating max ancient storages (anza-xyz#1781)
Browse files Browse the repository at this point in the history
  • Loading branch information
brooksprumo authored and samkim-crypto committed Jul 31, 2024
1 parent 4d3208e commit 02add4e
Showing 1 changed file with 51 additions and 3 deletions.
54 changes: 51 additions & 3 deletions accounts-db/src/ancient_append_vecs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ impl AncientSlotInfos {
for (i, info) in self.all_infos.iter().enumerate() {
cumulative_bytes += info.alive_bytes;
let ancient_storages_required =
(cumulative_bytes.0 / tuning.ideal_storage_size + 1) as usize;
div_ceil(cumulative_bytes.0, tuning.ideal_storage_size) as usize;
let storages_remaining = total_storages - i - 1;

// if the remaining uncombined storages and the # of resulting
Expand Down Expand Up @@ -1109,6 +1109,25 @@ pub fn is_ancient(storage: &AccountsFile) -> bool {
storage.capacity() >= get_ancient_append_vec_capacity()
}

/// Divides `x` by `y` and rounds up
///
/// # Notes
///
/// It is undefined behavior if `x + y` overflows a u64.
/// Debug builds check this invariant, and will panic if broken.
fn div_ceil(x: u64, y: NonZeroU64) -> u64 {
let y = y.get();
debug_assert!(
x.checked_add(y).is_some(),
"x + y must not overflow! x: {x}, y: {y}",
);
// SAFETY: The caller guaranteed `x + y` does not overflow
// SAFETY: Since `y` is NonZero:
// - we know the denominator is > 0, and thus safe (cannot have divide-by-zero)
// - we know `x + y` is non-zero, and thus the numerator is safe (cannot underflow)
(x + y - 1) / y
}

#[cfg(test)]
pub mod tests {
use {
Expand All @@ -1128,7 +1147,10 @@ pub mod tests {
},
accounts_hash::AccountHash,
accounts_index::UpsertReclaim,
append_vec::{aligned_stored_size, AppendVec, AppendVecStoredAccountMeta},
append_vec::{
aligned_stored_size, AppendVec, AppendVecStoredAccountMeta,
MAXIMUM_APPEND_VEC_FILE_SIZE,
},
storable_accounts::{tests::build_accounts_from_storage, StorableAccountsBySlot},
},
rand::seq::SliceRandom as _,
Expand All @@ -1140,6 +1162,7 @@ pub mod tests {
std::{collections::HashSet, ops::Range},
strum::IntoEnumIterator,
strum_macros::EnumIter,
test_case::test_case,
};

fn get_sample_storages(
Expand Down Expand Up @@ -2875,7 +2898,7 @@ pub mod tests {
.collect();
assert_eq!(
infos.all_infos.len() as u64,
tuning.max_resulting_storages.get() - 1,
tuning.max_resulting_storages.get(),
);
assert!(high_slots
.iter()
Expand Down Expand Up @@ -3808,4 +3831,29 @@ pub mod tests {
assert!(expected_ref_counts.is_empty());
}
}

#[test_case(0, 1 => 0)]
#[test_case(1, 1 => 1)]
#[test_case(2, 1 => 2)]
#[test_case(2, 2 => 1)]
#[test_case(2, 3 => 1)]
#[test_case(2, 4 => 1)]
#[test_case(3, 4 => 1)]
#[test_case(4, 4 => 1)]
#[test_case(5, 4 => 2)]
#[test_case(0, u64::MAX => 0)]
#[test_case(MAXIMUM_APPEND_VEC_FILE_SIZE - 1, MAXIMUM_APPEND_VEC_FILE_SIZE => 1)]
#[test_case(MAXIMUM_APPEND_VEC_FILE_SIZE + 1, MAXIMUM_APPEND_VEC_FILE_SIZE => 2)]
fn test_div_ceil(x: u64, y: u64) -> u64 {
div_ceil(x, NonZeroU64::new(y).unwrap())
}

#[should_panic(expected = "x + y must not overflow")]
#[test_case(1, u64::MAX)]
#[test_case(u64::MAX, 1)]
#[test_case(u64::MAX/2 + 2, u64::MAX/2)]
#[test_case(u64::MAX/2, u64::MAX/2 + 2)]
fn test_div_ceil_overflow(x: u64, y: u64) {
div_ceil(x, NonZeroU64::new(y).unwrap());
}
}

0 comments on commit 02add4e

Please sign in to comment.