Skip to content

fix: version-gate commit_batch accumulated costs #637

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
QuantumExplorer merged 2 commits into from
Mar 10, 2026
Merged

fix: version-gate commit_batch accumulated costs #637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
041ed2c
fix: version-gate commit_batch accumulated costs
QuantumExplorer Mar 10, 2026
460c1a1
refactor: rename MerkCommitVersions to MerkBatchVersions
QuantumExplorer Mar 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions grovedb-version/src/version/merk_versions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,17 @@ use versioned_feature_core::FeatureVersion;

#[derive(Clone, Debug, Default)]
pub struct MerkVersions {
pub batch: MerkBatchVersions,
pub average_case_costs: MerkAverageCaseCostsVersions,
}

#[derive(Clone, Debug, Default)]
pub struct MerkBatchVersions {
/// Version 0: commit_batch discards accumulated batch costs (legacy bug)
/// Version 1: commit_batch returns accumulated batch costs
pub commit: FeatureVersion,
}

#[derive(Clone, Debug, Default)]
pub struct MerkAverageCaseCostsVersions {
pub add_average_case_merk_propagate: FeatureVersion,
Expand Down
3 changes: 2 additions & 1 deletion grovedb-version/src/version/v1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::version::{
GroveDBOperationsWorstCaseVersions, GroveDBPathQueryMethodVersions, GroveDBQueryLimits,
GroveDBReplicationVersions, GroveDBVersions,
},
merk_versions::{MerkAverageCaseCostsVersions, MerkVersions},
merk_versions::{MerkAverageCaseCostsVersions, MerkBatchVersions, MerkVersions},
GroveVersion,
};

Expand Down Expand Up @@ -206,6 +206,7 @@ pub const GROVE_V1: GroveVersion = GroveVersion {
},
},
merk_versions: MerkVersions {
batch: MerkBatchVersions { commit: 0 },
average_case_costs: MerkAverageCaseCostsVersions {
add_average_case_merk_propagate: 0,
sum_tree_estimated_size: 0,
Expand Down
3 changes: 2 additions & 1 deletion grovedb-version/src/version/v2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::version::{
GroveDBOperationsWorstCaseVersions, GroveDBPathQueryMethodVersions, GroveDBQueryLimits,
GroveDBReplicationVersions, GroveDBVersions,
},
merk_versions::{MerkAverageCaseCostsVersions, MerkVersions},
merk_versions::{MerkAverageCaseCostsVersions, MerkBatchVersions, MerkVersions},
GroveVersion,
};

Expand Down Expand Up @@ -206,6 +206,7 @@ pub const GROVE_V2: GroveVersion = GroveVersion {
},
},
merk_versions: MerkVersions {
batch: MerkBatchVersions { commit: 0 },
average_case_costs: MerkAverageCaseCostsVersions {
add_average_case_merk_propagate: 1, // changed
sum_tree_estimated_size: 1, // changed
Expand Down
3 changes: 2 additions & 1 deletion grovedb-version/src/version/v3.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::version::{
GroveDBOperationsWorstCaseVersions, GroveDBPathQueryMethodVersions, GroveDBQueryLimits,
GroveDBReplicationVersions, GroveDBVersions,
},
merk_versions::{MerkAverageCaseCostsVersions, MerkVersions},
merk_versions::{MerkAverageCaseCostsVersions, MerkBatchVersions, MerkVersions},
GroveVersion,
};

Expand Down Expand Up @@ -206,6 +206,7 @@ pub const GROVE_V3: GroveVersion = GroveVersion {
},
},
merk_versions: MerkVersions {
batch: MerkBatchVersions { commit: 1 }, // return accumulated batch costs
average_case_costs: MerkAverageCaseCostsVersions {
add_average_case_merk_propagate: 1,
sum_tree_estimated_size: 1,
Expand Down
8 changes: 7 additions & 1 deletion merk/src/merk/apply.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,13 @@ where
// 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)
self.commit(
key_updates,
aux,
options,
old_specialized_cost,
grove_version,
)
})
}
}
16 changes: 12 additions & 4 deletions merk/src/merk/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ where
aux: &AuxMerkBatch<K>,
options: Option<MerkOptions>,
old_specialized_cost: &impl Fn(&Vec<u8>, &Vec<u8>) -> Result<u32, Error>,
grove_version: &GroveVersion,
) -> CostResult<(), Error>
where
K: AsRef<[u8]>,
Expand Down Expand Up @@ -501,10 +502,17 @@ where
}

// write to db
self.storage
.commit_batch(batch)
.map_err(StorageError)
.add_cost(cost)
let commit_result = self.storage.commit_batch(batch).map_err(StorageError);

@coderabbitai coderabbitai Bot Mar 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add call-site context before returning commit_batch failures.

This new path collapses the final batch flush failure into a bare StorageError, which makes it much harder to tell from logs that the error came from commit_batch rather than earlier tree work.

🛠️ Proposed fix 
-        let commit_result = self.storage.commit_batch(batch).map_err(StorageError);
+        let commit_result = self
+            .storage
+            .commit_batch(batch)
+            .map_err(|e| Error::CorruptedData(format!("commit_batch failed: {}", e)));
As per coding guidelines, "Wrap errors with context using `.map_err(|e| Error::CorruptedData(format!("context: {}", e)))` pattern in Rust source files".
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let commit_result = self.storage.commit_batch(batch).map_err(StorageError);
let commit_result = self
.storage
.commit_batch(batch)
.map_err(|e| Error::CorruptedData(format!("commit_batch failed: {}", e)));
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@merk/src/merk/mod.rs` at line 505, The call to
self.storage.commit_batch(batch) currently maps all failures to StorageError,
losing call-site context; change the mapping to wrap the error with a
descriptive message (e.g., via .map_err(|e|
Error::CorruptedData(format!("commit_batch failed: {}", e)))) so the returned
error includes that it originated from commit_batch; update the binding
commit_result (or its use) to use this contextualized map_err instead of
StorageError.

if grove_version.merk_versions.batch.commit >= 1 {
// V1+: preserve accumulated batch costs (seek counts, storage costs)
commit_result.add_cost(cost)
} else {
// V0: discard batch costs (legacy behavior)
CostContext {
value: commit_result.value,
cost,
}
}
}

/// Walk
Expand Down
Loading