[TieredStorage] Write owners block for HotAccountStorage

[yhchiang-sol]

Problem

So far the current HotStorageWriter::write_accounts() only writes
accounts blocks and index block.

Summary of Changes

The PR further writes owners block in HotStorageWriter::write_accounts().

Test Plan

Extended existing test for HotStorageWriter to cover the owners block.

[yhchiang-sol]

FYI: This function isn't used, but is required by the CI check as we have a len() function:

error: struct `OwnersTable` has a public `len` method, but no `is_empty` method
--
  | --> accounts-db/src/tiered_storage/owners.rs:102:5
  | \|
  | 102 \|     pub fn len(&self) -> usize {
  | \|     ^^^^^^^^^^^^^^^^^^^^^^^^^^
  | \|
  | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#len_without_is_empty
  | = note: `-D clippy::len-without-is-empty` implied by `-D warnings`
  | = help: to override `-D warnings` add `#[allow(clippy::len_without_is_empty)]`

[codecov]

Codecov Report

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

Comparison is base (b04765f) 81.6% compared to head (8d02f02) 81.7%.
Report is 13 commits behind head on master.

❗ Current head 8d02f02 differs from pull request most recent head 20110b7. Consider uploading reports for the commit 20110b7 to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #34927    +/-   ##
========================================
  Coverage    81.6%    81.7%            
========================================
  Files         828      830     +2     
  Lines      224339   224672   +333     
========================================
+ Hits       183274   183559   +285     
- Misses      41065    41113    +48     

[yhchiang-sol]

Rebased and cleared the dependencies.

[brooksprumo]

Two small change requests, otherwise looks good.

[brooksprumo]

Ah ha! I was wondering about this the other day and how we wanted to handle it. This basically guarantees every hot storage file will have one additional owner that represents "no owner". We could also optimize this away by storing some flag, or known owner's offset to indicate this account has no owner, or always using the first owner offset, since the value itself does not matter.

For this PR, I think this impl is fine.

Please make this a constant though. (And maybe renamed? Up to you.)

        const DEFAULT_PUBKEY: Pubkey = Pubkey::default();

[yhchiang-sol]

I actually tried this first, but Rust complains that I cannot do this:

   Compiling solana-accounts-db v1.18.0 (/Users/keelar/solana/yhchiang-sol/accounts-db)
error[E0015]: cannot call non-const fn `<solana_sdk::pubkey::Pubkey as std::default::Default>::default` in constants
   --> accounts-db/src/tiered_storage/hot.rs:545:40
    |
545 |         const DEFAULT_PUBKEY: Pubkey = Pubkey::default();
    |                                        ^^^^^^^^^^^^^^^^^
    |
    = note: calls in constants are limited to constant functions, tuple structs and tuple variants

Let me try the lazy_init again (I think I might have tried that before as well, forgot what was complained).

[yhchiang-sol]

This basically guarantees every hot storage file will have one additional owner that represents "no owner". We could also optimize this away by storing some flag

I think one way to achieve this is to have one special value of OwnerOffset indicating no owner (probably OwnerOffset(u32::MAX) I think). This is probably a better way to handle it. Let me try to handle it.

[yhchiang-sol]

Damn. I need to pick some value other than u32::MAX because hot-storage stores owners offset using only 29 bits.

 struct HotMetaPackedFields {
     /// A hot account entry consists of the following elements:
     ///
     /// * HotAccountMeta
     /// * [u8] account data
     /// * 0-7 bytes padding
     /// * optional fields
     ///
     /// The following field records the number of padding bytes used
     /// in its hot account entry.
     padding: B3,
     /// The index to the owner of a hot account inside an AccountsFile.
     owner_offset: B29,
 }

In addition, a MAX value like this also becomes a special case in the boundary check. Will probably do it in a separate PR.

[yhchiang-sol]

Fixed. Use const OWNER_NO_OWNER defined in owners.rs while keeping the OwnerOffset logic the same. We can continue the discussion and I will create a separate PR for how to avoid saving extra owner per file.

[brooksprumo]

I actually tried this first, but Rust complains that I cannot do this

Ah right, duh, my bad.

Damn. I need to pick some value other than u32::MAX because hot-storage stores owners offset using only 29 bits.

We can steal another bit from this struct. The owner offset doesn't need 29 bits, because that would exceed the file size limits. Is that the best use of this bit? I dunno! But it's an option, and we could change later if needed.

e.g.
16 GiB file size limit
if an account is only pubkey + owner + lamports, that's 32 + 32 + 8 = 72 bytes
16 GiB / 72 bytes per account = 238,609,294 accounts
and 2^28 = 268,435,456

So 28 bits can hold all the owners in the worst possible case (which practically cannot happen)

[yhchiang-sol]

My thought on this is that: I am not sure whether we might change the bits later when we already decided a const limit (for instance, 28 should be more than enough, but I am not sure whether we will squeeze it further for other purpose and change the limit to 27 bits).

If we want a const to represent the owner offset for no owner that we will never change, then 0 could be a good one? But as a downside, we also need to properly do -1 when reading any entry.

[brooksprumo]

I think the current impl with the default Pubkey representing 'no owner' is fine. Note that the system program's address is the default pubkey, so we'll likely never be storing an unnecessary owner in the owners block.

[brooksprumo]

lgtm

[brooksprumo]

nit: Please prefer "rent paying" instead of "non rent exempt". (1) this is the terminology used elsewhere, and (2) then avoids a double-negative.

(I know this is only a comment in a test, but hopefully this also informs any future code as well.)

Also note, this can be addressed in a subsequent PR.

[yhchiang-sol]

Good suggestion! Will do it in a separate PR.

[yhchiang-sol]

Have piggybacked in #34969.

[yhchiang-sol]

Merging the PR as the timeout on local-cluster test doesn't look related.