From b28a3a2e1a1154610cf3024b0a8fc564592b34e8 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Thu, 21 May 2026 06:09:19 +0700 Subject: [PATCH 1/6] Preserve default section storage removals --- costs/src/storage_cost/removal.rs | 136 +++++++++++------- costs/tests/coverage_regression.rs | 84 ++++++++++- grovedb-version/src/tests.rs | 25 ++++ .../src/version/grovedb_versions.rs | 11 ++ grovedb-version/src/version/v1.rs | 5 +- grovedb-version/src/version/v2.rs | 5 +- grovedb-version/src/version/v3.rs | 5 +- grovedb/src/batch/mod.rs | 14 ++ .../src/batch/single_deletion_cost_tests.rs | 50 ++++++- grovedb/src/operations/delete/mod.rs | 14 ++ merk/src/merk/apply.rs | 62 ++++---- 11 files changed, 322 insertions(+), 89 deletions(-) diff --git a/costs/src/storage_cost/removal.rs b/costs/src/storage_cost/removal.rs index ca5fed186..5592955af 100644 --- a/costs/src/storage_cost/removal.rs +++ b/costs/src/storage_cost/removal.rs @@ -28,6 +28,7 @@ use std::{ borrow::BorrowMut, + cell::Cell, cmp::Ordering, collections::BTreeMap, ops::{Add, AddAssign}, @@ -60,6 +61,83 @@ pub enum StorageRemovedBytes { SectionedStorageRemoval(StorageRemovalPerEpochByIdentifier), } +thread_local! { + static BASIC_SECTIONED_REMOVAL_ADDITION_VERSION: Cell = const { Cell::new(0) }; +} + +/// Guard that restores the previous storage-removal arithmetic version when +/// dropped. +pub struct BasicSectionedRemovalAdditionVersionGuard { + previous_version: u16, +} + +impl Drop for BasicSectionedRemovalAdditionVersionGuard { + fn drop(&mut self) { + BASIC_SECTIONED_REMOVAL_ADDITION_VERSION.with(|current_version| { + current_version.set(self.previous_version); + }); + } +} + +/// Use a storage-removal arithmetic version until the returned guard is +/// dropped. +pub fn use_basic_sectioned_removal_addition_version( + version: u16, +) -> BasicSectionedRemovalAdditionVersionGuard { + let previous_version = BASIC_SECTIONED_REMOVAL_ADDITION_VERSION + .with(|current_version| current_version.replace(version)); + BasicSectionedRemovalAdditionVersionGuard { previous_version } +} + +/// Run storage-removal arithmetic using a specific version. +pub fn with_basic_sectioned_removal_addition_version(version: u16, f: impl FnOnce() -> T) -> T { + let _guard = use_basic_sectioned_removal_addition_version(version); + f() +} + +fn basic_sectioned_removal_addition_version() -> u16 { + BASIC_SECTIONED_REMOVAL_ADDITION_VERSION.with(Cell::get) +} + +fn add_basic_removal_to_sectioned_map( + map: &mut StorageRemovalPerEpochByIdentifier, + removed_bytes: u32, +) { + let epoch_map = map.entry(Identifier::default()).or_default(); + let old_value = epoch_map.remove(UNKNOWN_EPOCH).unwrap_or_default(); + epoch_map.insert(UNKNOWN_EPOCH, old_value.saturating_add(removed_bytes)); +} + +fn legacy_add_basic_removal_to_sectioned_map( + map: &mut StorageRemovalPerEpochByIdentifier, + removed_bytes: u32, +) { + let default = Identifier::default(); + if let std::collections::btree_map::Entry::Vacant(e) = map.entry(default) { + let mut new_map = IntMap::new(); + new_map.insert(UNKNOWN_EPOCH, removed_bytes); + e.insert(new_map); + } else { + let mut old_section_map = map.remove(&default).unwrap_or_default(); + if let Some(old_value) = old_section_map.remove(UNKNOWN_EPOCH) { + old_section_map.insert(UNKNOWN_EPOCH, old_value.saturating_add(removed_bytes)); + } else { + old_section_map.insert(UNKNOWN_EPOCH, removed_bytes); + } + } +} + +fn add_basic_removal_to_sectioned_map_for_current_version( + map: &mut StorageRemovalPerEpochByIdentifier, + removed_bytes: u32, +) { + if basic_sectioned_removal_addition_version() >= 1 { + add_basic_removal_to_sectioned_map(map, removed_bytes); + } else { + legacy_add_basic_removal_to_sectioned_map(map, removed_bytes); + } +} + impl Add for StorageRemovedBytes { type Output = Self; @@ -74,38 +152,14 @@ impl Add for StorageRemovedBytes { NoStorageRemoval => BasicStorageRemoval(s), BasicStorageRemoval(r) => BasicStorageRemoval(s.saturating_add(r)), SectionedStorageRemoval(mut map) => { - let default = Identifier::default(); - if let std::collections::btree_map::Entry::Vacant(e) = map.entry(default) { - let mut new_map = IntMap::new(); - new_map.insert(UNKNOWN_EPOCH, s); - e.insert(new_map); - } else { - let mut old_section_map = map.remove(&default).unwrap_or_default(); - if let Some(old_value) = old_section_map.remove(UNKNOWN_EPOCH) { - old_section_map.insert(UNKNOWN_EPOCH, old_value.saturating_add(s)); - } else { - old_section_map.insert(UNKNOWN_EPOCH, s); - } - } + add_basic_removal_to_sectioned_map_for_current_version(&mut map, s); SectionedStorageRemoval(map) } }, SectionedStorageRemoval(mut smap) => match rhs { NoStorageRemoval => SectionedStorageRemoval(smap), BasicStorageRemoval(r) => { - let default = Identifier::default(); - if let std::collections::btree_map::Entry::Vacant(e) = smap.entry(default) { - let mut new_map = IntMap::new(); - new_map.insert(UNKNOWN_EPOCH, r); - e.insert(new_map); - } else { - let mut old_section_map = smap.remove(&default).unwrap_or_default(); - if let Some(old_value) = old_section_map.remove(UNKNOWN_EPOCH) { - old_section_map.insert(UNKNOWN_EPOCH, old_value.saturating_add(r)); - } else { - old_section_map.insert(UNKNOWN_EPOCH, r); - } - } + add_basic_removal_to_sectioned_map_for_current_version(&mut smap, r); SectionedStorageRemoval(smap) } SectionedStorageRemoval(rmap) => { @@ -144,40 +198,14 @@ impl AddAssign for StorageRemovedBytes { NoStorageRemoval => {} BasicStorageRemoval(r) => *s = s.saturating_add(r), SectionedStorageRemoval(mut map) => { - let default = Identifier::default(); - if let Some(mut old_int_map) = map.remove(&default) { - if old_int_map.contains_key(UNKNOWN_EPOCH) { - let old_value = old_int_map.remove(UNKNOWN_EPOCH).unwrap_or_default(); - old_int_map.insert(UNKNOWN_EPOCH, old_value.saturating_add(*s)); - } else { - old_int_map.insert(UNKNOWN_EPOCH, *s); - } - } else { - let mut new_map = IntMap::new(); - new_map.insert(UNKNOWN_EPOCH, *s); - map.insert(default, new_map); - } + add_basic_removal_to_sectioned_map_for_current_version(&mut map, *s); *self = SectionedStorageRemoval(map) } }, SectionedStorageRemoval(smap) => match rhs { NoStorageRemoval => {} BasicStorageRemoval(r) => { - let default = Identifier::default(); - let map_to_insert = if let Some(mut old_int_map) = smap.remove(&default) { - if old_int_map.contains_key(UNKNOWN_EPOCH) { - let old_value = old_int_map.remove(UNKNOWN_EPOCH).unwrap_or_default(); - old_int_map.insert(UNKNOWN_EPOCH, old_value.saturating_add(r)); - } else { - old_int_map.insert(UNKNOWN_EPOCH, r); - } - old_int_map - } else { - let mut new_map = IntMap::new(); - new_map.insert(UNKNOWN_EPOCH, r); - new_map - }; - smap.insert(default, map_to_insert); + add_basic_removal_to_sectioned_map_for_current_version(smap, r); } SectionedStorageRemoval(rmap) => { rmap.into_iter().for_each(|(identifier, mut int_map_b)| { diff --git a/costs/tests/coverage_regression.rs b/costs/tests/coverage_regression.rs index 3fadc8792..ee753f98d 100644 --- a/costs/tests/coverage_regression.rs +++ b/costs/tests/coverage_regression.rs @@ -8,7 +8,10 @@ use grovedb_costs::{ error::Error, storage_cost::{ key_value_cost::KeyValueStorageCost, - removal::{Identifier, StorageRemovedBytes, StorageRemovedBytes::*}, + removal::{ + with_basic_sectioned_removal_addition_version, Identifier, StorageRemovedBytes, + StorageRemovedBytes::*, + }, transition::OperationStorageTransitionType, StorageCost, }, @@ -607,7 +610,10 @@ fn storage_removed_bytes_add_and_add_assign_paths_are_exercised() { let basic_plus_sectioned_missing_default = BasicStorageRemoval(7) + sectioned(identifier_a, 8, 9); - assert!(basic_plus_sectioned_missing_default.has_removal()); + assert_eq!( + basic_plus_sectioned_missing_default.total_removed_bytes(), + 16 + ); let basic_plus_sectioned_with_default = BasicStorageRemoval(4) + sectioned( @@ -619,13 +625,17 @@ fn storage_removed_bytes_add_and_add_assign_paths_are_exercised() { basic_plus_sectioned_with_default, SectionedStorageRemoval(_) )); + assert_eq!(basic_plus_sectioned_with_default.total_removed_bytes(), 0); let sectioned_plus_no = sectioned(identifier_a, 1, 2) + NoStorageRemoval; assert!(sectioned_plus_no.has_removal()); let sectioned_plus_basic_missing_default = sectioned(identifier_a, 3, 4) + BasicStorageRemoval(5); - assert!(sectioned_plus_basic_missing_default.has_removal()); + assert_eq!( + sectioned_plus_basic_missing_default.total_removed_bytes(), + 9 + ); let sectioned_plus_basic_with_default = sectioned( Identifier::default(), @@ -636,6 +646,7 @@ fn storage_removed_bytes_add_and_add_assign_paths_are_exercised() { sectioned_plus_basic_with_default, SectionedStorageRemoval(_) )); + assert_eq!(sectioned_plus_basic_with_default.total_removed_bytes(), 0); let sectioned_plus_sectioned = sectioned(identifier_a, 1, 10) + sectioned(identifier_a, 1, 20); assert_eq!(sectioned_plus_sectioned.total_removed_bytes(), 30); @@ -660,13 +671,30 @@ fn storage_removed_bytes_add_and_add_assign_paths_are_exercised() { basic_assign_with_sectioned, SectionedStorageRemoval(_) )); + assert_eq!(basic_assign_with_sectioned.total_removed_bytes(), 14); + + let mut basic_assign_with_default_sectioned = BasicStorageRemoval(4); + basic_assign_with_default_sectioned += sectioned( + Identifier::default(), + grovedb_costs::storage_cost::removal::UNKNOWN_EPOCH, + 10, + ); + assert_eq!(basic_assign_with_default_sectioned.total_removed_bytes(), 0); let mut sectioned_assign = sectioned(identifier_a, 10, 1); sectioned_assign += NoStorageRemoval; assert!(sectioned_assign.has_removal()); sectioned_assign += BasicStorageRemoval(2); - assert!(sectioned_assign.has_removal()); + assert_eq!(sectioned_assign.total_removed_bytes(), 3); + + let mut sectioned_default_assign = sectioned( + Identifier::default(), + grovedb_costs::storage_cost::removal::UNKNOWN_EPOCH, + 11, + ); + sectioned_default_assign += BasicStorageRemoval(2); + assert_eq!(sectioned_default_assign.total_removed_bytes(), 0); sectioned_assign += sectioned(identifier_a, 10, 3); assert!(sectioned_assign.has_removal()); @@ -692,6 +720,54 @@ fn storage_removed_bytes_add_and_add_assign_paths_are_exercised() { assert_eq!(sectioned(identifier_b, 2, 11).total_removed_bytes(), 11); } +#[test] +fn latest_storage_removed_bytes_add_preserves_default_section() { + let identifier_a = [1u8; 32]; + + with_basic_sectioned_removal_addition_version(1, || { + let basic_plus_sectioned_with_default = BasicStorageRemoval(4) + + sectioned( + Identifier::default(), + grovedb_costs::storage_cost::removal::UNKNOWN_EPOCH, + 5, + ); + assert_eq!(basic_plus_sectioned_with_default.total_removed_bytes(), 9); + + let sectioned_plus_basic_with_default = sectioned( + Identifier::default(), + grovedb_costs::storage_cost::removal::UNKNOWN_EPOCH, + 6, + ) + BasicStorageRemoval(7); + assert_eq!(sectioned_plus_basic_with_default.total_removed_bytes(), 13); + + let mut basic_assign_with_default_sectioned = BasicStorageRemoval(4); + basic_assign_with_default_sectioned += sectioned( + Identifier::default(), + grovedb_costs::storage_cost::removal::UNKNOWN_EPOCH, + 10, + ); + assert_eq!( + basic_assign_with_default_sectioned.total_removed_bytes(), + 14 + ); + + let mut sectioned_default_assign = sectioned( + Identifier::default(), + grovedb_costs::storage_cost::removal::UNKNOWN_EPOCH, + 11, + ); + sectioned_default_assign += BasicStorageRemoval(2); + assert_eq!(sectioned_default_assign.total_removed_bytes(), 13); + + let basic_plus_sectioned_missing_default = + BasicStorageRemoval(7) + sectioned(identifier_a, 8, 9); + assert_eq!( + basic_plus_sectioned_missing_default.total_removed_bytes(), + 16 + ); + }); +} + #[test] fn error_display_includes_expected_data() { let err = Error::StorageCostMismatch { diff --git a/grovedb-version/src/tests.rs b/grovedb-version/src/tests.rs index 7d68f661a..1c760a1e4 100644 --- a/grovedb-version/src/tests.rs +++ b/grovedb-version/src/tests.rs @@ -198,6 +198,31 @@ fn v2_has_updated_merk_average_case_costs() { ); } +#[test] +fn v3_uses_fixed_basic_to_sectioned_storage_removal_addition() { + assert_eq!( + GROVE_V1 + .grovedb_versions + .storage_costs + .add_basic_storage_removal_to_sectioned_storage_removal, + 0 + ); + assert_eq!( + GROVE_V2 + .grovedb_versions + .storage_costs + .add_basic_storage_removal_to_sectioned_storage_removal, + 0 + ); + assert_eq!( + GROVE_V3 + .grovedb_versions + .storage_costs + .add_basic_storage_removal_to_sectioned_storage_removal, + 1 + ); +} + // ── Default trait for version structs ───────────────────────────────── #[test] diff --git a/grovedb-version/src/version/grovedb_versions.rs b/grovedb-version/src/version/grovedb_versions.rs index 2c404b4b6..fbaf37986 100644 --- a/grovedb-version/src/version/grovedb_versions.rs +++ b/grovedb-version/src/version/grovedb_versions.rs @@ -7,10 +7,21 @@ pub struct GroveDBVersions { pub operations: GroveDBOperationsVersions, pub aggregate_sum_path_query_methods: GroveDBAggregateSumPathQueryMethodVersions, pub path_query_methods: GroveDBPathQueryMethodVersions, + pub storage_costs: GroveDBStorageCostVersions, pub replication: GroveDBReplicationVersions, pub query_limits: GroveDBQueryLimits, } +#[derive(Clone, Debug, Default)] +pub struct GroveDBStorageCostVersions { + /// `StorageRemovedBytes` addition between basic and sectioned removals. + /// + /// Version 0 preserves the legacy behavior where adding basic removal + /// bytes to an existing default section can drop that default section. + /// Version 1 reinserts the updated default section. + pub add_basic_storage_removal_to_sectioned_storage_removal: FeatureVersion, +} + #[derive(Clone, Debug)] pub struct GroveDBQueryLimits { pub max_aggregate_sum_query_elements_scanned: u16, diff --git a/grovedb-version/src/version/v1.rs b/grovedb-version/src/version/v1.rs index b5f05eae5..461ad68ca 100644 --- a/grovedb-version/src/version/v1.rs +++ b/grovedb-version/src/version/v1.rs @@ -11,7 +11,7 @@ use crate::version::{ GroveDBOperationsInsertVersions, GroveDBOperationsPrivateDocumentStoreVersions, GroveDBOperationsProofVersions, GroveDBOperationsQueryVersions, GroveDBOperationsVersions, GroveDBOperationsWorstCaseVersions, GroveDBPathQueryMethodVersions, GroveDBQueryLimits, - GroveDBReplicationVersions, GroveDBVersions, + GroveDBReplicationVersions, GroveDBStorageCostVersions, GroveDBVersions, }, merk_versions::{ MerkAverageCaseCostsVersions, MerkBatchVersions, MerkProofVersions, MerkTreeVersions, @@ -241,6 +241,9 @@ pub const GROVE_V1: GroveVersion = GroveVersion { unified_read_mode: 0, per_instance_query_limits: 0, }, + storage_costs: GroveDBStorageCostVersions { + add_basic_storage_removal_to_sectioned_storage_removal: 0, + }, replication: GroveDBReplicationVersions { get_subtrees_metadata: 0, fetch_chunk: 0, diff --git a/grovedb-version/src/version/v2.rs b/grovedb-version/src/version/v2.rs index 3ed8c3fd3..618089b4a 100644 --- a/grovedb-version/src/version/v2.rs +++ b/grovedb-version/src/version/v2.rs @@ -11,7 +11,7 @@ use crate::version::{ GroveDBOperationsInsertVersions, GroveDBOperationsPrivateDocumentStoreVersions, GroveDBOperationsProofVersions, GroveDBOperationsQueryVersions, GroveDBOperationsVersions, GroveDBOperationsWorstCaseVersions, GroveDBPathQueryMethodVersions, GroveDBQueryLimits, - GroveDBReplicationVersions, GroveDBVersions, + GroveDBReplicationVersions, GroveDBStorageCostVersions, GroveDBVersions, }, merk_versions::{ MerkAverageCaseCostsVersions, MerkBatchVersions, MerkProofVersions, MerkTreeVersions, @@ -241,6 +241,9 @@ pub const GROVE_V2: GroveVersion = GroveVersion { unified_read_mode: 0, per_instance_query_limits: 0, }, + storage_costs: GroveDBStorageCostVersions { + add_basic_storage_removal_to_sectioned_storage_removal: 0, + }, replication: GroveDBReplicationVersions { get_subtrees_metadata: 0, fetch_chunk: 0, diff --git a/grovedb-version/src/version/v3.rs b/grovedb-version/src/version/v3.rs index 364fc34bb..5c846f814 100644 --- a/grovedb-version/src/version/v3.rs +++ b/grovedb-version/src/version/v3.rs @@ -11,7 +11,7 @@ use crate::version::{ GroveDBOperationsInsertVersions, GroveDBOperationsPrivateDocumentStoreVersions, GroveDBOperationsProofVersions, GroveDBOperationsQueryVersions, GroveDBOperationsVersions, GroveDBOperationsWorstCaseVersions, GroveDBPathQueryMethodVersions, GroveDBQueryLimits, - GroveDBReplicationVersions, GroveDBVersions, + GroveDBReplicationVersions, GroveDBStorageCostVersions, GroveDBVersions, }, merk_versions::{ MerkAverageCaseCostsVersions, MerkBatchVersions, MerkProofVersions, MerkTreeVersions, @@ -245,6 +245,9 @@ pub const GROVE_V3: GroveVersion = GroveVersion { unified_read_mode: 0, per_instance_query_limits: 0, }, + storage_costs: GroveDBStorageCostVersions { + add_basic_storage_removal_to_sectioned_storage_removal: 1, + }, replication: GroveDBReplicationVersions { get_subtrees_metadata: 0, fetch_chunk: 0, diff --git a/grovedb/src/batch/mod.rs b/grovedb/src/batch/mod.rs index 1ba58a71e..4b2e9d25d 100644 --- a/grovedb/src/batch/mod.rs +++ b/grovedb/src/batch/mod.rs @@ -5962,6 +5962,13 @@ impl GroveDb { .apply_batch .apply_batch_with_element_flags_update ); + let _storage_removal_version_guard = + grovedb_costs::storage_cost::removal::use_basic_sectioned_removal_addition_version( + grove_version + .grovedb_versions + .storage_costs + .add_basic_storage_removal_to_sectioned_storage_removal, + ); let mut cost = OperationCost::default(); let tx = TxRef::new(&self.db, transaction); @@ -6406,6 +6413,13 @@ impl GroveDb { .apply_batch .apply_partial_batch_with_element_flags_update ); + let _storage_removal_version_guard = + grovedb_costs::storage_cost::removal::use_basic_sectioned_removal_addition_version( + grove_version + .grovedb_versions + .storage_costs + .add_basic_storage_removal_to_sectioned_storage_removal, + ); let mut cost = OperationCost::default(); let tx = TxRef::new(&self.db, transaction); diff --git a/grovedb/src/batch/single_deletion_cost_tests.rs b/grovedb/src/batch/single_deletion_cost_tests.rs index e77e10c9d..1ea840eb3 100644 --- a/grovedb/src/batch/single_deletion_cost_tests.rs +++ b/grovedb/src/batch/single_deletion_cost_tests.rs @@ -5,7 +5,8 @@ mod tests { use grovedb_costs::storage_cost::removal::{ Identifier, StorageRemovalPerEpochByIdentifier, - StorageRemovedBytes::SectionedStorageRemoval, + StorageRemovedBytes::{BasicStorageRemoval, SectionedStorageRemoval}, + UNKNOWN_EPOCH, }; use grovedb_merk::tree_type::TreeType; use grovedb_version::version::GroveVersion; @@ -17,6 +18,53 @@ mod tests { Element, }; + #[test] + fn latest_delete_preserves_basic_plus_default_sectioned_removal_cost() { + let grove_version = GroveVersion::latest(); + let db = make_empty_grovedb(); + + let insertion_cost = db + .insert( + EMPTY_PATH, + b"key1", + Element::new_item_with_flags(b"cat".to_vec(), Some(b"apple".to_vec())), + None, + None, + grove_version, + ) + .cost_as_result() + .expect("expected to insert successfully"); + + let deletion_cost = db + .delete_with_sectional_storage_function( + EMPTY_PATH, + b"key1", + None, + None, + &mut |_element_flags, removed_key_bytes, removed_value_bytes| { + let mut removed_bytes = StorageRemovalPerEpochByIdentifier::default(); + let mut removed_bytes_for_identity = IntMap::new(); + removed_bytes_for_identity.insert(UNKNOWN_EPOCH, removed_value_bytes); + removed_bytes.insert(Identifier::default(), removed_bytes_for_identity); + Ok(( + BasicStorageRemoval(removed_key_bytes), + SectionedStorageRemoval(removed_bytes), + )) + }, + grove_version, + ) + .cost_as_result() + .expect("expected to delete successfully"); + + assert_eq!( + deletion_cost + .storage_cost + .removed_bytes + .total_removed_bytes(), + insertion_cost.storage_cost.added_bytes + ); + } + #[test] fn test_batch_one_deletion_tree_costs_match_non_batch_on_transaction() { let grove_version = GroveVersion::latest(); diff --git a/grovedb/src/operations/delete/mod.rs b/grovedb/src/operations/delete/mod.rs index 62eb54e5e..7537d7098 100644 --- a/grovedb/src/operations/delete/mod.rs +++ b/grovedb/src/operations/delete/mod.rs @@ -402,6 +402,13 @@ impl GroveDb { .delete .delete_with_sectional_storage_function ); + let _storage_removal_version_guard = + grovedb_costs::storage_cost::removal::use_basic_sectioned_removal_addition_version( + grove_version + .grovedb_versions + .storage_costs + .add_basic_storage_removal_to_sectioned_storage_removal, + ); let tx = TxRef::new(&self.db, transaction); @@ -534,6 +541,13 @@ impl GroveDb { .delete .delete_if_empty_tree_with_sectional_storage_function ); + let _storage_removal_version_guard = + grovedb_costs::storage_cost::removal::use_basic_sectioned_removal_addition_version( + grove_version + .grovedb_versions + .storage_costs + .add_basic_storage_removal_to_sectioned_storage_removal, + ); let options = DeleteOptions { allow_deleting_non_empty_trees: false, diff --git a/merk/src/merk/apply.rs b/merk/src/merk/apply.rs index 9a045b6cb..d0b68b0b9 100644 --- a/merk/src/merk/apply.rs +++ b/merk/src/merk/apply.rs @@ -428,34 +428,42 @@ where } } - let maybe_walker = self - .tree - .take() - .map(|tree| Walker::new(tree, self.source())); + grovedb_costs::storage_cost::removal::with_basic_sectioned_removal_addition_version( + grove_version + .grovedb_versions + .storage_costs + .add_basic_storage_removal_to_sectioned_storage_removal, + || { + let maybe_walker = self + .tree + .take() + .map(|tree| Walker::new(tree, self.source())); - Walker::apply_to( - maybe_walker, - batch, - self.source(), - old_specialized_cost, - value_defined_cost_fn, - get_temp_new_value_with_old_flags, - update_tree_value_based_on_costs, - section_removal_bytes, - old_value_observer, - grove_version, + Walker::apply_to( + maybe_walker, + batch, + self.source(), + old_specialized_cost, + value_defined_cost_fn, + get_temp_new_value_with_old_flags, + update_tree_value_based_on_costs, + section_removal_bytes, + old_value_observer, + grove_version, + ) + .flat_map_ok(|(maybe_tree, key_updates)| { + // we set the new root node of the merk tree + self.tree.set(maybe_tree); + // commit changes to db + self.commit( + key_updates, + aux, + options, + old_specialized_cost, + grove_version, + ) + }) + }, ) - .flat_map_ok(|(maybe_tree, key_updates)| { - // we set the new root node of the merk tree - self.tree.set(maybe_tree); - // commit changes to db - self.commit( - key_updates, - aux, - options, - old_specialized_cost, - grove_version, - ) - }) } } From 30012219ca50452871b672a4e5750d0c3aea886b Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Thu, 21 May 2026 20:33:49 +0700 Subject: [PATCH 2/6] fix(costs): keep Sectioned+=Basic removal correct in legacy path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The default-section-drop bug only ever affected three of the four basic-into-sectioned arms (Add: Basic+Sectioned, Add: Sectioned+Basic, AddAssign: Basic+=Sectioned). The fourth, AddAssign: Sectioned+=Basic, reinserted the default section correctly in every shipped version. The prior rework routed all four through the version selector, which made the legacy/v0 path *drop* the default section for the fourth arm too — regressing shipped v1/v2 output (its own test asserted the wrong 0). Route Sectioned+=Basic through the always-correct helper unconditionally and assert the preserved total (13). Document why the thread-local selector exists and which arms it governs. Co-Authored-By: Claude Opus 4.7 (1M context) --- costs/src/storage_cost/removal.rs | 45 +++++++++++++++++++++++++++++- costs/tests/coverage_regression.rs | 5 +++- 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/costs/src/storage_cost/removal.rs b/costs/src/storage_cost/removal.rs index 5592955af..a877e6b21 100644 --- a/costs/src/storage_cost/removal.rs +++ b/costs/src/storage_cost/removal.rs @@ -61,6 +61,27 @@ pub enum StorageRemovedBytes { SectionedStorageRemoval(StorageRemovalPerEpochByIdentifier), } +// Version selector for the basic-into-sectioned storage-removal arithmetic. +// +// Why a thread-local instead of an explicit parameter: the version-sensitive +// combination happens inside the `Add`/`AddAssign` operator overloads for +// `StorageRemovedBytes`, which are reached through `StorageCost` and +// `OperationCost` aggregation at hundreds of version-less call sites (every +// `add_cost` / `cost_return_on_error!`). Those operator signatures cannot carry +// a `grove_version`, and `grovedb-costs` does not depend on `grovedb-version`. +// The version is only known at the GroveDB apply / delete / batch entry points, +// which install a guard ([`use_basic_sectioned_removal_addition_version`] / +// [`with_basic_sectioned_removal_addition_version`]) for the duration of the +// operation. +// +// The default is `0` (legacy / shipped v1/v2 behavior). That default is the safe +// one: an un-guarded caller reproduces historical behavior rather than silently +// "upgrading" to the fixed arithmetic and diverging from the rest of the +// network. Only an explicit guard set from a v3+ context enables the fix. +// +// Note: this selector only affects the three historically-buggy arms. The +// `Sectioned += Basic` arm was always correct and bypasses the selector +// entirely (see [`AddAssign`]). thread_local! { static BASIC_SECTIONED_REMOVAL_ADDITION_VERSION: Cell = const { Cell::new(0) }; } @@ -99,6 +120,10 @@ fn basic_sectioned_removal_addition_version() -> u16 { BASIC_SECTIONED_REMOVAL_ADDITION_VERSION.with(Cell::get) } +/// Correct behavior: fold the basic removal into the default identifier's +/// `UNKNOWN_EPOCH` entry while preserving the rest of the default section. Used +/// unconditionally for the always-correct `Sectioned += Basic` arm, and for the +/// fixed (v3+) path of the three historically-buggy arms. fn add_basic_removal_to_sectioned_map( map: &mut StorageRemovalPerEpochByIdentifier, removed_bytes: u32, @@ -108,6 +133,11 @@ fn add_basic_removal_to_sectioned_map( epoch_map.insert(UNKNOWN_EPOCH, old_value.saturating_add(removed_bytes)); } +/// Buggy shipped (v1/v2) behavior, preserved verbatim for replay +/// compatibility: when the default identifier already exists it is removed, +/// mutated, and then **dropped** instead of reinserted, losing the rest of the +/// default section. Only reachable through the legacy (v0) path of the three +/// historically-buggy arms. fn legacy_add_basic_removal_to_sectioned_map( map: &mut StorageRemovalPerEpochByIdentifier, removed_bytes: u32, @@ -127,6 +157,11 @@ fn legacy_add_basic_removal_to_sectioned_map( } } +/// Version-selecting helper for the three historically-buggy arms only +/// (Add: Basic+Sectioned, Add: Sectioned+Basic, AddAssign: Basic+=Sectioned). +/// v0 keeps the buggy shipped behavior; v3+ uses the corrected behavior. The +/// always-correct `Sectioned += Basic` arm must NOT use this — it calls +/// [`add_basic_removal_to_sectioned_map`] directly. fn add_basic_removal_to_sectioned_map_for_current_version( map: &mut StorageRemovalPerEpochByIdentifier, removed_bytes: u32, @@ -205,7 +240,15 @@ impl AddAssign for StorageRemovedBytes { SectionedStorageRemoval(smap) => match rhs { NoStorageRemoval => {} BasicStorageRemoval(r) => { - add_basic_removal_to_sectioned_map_for_current_version(smap, r); + // `Sectioned += Basic` reinserted the default section + // correctly in EVERY shipped version, so it is intentionally + // NOT version-gated — always use the default-section- + // preserving helper. The three historically-buggy arms + // (Add: Basic+Sectioned, Add: Sectioned+Basic, AddAssign: + // Basic+=Sectioned) are gated; this one was never broken, so + // routing it through the version selector would *regress* + // shipped v1/v2 output (drop the default section under v0). + add_basic_removal_to_sectioned_map(smap, r); } SectionedStorageRemoval(rmap) => { rmap.into_iter().for_each(|(identifier, mut int_map_b)| { diff --git a/costs/tests/coverage_regression.rs b/costs/tests/coverage_regression.rs index ee753f98d..75bbdbba3 100644 --- a/costs/tests/coverage_regression.rs +++ b/costs/tests/coverage_regression.rs @@ -688,13 +688,16 @@ fn storage_removed_bytes_add_and_add_assign_paths_are_exercised() { sectioned_assign += BasicStorageRemoval(2); assert_eq!(sectioned_assign.total_removed_bytes(), 3); + // `Sectioned += Basic` was always correct (it reinserts the default + // section), so it preserves both removals (11 + 2) even on the legacy/v0 + // path exercised here — it is not version-gated. let mut sectioned_default_assign = sectioned( Identifier::default(), grovedb_costs::storage_cost::removal::UNKNOWN_EPOCH, 11, ); sectioned_default_assign += BasicStorageRemoval(2); - assert_eq!(sectioned_default_assign.total_removed_bytes(), 0); + assert_eq!(sectioned_default_assign.total_removed_bytes(), 13); sectioned_assign += sectioned(identifier_a, 10, 3); assert!(sectioned_assign.has_removal()); From b8b61b8987cc7da077bc440b5eb292982cad6bcf Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Sat, 22 Aug 2026 01:53:28 +0700 Subject: [PATCH 3/6] fix(version): re-gate fixed storage-removal arithmetic to GROVE_V4 GROVE_V3 went live on mainnet (~mid-June 2026) with the legacy default-section-dropping removal arithmetic, so the issue #683 fix can no longer activate in v3 without forking replay of v3 blocks. Move the activation to GROVE_V4: v1..v3 keep the legacy behavior, v4+ preserves the default section. Adds a v3 regression test asserting the legacy undercount is kept, and extends the version-gate test to cover v4. Co-Authored-By: Claude Fable 5 --- costs/src/storage_cost/removal.rs | 12 ++-- grovedb-version/src/tests.rs | 12 +++- grovedb-version/src/version/v3.rs | 5 +- grovedb-version/src/version/v4.rs | 17 +++++- .../src/batch/single_deletion_cost_tests.rs | 61 +++++++++++++++++++ 5 files changed, 98 insertions(+), 9 deletions(-) diff --git a/costs/src/storage_cost/removal.rs b/costs/src/storage_cost/removal.rs index a877e6b21..1c3d33120 100644 --- a/costs/src/storage_cost/removal.rs +++ b/costs/src/storage_cost/removal.rs @@ -74,10 +74,10 @@ pub enum StorageRemovedBytes { // [`with_basic_sectioned_removal_addition_version`]) for the duration of the // operation. // -// The default is `0` (legacy / shipped v1/v2 behavior). That default is the safe +// The default is `0` (legacy / shipped v1..v3 behavior). That default is the safe // one: an un-guarded caller reproduces historical behavior rather than silently // "upgrading" to the fixed arithmetic and diverging from the rest of the -// network. Only an explicit guard set from a v3+ context enables the fix. +// network. Only an explicit guard set from a v4+ context enables the fix. // // Note: this selector only affects the three historically-buggy arms. The // `Sectioned += Basic` arm was always correct and bypasses the selector @@ -123,7 +123,7 @@ fn basic_sectioned_removal_addition_version() -> u16 { /// Correct behavior: fold the basic removal into the default identifier's /// `UNKNOWN_EPOCH` entry while preserving the rest of the default section. Used /// unconditionally for the always-correct `Sectioned += Basic` arm, and for the -/// fixed (v3+) path of the three historically-buggy arms. +/// fixed (v4+) path of the three historically-buggy arms. fn add_basic_removal_to_sectioned_map( map: &mut StorageRemovalPerEpochByIdentifier, removed_bytes: u32, @@ -133,7 +133,7 @@ fn add_basic_removal_to_sectioned_map( epoch_map.insert(UNKNOWN_EPOCH, old_value.saturating_add(removed_bytes)); } -/// Buggy shipped (v1/v2) behavior, preserved verbatim for replay +/// Buggy shipped (v1..v3) behavior, preserved verbatim for replay /// compatibility: when the default identifier already exists it is removed, /// mutated, and then **dropped** instead of reinserted, losing the rest of the /// default section. Only reachable through the legacy (v0) path of the three @@ -159,7 +159,7 @@ fn legacy_add_basic_removal_to_sectioned_map( /// Version-selecting helper for the three historically-buggy arms only /// (Add: Basic+Sectioned, Add: Sectioned+Basic, AddAssign: Basic+=Sectioned). -/// v0 keeps the buggy shipped behavior; v3+ uses the corrected behavior. The +/// v0 keeps the buggy shipped behavior; v4+ uses the corrected behavior. The /// always-correct `Sectioned += Basic` arm must NOT use this — it calls /// [`add_basic_removal_to_sectioned_map`] directly. fn add_basic_removal_to_sectioned_map_for_current_version( @@ -247,7 +247,7 @@ impl AddAssign for StorageRemovedBytes { // (Add: Basic+Sectioned, Add: Sectioned+Basic, AddAssign: // Basic+=Sectioned) are gated; this one was never broken, so // routing it through the version selector would *regress* - // shipped v1/v2 output (drop the default section under v0). + // shipped v1..v3 output (drop the default section under v0). add_basic_removal_to_sectioned_map(smap, r); } SectionedStorageRemoval(rmap) => { diff --git a/grovedb-version/src/tests.rs b/grovedb-version/src/tests.rs index 1c760a1e4..4420084cc 100644 --- a/grovedb-version/src/tests.rs +++ b/grovedb-version/src/tests.rs @@ -199,7 +199,10 @@ fn v2_has_updated_merk_average_case_costs() { } #[test] -fn v3_uses_fixed_basic_to_sectioned_storage_removal_addition() { +fn v4_uses_fixed_basic_to_sectioned_storage_removal_addition() { + // v1..v3 are live on mainnet with the legacy (default-section-dropping) + // removal arithmetic; only v4+ activates the fix. A `1` on any earlier + // version would change replayed historical costs. assert_eq!( GROVE_V1 .grovedb_versions @@ -219,6 +222,13 @@ fn v3_uses_fixed_basic_to_sectioned_storage_removal_addition() { .grovedb_versions .storage_costs .add_basic_storage_removal_to_sectioned_storage_removal, + 0 + ); + assert_eq!( + GROVE_V4 + .grovedb_versions + .storage_costs + .add_basic_storage_removal_to_sectioned_storage_removal, 1 ); } diff --git a/grovedb-version/src/version/v3.rs b/grovedb-version/src/version/v3.rs index 5c846f814..1648bc670 100644 --- a/grovedb-version/src/version/v3.rs +++ b/grovedb-version/src/version/v3.rs @@ -246,7 +246,10 @@ pub const GROVE_V3: GroveVersion = GroveVersion { per_instance_query_limits: 0, }, storage_costs: GroveDBStorageCostVersions { - add_basic_storage_removal_to_sectioned_storage_removal: 1, + // GROVE_V3 shipped to mainnet with the legacy (default-section- + // dropping) removal arithmetic; changing it here would break + // replay of v3 blocks. The fix activates in GROVE_V4. + add_basic_storage_removal_to_sectioned_storage_removal: 0, }, replication: GroveDBReplicationVersions { get_subtrees_metadata: 0, diff --git a/grovedb-version/src/version/v4.rs b/grovedb-version/src/version/v4.rs index 88b56c5b2..54e52f1b2 100644 --- a/grovedb-version/src/version/v4.rs +++ b/grovedb-version/src/version/v4.rs @@ -227,6 +227,15 @@ //! subtree changes never disturbed it. Gated because (i) moves a committed //! root and (ii)/(iii) flip an accepted/rejected outcome. //! +//! - `storage_costs.add_basic_storage_removal_to_sectioned_storage_removal: +//! 1` — combining a `BasicStorageRemoval` with a `SectionedStorageRemoval` +//! folds the basic bytes into the default identifier's `UNKNOWN_EPOCH` +//! entry while PRESERVING the rest of the default section (issue #683). +//! V1..V3 keep the shipped arithmetic, which drops the mutated default +//! section in three of the four `Add`/`AddAssign` arms, undercounting +//! removed bytes — preserved because removal totals feed fee refunds and +//! historical blocks must replay to identical costs. +//! //! Note that `GroveVersion::latest()` resolves to this version, so anything //! defaulting to "latest" — tests, benchmarks, tools — exercises every gate //! listed above rather than V3 behaviour. @@ -256,7 +265,7 @@ use crate::version::{ GroveDBOperationsInsertVersions, GroveDBOperationsPrivateDocumentStoreVersions, GroveDBOperationsProofVersions, GroveDBOperationsQueryVersions, GroveDBOperationsVersions, GroveDBOperationsWorstCaseVersions, GroveDBPathQueryMethodVersions, GroveDBQueryLimits, - GroveDBReplicationVersions, GroveDBVersions, + GroveDBReplicationVersions, GroveDBStorageCostVersions, GroveDBVersions, }, merk_versions::{ MerkAverageCaseCostsVersions, MerkBatchVersions, MerkProofVersions, MerkTreeVersions, @@ -510,6 +519,12 @@ pub const GROVE_V4: GroveVersion = GroveVersion { unified_read_mode: 1, per_instance_query_limits: 1, // Query::limit served on trusted reads (V4+) }, + storage_costs: GroveDBStorageCostVersions { + // Basic+sectioned removal addition preserves the default section + // (issue #683); v1..v3 keep the legacy default-section-dropping + // arithmetic for replay compatibility. + add_basic_storage_removal_to_sectioned_storage_removal: 1, + }, replication: GroveDBReplicationVersions { get_subtrees_metadata: 0, fetch_chunk: 0, diff --git a/grovedb/src/batch/single_deletion_cost_tests.rs b/grovedb/src/batch/single_deletion_cost_tests.rs index 1ea840eb3..8574a8a71 100644 --- a/grovedb/src/batch/single_deletion_cost_tests.rs +++ b/grovedb/src/batch/single_deletion_cost_tests.rs @@ -65,6 +65,67 @@ mod tests { ); } + #[test] + fn v3_delete_keeps_legacy_basic_plus_default_sectioned_removal_cost() { + // GROVE_V3 is live on mainnet with the legacy removal arithmetic that + // drops the mutated default section when a basic removal is combined + // with a sectioned one (issue #683). Replay of v3 blocks depends on + // reproducing that undercount exactly, so v3 must NOT pick up the v4 + // fix exercised by + // `latest_delete_preserves_basic_plus_default_sectioned_removal_cost`. + let grove_version = &grovedb_version::version::v3::GROVE_V3; + let db = make_empty_grovedb(); + + let insertion_cost = db + .insert( + EMPTY_PATH, + b"key1", + Element::new_item_with_flags(b"cat".to_vec(), Some(b"apple".to_vec())), + None, + None, + grove_version, + ) + .cost_as_result() + .expect("expected to insert successfully"); + + let deletion_cost = db + .delete_with_sectional_storage_function( + EMPTY_PATH, + b"key1", + None, + None, + &mut |_element_flags, removed_key_bytes, removed_value_bytes| { + let mut removed_bytes = StorageRemovalPerEpochByIdentifier::default(); + let mut removed_bytes_for_identity = IntMap::new(); + removed_bytes_for_identity.insert(UNKNOWN_EPOCH, removed_value_bytes); + removed_bytes.insert(Identifier::default(), removed_bytes_for_identity); + Ok(( + BasicStorageRemoval(removed_key_bytes), + SectionedStorageRemoval(removed_bytes), + )) + }, + grove_version, + ) + .cost_as_result() + .expect("expected to delete successfully"); + + // Legacy behavior undercounts: the combined removal loses bytes + // relative to what was actually inserted. + assert!( + deletion_cost + .storage_cost + .removed_bytes + .total_removed_bytes() + < insertion_cost.storage_cost.added_bytes, + "v3 must keep the legacy undercounting removal arithmetic; got {} of {} added bytes", + deletion_cost + .storage_cost + .removed_bytes + .total_removed_bytes(), + insertion_cost.storage_cost.added_bytes + ); + } + #[test] fn test_batch_one_deletion_tree_costs_match_non_batch_on_transaction() { let grove_version = GroveVersion::latest(); From 89a1bb442db174e357e1d103adc23b2cd766f3ce Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Sat, 22 Aug 2026 17:50:25 +0700 Subject: [PATCH 4/6] test(batch): pin v3 legacy removal total to the exact shipped figure The v3 regression only asserted the legacy total was below the inserted bytes. Replay compatibility depends on the exact shipped value, so pin it: under GROVE_V3 the combined Basic+=Sectioned removal is an empty sectioned map (0 of 155 added bytes), under GROVE_V4 it is {default: {UNKNOWN_EPOCH: 155}}. Both tests share one helper that also captures the key/value bytes the sectional callback observed, so the undercount is stated explicitly. Co-Authored-By: Claude Fable 5 --- .../src/batch/single_deletion_cost_tests.rs | 140 ++++++++++-------- 1 file changed, 81 insertions(+), 59 deletions(-) diff --git a/grovedb/src/batch/single_deletion_cost_tests.rs b/grovedb/src/batch/single_deletion_cost_tests.rs index 8574a8a71..fec65e185 100644 --- a/grovedb/src/batch/single_deletion_cost_tests.rs +++ b/grovedb/src/batch/single_deletion_cost_tests.rs @@ -4,7 +4,7 @@ mod tests { use grovedb_costs::storage_cost::removal::{ - Identifier, StorageRemovalPerEpochByIdentifier, + Identifier, StorageRemovalPerEpochByIdentifier, StorageRemovedBytes, StorageRemovedBytes::{BasicStorageRemoval, SectionedStorageRemoval}, UNKNOWN_EPOCH, }; @@ -18,9 +18,22 @@ mod tests { Element, }; - #[test] - fn latest_delete_preserves_basic_plus_default_sectioned_removal_cost() { - let grove_version = GroveVersion::latest(); + /// Inserts one flagged item at the root and deletes it through + /// `delete_with_sectional_storage_function`, reporting the removed key + /// bytes as a `BasicStorageRemoval` and the removed value bytes as a + /// `SectionedStorageRemoval` under the default identifier's + /// `UNKNOWN_EPOCH`. That is exactly the shape Drive's sectional removal + /// callback produces, and it is what drives the `Basic += Sectioned` + /// arm in `StorageRemovedBytes` when Merk folds the key and value + /// removals together. + /// + /// Returns `(added_bytes, removed_bytes, removed_key_bytes, + /// removed_value_bytes)` — the insertion's added bytes, the deletion's + /// combined `StorageRemovedBytes`, and the two raw figures the callback + /// observed (so the test can state what the legacy arithmetic loses). + fn insert_then_delete_with_basic_key_and_default_sectioned_value( + grove_version: &GroveVersion, + ) -> (u32, StorageRemovedBytes, u32, u32) { let db = make_empty_grovedb(); let insertion_cost = db @@ -35,6 +48,7 @@ mod tests { .cost_as_result() .expect("expected to insert successfully"); + let mut observed_removed = (0u32, 0u32); let deletion_cost = db .delete_with_sectional_storage_function( EMPTY_PATH, @@ -42,6 +56,7 @@ mod tests { None, None, &mut |_element_flags, removed_key_bytes, removed_value_bytes| { + observed_removed = (removed_key_bytes, removed_value_bytes); let mut removed_bytes = StorageRemovalPerEpochByIdentifier::default(); let mut removed_bytes_for_identity = IntMap::new(); removed_bytes_for_identity.insert(UNKNOWN_EPOCH, removed_value_bytes); @@ -56,12 +71,43 @@ mod tests { .cost_as_result() .expect("expected to delete successfully"); + ( + insertion_cost.storage_cost.added_bytes, + deletion_cost.storage_cost.removed_bytes, + observed_removed.0, + observed_removed.1, + ) + } + + /// Bytes added by inserting `key1 -> Item("cat", flags "apple")` at the + /// root, and therefore the bytes a full-refund deletion must report. + const BASIC_PLUS_DEFAULT_SECTIONED_ADDED_BYTES: u32 = 155; + + #[test] + fn latest_delete_preserves_basic_plus_default_sectioned_removal_cost() { + // GROVE_V4+ (issue #683): folding the basic key removal into the + // sectioned value removal keeps the default section, so the deletion + // refunds every byte the insertion added. + let (added_bytes, removed_bytes, removed_key_bytes, removed_value_bytes) = + insert_then_delete_with_basic_key_and_default_sectioned_value(GroveVersion::latest()); + + assert_eq!(added_bytes, BASIC_PLUS_DEFAULT_SECTIONED_ADDED_BYTES); assert_eq!( - deletion_cost - .storage_cost - .removed_bytes - .total_removed_bytes(), - insertion_cost.storage_cost.added_bytes + removed_key_bytes + removed_value_bytes, + BASIC_PLUS_DEFAULT_SECTIONED_ADDED_BYTES, + "the sectional callback must see every added byte split across key and value" + ); + + // Both the basic key bytes and the sectioned value bytes land in the + // default identifier's UNKNOWN_EPOCH entry. + let mut expected_epochs = IntMap::new(); + expected_epochs.insert(UNKNOWN_EPOCH, BASIC_PLUS_DEFAULT_SECTIONED_ADDED_BYTES); + let mut expected_sections = StorageRemovalPerEpochByIdentifier::default(); + expected_sections.insert(Identifier::default(), expected_epochs); + assert_eq!(removed_bytes, SectionedStorageRemoval(expected_sections)); + assert_eq!( + removed_bytes.total_removed_bytes(), + BASIC_PLUS_DEFAULT_SECTIONED_ADDED_BYTES ); } @@ -70,59 +116,35 @@ mod tests { // GROVE_V3 is live on mainnet with the legacy removal arithmetic that // drops the mutated default section when a basic removal is combined // with a sectioned one (issue #683). Replay of v3 blocks depends on - // reproducing that undercount exactly, so v3 must NOT pick up the v4 - // fix exercised by + // reproducing that undercount EXACTLY, so this pins the legacy figure + // rather than merely asserting "less than added": v3 must report an + // empty sectioned removal — zero bytes — for a deletion whose callback + // observed all 155 added bytes. The v4 fix is exercised by // `latest_delete_preserves_basic_plus_default_sectioned_removal_cost`. - let grove_version = &grovedb_version::version::v3::GROVE_V3; - let db = make_empty_grovedb(); - - let insertion_cost = db - .insert( - EMPTY_PATH, - b"key1", - Element::new_item_with_flags(b"cat".to_vec(), Some(b"apple".to_vec())), - None, - None, - grove_version, - ) - .cost_as_result() - .expect("expected to insert successfully"); + let (added_bytes, removed_bytes, removed_key_bytes, removed_value_bytes) = + insert_then_delete_with_basic_key_and_default_sectioned_value( + &grovedb_version::version::v3::GROVE_V3, + ); - let deletion_cost = db - .delete_with_sectional_storage_function( - EMPTY_PATH, - b"key1", - None, - None, - &mut |_element_flags, removed_key_bytes, removed_value_bytes| { - let mut removed_bytes = StorageRemovalPerEpochByIdentifier::default(); - let mut removed_bytes_for_identity = IntMap::new(); - removed_bytes_for_identity.insert(UNKNOWN_EPOCH, removed_value_bytes); - removed_bytes.insert(Identifier::default(), removed_bytes_for_identity); - Ok(( - BasicStorageRemoval(removed_key_bytes), - SectionedStorageRemoval(removed_bytes), - )) - }, - grove_version, - ) - .cost_as_result() - .expect("expected to delete successfully"); + assert_eq!(added_bytes, BASIC_PLUS_DEFAULT_SECTIONED_ADDED_BYTES); + assert_eq!( + removed_key_bytes + removed_value_bytes, + BASIC_PLUS_DEFAULT_SECTIONED_ADDED_BYTES, + "the sectional callback sees the same bytes under every version" + ); - // Legacy behavior undercounts: the combined removal loses bytes - // relative to what was actually inserted. - assert!( - deletion_cost - .storage_cost - .removed_bytes - .total_removed_bytes() - < insertion_cost.storage_cost.added_bytes, - "v3 must keep the legacy undercounting removal arithmetic; got {} of {} added bytes", - deletion_cost - .storage_cost - .removed_bytes - .total_removed_bytes(), - insertion_cost.storage_cost.added_bytes + // Legacy `Basic += Sectioned`: the default section is removed from the + // map, mutated, and never reinserted — the whole removal is lost. + assert_eq!( + removed_bytes, + SectionedStorageRemoval(StorageRemovalPerEpochByIdentifier::default()), + "v3 must keep the legacy default-section-dropping removal arithmetic" + ); + assert_eq!( + removed_bytes.total_removed_bytes(), + 0, + "v3 legacy total must stay pinned at the shipped value (0 of {} added bytes)", + BASIC_PLUS_DEFAULT_SECTIONED_ADDED_BYTES ); } From c00356e5928530a2b9cd5b3403ba67ca5717eaa3 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Mon, 7 Sep 2026 08:20:09 +0700 Subject: [PATCH 5/6] test(costs): pin the #683 audit C008 removal matrix for legacy and V4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit group C008 (September 2026 addendum on #683) asked for every mixed Basic/Sectioned removal arm and aggregation order to be pinned under both the legacy (V1..V3) and corrected (V4) arithmetic, with a default owner that already carries epoch attribution so both losses — existing attribution and incoming basic bytes — are visible. - costs: exact-map matrix for `Basic + Sectioned`, `Sectioned + Basic`, `Basic += Sectioned`, `Sectioned += Basic` over three default-section shapes (present with UNKNOWN_EPOCH, present without it, absent), under version 0 and 1; `Sectioned += Basic` asserted identical unguarded, v0 and v1; guard nests/restores and the unguarded default is legacy. - grovedb: two epoch-flagged items deleted in one `apply_batch_with_element_flags_update` batch — V4 yields one default section carrying both epochs plus the key bytes under UNKNOWN_EPOCH (302 of 302 bytes); V3 pinned to the empty legacy map (0 of 302). - docs/crates/costs.md: per-arm behaviour table, the guarded entry points, and the requirement that aggregation outside a GroveDB call install the guard to get the corrected arithmetic. Co-Authored-By: Claude Fable 5.1 --- costs/tests/coverage_regression.rs | 297 ++++++++++++++++++ docs/crates/costs.md | 60 ++++ .../src/batch/single_deletion_cost_tests.rs | 140 +++++++++ 3 files changed, 497 insertions(+) diff --git a/costs/tests/coverage_regression.rs b/costs/tests/coverage_regression.rs index 75bbdbba3..c56e90397 100644 --- a/costs/tests/coverage_regression.rs +++ b/costs/tests/coverage_regression.rs @@ -787,3 +787,300 @@ fn error_display_includes_expected_data() { assert!(display.contains("replaced: 9")); assert!(display.contains("actual:10")); } + +// ── Issue #683 / audit C008: basic-into-sectioned removal matrix ───────── +// +// Every arm that folds a `BasicStorageRemoval` into a +// `SectionedStorageRemoval`, in both aggregation orders and both operator +// forms, pinned to the exact resulting map under the legacy (v1..v3) and +// corrected (v4) arithmetic. The default owner already carries attribution +// in two known epochs AND an `UNKNOWN_EPOCH` entry, next to an unrelated +// identity-owned section, so the legacy figures show both losses the audit +// names: the default owner's existing epoch attribution AND the incoming +// basic bytes. + +const AUDIT_IDENTITY: Identifier = [1u8; 32]; +const AUDIT_BASIC_BYTES: u32 = 11; + +fn epochs(entries: &[(u16, u32)]) -> IntMap { + entries.iter().copied().collect() +} + +fn sections(entries: &[(Identifier, IntMap)]) -> StorageRemovedBytes { + SectionedStorageRemoval(entries.iter().cloned().collect::>()) +} + +/// Default owner with attribution in epochs 3 and 5 plus an `UNKNOWN_EPOCH` +/// entry, alongside an identity-owned section in epoch 2. Total 97 bytes. +fn audit_default_with_unknown() -> StorageRemovedBytes { + sections(&[ + ( + Identifier::default(), + epochs(&[ + (3, 20), + (5, 30), + (grovedb_costs::storage_cost::removal::UNKNOWN_EPOCH, 7), + ]), + ), + (AUDIT_IDENTITY, epochs(&[(2, 40)])), + ]) +} + +/// Default owner with attribution in epoch 3 only — no `UNKNOWN_EPOCH` +/// entry — so the legacy "insert fresh UNKNOWN_EPOCH into the detached map" +/// branch is the one that fires. Total 20 bytes. +fn audit_default_without_unknown() -> StorageRemovedBytes { + sections(&[(Identifier::default(), epochs(&[(3, 20)]))]) +} + +/// No default owner at all: only the identity-owned section. Total 40 bytes. +fn audit_default_absent() -> StorageRemovedBytes { + sections(&[(AUDIT_IDENTITY, epochs(&[(2, 40)]))]) +} + +/// Result of every mixed arm for one `sectioned` input and the fixed +/// `AUDIT_BASIC_BYTES` basic removal, evaluated under the version currently +/// selected by the thread-local guard (or its default when unguarded). +#[derive(Debug, PartialEq)] +struct MixedArmOutputs { + basic_plus_sectioned: StorageRemovedBytes, + sectioned_plus_basic: StorageRemovedBytes, + basic_add_assign_sectioned: StorageRemovedBytes, + sectioned_add_assign_basic: StorageRemovedBytes, +} + +fn run_mixed_arms(sectioned: impl Fn() -> StorageRemovedBytes) -> MixedArmOutputs { + let mut basic_add_assign_sectioned = BasicStorageRemoval(AUDIT_BASIC_BYTES); + basic_add_assign_sectioned += sectioned(); + + let mut sectioned_add_assign_basic = sectioned(); + sectioned_add_assign_basic += BasicStorageRemoval(AUDIT_BASIC_BYTES); + + MixedArmOutputs { + basic_plus_sectioned: BasicStorageRemoval(AUDIT_BASIC_BYTES) + sectioned(), + sectioned_plus_basic: sectioned() + BasicStorageRemoval(AUDIT_BASIC_BYTES), + basic_add_assign_sectioned, + sectioned_add_assign_basic, + } +} + +/// The corrected result: the basic bytes land in the default owner's +/// `UNKNOWN_EPOCH` entry and every other entry survives. This is what all +/// four arms must produce under v1, and what `Sectioned += Basic` has always +/// produced. +fn corrected_default_with_unknown() -> StorageRemovedBytes { + sections(&[ + ( + Identifier::default(), + epochs(&[ + (3, 20), + (5, 30), + ( + grovedb_costs::storage_cost::removal::UNKNOWN_EPOCH, + 7 + AUDIT_BASIC_BYTES, + ), + ]), + ), + (AUDIT_IDENTITY, epochs(&[(2, 40)])), + ]) +} + +fn corrected_default_without_unknown() -> StorageRemovedBytes { + sections(&[( + Identifier::default(), + epochs(&[ + (3, 20), + ( + grovedb_costs::storage_cost::removal::UNKNOWN_EPOCH, + AUDIT_BASIC_BYTES, + ), + ]), + )]) +} + +fn corrected_default_absent() -> StorageRemovedBytes { + sections(&[ + ( + Identifier::default(), + epochs(&[( + grovedb_costs::storage_cost::removal::UNKNOWN_EPOCH, + AUDIT_BASIC_BYTES, + )]), + ), + (AUDIT_IDENTITY, epochs(&[(2, 40)])), + ]) +} + +#[test] +fn legacy_basic_sectioned_removal_matrix_pins_shipped_v1_to_v3_output() { + // Version 0 is the shipped v1..v3 arithmetic. The three buggy arms detach + // the default owner's epoch map, fold the basic bytes in, and drop it: + // both the owner's existing attribution (20 + 30 + 7) and the incoming + // basic bytes (11) vanish, leaving only the identity-owned 40. The + // `Sectioned += Basic` sibling reinserts and keeps all 108. + let expected_legacy_loss = sections(&[(AUDIT_IDENTITY, epochs(&[(2, 40)]))]); + let outputs = with_basic_sectioned_removal_addition_version(0, || { + run_mixed_arms(audit_default_with_unknown) + }); + assert_eq!( + outputs, + MixedArmOutputs { + basic_plus_sectioned: expected_legacy_loss.clone(), + sectioned_plus_basic: expected_legacy_loss.clone(), + basic_add_assign_sectioned: expected_legacy_loss.clone(), + sectioned_add_assign_basic: corrected_default_with_unknown(), + } + ); + assert_eq!(outputs.basic_plus_sectioned.total_removed_bytes(), 40); + assert_eq!( + outputs.sectioned_add_assign_basic.total_removed_bytes(), + 108 + ); + + // Default owner present without an UNKNOWN_EPOCH entry: the detached map + // gets a fresh UNKNOWN_EPOCH entry and is then dropped, so the legacy + // result is an EMPTY sectioned removal — zero bytes of 31. + let outputs = with_basic_sectioned_removal_addition_version(0, || { + run_mixed_arms(audit_default_without_unknown) + }); + let empty = SectionedStorageRemoval(BTreeMap::new()); + assert_eq!( + outputs, + MixedArmOutputs { + basic_plus_sectioned: empty.clone(), + sectioned_plus_basic: empty.clone(), + basic_add_assign_sectioned: empty, + sectioned_add_assign_basic: corrected_default_without_unknown(), + } + ); + assert_eq!(outputs.basic_plus_sectioned.total_removed_bytes(), 0); + assert_eq!(outputs.sectioned_add_assign_basic.total_removed_bytes(), 31); + + // Default owner absent: the vacant-entry branch was always correct, so + // every arm agrees even on the legacy path. + let outputs = + with_basic_sectioned_removal_addition_version(0, || run_mixed_arms(audit_default_absent)); + assert_eq!( + outputs, + MixedArmOutputs { + basic_plus_sectioned: corrected_default_absent(), + sectioned_plus_basic: corrected_default_absent(), + basic_add_assign_sectioned: corrected_default_absent(), + sectioned_add_assign_basic: corrected_default_absent(), + } + ); + assert_eq!(outputs.basic_plus_sectioned.total_removed_bytes(), 51); +} + +#[test] +fn v4_basic_sectioned_removal_matrix_preserves_default_section_in_every_arm() { + // Version 1 (GROVE_V4): all four arms reinsert the updated default + // section, so both aggregation orders and both operator forms agree on + // the exact map, and the total is the sum of every input. + let outputs = with_basic_sectioned_removal_addition_version(1, || { + run_mixed_arms(audit_default_with_unknown) + }); + assert_eq!( + outputs, + MixedArmOutputs { + basic_plus_sectioned: corrected_default_with_unknown(), + sectioned_plus_basic: corrected_default_with_unknown(), + basic_add_assign_sectioned: corrected_default_with_unknown(), + sectioned_add_assign_basic: corrected_default_with_unknown(), + } + ); + assert_eq!(outputs.basic_plus_sectioned.total_removed_bytes(), 108); + + let outputs = with_basic_sectioned_removal_addition_version(1, || { + run_mixed_arms(audit_default_without_unknown) + }); + assert_eq!( + outputs, + MixedArmOutputs { + basic_plus_sectioned: corrected_default_without_unknown(), + sectioned_plus_basic: corrected_default_without_unknown(), + basic_add_assign_sectioned: corrected_default_without_unknown(), + sectioned_add_assign_basic: corrected_default_without_unknown(), + } + ); + assert_eq!(outputs.basic_plus_sectioned.total_removed_bytes(), 31); + + let outputs = + with_basic_sectioned_removal_addition_version(1, || run_mixed_arms(audit_default_absent)); + assert_eq!( + outputs, + MixedArmOutputs { + basic_plus_sectioned: corrected_default_absent(), + sectioned_plus_basic: corrected_default_absent(), + basic_add_assign_sectioned: corrected_default_absent(), + sectioned_add_assign_basic: corrected_default_absent(), + } + ); + assert_eq!(outputs.basic_plus_sectioned.total_removed_bytes(), 51); +} + +#[test] +fn sectioned_add_assign_basic_is_identical_across_removal_versions() { + // The always-correct sibling control: `Sectioned += Basic` must produce + // the same exact map whether the legacy or the corrected arithmetic is + // selected, and whether or not any guard is installed at all. + for sectioned in [ + audit_default_with_unknown as fn() -> StorageRemovedBytes, + audit_default_without_unknown, + audit_default_absent, + ] { + let unguarded = run_mixed_arms(sectioned).sectioned_add_assign_basic; + let legacy = with_basic_sectioned_removal_addition_version(0, || { + run_mixed_arms(sectioned).sectioned_add_assign_basic + }); + let corrected = with_basic_sectioned_removal_addition_version(1, || { + run_mixed_arms(sectioned).sectioned_add_assign_basic + }); + assert_eq!(unguarded, legacy); + assert_eq!(legacy, corrected); + assert_eq!( + corrected.total_removed_bytes(), + sectioned().total_removed_bytes() + AUDIT_BASIC_BYTES + ); + } +} + +#[test] +fn removal_addition_version_defaults_to_legacy_and_guard_restores_previous() { + // Unguarded aggregation (a caller outside any GroveDB entry point) runs + // the legacy arithmetic — the safe direction, since it reproduces shipped + // output rather than silently upgrading. + let expected_legacy_loss = sections(&[(AUDIT_IDENTITY, epochs(&[(2, 40)]))]); + assert_eq!( + run_mixed_arms(audit_default_with_unknown).basic_plus_sectioned, + expected_legacy_loss + ); + + // Guards nest and restore: inside v1 the fix applies; a nested v0 guard + // reverts to legacy; dropping it returns to v1; dropping the outer guard + // returns to the unguarded default. + with_basic_sectioned_removal_addition_version(1, || { + assert_eq!( + run_mixed_arms(audit_default_with_unknown).basic_plus_sectioned, + corrected_default_with_unknown() + ); + { + let _inner = + grovedb_costs::storage_cost::removal::use_basic_sectioned_removal_addition_version( + 0, + ); + assert_eq!( + run_mixed_arms(audit_default_with_unknown).basic_plus_sectioned, + expected_legacy_loss + ); + } + assert_eq!( + run_mixed_arms(audit_default_with_unknown).basic_plus_sectioned, + corrected_default_with_unknown() + ); + }); + assert_eq!( + run_mixed_arms(audit_default_with_unknown).basic_plus_sectioned, + expected_legacy_loss + ); +} diff --git a/docs/crates/costs.md b/docs/crates/costs.md index 62647c5c6..d3d92db90 100644 --- a/docs/crates/costs.md +++ b/docs/crates/costs.md @@ -42,6 +42,66 @@ pub struct StorageCost { - `replaced_bytes`: Existing data overwritten - `removed_bytes`: Data deleted from storage +#### Removed bytes: folding a basic removal into a sectioned one (issue #683) + +`removed_bytes` is a `StorageRemovedBytes`: `NoStorageRemoval`, a plain +`BasicStorageRemoval(u32)`, or a `SectionedStorageRemoval` map of +`owner identifier → epoch → bytes` (Drive attributes refunds by owner and +epoch through it). When a basic removal is combined with a sectioned one the +basic bytes are folded into the **default owner's** (`[0; 32]`) section +under `UNKNOWN_EPOCH`. Four operator arms do this: `Basic + Sectioned`, +`Sectioned + Basic`, `Basic += Sectioned` and `Sectioned += Basic`. + +Three of them shipped with a defect: when the default owner already had a +section they detached its epoch map, folded the basic bytes in, and never +reinserted it, so the owner's existing epoch attribution AND the incoming +basic bytes were both lost (only identity-owned sections survived). +`Sectioned += Basic` always reinserted correctly and is version-independent. + +The arithmetic is selected by +`grovedb_versions.storage_costs.add_basic_storage_removal_to_sectioned_storage_removal`: + +| arm | GROVE_V1..V3 (v0, legacy) | GROVE_V4 (v1) | +|---|---|---| +| `Basic + Sectioned`, `Sectioned + Basic`, `Basic += Sectioned` | default owner present: its section is dropped (its epochs and the basic bytes vanish); default owner absent: correct | default section preserved, basic bytes added to its `UNKNOWN_EPOCH` entry | +| `Sectioned += Basic` | correct | correct (unchanged) | + +Legacy output is kept byte-exact because removal totals are part of the +replayed cost record. Because the operator impls cannot carry a +`GroveVersion` (and `grovedb-costs` has no `grovedb-version` dependency), +the selected version travels in a thread-local installed by an RAII guard +(`use_basic_sectioned_removal_addition_version` / +`with_basic_sectioned_removal_addition_version`) at the version-aware entry +points: `Merk::apply_unchecked_with_old_value_observer` (which every Merk +apply funnels through), `GroveDb::delete_with_sectional_storage_function`, +`delete_if_empty_tree_with_sectional_storage_function`, +`apply_batch_with_element_flags_update` and +`apply_partial_batch_with_element_flags_update` (so `delete_up_tree_while_empty_with_sectional_storage` +is covered too). The storage-batch commit that sums +`KeyValueStorageCost::combined_removed_bytes` runs inside those scopes. + +The unguarded default is `0` (legacy): a caller that never installs a guard +reproduces shipped output rather than silently upgrading. **Any consumer +that combines `StorageRemovedBytes` outside a GroveDB call** — for example +summing per-operation `OperationCost`s or `StorageCost`s across operations — +runs the legacy arithmetic even under GROVE_V4 unless it installs the guard +itself around that aggregation: + +```rust +let _guard = grovedb_costs::storage_cost::removal::use_basic_sectioned_removal_addition_version( + grove_version + .grovedb_versions + .storage_costs + .add_basic_storage_removal_to_sectioned_storage_removal, +); +total_cost += op_cost; // basic-into-default-section folds now use the selected version +``` + +The exact per-arm maps for both versions are pinned in +`costs/tests/coverage_regression.rs` (the `*_basic_sectioned_removal_matrix_*` +tests) and the GroveDB entry points in +`grovedb/src/batch/single_deletion_cost_tests.rs`. + ### CostResult A wrapper type that pairs computation results with their costs: diff --git a/grovedb/src/batch/single_deletion_cost_tests.rs b/grovedb/src/batch/single_deletion_cost_tests.rs index fec65e185..f0058b3ae 100644 --- a/grovedb/src/batch/single_deletion_cost_tests.rs +++ b/grovedb/src/batch/single_deletion_cost_tests.rs @@ -148,6 +148,146 @@ mod tests { ); } + /// Inserts two flagged items at the root and deletes both in ONE batch + /// through `apply_batch_with_element_flags_update`, reporting each + /// deletion's key bytes as a `BasicStorageRemoval` and its value bytes as + /// a `SectionedStorageRemoval` under the default identifier, attributed to + /// the epoch stored in the element's flags (`key1` → epoch 3, `key2` → + /// epoch 5). Exercises the guarded batch entry point (issue #683, audit + /// C008) and the aggregation the audit names: a default section that + /// already carries epoch attribution combined with incoming basic bytes. + /// + /// Returns `(total_added_bytes, removed_bytes, observed)` where `observed` + /// maps each epoch to the `(key_bytes, value_bytes)` the callback saw. + fn insert_two_epoch_flagged_items_then_delete_in_batch( + grove_version: &GroveVersion, + ) -> ( + u32, + StorageRemovedBytes, + std::collections::BTreeMap, + ) { + let db = make_empty_grovedb(); + + let mut total_added_bytes = 0u32; + for (key, epoch) in [(b"key1".as_slice(), 3u8), (b"key2".as_slice(), 5u8)] { + total_added_bytes += db + .insert( + EMPTY_PATH, + key, + Element::new_item_with_flags(b"cat".to_vec(), Some(vec![epoch])), + None, + None, + grove_version, + ) + .cost_as_result() + .expect("expected to insert successfully") + .storage_cost + .added_bytes; + } + + let tx = db.start_transaction(); + let mut observed = std::collections::BTreeMap::new(); + let ops = vec![ + QualifiedGroveDbOp::delete_op(vec![], b"key1".to_vec()), + QualifiedGroveDbOp::delete_op(vec![], b"key2".to_vec()), + ]; + let batch_cost = db + .apply_batch_with_element_flags_update( + ops, + None, + |_, _, _| Ok(false), + |element_flags, removed_key_bytes, removed_value_bytes| { + let epoch = element_flags[0] as u16; + observed.insert(epoch, (removed_key_bytes, removed_value_bytes)); + let mut removed_bytes = StorageRemovalPerEpochByIdentifier::default(); + let mut removed_bytes_for_identity = IntMap::new(); + removed_bytes_for_identity.insert(epoch, removed_value_bytes); + removed_bytes.insert(Identifier::default(), removed_bytes_for_identity); + Ok(( + BasicStorageRemoval(removed_key_bytes), + SectionedStorageRemoval(removed_bytes), + )) + }, + Some(&tx), + grove_version, + ) + .cost_as_result() + .expect("expected to delete successfully"); + tx.commit().expect("expected to commit"); + + ( + total_added_bytes, + batch_cost.storage_cost.removed_bytes, + observed, + ) + } + + /// Bytes added by inserting `key1 -> Item("cat", flags [3])` and + /// `key2 -> Item("cat", flags [5])` at the root, and therefore the bytes a + /// full-refund batch deletion of both must report. + const TWO_EPOCH_FLAGGED_ITEMS_ADDED_BYTES: u32 = 302; + + #[test] + fn latest_batch_delete_preserves_epoch_attribution_and_basic_bytes() { + // GROVE_V4+ (issue #683): each deletion's `Basic + Sectioned{default: + // {epoch}}` fold keeps the default section, and the two deletions + // merge into one default section carrying both epochs plus the key + // bytes under UNKNOWN_EPOCH. + let (added_bytes, removed_bytes, observed) = + insert_two_epoch_flagged_items_then_delete_in_batch(GroveVersion::latest()); + + assert_eq!(added_bytes, TWO_EPOCH_FLAGGED_ITEMS_ADDED_BYTES); + let (key_bytes_3, value_bytes_3) = observed[&3]; + let (key_bytes_5, value_bytes_5) = observed[&5]; + assert_eq!( + key_bytes_3 + value_bytes_3 + key_bytes_5 + value_bytes_5, + TWO_EPOCH_FLAGGED_ITEMS_ADDED_BYTES, + "the sectional callback must see every added byte across both deletions" + ); + + let mut expected_epochs = IntMap::new(); + expected_epochs.insert(3, value_bytes_3); + expected_epochs.insert(5, value_bytes_5); + expected_epochs.insert(UNKNOWN_EPOCH, key_bytes_3 + key_bytes_5); + let mut expected_sections = StorageRemovalPerEpochByIdentifier::default(); + expected_sections.insert(Identifier::default(), expected_epochs); + assert_eq!(removed_bytes, SectionedStorageRemoval(expected_sections)); + assert_eq!( + removed_bytes.total_removed_bytes(), + TWO_EPOCH_FLAGGED_ITEMS_ADDED_BYTES + ); + } + + #[test] + fn v3_batch_delete_keeps_legacy_loss_of_epoch_attribution_and_basic_bytes() { + // GROVE_V3 legacy: the default section holds the epoch-attributed + // value bytes but no UNKNOWN_EPOCH entry, so the legacy `Basic + + // Sectioned` fold detaches it, adds a fresh UNKNOWN_EPOCH entry and + // drops the whole map. Both the epoch attribution and the basic key + // bytes are lost for both deletions — pinned exactly at an empty + // sectioned removal, 0 of 302 added bytes. + let (added_bytes, removed_bytes, observed) = + insert_two_epoch_flagged_items_then_delete_in_batch( + &grovedb_version::version::v3::GROVE_V3, + ); + + assert_eq!(added_bytes, TWO_EPOCH_FLAGGED_ITEMS_ADDED_BYTES); + let (key_bytes_3, value_bytes_3) = observed[&3]; + let (key_bytes_5, value_bytes_5) = observed[&5]; + assert_eq!( + key_bytes_3 + value_bytes_3 + key_bytes_5 + value_bytes_5, + TWO_EPOCH_FLAGGED_ITEMS_ADDED_BYTES, + "the sectional callback sees the same bytes under every version" + ); + + assert_eq!( + removed_bytes, + SectionedStorageRemoval(StorageRemovalPerEpochByIdentifier::default()), + "v3 must keep the legacy default-section-dropping removal arithmetic" + ); + assert_eq!(removed_bytes.total_removed_bytes(), 0); + } + #[test] fn test_batch_one_deletion_tree_costs_match_non_batch_on_transaction() { let grove_version = GroveVersion::latest(); From f94265e567979c11ad4e53f4bc3ab75fe050337d Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Mon, 7 Sep 2026 08:36:48 +0700 Subject: [PATCH 6/6] fix(costs): bind removal version guard to its thread --- costs/src/storage_cost/removal.rs | 44 +++++++++++++-- costs/tests/coverage_regression.rs | 55 +++++++++++++++++++ docs/crates/costs.md | 18 +++++- grovedb-version/src/version/v3.rs | 4 +- grovedb-version/src/version/v4.rs | 5 +- .../src/batch/single_deletion_cost_tests.rs | 14 ++--- 6 files changed, 122 insertions(+), 18 deletions(-) diff --git a/costs/src/storage_cost/removal.rs b/costs/src/storage_cost/removal.rs index 1c3d33120..bb9718f3d 100644 --- a/costs/src/storage_cost/removal.rs +++ b/costs/src/storage_cost/removal.rs @@ -31,6 +31,7 @@ use std::{ cell::Cell, cmp::Ordering, collections::BTreeMap, + marker::PhantomData, ops::{Add, AddAssign}, }; @@ -86,10 +87,35 @@ thread_local! { static BASIC_SECTIONED_REMOVAL_ADDITION_VERSION: Cell = const { Cell::new(0) }; } -/// Guard that restores the previous storage-removal arithmetic version when -/// dropped. +/// Guard that restores the previous storage-removal arithmetic version on the +/// current thread when dropped. +/// +/// Keep guards in synchronous scopes and drop nested guards in reverse creation +/// order. Never hold a guard across an `.await`: other tasks on the same thread +/// would observe its version while the owning task is suspended. +/// +/// The guard cannot be moved to another thread: +/// +/// ```compile_fail,E0277 +/// use grovedb_costs::storage_cost::removal::use_basic_sectioned_removal_addition_version; +/// +/// let guard = use_basic_sectioned_removal_addition_version(1); +/// std::thread::spawn(move || drop(guard)); +/// ``` +/// +/// Nor can it be shared between threads: +/// +/// ```compile_fail,E0277 +/// use grovedb_costs::storage_cost::removal::BasicSectionedRemovalAdditionVersionGuard; +/// +/// fn require_sync() {} +/// require_sync::(); +/// ``` +#[must_use = "keep the guard alive for the synchronous storage-removal aggregation scope"] pub struct BasicSectionedRemovalAdditionVersionGuard { previous_version: u16, + // Drop restores thread-local state, so the guard must be !Send and !Sync. + _not_send_or_sync: PhantomData<*mut ()>, } impl Drop for BasicSectionedRemovalAdditionVersionGuard { @@ -100,17 +126,23 @@ impl Drop for BasicSectionedRemovalAdditionVersionGuard { } } -/// Use a storage-removal arithmetic version until the returned guard is -/// dropped. +/// Use a storage-removal arithmetic version on the current thread until the +/// returned guard is dropped. Keep it in a synchronous scope without `.await`, +/// and drop nested guards in reverse creation order. pub fn use_basic_sectioned_removal_addition_version( version: u16, ) -> BasicSectionedRemovalAdditionVersionGuard { let previous_version = BASIC_SECTIONED_REMOVAL_ADDITION_VERSION .with(|current_version| current_version.replace(version)); - BasicSectionedRemovalAdditionVersionGuard { previous_version } + BasicSectionedRemovalAdditionVersionGuard { + previous_version, + _not_send_or_sync: PhantomData, + } } -/// Run storage-removal arithmetic using a specific version. +/// Run synchronous storage-removal arithmetic using a specific version on the +/// current thread. If `f` returns a future, the guard has already been dropped +/// when that future is polled; perform the arithmetic inside `f` itself. pub fn with_basic_sectioned_removal_addition_version(version: u16, f: impl FnOnce() -> T) -> T { let _guard = use_basic_sectioned_removal_addition_version(version); f() diff --git a/costs/tests/coverage_regression.rs b/costs/tests/coverage_regression.rs index c56e90397..b9789ba00 100644 --- a/costs/tests/coverage_regression.rs +++ b/costs/tests/coverage_regression.rs @@ -1084,3 +1084,58 @@ fn removal_addition_version_defaults_to_legacy_and_guard_restores_previous() { expected_legacy_loss ); } + +#[test] +fn removal_addition_version_is_isolated_between_threads() { + let expected_legacy_loss = sections(&[(AUDIT_IDENTITY, epochs(&[(2, 40)]))]); + with_basic_sectioned_removal_addition_version(1, || { + std::thread::spawn(|| { + let expected_legacy_loss = sections(&[(AUDIT_IDENTITY, epochs(&[(2, 40)]))]); + assert_eq!( + run_mixed_arms(audit_default_with_unknown).basic_plus_sectioned, + expected_legacy_loss + ); + with_basic_sectioned_removal_addition_version(1, || { + assert_eq!( + run_mixed_arms(audit_default_with_unknown).basic_plus_sectioned, + corrected_default_with_unknown() + ); + }); + assert_eq!( + run_mixed_arms(audit_default_with_unknown).basic_plus_sectioned, + expected_legacy_loss + ); + }) + .join() + .expect("worker should preserve its own removal arithmetic version"); + + assert_eq!( + run_mixed_arms(audit_default_with_unknown).basic_plus_sectioned, + corrected_default_with_unknown() + ); + }); + assert_eq!( + run_mixed_arms(audit_default_with_unknown).basic_plus_sectioned, + expected_legacy_loss + ); +} + +#[test] +fn removal_addition_version_restores_previous_after_panic() { + with_basic_sectioned_removal_addition_version(1, || { + let result = std::panic::catch_unwind(|| { + with_basic_sectioned_removal_addition_version(0, || { + panic!("unwind a nested removal arithmetic scope"); + }); + }); + assert!(result.is_err()); + assert_eq!( + run_mixed_arms(audit_default_with_unknown).basic_plus_sectioned, + corrected_default_with_unknown() + ); + }); + assert_eq!( + run_mixed_arms(audit_default_with_unknown).basic_plus_sectioned, + sections(&[(AUDIT_IDENTITY, epochs(&[(2, 40)]))]) + ); +} diff --git a/docs/crates/costs.md b/docs/crates/costs.md index d3d92db90..2ff9cca8f 100644 --- a/docs/crates/costs.md +++ b/docs/crates/costs.md @@ -58,6 +58,15 @@ reinserted it, so the owner's existing epoch attribution AND the incoming basic bytes were both lost (only identity-owned sections survived). `Sectioned += Basic` always reinserted correctly and is version-independent. +Drive's storage-flags callback returns basic/basic removals for unflagged +elements and sectioned/sectioned removals for flagged elements (or no removal +for zero bytes). Its mixed-removal case arises when those results are +aggregated. Drive separates the default identifier's section into +`FeeResult.removed_bytes_from_system` before calculating identity refunds; +identity-owned sections are unaffected by this defect. The GroveDB deletion +regressions use custom basic/sectioned callbacks to exercise the arithmetic +directly, so their lost-byte totals do not establish lost identity refunds. + The arithmetic is selected by `grovedb_versions.storage_costs.add_basic_storage_removal_to_sectioned_storage_removal`: @@ -85,7 +94,14 @@ reproduces shipped output rather than silently upgrading. **Any consumer that combines `StorageRemovedBytes` outside a GroveDB call** — for example summing per-operation `OperationCost`s or `StorageCost`s across operations — runs the legacy arithmetic even under GROVE_V4 unless it installs the guard -itself around that aggregation: +itself around that aggregation. + +The guard is `!Send` and `!Sync` so it stays on its originating thread. Keep it +in a synchronous scope, drop nested guards in reverse creation order, and +never hold it across an `.await`: even on one thread, other tasks would +observe its version while the owning task is suspended. The closure helper +also scopes only synchronous work, not a future returned by the closure. +For example: ```rust let _guard = grovedb_costs::storage_cost::removal::use_basic_sectioned_removal_addition_version( diff --git a/grovedb-version/src/version/v3.rs b/grovedb-version/src/version/v3.rs index 1648bc670..cd19db595 100644 --- a/grovedb-version/src/version/v3.rs +++ b/grovedb-version/src/version/v3.rs @@ -247,8 +247,8 @@ pub const GROVE_V3: GroveVersion = GroveVersion { }, storage_costs: GroveDBStorageCostVersions { // GROVE_V3 shipped to mainnet with the legacy (default-section- - // dropping) removal arithmetic; changing it here would break - // replay of v3 blocks. The fix activates in GROVE_V4. + // dropping) removal arithmetic. Preserve those cost results for + // callers replaying v3 operations; the fix activates in GROVE_V4. add_basic_storage_removal_to_sectioned_storage_removal: 0, }, replication: GroveDBReplicationVersions { diff --git a/grovedb-version/src/version/v4.rs b/grovedb-version/src/version/v4.rs index 54e52f1b2..2638ee845 100644 --- a/grovedb-version/src/version/v4.rs +++ b/grovedb-version/src/version/v4.rs @@ -233,8 +233,9 @@ //! entry while PRESERVING the rest of the default section (issue #683). //! V1..V3 keep the shipped arithmetic, which drops the mutated default //! section in three of the four `Add`/`AddAssign` arms, undercounting -//! removed bytes — preserved because removal totals feed fee refunds and -//! historical blocks must replay to identical costs. +//! removed bytes — preserved to reproduce historical cost results. +//! Identity-owned sections are unaffected; Drive accounts for the default +//! section as system removals, separately from identity fee refunds. //! //! Note that `GroveVersion::latest()` resolves to this version, so anything //! defaulting to "latest" — tests, benchmarks, tools — exercises every gate diff --git a/grovedb/src/batch/single_deletion_cost_tests.rs b/grovedb/src/batch/single_deletion_cost_tests.rs index f0058b3ae..04d6bb99b 100644 --- a/grovedb/src/batch/single_deletion_cost_tests.rs +++ b/grovedb/src/batch/single_deletion_cost_tests.rs @@ -22,10 +22,10 @@ mod tests { /// `delete_with_sectional_storage_function`, reporting the removed key /// bytes as a `BasicStorageRemoval` and the removed value bytes as a /// `SectionedStorageRemoval` under the default identifier's - /// `UNKNOWN_EPOCH`. That is exactly the shape Drive's sectional removal - /// callback produces, and it is what drives the `Basic += Sectioned` - /// arm in `StorageRemovedBytes` when Merk folds the key and value - /// removals together. + /// `UNKNOWN_EPOCH`. This custom callback exercises the mixed-removal + /// arithmetic directly. Drive's storage-flags callback instead returns + /// basic/basic removals for unflagged elements and sectioned/sectioned + /// removals for flagged elements (or no removal for zero bytes). /// /// Returns `(added_bytes, removed_bytes, removed_key_bytes, /// removed_value_bytes)` — the insertion's added bytes, the deletion's @@ -80,14 +80,14 @@ mod tests { } /// Bytes added by inserting `key1 -> Item("cat", flags "apple")` at the - /// root, and therefore the bytes a full-refund deletion must report. + /// root, and therefore the bytes a complete deletion must report. const BASIC_PLUS_DEFAULT_SECTIONED_ADDED_BYTES: u32 = 155; #[test] fn latest_delete_preserves_basic_plus_default_sectioned_removal_cost() { // GROVE_V4+ (issue #683): folding the basic key removal into the // sectioned value removal keeps the default section, so the deletion - // refunds every byte the insertion added. + // accounts for every byte the insertion added. let (added_bytes, removed_bytes, removed_key_bytes, removed_value_bytes) = insert_then_delete_with_basic_key_and_default_sectioned_value(GroveVersion::latest()); @@ -224,7 +224,7 @@ mod tests { /// Bytes added by inserting `key1 -> Item("cat", flags [3])` and /// `key2 -> Item("cat", flags [5])` at the root, and therefore the bytes a - /// full-refund batch deletion of both must report. + /// complete batch deletion of both must report. const TWO_EPOCH_FLAGGED_ITEMS_ADDED_BYTES: u32 = 302; #[test]