feat(merk,grovedb): expose aggregate_count proof verification under verify feature#658
Conversation
…erify feature The AggregateCountOnRange proof primitive (PR #656) had its modules gated behind feature = "minimal" in both grovedb-merk and grovedb, making the verifier entry points (`verify_aggregate_count_on_range_proof` and `GroveDb::verify_aggregate_count_query`) unreachable from downstream lean-verifier crates that depend on `grovedb`/`grovedb-merk` with `default-features = false, features = ["verify"]`. Widen the module gates to `any(feature = "minimal", feature = "verify")` to match the pattern already used by `query::common`, `query::merge`, `query::query_item`, and `query::verify` in the same file. Refactor aggregate_count.rs to keep prover-only items (`RefWalker`, `Fetch`, `AggregateData`, `emit_count_proof`, etc.) gated `feature = "minimal"` while exposing the verifier (`verify_aggregate_count_on_range_proof`, `verify_count_shape`, `classify_subtree`, `key_strictly_inside`) under the wider gate. Add a `cfg(all(test, feature = "verify"))` test module with hex-fixture round-trip, empty-merk, and byte-mutation no-silent-forgery tests that exercise the verifier without any prover dependency, plus an `#[ignore]`d helper to regenerate the fixture if the proof encoding ever changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR restructures feature flags to enable verify-only builds of proof verification code. It broadens the ChangesCount Proof Feature Gating and Verification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #658 +/- ##
========================================
Coverage 90.63% 90.64%
========================================
Files 184 184
Lines 54763 54826 +63
========================================
+ Hits 49637 49699 +62
- Misses 5126 5127 +1
🚀 New features to boost your workflow:
|
The verify_only_tests module was gated cfg(all(test, feature = "verify")), which hid it from CI runs that only enable the default `full` feature set (which doesn't include `verify`). Codecov flagged the test bodies as uncovered patch lines as a result. Widen to cfg(all(test, any(feature = "minimal", feature = "verify"))). The verifier function is available under both gates and these tests have no prover dependency, so the `verify`-only gate was buying nothing except missing CI coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The #[ignore]d dump_verify_only_fixtures test never ran in CI, so its ~17 lines counted as uncovered patch lines. Replace it with an active verify_only_fixture_matches_fresh_prover_output test that runs every build, asserts the hardcoded fixture in verify_only_tests still matches fresh prover output byte-for-byte, and prints regeneration-ready constants on mismatch. This catches encoding drift automatically and gets the fixture maintenance lines into CI coverage. Promote the FIXTURE_*_HEX/COUNT constants to pub(super) so the parity test can compare against them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both test modules were gated with feature = "minimal" / any(minimal, verify), but the crate convention (e.g., merk/src/element/tree_type.rs) is plain #[cfg(test)] — tests rely on default = ["full"] (which pulls in minimal) being active at test time. The extra gates were defensive beyond what the surrounding code does. Also tighten the verify_only_tests doc comment now that the drift check runs on every build instead of being a separate ignored helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Reviewed |
Two PRs landed on develop while this PR was open: - #659: Element::NotSummed wrapper variant - #658: aggregate_count proof verification under verify feature Conflicts resolved in 8 files. The substantive resolution work: DISCRIMINANT COLLISION. The shipped cidx PR used ElementType discriminant 16 for CountIndexedTree and 17 for ProvableCountIndexedTree. Develop's NotSummed PR allocated byte 16 as NOT_SUMMED_WRAPPER_DISCRIMINANT. Both are still pre-shipping, so renumbering is safe — and since the NotSummed wrapper byte semantics need to round-trip across the wire, the wrapper byte stays at 16 and the cidx discriminants shift: CountIndexedTree: 16 → 17 ProvableCountIndexedTree: 17 → 18 NonCountedCountIndexedTree: 144 → 145 (= 0x80 | 17) NonCountedProvableCountIndexedTree: 145 → 146 (= 0x80 | 18) ENUM VARIANT ORDER. Bincode encodes Element variants by ENUM ORDER (not by ElementType discriminant). The Element enum has been reordered so NotSummed appears at variant index 16 (matching NOT_SUMMED_WRAPPER_DISCRIMINANT), and CountIndexedTree / ProvableCountIndexedTree appear AFTER NotSummed at indices 17/18 — matching their new ElementType discriminants. Without this reordering, bincode would still write 16 for CountIndexedTree but from_serialized_value would interpret 16 as the NotSummed wrapper. WRAPPER NESTING CHECK in `from_serialized_value`: now explicitly rejects both nested NonCounted and cross-nesting with NotSummed (inner_byte == 15 || inner_byte == 16), in addition to the existing reject-high-bit-twin guard. Other resolutions are straightforward additions: both branches added arms to match expressions across helpers.rs, tree_type.rs, visualize.rs; merged additively. The two `mod tests` blocks in grovedb-element/src/element/mod.rs (one from each PR) renamed the cidx-side block to `cidx_tests` to avoid duplicate name. All 1944+ workspace tests pass post-merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
The
AggregateCountOnRangeproof primitive (landed in #656) had its modules gated behindfeature = "minimal"in bothgrovedb-merkandgrovedb, making the verifier entry points unreachable from downstream lean-verifier crates that depend ongrovedb/grovedb-merkwithdefault-features = false, features = ["verify"]— e.g. dashpay/platform'srs-drive-proof-verifier, which can't currently verify range-count proofs returned by the server.This widens the module gates to
any(feature = "minimal", feature = "verify")and refactorsaggregate_count.rsso prover-only items stayfeature = "minimal"-gated while the verifier compiles underverifyalone.What changed
merk/src/proofs/query/mod.rs— widened the gate onpub mod aggregate_count;and thepub use aggregate_count::verify_aggregate_count_on_range_proof;re-export fromfeature = "minimal"toany(minimal, verify). Matches the existing pattern used forquery::common,query::merge,query::query_item,query::verifyin the same file.grovedb/src/operations/proof/mod.rs— same widening onmod aggregate_count;.merk/src/proofs/query/aggregate_count.rs— split internal feature gates:verify_aggregate_count_on_range_proof,verify_count_shape,classify_subtree,key_strictly_inside,NULL_HASH,SubtreeClassification) — compile underany(minimal, verify).is_provable_count_bearing,provable_count_from_aggregate,impl RefWalker,emit_count_proof, prover-only importsOp/AggregateData/Fetch/RefWalker/ValueDefinedCostType/TreeType/GroveVersion/LinkedList) — stayfeature = "minimal".cfg(all(test, feature = "minimal")).cfg(all(test, feature = "verify"))verify_only_testsmodule with three tests:empty_merk_returns_null_hash_and_zero_count— basic verifier wiring smoke test.fixture_15_key_range_c_to_l_verifies— hex-encoded proof fixture round-trip.fixture_byte_mutation_does_not_silently_forge_count— single-fixture analogue of the existingfuzz_byte_mutation_no_silent_forgerytest, exercising the shape walk +node_hash_with_countrecomputation path without any prover dependency.#[ignore]ddump_verify_only_fixtureshelper to regenerate the hex fixture if the proof encoding ever changes.Why
dashpay/platform's #3623 has a
DriveDocumentCountQuery::aggregate_count_path_queryhelper (gatedcfg(any(server, verify))) that already builds the matchingPathQuery. Once this gate widens, the SDK side'sFromProof<DocumentCountQuery>forDocumentCountcan drop its current "this is gated upstream" error stub and callGroveDb::verify_aggregate_count_query(&proof_bytes, &path_query, grove_version)directly.The original gate on
aggregate_countwas the odd one out — likely an oversight in #656, since proof-verification primitives are exactly whatverifyis meant to expose.Test plan
cargo check -p grovedb-merk --no-default-features --features verify— clean.cargo check -p grovedb --no-default-features --features verify— clean.cargo clippy -p grovedb-merk --no-default-features --features verify --no-deps— clean (1 pre-existing warning unrelated to these changes).cargo clippy -p grovedb --no-default-features --features verify --no-deps— clean (4 pre-existing warnings inaggregate_sum_query/mod.rsunrelated to these changes).cargo test -p grovedb-merk --features "full,verify" --lib aggregate_count— 36 pass, 1 ignored (the fixture-regeneration helper).cargo test -p grovedb --lib aggregate_count— 32 pass.cargo test -p grovedb-merk --lib aggregate_count(defaultfullfeatures) — 33 pass, no regressions.Known caveat
The merk crate's
lib-testtarget does not compile under--no-default-features --features verifystandalone, but this is a pre-existing condition (verified by stashing this branch's changes and reproducing): tests inmerk/src/element/tree_type.rsreference minimal-only items without acfg(feature = "minimal")gate. The downstream-relevant artifact (the library itself underverify) compiles cleanly, which is what consumers likers-drive-proof-verifierneed.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests