Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

ledger-tool: Condense ProcessOptions parsing logic #34694

Merged
merged 8 commits into from
Jan 11, 2024

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Jan 8, 2024

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 PR

Summary 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 from false to true for all commands that previously had false 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 resulting Bank 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 commands
  • use_snapshot_archives_at_startup ==> Default value changes from WhenNewest to Always for bank-hash and shred-version commands
    • ledger-tool does *not* fastboot by default #34228 previously established that we want Always 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 free

I 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

@steviez steviez changed the title Lt condense process options ledger-tool: Condense ProcessOptions parsing logic Jan 8, 2024
Copy link

codecov bot commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7ee9d9a) 81.8% compared to head (c235f7d) 81.8%.
Report is 1 commits behind head on master.

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     

@steviez steviez force-pushed the lt_condense_process_options branch 2 times, most recently from 50fb836 to 8411b93 Compare January 10, 2024 19:08
@steviez steviez force-pushed the lt_condense_process_options branch from 8411b93 to c235f7d Compare January 10, 2024 22:03
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

lgtm!

@steviez steviez merged commit a203f14 into solana-labs:master Jan 11, 2024
20 checks passed
@steviez steviez deleted the lt_condense_process_options branch January 11, 2024 19:24
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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants