Skip to content

Commit dd90e6b

Browse files
fix+test: close verify_grovedb cidx content-consistency gap
Was the highest-priority audit item from my earlier self-grade: the verify_grovedb H1-A walk verifies *chain* integrity but not *content* consistency between the primary's count_value field and what the secondary actually contains. The nested-cidx bug found in a8bb34f was exactly this class — stale secondary that internally hashes correctly but reports wrong counts. H1-A passed; queries lied. Three changes: 1. CONTENT-CONSISTENCY CHECK in verify_grovedb. After the H1-A check on each cidx primary, walk both Merks' raw storage and assert per-entry consistency. Mismatches are recorded in the existing VerificationIssues HashMap with sentinel path suffixes (__cidx_primary_orphan__, __cidx_secondary_orphan__, __cidx_count_mismatch__, __cidx_secondary_malformed_key__) so the public API stays unchanged. 2. db.insert() REJECTS cidx primary targets. Adding the check revealed a real direct-path bug: db.insert(cidx_primary, ...) wrote to the primary without mirroring to the secondary, leaving the same kind of drift the new check catches. Route users to insert_into_count_indexed_tree with a NotSupported error. 3. DELIBERATE-CORRUPTION TESTS. Three tests directly manipulate the secondary's storage via Element::insert/delete to introduce each drift class: - verify_grovedb_catches_secondary_missing_entry_for_primary - verify_grovedb_catches_orphan_in_secondary - verify_grovedb_catches_count_mismatch_between_primary_and_ secondary Plus direct_db_insert_into_cidx_primary_is_rejected covering the rejection from item 2. Without item 1, all three corruption tests would silently pass an integrity check. With it, the class of bug that took an audit to find is now CI-caught for any future regression. All 1576 grovedb tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 2498b97 commit dd90e6b

3 files changed

Lines changed: 504 additions & 7 deletions

File tree

grovedb/src/lib.rs

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1305,6 +1305,99 @@ impl GroveDb {
13051305
);
13061306
}
13071307

1308+
// Cidx content-consistency check.
1309+
//
1310+
// The H1-A check above verifies *chain* integrity —
1311+
// the cidx element's recorded value_hash matches
1312+
// `combine_hash_three(value_hash(bytes),
1313+
// primary_root, secondary_root)`. That binds the
1314+
// two Merks' root hashes into the parent's hash,
1315+
// but it does NOT verify the secondary's contents
1316+
// match what the primary says they should be.
1317+
//
1318+
// A bug like nested-cidx batch bubble-up forgetting
1319+
// to mirror the count change (caught at audit in
1320+
// a8bb34fb) leaves the secondary internally
1321+
// consistent — its root hash is the correct hash
1322+
// of its on-disk content — but its content is
1323+
// stale relative to the primary. H1-A passes;
1324+
// queries return wrong results.
1325+
//
1326+
// Walk both Merks here and assert per-entry
1327+
// consistency: every primary entry with
1328+
// count_value=c at key=k must correspond to
1329+
// exactly one secondary entry at
1330+
// (c.to_be_bytes() ‖ k). Each mismatch is recorded
1331+
// in `issues` with a sentinel path suffix so the
1332+
// existing `VerificationIssues` type doesn't need
1333+
// to change.
1334+
let mut primary_entries: HashMap<Vec<u8>, u64> = HashMap::new();
1335+
let mut content_iter =
1336+
KVIterator::new(primary_merk.storage.raw_iter(), &all_query).unwrap();
1337+
while let Some((p_key, p_value)) = content_iter.next_kv().unwrap() {
1338+
let p_elem = Element::raw_decode(&p_value, grove_version)?;
1339+
primary_entries.insert(p_key, p_elem.count_value_or_default());
1340+
}
1341+
drop(content_iter);
1342+
1343+
let mut secondary_entries: HashMap<Vec<u8>, u64> = HashMap::new();
1344+
let mut sec_iter =
1345+
KVIterator::new(secondary_merk.storage.raw_iter(), &all_query).unwrap();
1346+
while let Some((sec_key, _sec_value)) = sec_iter.next_kv().unwrap() {
1347+
// Secondary keys are (count_be ‖ original_key);
1348+
// a malformed key short of 8 bytes is itself a
1349+
// drift indicator.
1350+
if sec_key.len() < 8 {
1351+
let mut p = new_path.to_vec();
1352+
p.push(b"__cidx_secondary_malformed_key__".to_vec());
1353+
p.push(sec_key.clone());
1354+
issues.insert(p, ([0u8; 32], [0u8; 32], [0u8; 32]));
1355+
continue;
1356+
}
1357+
let mut count_bytes = [0u8; 8];
1358+
count_bytes.copy_from_slice(&sec_key[..8]);
1359+
let sec_count = u64::from_be_bytes(count_bytes);
1360+
let original_key = sec_key[8..].to_vec();
1361+
secondary_entries.insert(original_key, sec_count);
1362+
}
1363+
drop(sec_iter);
1364+
1365+
// For each primary entry, the secondary must have
1366+
// a matching entry at the same count_value.
1367+
for (p_key, p_count) in &primary_entries {
1368+
match secondary_entries.get(p_key) {
1369+
None => {
1370+
let mut p = new_path.to_vec();
1371+
p.push(b"__cidx_primary_orphan__".to_vec());
1372+
p.push(p_key.clone());
1373+
issues.insert(p, ([0u8; 32], [0u8; 32], [0u8; 32]));
1374+
}
1375+
Some(s_count) if s_count != p_count => {
1376+
let mut p = new_path.to_vec();
1377+
p.push(b"__cidx_count_mismatch__".to_vec());
1378+
p.push(p_key.clone());
1379+
let mut expected = [0u8; 32];
1380+
expected[24..32].copy_from_slice(&p_count.to_be_bytes());
1381+
let mut actual = [0u8; 32];
1382+
actual[24..32].copy_from_slice(&s_count.to_be_bytes());
1383+
issues.insert(p, ([0u8; 32], expected, actual));
1384+
}
1385+
Some(_) => { /* matches */ }
1386+
}
1387+
}
1388+
// For each secondary entry, the primary must have
1389+
// a matching entry at that exact key (count match
1390+
// is already checked above; here we look for
1391+
// orphans in the secondary).
1392+
for s_key in secondary_entries.keys() {
1393+
if !primary_entries.contains_key(s_key) {
1394+
let mut p = new_path.to_vec();
1395+
p.push(b"__cidx_secondary_orphan__".to_vec());
1396+
p.push(s_key.clone());
1397+
issues.insert(p, ([0u8; 32], [0u8; 32], [0u8; 32]));
1398+
}
1399+
}
1400+
13081401
issues.extend(self.verify_merk_and_submerks_in_transaction(
13091402
primary_merk,
13101403
&new_path_ref,

grovedb/src/operations/insert/mod.rs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,30 @@ impl GroveDb {
201201
grove_version
202202
)
203203
);
204+
205+
// Inserting an item directly into a CountIndexedTree primary
206+
// via `db.insert()` would update the primary's content but
207+
// leave the secondary stale — `add_element_on_transaction`
208+
// has no secondary-mirror hook. The dedicated
209+
// `insert_into_count_indexed_tree` exists precisely for this
210+
// case and handles the dual-Merk write. Reject early with a
211+
// pointer to the right API to prevent silent data drift.
212+
// (References — which carry no count change at the cidx
213+
// level — are still allowed; the cidx primary handles them via
214+
// `insert_into_count_indexed_tree`'s reference resolution
215+
// path, but the path used here goes through generic merk
216+
// insertion only and does not need the cidx mirror.)
217+
if subtree_to_insert_into.tree_type.is_count_indexed_primary() {
218+
return Err(Error::NotSupported(
219+
"direct `db.insert` into a CountIndexedTree primary is not supported \
220+
(the secondary index would not be mirrored). Use \
221+
`db.insert_into_count_indexed_tree(...)` for element insertion or \
222+
`db.delete_from_count_indexed_tree(...)` for removal"
223+
.to_string(),
224+
))
225+
.wrap_with_cost(cost);
226+
}
227+
204228
// if we don't allow a tree override then we should check
205229

206230
if options.checks_for_override() {

0 commit comments

Comments
 (0)