feat: add a sanity check for sync state of rocksdb vs sqlite, minor partitioning changes #1532

drahnr · 2026-01-19T14:36:34Z

Could we move all loading-related methods into a separate file? Basically, I'd split this file into:

src/state/mod.rs
src/state/loader.rs

I think overall, we need 3 loading modes:

* Load everything from memory (when rocksdb feature is disabled).
* Use persistent data from RockDB (when rocksdb feature is enabled). This would probably require passing filepaths to RocksDB databases on startup (or at least assuming some defaults).
* Rebuild RocksDB data from the database (when rocksdb feature is enabled and we are starting a new node, or the user sets some flag that indicates that RocksDB databases are to be re-built).

I think currently we have 1 and 2, and maybe some parts of 3.

Changes

Adds a --force-rebuild flag to the CLI to force re-building of the tree stores from sqlite
Error out if we detect a mismatch of the tree stores and sqlite

crates/store/src/state/loader.rs

drahnr · 2026-01-19T15:57:34Z

bin/node/src/commands/store.rs

+        /// has diverged from the database. This will delete existing tree storage and rebuild
+        /// it from scratch, which may take some time for large databases.
+        #[arg(long = "force-rebuild-tree-storage", default_value_t = false)]
+        force_rebuild_tree_storage: bool,


Relevant code to review 👆

I'm not convinced we want this to be an explicit option. We can also trigger this by manually deleting the database which should be sufficient so long as things are sort of working okay..

From a deployment perspective, this is problematic because we will either have it permanently true or false. Doing this once-off would require a manual intervention in any case.

Good point! I think manually removing the files is fine. We should probably just document this (as behavior may not be obvious).

I think the option is preferred, since it doesn't require having knowledge what all of this means. Deleting a specific subset of files seems to be an unnecessary footgun for an operator.

But there is no operator that will be able to use it since you cannot "deploy" with this option? Its only useable in a local context imo.

We similarly don't have an option to rebuild the sqlite database for example. Though technically its possible from raw blocks.

imo the fewer options there are, the better. The rarer the occurrence, the more "manual" it should be. If this becomes a common problem, then sure we can address it differently.

I agree, that it should be done as a subcommand, the current form 'd require a separate deployment cycle, which makes it less useful.

drahnr · 2026-01-19T15:58:28Z

Left markers for most relevant code changes

bobbinth

Looks good! Thank you! I left some comments inline - many of them are for the future.

crates/store/src/state/mod.rs

bin/node/src/commands/bundled.rs

crates/store/src/state/mod.rs

crates/store/src/state/loader.rs

bobbinth · 2026-01-20T01:25:46Z

crates/store/src/state/loader.rs

+        NullifierTree::with_storage_from_entries(self, entries)
+            .map_err(StateInitializationError::FailedToCreateNullifierTree)


Not for this PR, but would be great to make the methodology of loading account and nullifier trees consistent. Currently, for nullifier tree we use NullifierTree::with_storage_from_entries() and for account tree we use AccountTree::new().

Some of these changes may need to happen in miden-base. Let's create issues for these (unless we have the already).

My thinking is that with_storage_from_entries won't work anyways for larger nullifier sets, and hence this will go away in the next PR #1536

crates/store/src/state/loader.rs

crates/store/src/state/mod.rs

crates/store/src/errors.rs

Mirko-von-Leipzig · 2026-01-20T07:57:24Z

bin/node/src/commands/store.rs

+        /// has diverged from the database. This will delete existing tree storage and rebuild
+        /// it from scratch, which may take some time for large databases.
+        #[arg(long = "force-rebuild-tree-storage", default_value_t = false)]
+        force_rebuild_tree_storage: bool,


But there is no operator that will be able to use it since you cannot "deploy" with this option? Its only useable in a local context imo.

We similarly don't have an option to rebuild the sqlite database for example. Though technically its possible from raw blocks.

imo the fewer options there are, the better. The rarer the occurrence, the more "manual" it should be. If this becomes a common problem, then sure we can address it differently.

crates/store/src/state/loader.rs

crates/store/src/errors.rs

Mirko-von-Leipzig · 2026-01-20T08:29:49Z

crates/store/src/state/mod.rs

Currently we return an error if we have a root mismatch during apply_block.

miden-node/crates/store/src/state.rs

Lines 423 to 425 in a2de154

if nullifier_tree_update.as_mutation_set().root() != header.nullifier_root() {

return Err(InvalidBlockError::NewBlockInvalidNullifierRoot.into());

}

Should we have a similar plan if the corruption happens "live"? Its a bit of a weird situation since we can't know if its an SMT bug, or a block building bug.

I think we'd just bubble the error up and find the corrupted tree store on startup, and do it that way.

Right but this doesn't actually bubble up to anywhere. This is part of the gRPC server on the store side, so it gets sent back to the block-producer which shrugs, rolls the block back and tries again.

I'll create a follow-up issue, maybe it's time for fatality to disambiguate which ones are relevant for the query response and which ones are an actual issue for the service running.

…-rebuild-rocksdb

drahnr · 2026-01-20T13:58:05Z

crates/block-producer/src/server/tests.rs

                ntx_builder_listener,
                block_producer_listener,
                data_directory: dir,
+                grpc_timeout: std::time::Duration::from_secs(30),


How did this compile previously??

bobbinth · 2026-01-20T22:15:15Z

crates/store/src/state/mod.rs

+    nullifier_tree: miden_protocol::block::nullifier_tree::NullifierTree<
+        miden_protocol::crypto::merkle::smt::LargeSmt<S>,
+    >,


nit (but let's address in a follow-up): we don't need to use fully-qualified paths here.

drahnr mentioned this pull request Jan 19, 2026

feat: rocksdb storage for AccountTree and NullifierTree by default #1326

Merged

drahnr requested a review from Mirko-von-Leipzig January 19, 2026 15:56

drahnr commented Jan 19, 2026

View reviewed changes

crates/store/src/state/loader.rs Show resolved Hide resolved

drahnr commented Jan 19, 2026

View reviewed changes

bobbinth approved these changes Jan 20, 2026

View reviewed changes

Mirko-von-Leipzig reviewed Jan 20, 2026

View reviewed changes

crates/store/src/errors.rs Outdated Show resolved Hide resolved

Mirko-von-Leipzig reviewed Jan 20, 2026

View reviewed changes

drahnr changed the title ~~feat: add force rebuild and add a sanity check for sync state of rocksdb vs sqlite~~ feat: add a sanity check for sync state of rocksdb vs sqlite, minor partitioning changes Jan 20, 2026

drahnr force-pushed the bernhard-startup-build-and-rebuild-rocksdb branch from 8c2d310 to a4f0119 Compare January 20, 2026 13:04

fix

24b0977

drahnr force-pushed the bernhard-startup-build-and-rebuild-rocksdb branch from a4f0119 to 24b0977 Compare January 20, 2026 13:05

drahnr added 3 commits January 20, 2026 14:25

fmt

cbbad6e

log

1262edd

Merge remote-tracking branch 'origin' into bernhard-startup-build-and…

3d4b6b8

…-rebuild-rocksdb

drahnr commented Jan 20, 2026

View reviewed changes

Mirko-von-Leipzig approved these changes Jan 20, 2026

View reviewed changes

drahnr merged commit 0fa7208 into next Jan 20, 2026
18 checks passed

drahnr deleted the bernhard-startup-build-and-rebuild-rocksdb branch January 20, 2026 14:09

bobbinth reviewed Jan 20, 2026

View reviewed changes

Mirko-von-Leipzig mentioned this pull request Jan 21, 2026

AccountTree: Establish re-synchronization between sqlite and rocksdb storage #1328

Closed

drahnr mentioned this pull request Jan 22, 2026

fix: lost changes from rocksdb PRs - remove full paths #1574

Merged

		NullifierTree::with_storage_from_entries(self, entries)
		.map_err(StateInitializationError::FailedToCreateNullifierTree)

	if nullifier_tree_update.as_mutation_set().root() != header.nullifier_root() {
	return Err(InvalidBlockError::NewBlockInvalidNullifierRoot.into());
	}

feat: add a sanity check for sync state of rocksdb vs sqlite, minor partitioning changes #1532

feat: add a sanity check for sync state of rocksdb vs sqlite, minor partitioning changes #1532

Uh oh!

Conversation

drahnr commented Jan 19, 2026

Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mirko-von-Leipzig Jan 20, 2026 • 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!

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!

drahnr commented Jan 19, 2026

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Mirko-von-Leipzig Jan 20, 2026 •

edited

Loading