Refactors accounts lt hash startup verification #3397
Conversation
| // When generating the index, we want to calculate the duplicates lt hash value (needed to do | ||
| // the lattice-based verification of the accounts in the background) optimistically. | ||
| // This means, either when the cli arg is set, or when the snapshot has an accounts lt hash. | ||
| let is_accounts_lt_hash_enabled = | ||
| accounts_db.is_experimental_accumulator_hash_enabled() || has_accounts_lt_hash; |
There was a problem hiding this comment.
Here's the main change that the rest of the PR falls out from: When loading from a snapshot, here is where we generate the index. And generate_index() needs to know if it should calculate the duplicates lt hash value.
Before snapshot support was added for accounts lt hash, the only thing we needed to check was the CLI arg. Now we need to check the CLI arg and the snapshot. Inside generate_index() we don't have a way to know if the accounts lt hash was in the snapshot, so we pass in a new parameter here.
(More info: And since we don't have a Bank instance here yet, we don't know which feature gates are enabled. In typical snapshot style, we use the presence of the accounts lt hash in the snapshot as a proxy for if the feature is enabled or not.)
| @@ -8597,6 +8597,7 @@ impl AccountsDb { | |||
| limit_load_slot_count_from_snapshot: Option<usize>, | |||
| verify: bool, | |||
| genesis_config: &GenesisConfig, | |||
| should_calculate_duplicates_lt_hash: bool, | |||
There was a problem hiding this comment.
Next, here's the new parameter for generate_index(). We use this parameter to determine whether or not to calculate the duplicates lt hash value, and not checking the cli arg.
A nice fallout is that now we only conditionally malloc the LtHash when handling the duplicates (below). No more perf hit when not using accounts lt hash.
| let mut duplicates_lt_hash = | ||
| should_calculate_duplicates_lt_hash.then(|| Box::new(DuplicatesLtHash::default())); |
There was a problem hiding this comment.
Here's where we conditionally malloc the LtHash. And the other change in this function can now switch based on if this is Some/None.
| match ( | ||
| self.duplicates_lt_hash.is_some(), | ||
| other.duplicates_lt_hash.is_some(), | ||
| ) { | ||
| (true, true) => { | ||
| // SAFETY: We just checked that both values are Some | ||
| self.duplicates_lt_hash | ||
| .as_mut() | ||
| .unwrap() | ||
| .0 | ||
| .mix_in(&other.duplicates_lt_hash.as_ref().unwrap().0); | ||
| } | ||
| (true, false) => { | ||
| // nothing to do; `other` doesn't have a duplicates lt hash | ||
| } | ||
| (false, true) => { | ||
| // `self` doesn't have a duplicates lt hash, so pilfer from `other` | ||
| self.duplicates_lt_hash = other.duplicates_lt_hash; | ||
| } | ||
| (false, false) => { | ||
| // nothing to do; no duplicates lt hash at all | ||
| } | ||
| } |
There was a problem hiding this comment.
Now that handling the duplicates can return the LtHash as an Option, we need to update how we handle merging. It's actually quite nice, as now intermediate threads that spin up to just do the merge-reduce don't need to malloc an LtHash either (they can use the allocation from the merged-from side).
| let verify_kind = match ( | ||
| duplicates_lt_hash.is_some(), | ||
| self.rc | ||
| .accounts | ||
| .accounts_db | ||
| .is_experimental_accumulator_hash_enabled(), | ||
| ) { | ||
| (true, _) => VerifyKind::Lattice, | ||
| (false, false) => VerifyKind::Merkle, | ||
| (false, true) => { | ||
| // Calculating the accounts lt hash from storages *requires* a duplicates_lt_hash. | ||
| // If it is None here, then we must use the index instead, which also means we | ||
| // cannot run in the background. | ||
| config.run_in_background = false; | ||
| VerifyKind::Lattice | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
And finally we get to startup verification! We can now primarily use the Option-ness of the passed-in duplicates_lt_hash to decide which kind of accounts verification to do. We still support using the CLI arg as a way to force using the lattice-based verification too (which is useful in tests, and also experimenting on running validators before the feature gate is enabled).
Problem
Startup verification was a bit clumsy due to the separate handling of the accounts lt hash via the cli arg and via the snapshot. These can (should) be unified, and also remove unnecessary memory allocations when the accounts lt hash is not in use.
Summary of Changes
Refactors accounts lt hash startup verification. Now, if the cli arg is set, or if the snapshot contains an accounts lt hash, then startup accounts verification will use lattice-based verification.