Remove duplicate SnapshotConfig validation code

[steviez]

Summary of Changes

Use the is_snapshot_config_valid() helper function instead of repeated logic.

[steviez]

For consistency between solana-validator and local-cluster tests (or really any other callers), we could move the checks these assertions perform into is_snapshot_config() valid. Currently, checking that these values are nonzero is done in the CLI parsing / logic in validator/src/main.rs.

I don't feel too strongly on that point - I care more about ripping out the duplicate logic that we get with is_snapshot_config_valid()

[codecov]

Codecov Report

Merging #32290 (ed4c902) into master (6f72258) will increase coverage by 0.0%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #32290   +/-   ##
=======================================
  Coverage    82.0%    82.0%           
=======================================
  Files         772      772           
  Lines      209480   209480           
=======================================
+ Hits       171863   171889   +26     
+ Misses      37617    37591   -26     

[apfitzge]

Change looks good to me. It appears that is_snapshot_config_valid has more strict logic that what was here before, but that's probably reasonable.

A couple of thoughts:

1. is_snapshot_config_valid uses Slot::MAX instead of DISABLED_SNAPSHOT_ARCHIVE_INTERVAL
2. why is is_snapshot_config_valid in validator instead of snapshot_config?

[steviez]

1. is_snapshot_config_valid uses Slot::MAX instead of DISABLED_SNAPSHOT_ARCHIVE_INTERVAL

Are you potentially looking at your local branch that isn't at tip? here is the tip:

solana/core/src/validator.rs

Lines 2259 to 2284 in 13aff74

	pub fn is_snapshot_config_valid(
	snapshot_config: &SnapshotConfig,
	accounts_hash_interval_slots: Slot,
	) -> bool {
	// if the snapshot config is configured to *not* take snapshots, then it is valid
	if !snapshot_config.should_generate_snapshots() {
	return true;
	}
	let full_snapshot_interval_slots = snapshot_config.full_snapshot_archive_interval_slots;
	let incremental_snapshot_interval_slots =
	snapshot_config.incremental_snapshot_archive_interval_slots;
	let is_incremental_config_valid =
	if incremental_snapshot_interval_slots == DISABLED_SNAPSHOT_ARCHIVE_INTERVAL {
	true
	} else {
	incremental_snapshot_interval_slots >= accounts_hash_interval_slots
	&& incremental_snapshot_interval_slots % accounts_hash_interval_slots == 0
	&& full_snapshot_interval_slots > incremental_snapshot_interval_slots
	};
	full_snapshot_interval_slots >= accounts_hash_interval_slots
	&& full_snapshot_interval_slots % accounts_hash_interval_slots == 0
	&& is_incremental_config_valid
	}

2. why is is_snapshot_config_valid in validator instead of snapshot_config?

Good point - could shift it to a method on SnapshotConfig

[apfitzge]

Are you potentially looking at your local branch that isn't at tip? here is the tip:

🤯 woops. Yeah you're right.

[brooksprumo]

2. why is is_snapshot_config_valid in validator instead of snapshot_config?

Historically the only snapshot config validation was done in validator, then slowly added into other places. Also, IIRC, there was divergence on what was valid, depending on validator vs local-cluster tests/etc.

Probably fine to make this a method on SnapshotConfig. And probably fine to do that in a subsequent PR :)

[steviez]

Historically the only snapshot config validation was done in validator, then slowly added into other places. Also, IIRC, there was divergence on what was valid, depending on validator vs local-cluster tests/etc.

Got it, thanks for the context. I think it is god to have the validation aligned; if there is a strong reason for something in local-cluster to be different, I'm open to it. Otherwise, I think it is better if our test path mirrors the real path as close as possible.

Probably fine to make this a method on SnapshotConfig. And probably fine to do that in a subsequent PR :)

🤝 - Maybe next I'm waiting on a ledger-tool run