Repository navigation
test(drive): pin that chained and composite joins prove the absence of a referenced document - #4852
Conversation
…f a referenced document A by-id join (chained query, composite by-id sub-query) treats a derived id with no document as an invalid proof, on the grounds that a `refersTo: permanentDocument` target cannot dangle. Moderator deletion will let a `canBeDeleted: false` document disappear, so that rule has to become "a proven absence is left out of the results". That is only sound if a prover cannot pass an existing document off as missing. These tests establish it at the grovedb level, for both surfaces: - with a referenced document absent from state, the honest merged proof satisfies `GroveDb::verify_query` against the full re-derived query; the exact-set assembly is the only thing refusing the result today - with a referenced document present but its id withheld from the outer component, verification fails inside grovedb (the proof lacks coverage for a key the verifier's re-derived query demands), before any assembly rule runs Each case runs over several tree positions (leaf, inner node, root, two neighbours, none present) so the absence boundary takes every shape. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
PR HygieneState: waiting-self-review · commit
Self-review is an author attestation that you have read the diff: This check passes when the policy is satisfied; the repository decides whether merging requires it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes add end-to-end tests for chained and composite query proofs. The tests distinguish valid absence proofs from incomplete proofs that omit existing referenced documents. ChangesProof soundness coverage
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Other Merge Risk: ⚪ Minimal · up to The added tests use the applicable result shape for authenticated missing documents, and no actionable merge risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
✅ Final review complete — no blockers (commit 89e57e3) · triage: normal |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4852 +/- ##
============================================
- Coverage 84.89% 75.37% -9.53%
============================================
Files 3062 3064 +2
Lines 410291 451890 +41599
============================================
- Hits 348331 340608 -7723
- Misses 61960 111282 +49322
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the test-only diff at head 89e57e3 against the surrounding proof verification and assembly code; no actionable in-scope issues were found. The four added tests distinguish genuinely absent referenced documents from withheld existing documents while preserving current assembly behavior. Independently ran both complete chained and composite query suites: 34 tests passed; git diff --check also passed.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The diff adds substantial adversarial and absence-proof tests across chained and composite joins that require understanding proof construction and verifier behavior, but changes no production logic or critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer
…eDocument) `refersTo: permanentDocument` only accepts a document type with `canBeDeleted: false`, so a reply to a deletable comment, or a like on a deletable post, cannot declare what it points at. `deletableDocument` is its disjoint counterpart: same declaration (`contractId`, `documentType`, `propertyAgreement`), but the referenced type must ALLOW deletion (`ReferencedDocumentTypeNotDeletableError`, 40131, otherwise), so a declaration always states which guarantee the reference carries. `permanentDocument` is unchanged. Write time: the referenced document must exist and every agreement pair must hold, exactly as for `permanentDocument`. Afterwards the target may be deleted; nothing blocks that. What a WRITER may not do is leave the reference dead: every replace of the referring document re-validates a `deletableDocument` reference, touched or not, so a dead one has to be repointed at a document that exists or cleared. A writer gate is therefore only ever checked against the new target. An `immutable` reference can only be cleared, and only once its target is gone: the replace action records the identifier each removed property held, and the immutable check lets that one change through after reading state. The referring document can always be deleted. Reads: chained queries and composite by-id joins accept either kind as the join source. A `permanentDocument` source keeps the strict rule (a missing document is an invalid proof). A `deletableDocument` source leaves a deleted document out and REPORTS its id: `missing_outer_ids` on the chained result, one list per sub-query on the composite result, on the unproven wire (two new proto fields, regenerated clients), in the proof verifier's types, and as `missingOuterIds` / `missingIds` in the wasm SDK. The omission is proven: every derived id is a queried key, and grovedb refuses a proof without the coverage to show one present or absent (#4852), which the new adversarial tests pin on the tolerant path, where no assembly rule stands behind it. Also: allowed as an indexOnly member key; refused for `preallocated` (the trees would outlive the target); a property cannot switch kind on a contract update. A dead reference stays dead: since #4859 a document id commits to the nonce of its create transition, so a deleted id can not be created again and a reference means that one document or nothing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Issue being fixed or feature implemented
A by-id join (the chained document query, and a composite query's by-id sub-query) treats a derived id that has no document as an invalid proof, and as corrupted state on the server. The justification in the code is that a
refersTo: permanentDocumenttarget cannot dangle.That stops being true once moderators can remove documents of a
canBeDeleted: falsetype (the moderator document deletion work, stacked on #4849):like.postId -> poststays a valid permanent reference, a moderator removes the post, and from then on every chained or composite page that contains a like of that post fails for every client, on the server and in the verifier.The fix is to turn the rule into "a join value whose document is proven absent is left out of the results". That is only sound if a prover cannot pass an EXISTING document off as missing. This PR does not change any behaviour. It pins, with tests, that the proof layer already gives that guarantee, so the follow-up can relax the two assembly functions and nothing else.
What was done?
Four tests, two per surface, in
rs-drive:chained_query_e2e_tests.rsshould_prove_the_absence_of_a_missing_referenced_post: with a referenced post absent from state, the honest merged proof satisfiesGroveDb::verify_queryagainst the full re-derived query (the verifier's authoritative pass, run on its own). The exact-set assembly is the only thing that refuses the result today (Error::Proof).should_reject_a_proof_withholding_an_existing_referenced_post: a dishonest prover builds the merged proof with one existing post's$idwithheld from the outer component. The normal verifier refuses it withError::GroveDB, before any assembly rule runs: the verifier re-derives the outer query from the PROVEN inner join values, and merk refuses abridged data inside an absence boundary ("Proof is missing data for query").composite_query_e2e_tests.rsshould_prove_the_absence_of_a_missing_joined_documentandshould_reject_a_proof_withholding_an_existing_joined_document: the same pair for a by-id join onquotedPostId.Each test runs over several tree positions (every post in turn, two neighbours together, the two edges together, and no referenced document in state at all), so the missing or withheld key lands as a leaf, an inner node and the root of the primary-key tree.
Notes for the reviewer:
canBeDeleted: falsetype (delete_document_for_contract_operations/v0/mod.rs:54). For the proof this is the same state: the key is absent between present neighbours.verify_queryleaves an absent key out of its results; it does not report it as a(path, key, None)entry. "Missing" is therefore the join values minus the proven outer ids.Err(Error::Proof(_))from the assembly). The follow-up that relaxes the assembly flips that one assertion per test and rewrites the two existingshould_refuse_a_dangling_referencetests.How Has This Been Tested?
On top of current
v4.2-dev(72b58f6):13 passed, 0 failed: the 4 new tests plus the existing chained suite and the composite refusal tests.
cargo clippy -p drive --tests --libreports nothing for the two files. The rest of the workspace was not run locally.Breaking Changes
None. Tests only.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit