Change Resolver from fn pointer to Arc<dyn Fn> for storage backends - #10
Closed
mwaddip wants to merge 5 commits into
Closed
Change Resolver from fn pointer to Arc<dyn Fn> for storage backends#10mwaddip wants to merge 5 commits into
mwaddip wants to merge 5 commits into
Conversation
…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>
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>
…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>
Author
|
Downstream impact heads-up: Scope (3 files):
Pure type-level adaptation, no semantic changes. Tracking PR on sigma-rust side, currently Draft pending this merge + a version bump: ergoplatform/sigma-rust#865. |
…f panicking `BatchAVLVerifier` tree reconstruction and the value-length checks in the modify path used raw slice indexing, `stack.pop().unwrap()`, and `assert!`, so a malformed / truncated / empty proof, an out-of-range key length, or an operation whose value length does not match the tree's fixed value length panicked instead of failing gracefully. The reference (scorex) `BatchAVLVerifier` wraps reconstruction in a `Try` and treats any failure as "no reconstructed tree", so downstream consumers (e.g. sigma's `CErgoTreeEvaluator` / `CAvlTreeVerifier`) never observe a panic — they see a verifier whose operations fail and whose digest is `None`. Honor the crate's existing `Result` / `ensure!` contract at the panic sites so the same inputs return `Err` here too: - `reconstruct_tree`: bounds-checked proof reads via a `read_proof_slice` helper (with `checked_add` to avoid length overflow), a guarded loop bound, and `ok_or` on the stack pops. - `modify_helper`: `assert!` -> `ensure!` on the value-length checks. Adds `tests/malformed_proof.rs` covering empty / garbage / truncated proofs, an oversized key length, and a wrong-length operation value. Co-Authored-By: Claude Opus 4.8 <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>
Author
|
Superseded by #15 — a self-contained branch off |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes Resolver from fn(&Digest32) -> Node to Arc<dyn Fn(&Digest32) -> Node + Send + Sync>. A bare fn can't capture a database handle, so VersionedAVLStorage / PersistentBatchAVLProver can't be backed by real storage today.
Arc not Box because AVLTree: Clone and Box isn't. All 22 tests pass; existing sites wrapped via Arc::new(...).