fix(platform): reject keep-history document deletes cleanly#3930
fix(platform): reject keep-history document deletes cleanly#3930thepastaclaw wants to merge 6 commits into
Conversation
…schema documentsKeepHistory: true together with canBeDeleted: true is self-contradictory — rs-drive's force-delete path unconditionally refuses to delete documents whose type keeps history (InvalidDeletionOfDocumentThatKeepsHistory). Catch the combination at contract creation time in the V2 document-type schema parser, mirroring the existing cross-flag rule for ContestedUniqueIndexOnMutableDocumentTypeError, so SDK users get a clean InvalidContractStructure error instead of a runtime delete failure. Gated by full_validation so already-deployed contradictory contracts continue to load through the V2 parser at v12+ (the rs-drive-abci delete-transition guard surfaces them as clean invalid-paid transitions in a follow-up commit). Refs dashpay#3927 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without this guard, a delete state transition against a document type whose schema sets documentsKeepHistory: true passes the structure validator (which only checks documents_can_be_deleted()) and reaches rs-drive's force-delete path, where it triggers InvalidDeletionOfDocumentThatKeepsHistory. The processor reclassifies that drive-layer error as ExecutionResult::InternalError — the transition is neither valid nor invalid-paid, leaving the SDK with no clean accept/reject signal. Add a documents_keep_history() check beside the existing documents_can_be_deleted() check in the delete-transition advanced structure validator so the contradiction surfaces as a normal invalid (paid) consensus error. This fixes the classification even for already-deployed contradictory contracts (e.g. testnet 5CBPiadGmx3Zsjc26g5onopcx7pdxHPbrRAUD2T2yAbC), which the DPP cross-flag rule from the previous commit cannot retroactively fix. The new note-contract-keep-history-and-can-be-deleted.json fixture is loaded with full_validation=false so it bypasses the DPP guard and exercises this layer directly. Refs dashpay#3927 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit added the documents_keep_history() check directly to advanced_structure_v0, which is the dispatched validator for ALL historical protocol versions (1-11) as well as v12. Mutating v0 would alter the consensus result of any historical block containing a delete against a keep-history doctype — pre-fix those returned ExecutionResult::InternalError (no fees, no state change); post-fix they would be classified as invalid-paid (fees deducted, state delta differs), breaking bit-for-bit chain replay at PROTOCOL_VERSION_11 and earlier. Move the check into a new advanced_structure_v1 module, restore v0 to its original behavior, extend the trait dispatcher to handle both versions, and bump document_delete_transition_structure_validation from 0 to 1 in DRIVE_ABCI_VALIDATION_VERSIONS_V8 (used by PROTOCOL_VERSION_12, the v3.1 hard fork). V1-V7 still dispatch v0 so pre-v12 chain history stays reproducible. Refs dashpay#3927 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wrap the v2 DocumentType keep-history + canBeDeleted contradiction through `consensus_or_protocol_data_contract_error` so that with the `validation` feature it returns `ProtocolError::ConsensusError` instead of bare `ProtocolError::DataContractError`. drive-abci's `transform_into_action_v0` only converts the consensus variant into a clean invalid (paid) transition with a bump action — the DataContractError variant propagated as an internal execution error in validator mode, defeating the parse-time guard's whole purpose for contract create/update. Pin the variant in the existing regression test so the classification can't silently regress. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 50 minutes and 5 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughEnforces that ChangesFix documentsKeepHistory + canBeDeleted contradiction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Review complete (commit 078c233) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs (1)
986-1019: 💤 Low valueConsider adding a test for
full_validation: falseto verify the gating.The validation logic is correctly gated by
full_validation, but there's no test verifying that contradictory contracts are accepted whenfull_validation: false(the scenario for re-parsing already-deployed contracts). A test would confirm the gate works as documented in lines 317-321.📝 Suggested test addition
/// `full_validation: false` must allow the contradictory combination so /// already-deployed contracts (e.g. testnet `5CBPiadGmx3Zsjc26g5onopcx7pdxHPbrRAUD2T2yAbC`) /// continue to load when re-parsed at v12+. #[test] fn doctype_keep_history_with_can_be_deleted_accepted_without_full_validation() { let schema = platform_value!({ "type": "object", "properties": { "label": { "type": "string", "maxLength": 50, "position": 0, }, }, "additionalProperties": false, "documentsKeepHistory": true, "canBeDeleted": true, }); let platform_version = PlatformVersion::latest(); let config = DataContractConfig::default_for_version(platform_version) .expect("default config available on latest platform version"); let result = DocumentTypeV2::try_from_schema( Identifier::new([1; 32]), 1, config.version(), "test_doc", schema, None, &BTreeMap::new(), &config, false, // full_validation = false &mut vec![], platform_version, ); assert!( result.is_ok(), "contradictory flags must be accepted when full_validation=false \ to allow already-deployed contracts to load; got {:?}", result.unwrap_err() ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs` around lines 986 - 1019, Add a new test function after the existing doctype_keep_history_with_can_be_deleted_rejected test to verify that when full_validation is set to false, the contradictory combination of documentsKeepHistory: true and canBeDeleted: true is accepted. The test should use the same schema structure as the existing rejection test, call DocumentTypeV2::try_from_schema with full_validation parameter set to false, and assert that the result is Ok instead of Err. This confirms the validation gating mechanism works correctly for re-parsing already-deployed contracts as documented.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rs (1)
381-390: ⚡ Quick winPin this regression to protocol version 12 to keep the gate test deterministic.
Using
current_platform_version()makes this test float withlatest, so it can stop validating the exact v12 gate behavior over time. Prefer explicitly setting protocol version12here (and optionally add a paired v11 assertion for legacy classification).Suggested change
- let mut platform = TestPlatformBuilder::new() - .build_with_mock_rpc() - .set_initial_state_structure(); + let protocol_version = 12; + let mut platform = TestPlatformBuilder::new() + .with_initial_protocol_version(protocol_version) + .build_with_mock_rpc() + .set_initial_state_structure(); let contract_path = "tests/supporting_files/contract/note/note-contract-keep-history-and-can-be-deleted.json"; - let platform_state = platform.state.load(); - let platform_version = platform_state - .current_platform_version() - .expect("expected to get current platform version"); + let platform_state = platform.state.load(); + let platform_version = PlatformVersion::get(protocol_version) + .expect("expected platform version for protocol version 12");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rs` around lines 381 - 390, The test is currently using current_platform_version() to retrieve the platform version, which will always use the latest protocol version and cause the test to float over time, making it unable to reliably validate v12 gate behavior. Replace the call to current_platform_version() with an explicit protocol version 12 assignment, ensuring the test pins to protocol version 12 to maintain deterministic gate validation. Optionally, add a paired assertion for legacy v11 classification to provide additional coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs`:
- Around line 986-1019: Add a new test function after the existing
doctype_keep_history_with_can_be_deleted_rejected test to verify that when
full_validation is set to false, the contradictory combination of
documentsKeepHistory: true and canBeDeleted: true is accepted. The test should
use the same schema structure as the existing rejection test, call
DocumentTypeV2::try_from_schema with full_validation parameter set to false, and
assert that the result is Ok instead of Err. This confirms the validation gating
mechanism works correctly for re-parsing already-deployed contracts as
documented.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rs`:
- Around line 381-390: The test is currently using current_platform_version() to
retrieve the platform version, which will always use the latest protocol version
and cause the test to float over time, making it unable to reliably validate v12
gate behavior. Replace the call to current_platform_version() with an explicit
protocol version 12 assignment, ensuring the test pins to protocol version 12 to
maintain deterministic gate validation. Optionally, add a paired assertion for
legacy v11 classification to provide additional coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25cb4c10-52f5-41b2-a9ed-9d76c405cfe4
📒 Files selected for processing (6)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/document/document_delete_transition_action/advanced_structure_v1/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/document/document_delete_transition_action/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rspackages/rs-drive-abci/tests/supporting_files/contract/note/note-contract-keep-history-and-can-be-deleted.jsonpackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v8.rs
…tests - Add DPP test asserting documentsKeepHistory + canBeDeleted is accepted when try_from_schema runs with full_validation=false (the restore / migration / cache-warmup path), pinning the gating behavior that lets already-deployed contradictory contracts continue to load at v12+. - Pin the drive-abci keep-history delete regression test to protocol version 12 via with_initial_protocol_version + PlatformVersion::get instead of floating on current_platform_version(), so the assertion stays bit-for-bit reproducible as new protocol versions ship. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Addressed both CodeRabbit nitpicks in 078c233:
Focused validation after the commit:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Targeted fix is correct: DPP parser cross-flag guard is gated by full_validation and uses consensus_or_protocol_data_contract_error to surface as ConsensusError in validator builds; drive-abci v1 validator is wired only at PROTOCOL_VERSION_12 so v11 replay is preserved; tests cover the rejected/accepted combinations. Three minor follow-ups: a misleading docstring about the canBeDeleted config default, a missing regression test for the omitted-default boundary, and a missing pre-v12 paired test for the new delete guard. No blockers.
🟡 2 suggestion(s) | 💬 1 nitpick(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-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs:329-339: Add regression test for omitted `canBeDeleted` (default=true) with `documentsKeepHistory: true`
The guard fires on resolved `DocumentTypeV1` fields, and `documents_can_be_deleted_contract_default()` is `true`, so a schema with `documentsKeepHistory: true` and `canBeDeleted` omitted is now rejected — the most likely real-world misuse. Every other branch is pinned (true+true rejected, true+false accepted, false+true accepted), but the omitted-defaults case is not. A future refactor that inspects raw schema keys instead of resolved values could pass the current tests while silently reopening the contradiction. Add a `doctype_keep_history_with_can_be_deleted_omitted_rejected` test that omits `canBeDeleted` to pin the resolved-default behavior.
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:379-390: Pin pre-v12 keep-history delete classification with a paired test
The new regression runs only against the current platform version, but this PR's correctness depends on `document_delete_transition_structure_validation` activating at v12 while v11 preserves the v0 path for chain replay. Adjacent version-gated deletion behavior in this file already has paired v11/latest helpers; adding the same v11 case for the contradictory keep-history fixture would catch an accidental change to the v11 version table or to the v0 validator that would otherwise alter consensus replay silently.
| /// Guard against an over-broad fix: `documentsKeepHistory: true` + | ||
| /// `canBeDeleted: false` is consistent (the doctype is append-only) | ||
| /// and must continue to parse cleanly. Same for `documentsKeepHistory: | ||
| /// true` with `canBeDeleted` omitted — covered by the existing | ||
| /// `doctype_keep_history_without_summable_accepted` test, which | ||
| /// leaves `canBeDeleted` at its config default (false). |
There was a problem hiding this comment.
💬 Nitpick: Docstring misstates canBeDeleted default and sibling test coverage
The comment claims the sibling doctype_keep_history_without_summable_accepted test leaves "canBeDeleted" at its config default (false). Two errors: (1) DEFAULT_CONTRACT_DOCUMENTS_CAN_BE_DELETED is true (packages/rs-dpp/src/data_contract/config/fields.rs:6), not false; (2) the sibling test (line 803-828) explicitly sets "canBeDeleted": false (line 817) rather than omitting it. The docstring implies the omitted-default path is covered and benign, but in fact under the new guard documentsKeepHistory: true with canBeDeleted omitted resolves to the default true and is now rejected at v12+. Correct the comment so future readers don't assume the omitted-default case has coverage.
source: ['claude']
| if full_validation && v1.documents_keep_history && v1.documents_can_be_deleted { | ||
| return Err(consensus_or_protocol_data_contract_error( | ||
| DataContractError::InvalidContractStructure(format!( | ||
| "document type \"{}\" sets both `documentsKeepHistory: true` and \ | ||
| `canBeDeleted: true`, but the storage layer unconditionally refuses to \ | ||
| delete a document whose type keeps history. Set one of the two flags to \ | ||
| false (or omit it).", | ||
| name, | ||
| )), | ||
| )); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Add regression test for omitted canBeDeleted (default=true) with documentsKeepHistory: true
The guard fires on resolved DocumentTypeV1 fields, and documents_can_be_deleted_contract_default() is true, so a schema with documentsKeepHistory: true and canBeDeleted omitted is now rejected — the most likely real-world misuse. Every other branch is pinned (true+true rejected, true+false accepted, false+true accepted), but the omitted-defaults case is not. A future refactor that inspects raw schema keys instead of resolved values could pass the current tests while silently reopening the contradiction. Add a doctype_keep_history_with_can_be_deleted_omitted_rejected test that omits canBeDeleted to pin the resolved-default behavior.
source: ['codex', 'claude']
| #[tokio::test] | ||
| async fn test_document_delete_on_document_type_that_keeps_history_is_rejected() { | ||
| let mut platform = TestPlatformBuilder::new() | ||
| .build_with_mock_rpc() | ||
| .set_initial_state_structure(); | ||
|
|
||
| let contract_path = "tests/supporting_files/contract/note/note-contract-keep-history-and-can-be-deleted.json"; | ||
|
|
||
| let platform_state = platform.state.load(); | ||
| let platform_version = platform_state | ||
| .current_platform_version() | ||
| .expect("expected to get current platform version"); |
There was a problem hiding this comment.
🟡 Suggestion: Pin pre-v12 keep-history delete classification with a paired test
The new regression runs only against the current platform version, but this PR's correctness depends on document_delete_transition_structure_validation activating at v12 while v11 preserves the v0 path for chain replay. Adjacent version-gated deletion behavior in this file already has paired v11/latest helpers; adding the same v11 case for the contradictory keep-history fixture would catch an accidental change to the v11 version table or to the v0 validator that would otherwise alter consensus replay silently.
source: ['codex']
There was a problem hiding this comment.
Resolved in 078c233 — Pin pre-v12 keep-history delete classification with a paired test no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Correction: this was auto-resolved incorrectly by the review dedupe/reconciliation step. The latest push does pin the regression test to protocol version 12, but the paired pre-v12/v11 classification coverage is still missing, so this finding remains valid at 078c233. Reopening/keeping this thread active.
|
CI note: Failure:
Why unrelated:
Local focused checks for this PR still pass:
|
1 participant
fix(platform): reject keep-history document deletes cleanly
Issue being fixed or feature implemented
Fixes #3927.
Document types could set both
documentsKeepHistory: trueandcanBeDeleted: true. That combination advertises deletability even thoughDrive refuses to delete keep-history documents, so delete attempts surfaced as
InternalErrorinstead of a clear consensus rejection.What was done?
during DPP schema parsing.
while preserving protocol/data-contract error behavior for non-validation
builds.
already-deployed contradictory contracts fail as invalid-paid transitions
instead of internal errors.
replay behavior is preserved.
keep-history/deletable fixture for the existing-contract path.
How Has This Been Tested?
Local worktree on macOS arm64, branch
fix/document-history-delete-error.cargo test -p dpp --features all_features data_contract::document_type::class_methods::try_from_schema::v2Broader DPP
try_from_schemasweep across v0/v1/v2cargo test -p dpp --features all_featurescargo test -p drive-abci --features mocks test_document_delete_on_document_type_that_keeps_history_is_rejectedcargo test -p drive-abci --features mocks data_contract_create::state::v0::testscargo test -p drive-abci --features mocksfocused batchstate-transition/deletion coverage
DPP no-default-features parser regression:
cargo test -p dpp \ doctype_keep_history_with_can_be_deleted_rejected \ --no-default-featuresDPP all-features parser regression:
cargo test -p dpp --features all_features \ doctype_keep_history_with_can_be_deleted_rejectedPre-commit hook:
cargo fmt --all -- --checkcargo checkworkspacePre-PR code-review gate was run after fixes and returned
ship.Breaking Changes
None.
Checklist
corresponding section if my code contains any
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes