Repository navigation
perf(drive): delete a consumed document from what its lookup read - #5360
QuantumExplorer wants to merge 1 commit into
Conversation
A create that consumes a commitment (a `refersTo` lookup with a computed key declaring `consume`, protocol version 14) read the commitment twice: the lookup in reference validation found it, and the delete read it again by id to know what to remove and refund. The second read was billed. The lookup now keeps the stored element's storage flags beside the document (`query_documents_with_flags`, the same path query and cost), `ConsumedDocument` carries both, and the delete runs from them through `Drive::force_delete_read_document_for_contract_operations`: the operations of `force_delete_document_for_contract_operations` less its read, dispatched on the same method version. A contested create's consume uses the new `ForceDeleteReadDocument` operation, the plain create's `AddDocumentAndDeleteConsumed`. A DPNS v3 domain create's processing fee drops from 2,525,540 to 2,514,960 credits; storage fee and the refund to the writer are unchanged. A Drive test checks the read-document delete's operations equal the force delete's after its read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit cec6d0a) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head cec6d0a and the PR's base-to-head diff. No blocking defects were confirmed; the overlapping cloning findings combine into one performance suggestion, and the new deletion guards lack direct negative-path coverage. This was static verification only: no builds or tests were run, and the supplied CI snapshot shows Kotlin checks queued without completed Rust validation.
🟡 2 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Move consumed document payloads instead of deep-cloning them
packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_create_transition.rs:58
ConsumedDocument now owns a full Document and its storage flags, so this to_vec() recursively clones each document's property map and values. The reference-validation block also clones the owned lookup result into ConsumedDocument before dropping the original. Both copies are avoidable: this converter owns mut self, and the subsequent owned document conversion discards the original consumed_documents field. The PR therefore introduces copying proportional to the consumed document bodies on every consuming create, including contracts with larger commitments than DPNS preorders. After completing the borrowed reference checks, move the lookup tuple into ConsumedDocument; add a take_consumed_documents accessor using std::mem::take and use it here, matching the existing take_* accessors.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The cross-layer change to lookup state, create conversion, and batch deletion is intricate and directly changes consensus-visible fee accounting and storage-refund inputs through force_delete_read_document_for_contract_operations_v0 and DocumentOperationType conversion. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_create_transition.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_create_transition.rs:58: Move consumed document payloads instead of deep-cloning them
ConsumedDocument now owns a full Document and its storage flags, so this to_vec() recursively clones each document's property map and values. The reference-validation block also clones the owned lookup result into ConsumedDocument before dropping the original. Both copies are avoidable: this converter owns mut self, and the subsequent owned document conversion discards the original consumed_documents field. The PR therefore introduces copying proportional to the consumed document bodies on every consuming create, including contracts with larger commitments than DPNS preorders. After completing the borrowed reference checks, move the lookup tuple into ConsumedDocument; add a take_consumed_documents accessor using std::mem::take and use it here, matching the existing take_* accessors.
In `packages/rs-drive/src/drive/document/delete/delete_document_for_contract_operations/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/drive/document/delete/delete_document_for_contract_operations/v0/mod.rs:222-236: Add negative-path tests for the new read-document deletion guards
The new helper independently duplicates the indexOnly and history-keeping guards from the existing force-delete path. The added operation-equivalence test uses a normal document type, so it does not verify either rejection; the existing history-deletion test exercises the old deletion path rather than this helper. Contract registration currently excludes these types from consumption, so this is a defensive API coverage gap, not a demonstrated execution defect. Add direct assert_matches! cases for the new entry point with an indexOnly type and a history-keeping type, asserting CorruptedCodeExecution and InvalidDeletionOfDocumentThatKeepsHistory respectively, to pin the guard parity promised by this PR.
| if document_type.index_only() { | ||
| return Err(Error::Drive(DriveError::CorruptedCodeExecution( | ||
| "indexOnly documents cannot be deleted by id: there is no primary-storage \ | ||
| row; use delete_index_only_document_for_contract_operations with the \ | ||
| document's values", | ||
| ))); | ||
| } | ||
|
|
||
| if document_type.documents_keep_history() { | ||
| return Err(Error::Drive( | ||
| DriveError::InvalidDeletionOfDocumentThatKeepsHistory( | ||
| "this document type keeps history and therefore can not be deleted", | ||
| ), | ||
| )); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Add negative-path tests for the new read-document deletion guards
The new helper independently duplicates the indexOnly and history-keeping guards from the existing force-delete path. The added operation-equivalence test uses a normal document type, so it does not verify either rejection; the existing history-deletion test exercises the old deletion path rather than this helper. Contract registration currently excludes these types from consumption, so this is a defensive API coverage gap, not a demonstrated execution defect. Add direct assert_matches! cases for the new entry point with an indexOnly type and a history-keeping type, asserting CorruptedCodeExecution and InvalidDeletionOfDocumentThatKeepsHistory respectively, to pin the guard parity promised by this PR.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
This branch has not been deployed
2 participants
Basic explanation
Registering a DPNS name from protocol version 14 deletes the name's preorder. The platform was reading that preorder from storage twice: once to check it is yours and old enough, and once more to delete it. The second read was billed to the user. Now the delete uses what the first read already found, so a name costs a little less to register (10,580 credits less in processing fees). Nothing else changes: the same preorder is deleted, and you get the same storage refund.
This applies to any contract whose
refersTolookup declaresconsume, not only DPNS. Stacked on #5337, which adds DPNS v3.Issue being fixed or feature implemented
A create that consumes a commitment (a
refersTolookup with a computedfindBykey andconsume: true, protocol version 14) read the commitment twice:force_delete_document_for_contract_operations, which reads the document again by id to know which index entries to remove and whom to refund (billed again).What was done?
fetch_document_through_lookupruns the same index query throughquery_documents_with_flagsand returns the document with its stored element's storage flags. It is the same path query at the same cost: no other fee pin moved.ConsumedDocumentcarries the document and storage flags the lookup read, instead of only its id.Drive::force_delete_read_document_for_contract_operations: the operations offorce_delete_document_for_contract_operationsless its read, with the same guards (refusesindexOnlyand history-keeping types). It dispatches on the samedelete_document_for_contract_operationsmethod version, so there is no new version field.AddDocumentAndDeleteConsumedtakes theConsumedDocuments and deletes each with the new method. A contested create's consume uses the newForceDeleteReadDocumentoperation instead ofForceDeleteDocument, which keeps serving moderator deletes and ttl expiry.Before and after
A DPNS v3 domain create (
should_charge_a_domain_create_at_protocol_versions_13_and_14):Protocol version 13 is unchanged (1,876,280 / 42,660,000 / 0).
In-place changes to shipped generations
fetch_document_through_lookup) and create action conversion 0: every protocol version selects these. The lookup query and the consumed-document delete are reached only for arefersTowith a computed key, which only meta-schema v3 (protocol version 14) admits. Before 14 no create carries a consumed document and no lookup runs, so nothing changes there.query_documents_with_flagsruns the same path query asquery_documentsfor a non-indexOnlytype. A consumed type is neverindexOnly(registration refuses it), and no other fee pin moved.How Has This Been Tested?
should_delete_a_read_document_as_a_force_delete_does_less_its_read: for a stored document, the new method's operations equal the force delete's after its single read, the storage removal and refund included.should_charge_a_domain_create_at_protocol_versions_13_and_14: the protocol version 14 processing fee pin moves from 2,525,540 to 2,514,960; storage and refund unchanged.cargo test -p drive --lib -- consume document::delete drive_op_batch: 88 passed.cargo test -p drive-abci --lib -- dpns creation_tests masternode_vote check_tx::v0 protocol_upgrade data_triggers commit_reveal owner_balance consumed: 344 passed, and the DPNS fee pin failed as expected before its update; after it,cargo test -p drive-abci --lib -- dpns creation_tests commit_reveal consumed owner_balance: 166 passed.cargo test -p drive-abci --test strategy_tests -- test_cases::voting_tests:: test_cases::upgrade_fork_tests::: 13 passed, 4 ignored.cargo clippy -p drive -p drive-abci --all-targetsandcargo fmt --all -- --check: clean.Breaking Changes
None for clients. The processing fee of a create that consumes a commitment drops by one document read at protocol version 14, which is unreleased.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code