Fix accounts index generation at startup when we don't use disk index

[HaoranYi]

Problem

During account's index generation at startup, one of the important steps is to find duplicated pubkeys and send them to clean.

However, the code path that finds duplicates at index generation, when disk index is disabled, is incorrect. And we are are not finding all the duplicates. This makes clean unable to clean old storages, and results in continue growing of account maps.

Note this only affects when the validator running with disk index disabled.

Summary of Changes

Fix finding duplicated (pubkey, slots) at start up when we disable disk index.

Fixes #

[HaoranYi]

Graph of account's map before the fix.

[HaoranYi]

Graph of account's map after the fix.

[brooksprumo]

Can you set the ACCOUNTS_INDEX_CONFIG_FOR_TESTING.index_limit_mb = IndexLimitMb::InMemOnly, and run the solana_runtime and solana_accounts_db tests?

[HaoranYi]

Can you set the ACCOUNTS_INDEX_CONFIG_FOR_TESTING.index_limit_mb = IndexLimitMb::InMemOnly, and run the solana_runtime and solana_accounts_db tests?

Both tests passed.

test result: ok. 538 passed; 0 failed; 4 ignored; 0 measured; 0 filtered out; finished in 116.30s

     Running tests/accounts.rs (/home/sol/src/agave/target/debug/deps/accounts-06e1f00a7377d075)

running 2 tests
test test_shrink_and_clean ... ok
test test_bad_bank_hash ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 24.62s

     Running tests/stake.rs (/home/sol/src/agave/target/debug/deps/stake-c3f800f796ce426c)

running 4 tests
test test_stake_create_and_split_single_signature ... ok
test test_stake_create_and_split_to_existing_system_account ... ok
test test_create_stake_account_from_seed ... ok
test test_stake_account_lifetime ... ok

test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.92s

   Doc-tests solana_runtime

running 1 test
test runtime/src/bank/builtins/core_bpf_migration/mod.rs - bank::builtins::core_bpf_migration::Bank::upgrade_core_bpf_program (line 287) ... ignored

test result: ok. 0 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in 0.00s

sol@haoran-dev-ny:~/src/agave/accounts-db$ tail -n 20 accounts_db_test.log
test accounts_index::tests::test_new_entry_and_update_code_paths ... ok
test accounts_db::tests::test_shrink_ancient_overflow_with_min_size ... ok
test shared_buffer_reader::tests::test_shared_buffer_sweep ... ok
test ancient_append_vecs::tests::test_write_ancient_accounts ... ok
test ancient_append_vecs::tests::test_clear_should_shrink_after_cutoff_simple ... ok
test ancient_append_vecs::tests::test_calc_accounts_to_combine_many_refs ... ok
test ancient_append_vecs::tests::test_calc_accounts_to_combine_simple ... ok
test accounts_db::tests::test_shrink_collect_simple ... ok
test storable_accounts::tests::test_storable_accounts_by_slot ... ok
test accounts_db::scan_account_storage::tests::test_accountsdb_scan_snapshot_stores_binning::append_vec ... ok
test accounts_db::scan_account_storage::tests::test_accountsdb_scan_snapshot_stores_binning::hot_storage ... ok

test result: ok. 599 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 31.72s

[brooksprumo]

One real question, one double check, and some nits

[HaoranYi]

Rebase to master to pickup #4069

[brooksprumo]

Thanks for fixing this!

[jeffwashington]

is this change necessary since this just got merged?
#4044

[jeffwashington]

is this change necessary since this just got merged? #4044

it seems like we could simplify both disk and in-mem index startup code if we just simply identify the pubkeys that are dups. We no longer have to identify them by slot, I don't think.

[HaoranYi]

is this change necessary since this just got merged? #4044

it seems like we could simplify both disk and in-mem index startup code if we just simply identify the pubkeys that are dups. We no longer have to identify them by slot, I don't think.

We still need to find all duplicated (key, slot) correctly. Duplicates are used for other things than clean too. Account data length and lattice hash both depends on finding the correct set of duplicated (key, slot) at startup.

[HaoranYi]

With #4044 backported, we probably don't need to backport this one.

Account data len may be wrong for no-disk. But that's just used in a metrics. So should be fine to let it be wrong.

But we still need this in master for lattic hash in master.

[jeffwashington]

there is a race here. 2 competing insert threads here could both find slot_list.len() = 1. Then, they serilialize access with the write lock on lock_and_update_slot_list so that one makes len 2, the next 3. I don't think this is bad in this case, but it is a race condition. lock_and_update_slot_list returns slot list len after insertion using write lock. We could set other_slot to None if lock_and_update_slot_list returns != 2, which means someone else won the race.

[HaoranYi]

yeah. fixed in ef3e873.

[HaoranYi]

it seems like we could simplify both disk and in-mem index startup code if we just simply identify the pubkeys that are dups. We no longer have to identify them by slot, I don't think.

We still need to populate (pubkey, slot) at startup.

This is because at startup, we use uncleaned_pubkeys to drive clean. And uncleaned_pubkeys require slot.

agave/accounts-db/src/accounts_db.rs

Line 1557 in d11072e

uncleaned_pubkeys: DashMap<Slot, Vec<Pubkey>>,

And the (pubkey, slot) is used here.

agave/accounts-db/src/accounts_db.rs

Line 8786 in d11072e

self.uncleaned_pubkeys.entry(slot).or_default().push(key);

Yes, with #4044 landed. We can

1. get rid of uncleaned_root from clean.
2. don't populate uncleaned_roots at startup.

But these works are out of scope of this PR.

I have two other follow up PRs for them #4095 and #4092.