feat: Implement disk-based persistence with Redb (Bounty #7) - #12
feat: Implement disk-based persistence with Redb (Bounty #7)#12a-shannon wants to merge 25 commits into
Conversation
a-shannon
changed the title
|
Nice work — really clean redb integration, and the per-key undo with prior values is the right call. We hit a related deletion bug on a separate Rust full-node implementation of the Ergo mainnet (using the persistence design from #10) and your model avoids it by construction: undoing an insert restores the prior bytes byte-for-byte rather than blindly deleting, so a digest that's still referenced from the rolled-back-to state survives correctly. Heads up on a separate but adjacent issue you'll likely hit once the storage runs through enough block applies. Empirical evidence after a fresh bootstrap + 250 blocks of steady-state validation: Filed #13 with a one-file fix: distinguish Curious whether your integration tests cover the multi-block lazy-materialization case (insert, flush, evict in-memory subtrees back to LabelOnly, modify a far-apart subtree, flush, walk back into the first subtree). That's the workload that exposes the bug. 🤖 Generated with Claude Code |
|
Hey @mwaddip, thanks a lot for the thorough review, the real-world mainnet context, and the kind words! We really appreciate it. Also, huge credit to you again for the Arc insight in PR #10 — it was the absolute prerequisite for getting this architecture off the ground. Your analysis of the contains_recursive bug is a brilliant catch and makes perfect sense. Since our RedbAVLStorage::update strictly relies on prover.removed_nodes() to physically delete keys from the DB, a false negative on reachability would absolutely trick our persistence layer into deleting active nodes, eventually leading to a panic!("Node not found -- database corrupted") from our Resolver. Failing safe (treating LabelOnly as true to prevent aggressive pruning) is exactly the correct fix for a lazily-materialized authenticated dictionary like ours. To answer your question: No, our current integration tests do not cover that exact multi-block lazy-materialization edge case. Our tests verify the basic persistence contract, state restoration, multi-version rollbacks, and correct proof generation after DB reloads. While we do have a test that persists and reloads a 500-node tree (which naturally yields LabelOnly stubs at the root), the subsequent operations don't simulate the specific sequence of "evict -> modify far-apart subtree -> trigger false GC rotation". That is definitely a blind spot in our test suite! I'll gladly jump over to PR #13 and drop an approving review. Since it's a focused fix on the core tree logic, it perfectly complements our persistence layer without any merge conflicts. Once your fix is merged, adding that exact stress-test scenario you described to the test suite would be a great way to lock down this lazy-materialization behavior for good. Thanks again for the massive heads-up and the great open-source collaboration! 🍻 |
…bsent
`contains_recursive` previously returned false at the catch-all `else`
arm whenever it reached a non-Internal node. That branch fires for both
Leaf nodes (terminal — return false is correct) and LabelOnly nodes
that the resolver could not materialise (return false is *unsafe*).
Used by `removed_nodes()` to decide which digests to delete from
persistent storage:
for cn in &self.base.changed_nodes_buffer_to_check {
if !self.contains(cn) {
self.base.changed_nodes_buffer.push(cn.clone())
}
}
If `contains()` walks into an unresolvable subtree, returning false says
"definitely not in tree → delete it". The caller deletes the node from
storage. But the node may still be referenced from the very subtree we
couldn't resolve. The next walk into that subtree hits a LabelOnly with
the deleted digest and bails:
ERROR ergo_sync::state: apply_state failed
error=UTXO state operation failed:
operation N failed: Should never reach this point.
If in prover, this is a bug. If in verifier, this proof is wrong.
Observed in production on a Rust full-node implementation of the Ergo
mainnet at v0.4.x. After ~250 blocks of steady-state validation with
the typical "most subtrees are LabelOnly, only walked paths are
materialised" prover state, an on-disk scan revealed:
Reachable nodes: 6,353,404
Missing references: 135 (parent in storage, child digest not in NODES_TABLE)
Orphan nodes: 378,274 (in storage but unreachable from root)
Fix: distinguish Leaf from LabelOnly at the catch-all. Leaf with
non-matching label remains terminal → false. LabelOnly that the
resolver couldn't materialise → true (fail safe). At worst this leaks
orphan nodes; never silently corrupts.
Refactored to use a `Kind` enum so we can scope the immutable borrow
that does the discrimination separately from the mutable borrow that
drives the Internal-node walk.
All 22 existing tests pass unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…upport The Resolver type was defined as a bare function pointer (fn(&Digest32) -> Node), which cannot capture state. This makes it impossible to implement VersionedAVLStorage with a real storage backend — the resolver needs to load nodes from a database, but a function pointer cannot hold a database reference. Changed to Arc<dyn Fn(&Digest32) -> Node + Send + Sync> which allows closures that capture storage handles. Arc (not Box) because AVLTree derives Clone. Send + Sync for thread safety with concurrent readers. All 22 existing tests pass unchanged (modulo wrapping bare functions in Arc::new). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
28862a1 changed Resolver from a plain fn pointer to Arc<dyn Fn> for the persistence backend, which broke every caller written against the old signature (sigma-rust's interpreter passes bare closures at 13 sites). Take `impl Fn(&Digest32) -> Node + Send + Sync + 'static` and wrap it in the Arc inside the constructor: fn-pointer-era callers compile unchanged, capturing closures (the persistence resolver) pass straight in without their own Arc::new. Callers holding a prebuilt Resolver construct the struct literally — `resolver` is a pub field. Internal call sites (prover test + tests/common) updated to drop the now-redundant Arc::new. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
new(impl Fn) (a4a2aa7) serves closure callers — including the interpreter's bare, unannotated |digest| closures, which infer their param type only because the bound is literally Fn(&Digest32). But impl Fn rejects a pre-built Resolver (Arc<dyn Fn> does not impl Fn, E0277), and a single generic bound cannot serve both: an impl IntoResolver bound that accepts Arc breaks unannotated closures (E0282). Add a second constructor with_resolver(resolver: Resolver, ...) taking a pre-built Arc directly (no re-wrap), for storage-backend callers (the node). The interpreter keeps using new(); the node uses with_resolver(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds a default-no-op method on the storage trait so callers can force a durable commit (fsync) of outstanding writes. Implementations using deferred writes (e.g. redb Durability::None) should override this to fsync the backing store. In-memory or always-durable storages can keep the default no-op. Motivation: PersistentBatchAVLProver::generate_proof_and_update_storage uses non-durable writes for throughput. Without a caller-triggered flush, graceful SIGTERM leaves uncommitted state pending; on reopen, redb sees no valid commit and treats the storage as empty. Periodic flush calls bound crash data loss. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> (cherry picked from commit 9a67695)
Proof generation (pack_tree) and persistence (collect_changed_nodes) both relied on the tree's visited/is_new flags, creating a conflict: generate_proof() called tree.reset(), clearing flags that a subsequent storage.update() needed to find changed nodes. This caused internal nodes created during AVL rebalancing to never be persisted. Decouple them: 1. Add modified_nodes tracking (AuthenticatedTreeOpsBase) — populated unconditionally in on_node_visit, cleared by generate_proof(). was_modified() keys on Rc::as_ptr, independent of tree flags. 2. Switch pack_tree from tree.visited() to was_modified() — proof generation no longer touches visited/is_new. 3. Replace tree.reset() in generate_proof() with modified_nodes.clear() + a needs_cycle_reset flag. The next perform_one_operation() resets visited/is_new (persistence flags) at the cycle boundary, after update() had a chance to collect them. 4. Relax check_tree_helper's post_proof assertion — flags are now cleared by update() or perform_one_operation(), not generate_proof(). 5. Reset the cloned tree in generate_proof_for_operations() so the clone's on_node_visit starts with fresh flags. modified_nodes is a BTreeMap keyed on the node's address rather than a list, because pack_tree asks was_modified() once per node it walks: a linear scan there makes proof generation quadratic in the number of visits, which on a remove-heavy benchmark costs minutes rather than seconds. The map's NodeId values keep each node alive, so an address cannot be recycled while it is still a key, and the set stays per-prover — a tree clone shares its nodes, so per-node state would leak proof bookkeeping between the original and the clone. (cherry picked from commit a42be22)
Add a public method that installs a persisted root and rebases the proof cycle atomically. Replaces the ad-hoc field assignments currently spread across four restore sites in the node. After restoring a storage-loaded root, callers must: - Clear is_new/visited flags on the fresh tree - Drop stale changed-node buffers from the previous cycle - Rebase old_top_node to the new root - Clear accumulated directions from any prior (failed) cycle restore_root() does all of this in one place, owning the proof-cycle invariant. (cherry picked from commit 87da2da)
restore_root dropped the changed-node buffers but not modified_nodes, the map pack_tree gates on. Only generate_proof() clears that map, and a cycle that is rewound never reaches it -- so a block that is applied and then rejected leaves its entire visited set marked. The next pack_tree() then expands nodes it should have labelled. Where the rewind reinstalls a root that is still address-live -- which is what a storage layer holding node handles does -- that is a different proof for identical tree state, not merely retained memory: 740 vs 735 bytes in the round-trip test below. PersistentBatchAVLProver::rollback had drifted the same way, by re-implementing the rewind by hand instead of calling it; it had already missed the old_top_node rebase once for the same reason. It now delegates to restore_root, so there is a single rewind implementation that cannot drift from itself. Each clause is pinned: with only the modified_nodes clear, the two rollback tests still fail.
- Add 'persistence' feature flag with redb optional dependency - Implement RedbVersionedStore with 4 typed tables (NODES, META, UNDO_LOG, VERSIONS) - Implement undo-log pattern with linked-list version chaining - Implement RedbAVLStorage with VersionedAVLStorage trait - Add 5 unit tests for versioned store operations - extern crate std when persistence is enabled (redb requires std) - Core crate remains no_std compatible
- test_persist_and_reload_basic: persist/reopen/verify version - test_persist_multi_version_and_rollback: multi-version + rollback to v1 - test_persist_proof_still_verifies: proof verification after persistence - test_rollback_versions_chain: version linked list traversal - test_large_tree_persistence: 500-element stress test
- Q1: Resolver uses Arc<RedbVersionedStore> instead of Database::open() per call. Eliminates OS file-lock panics and massive I/O overhead. - Q2: Content-addressed node dedup - skip persisting nodes whose hash already exists in DB (idempotent by definition). Eliminates write amplification from re-persisting unchanged nodes. - Q4: TOCTOU fix - read next_lsn and parent_version inside the write transaction for full atomicity under concurrent access.
4c88958 to
6f7eee0
Compare
2 participants
Resolves #7
(Includes the architectural prerequisite from #10)
This PR implements the requested disk-based persistence for
ergo_avltree_rust, enabling the state versioning and rollback capabilities required by Ergo applications like ChainCash.Architecture
We chose
redbas the storage backend. It is pure-Rust, ACID-compliant, and completely avoids the heavy C++ build dependencies (and cross-compilation headaches) associated with RocksDB or LevelDB.The core crate remains strictly
#![no_std]. The persistence module and itsstdrequirements are cleanly gated behind apersistencefeature flag.Key Features
Resolverfrom afnpointer to anArc<dyn Fn>to allow closures to safely capture the database handle (credit to @mwaddip's insight in PR Change Resolver from fn pointer to Arc<dyn Fn> for storage backends #10).nodes,meta,undo_log,versions) mirroring the ScalaLDBVersionedStore.parent_version_id, allowing deterministic backward traversal and restoration duringrollback().Arc<RedbVersionedStore>in the resolver closure to prevent OS-level file lock panics and avoid repetitiveDatabase::openoverhead.pack()/unpack()methods. Unresolved children are gracefully converted toLabelOnlyand lazily loaded from the DB via theResolver.Testing
no_stdtests pass cleanly without the feature flag.Future Work (Out of Scope for this Bounty)
keepVersions), preventing unbound DB growth on long-running nodes.VersionedAVLStorage::update()takes no explicitversion_id, so we use the Tree Digest. A future trait update could accept explicit block IDs for better empty-block handling.Ready for review! Thank you to kushti for the bounty.