Repository navigation
fix(drive-abci)!: validate property constraints on moderator restores - #5329
Conversation
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughModerator document restores now validate ChangesModerator Restore Constraints
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
Suggested reviewers: Merge Risk: ⚪ Minimal · up to 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)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation 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.)
✨ 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 |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-10-08T11:21:30.022Z |
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
|
✅ Final review complete — no blockers (commit ec773b6) · triage: normal |
thepastaclaw
left a comment
There was a problem hiding this comment.
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:
normalbygpt-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); 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(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; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
|
Bots are done — your move: post |
|
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:
The diff is at https://github.com/dashpay/platform/pull/5335/files if any of it is useful. |
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?
validate_property_constraintswith 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.Full property-schema validation and
deleteConstraintsare outside restore. This prevents new violations and performs no historical repair; existing owner deletion is available only where the type's deletion policy, includingdeleteConstraints, permits it.In-place changes to shipped generations
Moderation state
v0is selected by the existing dispatcher, butContractUserModerationis inactive below PV14: raw decoding andvalidate_is_allowedreject 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.
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.cargo clippy -p drive-abci --all-features --all-targets --jobs 2 -- -D warningspassed.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:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
PR Hygiene ·
ec773b6/self-reviewedWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit