[TieredStorage] rent_epoch() returns 0 for zero-lamport accounts #35344

yhchiang-sol · 2024-02-28T08:02:29Z

Problem

In TieredAccountMeta, RENT_EXEMPT_RENT_EPOCH will be used when
its optional field rent_epoch is None. However, for legacy reasons, 0
should be used for zero-lamport accounts.

Summary of Changes

Return 0 for TieredAccountMeta::rent_epoch() for zero-lamport accounts.

Test Plan

accounts_db::tests::test_clean_zero_lamport_and_dead_slot

codecov · 2024-02-28T09:24:15Z

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 81.7%. Comparing base (6ee3bb9) to head (f45aafa).
Report is 38 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #35344   +/-   ##
=======================================
  Coverage    81.7%    81.7%           
=======================================
  Files         834      834           
  Lines      224299   224302    +3     
=======================================
+ Hits       183361   183402   +41     
+ Misses      40938    40900   -38

accounts-db/src/tiered_storage/readable.rs

zero-lamport account.

accounts-db/src/tiered_storage/readable.rs

…labs#33964) [TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033) [TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031) [TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047) [TieredStorage] HotStorageReader::get_account_address (solana-labs#34032) [TieredStorage] OwnersBlock (solana-labs#34052) [TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053) [TieredStorage] Define OwnerOffset as u32 (solana-labs#34105) [TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106) Refactors TieredStorageFile read/write methods (solana-labs#34147) [TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133) [TieredStorage] Make IndexOffset use u32 (solana-labs#34152) Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187) [TieredStorage] Make AccountOffset use u32 (solana-labs#34151) [TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155) [TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335) [TieredStorage] Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404) [TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376) [TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349) [TieredStorage] boundary check for get_account_address() (solana-labs#34529) Sanitizes tiered storage footer after reading from disk (solana-labs#34200) Adds read/write/get_pod() fns to tiered storage (solana-labs#34415) Uses consistent error types in tiered storage (solana-labs#34110) [TieredStorage] Boundary check for get_account_offset() (solana-labs#34531) [TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350) [TieredStorage] Fix typos in index.rs (solana-labs#34546) [TieredStorage] HotAccountsReader::get_account (solana-labs#34499) [TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658) [TieredStorage] HotStorageWriter::new() (solana-labs#34659) [TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724) [TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854) [TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853) [TieredStorage] writing hot account blocks and index blocks (solana-labs#34828) [TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950) [TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927) [TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969) [TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967) [TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968) [TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929) [TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046) [TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031) [TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049) [TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065) [TieredStorage] Make TieredStorage::write_accounts() thread-safe (solana-labs#35143) [TieredStorage] rent_epoch() returns 0 for zero-lamport accounts (solana-labs#35344)

…ana-labs#35344) In TieredAccountMeta, RENT_EXEMPT_RENT_EPOCH will be used when its optional field rent_epoch is None. However, for legacy reasons, 0 should be used for zero-lamport accounts. Return 0 for TieredAccountMeta::rent_epoch() for zero-lamport accounts. accounts_db::tests::test_clean_zero_lamport_and_dead_slot

jeffwashington · 2024-03-08T18:35:13Z

accounts-db/src/tiered_storage/readable.rs

+                // match the default states of AccountSharedData.  Otherwise,
+                // a hash mismatch will occur.
+                Epoch::default()
+            })
    }


@yhchiang-sol are we going to also change what gets written:
https://github.com/jeffwashington/solana/blob/ef6c506ab673c570220f39ba2830cd3ce942a40d/accounts-db/src/tiered_storage/hot.rs#L593

writing a 0 lamport account would currently store the rent_epoch as a 0 u64

writing a 0 lamport account would currently store the rent_epoch as a 0 u64

It shouldn't be the case because zero lamport account will be None:
https://github.com/jeffwashington/solana/blob/ef6c506ab673c570220f39ba2830cd3ce942a40d/accounts-db/src/account_storage/meta.rs#L81

And, for None case, we will write None for the rent_epoch
https://github.com/jeffwashington/solana/blob/ef6c506ab673c570220f39ba2830cd3ce942a40d/accounts-db/src/tiered_storage/hot.rs#L597

jeffwashington · 2024-03-08T18:37:09Z

accounts_db::tests::test_clean_zero_lamport_and_dead_slot

how will this test test this change in hot storage?
Does that test fail when hot storage is the default? And now it passes? Do we have any tests that are testing hot storage now? What is our plan there?

…labs#33964) [TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033) [TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031) [TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047) [TieredStorage] HotStorageReader::get_account_address (solana-labs#34032) [TieredStorage] OwnersBlock (solana-labs#34052) [TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053) [TieredStorage] Define OwnerOffset as u32 (solana-labs#34105) [TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106) Refactors TieredStorageFile read/write methods (solana-labs#34147) [TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133) [TieredStorage] Make IndexOffset use u32 (solana-labs#34152) Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187) [TieredStorage] Make AccountOffset use u32 (solana-labs#34151) [TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155) [TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335) [TieredStorage] Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404) [TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376) [TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349) [TieredStorage] boundary check for get_account_address() (solana-labs#34529) Sanitizes tiered storage footer after reading from disk (solana-labs#34200) Adds read/write/get_pod() fns to tiered storage (solana-labs#34415) Uses consistent error types in tiered storage (solana-labs#34110) [TieredStorage] Boundary check for get_account_offset() (solana-labs#34531) [TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350) [TieredStorage] Fix typos in index.rs (solana-labs#34546) [TieredStorage] HotAccountsReader::get_account (solana-labs#34499) [TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658) [TieredStorage] HotStorageWriter::new() (solana-labs#34659) [TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724) [TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854) [TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853) [TieredStorage] writing hot account blocks and index blocks (solana-labs#34828) [TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950) [TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927) [TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969) [TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967) [TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968) [TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929) [TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046) [TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031) [TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049) [TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065) [TieredStorage] Make TieredStorage::write_accounts() thread-safe (solana-labs#35143) [TieredStorage] rent_epoch() returns 0 for zero-lamport accounts (solana-labs#35344) [TieredStorage] Deprecate the use of account-hash in HotStorage (solana-labs#93)

yhchiang-sol · 2024-03-10T03:15:25Z

how will this test test this change in hot storage?

Was running the test with local PRs that make hot-storage default.

Does that test fail when hot storage is the default? And now it passes?

Yes

Do we have any tests that are testing hot storage now? What is our plan there?

Currently, we have tests inside tiered-storage directory and tiered_storage.rs.

For integration tests in the accounts-db level, the current plan is to extend existing tests in the accounts-db directory to run both AppendVec and tiered-storage.

[TieredStorage] Return rent_epoch() 0 for zero-lamport accounts

7689b7f

yhchiang-sol requested a review from brooksprumo February 28, 2024 20:18

brooksprumo reviewed Feb 28, 2024

View reviewed changes

accounts-db/src/tiered_storage/readable.rs Show resolved Hide resolved

yhchiang-sol requested a review from brooksprumo March 1, 2024 18:45

Improve code comment to explain why Epoch::Default() is used for

f45aafa

zero-lamport account.

brooksprumo approved these changes Mar 1, 2024

View reviewed changes

accounts-db/src/tiered_storage/readable.rs Show resolved Hide resolved

yhchiang-sol merged commit 608329b into solana-labs:master Mar 1, 2024
35 checks passed

jeffwashington reviewed Mar 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TieredStorage] rent_epoch() returns 0 for zero-lamport accounts #35344

[TieredStorage] rent_epoch() returns 0 for zero-lamport accounts #35344

yhchiang-sol commented Feb 28, 2024

codecov bot commented Feb 28, 2024 •

edited

Loading

jeffwashington Mar 8, 2024

jeffwashington Mar 8, 2024

yhchiang-sol Mar 10, 2024

jeffwashington commented Mar 8, 2024

yhchiang-sol commented Mar 10, 2024

[TieredStorage] rent_epoch() returns 0 for zero-lamport accounts #35344

[TieredStorage] rent_epoch() returns 0 for zero-lamport accounts #35344

Conversation

yhchiang-sol commented Feb 28, 2024

Problem

Summary of Changes

Test Plan

codecov bot commented Feb 28, 2024 • edited Loading

Codecov Report

jeffwashington Mar 8, 2024

Choose a reason for hiding this comment

jeffwashington Mar 8, 2024

Choose a reason for hiding this comment

yhchiang-sol Mar 10, 2024

Choose a reason for hiding this comment

jeffwashington commented Mar 8, 2024

yhchiang-sol commented Mar 10, 2024

codecov bot commented Feb 28, 2024 •

edited

Loading