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

check whether accounts and snapshots are using the same path #1853

Merged
merged 4 commits into from
Jun 27, 2024

Conversation

yihau
Copy link
Member

@yihau yihau commented Jun 25, 2024

Problem

both accounts and snapshots have the same name subdirectory, snapshot. catch this situation earlier to prevent unexpected behavior.

Summary of Changes

  • ensure account_path and snapshot_path are unique
  • add test for it

@yihau yihau marked this pull request as ready for review June 25, 2024 06:47
@yihau yihau requested review from brooksprumo and steviez June 25, 2024 06:47
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.

"Hilariously", the "snapshots" subdirectory is not standard...

--snapshots will create a "snapshots" subdirectory
--accounts will create a "snapshot" subdirectory

note the missing S above!

But, when using ledger tool, the subdirectory for snapshots under the ledger path will be "snapshot" (no S)!

I think we'll want to fixup the ledger-tool stuff. And all of this pain caused by using string literals instead of constants...

I do think enforcing unique-ness here is a good idea. The snapshot-vs-snapshots is confusing, and could cause issues when running ledger-tool.

validator/src/main.rs Outdated Show resolved Hide resolved
validator/src/main.rs Outdated Show resolved Hide resolved
Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

I think we'll want to fixup the ledger-tool stuff. And all of this pain caused by using string literals instead of constants...

Yeah, I'm down for making ledger-tool consistent with validator.

Also, I think we should potentially expand further and check for uniqueness of other specifiable paths. A few weeks ago, I hit an issue because I accidentally specified several items to be at the same path (--ledger) instead of specifying unique sub-directories for each.

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.

The changes look good to me, thanks! Some tiny nits/requests:

validator/src/main.rs Show resolved Hide resolved
validator/src/main.rs Outdated Show resolved Hide resolved
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:

@yihau yihau merged commit 7117ed9 into anza-xyz:master Jun 27, 2024
51 checks passed
@yihau yihau deleted the validator-path-verify branch June 27, 2024 08:49
samkim-crypto pushed a commit to samkim-crypto/agave that referenced this pull request Jul 31, 2024
…z#1853)

* check whether accounts and snapshots are using the same path

* patch and update test

* remove new line

* align
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.

3 participants