fix(platform): reject contradictory keep-history document deletes - #4218
Conversation
Document types could set both documentsKeepHistory: true and canBeDeleted: true, so delete attempts surfaced as InternalError instead of a clean consensus rejection. - Reject the combination during DPP schema parsing via a new try_from_schema v3, active from protocol version 13 (CONTRACT_VERSIONS_V5). v2 stays untouched: protocol version 12 is released and consensus-frozen. - Add a drive-abci delete-transition structure-validation v1 that rejects deletes on keep-history document types as invalid (paid) transitions, gated at protocol version 13 via DRIVE_ABCI_VALIDATION_VERSIONS_V9. - Regression coverage on both sides of the v12/v13 boundary, including a contradictory fixture for the already-deployed-contract path. Fixes #3927 Co-Authored-By: PastaClaw <thepastaclaw@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 25 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (15)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughProtocol 14 adds version 4 document schema validation and version 1 delete-transition validation. It rejects contradictory keep-history and deletion flags, supports repair of legacy contracts, classifies keep-history deletes as invalid paid transitions, and preserves protocol 12–13 behavior. ChangesKeep-history deletion validation
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change is mergeable with no identified blocking risk. Sequence Diagram(s)sequenceDiagram
participant DeleteTransition
participant DocumentDeleteTransitionActionStructureValidationV1
participant DataContract
DeleteTransition->>DocumentDeleteTransitionActionStructureValidationV1: validate_structure_v1
DocumentDeleteTransitionActionStructureValidationV1->>DataContract: look up document type
DocumentDeleteTransitionActionStructureValidationV1-->>DeleteTransition: return invalid paid error for keep-history type
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 78.95% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 14 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
🕓 Queued for automated review — 61st in line, estimated start in ~50 h (commit d4e3dca)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4218 +/- ##
============================================
+ Coverage 85.35% 87.24% +1.89%
============================================
Files 2765 2795 +30
Lines 369457 364790 -4667
============================================
+ Hits 315349 318274 +2925
+ Misses 54108 46516 -7592
🚀 New features to boost your workflow:
|
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This recreation of #3930 against protocol version 13 correctly rejects the contradictory documentsKeepHistory:true + canBeDeleted:true combination in try_from_schema_v3 (gated by full_validation and CONTRACT_VERSIONS_V5/protocol 13) while leaving the consensus-frozen v2 parser (protocol 12) untouched, and adds a matching drive-abci structure-validation guard (v1, gated by DRIVE_ABCI_VALIDATION_VERSIONS_V9/protocol 13) that turns already-deployed contradictory contracts' deletes into invalid-paid consensus errors instead of InternalError. I independently re-verified all four prior findings from PR #3930 against this exact head and confirm all four are FIXED in the recreated implementation. One non-blocking test-coverage gap remains in the paired protocol-12/13 delete regression: it only asserts aggregate result-bucket counts rather than the specific error identity, so an unrelated failure could false-positive the assertion.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— security-auditor (completed),claude-sonnet-5— rust-quality (completed)
🟡 1 suggestion(s)
🤖 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-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rs:538-554: Paired keep-history delete regression checks aggregate counts, not the specific error
run_document_delete_on_document_type_that_keeps_history_at_protocol_version only asserts processing_result.invalid_paid_count() == expect_invalid_paid and a separate InternalError count. Since exactly one transition is submitted, any unrelated invalid-paid consensus error (e.g. a nonce or signature failure) at v13, or any unrelated InternalError at v12, would satisfy these assertions while the actual regression this test is meant to pin — the new keep-history structure guard at v13 and the historical Drive-layer InvalidDeletionOfDocumentThatKeepsHistory failure at v12 — silently stops firing. Asserting the concrete error identity (InvalidDocumentTransitionActionError with the keep-history message at v13, and the keep-history Drive error text inside the InternalError at v12) would make this regression fail loudly if the underlying guard regresses or is bypassed by some other error path.
| assert_eq!( | ||
| processing_result.invalid_paid_count(), | ||
| usize::from(expect_invalid_paid), | ||
| "unexpected invalid-paid classification at protocol version {protocol_version}" | ||
| ); | ||
| assert_eq!(processing_result.invalid_unpaid_count(), 0); | ||
| assert_eq!(processing_result.valid_count(), 0); | ||
| let internal_error_count = processing_result | ||
| .execution_results() | ||
| .iter() | ||
| .filter(|result| matches!(result, StateTransitionExecutionResult::InternalError(_))) | ||
| .count(); | ||
| assert_eq!( | ||
| internal_error_count, | ||
| usize::from(!expect_invalid_paid), | ||
| "unexpected InternalError classification at protocol version {protocol_version}" | ||
| ); |
There was a problem hiding this comment.
🟡 Suggestion: Paired keep-history delete regression checks aggregate counts, not the specific error
run_document_delete_on_document_type_that_keeps_history_at_protocol_version only asserts processing_result.invalid_paid_count() == expect_invalid_paid and a separate InternalError count. Since exactly one transition is submitted, any unrelated invalid-paid consensus error (e.g. a nonce or signature failure) at v13, or any unrelated InternalError at v12, would satisfy these assertions while the actual regression this test is meant to pin — the new keep-history structure guard at v13 and the historical Drive-layer InvalidDeletionOfDocumentThatKeepsHistory failure at v12 — silently stops firing. Asserting the concrete error identity (InvalidDocumentTransitionActionError with the keep-history message at v13, and the keep-history Drive error text inside the InternalError at v12) would make this regression fail loudly if the underlying guard regresses or is bypassed by some other error path.
| assert_eq!( | |
| processing_result.invalid_paid_count(), | |
| usize::from(expect_invalid_paid), | |
| "unexpected invalid-paid classification at protocol version {protocol_version}" | |
| ); | |
| assert_eq!(processing_result.invalid_unpaid_count(), 0); | |
| assert_eq!(processing_result.valid_count(), 0); | |
| let internal_error_count = processing_result | |
| .execution_results() | |
| .iter() | |
| .filter(|result| matches!(result, StateTransitionExecutionResult::InternalError(_))) | |
| .count(); | |
| assert_eq!( | |
| internal_error_count, | |
| usize::from(!expect_invalid_paid), | |
| "unexpected InternalError classification at protocol version {protocol_version}" | |
| ); | |
| assert_eq!( | |
| processing_result.invalid_paid_count(), | |
| usize::from(expect_invalid_paid), | |
| "unexpected invalid-paid classification at protocol version {protocol_version}" | |
| ); | |
| assert_eq!(processing_result.invalid_unpaid_count(), 0); | |
| assert_eq!(processing_result.valid_count(), 0); | |
| match (expect_invalid_paid, processing_result.execution_results().as_slice()) { | |
| (true, [result]) => { | |
| let message = format!("{result:?}"); | |
| assert!( | |
| message.contains("keep history and therefore can not be deleted"), | |
| "expected the keep-history InvalidDocumentTransitionActionError at v13, got: {message}" | |
| ); | |
| } | |
| (false, [StateTransitionExecutionResult::InternalError(message)]) => assert!( | |
| message.contains("invalid deletion of document that keeps history") | |
| || message.to_lowercase().contains("keepshistory"), | |
| "expected the historical keep-history Drive error at v12, got: {message}" | |
| ), | |
| (_, results) => panic!( | |
| "unexpected execution result at protocol version {protocol_version}: {results:?}" | |
| ), | |
| } |
source: ['codex']
…y repairs Activate the schema rejection and delete guard only at protocol 14, leaving protocol 12 and 13 validation maps unchanged. Wrap the current ranked-schema parser and retain all existing delete structure checks. Permit owners to disable the unusable delete flag on legacy keep-history document types while preserving history, other immutable configuration, and schema compatibility. Cover repair and a subsequent signed update against persisted protocol 13 data. Merge the current v4.2-dev target to use its protocol 14 method maps.
cargo fmt --check fails on v4.2-dev since #4218 landed this test unformatted. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
Fixes #3927. A document type could advertise deletion while keeping history, even though Drive cannot delete history-bearing documents. Deletes then surfaced as internal errors. Reject the contradictory combination at protocol 14 while preserving released protocol behavior and providing a repair path for existing contracts.
What was done?
try_from_schemav3 parser and select delete structure validation v1.canBeDeleted: truetofalsewhen both old and new document types keep history. All other configuration, index, and schema compatibility checks remain enforced. The correction does not rewrite stored documents.How Has This Been Tested?
Validation performed locally on macOS arm64. Both new failure regressions were observed before implementing the fixes.
cargo test --offline -p platform-version --test keep_history_delete_versions— 2 passed; the assertions failed before moving the gates off protocol 13.cargo test --offline -p dpp --features all_features --lib— 4,141 passed, 6 ignored. Includes 13 focused parser/repair tests covering explicit and omitted defaults, protocol 12/13 compatibility, forbidden flag changes, and schema/config checks during repair. The repair regression failed before implementing the correction path.cargo test --offline -p drive-abci --features mocks --lib batch::tests::document— 164 passed. Covers v12/v13 internal-error replay, the exact v14 paid rejection, indexOnly deletion, and a populated v13 contract repaired and updated through signed transitions after selecting v14.cargo test --offline -p drive-abci --features mocks --lib data_contract— 145 passed, 2 ignored.cargo check --offline -p dppandcargo clippy --offline -p dpp --features all_features— passed. The default build reports three existing unused-import warnings.Breaking Changes
No change to consensus behavior for released protocols through 13. At protocol 14, new contradictory document types are rejected and legacy delete attempts become paid consensus errors. To update an existing contradictory contract, explicitly set
canBeDeleted: falseon each keep-history document type in the update. History and the other immutable configuration flags remain unchanged.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Compatibility