Cleanup SnapshotConfig struct and usage

steviez commented 1 year ago

General note

SnapshotConfig is currently fine as-is; this task is a cleanup task that would be good for someone dipping their toes into Rust and/or as a learning experience to see all the places that touch snapshots within the validator client. If any potential readers have interest in picking up this task, feel free to indicate in a comment.

Problem

Validators can run with varying levels of what is enabled / disabled in regards to snapshots. Some of the variations:

Validator can choose ONLY load from snapshot and NOT generate any snapshots
- Technically, validators can load from genesis but on mainnet/testnet/devnet, this is not practical
Validators can generate ONLY full snapshots without incremental snapshots
Validators can generate full AND incremental snapshots

However, the SnapshotConfig field also contains some fields that must be specified. So, the validator always has a SnapshotConfig struct, and some fields just get ignored or set to sentinel values.

Proposed Solution

Rework the SnapshotConfig parameter to better reflect the different modes of operation. After a little chatter with @brooksprumo and @apfitzge on some PR's that spawned this conversation (https://github.com/solana-labs/solana/pull/32236 and https://github.com/solana-labs/solana/pull/32237), one potential idea might look something like:

pub enum SnapshotMode {
    Disabled
    LoadOnly(Config)
    LoadAndGenerate(Config)
}

And then within the Config struct (actual PR should pick a better name, just using short name here for illustration), use appropriate types to represent items are not always required (ie Option<T>) or may have specific requirements (ie NonZero types if the value must explicitly be greater than 0`).

taabodim commented 1 year ago

@steviez I'd like to work on this issue, but it seems to need some in depth discussions, since SnapshotConfig is used in different places. let me know if you want to discuss this further.

steviez commented 1 year ago

Hi @taabodim,

I'd like to work on this issue

I'm glad to hear it!

but it seems to need some in depth discussions, since SnapshotConfig is used in different places

Hmm, I'm not sure I follow. You are correct that the SnapshotConfig struct is used in a handful of places, but the work to complete this issue would involve: 1) Updating the struct so that the types reflect the requirements of various fields (Option for non-required field, etc) 2) Updating all of the locations that use the struct

After doing 1), performing 2) would be necessary in order to get the code compiling and consistent with the new struct format.

Does that help clear things up, or do you have other questions / concerns that I may not have addressed ?

taabodim commented 1 year ago

@steviez

thanks for clearing it up. yes, I wanted to discuss the details of step 1.

it's not clear which fields are optional. if you could specify those, that would make it easier for me to do it. also, what are the other stuff that you plan to do as part of step 1?

steviez commented 1 year ago

Hi @taabodim - Sorry for the very delayed response. In any case, if you or someone else are still interested in working on this. All of the current configuration information is here: https://github.com/solana-labs/solana/blob/746f69772a33f98c1d9afab91eda14f491c7c2e9/runtime/src/snapshot_config.rs#L10-L49

And this enum is also relevant: https://github.com/solana-labs/solana/blob/746f69772a33f98c1d9afab91eda14f491c7c2e9/runtime/src/snapshot_config.rs#L93-L99

it's not clear which fields are optional.

I think some pre-existing knowledge of what is optional / is not within the validator would be helpful. That being said, here is a quick stab at some structs that I think would get us most of the way there. Note that I haven't thoroughly checked everything, but the use of NonZero and Option<_> types should illustrate constraints that certain fields have when the node is running in a certain "mode".

/// Specifies the ways that snapshots are allowed to be used
pub enum SnapshotMode {
    Disabled,
    LoadOnly(SnapshotLoadOnlyModeConfig),
    LoadAndGenerate(SnapshotLoadAndGenerateModeConfig),
}

pub struct SnapshotStorageConfig {
    /// Path to the directory where snapshot archives are stored
    pub archives_dir: PathBuf,
    /// Maximum number of snapshot archives to retain
    pub archives_to_retain: NonZeroUsize,
}

pub struct SnapshotLoadConfig {
    /// Full snapshot storage configuration
    pub full_snapshot_config: SnapshotTypeStorageConfig,
    /// Incremental snapshot storage configuration
    pub incremental_snapshot_config: Option<SnapshotTypeStorageConfig>,
    /// Path to the directory where bank snapshots are stored
    pub bank_snapshots_dir: PathBuf,
    /// The archive format to use for snapshots
    pub archive_format: ArchiveFormat,
    /// Snapshot version to generate
    pub snapshot_version: SnapshotVersion,
    /// This is the `debug_verify` parameter to use when calling `update_accounts_hash()`
    /// TODO: this boolean doesn't actually look to be used anywhere ???
    pub accounts_hash_debug_verify: bool,
}

pub struct SnapshotGenerateConfig {
    /// Generate a new full snapshot archive every this many slots
    pub full_snapshot_archive_interval_slots: NonZeroUsize,
    /// Generate a new incremental snapshot archive every this many slots
    pub incremental_snapshot_archive_interval_slots: Option<NonZeroUsize>,
    /// Thread niceness adjustment for snapshot packager service
    pub packager_thread_niceness_adj: i8,
}

pub struct SnapshotLoadOnlyModeConfig {
    pub load_config: SnapshotLoadConfig,
}

pub struct SnapshotLoadAndGenerateModeConfig {
    pub load_config: SnapshotLoadConfig,
    pub generate_config: SnapshotGenerateConfig,
}

amarwane commented 1 year ago

Hello, This request is still relevant, I would like to contribute to this project, any advice? Thanks

steviez commented 11 months ago

Hello, This request is still relevant, I would like to contribute to this project, any advice? Thanks

I think the cleanup I mentioned could still be nice. As for advice, I don't really have much else to add; I have already given a fairly detailed layout of what I think would work. If things don't make sense as-is (whether the Rust aspect or the validator aspect of it), I might suggest to do some learning / reading on your own before trying to tackle the task

solana-labs / solana