fix: remove ProveOptions from V1 proofs#647
Conversation
…tack V0 proofs embed ProveOptions (including decrease_limit_on_empty_sub_query_result) in the serialized proof bytes. Since the verifier deserializes and trusts these options, a malicious prover can manipulate how many results the verifier returns. V1 proofs no longer embed ProveOptions. The verifier uses ProveOptions::default() instead, ensuring the prover cannot influence limit-counting behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
grovedb/src/operations/proof/generate.rs (1)
820-856:⚠️ Potential issue | 🟠 MajorReject non-default
ProveOptionsfor V1 proofs.
GroveDb::prove_query_non_serialized_v1still shapes the proof with the caller-suppliedprove_options, but both V1 verifier entry points now always useProveOptions::default(). A proof generated withdecrease_limit_on_empty_sub_query_result = falsetherefore no longer round-trips under V1. V1 generation should either ignore that knob or fail fast when it is set away from the default.Suggested direction
pub(crate) fn prove_query_non_serialized_v1( &self, path_query: &PathQuery, prove_options: Option<ProveOptions>, grove_version: &GroveVersion, ) -> CostResult<GroveDBProof, Error> { let mut cost = OperationCost::default(); - let prove_options = prove_options.unwrap_or_default(); + if matches!( + prove_options, + Some(ProveOptions { + decrease_limit_on_empty_sub_query_result: false, + }) + ) { + return Err(Error::NotSupported( + "V1 proofs always use ProveOptions::default()".to_string(), + )) + .wrap_with_cost(cost); + } + let prove_options = ProveOptions::default();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grovedb/src/operations/proof/generate.rs` around lines 820 - 856, In prove_query_non_serialized_v1, detect when the caller supplied non-default ProveOptions (compare the unwrapped prove_options to ProveOptions::default()) and return an error (e.g., Error::InvalidQuery or a new error variant) instead of using them; update the function to reject any ProveOptions that differ from default before calling prove_subqueries_v1 so V1 proofs cannot be generated with non-default knobs like decrease_limit_on_empty_sub_query_result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@grovedb/src/operations/proof/generate.rs`:
- Around line 820-856: In prove_query_non_serialized_v1, detect when the caller
supplied non-default ProveOptions (compare the unwrapped prove_options to
ProveOptions::default()) and return an error (e.g., Error::InvalidQuery or a new
error variant) instead of using them; update the function to reject any
ProveOptions that differ from default before calling prove_subqueries_v1 so V1
proofs cannot be generated with non-default knobs like
decrease_limit_on_empty_sub_query_result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03ec1b91-0fdb-4d56-8bf1-cc87d550c31b
📒 Files selected for processing (5)
grovedb/src/debugger.rsgrovedb/src/operations/proof/generate.rsgrovedb/src/operations/proof/mod.rsgrovedb/src/operations/proof/verify.rsgrovedb/src/tests/proof_depth_limit_tests.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #647 +/- ##
========================================
Coverage 90.83% 90.83%
========================================
Files 182 182
Lines 51911 51909 -2
========================================
- Hits 47151 47150 -1
+ Misses 4760 4759 -1
🚀 New features to boost your workflow:
|
Summary
prove_optionsfield fromGroveDBProofV1struct — V1 proofs no longer embed prover-controlled optionsProveOptions::default()instead of deserializing options from the untrusted proofBackground
ProveOptions(specificallydecrease_limit_on_empty_sub_query_result) was embedded in the serialized proof and trusted by the verifier. A malicious prover could set this flag totrueto make empty subquery results consume the query limit, causing the verifier to return fewer results than actually exist. Both honest and malicious proofs verified correctly — the verifier couldn't distinguish them.Since all Platform queries use
decrease_limit_on_empty_sub_query_result = true(the default), removing it from V1 proofs has no practical impact while closing the attack vector.Test plan
cargo clippy -- -D warningscleancargo build --no-default-features --features verifybuilds🤖 Generated with Claude Code
Summary by CodeRabbit