Recompute hash on load if default hash is stored for the account

[HaoranYi]

Problem

We are going to store default hash to account's storage in #469.

Before #469 is merged, we need to have all nodes support recompute hash on
load. This PR adds that support.

Summary of Changes

Recompute account's hash on load if the hash stored in account storage is
default hash.

Fixes #

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 81.8%. Comparing base (b1919bd) to head (7449db1).
Report is 30 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #519   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         842      842           
  Lines      228343   228428   +85     
=======================================
+ Hits       186954   187040   +86     
+ Misses      41389    41388    -1     

[jeffwashington]

lgtm

[jeffwashington]

we should consider backporting this. before the next release is cut.

[brooksprumo]

Are the two functions modified in this PR the only two that are used to load account hashes? Mostly wondering if the Accounts Hash Cache stuff is covered by this PR.

[brooksprumo]

we should consider backporting this. before the next release is cut.

This was posted over in Discord a few days ago: https://discord.com/channels/428295358100013066/439194979856809985/1222663250206789673

Since this change does introduce some potential perf changes (we're now potentially recomputing account hashes), it may need additional justification for backporting. I know that the v1.17 to v1.18 transition won't be impacted, since all real/alive/non-zero lamport accounts will have a non-default account hash, and thus not recompute the hash here, but the v1.18 to v2.0 transition will be impacted. I'm not sure by how much, or how to quantify it though. Regardless of backporting or not, that transition impact will impact some version regardless, so may be OK to backport.

[jeffwashington]

(we're now potentially recomputing account hashes

This will not happen on any 1.18 validators. We could even leave out the accum part that uses the write cache in this change and thus, the backport. the write cache already computes a hash. The only issue here is loading from a snapshot (and thus append vec) where we did not include non-default hashes when the snapshot was created. The only effective change will be at startup no matter what.

[jeffwashington]

Are the two functions modified in this PR the only two that are used to load account hashes? Mostly wondering if the Accounts Hash Cache stuff is covered by this PR.

This is already handled here:

agave/accounts-db/src/accounts_db.rs

Lines 2273 to 2285 in 8153c52

	let hash_is_missing = loaded_hash == AccountHash(Hash::default());
	if self.config.check_hash || hash_is_missing {
	let computed_hash = loaded_account.compute_hash(pubkey);
	if hash_is_missing {
	loaded_hash = computed_hash;
	} else if self.config.check_hash && computed_hash != loaded_hash {
	info!(
	"hash mismatch found: computed: {}, loaded: {}, pubkey: {}",
	computed_hash.0, loaded_hash.0, pubkey
	);
	self.mismatch_found.fetch_add(1, Ordering::Relaxed);
	}
	}

and even here for the testing only index version:

agave/accounts-db/src/accounts_db.rs

Lines 6665 to 6672 in 8153c52

	let hash_is_missing = loaded_hash == AccountHash(Hash::default());
	if config.check_hash || hash_is_missing {
	let computed_hash =
	loaded_account.compute_hash(pubkey);
	if hash_is_missing {
	loaded_hash = computed_hash;
	}
	else if config.check_hash && computed_hash != loaded_hash {

[brooksprumo]

[mergify]

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

[jeffwashington]

This was posted over in Discord a few days ago: https://discord.com/channels/428295358100013066/439194979856809985/1222663250206789673

Since this change does introduce some potential perf changes (we're now potentially recomputing account hashes), it may need additional justification for backporting. I know that the v1.17 to v1.18 transition won't be impacted, since all real/alive/non-zero lamport accounts will have a non-default account hash, and thus not recompute the hash here, but the v1.18 to v2.0 transition will be impacted. I'm not sure by how much, or how to quantify it though. Regardless of backporting or not, that transition impact will impact some version regardless, so may be OK to backport.

"...backports to v1.18 should only be done if necessary to fix a bug or significant performance regression". The issue here is state size and resulting poor performance. Writing default hash (and being tolerant of that) will be delayed by a release cycle if we do not put this in 1.18. 1.18 will fail to load snapshots generated by the next version.