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

Adds experimental support for accounts lt hash #3060

Merged
merged 2 commits into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

102 changes: 100 additions & 2 deletions accounts-db/src/accounts_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ use {
},
accounts_hash::{
AccountHash, AccountLtHash, AccountsDeltaHash, AccountsHash, AccountsHashKind,
AccountsHasher, CalcAccountsHashConfig, CalculateHashIntermediate, HashStats,
IncrementalAccountsHash, SerdeAccountsDeltaHash, SerdeAccountsHash,
AccountsHasher, AccountsLtHash, CalcAccountsHashConfig, CalculateHashIntermediate,
HashStats, IncrementalAccountsHash, SerdeAccountsDeltaHash, SerdeAccountsHash,
SerdeIncrementalAccountsHash, ZeroLamportAccounts, ZERO_LAMPORT_ACCOUNT_HASH,
ZERO_LAMPORT_ACCOUNT_LT_HASH,
},
Expand Down Expand Up @@ -501,6 +501,7 @@ pub const ACCOUNTS_DB_CONFIG_FOR_TESTING: AccountsDbConfig = AccountsDbConfig {
test_skip_rewrites_but_include_in_bank_hash: false,
storage_access: StorageAccess::Mmap,
scan_filter_for_shrinking: ScanFilter::OnlyAbnormalWithVerify,
enable_experimental_accumulator_hash: false,
};
pub const ACCOUNTS_DB_CONFIG_FOR_BENCHMARKS: AccountsDbConfig = AccountsDbConfig {
index: Some(ACCOUNTS_INDEX_CONFIG_FOR_BENCHMARKS),
Expand All @@ -517,6 +518,7 @@ pub const ACCOUNTS_DB_CONFIG_FOR_BENCHMARKS: AccountsDbConfig = AccountsDbConfig
test_skip_rewrites_but_include_in_bank_hash: false,
storage_access: StorageAccess::Mmap,
scan_filter_for_shrinking: ScanFilter::OnlyAbnormalWithVerify,
enable_experimental_accumulator_hash: false,
};

pub type BinnedHashData = Vec<Vec<CalculateHashIntermediate>>;
Expand Down Expand Up @@ -612,6 +614,7 @@ pub struct AccountsDbConfig {
pub test_partitioned_epoch_rewards: TestPartitionedEpochRewards,
pub storage_access: StorageAccess,
pub scan_filter_for_shrinking: ScanFilter,
pub enable_experimental_accumulator_hash: bool,
}

#[cfg(not(test))]
Expand Down Expand Up @@ -1508,6 +1511,10 @@ pub struct AccountsDb {

/// The latest full snapshot slot dictates how to handle zero lamport accounts
latest_full_snapshot_slot: SeqLock<Option<Slot>>,

/// Flag to indicate if the experimental accounts lattice hash is enabled.
/// (For R&D only; a feature-gate also exists to turn this on and make it a part of consensus.)
pub is_experimental_accumulator_hash_enabled: AtomicBool,
}

#[derive(Debug, Default)]
Expand Down Expand Up @@ -2477,6 +2484,8 @@ impl AccountsDb {
// rayon needs a lot of stack
const ACCOUNTS_STACK_SIZE: usize = 8 * 1024 * 1024;

let default_accounts_db_config = AccountsDbConfig::default();

AccountsDb {
create_ancient_storage: CreateAncientStorage::default(),
verify_accounts_hash_in_bg: VerifyAccountsHashInBackground::default(),
Expand Down Expand Up @@ -2541,6 +2550,9 @@ impl AccountsDb {
epoch_accounts_hash_manager: EpochAccountsHashManager::new_invalid(),
test_skip_rewrites_but_include_in_bank_hash: false,
latest_full_snapshot_slot: SeqLock::new(None),
is_experimental_accumulator_hash_enabled: default_accounts_db_config
.enable_experimental_accumulator_hash
.into(),
}
}

Expand Down Expand Up @@ -2587,6 +2599,8 @@ impl AccountsDb {
accounts_update_notifier: Option<AccountsUpdateNotifier>,
exit: Arc<AtomicBool>,
) -> Self {
let default_accounts_db_config = AccountsDbConfig::default();

let accounts_index = AccountsIndex::new(
accounts_db_config.as_mut().and_then(|x| x.index.take()),
exit,
Expand Down Expand Up @@ -2648,6 +2662,12 @@ impl AccountsDb {
.map(|config| config.scan_filter_for_shrinking)
.unwrap_or_default();

let enable_experimental_accumulator_hash = accounts_db_config
.as_ref()
.map(|config| config.enable_experimental_accumulator_hash)
.unwrap_or(default_accounts_db_config.enable_experimental_accumulator_hash)
.into();

let paths_is_empty = paths.is_empty();
let mut new = Self {
paths,
Expand All @@ -2671,6 +2691,7 @@ impl AccountsDb {
test_skip_rewrites_but_include_in_bank_hash,
storage_access,
scan_filter_for_shrinking,
is_experimental_accumulator_hash_enabled: enable_experimental_accumulator_hash,
..Self::default_with_accounts_index(
accounts_index,
base_working_path,
Expand Down Expand Up @@ -2737,6 +2758,18 @@ impl AccountsDb {
.expect("Cluster type must be set at initialization")
}

/// Returns if the experimental accounts lattice hash is enabled
pub fn is_experimental_accumulator_hash_enabled(&self) -> bool {
self.is_experimental_accumulator_hash_enabled
.load(Ordering::Acquire)
}

/// Sets if the experimental accounts lattice hash is enabled
pub fn set_is_experimental_accumulator_hash_enabled(&self, is_enabled: bool) {
self.is_experimental_accumulator_hash_enabled
.store(is_enabled, Ordering::Release);
}

/// While scanning cleaning candidates obtain slots that can be
/// reclaimed for each pubkey. In addition, if the pubkey is
/// removed from the index, insert in pubkeys_removed_from_accounts_index.
Expand Down Expand Up @@ -7147,6 +7180,71 @@ impl AccountsDb {
(accounts_hash, total_lamports)
}

/// Calculates the accounts lt hash
///
/// Only intended to be called at startup (or by tests).
/// Only intended to be used while testing the experimental accumulator hash.
pub fn calculate_accounts_lt_hash_at_startup(
Copy link

@dmakarov dmakarov Oct 3, 2024

Choose a reason for hiding this comment

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

intentionally no tests for this method in accounts_db.rs?

Copy link
Author

@brooksprumo brooksprumo Oct 3, 2024

Choose a reason for hiding this comment

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

The tests are within bank/accounts_lt_hash.rs, so that I could setup the initial state with multiple banks and transactions first.

Edit: And also leverage the bank's accounts lt hash as a comparison for what calculate_accounts_lt_hash_at_startup() returned.

Copy link

Choose a reason for hiding this comment

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

maybe a line in the comment above explaining that this method is tested elsewhere and the absence of tests is not an omission could be helpful.

Copy link
Author

Choose a reason for hiding this comment

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

How strongly do you feel about this? I ask because personally I usually grep for functions to find their tests. I'm also slightly apprehensive that if the test moves/is renamed/refactored that a comment here would not get updated, and thus be yet-another wrong comment in the codebase.

Copy link

@dmakarov dmakarov Oct 4, 2024

Choose a reason for hiding this comment

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

up to you. I would find it helpful. i don't insist it's the same for everyone. Comments become outdated for other reasons too.

&self,
ancestors: &Ancestors,
startup_slot: Slot,
) -> AccountsLtHash {
debug_assert!(self.is_experimental_accumulator_hash_enabled());

// This impl iterates over all the index bins in parallel, and computes the lt hash
// sequentially per bin. Then afterwards reduces to a single lt hash.
// This implementation is quite fast. Runtime is about 150 seconds on mnb as of 10/2/2024.
// The sequential implementation took about 6,275 seconds!
// A different parallel implementation that iterated over the bins *sequentially* and then
// hashed the accounts *within* a bin in parallel took about 600 seconds. That impl uses
// less memory, as only a single index bin is loaded into mem at a time.
Copy link

Choose a reason for hiding this comment

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

yeah. using more memory should be fine. it is at startup anyway.

let lt_hash = self
.accounts_index
.account_maps
.par_iter()
.fold(
LtHash::identity,
|mut accumulator_lt_hash, accounts_index_bin| {
for pubkey in accounts_index_bin.keys() {
let account_lt_hash = self
.accounts_index
.get_with_and_then(
&pubkey,
Some(ancestors),
Some(startup_slot),
false,
|(slot, account_info)| {
(!account_info.is_zero_lamport()).then(|| {
self.get_account_accessor(
slot,
&pubkey,
&account_info.storage_location(),
)
.get_loaded_account(|loaded_account| {
Self::lt_hash_account(&loaded_account, &pubkey)
})
// SAFETY: The index said this pubkey exists, so
// there must be an account to load.
.unwrap()
})
},
)
.flatten();
if let Some(account_lt_hash) = account_lt_hash {
accumulator_lt_hash.mix_in(&account_lt_hash.0);
}
}
accumulator_lt_hash
},
)
.reduce(LtHash::identity, |mut accum, elem| {
accum.mix_in(&elem);
accum
});

AccountsLtHash(lt_hash)
}

/// This is only valid to call from tests.
/// run the accounts hash calculation and store the results
pub fn update_accounts_hash_for_tests(
Expand Down
3 changes: 1 addition & 2 deletions accounts-db/src/accounts_hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1262,8 +1262,7 @@ pub const ZERO_LAMPORT_ACCOUNT_HASH: AccountHash =
pub struct AccountLtHash(pub LtHash);

/// The AccountLtHash for a zero-lamport account
pub const ZERO_LAMPORT_ACCOUNT_LT_HASH: AccountLtHash =
AccountLtHash(LtHash([0; LtHash::NUM_ELEMENTS]));
pub const ZERO_LAMPORT_ACCOUNT_LT_HASH: AccountLtHash = AccountLtHash(LtHash::identity());

/// Lattice hash of all accounts
#[derive(Debug, Clone, Eq, PartialEq)]
Expand Down
4 changes: 4 additions & 0 deletions ledger-tool/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,10 @@ pub fn accounts_db_args<'a, 'b>() -> Box<[Arg<'a, 'b>]> {
.possible_values(&["mmap", "file"])
.help("Access account storage using this method")
.hidden(hidden_unless_forced()),
Arg::with_name("accounts_db_experimental_accumulator_hash")
.long("accounts-db-experimental-accumulator-hash")
.help("Enables the experimental accumulator hash")
.hidden(hidden_unless_forced()),
]
.into_boxed_slice()
}
Expand Down
1 change: 1 addition & 0 deletions programs/sbf/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions runtime/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ license = { workspace = true }
edition = { workspace = true }

[dependencies]
ahash = { workspace = true }
aquamarine = { workspace = true }
arrayref = { workspace = true }
base64 = { workspace = true }
Expand Down
Loading
Loading