Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TieredStorage] Make TieredStorage's Offset compatible with AccountsDB's offset #74

Merged
merged 2 commits into from
Apr 1, 2024

Conversation

yhchiang-sol
Copy link

@yhchiang-sol yhchiang-sol commented Mar 4, 2024

Problem

AccountsDB currently assumes all offsets are multiple of 8, and this
applies to all AccountsFile API. However, TieredStorage uses its
IndexOffset that does not have this assumption.

Summary of Changes

This PR adds a conversion to make TieredStorage::IndexOffset
compatible with the AccountsDB offset inside the AccountsFile layer.

Test Plan

Tested by running a validator with TieredStorage manually enabled on mnb.
More accounts-db tests will be enabled with #334

@yhchiang-sol yhchiang-sol marked this pull request as draft March 11, 2024 17:00
@yhchiang-sol yhchiang-sol force-pushed the ts-pr-reduced-offset branch from 5a942df to 1590a84 Compare March 11, 2024 23:59
@yhchiang-sol yhchiang-sol changed the title Ts pr reduced offset [TieredStorage] Make TieredStorage's Offset compatible with AccountsDB's offset Mar 12, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2024

Codecov Report

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

Project coverage is 81.9%. Comparing base (c867522) to head (2aa876c).
Report is 44 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #74   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         840      840           
  Lines      228098   228110   +12     
=======================================
+ Hits       186818   186858   +40     
+ Misses      41280    41252   -28     

@yhchiang-sol yhchiang-sol force-pushed the ts-pr-reduced-offset branch from 1590a84 to 51b0481 Compare March 15, 2024 17:36
@yhchiang-sol yhchiang-sol force-pushed the ts-pr-reduced-offset branch 2 times, most recently from 6b9e576 to 5f5515e Compare March 26, 2024 02:00
@yhchiang-sol yhchiang-sol force-pushed the ts-pr-reduced-offset branch from 5f5515e to 97a8ffd Compare March 26, 2024 02:09
@yhchiang-sol yhchiang-sol marked this pull request as ready for review March 26, 2024 06:01
Copy link

@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.

Hmm, we may need to make the index not divide by 8 automatically, and instead only do it for append vecs. I'm not sure how this will interact with forwards/backwards compatibility though.

Or maybe the StorageLocation gets a new variant? That may impact many other things too.

@brooksprumo
Copy link

@jeffwashington Adding you here as a reviewer. What do you think is a good way forward here, w.r.t. storing different offsets in the index?

@yhchiang-sol
Copy link
Author

Hmm, we may need to make the index not divide by 8 automatically, and instead only do it for append vecs. I'm not sure how this will interact with forwards/backwards compatibility though.

Yep, I think this isn't the best way but offers an intermediate step to make tiered-storage runnable without changing the accounts-db + append-vec code path/logic. Ideally, we want to have accounts-db + append-vec logic untouched until we make tiered-storage default / proven to be stable.

@jeffwashington
Copy link

jeffwashington commented Mar 29, 2024

Hmm, we may need to make the index not divide by 8 automatically, and instead only do it for append vecs. I'm not sure how this will interact with forwards/backwards compatibility though.

Yep, I think this isn't the best way but offers an intermediate step to make tiered-storage runnable without changing the accounts-db + append-vec code path/logic. Ideally, we want to have accounts-db + append-vec logic untouched until we make tiered-storage default / proven to be stable.

this seems like it may be ok. I'm trying to figure out why the validators and ledger-tool were working without this change. I guess the indexes are *8 already somewhere else? I know I have hacked this in in a few places in my test branches. Maybe that's what I'm thinking of.

update: a version of this pr was in the prototype branch I was running:
4fbts-test-all

@jeffwashington
Copy link

this pr does appear consistent with the one I was running 4fbts-test-all. I think it is good to go.

Copy link

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm

@jeffwashington
Copy link

Hmm, we may need to make the index not divide by 8 automatically, and instead only do it for append vecs. I'm not sure how this will interact with forwards/backwards compatibility though.

Or maybe the StorageLocation gets a new variant? That may impact many other things too.

the index is using those extra bits for flags (like zero lamports), knowing that indexes were 8-byte aligned. This would be tricky to change. It does place an upper limit on offsets. This is not a problem at all for a 0,1,2 index. It is a limiting factor already for append vec offsets (where entries have a min size of 136 bytes or so). I thing this implementation is reasonable.

@brooksprumo
Copy link

the index is using those extra bits for flags (like zero lamports), knowing that indexes were 8-byte aligned. This would be tricky to change. It does place an upper limit on offsets. This is not a problem at all for a 0,1,2 index. It is a limiting factor already for append vec offsets (where entries have a min size of 136 bytes or so). I thing this implementation is reasonable.

The Hot Storage format has a max of 29 bits for the owner's offset. So this somewhat conservatively sets a maximum number of accounts per hot storage to 29 bits as well. So if we say there's a max of 29 bits for number of accounts, the hot storage will be compatible with the index's account info too, I believe.

Copy link

@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:

We can fixup/refactor the offsets later.

@jeffwashington jeffwashington merged commit b47a4ec into anza-xyz:master Apr 1, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants