Skip to content

Removes test_hash_calculation from AccountsBackgroundService handle_snapshot_requests() #6988

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

Merged
brooksprumo merged 1 commit into from
Jul 16, 2025
Merged

Removes test_hash_calculation from AccountsBackgroundService handle_snapshot_requests() #6988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 0 additions & 4 deletions core/src/accounts_hash_verifier.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,10 +379,6 @@ impl AccountsHashVerifier {
);
}

if let Some(expected_hash) = accounts_package.accounts_hash_for_testing {
assert_eq!(expected_hash, accounts_hash);
};

datapoint_info!(
"accounts_hash_verifier",
("calculate_hash", measure_hash_us, i64),
Expand Down
4 changes: 2 additions & 2 deletions core/tests/snapshots.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ where
.unwrap()
.set_root(bank.slot(), Some(&snapshot_controller), None)
.unwrap();
snapshot_request_handler.handle_snapshot_requests(false, 0, &AtomicBool::new(false));
snapshot_request_handler.handle_snapshot_requests(0, &AtomicBool::new(false));
}
}

Expand Down Expand Up @@ -450,7 +450,7 @@ fn test_bank_forks_incremental_snapshot() {
.unwrap()
.set_root(bank.slot(), Some(&snapshot_controller), None)
.unwrap();
snapshot_request_handler.handle_snapshot_requests(false, 0, &AtomicBool::new(false));
snapshot_request_handler.handle_snapshot_requests(0, &AtomicBool::new(false));
}

// Since AccountsBackgroundService isn't running, manually make a full snapshot archive
Expand Down
49 changes: 3 additions & 46 deletions runtime/src/accounts_background_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@ use {
crossbeam_channel::{Receiver, SendError, Sender},
log::*,
rayon::iter::{IntoParallelIterator, ParallelIterator},
solana_accounts_db::{
accounts_db::CalcAccountsHashDataSource, accounts_hash::CalcAccountsHashConfig,
},
solana_clock::{BankId, Slot},
solana_measure::{measure::Measure, measure_us},
stats::StatsManager,
Expand Down Expand Up @@ -147,7 +144,6 @@ impl SnapshotRequestHandler {
#[allow(clippy::type_complexity)]
pub fn handle_snapshot_requests(
&self,
test_hash_calculation: bool,
non_snapshot_time_us: u128,
exit: &AtomicBool,
) -> Option<Result<Slot, SnapshotError>> {
Expand All @@ -167,7 +163,6 @@ impl SnapshotRequestHandler {

let accounts_package_kind = new_accounts_package_kind(&snapshot_request)?;
Some(self.handle_snapshot_request(
test_hash_calculation,
non_snapshot_time_us,
snapshot_request,
accounts_package_kind,
Expand Down Expand Up @@ -270,7 +265,6 @@ impl SnapshotRequestHandler {

fn handle_snapshot_request(
&self,
test_hash_calculation: bool,
non_snapshot_time_us: u128,
snapshot_request: SnapshotRequest,
accounts_package_kind: AccountsPackageKind,
Expand Down Expand Up @@ -299,14 +293,6 @@ impl SnapshotRequestHandler {
.set_latest_full_snapshot_slot(snapshot_root_bank.slot());
}

let previous_accounts_hash = test_hash_calculation.then(|| {
// We have to use the index version here.
// We cannot calculate the non-index way because cache
// has not been flushed and stores don't match reality.
snapshot_root_bank
.update_accounts_hash(CalcAccountsHashDataSource::IndexForTests, false)
});

let mut flush_accounts_cache_time = Measure::start("flush_accounts_cache_time");
// Forced cache flushing MUST flush all roots <= snapshot_root_bank.slot().
// That's because `snapshot_root_bank.slot()` must be root at this point,
Expand All @@ -327,26 +313,6 @@ impl SnapshotRequestHandler {
);
flush_accounts_cache_time.stop();

let accounts_hash_for_testing = previous_accounts_hash.map(|previous_accounts_hash| {
let (this_accounts_hash, capitalization) = snapshot_root_bank
.accounts()
.accounts_db
.calculate_accounts_hash_from(
CalcAccountsHashDataSource::Storages,
snapshot_root_bank.slot(),
&CalcAccountsHashConfig {
use_bg_thread_pool: true,
ancestors: None,
epoch_schedule: snapshot_root_bank.epoch_schedule(),
epoch: snapshot_root_bank.epoch(),
store_detailed_debug_info_on_failure: false,
},
);
assert_eq!(previous_accounts_hash, this_accounts_hash);
assert_eq!(capitalization, snapshot_root_bank.capitalization());
this_accounts_hash
});

let mut clean_time = Measure::start("clean_time");
snapshot_root_bank.clean_accounts();
clean_time.stop();
Expand All @@ -368,7 +334,6 @@ impl SnapshotRequestHandler {
&snapshot_root_bank,
snapshot_storages,
status_cache_slot_deltas,
accounts_hash_for_testing,
Copy link
Copy Markdown
Author

@brooksprumo brooksprumo Jul 16, 2025

Choose a reason for hiding this comment

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

accounts_hash_for_testing is always None because up on line 330 previous_accounts_hash is also always None, because up on line 203 test_hash_calculation is always false. So we remove accounts_hash_for_testing too.

),
AccountsPackageKind::EpochAccountsHash => panic!(
"Illegal account package type: EpochAccountsHash packages must \
Expand Down Expand Up @@ -509,15 +474,11 @@ impl AbsRequestHandlers {
#[allow(clippy::type_complexity)]
pub fn handle_snapshot_requests(
&self,
test_hash_calculation: bool,
non_snapshot_time_us: u128,
exit: &AtomicBool,
) -> Option<Result<Slot, SnapshotError>> {
self.snapshot_request_handler.handle_snapshot_requests(
test_hash_calculation,
non_snapshot_time_us,
exit,
)
self.snapshot_request_handler
.handle_snapshot_requests(non_snapshot_time_us, exit)
}
}

Expand Down Expand Up @@ -601,11 +562,7 @@ impl AccountsBackgroundService {
let snapshot_handle_result = bank
.has_initial_accounts_hash_verification_completed()
.then(|| {
request_handlers.handle_snapshot_requests(
false, // test_hash_calculation. Will be removed soon.
non_snapshot_time,
&exit,
)
Comment on lines -604 to -608
Copy link
Copy Markdown
Author

@brooksprumo brooksprumo Jul 16, 2025

Choose a reason for hiding this comment

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

Here's the start/main change: test_hash_calculation is always false, so we can remove the param.

request_handlers.handle_snapshot_requests(non_snapshot_time, &exit)
})
.flatten();

Expand Down
2 changes: 0 additions & 2 deletions runtime/src/snapshot_bank_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,6 @@ fn bank_to_full_snapshot_archive_with(
bank,
snapshot_storages,
status_cache_slot_deltas,
None,
);
let snapshot_package =
SnapshotPackage::new(accounts_package, merkle_or_lattice_accounts_hash, None);
Expand Down Expand Up @@ -956,7 +955,6 @@ pub fn bank_to_incremental_snapshot_archive(
bank,
snapshot_storages,
status_cache_slot_deltas,
None,
);
let snapshot_package = SnapshotPackage::new(
accounts_package,
Expand Down
6 changes: 0 additions & 6 deletions runtime/src/snapshot_package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ pub struct AccountsPackage {
pub block_height: Slot,
pub snapshot_storages: Vec<Arc<AccountStorageEntry>>,
pub expected_capitalization: u64,
pub accounts_hash_for_testing: Option<AccountsHash>,
pub accounts: Arc<Accounts>,
pub epoch_schedule: EpochSchedule,
pub rent_collector: RentCollector,
Expand All @@ -54,7 +53,6 @@ impl AccountsPackage {
bank: &Bank,
snapshot_storages: Vec<Arc<AccountStorageEntry>>,
status_cache_slot_deltas: Vec<BankSlotDelta>,
accounts_hash_for_testing: Option<AccountsHash>,
) -> Self {
let slot = bank.slot();
if let AccountsPackageKind::Snapshot(snapshot_kind) = package_kind {
Expand Down Expand Up @@ -107,7 +105,6 @@ impl AccountsPackage {
package_kind,
bank,
snapshot_storages,
accounts_hash_for_testing,
accounts_hash_algorithm,
Some(snapshot_info),
)
Expand All @@ -117,7 +114,6 @@ impl AccountsPackage {
package_kind: AccountsPackageKind,
bank: &Bank,
snapshot_storages: Vec<Arc<AccountStorageEntry>>,
accounts_hash_for_testing: Option<AccountsHash>,
accounts_hash_algorithm: AccountsHashAlgorithm,
snapshot_info: Option<SupplementalSnapshotInfo>,
) -> Self {
Expand All @@ -127,7 +123,6 @@ impl AccountsPackage {
block_height: bank.block_height(),
snapshot_storages,
expected_capitalization: bank.capitalization(),
accounts_hash_for_testing,
accounts: bank.accounts(),
epoch_schedule: bank.epoch_schedule().clone(),
rent_collector: bank.rent_collector().clone(),
Expand All @@ -150,7 +145,6 @@ impl AccountsPackage {
block_height: Slot::default(),
snapshot_storages: Vec::default(),
expected_capitalization: u64::default(),
accounts_hash_for_testing: Option::default(),
accounts: Arc::new(accounts),
epoch_schedule: EpochSchedule::default(),
rent_collector: RentCollector::default(),
Expand Down
Loading