diff --git a/costs/src/storage_cost/removal.rs b/costs/src/storage_cost/removal.rs index ca5fed186..bb9718f3d 100644 --- a/costs/src/storage_cost/removal.rs +++ b/costs/src/storage_cost/removal.rs @@ -28,8 +28,10 @@ use std::{ borrow::BorrowMut, + cell::Cell, cmp::Ordering, collections::BTreeMap, + marker::PhantomData, ops::{Add, AddAssign}, }; @@ -60,6 +62,149 @@ 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..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 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 +// entirely (see [`AddAssign`]). +thread_local! { + static BASIC_SECTIONED_REMOVAL_ADDITION_VERSION: Cell = const { Cell::new(0) }; +} + +/// 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 { + fn drop(&mut self) { + BASIC_SECTIONED_REMOVAL_ADDITION_VERSION.with(|current_version| { + current_version.set(self.previous_version); + }); + } +} + +/// 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, + _not_send_or_sync: PhantomData, + } +} + +/// 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() +} + +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 (v4+) path of the three historically-buggy arms. +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)); +} + +/// 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 +/// historically-buggy arms. +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); + } + } +} + +/// 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; 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( + 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 +219,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 +265,22 @@ 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); + // `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..v3 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 3fadc8792..b9789ba00 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,33 @@ 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); + + // `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(), 13); sectioned_assign += sectioned(identifier_a, 10, 3); assert!(sectioned_assign.has_removal()); @@ -692,6 +723,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 { @@ -708,3 +787,355 @@ 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 + ); +} + +#[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 62647c5c6..2ff9cca8f 100644 --- a/docs/crates/costs.md +++ b/docs/crates/costs.md @@ -42,6 +42,82 @@ 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. + +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`: + +| 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. + +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( + 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-version/src/tests.rs b/grovedb-version/src/tests.rs index 7d68f661a..4420084cc 100644 --- a/grovedb-version/src/tests.rs +++ b/grovedb-version/src/tests.rs @@ -198,6 +198,41 @@ fn v2_has_updated_merk_average_case_costs() { ); } +#[test] +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 + .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, + 0 + ); + assert_eq!( + GROVE_V4 + .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..cd19db595 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,12 @@ pub const GROVE_V3: GroveVersion = GroveVersion { unified_read_mode: 0, per_instance_query_limits: 0, }, + storage_costs: GroveDBStorageCostVersions { + // GROVE_V3 shipped to mainnet with the legacy (default-section- + // 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 { get_subtrees_metadata: 0, fetch_chunk: 0, diff --git a/grovedb-version/src/version/v4.rs b/grovedb-version/src/version/v4.rs index 88b56c5b2..2638ee845 100644 --- a/grovedb-version/src/version/v4.rs +++ b/grovedb-version/src/version/v4.rs @@ -227,6 +227,16 @@ //! 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 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 //! listed above rather than V3 behaviour. @@ -256,7 +266,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 +520,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/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..04d6bb99b 100644 --- a/grovedb/src/batch/single_deletion_cost_tests.rs +++ b/grovedb/src/batch/single_deletion_cost_tests.rs @@ -4,8 +4,9 @@ mod tests { use grovedb_costs::storage_cost::removal::{ - Identifier, StorageRemovalPerEpochByIdentifier, - StorageRemovedBytes::SectionedStorageRemoval, + Identifier, StorageRemovalPerEpochByIdentifier, StorageRemovedBytes, + StorageRemovedBytes::{BasicStorageRemoval, SectionedStorageRemoval}, + UNKNOWN_EPOCH, }; use grovedb_merk::tree_type::TreeType; use grovedb_version::version::GroveVersion; @@ -17,6 +18,276 @@ mod tests { Element, }; + /// 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`. 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 + /// 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 + .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 mut observed_removed = (0u32, 0u32); + let deletion_cost = db + .delete_with_sectional_storage_function( + EMPTY_PATH, + b"key1", + 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); + 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"); + + ( + 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 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 + // 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()); + + 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 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 + ); + } + + #[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 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 (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, + ); + + 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 `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 + ); + } + + /// 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 + /// complete 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(); 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, - ) - }) } }