Removes unnecessary Accounts constructors

[brooksprumo]

Problem

There are so many Accounts and AccountsDb constructors... Lots of overlap/duplication.

Now that #34466 has merged, we can more easily see how almost all of the Accounts constructors are unnecessary. Instead, construct AccountsDb and pass that into the one remaining Accounts constructor¹.

Summary of Changes

• Removes new_with_config()
• Removes default_for_tests()
• Removes new_with_config_for_benches()
• Removes new_with_config_for_tests()

Notes

Reviewing this PR will likely be easier by going commit-by-commit.

Each commit basically removes one constructor for Accounts, and updates all its old callers.

Footnotes

1. Some fancy software peoples may call this Dependency Injection (TM) ↩

[codecov]

Codecov Report

Merging #34471 (e7f48d0) into master (aaccbdd) will increase coverage by 0.0%.
The diff coverage is 87.5%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #34471   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         819      819           
  Lines      220894   220907   +13     
=======================================
+ Hits       180753   180820   +67     
+ Misses      40141    40087   -54     

[steviez]

Almost all of the cases you've touched look something like this one:

let accounts = Accounts::new(Arc::new(accounts_db))

Wondering if we should make Accounts::new() take an AccountsDb instead of Arc<AccountsDb>, and then we can shift the repeated Arc::new() inside Accounts::new()?

Also, @apfitzge had a clever suggestion on one of my PR's the other day that might be applicable here if we want to accept Arc<AccountsDb> or AccountsDb: #34449 (comment)

[brooksprumo]

Wondering if we should make Accounts::new() take an AccountsDb instead of Arc<AccountsDb>, and then we can shift the repeated Arc::new() inside Accounts::new()?

Yeah, I'd prefer if Accounts::new() took AccountsDb instead of Arc<AccountsDb>. But, Bank::_new_from_parent() clones the AccountsDb into the new bank's Accounts, so there must be a ctor that takes an Arc<AccountsDb>.

solana/runtime/src/bank.rs

Line 1274 in aaccbdd

accounts: Arc::new(Accounts::new(accounts_db)),

We could keep a second ctor around, but I'm really trying to reduce ctors where possible.

Also, @apfitzge had a clever suggestion on one of my PR's the other day that might be applicable here if we want to accept Arc<AccountsDb> or AccountsDb: #34449 (comment)

Clever, indeed! Since Arc already has implements From<T> for Arc<T>, this allows the caller to use .into() already. This still requires the caller to do something special. I understand the Into<Arc<AccountsDb>> as a function parameter would remove the burden from the caller, which may be nice. I'm on the fence. Want me to do the Into way? I'll do basically anything to avoid adding more constructors heh.

[steviez]

I'll do basically anything to avoid adding more constructors heh.

😆 😆 😆

I'm a little on the fence as well ha and don't have too strong of an option; I just saw the repeated code so figured I'd mention it.

That being said, I think this PR is still valuable as-is; Arc::new() is pretty simple and having some of these repeated is probably better than someone having to choose from a handful of constructors. So, I think we could defer a decision to another PR and ship this one as-is

[brooksprumo]

I think I've come around to the Into<> idea. Here's a PR for it: #34476.

(I'm going to merge this PR as-is, since it feels like a complete thought, and the Into<> one is a different thought.)