Skip to content

Commit fb71003

Browse files
refactor: extract cidx mutates check into exhaustive GroveOp method
Structural guard against the nested-cidx bug class (commit a8bb34f). That bug existed because the pre-state capture in execute_ops_on_path used an inline `matches!()` against a hand-maintained list of mutating GroveOp variants. When `ReplaceCountIndexedTreeRootKeys` was added as part of cidx primary bubble-up support, the inline match wasn't updated — so the new variant silently fell through to "doesn't mutate" and the outer's secondary stayed stale. The bug passed H1-A integrity checks; only a manual audit caught it. The fix in a8bb34f added the variant to the list, but the list is still hand-maintained and the next new variant has the same trap. This commit converts the inline `matches!()` into a method `GroveOp::can_mutate_child_count(&self) -> bool` with an exhaustive `match` and no wildcard arm. Adding any new `GroveOp` variant now forces the author to classify it explicitly — the compiler will refuse to compile until they do. Same protection the existing `to_u8()` method already provides for ordering. Each variant has a comment explaining why it's `true` or `false`: - Leaf-level mutations (Insert/Replace/Patch/Delete/RefreshReference) → true: directly change a key's count_value. - Bubble-up ops (ReplaceTreeRootKey, InsertTreeWithRootHash, ReplaceNonMerkTreeRoot, InsertNonMerkTree, and the new ReplaceCountIndexedTreeRootKeys) → true: each updates the child element bytes which, for count-bearing trees, changes the aggregated count_value (the secondary's sort key). - Non-Merk-tree leaf inserts (Commitment/MMR/BulkAppend/DenseTree) → false: these trees use non-Merk storage and don't contribute counts the same way; their propagation is tree-specific. No behavior change. All 1576 grovedb tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent dd90e6b commit fb71003

1 file changed

Lines changed: 67 additions & 27 deletions

File tree

grovedb/src/batch/mod.rs

Lines changed: 67 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -425,6 +425,67 @@ impl GroveOp {
425425
GroveOp::ReplaceCountIndexedTreeRootKeys { .. } => 17,
426426
}
427427
}
428+
429+
/// True iff this op, when applied at a cidx primary's path, can
430+
/// change the `count_value` (or absence) of the element at the
431+
/// op's key — and therefore requires the cidx primary's secondary
432+
/// mirror to be updated for that key.
433+
///
434+
/// Used by `execute_ops_on_path`'s pre-state capture pass: only
435+
/// ops that may change a key's count_value are read from disk
436+
/// before apply, so the post-apply mirror knows the (old, new)
437+
/// count delta.
438+
///
439+
/// # Single source of truth — keep in sync with `GroveOp` variants
440+
///
441+
/// This method uses an **exhaustive `match`** with no wildcard
442+
/// arm. Adding a new `GroveOp` variant forces an explicit decision
443+
/// here; the compiler will refuse to compile until the variant is
444+
/// classified. This is the guard that prevents the nested-cidx
445+
/// mirror-bug class (commit a8bb34fb) from recurring: that bug
446+
/// existed because the original inline `matches!()` was a
447+
/// non-exhaustive check, so the newly added
448+
/// `ReplaceCountIndexedTreeRootKeys` variant silently fell through
449+
/// to "doesn't mutate" and the outer's secondary stayed stale.
450+
pub(crate) fn can_mutate_child_count(&self) -> bool {
451+
match self {
452+
// User-facing leaf-level mutations: insert/replace/patch
453+
// all set a new (or unchanged) count_value on the affected
454+
// key; delete removes it. All require secondary mirror.
455+
GroveOp::InsertWithKnownToNotAlreadyExist { .. }
456+
| GroveOp::InsertIfNotExists { .. }
457+
| GroveOp::InsertOrReplace { .. }
458+
| GroveOp::Replace { .. }
459+
| GroveOp::Patch { .. }
460+
| GroveOp::Delete
461+
| GroveOp::DeleteTree(..)
462+
| GroveOp::RefreshReference { .. } => true,
463+
464+
// Bubble-up ops emitted by propagation. Each updates the
465+
// child element's bytes at the parent level — for
466+
// count-bearing trees this changes the aggregated
467+
// count_value, which is the secondary's sort key. Without
468+
// this arm, nested-cidx bubble-up silently leaves the
469+
// outer's secondary stale (commit a8bb34fb).
470+
GroveOp::ReplaceTreeRootKey { .. }
471+
| GroveOp::InsertTreeWithRootHash { .. }
472+
| GroveOp::ReplaceNonMerkTreeRoot { .. }
473+
| GroveOp::InsertNonMerkTree { .. }
474+
| GroveOp::ReplaceCountIndexedTreeRootKeys { .. } => true,
475+
476+
// Non-Merk-tree leaf inserts (commitment/MMR/bulk-append/
477+
// dense) don't change count_value for their own entries
478+
// because these trees use non-Merk storage and don't
479+
// contribute counts to a parent cidx the same way. Their
480+
// own internal aggregation is handled by the tree-specific
481+
// propagation; they do NOT need a per-entry cidx secondary
482+
// mirror at the leaf level.
483+
GroveOp::CommitmentTreeInsert { .. }
484+
| GroveOp::MmrTreeAppend { .. }
485+
| GroveOp::BulkAppend { .. }
486+
| GroveOp::DenseTreeInsert { .. } => false,
487+
}
488+
}
428489
}
429490

430491
impl PartialOrd for GroveOp {
@@ -1912,33 +1973,12 @@ where
19121973
let mut pre: HashMap<Vec<u8>, Option<u64>> = HashMap::new();
19131974
for (key_info, op) in ops_at_path_by_key.iter() {
19141975
let key_bytes = key_info.get_key_clone();
1915-
let mutates = matches!(
1916-
op,
1917-
GroveOp::InsertWithKnownToNotAlreadyExist { .. }
1918-
| GroveOp::InsertIfNotExists { .. }
1919-
| GroveOp::InsertOrReplace { .. }
1920-
| GroveOp::Replace { .. }
1921-
| GroveOp::Patch { .. }
1922-
| GroveOp::Delete
1923-
| GroveOp::DeleteTree(..)
1924-
| GroveOp::RefreshReference { .. }
1925-
| GroveOp::ReplaceTreeRootKey { .. }
1926-
| GroveOp::InsertTreeWithRootHash { .. }
1927-
| GroveOp::ReplaceNonMerkTreeRoot { .. }
1928-
| GroveOp::InsertNonMerkTree { .. }
1929-
// Nested cidx: when a child cidx primary
1930-
// bubbles up to its OUTER cidx primary, the
1931-
// child's element bytes (count_value field)
1932-
// change as part of this op. The outer's
1933-
// secondary entry for the child must move
1934-
// from (old_count_be ‖ child_key) to
1935-
// (new_count_be ‖ child_key). Without this
1936-
// arm the outer's secondary silently reports
1937-
// stale counts even though H1-A integrity
1938-
// still passes.
1939-
| GroveOp::ReplaceCountIndexedTreeRootKeys { .. }
1940-
);
1941-
if mutates && !pre.contains_key(&key_bytes) {
1976+
// Single source of truth: `GroveOp::can_mutate_child_count`
1977+
// uses an exhaustive match so adding a new variant
1978+
// forces explicit classification at the type-system
1979+
// level. This is the structural guard against the
1980+
// nested-cidx bug class (commit a8bb34fb).
1981+
if op.can_mutate_child_count() && !pre.contains_key(&key_bytes) {
19421982
let maybe_bytes = cost_return_on_error!(
19431983
&mut cost,
19441984
merk.get(

0 commit comments

Comments
 (0)