Use enum for data source with calc_accounts_hash() #28584
Conversation
brooksprumo
force-pushed
the
calculate-accounts-hash/data-source
branch
2 times, most recently
from
0f3420f to
cc590e7
Compare
brooksprumo
force-pushed
the
calculate-accounts-hash/data-source
branch
from
cc590e7 to
d58c092
Compare
brooksprumo
added
CI
|
This is the last PR for |
| self.calculate_accounts_hash_from_storages(config, &storages, timings) | ||
| } else { | ||
| self.calculate_accounts_hash_from_index(slot, config) | ||
| self.calculate_accounts_hash_from_storages(config, &storages, timings) |
There was a problem hiding this comment.
outside scope of this PR: it's odd to me that
calculate_accounts_hash_from_index takes (slot, config), but
calculate_accounts_hash_from_storages takes (config, storages, timings)
It seems like it's a cleaner interface to have these take the same args and calculate_accounts_hash_from_storages would wrap the functionality above (7230 - 7255).
| /// Specify the source of the accounts data when calculating the accounts hash | ||
| #[derive(Debug, Copy, Clone, Eq, PartialEq)] | ||
| pub enum CalcAccountsHashDataSource { | ||
| Index, |
There was a problem hiding this comment.
IndexForTests
This option should not be used by the real validator. This code path could be deleted completely except it is useful as a check on the account index, clean, and the storage based calculation.
There was a problem hiding this comment.
The Index version is used by Bank::update_accounts_hash(), which is called by snapshot_utils::bank_to_full/incrmental_snapshot_archive(), which is called by solana-leger-tool (with create-snapshot) and solana-test-validator (when warping).
The Index version is also called in Accounts Background Service, if test_hash_calculation is true when handling snapshot requests.
I understand that the normal validator does not use the Index version ever, but it does seem a bit strange to me to see ForTests in code paths/call graphs that are not necessarily tests.
Since the previous parameter was just use_index, and not use_index_for_tests, would it be OK to leave this variant as Index? I could also add comments here (or on the uses) saying this is meant for tests/not-normal-validators. Would that work?
There was a problem hiding this comment.
it is confusing, but it would be nice if we're documenting it that somehow we indicate that it is for debugging or tests. A comment is better than nothing.
There was a problem hiding this comment.
Ok will do. I'm going to fix this in a subsequent PR, since (1) it feels different than this PR's intent, and (2) then I don't need to babysit CI again.
I've created this issue to track the work:
#28606
Problem
I find that the raw boolean for the
use_indexparameter incalc_accounts_hash()could be more obvious. (1) There are multiple boolean parameters to this family of functions, and when boolean literals are used, I lose track of what each value is for. (2) Raw booleans make it possible to accidentally switch parameters around inadvertently during a refactor since the types are all the same.Summary of Changes
Covert the
use_indexboolean to an enum. This is now type-safe and self-documenting. Yay!