-
Notifications
You must be signed in to change notification settings - Fork 185
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
Refactors startup verify with accounts lt hash #3318
base: master
Are you sure you want to change the base?
Conversation
let verify_kind = if self | ||
.rc | ||
.accounts | ||
.accounts_db | ||
.is_experimental_accumulator_hash_enabled() | ||
{ | ||
VerifyKind::Lattice | ||
} else { | ||
VerifyKind::Merkle | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the main change: we only look at the CLI arg to determine which hash kind to verify with.
Note that Bank::is_accounts_lt_hash_enabled() is the same as AccountsDb::is_experimental_accumulator_hash_enabled() today, but not when we add feature gate logic. This PR future proofs the feature gate changes, thus reducing future complexity.
if self.is_experimental_accumulator_hash_enabled() { | ||
let old_val = outer_duplicates_lt_hash.replace(duplicates_lt_hash); | ||
assert!(old_val.is_none()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now also wrap the duplicates lt hash, that generate_index() computes, in an Option. If the CLI arg is not set, we did not compute the duplicates. This Option makes that explicit.
In the future, we'll be able to check the snapshot if the accounts lt hash is enabled.
/// Spin up the background services fully then test taking & verifying snapshots | ||
#[test_matrix( | ||
V1_2_0, | ||
[Development, Devnet, Testnet, MainnetBeta], | ||
[VerifyAccountsKind::Merkle, VerifyAccountsKind::Lattice] | ||
)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes add permutations on this snapshot test for verifying the accounts with both merkle and lattice based accounts hashing.
match verify_kind { | ||
VerifyKind::Lattice => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the rest of the changes here look like a lot, but they are just moving from if {} else {}
to a match
with the VerifyKind
arms. Ignoring whitespace in the diff will help. No logic was changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By Introducing the enum VerifyKind
variant and using it for hash verify instead of checking the feature enable makes the code clearer, a good improvment.
lgtm.
Problem
While working on a branch to feature-gate accounts lt hash changes, I ran into issues in tests around startup accounts verification.
When the feature was enabled, I would get startup account verification mismatches. This is because the code to compute the accounts lt hash at startup from storages relies on
generate_index()
finding the duplicates lt hash. Since generate_index() runs before there is a Bank instance, that function cannot check if the feature is enabled or not. Instead it relies on the accounts lt hash cli arg. So if the feature is enabled and the cli arg is not, then generate_index() would not compute the duplicates lt hash, yet verify_accounts_hash() would need it.Summary of Changes