Remove old way of account hashing #16442

brooksprumo · 2021-04-08T16:15:20Z

Account data hashing used to use different ways of hashing on different
clusters. That is no longer the case, but the old code still existed.
This commit removes that old, now used code.

NOTE The golden hash values in bank.rs needed to be updated. Since the original code that selected the hash algorithm used if > instead of if >=, this meant that the genesis block's hash always used the old hashing method, which is no longer valid.

The new golden hash values in accounts_db.rs were also validated by
changing if slot > to if slot >= on master and running cargo test,
then confirming the actual values matched the golden values in this commit.

Validated by running cargo test from within runtime.

(This work was originally based on PR #16375 from @jeffwashington)

runtime/src/accounts_db.rs

brooksprumo · 2021-04-08T16:20:20Z

runtime/src/accounts_db.rs

@@ -2955,53 +2910,14 @@ impl AccountsDb {
        hasher.result()
    }

-    pub fn hash_account_data(
+    fn hash_account_data(


It's hard to tell from the Github diff; I've basically just renamed the blake3_hash_account_data() function to hash_account_data. (And also removed pub, because it doesn't look like anything else was/is calling this function.)

runtime/src/accounts_db.rs

brooksprumo · 2021-04-08T16:23:57Z

runtime/src/bank.rs

                );
            }
            if bank.slot == 128 {
                assert_eq!(
                    bank.hash().to_string(),
-                    "FQnVhDVjhCyfBxFb3bdm3CLiuCePvWuW5TGDsLBZnKAo"
+                    "AtWu4tubU9zGFChfHtQghQx3RVWtMQu6Rj49rQymFc4z"


Same thing about these hashes too.

codecov · 2021-04-08T19:42:00Z

Codecov Report

Merging #16442 (7dd2de2) into master (f641429) will decrease coverage by 0.0%.
The diff coverage is 90.6%.

@@            Coverage Diff            @@
##           master   #16442     +/-   ##
=========================================
- Coverage    80.0%    80.0%   -0.1%     
=========================================
  Files         413      413             
  Lines      112207   112135     -72     
=========================================
- Hits        89850    89763     -87     
- Misses      22357    22372     +15

runtime/src/accounts_db.rs

ryoqun · 2021-04-09T11:44:29Z

@brooksprumo I've run my-not-merged-yet-too-old hidden facility of running your branch against mainnet-beta and testnet: https://buildkite.com/solana-labs/solana/builds/43855#f4be3751-c5eb-4c49-9f7e-360f03c0d23d :)

#12175

ryoqun

code looks pretty what I really wanted to be like! good work!

please check the golden hashes by the method t I said in another comment and wait for the live-cluster tests..

jeffwashington · 2021-04-09T14:41:07Z

I can confirm that test but it may be Monday. Sent from my phone.

…

On Apr 9, 2021, at 8:38 AM, Brooks Prumo ***@***.***> wrote: @brooksprumo commented on this pull request. In runtime/src/accounts_db.rs: > lamports: 129, slot: 0, pubkey: pubkey128, }, CalculateHashIntermediate { version: 3, - hash: Hash::from_str("7nhnUMjRsaA83HgvEJVv3YrDqHd1SCoVbvsWDTXzCgfh").unwrap(), + hash: Hash::from_str("BcfjjoWJDE5rEPsZccCQU1pCPMCH5Z2H7uHZVk4Yz4gt").unwrap(), When you checkout the old code, and replace > with >=, do you see these new hashes? Great idea! I grabbed master at 8ec7e2e and changed account_db.rs on lines 2885 and 2918 to be if slot >= then ran cargo test --lib from runtime. The tests failed as expected, and the "actual" hashes do match the new ones I've used (at least for accounts_db.rs). However, doing this method, the test test_accountsdb_calculate_accounts_hash_without_index() at line 5541 also failed. I'm assuming that's OK, but can you confirm, @ryoqun? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

brooksprumo · 2021-04-09T14:41:13Z

code looks pretty what I really wanted to be like! good work!

Thanks!

please check the golden hashes by the method t I said in another comment and wait for the live-cluster tests..

I've checked the golden hashes per the method you described and the hashes match. I left a reply, as there was a new test failure, but I'm assuming its erroneous.

Also, looks like the live-cluster tests have completed successfully. Should I make this a real PR now?

Pull request has been modified.

ryoqun · 2021-04-09T15:03:30Z

Should I make this a real PR now?

Of course. :)

runtime/src/accounts_db.rs

ryoqun

LGTM with nit. Thanks for working on this carefully.

Pull request has been modified.

Account data hashing used to use different ways of hashing on different clusters. That is no longer the case, but the old code still existed. This commit removes that old, now used code. **NOTE** The golden hash values in bank.rs needed to be updated. Since the original code that selected the hash algorithm used `if >` instead of `if >=`, this meant that the genesis block's hash _always_ used the old hashing method, which is no longer valid. Validated by running `cargo test` successfully.

jeffwashington · 2021-04-13T19:26:47Z

runtime/src/snapshot_utils.rs

@@ -979,7 +979,6 @@ pub fn process_accounts_package_pre(
        let (hash, lamports) = AccountsDb::calculate_accounts_hash_without_index(
            &accounts_package.storages,
            thread_pool,
-            accounts_package.cluster_type,


Hey, @brooksprumo , in another pr you could consider removing cluster_type from accounts_package maybe. Maybe we don't need it for anything else?

@ryoqun Do you know if cluster_type is used for anything still?

It looks like Cluster Type isn't really used in accounts.rs nor accounts_db.rs, but is used in bank.rs, and therefore Genesis config/utils. I'm not sure if the Cluster Type is bank.rs still needs to be there though. Same thing for Snapshot Utils/Serde.

In the Bank impl, there's also a lot of test-only configuration only. I'm still trying to understand if all the cluster type related code can be moved to the actual test module. I'm going to keep reading through to code to learn more; happy to get your thoughts on Cluster Type in general too!

jeffwashington

lgtm.
Ran bench-tps, ledger-tool verify, and joined mainnet-beta. All looked fine.

brooksprumo commented Apr 8, 2021

View reviewed changes

ryoqun reviewed Apr 9, 2021

View reviewed changes

runtime/src/accounts_db.rs Show resolved Hide resolved

ryoqun reviewed Apr 9, 2021

View reviewed changes

runtime/src/accounts_db.rs Show resolved Hide resolved

ryoqun previously approved these changes Apr 9, 2021

View reviewed changes

brooksprumo force-pushed the bprumo/account-hashing branch from 8955469 to 0527a14 Compare April 9, 2021 14:58

brooksprumo marked this pull request as ready for review April 9, 2021 16:23

jeffwashington added the CI Pull Request is ready to enter CI label Apr 12, 2021

solana-grimes removed the CI Pull Request is ready to enter CI label Apr 12, 2021

brooksprumo mentioned this pull request Apr 12, 2021

Use slot 1 for tests' sample store and accounts #16494

Merged

brooksprumo force-pushed the bprumo/account-hashing branch from 0527a14 to 749d02f Compare April 12, 2021 22:22

ryoqun reviewed Apr 13, 2021

View reviewed changes

runtime/src/accounts_db.rs Outdated Show resolved Hide resolved

ryoqun previously approved these changes Apr 13, 2021

View reviewed changes

ryoqun mentioned this pull request Apr 13, 2021

Minimum diff for blake3 #16455

Closed

brooksprumo force-pushed the bprumo/account-hashing branch from 25c7768 to b1f211e Compare April 13, 2021 14:12

brooksprumo force-pushed the bprumo/account-hashing branch from b1f211e to 7dd2de2 Compare April 13, 2021 14:16

jeffwashington reviewed Apr 13, 2021

View reviewed changes

jeffwashington self-requested a review April 13, 2021 19:27

jeffwashington approved these changes Apr 13, 2021

View reviewed changes

brooksprumo merged commit 17aa45f into solana-labs:master Apr 13, 2021

brooksprumo deleted the bprumo/account-hashing branch April 13, 2021 19:43

jeffwashington mentioned this pull request Apr 15, 2021

BPF program serialization/deserialization may no longer be necessary #14523

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove old way of account hashing #16442

Remove old way of account hashing #16442

brooksprumo commented Apr 8, 2021 •

edited

Loading

brooksprumo Apr 8, 2021

brooksprumo Apr 8, 2021

codecov bot commented Apr 8, 2021 •

edited

Loading

ryoqun commented Apr 9, 2021

ryoqun left a comment

jeffwashington commented Apr 9, 2021 via email

brooksprumo commented Apr 9, 2021

ryoqun commented Apr 9, 2021

ryoqun left a comment

jeffwashington Apr 13, 2021

brooksprumo Apr 13, 2021

brooksprumo Apr 13, 2021 •

edited

Loading

jeffwashington left a comment

Remove old way of account hashing #16442

Remove old way of account hashing #16442

Conversation

brooksprumo commented Apr 8, 2021 • edited Loading

brooksprumo Apr 8, 2021

Choose a reason for hiding this comment

brooksprumo Apr 8, 2021

Choose a reason for hiding this comment

codecov bot commented Apr 8, 2021 • edited Loading

Codecov Report

ryoqun commented Apr 9, 2021

ryoqun left a comment

Choose a reason for hiding this comment

jeffwashington commented Apr 9, 2021 via email

brooksprumo commented Apr 9, 2021

ryoqun commented Apr 9, 2021

ryoqun left a comment

Choose a reason for hiding this comment

jeffwashington Apr 13, 2021

Choose a reason for hiding this comment

brooksprumo Apr 13, 2021

Choose a reason for hiding this comment

brooksprumo Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

jeffwashington left a comment

Choose a reason for hiding this comment

brooksprumo commented Apr 8, 2021 •

edited

Loading

codecov bot commented Apr 8, 2021 •

edited

Loading

brooksprumo Apr 13, 2021 •

edited

Loading