This repository has been archived by the owner on Jan 22, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
ledger-tool: Condense ProcessOptions parsing logic #34694
Merged
steviez
merged 8 commits into
solana-labs:master
from
steviez:lt_condense_process_options
Jan 11, 2024
Merged
ledger-tool: Condense ProcessOptions parsing logic #34694
steviez
merged 8 commits into
solana-labs:master
from
steviez:lt_condense_process_options
Jan 11, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #34694 +/- ##
=======================================
Coverage 81.8% 81.8%
=======================================
Files 823 823
Lines 222725 222725
=======================================
+ Hits 182300 182315 +15
+ Misses 40425 40410 -15 |
50fb836
to
8411b93
Compare
3 tasks
The helper is now sufficient for all of the args accounts, graph and capitalization subcommands use
8411b93
to
c235f7d
Compare
CriesofCarrots
approved these changes
Jan 11, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
CriesofCarrots
pushed a commit
to CriesofCarrots/solana
that referenced
this pull request
Jan 12, 2024
The code to parse process options was repeated in several (8) commands that replay block processing. So, move the argument parsing into a common helper that can be used by all of those commands.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
The code to parse
ProcessOptions
is repeated in several places. The repetition adds clutter and makes it easier to "miss a spot" if adding a new flag that should be available for all commands. Granted, there are a handful of flags that are not available on all commands, but I plan to tackle that to some extent in a follow-up PRSummary of Changes
Introduce a function that parses all of the
ProcessOptions
related arguments, and use that helper across all of the subcommands that process blocks.There are some changes to default behavior with this PR; however, I think the changes are justified:
run_verification
==> Default value changes fromfalse
totrue
for all commands that previously hadfalse
hard-coded. My opinion is that this is fine as we're making the replay more rigorous; if there is something wrong with PoH or a transaction signature, it seems likely that the resultingBank
could also be invalid so better to do the check. And as alluded to in above comment, I plan to later update how the args are added to each subcommand for the sake of better consistency across the commands; this would add the option to opt out of PoH / sigverify to these commandsuse_snapshot_archives_at_startup
==> Default value changes fromWhenNewest
toAlways
forbank-hash
andshred-version
commandsAlways
as the default; these two command just didn't get the default since they did not previously have the arg available as a subcommand. So, this PR adds the arg to these commands, which gets us consistency amongst the commands (in terms of available flags) while also getting us the correct default for freeI manually compared the default
ProcessOptions
for each command that processes, and confirmed these are the only differences / that this function does not error for each command that utilizes it