feat: bidirectional references (non-batched operations) - #345
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGroveDB adds bidirectional references with inline backward-reference metadata, two-layer hashes, version-gated propagation, cascade deletion, cache-aware transactions, and dedicated proof handling. Queries strip internal metadata, while batches and unsupported tree configurations fail closed. ChangesBidirectional references
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to This PR adds persisted bidirectional-reference metadata that drives propagation and cascade deletion. On the current head, caller-supplied inverse entries can cause unrelated records to be rewritten or deleted, while malformed proofs can trigger excessive allocation and one authenticated-tree path bypasses a required shape guard; merge should be blocked until these integrity and availability issues are fixed. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 83.69% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 282 functions across 72 files. (2 skipped: 2 unsupported.) ✨ 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 |
3fd7002 to
ac4b627
Compare
fominok
changed the title
ad93977 to
fe66ee7
Compare
eec7145 to
4b58b52
Compare
0d98726 to
719b44f
Compare
6df25a5 to
63d319b
Compare
|
This is Claude. Status update and the plan for the next phase. Landed since the redesign: the full CodeRabbit + QuantumExplorer review rounds (14 real fixes across proofs, chunks, batch chain hashing, reciprocity auditing, hop budgets, and sweep guards), and Plan: Part 1 — Bidirectional references in
|
…eeping (batching M1) Every decision about what a family write implies — target registration (upsert by inverted path, budget-checked), referrer-chain propagation with lazy dangling/stale cleanup, cascade sets with per-entry consent, registration removal with unflagged-write tolerance, identical-edge no-ops, and the prospective-component hop budget — now lives in bidirectional_references/semantics.rs as pure planning functions over an abstract read-only ChainStore. Planners return an ordered Plan of DerivedMutations (writes with optional end hashes, deletes) and never read their own writes: the one place the interleaved flow re-read a value it had just written (propagation seeding after a reference rewrite) now threads the new element explicitly. handling.rs shrinks to the MerkCache driver: a ChainStore impl over the transaction's cache (so planning sees earlier operations' uncommitted writes) and an applier that executes mutations in plan order, applying the caller's insert options to the primary write only. This is the keystone for apply_batch support: the batch preprocessor will drive the same planners over a state-plus-pending-ops view and turn plans into ordinary batch ops, so live and batched semantics cannot drift. Semantic preservation: the full 5183-test workspace suite passes unchanged. New unit tests drive the planners against an in-memory mock store — plan shapes for registration + primary ordering, identical-edge no-op, cascade order and consent, propagation rewrites with lazy cleanup, and the component budget — proving the core is driver-independent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
BatchApplyOptions gains propagate_backward_references — the per-batch opt-in mirroring the live flow's flag, active on GROVE_V4+ (the same activation that selects the live flagged insert; V4 is unreleased and this branch defines it, so batch support rides it rather than minting a new version — deviation from the posted plan, recorded here). Under the flag, ops carrying the three backward-references ITEM variants become valid, and a preprocessing pass expands the batch with the derived operations the live flow would perform, planned by the SHARED semantic core over a read-only pre-batch view (DbChainStore): - caller-supplied referrer lists are replaced with the stored ones (fresh inserts get empty lists — the caller-authority rule); - overwrites of registered targets append referrer rewrites and lazy referrer-list cleanups; - deletions and incompatible overwrites append consent-checked cascade deletions; - no-change overwrites and InsertIfNotExists over existing keys expand to nothing, mirroring the live delta gate. Derived writes carry their final combined node value hash and execute through the new internal GroveOp::ReplaceBackwardReferenceFamilyMember (tag 20; rejected when supplied by callers). The execute layer writes family items through PutWithProvidedValueHash, and in-batch reference resolution treats pending family ops by the chain-hash convention (logical hash for item terminals, forward-path follow for references). Fail closed beyond M2's scope: BidirectionalReference ELEMENT ops stay rejected (M3); derived operations colliding with the batch's own ops surface as consistency conflicts (M4 will specify merge rules); partial batches and estimated costs refuse the new op (M5). Master invariant, tested: for every covered scenario — fresh insert, chain propagation, sum-twin overwrite, delete cascade, plain-overwrite cascade — the batch produces the BYTE-IDENTICAL root hash of the live flagged flow, and verify_grovedb stays clean. Plus consent rejection, caller-list clearing, flag-off/pre-V4/derived-op/collision rejections. Full workspace suite: 5191 passing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Rereview of current head 4fd430a7: six actionable findings remain (two P1, four P2). The filtered bidirectional-reference suite and current CI are green, but focused regressions reproduce the integrity, restoration, and hop-budget failures described inline. The new M2 batch commit moved the batch anchor but did not address the hop-one logical-hash omission for ItemWithSumItemWithBackwardsReferences.
| // Same forward path means same target and same inverted path: | ||
| // the registration was just refreshed in place by the upsert | ||
| // above, and removing it here would strip the edge entirely. | ||
| if old_reference.forward_reference_path != reference.forward_reference_path { |
There was a problem hiding this comment.
[P1] Compare resolved target positions, not path encodings
Different ReferencePathType encodings can resolve to the same target. In that case this planner first schedules the replacement registration, then plan_remove_registration reads the pre-plan target and schedules a second write without that registration; the latter write wins and leaves the live edge without a reciprocal entry. I reproduced this through GroveDb::insert by retargeting SiblingReference(target) to AbsolutePathReference(same target): the call succeeded but the target ended with zero referrers instead of one. Compare resolved Position values or coalesce writes through a plan overlay.
There was a problem hiding this comment.
This is Claude. Fixed in b487ee7 — targets are now compared by RESOLVED position (path_from_reference_path_type on both edges), and a same-target re-encoding replaces the old entry in place on the registration write itself, so no stale removal write can race it. Reproduced your sibling→absolute scenario in two regression tests: a semantics-core unit test (plan shape: registration + primary only, one surviving entry with the new encoding's inversion) and the end-to-end same_target_retarget_with_different_encoding_keeps_the_registration (exactly one registration survives, graph verifies, propagation still fires). Being in the shared core, the fix covers the live and batch drivers alike.
🤖 Addressed by Claude Code
| // KVValueHashFeatureType (whose item variants get a | ||
| // recompute check above); accepting them here would | ||
| // let the bytes ride unbound on the carried hash. | ||
| if matches!( |
There was a problem hiding this comment.
[P1] Accept the node shape emitted for normal-tree Bidi references
The chunk producer maps KvRefValueHash to KVValueHash, including a BidirectionalReference stored in a normal tree, but this branch rejects BidirectionalReference bytes in exactly that shape. A legitimate producer-generated chunk therefore fails restoration with InvalidChunkProof. Emit an accepted feature-bearing node for Bidi references or align this arm with the existing reference-chunk trust model.
There was a problem hiding this comment.
This is Claude. Fixed in b487ee7 — the KVValueHash restore arm now rejects only the ITEM variants (whose bytes are bound by the KVValueHashFeatureType recompute) and accepts a BidirectionalReference, aligning with the plain-reference chunk trust model: its value hash embeds the resolved end-of-chain hash, which is not locally derivable, and it is authenticated by the chunk's node hashes like every provided hash in this shape. New state-sync round-trip test over a populated reference graph (state_sync_populated_bidirectional_reference_graph_round_trip), confirmed to fail with InvalidChunkProof without the fix.
🤖 Addressed by Claude Code
| // registering a referrer would ripple through chains. Sniff the | ||
| // type from the serialized bytes and recompute for that family | ||
| // only; everything else keeps the fast path unchanged. | ||
| if matches!( |
There was a problem hiding this comment.
[P2] Include the new sum-carrying twin in logical-hash recomputation
This hop-one shortcut handles the two older backward-reference item types but omits ItemWithSumItemWithBackwardsReferences. Its stored node hash includes the referrer list, while a dependent reference must commit to the stripped logical hash. A batch reference to the twin succeeds but leaves verify_grovedb reporting an inconsistent commitment. Please include the third family member, preferably through a shared family predicate.
There was a problem hiding this comment.
This is Claude. Fixed in b487ee7 — added ElementType::is_backward_references_item() as the shared family predicate and switched the hop-1 shortcut (and the chunk-restore guard) to it, so the twin takes the stripped-logical-hash recompute like its siblings and a future family member cannot silently miss a dispatch. batch_hop_one_references_commit_the_logical_hash_of_family_targets now also covers a hop-1 reference to the sum-carrying twin with verify_grovedb clean.
🤖 Addressed by Claude Code
| Element::Reference(reference_path, ..) | ||
| | Element::ReferenceWithSumItem(reference_path, ..) => { | ||
| | Element::ReferenceWithSumItem(reference_path, ..) | ||
| | Element::BidirectionalReference(BidirectionalReference { |
There was a problem hiding this comment.
[P2] Preserve the source BidirectionalReference max_hop
This destructuring keeps only the forward path and invokes traversal with the global default, so direct get can follow more edges than the source permits; a max_hop = 1 source resolved through a second reference in a focused regression. Aggregate-sum has the same loss at its initial dispatch and also ignores the max_hop retained on intermediate references. Route every read surface through the same budget-aware traversal and pass the source limit.
There was a problem hiding this comment.
This is Claude. Fixed in b487ee7 — get_caching_optional now dispatches bidirectional references through follow_reference_with_max_hop with the source edge's declared budget, and the aggregate-sum surface applies the same treatment at its initial dispatch (source budget seeds hops_left) and its intermediate-edge handling (per-edge caps). Plain references keep their historical global-budget behavior on all surfaces. Tests: direct-get assertions in per_edge_max_hop_is_enforced_on_reads and budget cases in sum_queries_resolve_backward_references_sum_items.
🤖 Addressed by Claude Code
| // much of the global budget remains. `hops_left` is | ||
| // decremented below, so an edge declaring `max_hop: 1` | ||
| // permits exactly its own hop and no further reference. | ||
| if let Some(edge_budget) = reference.max_hop { |
There was a problem hiding this comment.
[P2] Do not consume an intermediate edge budget before fetching its target
For head -> mid(max_hop=1) -> terminal, this caps hops_left to one after mid has already been fetched, advances to terminal, and then decrements to zero before fetching that terminal. The valid direct edge therefore returns ReferenceLimit. Track remaining outgoing edges separately or adjust the cap/decrement order; also test that one direct terminal is allowed while a second outgoing reference is rejected.
There was a problem hiding this comment.
This is Claude. Fixed in b487ee7 — the mid-chain cap now accounts for the already-paid fetch of the edge's own node: hops_left = min(hops_left, edge_budget + 1) before the decrement, leaving exactly edge_budget further fetches. A max_hop: 1 edge reaches its direct terminal and rejects any reference beyond it — both directions tested in per_edge_max_hop_is_enforced_on_reads (direct terminal resolves; a second outgoing reference returns ReferenceLimit), with the aggregate-sum loop getting the equivalent ordering fix.
🤖 Addressed by Claude Code
| "NonCounted cannot wrap another wrapper", | ||
| )); | ||
| } | ||
| if matches!( |
There was a problem hiding this comment.
[P2] Mirror this family guard in into_non_counted
new_non_counted correctly rejects the bidirectional/backward-reference family here, but Element::into_non_counted below falls through to Ok(NonCounted(Box::new(other))) for the same elements. Wrapper validation and serialization then reject the value, so the public conversion helper returns Ok containing an unusable element. Have the fallback delegate to new_non_counted or duplicate this family guard.
There was a problem hiding this comment.
This is Claude. Fixed in b487ee7 — into_non_counted's fallback now delegates to new_non_counted, so the backward-references family guard (and any future guard) applies on both construction paths. Test: into_non_counted_rejects_backward_references_family covers all three item variants.
🤖 Addressed by Claude Code
The preprocessor now covers the whole family. Under BatchApplyOptions::propagate_backward_references it simulates one canonical sequential order over an overlay of pending position states (pre-batch DB state plus the batch's staged effects): pass 1 stages every non-reference op in user order and plans item-family and bidi-position bookkeeping (deletes and overwrites of positions holding bidirectional references now cascade / deregister exactly like the live flow); pass 2 processes BidirectionalReference ops in topological order - targets before their referrers - so references to targets created in the SAME batch work regardless of op order, whole chains can be created in one batch, and the hop/component budgets are validated against the prospective post-batch state. All decisions come from the shared semantic core (M1), so live and batched semantics cannot drift. User reference ops convert into the internal derived write (ReplaceBackwardReferenceFamilyMember) with the end hash resolved through the overlay; identical-edge re-inserts convert into nothing, mirroring the live no-op; registrations onto targets written in the same batch merge into the target op's element. Pending family ops became resolvable by other in-batch references through follow_reference_get_value_hash (stripped logical hash for item terminals, forward-path follow for references). Conflict rules (M4), specified and fail closed: a reference inserted in the same batch that deletes its target -> error; a cascade deleting a position another op touches -> error; a propagation rewrite hitting a user delete -> error; RefreshReference on a bidi position -> rejected. InsertWithKnownToNotAlreadyExist over an existing family position now errors instead of silently skipping bookkeeping. Master invariant, tested across the scenario matrix (fresh inserts, in-batch targets and chains in any order, retargets with and without upstream referrers, identical edges, bidi deletes and overwrites, two refs to one target, ref-plus-target-overwrite): batch and live flagged execution produce BYTE-IDENTICAL root hashes, and verify_grovedb stays clean. 24 tests in the batch suite; full workspace green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Versioned exactly like the apply path: new FeatureVersions average_case_backward_references_fan_out / worst_case_backward_references_fan_out are 0 on V1..V3 (family ops estimated as plain elements, the derived op refused - byte-stable for replay of historical admission decisions) and 1 on GROVE_V4. On V4, under BatchApplyOptions::propagate_backward_references, ops carrying family elements and deletes charge the derived fan-out (registration incl. the target's node growth, chain propagation, cascade deletion), with counts bounded by the apply path's budgets: <=32 referrers per item, <=10-hop chains, 1 referrer per reference. The worst-case model charges the full bound (every referrer chain rewritten, each in its own subtree with its own merk propagation, biggest-node sizes); the average model charges a small documented typical shape (like the MMR trailing-ones average) sized from the declared layer. The internal ReplaceBackwardReferenceFamilyMember op gets a real model (same-size replace plus the combine calls) instead of a refusal. Contract tests: worst-case estimate >= actual for a flagged registered- target overwrite, a delete cascade, and a reference insert whose target is created in the same batch; fan-out terms activate only under the flag and only for family elements; pre-V4 estimates are byte-identical with and without the flag. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… read budgets, wrapper guard Six findings from the 2026-08-29 review round: - [P1] plan_reference_insertion compared forward-path ENCODINGS to decide whether the old registration must be removed, so retargeting an edge onto the SAME target under a different ReferencePathType encoding scheduled a stale removal write (computed from the pre-plan target) that raced and won over the registration, leaving the live edge with zero referrers. Targets are now compared by RESOLVED position, and a same-target re-encoding replaces the entry in place on the registration write itself. Being in the shared semantic core, the fix covers the live and the batch drivers alike. - [P1] The chunk producer emits a BidirectionalReference stored in a normal tree as a KVValueHash node (the KvRefValueHash mapping), which the restorer categorically rejected — legitimate chunks failed to restore. The rejection now covers only the ITEM variants (whose bytes are bound by the KVValueHashFeatureType recompute); a bidi reference keeps exactly the trust model chunks give plain references, since its value hash embeds the non-locally-derivable end-of-chain hash. State sync round-trip test over a populated reference graph, confirmed to fail without the fix. - [P2] The batch hop-1 fast path special-cased two of the three backward-references item variants; a hop-1 reference to the ItemWithSumItemWithBackwardsReferences twin baked the combined hash instead of the stripped logical hash. All family dispatch-by- serialized-type sites now share ElementType::is_backward_references_item. - [P2] Direct get dropped the source bidi edge's declared max_hop (resolving further than the edge permits), and the mid-chain per-edge cap consumed the already-paid fetch of the edge's own node — a max_hop-1 edge with a DIRECT terminal returned ReferenceLimit. The source edge now threads its budget into follow_reference_with_max_hop and the mid-chain cap allows exactly edge_budget further fetches. The aggregate-sum surface gets the same treatment at its initial dispatch and its intermediate-edge handling. - [P2] Element::into_non_counted's fallback bypassed new_non_counted's backward-references family guard, returning Ok with an element that wrapper validation and serialization then refuse; it now delegates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
This is Claude. Batching milestones M3–M5 are complete at b487ee7, finishing the plan posted above — plus the six-finding review round from earlier today. M3 — bidirectional-reference ops in
|
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Re-review of the latest changes found four new merge-blocking issues. All four were confirmed on b487ee7b with focused regressions; the prior six findings are resolved.
| }, | ||
| ); | ||
| } else { | ||
| *op_element = element.clone(); |
There was a problem hiding this comment.
[P1] Do not fold derived rewrites into arbitrary user ops
This collision path accepts unconditional and conditional writes alike, then replaces every non-bidirectional payload with the derived element. An existing-key plain InsertIfNotExists can therefore swallow a required reverse registration even though it never executes, while a propagation colliding with a later plain overwrite can silently discard that caller write.
I reproduced both outcomes: one successful batch left a new forward reference with zero reverse registrations; another left an intermediate reference live after the caller requested a plain overwrite. Restrict in-place merging to compatible family writes that are guaranteed to execute; otherwise preserve sequential semantics or reject the collision. Please add both order-sensitive regressions and compare their roots with equivalent sequential execution.
There was a problem hiding this comment.
This is Claude. Fixed in 6478dbb — in-place merging is now restricted to already-processed, guaranteed-to-execute family writes. Your two repros are regression tests: (1) InsertIfNotExists over an existing key is dropped outright for ANY payload, so the registration lands as a derived op instead of being swallowed (batch_no_op_insert_if_not_exists_does_not_swallow_registration, asserts the entry exists plus root parity with the sequential twin); (2) a propagation colliding with a later plain overwrite stays a derived op that the later op's own processing supersedes — dropping it and planning the dereg its overwrite requires — so the caller's write wins with sequential semantics (batch_later_plain_overwrite_supersedes_propagation, both op orders, root-parity-checked against sequential live twins). Collisions with deletes/refreshes stay fail-closed.
🤖 Addressed by Claude Code
| let path_slices: Vec<&[u8]> = path.iter().map(|p| p.as_slice()).collect(); | ||
| let merk = cost_return_on_error!( | ||
| &mut cost, | ||
| self.db.open_transactional_merk_at_path( |
There was a problem hiding this comment.
[P1] Model same-batch-created subtrees in the overlay
For an item below a subtree created earlier in the same batch, the position is absent from this element overlay, so this fallback attempts to open the path from committed storage. The staged parent tree is not committed yet and the otherwise valid batch fails with InvalidParentLayerPath before the normal batch engine can create and populate it.
Track prospective subtree existence in the overlay or avoid committed lookup for paths known to be created by the batch. Please cover both ordinary and backward-reference-family items below newly created subtrees.
There was a problem hiding this comment.
This is Claude. Fixed in 6478dbb — a pre-scan over all ops (shallowest paths first, so nesting works regardless of op order) marks tree writes with no committed tree as FRESH subtrees; element_at at or below them resolves through the overlay alone and never opens committed storage. batch_populates_a_subtree_created_in_the_same_batch covers ordinary items, family items, and references under a new subtree plus a nested new subtree, in forward AND fully reversed op order (byte-identical roots), with the reciprocity audit clean. One note: exact root parity with a sequential live twin is deliberately not asserted for fresh construction — batch-built and sequentially-built merks legitimately differ in tree shape when building a fresh tree (I verified this holds for plain unflagged 2-item batches on develop-era code too), so the master parity invariant remains scoped to deltas on existing trees.
🤖 Addressed by Claude Code
| // A delete cannot see what it deletes; under the flag it | ||
| // may cascade, so it charges the full item fan-out. | ||
| | None => Some(super::BackwardReferencesFanOut::worst_item()), | ||
| Some(_) => None, |
There was a problem hiding this comment.
[P1] Charge fan-out from possible displaced state
Fan-out is classified solely from the proposed element, so a plain overwrite receives no surcharge. Actual preprocessing still loads the stored value and may discover a registered family element requiring propagation or cascade work.
On a stored two-link chain, a flagged plain InsertOrReplace estimated 18 seeks and actually used 19, so OperationCost::worse_or_eq_than was false. The missing work scales with stored fan-out. Every overwrite-capable operation needs the maximum possible displaced-state read and fan-out unless conservative previous-state metadata is supplied. Please test all overwrite variants component by component.
There was a problem hiding this comment.
This is Claude. Fixed in 6478dbb — classification no longer trusts the proposed element: every overwrite-capable op and delete under the flag charges the displaced-state item bound (a plain payload can land on a registered family element), and a BidirectionalReference insertion charges its reference terms ON TOP of that bound (it can displace a registered item too). Applied to both worst and average models, GROVE_V4-gated as before. Your failing case is now the contract test worst_case_estimate_covers_flagged_plain_overwrite_of_registered_target (plain InsertOrReplace over a stored two-link chain, worst ≥ actual component-wise), and fan_out_terms_activate_only_with_the_flag now pins that plain writes DO pick up the bound under the flag while staying byte-stable without it and pre-V4.
🤖 Addressed by Claude Code
| // inverted forward path (bounded by the reference's own | ||
| // encoded size plus a key-sized re-anchoring term) and | ||
| // the cascade flag with framing. | ||
| let entry_bound = reference |
There was a problem hiding this comment.
[P1] Bound the serialized inverted registration
The registration bound uses the forward reference's serialized size plus one key, but an absolute inverted reference serializes the referrer's complete qualified origin path. A deep origin can therefore point to a shallow target with a small forward encoding while creating a much larger reverse entry.
With an eight-level origin using 200-byte segments, worst-case added bytes were 459 and actual added bytes were 1,790, so the component-wise bound failed. Size the inverted reference from the operation path and key, or charge a conservative maximum origin-path bound in both average and worst-case models. Please cover every ReferencePathType and maximally asymmetric paths.
There was a problem hiding this comment.
This is Claude. Fixed in 6478dbb — the registration-growth bound is now sized from the op's own qualified origin: every ReferencePathType::invert output is built from subsets of the origin's path segments plus small scalars (verified against all seven variants — the absolute inversion carries every segment plus the key, the upstream/cousin forms carry subsets), so the bound sums the op path's segment widths plus the key width plus framing. worst_case_cost/average_case_cost gained the op-path parameter to see those widths. Your asymmetric repro is the contract test worst_case_estimate_covers_deep_origin_registration_growth: an eight-level origin with 200-byte segments registering on a shallow target through a small absolute forward encoding, worst ≥ actual component-wise including added bytes.
🤖 Addressed by Claude Code
…aced-state and inverted-path cost bounds Four P1 findings from the 2026-08-30 review round: - Derived rewrites no longer fold into arbitrary user ops. In-place merging is restricted to already-processed, guaranteed-to-execute family writes; InsertIfNotExists over an existing key writes nothing and is dropped outright (any payload), so a registration can never be swallowed by an op that never lands; and a rewrite colliding with a write op LATER in the canonical order stays a derived op that the later op's own processing supersedes (dropping it and planning the bookkeeping its overwrite requires) — the caller's write wins with sequential semantics, in either op order. Regression tests: the swallowed-registration repro and the superseded-propagation repro, both root-parity-checked against sequential live twins. - The overlay models same-batch-created subtrees. A pre-scan (shallowest paths first, so nesting works in any op order) marks tree writes with no committed tree as FRESH; reads at or below them resolve through the overlay alone instead of failing with InvalidParentLayerPath on the not-yet-committed parent. A flagged batch can now create a subtree and populate it with ordinary items, family items, and references in one go. Exact root parity with a sequential twin is deliberately not asserted for fresh construction — batch-built and sequentially-built merks legitimately differ in shape (a long-standing property of plain unflagged batches too, verified); the test asserts semantic equivalence, order-insensitivity, and a clean reciprocity audit. - Estimation charges the DISPLACED-state fan-out. Classification by the proposed element alone missed that a flagged plain overwrite (or any write) can land on a registered family element whose propagation or cascade the preprocessor performs; every overwrite-capable op and delete under the flag now charges the item bound (references charge it on top of their own terms). The reviewer's failing case — a plain InsertOrReplace over a stored two-link chain — is a worst>=actual contract test. - The registration-growth bound is sized from the op's own qualified path. Every ReferencePathType::invert output is built from subsets of the origin's path segments plus small scalars, so the bound sums the op path's segment widths plus the key (an absolute inversion carries them all) — a deep origin pointing at a shallow target through a tiny forward encoding is now covered. worst_case_cost/average_case_cost gained the op-path parameter. Contract test: eight-level, 200-byte segment origin registering on a shallow target, worst >= actual. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| // are dropped at processing time, so a retained op | ||
| // here always writes. | ||
| let mergeable_into_user_op = retained_user_op | ||
| .filter(|&index| index < current_index) |
There was a problem hiding this comment.
[P1] Merge stale cleanup into the current family write
A flagged overwrite of a family item containing a dangling registration produces a cleanup mutation for the same item. Because index == current_index fails this strict comparison, the cleanup becomes a second operation and the otherwise valid batch is rejected with InvalidBatchOperation. I reproduced this through the public APIs by registering a referrer, deleting it through the supported unflagged batch path, and then submitting one flagged target overwrite. Treat the currently processed family write as mergeable and retain the lazily cleaned referrer list.
There was a problem hiding this comment.
This is Claude. Fixed in 6398dc1 — the currently processed op now counts as processed in the merge condition (index <= current_index), so its own plan's stale-entry cleanup folds into the op instead of becoming a second op on the same position. This is safe precisely because the only mutation a plan can aim at its own position is that cleanup (the element it writes minus the dangling entry). Your repro is the regression test batch_flagged_overwrite_folds_own_stale_cleanup: referrer registered, removed through the supported unflagged batch path, then one flagged target overwrite — succeeds, retains the lazily-cleaned list, and root-parity-matches the live twin.
🤖 Addressed by Claude Code
| | GroveOp::Replace { element } | ||
| | GroveOp::InsertIfNotExists { element, .. } | ||
| | GroveOp::InsertWithKnownToNotAlreadyExist { element } | ||
| if element.is_any_tree() => |
There was a problem hiding this comment.
[P1] Include Patch in fresh-subtree detection
Patch is executed as a general element write and can create a tree, but this pre-scan omits it. A flagged batch containing Patch(empty_tree) plus a child write consequently reads below a parent absent from committed storage and fails with InvalidParentLayerPath. I reproduced the failure through QualifiedGroveDbOp::patch_op and public apply_batch; the equivalent InsertOrReplace form is covered by the new passing test. Include tree-valued Patch operations in tree_write_positions.
There was a problem hiding this comment.
This is Claude. Fixed in 6398dc1 — tree-valued Patch ops are included in the fresh-subtree pre-scan. Test batch_patch_created_subtree_is_fresh runs patch_op(empty_tree) plus a child write through public apply_batch under the flag and verifies the child is readable with a clean audit.
🤖 Addressed by Claude Code
| Self { | ||
| rewrites: BACKWARD_REFERENCES_WORST_ITEM_FAN_OUT, | ||
| resolution_loads: BACKWARD_REFERENCES_WORST_ITEM_FAN_OUT, | ||
| propagations: BACKWARD_REFERENCES_WORST_ITEM_FAN_OUT, |
There was a problem hiding this comment.
[P3] Bound foreign subtree propagation depth
The fixed fan-out bounds reference-graph breadth but not the Grove depth of each derived referrer write. With one valid absolute-path referrer 1,200 subtrees deep, the V4 worst-case estimator reported seek_count = 3218 while the identical flagged overwrite used seek_count = 3614, so worse_or_eq_than returned false. A depth-64 negative control remained covered. Enforce a global Grove path-depth limit and charge it per derived propagation, or require enough layer-chain information to bound every foreign derived position and reject estimation when it is unavailable.
There was a problem hiding this comment.
This is Claude. Fixed in 6398dc1, via your first suggested option — a depth limit charged per derived propagation. New rule in the shared semantic core (live and batch flows alike): both ends of a bidirectional edge must sit at most MAX_BACKWARD_REFERENCES_GROVE_DEPTH = 32 subtree levels deep, enforced at registration; since every later derived write (propagation rewrite, cascade deletion, registration cleanup) lands at an edge position, the bound covers all derived positions transitively. The estimators then charge the ancestor walk per derived propagation: worst case charges the full 32-level bound at biggest-node sizes, average a typical 2-level walk at declared-layer sizes. Your depth-1200 referrer is now rejected up front (batch_registration_depth_is_bounded covers depth 33 rejected in batch and live flows, depth 32 accepted with a clean reciprocity audit), and everything below the bound is charged for. The rule and its rationale are documented in the ADR and the model header. I scoped the limit to bidirectional-edge positions rather than a global Grove depth limit — constraining unrelated inserts seemed out of scope for this PR; happy to generalize if you'd prefer a global limit.
🤖 Addressed by Claude Code
…ees, bounded propagation depth Three findings from the 2026-08-30 second wave: - A flagged overwrite of a family item carrying a DANGLING registration plans a stale-entry cleanup targeting the op's own position; the strict `index < current_index` merge condition pushed that cleanup into a second op on the same position, rejecting the valid batch. The currently processed op now counts as processed (`<=`) — its own plan's cleanup folds into it. Regression test builds the dangling state through the supported unflagged batch path and root-parity checks the flagged overwrite against the live twin. - `Patch` executes as a general element write and can create a tree, but the fresh-subtree pre-scan omitted it, so Patch(tree) + child write failed with InvalidParentLayerPath. Tree-valued Patch ops are now in the pre-scan. Test: patch_op(empty_tree) + child insert. - Foreign-subtree propagation depth is now bounded and charged. New rule (shared core, live + batch): both ends of a bidirectional edge must sit at most MAX_BACKWARD_REFERENCES_GROVE_DEPTH = 32 subtree levels deep, enforced at registration — every later derived write lands at an edge position, so the bound covers propagation, cascade, and cleanup positions transitively. The estimators charge the ancestor walk per derived propagation: worst case charges the full 32-level bound at biggest-node sizes, average charges a typical 2-level walk at declared-layer sizes. This closes the reported escape (a valid referrer parked 1200 subtrees deep out-costing the fixed estimate): deeper referrers are now rejected up front, and everything below the bound is charged for. Tests: depth 33 rejected in batch and live flows, depth 32 accepted with a clean reciprocity audit; ADR updated with the rule and its rationale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| // registration rule bounds every bidirectional-edge position to | ||
| // `MAX_BACKWARD_REFERENCES_GROVE_DEPTH` levels, so that many | ||
| // biggest-node ancestor updates is a true ceiling. | ||
| for _ in 0..crate::bidirectional_references::MAX_BACKWARD_REFERENCES_GROVE_DEPTH { |
There was a problem hiding this comment.
[P3] Charge propagation within every ancestor Merk
The depth cap bounds how many parent Merks a foreign rewrite crosses, but this loop charges only one node load and layered replacement per ancestor. Actual batch bubbling executes the operation in every parent Merk and propagates the changed tree element through that Merk to its root. With maximum legal fan-out across distinct depth-32 branches and populated ancestor Merks, the omitted height-dependent work can exceed the proxy, so the worst-case estimate is not a true upper bound.
Charge a complete parent-Merk open and worst-case propagation per level, or require foreign-path layer information and reject estimation when it is unavailable. Please add componentwise estimate-vs-actual coverage for the maximum shape.
There was a problem hiding this comment.
This is Claude. Fixed in a2f345a — each ancestor level now charges the complete bubbling step: the parent-Merk open, the changed tree element's biggest-node load and layered rewrite, and the declared layer's full worst-case in-Merk propagation (the declared-layer contract now explicitly covers ancestor Merks of the component's subtrees; the average model charges the same per-level shape at declared sizes over its 2-level walk). One consequence worth noting: at the maximum legal shape the true bound exceeds the u32 cost domain (~hundreds of derived propagations × 32 levels × biggest-node propagation), so the worst model computes the per-level unit once and scales it with saturating arithmetic — sound because any REAL batch's actual cost must itself fit that domain, so a saturated estimate still dominates every actual. Your requested componentwise coverage is the contract test worst_case_estimate_covers_max_fan_out_deep_component: 32 referrers on one target, each in its own depth-32 branch with a populated ancestor Merk at every level, worst ≥ actual for the flagged overwrite.
🤖 Addressed by Claude Code
The depth-bounded ancestor walk charged one node load and one layered tree-element rewrite per level, but actual batch bubbling also propagates the changed element THROUGH each ancestor Merk to its root - height-dependent work that could exceed the estimate at maximum fan-out across populated ancestor Merks. Each ancestor level now charges the complete bubbling step: the parent-Merk open, the changed tree element's biggest-node load and layered rewrite, and the declared layer's full worst-case in-Merk propagation (the declared layer's contract now explicitly covers ancestor Merks of the component's subtrees). The average model charges the same per-level shape at declared-layer sizes over its typical 2-level walk. At the maximum legal shape (hundreds of derived propagations x the full 32-level registration depth x biggest-node propagation) the true bound exceeds the u32 cost domain, so the worst model computes the per-level unit once and scales it with saturating arithmetic - sound because any REAL batch's actual cost must itself fit that domain, so a saturated estimate still dominates every actual (ordinary += would panic on overflow in debug builds instead). Contract test worst_case_estimate_covers_max_fan_out_deep_component pins the reviewer's maximum shape: 32 referrers on one target, each in its own depth-32 branch with populated ancestor Merks at every level, worst-case estimate >= actual componentwise for the flagged overwrite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Rereview of head a2f345afcea02cf4e86bc1dae11b72ea0fdbff2d. The previously reported ancestor-Merk propagation-cost issue is resolved. I found the nine inline issues below; CI is currently green.
| kv_digest_to_kv_hash(key.as_slice(), value_hash) | ||
| .flat_map(|kv_hash| compute_hash(self, kv_hash)) | ||
| } | ||
| Node::KVBackwardsReferencesValueHash(key, value, backrefs_hash) => { |
There was a problem hiding this comment.
[P1] Enforce ordering for the new proof node
KVBackwardsReferencesValueHash contains a key but is missing from both ordering patterns. A malicious proof can push an authenticated parent before its real left child and attach it with ChildInverted; the reconstructed root still matches, but an exact query for the child is consumed as absent while visiting the parent. Include this variant in both Push and PushInverted ordering checks and add a parent-before-child regression.
There was a problem hiding this comment.
This is Claude. Fixed in e67e3d6 — KVBackwardsReferencesValueHash is now in BOTH the Push and PushInverted key-ordering patterns. Regression test backwards_references_nodes_enforce_key_ordering covers the parent-before-child shape in both directions (parent pushed first + ChildInverted, and the inverted-order dual), asserting the ordering rejection.
🤖 Addressed by Claude Code
| ); | ||
| batch_operations.push(( | ||
| key_info.get_key(), | ||
| Op::PutWithProvidedValueHash( |
There was a problem hiding this comment.
[P2] Recompute hashes after flags updates
This derived operation carries a node hash computed before Merk application, but the public flags callback can subsequently change and reserialize the element. put_value_with_provided_value_hash then installs the stale supplied hash, committing bytes that no longer match their authenticated value hash. Recompute the family hash after mutation or reject flag changes for provided-hash operations.
There was a problem hiding this comment.
This is Claude. Fixed in e67e3d6, via your reject option — put_value_with_provided_value_hash now fails closed if the just-in-time value update (the flags carry-over or the public flags callback) actually changes the bytes: the caller of a provided-hash write supplies the element with its authoritative flags already in place (the planners carry stored flags through), so a real mutation is a contract violation that would break the bytes/hash binding. Test batch_flags_mutation_on_derived_rewrite_fails_closed covers both an inert callback (the derived rewrite goes through, audit clean) and a mutating one (the batch errors and nothing mismatched is committed).
🤖 Addressed by Claude Code
| expansion.store.stage(position, Some(element)); | ||
| } | ||
| } | ||
| GroveOp::Delete | GroveOp::DeleteTree(..) => { |
There was a problem hiding this comment.
[P2] Reconcile descendants before recursive deletion
The flagged preprocessor treats DeleteTree like deletion of only the tree element, while later cleanup recursively clears every descendant subtree. Nested bidirectional references therefore disappear without removing external registrations, enforcing cascade policy, or updating surviving referrers. Enumerate family edges crossing the deletion boundary and plan their mutations atomically before clearing storage.
There was a problem hiding this comment.
This is Claude. Fixed in e67e3d6, fail closed — a flagged batch now refuses to delete a NON-EMPTY subtree (committed content, staged content, or content of a subtree the batch itself created), with the error directing callers to the live flagged delete, whose descendant walk performs the full bookkeeping, or to empty the subtree first. I chose refusal over in-preprocessor enumeration deliberately: batch deletion of populated subtrees has known pre-existing element-cleanup gaps even unflagged, so simulating the visitor's cascade semantics on top of that engine behavior would paper over an unsound base. Empty subtrees still delete; ADR documents the limitation. Test: batch_flagged_non_empty_subtree_deletion_is_refused (pre-existing content, same-batch-created content, and the still-working empty case).
🤖 Addressed by Claude Code
| // (the two-layer scheme's combine for items; for referrer | ||
| // rewrites additionally combined with the resolved end | ||
| // hash). | ||
| GroveOp::ReplaceBackwardReferenceFamilyMember { |
There was a problem hiding this comment.
[P2] Mirror the Provable-tree family guard
Direct insertion rejects backward-reference-family elements in Provable aggregate trees because no proof node binds both their combined value hash and the aggregate. This derived batch arm persists the family member without an equivalent host-tree guard, so batch insertion can create an unsupported combination. Apply the shared insertability validation here for all four Provable aggregate hosts.
There was a problem hiding this comment.
This is Claude. Fixed in e67e3d6 — the derived-rewrite execution arm now runs the shared validate_insertable_into host-tree guard before persisting, mirroring direct insertion's rejection of the item family in all four Provable aggregate hosts. Note this is defense in depth rather than a reachable path today: derived writes only land on family items (which cannot exist in Provable hosts, per the direct-insert guard) or bidirectional references (which ARE supported there), and the internal op is rejected when supplied by callers — but the arm no longer relies on that reasoning holding forever.
🤖 Addressed by Claude Code
| &mut |proof_node, node_traversal_instruction, parent_key| { | ||
| match &proof_node.node { | ||
| Node::KVValueHashFeatureType(key, value, vh, feature_type) => { | ||
| // A backward-references ITEM's stored value hash is |
There was a problem hiding this comment.
[P3] Keep failed chunk processing retryable
For a non-root chunk, the parent link is committed and its parent_keys entry removed before write_chunk performs this new family bytes/hash validation. If validation fails, the expected hash remains but a valid retry fails because its parent key is gone. Validate before mutating the parent, or stage both changes atomically and roll back the maps on error.
There was a problem hiding this comment.
This is Claude. Fixed in e67e3d6 — the parent-link rewrite (and the root-chunk base-root-key set) now run only AFTER write_chunk and its validations succeed: the needed key/aggregate are captured before the write consumes the tree, and parent_keys is only consumed on the success path, so a failed chunk leaves both maps intact for a valid retry. (chunk_id_to_root_hash already behaved this way; the parent bookkeeping now matches it.)
🤖 Addressed by Claude Code
| // resolution seeds its visited set with the starting qualified path — | ||
| // so a cycle that would only materialize AFTER the write is rejected | ||
| // before any mutation. | ||
| let (target_value_hash, downstream_hops) = |
There was a problem hiding this comment.
[P3] Validate the new edge's max_hop
The planner computes downstream_hops but compares it only with the global ten-hop budget. It therefore accepts max_hop: 0 even for a direct target, or max_hop: 1 for a two-hop chain, although public reads deterministically return ReferenceLimit. Reject prospective edges whose downstream length exceeds their declared budget.
There was a problem hiding this comment.
This is Claude. Fixed in e67e3d6 — plan_reference_insertion rejects an edge whose measured downstream chain exceeds its own declared max_hop (BidirectionalReferenceRule), so dead edges that reads would deterministically refuse never persist; being in the shared core it covers live and batch flows. Tests: bidi_insert_rejects_undersized_max_hop (max_hop 0 vs a direct target, max_hop 1 vs a two-hop chain, in both flows) plus the direct-target acceptance; the existing read-budget tests now build their out-of-budget states through post-insert evolution (a target overwritten into a plain reference), which remains the one legitimate way such edges arise. ADR rule added.
🤖 Addressed by Claude Code
| }; | ||
| Element::Reference( | ||
| reference.forward_reference_path, | ||
| reference.max_hop, |
There was a problem hiding this comment.
[P3] Preserve max_hop during proof rewriting
The bidirectional reference is normalized into a plain Reference containing max_hop, but the following pattern discards that field and calls unrestricted follow_reference. Proof generation can consequently return a terminal value that ordinary get rejects. Preserve the budget and use follow_reference_with_max_hop in every result, filler, and count-offset branch.
There was a problem hiding this comment.
This is Claude. Fixed in e67e3d6 — the bidi normalization in every V1 proof-generation branch (result, filler, and count-offset/aggregate rewrite) now captures the edge's declared max_hop and dereferences through follow_reference_with_max_hop; plain references keep their historical global budget, and the V0 prover is untouched (bidi never reaches it). Test proof_generation_respects_bidi_max_hop: proving an edge that fell out of budget after insertion returns ReferenceLimit exactly like get.
🤖 Addressed by Claude Code
| ); | ||
| continue; | ||
| } | ||
| let Some((referrer_key, referrer_path)) = referrer_qualified.split_last() else { |
There was a problem hiding this comment.
[P3] Report an empty resolved inverse path
A corrupt AbsolutePathReference([]) resolves successfully to an empty qualified path, so it bypasses the error branch; split_last then returns None and the audit silently continues. Record this as an invalid inverse so verify_grovedb(..., verify_references=true) does not report a clean graph.
There was a problem hiding this comment.
This is Claude. Fixed in e67e3d6 — an inverse that resolves to an EMPTY qualified path (e.g. a corrupt AbsolutePathReference([])) is now recorded as an invalid-inverse issue instead of silently passing. Covered in verify_grovedb_reports_missing_forward_registration_and_empty_inverse (the forged entry is planted with a hash-consistent direct merk write, so only the audit can see it).
🤖 Addressed by Claude Code
| ); | ||
| } | ||
|
|
||
| if matches!(element, Element::BidirectionalReference(..)) { |
There was a problem hiding this comment.
[P3] Verify forward registrations too
This helper validates only backward entries that already exist. When walking a BidirectionalReference, the verifier checks its terminal hash and its own inbound list, but never verifies that its forward target contains this edge's inverse registration. Missing registrations therefore pass the advertised reciprocity audit; resolve the immediate target and check membership of the canonical inverse.
There was a problem hiding this comment.
This is Claude. Fixed in e67e3d6 — the audit now verifies the FORWARD direction: for every live BidirectionalReference it resolves the immediate target and requires the edge's canonical inverse (forward_path.invert(origin)) to be a member of the target's referrer list, reporting a ?missing-registration issue otherwise. One implementation subtlety worth noting: the membership read is merk-level, because every public read strips exactly the list this audit inspects. Test plants a hash-consistent stripped target (the ref commits to the LOGICAL hash, so nothing else notices) and asserts the report.
🤖 Addressed by Claude Code
…rence, audit directions, read-budget validation Nine findings (1 P1, 3 P2, 5 P3): - [P1] KVBackwardsReferencesValueHash joins BOTH Push/PushInverted key- ordering checks in proof execution; previously a malicious proof could push an authenticated parent before its real left child and make an exact query for the child read as absent. Parent-before-child regression in both directions. - [P2] A provided-value-hash write now fails closed if a just-in-time value mutation (flags carry-over or the public flags callback) changes the bytes mid-apply — committing them would break the bytes/hash binding the derived rewrite authenticated. Test covers both an inert callback (rewrite goes through) and a mutating one (batch fails, no mismatch committed). - [P2] A flagged batch refuses to delete a NON-EMPTY subtree: its descendants may hold bidirectional-reference participants whose external registrations, cascade consents, and surviving referrers the batch engine's wholesale clearing would skip. The live flagged delete (which walks descendants with full bookkeeping) remains the supported path; empty subtrees still delete. ADR documents the limitation. - [P2] The derived-rewrite execution arm applies the shared validate_insertable_into host-tree guard, mirroring direct insertion's Provable*-aggregate rejection of the item family (defense in depth — planners cannot currently produce such a write, since family items cannot exist in Provable hosts to begin with). - [P3] Chunk processing stays retryable: the parent-link rewrite and root-key set now run only AFTER write_chunk (and its family bytes/hash validation) succeeds, so a failed chunk no longer consumes the parent_keys entry a valid retry needs. - [P3] plan_reference_insertion rejects edges whose downstream chain exceeds their own declared max_hop (dead edges that reads would always refuse). Existing read-budget tests reworked to build their over-budget states through post-insert evolution. - [P3] Proof generation threads the bidi edge's declared max_hop through every dereferencing branch (result, filler, count-offset) via follow_reference_with_max_hop; proving an out-of-budget edge now returns ReferenceLimit exactly like get. Plain references keep their historical global budget. - [P3] The reciprocity audit reports a corrupt EMPTY resolved inverse path instead of silently skipping it. - [P3] The audit now verifies the FORWARD direction too: a live edge whose immediate target lacks the edge's canonical inverse registration is reported (merk-level read — public reads strip the very list the audit inspects). Test plants a hash-consistent stripped target that only this check can catch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Automated rereview of head e67e3d6 after the substantial round-5 changes. The nine prior findings are resolved in their reported forms. I found the two new inline issues below; all current CI checks are green.
| // public reads enforce `max_hop` deterministically, so an edge whose | ||
| // chain is already longer than its declaration would never resolve — | ||
| // reject it at insertion instead of persisting a dead edge. | ||
| if let Some(declared) = reference.max_hop |
There was a problem hiding this comment.
[P2] Revalidate upstream max_hop budgets on retarget
This new check validates only the edge being written. Suppose A(max_hop=2) -> B -> C is valid, then B is retargeted to D -> E with B's own budget admitting those two downstream hops. The upstream walk below still checks only the global ten-hop total, so the retarget succeeds and propagates the new terminal hash even though A now needs three hops; public get/proof calls through A deterministically return ReferenceLimit. While walking reciprocal referrers, compare each ancestor's declared max_hop with upstream_distance + downstream_hops and reject atomically; add live and flagged-batch regressions for this retarget.
There was a problem hiding this comment.
This is Claude. Fixed in b369cf5 — the upstream walk now checks each ancestor's own declared max_hop against upstream_distance + new_downstream_hops and rejects the write atomically (BidirectionalReferenceRule) before any mutation is planned. Being in the shared planner, live and flagged-batch retargets reject identically. Regression retarget_rejects_upstream_max_hop_violation builds your exact scenario — A(max_hop=2) -> B -> C valid, retarget B onto a two-hop tail — asserting rejection in both flows and that A still resolves through the unchanged chain.
🤖 Addressed by Claude Code
| } | ||
| let mut cost = OperationCost::default(); | ||
| let path_slices: Vec<&[u8]> = qualified.iter().map(|p| p.as_slice()).collect(); | ||
| let merk = cost_return_on_error!( |
There was a problem hiding this comment.
[P2] Charge the new subtree emptiness probe
subtree_has_content now opens the child Merk and calls is_empty_tree for every flagged DeleteTree, and that cost is accumulated by the real apply_batch path. Both estimators are unchanged: their DeleteTree arms still delegate directly to average_case_merk_delete_tree / worst_case_merk_delete_tree, which price the parent-Merk deletion but not this child-path open/root read. Estimated cost therefore understates actual work once per tree deletion (and Error/Skip may perform an additional later emptiness scan). Add the probe to both V4 estimator branches and cover estimated-versus-actual costs for all deletion behaviors.
There was a problem hiding this comment.
This is Claude. Fixed in b369cf5 — both V4 estimator branches now price the emptiness probe (a subtree open plus its root read; biggest-node sizes in the worst model, declared-layer sizes in the average model) on Delete AND DeleteTree of tree elements, and the DeleteTree arms — which indeed carried no fan-out at all — now wrap the displaced-state fan-out like Delete does. Contract test worst_case_estimate_covers_flagged_empty_tree_deletion pins the accepting path (worst ≥ actual componentwise); the Error/Skip deletion behaviors share the same probe-then-refuse prefix, whose cost the same term covers.
🤖 Addressed by Claude Code
…s-probe estimation Two P2 findings from the automated re-review of e67e3d6: - Retargets revalidate every UPSTREAM ancestor's declared budget: while walking the reciprocal referrer chain, each ancestor's max_hop is checked against upstream_distance + new downstream length and the write is rejected atomically on violation — previously A(max_hop=2) -> B -> C stayed valid while retargeting B onto a two-hop tail silently left A unresolvable (reads through A would deterministically hit ReferenceLimit). Shared-core check, so live and flagged-batch flows reject identically; regression covers both plus A's continued resolution through the unchanged chain. - The estimators charge the flagged deletion emptiness probe: the apply path opens the deleted tree's child subtree and reads its root before admitting a flagged Delete/DeleteTree, but neither estimator priced it (and the DeleteTree arms carried no displaced-state fan-out at all). Both arms now wrap the fan-out and add the two-node probe (biggest-node sizes in worst case, declared-layer sizes in average). Contract test: flagged empty-tree deletion, worst >= actual. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Automated rereview of head b369cf5 after the round-6 changes. Both findings from the preceding review are fixed. I found one new flagged-batch correctness issue below; all current CI checks are green.
| // B is retargeted onto a two-hop chain — reads | ||
| // through A would deterministically hit | ||
| // `ReferenceLimit`. | ||
| if let Element::BidirectionalReference(ref ancestor) = resolved.element |
There was a problem hiding this comment.
[P2] Validate hop budgets against pending upstream reference ops
In a flagged batch this resolves the stored/overlay ancestor, but pass 1 defers every BidirectionalReference write and pass 2 processes targets before referrers. Starting from A(max_hop=2) -> B -> C, a single batch that retargets B onto D -> E and raises A's max_hop to 3 has a valid final state, yet B is planned first and this check still sees stored A with budget 2, so the whole batch is rejected before A's update is staged. Retargeting A away from B in the same unordered batch can similarly make acceptance depend on user-op order. Consult pending referrer ops while walking upstream (or pre-stage their prospective edge/budget), and add paired-update tests in both input orders.
There was a problem hiding this comment.
This is Claude. Fixed in ba651b9 — the prospective-component budget now validates against PENDING edges in the same batch. ChainStore gains pending_reference_at (default None, so live flows are untouched); the batch overlay records every deferred pass-2 reference op's declared forward path and max_hop (cleared as ops are planned or dissolve), and the upstream walk lets a pending declaration supersede the stored edge: the raised budget is what gets checked, and an ancestor whose pending edge no longer points at the current position detaches the component (that op is validated at its own planning turn against its own new chain). batch_paired_upstream_updates_validate_against_pending_edges covers your both scenarios in BOTH op orders — retarget-B-plus-raise-A accepted with root parity against the live sequential twin, retarget-A-away frees B — plus the control that retargeting B alone stays rejected.
🤖 Addressed by Claude Code
Element::BidirectionalReference(BidirectionalReference) was the only variant whose flags lived inside its payload struct instead of as the trailing Option<ElementFlags> on the enum variant. That asymmetry made every generic flags path (get_flags family, flags-size cost arms, the JIT flags-update callbacks, wrapper validation) special-case the family - exactly the shape of several dispatch-miss findings this week - and put the flags mid-encoding instead of at the tail like every other variant. The variant is now BidirectionalReference(BidirectionalReference, Option<ElementFlags>) and the struct models pure edge semantics (forward path, referrer list, cascade, max_hop). The generic flags accessors handle it positionally like every other variant. The shared planner (plan_reference_insertion) and the live driver thread flags as an explicit parameter; identical-edge detection compares variant-level flags. Wire format for discriminant 25 changes (flags move to the encoding tail) - free today, since the family is GROVE_V4-only and unreleased. ADR updated. Purely mechanical beyond the planner signature; no behavior change. Full grovedb (3037), merk (730), element suites green; clippy and fmt clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
This is Claude. One API-shape change on top of the review fixes, at 2016027: 🤖 Generated with Claude Code |
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Fresh review of 2016027ddf886fbc434e6afd4aa5347851331c4a. One new actionable finding is attached inline; no additional security finding was confirmed.
| cascade_on_update: true, | ||
| max_hop: Some(5), | ||
| backward_references: Vec::new(), | ||
| flags, |
There was a problem hiding this comment.
[P1] Update all tests to the two-field variant
The production refactor removes flags from BidirectionalReference and makes Element::BidirectionalReference take (reference, flags), but this test module still constructs and matches the old shape (with further occurrences below), as do the serde test in element/mod.rs and the visualization tests. The pinned-head cargo check --workspace --all-features --all-targets job fails with 17 errors (E0425, E0560, E0061, and E0023), so neither linting nor any Ubuntu test shard can pass. Move each test flag into the enum's second field, update every one-field pattern, and assert flags from that field instead.
There was a problem hiding this comment.
This is Claude. Fixed in ba651b9 — every test construction and pattern across the element crate (the bidirectional_reference test module, the serde round-trip in element/mod.rs, and the visualization tests) now uses the two-field variant, with flags asserted from the enum field. Root cause of the CI failure: I validated the refactor with cargo check, which skips test targets — the pinned-head --all-targets job caught what I didn't. CI is running on the new head.
🤖 Addressed by Claude Code
…udget validation Two findings on the refactored head: - [P1] The grovedb-element TEST modules (bidirectional_reference tests, the serde round-trip, the visualization tests) still built the one-field variant with in-struct flags, failing the pinned-head --all-targets check and every test shard. All test constructions and patterns now use the two-field variant, with flags asserted from the enum field. (Root cause of the CI failure on 2016027: the refactor was validated with cargo check, which skips test targets.) - [P2] The prospective-component budget now validates against PENDING edges in the same unordered batch. ChainStore gains pending_reference_at (default None for live flows); the batch overlay tracks every deferred pass-2 reference op's declared forward path and max_hop, cleared as ops are planned or dissolve. During the upstream walk, a pending declaration at an ancestor position supersedes the stored edge: its (possibly raised) budget is what the check uses, and an ancestor whose pending edge no longer points at the current position detaches the component (that op is validated at its own turn). A batch that retargets B onto a longer tail AND raises A's max_hop — or retargets A away from B — is now accepted in either op order, with root parity against the live sequential twin; retargeting B alone stays rejected (control). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Fresh review of the full PR at ba651b9. I confirmed two remaining batch reference-planning defects below.
| | GroveOp::InsertIfNotExists { element, .. } | ||
| | GroveOp::InsertWithKnownToNotAlreadyExist { element } => { | ||
| if let Element::BidirectionalReference(reference, _) = element { | ||
| expansion.store.stage_pending_reference( |
There was a problem hiding this comment.
[P2] Do not stage conditional edges that will be skipped
This declaration is visible before InsertIfNotExists is gated on the stored value. With stored A(max_hop=2) -> B -> C, submit a flagged batch that retargets B onto D -> E and also calls insert_if_not_exists_or_skip_op for the already-existing A with max_hop=3. Targets-first ordering plans B first, so it consumes the pending 3-hop declaration and accepts the longer chain; A's turn then clears the declaration and drops the conditional op. The batch consequently commits A(max_hop=2) -> B -> D -> E, and reads/proofs through A deterministically hit ReferenceLimit. Gate these conditional ops before any pending declaration can be consumed (or stage only declarations proven to write), and cover both input orders.
There was a problem hiding this comment.
This is Claude. Fixed in 45343f8 — pending declarations are staged only for edges proven to write: InsertIfNotExists / InsertWithKnownToNotAlreadyExist reference ops advertise their edge only when the position is actually vacant at staging time (which is stable through planning — one op per position, and cascades refuse user-op positions), so a skip-gated "raise" leaves the STORED budget authoritative and your exact scenario is rejected. Regression batch_skipped_conditional_does_not_relax_upstream_budget covers both op orders and asserts the stored graph stays intact and clean.
🤖 Addressed by Claude Code
| ) | ||
| .map(|qualified| qualified == current_qualified) | ||
| .unwrap_or(false); | ||
| if !still_points_back { |
There was a problem hiding this comment.
[P2] Exclude detached ancestors before counting their hop
upstream_hops was already incremented—and the global limit may already have broken the loop—before this branch discovers that the pending ancestor points away. For example, with stored A -> B, a same-batch retarget of A away from B should let B target a downstream chain of exactly MAX_REFERENCE_HOPS; the prospective B component is valid, but the loop counts the detached A as hop 1 and rejects 1 + MAX_REFERENCE_HOPS. Resolve the effective edge before incrementing/checking the hop (or undo the count on this branch), and add a boundary test at the full ten-hop limit.
There was a problem hiding this comment.
This is Claude. Fixed in 45343f8 — the walk now resolves the ancestor and determines its effective edge FIRST; only live component members increment upstream_hops and face the global-limit check, so a detached (or stale) ancestor consumes nothing. Your boundary case is the regression batch_detached_ancestor_frees_full_downstream_budget: stored A -> B, same-batch retarget of A away while B takes a downstream chain of exactly MAX_REFERENCE_HOPS — accepted in both op orders with a clean audit.
🤖 Addressed by Claude Code
…estor hop counting Two P2 refinements of the pending-upstream validation: - A conditional insert whose gate will SKIP it no longer advertises a pending edge. InsertIfNotExists / InsertWithKnownToNotAlreadyExist reference ops stage their declaration only when the position is actually vacant — otherwise the op writes nothing and the STORED edge stays authoritative, closing the escape where a skip-gated "raise A's max_hop" op let a same-batch retarget of B commit A(2) -> B -> D -> E with reads through A deterministically failing. (Existence at staging time is stable through planning: one op per position, and cascades refuse user-op positions.) Regression in both op orders, asserting the stored graph stays intact. - Detached ancestors no longer consume component budget. The upstream walk resolves the ancestor and determines its EFFECTIVE edge first; only live members increment upstream_hops and face the global-limit check. Previously the hop was counted (and the limit could break the loop) before discovering the pending ancestor points away, so with stored A -> B, retargeting A away in the same batch wrongly rejected B taking a downstream chain of exactly MAX_REFERENCE_HOPS. Boundary regression at the full ten-hop limit, both op orders. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
This is Claude. Closing in favor of #843, which carries this PR's exact content (plus the final CodeRabbit round) as two cleanly-attributed commits — Evgeny's ported feature and the subsequent redesign/hardening/batching work — so it can merge with a merge commit. All review history stays here for reference. 🤖 Generated with Claude Code |
4 participants
This PR introduces several new types of
Element: bidirectional reference, item and sum item that support bidirectional references pointing to them using backward references.Issue being fixed or feature implemented
This way GroveDB gets support of consistent references (and chains of references) that will receive updates in case the (in)direct pointed to element has changed.
What was done?
New
Elementvariants as well as new subsystem dedicated to reaching consistency of bidirectional references.How Has This Been Tested?
Breaking Changes
Bidirectional references are processed through
MerkCachethat in some cases reduces the costs, so a new version was used and those internals that diverged also has a bumped version to use. v1 API is unaffected.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes