Skip to content

fix(drive-abci)!: validate property constraints on moderator restores - #5329

Merged
QuantumExplorer merged 2 commits into
v5.0-devfrom
shumkov/audit-v5-m1-restore-caps
Oct 8, 2026
Merged

QuantumExplorer merged 2 commits into
v5.0-devfrom
shumkov/audit-v5-m1-restore-caps

Conversation

@shumkov

@shumkov shumkov commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Basic explanation

What this does: A moderator restoring a deleted document must satisfy its type's property rules using current document totals and the document's original owner.

Value: Keeps count and sum limits intact when a document returns, preserving the remaining documents' ability to be edited.

Risks: Consensus behavior changes at PV14: restores that break a cap, lower bound or cross-type prerequisite are refused. PV13 and earlier remain unchanged. Replay/activation for already-deployed PV14 builds requires a release decision.

Issue being fixed or feature implemented

Moderation restore checked uniqueness but skipped propertyConstraints, while Drive reinserted the document and updated aggregates. A restored count or sum could exceed the contract's limit, after which ordinary owner replacements failed with 10422. This addresses M1 in the v5 audit.

What was done?

  • Added a thin restore wrapper around the existing aggregate reader: original document id/owner, no stored live contribution, all restored-type rules.
  • After uniqueness, judge validate_property_constraints with retained system values and post-insertion totals. Invalid block restores use the existing paid nonce-bump path before any document or removal-record write; mempool rejection persists neither fees nor nonce changes.
  • Documented the behavior in the property-constraints book and PV14 entry 87. Rule-free restores retain their exact fees.

Full property-schema validation and deleteConstraints are outside restore. This prevents new violations and performs no historical repair; existing owner deletion is available only where the type's deletion policy, including deleteConstraints, permits it.

In-place changes to shipped generations

Moderation state v0 is selected by the existing dispatcher, but ContractUserModeration is inactive below PV14: raw decoding and validate_is_allowed reject it before transformation. The added validation cannot change PV13 or earlier blocks; raw processing and first-admission controls verify rejection without balance or nonce changes. The batch transformer's new wrapper is additive; existing reader bodies and callers are unchanged, and pre-PV14 types carry no property rules. No shipped version table, error code, fee schedule or limit changes.

How Has This Been Tested?

Deterministic local processor fixtures with signed serialized transitions; no devnet mutation or live reproduction.

  • Test-first signed 9f7b160905: four real failures because invalid restores succeeded (count, sum, same-block second restore, deleted cross-type prerequisite); three valid controls passed. Local diagnostic processing confirmed count 2/sum 120 and subsequent owner replacement refusal 10422.
  • The same seven tests pass with the fix, covering paid fee/nonce effects, unchanged removal/document state, successful post-refusal replacement, original-owner filtering, valid boundary restores and the full rule-free fee baseline.
  • Neighbor moderation module: 141 passed, including PV13, uniqueness, TTL, removals and field changes. Neighbor propertyConstraints module: 38 passed, including aggregate create/replace behavior.
  • Scoped rustfmt and cargo clippy -p drive-abci --all-features --all-targets --jobs 2 -- -D warnings passed.
cargo test -p drive-abci --lib --features mocks restore_constraints --jobs 2
cargo test -p drive-abci --lib --features mocks contract_user_moderation::tests:: --jobs 2
cargo test -p drive-abci --lib --features mocks batch::tests::document::property_constraints --jobs 2

Breaking Changes

PV14 moderator restores now apply every restored-type property constraint. A violating block restore is paid-refused with 10422; its document and removal record remain unchanged. Earlier protocol versions cannot execute this transition. Activation/replay on networks already running PV14 remains a separate release gate.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed
  • If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

PR Hygiene · ec773b6

  • Bots — coderabbitai ✓ · thepastaclaw ✓
  • Self-review — post /self-reviewed
  • Build green
  • Approvals — you own every area touched; none needed

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

Summary by CodeRabbit

  • Bug Fixes
    • Moderator restores now check the restored document against its property constraints, including owner limits, aggregate totals, and cross-type requirements. Checks use the document’s original owner and values.
    • If a restore violates a constraint during block processing, the document remains deleted and its removal record remains unchanged; the moderator is charged and their nonce is consumed. A mempool refusal does not persist fees or a nonce change.
    • Uniqueness is checked before property constraints. Restores do not perform full property-schema validation or evaluate delete constraints.

Raw processor regressions fail on unfixed 683dfc5: count cap, sum budget, sequential restore and cross-type prerequisite restores succeed where paid 10422 is expected. Valid controls pass and the complete rule-free fee baseline is pinned before production edits. Local processing also confirmed stored totals 2 and 120, followed by owner replacement refusal 10422.
Read post-insertion totals for the retained document with its original id and owner, then judge all restored-type property constraints after uniqueness. Refuse with the existing paid nonce-bump path before constructing document or removal-record writes. Rule-free metering remains unchanged; full schema validation and deleteConstraints remain outside restore.

Test would have caught this in CI: ✖ before fix, ✔ after. On signed test-only 9f7b160, four real processor regressions failed because invalid restores succeeded; all seven pass with the fix. Neighbor moderation 141/141 and property constraints 38/38 pass, including prior-version inactivity and no-rule fee baseline. Scoped formatting and all-features/all-targets clippy passed.

In-place state v0 change is inert below protocol version 14: ContractUserModeration is inactive there before decoding/validation. Existing aggregate reader bodies and callers are unchanged.
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: dashpay/platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6548856e-7da7-4874-89df-67df3a45b33a
📥 Commits

Reviewing files that changed from the base of the PR and between 575b7a3 and ec773b6.

📒 Files selected for processing (7)
  • book/src/contract-keywords/property-constraints.md
  • packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/mod.rs
  • packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v0/property_constraint_aggregates.rs
  • packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/contract_user_moderation/state/v0/mod.rs
  • packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/contract_user_moderation/tests.rs
  • packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/contract_user_moderation/tests/restore_constraints.rs
  • packages/rs-platform-version/src/version/v14.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Moderator document restores now validate propertyConstraints against aggregates that include the restored document. Tests and documentation cover constraint outcomes, refusal effects, and protocol-version behavior.

Changes

Moderator Restore Constraints

Layer / File(s) Summary
Aggregate reads and restore validation
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/..., packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/contract_user_moderation/state/v0/mod.rs, book/src/contract-keywords/property-constraints.md, packages/rs-platform-version/src/version/v14.rs
A restore-specific aggregate reader treats the retained document as an insertion. The restore transition validates property constraints before recording restoration. The documentation describes aggregate handling, refusal outcomes, and validation exclusions.
Restore constraint and protocol tests
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/contract_user_moderation/tests.rs, packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/contract_user_moderation/tests/restore_constraints.rs
Tests cover aggregate limits, exact-limit and unconstrained restores, cross-type prerequisites, multiple restores in one block, refusal effects, and protocol-version inactivity.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ModeratorRestore
  participant RestoreAggregateReader
  participant PropertyConstraintValidation
  participant RemovalRecord
  ModeratorRestore->>RestoreAggregateReader: Read totals as if the document were inserted
  RestoreAggregateReader-->>ModeratorRestore: Return aggregate totals
  ModeratorRestore->>PropertyConstraintValidation: Validate restored document properties
  PropertyConstraintValidation-->>ModeratorRestore: Return validation result
  ModeratorRestore->>RemovalRecord: Record restoration when validation succeeds
Loading

Suggested reviewers: quantumexplorer

Merge Risk: ⚪ Minimal · up to ec773

No actionable merge-blocking issue was identified in the restore-constraint change. It is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 6 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely identifies the main change: validating property constraints during moderator restores.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 6 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added this to the v5.0.0 milestone Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

📖 Book Preview built successfully.

Download the preview from the workflow artifacts.
To view locally: download the artifact, unzip, and open index.html.

Updated at 2026-10-08T11:21:30.022Z

@shumkov
shumkov marked this pull request as ready for review October 8, 2026 11:21
@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added the waiting-bots Waiting for the review bots to report on this head label Oct 8, 2026
@thepastaclaw

thepastaclaw commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Final review complete — no blockers (commit ec773b6) · triage: normal

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Final validation — Phase 1 + Phase 2

Static verification of the complete PR range at ec773b6 corroborates the supplied reviewers’ clean assessments: restores use retained owner/system values and billed post-insertion aggregates, with constraint failures returning the paid nonce-bump action before restoration writes. No in-scope findings remain; earlier protocols cannot execute moderation, and activation/replay compatibility for existing PV14 deployments is explicitly documented as a release decision. No local builds or tests were run; the supplied CI snapshot reports successful Rust workspace tests and other completed checks, with PR Hygiene still pending.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: normal by gpt-6.1-sol (effort low) — The diff changes consensus validation in transform_document_restore_v0, but the production change is a small, contained wrapper and validation call reusing existing aggregate and rejection paths, rather than a large or intricate implementation.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 8, 2026
@QuantumExplorer

Copy link
Copy Markdown
Member

I'm closing #5335, which duplicated this fix. The approach is the same (every rule judged as a create, original owner and times, totals as after the insertion, 10422 paid), and the tests here are more complete. A few things from #5335 that may be worth carrying over:

  1. book/src/data-model/contract-moderation.md, "Restoring Documents": the paragraph that lists every check a restore makes doesn't mention the rules yet, and it ends with "so the owner, the revision, the timestamps, the references and the schema are not checked again." fix(platform)!: a moderator's restore must meet the type's countOf and sumOf rules (PV14) #5335 added, before the unique-index item: "the document meets every rule of the type's propertyConstraints as a create would, its countOf and sumOf totals read as they will be once it is back (DocumentPropertyConstraintViolatedError, 10422: the document re-enters every count and sum its deletion took it out of, and other documents may have filled a cap or a budget meanwhile)". It also changed "The record read is billed" to "The record read and the totals read are billed".
  2. PV14 note 21 lists the restore's refusals (41119, 41120, 41121, 41122, 40105). A pointer to 10422 and note 87 would keep that list complete.
  3. property-constraints.md, "Judged when the rule's own type is written": this says a rule on its own type "holds for good, since every write that could raise the total is judged". After this fix that's true again, and adding ", a moderator's restore included" there makes it explicit.
  4. Check order (optional): here the rules run after the unique-index check. A create judges propertyConstraints in advanced structure, before its uniqueness check in state validation, and transform_document_fields_change_v0 in this same file judges the rules before validate_moderated_document_uniqueness. Judging the restore's rules first would make all three agree. The only visible effect is which code a restore that breaks both gets (10422 rather than 40105).
  5. One copy instead of two (minor): since refactor(platform): share the delete state checks and cover what delete rules leave alone #5330 merged, read_property_constraint_aggregates builds its own Value from the properties, and the restore builds another for validate_property_constraints. fix(platform)!: a moderator's restore must meet the type's countOf and sumOf rules (PV14) #5335 had the restore helper take the &Value the rules are judged on, the way refactor(platform): share the delete state checks and cover what delete rules leave alone #5330 reshaped read_delete_constraint_aggregates, so the properties are copied once.

The diff is at https://github.com/dashpay/platform/pull/5335/files if any of it is useful.

@QuantumExplorer
QuantumExplorer merged commit 96924e2 into v5.0-dev Oct 8, 2026
59 of 60 checks passed
@QuantumExplorer
QuantumExplorer deleted the shumkov/audit-v5-m1-restore-caps branch October 8, 2026 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-self-review Waiting for the author to post /self-reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants