Skip to content

Commit 658daec

Browse files
committed
fix(warp-core): charge the node record against the create write budget
The create-from-absence write charge counted only 32 (attachment type id) + replacement_len, but the same patch also writes WarpOp::UpsertNode's 32-byte NodeRecord.ty. An invocation delegated only enough write budget for the attachment could commit 32 more bytes than declared, and the receipt's consumed_budget underreported actual resource consumption. Found by Codex's automated review on PR #686. Changed the charge to 64 (node type + attachment type) + replacement_len; the update path's 32 + replacement_len is untouched, since that branch never writes a node record. RED: create_from_absence_charges_the_node_record_against_the_write_budget delegated a write budget between the old (39) and correct (71) requirement for a 7-byte replacement and expected BudgetExceeded; the buggy charge let it commit instead. GREEN: same test passes; updated create_from_absence_commits_one_new_node_and_attachment_patch's consumed-budget assertion (39 -> 71) to match the corrected charge. 23 tests pass (was 22); full pipeline suite green.
1 parent 361dae1 commit 658daec

2 files changed

Lines changed: 65 additions & 2 deletions

File tree

crates/warp-core/src/echo_operation.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2414,7 +2414,11 @@ pub(crate) fn prepare_operation_v1(
24142414
if store.node_attachment(&node.local_id).is_some() {
24152415
return obstruction(EchoOperationObstructionKindV1::PreconditionMismatch);
24162416
}
2417-
let Some(write_bytes) = 32_u64.checked_add(replacement_len) else {
2417+
// PR #686 review finding #3 (Codex): this patch writes
2418+
// two 32-byte values, not one -- the UpsertNode's
2419+
// NodeRecord.ty in addition to the attachment atom's
2420+
// type id -- so the write charge must count both.
2421+
let Some(write_bytes) = 64_u64.checked_add(replacement_len) else {
24182422
return obstruction(EchoOperationObstructionKindV1::BudgetExceeded);
24192423
};
24202424
if !budget_meter.charge(1, 0, write_bytes) {

crates/warp-core/tests/executable_operation_pipeline_tests.rs

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1835,7 +1835,7 @@ fn create_from_absence_commits_one_new_node_and_attachment_patch() {
18351835
);
18361836
assert_eq!(
18371837
execution.receipt().consumed_budget(),
1838-
EchoOperationBudgetV1::new(3, 64, 39)
1838+
EchoOperationBudgetV1::new(3, 64, 71)
18391839
);
18401840

18411841
let state = host
@@ -2353,3 +2353,62 @@ fn create_from_absence_invocation_refuses_at_admission_against_an_orphaned_attac
23532353
EchoOperationInvocationAdmissionErrorKindV1::BasisMismatch
23542354
);
23552355
}
2356+
2357+
/// Regression for PR #686 review finding #3 (Codex, P2): the create-from-
2358+
/// absence write charge must account for the `UpsertNode` op's 32-byte
2359+
/// `NodeRecord.ty` in addition to the attachment atom's 32-byte type id and
2360+
/// its replacement bytes -- not `32 + replacement_len` alone, which
2361+
/// undercounts by exactly the node record's width and lets a caller commit
2362+
/// more bytes than it delegated.
2363+
#[test]
2364+
fn create_from_absence_charges_the_node_record_against_the_write_budget() {
2365+
let (mut host, head_key, _existing_node) = fixture_host();
2366+
let installed = install_fixture_operation(&mut host);
2367+
let new_node = NodeKey {
2368+
warp_id: make_warp_id("operation-fixture"),
2369+
local_id: make_node_id("operation-fixture-budget-created"),
2370+
};
2371+
let application_basis =
2372+
warp_core::echo_operation_anchored_node_absent_application_basis_v1(new_node);
2373+
let evaluation_basis = host
2374+
.echo_operation_evaluation_basis_v1(head_key, application_basis)
2375+
.expect("Echo resolves the exact current parent basis");
2376+
let replacement = b"created".to_vec();
2377+
// 32 (attachment type) + replacement.len() = 39: enough under the buggy
2378+
// charge, which never counts the UpsertNode's own 32-byte NodeRecord.ty.
2379+
// 64 (node type + attachment type) + replacement.len() = 71: the correct
2380+
// requirement. This budget sits strictly between the two.
2381+
let insufficient_for_node_record = EchoOperationBudgetV1::new(16, 4_096, 50);
2382+
let invocation = EchoOperationInvocationV1::anchored_node_attachment_compare_and_set(
2383+
installed.package_id(),
2384+
installed.operation_coordinate(),
2385+
evaluation_basis,
2386+
digest("fixture-authority-grant"),
2387+
insufficient_for_node_record,
2388+
new_node,
2389+
None,
2390+
replacement,
2391+
);
2392+
let invocation_bytes = invocation
2393+
.to_canonical_bytes()
2394+
.expect("the create-from-absence invocation is canonical");
2395+
let invocation_policy = EchoOperationInvocationAdmissionPolicyV1::new(
2396+
digest("fixture-authority-profile"),
2397+
digest("fixture-authority-grant"),
2398+
EchoOperationBudgetV1::new(16, 4_096, 4_096),
2399+
);
2400+
let admitted_invocation = host
2401+
.admit_echo_operation_invocation_v1(&invocation_policy, &invocation_bytes)
2402+
.expect("admission's coarser check does not itself meter the write charge");
2403+
let preparation = host.prepare_echo_operation_v1(admitted_invocation);
2404+
let EchoOperationPreparationV1::Obstructed(obstruction) = preparation else {
2405+
panic!(
2406+
"a write budget that covers only the attachment, not the node record too, \
2407+
must not prepare a patch"
2408+
);
2409+
};
2410+
assert_eq!(
2411+
obstruction.kind(),
2412+
EchoOperationObstructionKindV1::BudgetExceeded
2413+
);
2414+
}

0 commit comments

Comments
 (0)