feat: option to skip ledger replay #460

Dodecahedr0x · 2025-07-16T15:51:16Z

Closes #418

This PR:

Adds a config parameter to skip ledger replay when starting a validator, discarding the old ledger but preserving accountsDB and the validator keypair.
Adds a test that checks that the state is still there but not the ledger

Snippets

TOML:

[ledger]
resume-strategy = "replay-and-resume"
path = "ledger.example.com"
size = 1000000000

CLI:

cargo run -- --ledger-resume-strategy replay-and-resume
LEDGER_RESUME_STRATEGY=replay-and-resume cargo run

Migration

Old config:

[ledger]
reset = true

Becomes:

[ledger]
resume-strategy = "reset"

Old config:

[ledger]
reset = false

Becomes:

[ledger]
resume-strategy = "replay"

thlorenz

Aside from the requests I had add some test for toml parsing for the new config (whichever way we decide to implement it).

Let's have a chat with @GabrielePicco about introducing that enum, as that makes sure users don't try to provide an invalid config and have different results than expected, i.e some might try to replay the ledger and then reset it for whatever reason, obviously that is not valid.
With an enum it is obvious and only valid configs are allowed.

Also please always include a usage example, i.e. a toml snippet, ENV var setting + arg override. That makes it much easier to see what config was added and how.

thlorenz · 2025-07-21T05:37:23Z

magicblock-accounts-db/src/lib.rs

 pub type StWLock = Arc<RwLock<()>>;

+pub const ACCOUNTSDB_DIR: &str = "accountsdb";
 const ACCOUNTSDB_SUB_DIR: &str = "accountsdb/main";


Use the above const you introduced here so to concat ACCOUNTSDB_SUB_DIR we don't change accountsdb substring in one place and forget it in the tother.

Addressed in f723e75 but it uses a method instead of a const to avoid creating a macro or doing more complex &str building

thlorenz · 2025-07-21T05:40:18Z

magicblock-api/src/ledger.rs

+        Slot::default()
+    };
+
+    if reset || skip_replay {


So if we skip_replay we remove the ledger here? How are we gonna be able to restart from that ledger now?

Only resuming just uses AccountsDB

thlorenz · 2025-07-21T05:42:21Z

magicblock-api/src/magic_validator.rs

            &identity_keypair,
-            config.validator_config.ledger.reset,
+            config.validator_config.ledger.reset
+                || config.validator_config.ledger.skip_replay,


So if we skip replay then the validator keypair can be different?
This might be intended, but it seems odd to me that we allow a different validator to continue with a ledger that was created by another validator.

Unintended, addressed in 1865b70. Resetting the validator keypair is part of this PR #458

thlorenz · 2025-07-21T05:46:09Z

magicblock-api/src/magic_validator.rs

            ledger.ledger_path().as_path(),
-            config.validator_config.ledger.reset,
+            config.validator_config.ledger.reset
+                || config.validator_config.ledger.skip_replay,


This is getting repetetive.
Please add a method to LedgerConfig, similar to replayed such that here you could just do

!config.validator_config.ledger.replayed()

Also consider adding this as a top level method, so not the entire codebase has to know how things are configured, i.e. it might be better to just be able todo:

if !config.ledger_replayed() { .. }

Addressed in 1865b70

thlorenz · 2025-07-21T05:51:45Z

magicblock-api/src/magic_validator.rs

-        let ledger = ledger::init(ledger_path, reset)?;
+        let (ledger, last_slot) = ledger::init(
+            ledger_path,
+            ledger_config.reset,


This is getting hairy, two booleans right next to each other. Easy to swap them by accident without noticing.
Do we need both or could we derive one from them?

Looking at these two bools all over the place it might be better to introduce an enum instead that is derived from the flags in the config, i.e.:

pub enum LedgerResumeStrategy { // implicitly replay is false here Reset, // do not reset and resume without replay ResumeOnly, // do not reset, replay and then resume ReplayAndResume }

You can pass this down and properly match to cover all cases everywhere. Also you could add convenience methods to the config to just check for a specific case.

Addressed in 1865b70

thlorenz · 2025-07-21T06:11:15Z

test-integration/test-ledger-restore/tests/03_single_block_tx_order.rs

 fn read(ledger_path: &Path, keypairs: &[Keypair]) -> Child {
    let (_, mut validator, ctx) =
-        setup_offline_validator(ledger_path, None, Some(SLOT_MS), false);
+        setup_offline_validator(ledger_path, None, Some(SLOT_MS), false, false);


ditto what's what here?

thlorenz · 2025-07-21T06:11:22Z

test-integration/test-ledger-restore/tests/04_flexi-counter.rs

        Some(programs),
        Some(SLOT_MS),
        true,
+        false,


ditto what's what here?

thlorenz · 2025-07-21T06:11:25Z

test-integration/test-ledger-restore/tests/04_flexi-counter.rs

        Some(programs),
        Some(SLOT_MS),
        false,
+        false,


ditto what's what here?

This goes for all tests that follow as well .... (not putting ditto everywhere :) )

thlorenz · 2025-07-21T06:12:45Z

test-integration/test-ledger-restore/tests/15_skip_replay.rs

+    slot: u64,
+) -> Child {
+    let (_, mut validator, ctx) =
+        setup_offline_validator(ledger_path, None, None, false, true);


Again, no clue what setting you're using here. reset? replay?

thlorenz · 2025-07-21T06:15:29Z

test-integration/test-tools/src/validator.rs

        .arg("--")
        .arg(config_path)
        .env("RUST_LOG_STYLE", log_suffix)
+        .env("RUST_LOG", "info")


Please don't override this env var here.
CI is logging on trace and devs wanna log on warn maybe.
Setting this in the code ignores the actual value of that env var.

Just run tests with that env var set i.e. RUST_LOG=warn make test.

Look at the makefile how it is defaulted, but if you set it from the command line that is the proper way to override it.

Yes, it's a mistake, addressed in 1865b70

thlorenz

LGTM aside from the naming Nits.

Please improve them before merging.

thlorenz · 2025-07-22T05:55:24Z

magicblock-accounts-db/src/lib.rs

 pub type StWLock = Arc<RwLock<()>>;

 pub const ACCOUNTSDB_DIR: &str = "accountsdb";
-const ACCOUNTSDB_SUB_DIR: &str = "accountsdb/main";


Consider using https://crates.io/crates/const_format since otherwise the method call is doing work every time.
With const_fmt you'd be able to assign to a const and use as you were before.

Addressed in ad9bdc8

thlorenz · 2025-07-22T05:57:16Z

magicblock-api/src/ledger.rs

 ) -> ApiResult<(Ledger, Slot)> {
    // Save the last slot from the previous ledger to restart from it
-    let last_slot = if skip_replay || !reset {
+    let last_slot = if resume_strategy.resume() {


I know I wasn't very good providing the naming for these things, just wanted to provide quick examples, but anything returning a bool should be something with is|has|does, etc.

resume looks like an action (in this case I'd think it resumes something and returns true if successful)

is_resuming is much better.

Addressed in 540667b

thlorenz · 2025-07-22T05:58:02Z

magicblock-api/src/magic_validator.rs

    // -----------------
    fn maybe_process_ledger(&self) -> ApiResult<()> {
-        if self.config.ledger.reset || self.config.ledger.skip_replay {
+        if !self.config.ledger.resume_strategy.replay() {


is_replaying

Addressed in 540667b

thlorenz · 2025-07-22T06:00:24Z

magicblock-config/tests/fixtures/05_all-goes.toml


 [ledger]
-reset = false
+resume-strategy = "replay-and-resume"


Again I wasn't very good with suggestions for the names. replay kinda implies resume right?

So I'd think the enum values should be: reset | replay | resume-only.

resume-only could also be just resume - up to you. The only part just makes it double clear that we won't replay.

Addressed in 540667b

Solves #418 This PR: - Adds a config parameter to skip ledger replay when starting a validator, discarding the old ledger but preserving accountsDB and the validator keypair. - Adds a test that checks that the state is still there but not the ledger TOML: ```toml [ledger] resume-strategy = "replay-and-resume" path = "ledger.example.com" size = 1000000000 ``` CLI: ```bash cargo run -- --ledger-resume-strategy replay-and-resume LEDGER_RESUME_STRATEGY=replay-and-resume cargo run ``` Old config: ```toml [ledger] reset = true ``` Becomes: ```toml [ledger] resume-strategy = "reset" ``` --- Old config: ```toml [ledger] reset = false ``` Becomes: ```toml [ledger] resume-strategy = "replay" ```

Dodecahedr0x force-pushed the feat/skip-replay branch from 0986d91 to 2680886 Compare July 17, 2025 15:48

Dodecahedr0x marked this pull request as ready for review July 18, 2025 12:53

Dodecahedr0x force-pushed the feat/skip-replay branch from 9a3c8f8 to f684bc4 Compare July 18, 2025 16:47

Dodecahedr0x requested review from GabrielePicco and thlorenz July 20, 2025 20:50

thlorenz requested changes Jul 21, 2025

View reviewed changes

Dodecahedr0x assigned bmuddha Jul 21, 2025

Dodecahedr0x added the P2 Medium priority label Jul 21, 2025

Dodecahedr0x mentioned this pull request Jul 21, 2025

Inconsistent config casing #477

Closed

Dodecahedr0x requested a review from thlorenz July 21, 2025 16:35

thlorenz approved these changes Jul 22, 2025

View reviewed changes

Dodecahedr0x removed the P2 Medium priority label Jul 24, 2025

Dodecahedr0x added 12 commits July 28, 2025 10:07

feat(config): skip replay option

468216a

test(ledger): skip replay

5b4d8ac

test(ledger): fix skip replay

45e1896

fix(ledger): last slot when not resetting

ff50fb5

fix(ledger): use updated cleanup method

1a96d68

feat(config): replay strategies enum

0c4e802

style(config): format

4d3d43c

feat(accountsdb): build sub dir from const

52738c1

feat(accountsdb): use const_format

ad9bdc8

refactor(config): rename resume strategies

540667b

test(config): fix resume strategy names

1f21a5d

feat(config): apply merge trait everywhere

9fa235d

Dodecahedr0x force-pushed the feat/skip-replay branch from b70fe3b to 9fa235d Compare July 28, 2025 08:19

GabrielePicco approved these changes Jul 31, 2025

View reviewed changes

Dodecahedr0x merged commit 1e5d6a4 into dev Aug 1, 2025
4 of 6 checks passed

Dodecahedr0x deleted the feat/skip-replay branch August 1, 2025 16:05

Dodecahedr0x mentioned this pull request Aug 5, 2025

feat: add option to skip ledger replay #418

Closed

feat: option to skip ledger replay #460

feat: option to skip ledger replay #460

Uh oh!

Conversation

Dodecahedr0x commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Snippets

Migration

Uh oh!

thlorenz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thlorenz Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thlorenz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Dodecahedr0x commented Jul 16, 2025 •

edited

Loading

thlorenz Jul 21, 2025 •

edited

Loading