[TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo>

[yhchiang-sol]

Problem

To allow hot-storage to use HotStorageWriter::write_account() to
implement AccountsFile::append_accounts(), it is required to
provide a Vector of StoredAccountInfo to allow AccountsDB to
properly prepare the entry for each account.

Summary of Changes

This PR enables HotStorageWriter::write_account() to return
Vec.

Test Plan

Extend existing tests for HotStorageWriter to verify the correctness
of the returned Vec.

[codecov]

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (5da06c5) 81.6% compared to head (b4e3acf) 81.6%.
Report is 33 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #34929    +/-   ##
========================================
  Coverage    81.6%    81.6%            
========================================
  Files         830      830            
  Lines      224746   224891   +145     
========================================
+ Hits       183512   183707   +195     
+ Misses      41234    41184    -50     

[brooksprumo]

Ah... very interesting...

We may want to tweak this API. It currently assumes that the total appended size is the same as summing up all the accounts appended. For Tiered Storage that's not the case.

Since we know all the accounts, and once we write then there will be no other writes, it means we know the total/final/complete size of the storage file. It'd probably be best to have this function return a tuple: a list of the stored account offsets, and a total size stored.

That would still work with AppendVec, and then would correctly set the size for Tiered Storage.

Since the size of the file in AccountStorageEntry::alive_bytes is used by shrink, I think we want to ensure we accurately compute the size here.

[yhchiang-sol]

We may want to tweak this API. It currently assumes that the total appended size is the same as summing up all the accounts appended. For Tiered Storage that's not the case.

Yep, I think there should exist a better API. But if I remember it correctly, an estimation won't make accounts-db panic based on my previous run joining pop-net and main-net a while back.

It'd probably be best to have this function return a tuple: a list of the stored account offsets, and a total size stored.

I think the size of each entry is used to estimate the saving of a shrink when selecting which file / account to shrink in accounts-db. So I think it does want the information for each individual account, but no need to be super accurate.

Since the size of the file in AccountStorageEntry::alive_bytes is used by shrink, I think we want to ensure we accurately compute the size here.

If I remember it correctly, accounts-db won't panic when hot-storage only provide an estimation based on my previous run on mainnet / popnet with the hot storage (a while back).

[brooksprumo]

I think the size of each entry is used to estimate the saving of a shrink when selecting which file / account to shrink in accounts-db. So I think it does want the information for each individual account, but no need to be super accurate.

I think we should fix the API first, and then return the correct size here. IIRC the current append vec code uses the size when shrinking/reclaiming and creating new append vecs, and I believe there are new assumptions about having zero extra size in the append vec files. This was not always the case.

Option (2) would be to use this PR as-is, and create a GH issue to address fixing the API and updating this function.

[yhchiang-sol]

Sure. Let's do option 2. Will create an issue as a follow-up.

[yhchiang-sol]

Just double-checked the accounts_db code.

When removing an account in AccountStorageEntry, it will maintain the alive_bytes by subtracting the stored_size. In that case, I think it's safer to not include the owner here in TieredStorage otherwise the alive_bytes will go negative.

Will update the PR to use an estimate that will no larger than the actual stored size.

[yhchiang-sol]

Updated the PR to consider only stored size directly contributed by the account (i.e., mainly account meta, data, and address).

[yhchiang-sol]

As for the API, the stored size of each account is still used when computing the estimated storage size saving when shrinking a specific accounts file (or append-vec). So I think the API should be good for now.

[yhchiang-sol]

Issue created: #35041

[yhchiang-sol]

Just undo one change that should belong to a different PR via git push -f. Hope it doesn't clear your review history.

[brooksprumo]

lgtm

[brooksprumo]

❤️