Adds AccountHash newtype #33597
Adds AccountHash newtype #33597
Conversation
| @@ -2399,13 +2399,13 @@ impl<'a> AppendVecScan for ScanState<'a> { | |||
| } else if self.config.check_hash && computed_hash != loaded_hash { | |||
| info!( | |||
| "hash mismatch found: computed: {}, loaded: {}, pubkey: {}", | |||
| computed_hash, loaded_hash, pubkey | |||
| computed_hash.0, loaded_hash.0, pubkey | |||
There was a problem hiding this comment.
I did .0 so that the output would be the hash directly. The AccountHash doesn't have Display implemented, so I'd either (1) need to impl Display, or (2) change the output to be Debug (with {:?}), but then the output would be AccountHash(actual-hash-here), which seems noisier to me. Let me know if you'd like me to change the message format though.
There was a problem hiding this comment.
I was thinking you implemented Borrow, I think. But if you don't specify what you're borrowing as, it likely will be &AccountsHash instead of calling the borrow &Hash.
I think I'd prefer display or debug fmt than .0. But, this isn't that important. Probably fine to leave as-is.
| ); | ||
| self.mismatch_found.fetch_add(1, Ordering::Relaxed); | ||
| } | ||
| } | ||
| let source_item = CalculateHashIntermediate { | ||
| hash: loaded_hash, | ||
| hash: loaded_hash.0, |
There was a problem hiding this comment.
is this preferred over *&loaded_hash? I'm not sure.
There was a problem hiding this comment.
maybe .into()? I'm not sure the cleanest. I've never liked .0. I find it hard to remember what .0 is. Not a deal breaker. Just brainstorming on the best api.
There was a problem hiding this comment.
Since we're going from AccountHash to Hash, will *&loaded_hash work? I haven't tried it, but since AccountHash impls Borrow, maybe it does work?
I also didn't implement From on AccountHash, which I also could. But without that impl, I don't think .into() will work here.
I've never liked .0. I find it hard to remember what .0 is.
Yes, I agree and feel the same way. I was planning on changing CalculateHashIntermediate::hash to be an AccountHash, which would fix this issue. Does that work?
| if (config.check_hash || hash_is_missing) && !self.is_filler_account(pubkey) { | ||
| let computed_hash = | ||
| loaded_account.compute_hash(*slot, pubkey, config.include_slot_in_hash); | ||
| if hash_is_missing { | ||
| loaded_hash = computed_hash; | ||
| } | ||
| else if config.check_hash && computed_hash != loaded_hash { | ||
| info!("hash mismatch found: computed: {}, loaded: {}, pubkey: {}", computed_hash, loaded_hash, pubkey); | ||
| info!("hash mismatch found: computed: {}, loaded: {}, pubkey: {}", computed_hash.0, loaded_hash.0, pubkey); |
There was a problem hiding this comment.
I think this is the same as #33597 (comment)
brooksprumo
force-pushed
the
account-hash/newtype
branch
from
a7ca3dc to
d0a963d
Compare
Codecov Report
@@ Coverage Diff @@
## master #33597 +/- ##
=======================================
Coverage 81.7% 81.7%
=======================================
Files 807 807
Lines 218252 218252
=======================================
+ Hits 178438 178467 +29
+ Misses 39814 39785 -29 |
Problem
We use a bare
Hashin many places. This can be (1) confusing trying to figure out what kind of hash a loneHashis supposed to be, and (2) error-prone, as it allows accidentally using a generic hash when an account hash was needed.Summary of Changes
Add
AccountHash, a newtype aroundHash, that is the result of hashing an account.This PR doesn't use
AccountHasheverywhere (e.x. not in the storage file code, and almost no changes in the accounts hashing code). That can be done in subsequent PRs, as I was trying to keep this one as small as possible.I tried to split up the PR across a few commits to make it easier to follow. Hopefully that helps!