Skip to content

[TieredStorage] Deprecate the use of account-hash in HotStorage#93

Merged
yhchiang-sol merged 2 commits intoanza-xyz:masterfrom
yhchiang-sol:ts-rm-acc-hash
Mar 7, 2024
Merged

[TieredStorage] Deprecate the use of account-hash in HotStorage#93
yhchiang-sol merged 2 commits intoanza-xyz:masterfrom
yhchiang-sol:ts-rm-acc-hash

Conversation

@yhchiang-sol
Copy link
Copy Markdown

Problem

TieredStorage stores account hash as an optional field inside its HotStorage.
However, the field isn't used and we have already decided to deprecate
the account hash.

Summary of Changes

Remove account-hash from the tiered-storage.

Test Plan

Existing tiered-storage tests.
Running validators w/ tiered-storage in mainnet-beta w/o storing account-hash.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.8%. Comparing base (3f9a7a5) to head (3229e60).
Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master      #93     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         837      838      +1     
  Lines      225922   225871     -51     
=========================================
- Hits       184955   184868     -87     
- Misses      40967    41003     +36     

@yhchiang-sol yhchiang-sol requested a review from brooksprumo March 6, 2024 20:19
/// Note that the storage representation of the optional fields might be
/// different from its in-memory representation.
#[derive(Debug, PartialEq, Eq, Clone)]
pub struct AccountMetaOptionalFields<'a> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the lifetime is a nice bonus!

Comment thread accounts-db/src/tiered_storage/readable.rs Outdated
@yhchiang-sol yhchiang-sol requested a review from brooksprumo March 6, 2024 23:39
Copy link
Copy Markdown

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@yhchiang-sol yhchiang-sol merged commit 0bf9ec8 into anza-xyz:master Mar 7, 2024
@jeffwashington
Copy link
Copy Markdown

For compressed state, we need to store the hash per account. We also likely need to store a heterogenous mixture of full accounts or hash.
It seems it is costing us 1 bit per account to be able to store an account's hash.
My inclination is that this pr should be reverted. Thoughts?

@yhchiang-sol
Copy link
Copy Markdown
Author

For compressed state, we need to store the hash per account. We also likely need to store a heterogenous mixture of full accounts or hash.
It seems it is costing us 1 bit per account to be able to store an account's hash.
My inclination is that this pr should be reverted. Thoughts?

If we want to use the minimum storage size to handle state-compressed accounts, then having a dedicated block for all the state-compressed accounts would probably lead to the best result. The resulting tiered-storage file would look something like this:

  • accounts blocks
  • index block
  • owners block
  • state compressed block
  • filter block (e.g. a bloomfilter to determine whether an account might exist in this file)
  • footer

A state-compressed block can be simply:

<pubkey-1><hash-1>
<pubkey-2><hash-2>
...

codebender828 pushed a commit to codebender828/agave that referenced this pull request Oct 3, 2024
…-xyz#93)

#### Problem
TieredStorage stores account hash as an optional field inside its HotStorage.
However, the field isn't used and we have already decided to deprecate
the account hash.

#### Summary of Changes
Remove account-hash from the tiered-storage.

#### Test Plan
Existing tiered-storage tests.
Running validators w/ tiered-storage in mainnet-beta w/o storing account-hash.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants