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

Bugfix: Account Shrink Paths must conform to account directory structure #32088

Merged

Conversation

apfitzge
Copy link
Contributor

Problem

Validator crashes if you specify account shrink paths. Discord context: https://discord.com/channels/428295358100013066/670512312339398668/1117869012576129074

#29496 introduced hardlinking for account storages, which created an expected account directory structure. Account directories are expected to have a structure:

account_path
   |
    -----run/
   |
    -----snapshot/

appendvecs in directories that do not follow this structure will cause get_account_path_from_appendvec_path to return an InvalidAppendVecPath.

The validator ensures account paths follow this structure, but does not do so for account shrink paths.

Summary of Changes

Ensure correct account directory structure for account shrink paths.

Testing

I was able to replicate the bug by running a local node w/ account shrink paths. On the snapshot after initial shrinking you see an panic:

thread 'solBgAccounts' panicked at 'snapshot bank: InvalidAppendVecPath("/home/apfitzge/dev/solana.worktrees/one-offs/config/bootstrap-validator/fking-shrink/82.128")', runtime/src/accounts_background_service.rs:396:18

Re-ran with the changes in this PR, and saw the snapshot generated w/o crashing and accounts in the <account-shrink-path>/run/ and <account-shrink-path>/snapshot/X/ paths.

Fixes #

@apfitzge apfitzge added the v1.16 PRs that should be backported to v1.16 label Jun 12, 2023
@apfitzge
Copy link
Contributor Author

Propsing that the fix in this PR is backported (after review), as users are running into the issue on v1.16.0.

@apfitzge apfitzge requested a review from brooksprumo June 12, 2023 21:49
@apfitzge
Copy link
Contributor Author

Out of the scope of this PR, but given all the issues we've had with this enforced directory structure, I've proposed to @brooksprumo we re-evaluate and potentially try to use the type-system to ensure we're checking the directories correctly.

However, as this fix is intended for backporting to v16, any of those more significant changes are not included in this PR.

@brooksprumo
Copy link
Contributor

Would you mind adding this to the v1.16.0 Known Issues here? https://github.com/solana-labs/solana/releases/tag/v1.16.0

validator/src/main.rs Outdated Show resolved Hide resolved
validator/src/main.rs Outdated Show resolved Hide resolved
@apfitzge
Copy link
Contributor Author

Would you mind adding this to the v1.16.0 Known Issues here? v1.16.0 (release)

Added a workaround & link to this PR. Didn't know about these notes, will use in the future

Copy link
Contributor

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

lgtm

@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Merging #32088 (1e32631) into master (7e7c286) will increase coverage by 0.0%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #32088   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         765      765           
  Lines      208497   208497           
=======================================
+ Hits       170861   170906   +45     
+ Misses      37636    37591   -45     

@apfitzge apfitzge merged commit de92761 into solana-labs:master Jun 12, 2023
@apfitzge apfitzge deleted the bugfix/shrink_paths_not_conforming branch June 12, 2023 23:42
mergify bot pushed a commit that referenced this pull request Jun 12, 2023
mergify bot added a commit that referenced this pull request Jun 13, 2023
… structure (backport of #32088) (#32090)

Bugfix: Account Shrink Paths must conform to account directory structure (#32088)

(cherry picked from commit de92761)

Co-authored-by: Andrew Fitzgerald <[email protected]>
jeffwashington pushed a commit to HaoranYi/solana that referenced this pull request Jun 13, 2023
wen-coding pushed a commit to wen-coding/solana that referenced this pull request Aug 15, 2023
wen-coding pushed a commit to wen-coding/solana that referenced this pull request Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.16 PRs that should be backported to v1.16
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants