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

validator: Move skip_poh_verify into the deprecated list #30885

Merged
merged 1 commit into from
Mar 27, 2023
Merged

validator: Move skip_poh_verify into the deprecated list #30885

merged 1 commit into from
Mar 27, 2023

Conversation

ilya-bobyr
Copy link
Contributor

@ilya-bobyr ilya-bobyr commented Mar 24, 2023

Follow-up for

  commit 809041b1519ebaa54d6ffe4ce194f04feb8eda0f
  Author: Illia Bobyr <[email protected]>
  Date:   Wed Mar 22 11:03:30 2023 -0700

      poh_verify => run_verification: Rename to be more accurate (#30811)

      `poh_verify` actually disables transaction signature, tick count and
      built in program argument verifications as well.  It is somewhat
      confusing to call it `poh_verify`.

@steviez
Copy link
Contributor

steviez commented Mar 24, 2023

Thanks for jumping on this so quick. As for the help message of the now deprecated arg, you can leave it as is. Instead, add it to this list:

static ref DEPRECATED_ARGS_AND_HELP: Vec<(&'static str, &'static str)> = vec![

which will get iterated through and spit out uniform messages about deprecated args:

solana/validator/src/cli.rs

Lines 1718 to 1727 in 24be850

pub fn warn_for_deprecated_arguments(matches: &ArgMatches) {
for (arg, help) in DEPRECATED_ARGS_AND_HELP.iter() {
if matches.is_present(arg) {
warn!(
"{}",
format!("--{arg} is deprecated. {help}").replace('_', "-")
);
}
}
}

which means you can also drop this:

solana/validator/src/main.rs

Lines 1194 to 1196 in 24be850

if matches.is_present("skip_poh_verify") {
eprintln!("--skip-poh-verify is deprecated. Replace with --skip-verification.");
}

@ilya-bobyr
Copy link
Contributor Author

Thanks for jumping on this so quick. As for the help message of the now deprecated arg, you can leave it as is. Instead, add it to this list:

static ref DEPRECATED_ARGS_AND_HELP: Vec<(&'static str, &'static str)> = vec![

which will get iterated through and spit out uniform messages about deprecated args:

solana/validator/src/cli.rs

Lines 1718 to 1727 in 24be850

pub fn warn_for_deprecated_arguments(matches: &ArgMatches) {
for (arg, help) in DEPRECATED_ARGS_AND_HELP.iter() {
if matches.is_present(arg) {
warn!(
"{}",
format!("--{arg} is deprecated. {help}").replace('_', "-")
);
}
}
}

which means you can also drop this:

solana/validator/src/main.rs

Lines 1194 to 1196 in 24be850

if matches.is_present("skip_poh_verify") {
eprintln!("--skip-poh-verify is deprecated. Replace with --skip-verification.");
}

I did a quick search, based on the get_deprecated_arguments(). Did not realize it was done via a separate list. Thank you for pointing it out.

Done.

Seems like accounts_db_skip_shrink is also missing from the DEPRECATED_ARGS_AND_HELP list.

Should I send a PR to add it?

@ilya-bobyr ilya-bobyr requested a review from steviez March 24, 2023 23:37
@steviez
Copy link
Contributor

steviez commented Mar 27, 2023

I did a quick search, based on the get_deprecated_arguments(). Did not realize it was done via a separate list. Thank you for pointing it out.

No problem!

Seems like accounts_db_skip_shrink is also missing from the DEPRECATED_ARGS_AND_HELP list.
Should I send a PR to add it?

Yeah, good catch. Given that you're not the first person to miss this, might be worth a quick look to try to combine the two lists down to a single point. Don't have a great idea at the moment, but given that they're single use at startup, I think we can live with something that isn't the most elegant if it better helps us (engineers) to get both items updated (or single source somehow)

Also, code looks good to go, but CI is failing ... looks like you need a rebase on top of: #30897

Follow-up for

  commit 809041b
  Author: Illia Bobyr <[email protected]>
  Date:   Wed Mar 22 11:03:30 2023 -0700

      poh_verify => run_verification: Rename to be more accurate (#30811)

      `poh_verify` actually disables transaction signature, tick count and
      built in program argument verifications as well.  It is somewhat
      confusing to call it `poh_verify`.
@ilya-bobyr ilya-bobyr merged commit 301a944 into solana-labs:master Mar 27, 2023
@ilya-bobyr ilya-bobyr deleted the pr/validator-deprecate-arg-skip_poh_verify branch March 27, 2023 23:12
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.

2 participants