solana-labs/solana

Cleanup SnapshotConfig struct and usage

Open

#32,243 opened on Jun 22, 2023

View on GitHub
 (6 comments) (0 reactions) (0 assignees)Rust (12,651 stars) (3,950 forks)batch import
good first issue

Description

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`).

Contributor guide

Tech stack
rust
Domain
backend
Issue type
refactor
DifficultyEstimated implementation difficulty for a new contributor, from 1 for very small changes to 5 for expert-level work.
3
Estimated timeA rough time range for an experienced contributor to investigate, implement, test, and prepare a pull request.
1-3 hours
Activity statusHow available the issue appears right now: fresh, active, stale, blocked, or waiting on maintainer input.
stale
ClarityHow clearly the issue explains the expected change, acceptance criteria, and next step.
mostly clear
Prerequisites
Basic Rust knowledgeUnderstanding of Solana validator snapshots
Newbie friendlinessA 1-100 score estimating how approachable this issue is for first-time contributors.
60
Research direction
Review the discussion in the issue and linked PRs (#32236, #32237) to understand the proposed SnapshotMode enum. The task involves reworking SnapshotConfig to use an enum with Disabled, LoadOnly, and LoadAndGenerate variants, and updating all usages in the validator client. Decide on naming for the inner Config struct and use appropriate types like Option<T> or NonZero. Ensure all code paths that currently use SnapshotConfig are updated to match the new design.