feat(platform)!: require fee history for storage refunds and credit their recorded owners - #4706
DCG-Claude wants to merge 20 commits into
Conversation
|
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 configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 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 |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-09-24T17:04:48.750Z |
|
⛔ Final review complete — 1 blocking finding(s) (commit 1a7bc4b) · triage: critical · Phase 2 only (queue backlog) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.3-dev #4706 +/- ##
============================================
- Coverage 84.10% 82.81% -1.29%
============================================
Files 2797 2801 +4
Lines 379933 386062 +6129
============================================
+ Hits 319541 319727 +186
- Misses 60392 66335 +5943
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Verified the Phase-2 findings against head 0b8d9c2. The new refund primitive misreports credits consumed by identity debt, violating its documented settlement contract; this is an in-scope API defect even though production lifecycle integration is deferred. The book also describes that deferred integration as already implemented. Verification was source-based; tests were not rerun.
🔴 1 blocking | 🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — This large, intricate diff changes consensus-versioned fee calculation and storage-refund crediting, directly affecting funds movement through functions such as Drive::calculate_fee_v1 and credit_storage_refunds_to_owners_operations. - Phase 1 reviewers: not run (skipped for throughput: 11 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— 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/drive/identity/update/methods/credit_storage_refunds_to_owners_operations/v0/mod.rs`:
- [BLOCKING] packages/rs-drive/src/drive/identity/update/methods/credit_storage_refunds_to_owners_operations/v0/mod.rs:59-67: Account for refund credits consumed by identity debt
`add_to_identity_balance_operations` does not necessarily add the requested amount to the owner's balance: when the existing balance is zero, `add_to_previous_balance_v0` first repays negative-credit debt. The new primitive nevertheless reports the entire refund as `credited` and exposes no amount for settling that debt repayment. For an owner with balance 0 and debt 100, a refund of 150 generates balance 50 and debt 0, but reports `credited = 150` and `routed_to_processing_pool = 0`. Negative credit is stored as an ordinary `Item`, outside the conservation sum trees, while recording the full pending refund contributes -150 to the pools. Following the documented caller contract therefore leaves accounted credits short by 100. The existing conservation test uses a positive owner balance and misses this branch. Although lifecycle callers are explicitly deferred to part 2, correct settlement reporting is a deliverable of this PR's new API; fixing this does not require activating those callers or changing the frozen helper. Account for and expose the debt-repayment portion so the caller can include it in its single processing-pool write, and add conservation tests for refunds below, equal to, and above outstanding debt.
In `book/src/fees/overview.md`:
- [SUGGESTION] book/src/fees/overview.md:244-245: Distinguish the refund primitive from pending lifecycle integration
This sentence describes lifecycle refund settlement as implemented, but the new primitive has no production callers at this head. In particular, `clean_up_after_contested_resources_vote_polls_end_v1` applies its cleanup operations with a discarded cost vector and never calculates or settles storage refunds. The PR explicitly reserves that integration for part 2. Describe settlement as the responsibility of future lifecycle callers, or state that the primitive is available but lifecycle integration is pending; no expansion of this PR's implementation scope is needed.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Make refund settlement debt-aware before activating lifecycle callers — Already covered by the canonical in-scope API finding rather than retained as a separate follow-up. The absence of production callers confirms there is no newly reachable lifecycle exploit at this head, but the new outcome explicitly promises credits added to balances and the amount callers must settle into the pool. Its failure to account for debt repayment directly violates this PR's primitive contract. Broader changes to the pre-existing state-transition settlement path remain outside this review.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
|
The red The re-pinned review engine from #4713 looks the caller workflow up on the pull request's base branch ( 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
At head 8b9e5aa, both prior findings are fixed: the refund outcome accounts separately for debt repayment, and the documentation explicitly identifies lifecycle settlement as pending. One non-blocking documentation inconsistency remains between the introductory payer-based description and the recorded-owner semantics. Verification used source and regression-test inspection; tests were not rerun.
🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Describe refunds as belonging to the recorded storage owner
book/src/fees/overview.md:51-53
The introduction says storage refunds return to the identity that paid the fee, but the new subsection at lines 236–245 identifies the owner recorded in storage flags as the refund recipient. The new settlement primitive likewise uses the owner identifiers in FeeRefunds, not an original-payer lookup. Align this introductory description with the recorded-owner rule so readers do not infer that paying for storage determines refund entitlement; retain the later qualification that lifecycle integration is pending.
Storage fees are **refundable**: when data is deleted, a portion of the original
storage fee becomes a refund attributed to the recorded owner of the stored bytes
(see [Refunds](#refunds) below).
source: gpt-6-astra (phase2-reviewer: general)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The diff introduces intricate protocol-versioned refund accounting that changes consensus fee calculation in consume_to_fees_v1 and funds movement in credit_storage_refunds_to_owners_operations_v0, including historical pricing, recorded-owner balance credits, debt repayment, and missing-owner pool allocation reporting. - Phase 1 reviewers: not run (skipped for throughput: 14 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— 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 `book/src/fees/overview.md`:
- [SUGGESTION] book/src/fees/overview.md:51-53: Describe refunds as belonging to the recorded storage owner
The introduction says storage refunds return to the identity that paid the fee, but the new subsection at lines 236–245 identifies the owner recorded in storage flags as the refund recipient. The new settlement primitive likewise uses the owner identifiers in FeeRefunds, not an original-payer lookup. Align this introductory description with the recorded-owner rule so readers do not infer that paying for storage determines refund entitlement; retain the later qualification that lifecycle integration is pending.
|
Good catch; the introduction still had the pre-storage-flags wording. Fixed in 43b587e: the sentence now says the refund goes to the owner recorded in the stored bytes' storage flags, which is not always the identity that paid the fee, and points at the Refunds section as before. I did not take the suggestion's exact text only because "attributed to" hides that payer and owner can differ, which is the point of the recorded-owner rule. 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The recorded-owner documentation, lifecycle-integration clarification, and debt-repayment accounting address all three prior findings. The proposed fee-history blocker is not supported by the current history representation and refund resolver; one non-blocking duplicate-read improvement remains. Verification was source-based at the exact head; no runtime tests were executed.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The diff introduces intricate, protocol-versioned changes to consensus fee calculation in consume_to_fees_v1 and funds movement in credit_storage_refunds_to_owners_operations_v0, including historical refund pricing, recorded-owner balance credits, debt repayment, and missing-owner processing-pool accounting. - Phase 1 reviewers: not run (skipped for throughput: 12 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— 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/drive/identity/update/methods/credit_storage_refunds_to_owners_operations/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/drive/identity/update/methods/credit_storage_refunds_to_owners_operations/v0/mod.rs:78-87: Avoid duplicate state reads when constructing refund credits
Each existing owner’s balance is read at line 49 and then read again inside `add_to_identity_balance_operations_v0`. For a zero-balance owner, the negative-credit element is also read here and again inside `add_to_previous_balance_v0`, producing four stateful reads for one owner’s update. These reads also flow through the cost accumulator, so batches with many owners accumulate unnecessary read work and processing costs. Share the fetched balance and debt results with the update construction, or derive debt repayment from the balance-update outcome, while preserving the shipped helper implementations and the new conservation accounting.
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The protocol version 15 fee-history enforcement and recorded-owner refund primitive are correctly versioned, preserve shipped behavior, and are supported by targeted conservation and read-count tests. All four prior findings were independently revalidated as fixed at the exact head, and no new in-scope defects were found.
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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — This large, intricate diff changes consensus-versioned fee calculation and identity balance crediting for storage refunds, directly affecting funds movement and owner attribution in files such asconsume_to_fees_v1andcredit_storage_refunds_to_owners_operations. - 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,muse-spark-1.3-contributor— security-auditor (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 11% left, 5h 100% left),glm-5.3-flash(zai below 15% reserve: 5h 99% left, weekly 13% left) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
|
🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
|
Ready for review — needs QuantumExplorer or shumkov. |
|
/self-reviewed f26c668 |
f26c668 to
dd8516c
Compare
|
Rebased onto the current What changed in the rebuild of the first commit (the protocol version 15 scaffolding), because upstream reworked the tables it copies from:
The one context-only difference in the group closing test is upstream's rename of the trusted deserialize trait import next to the hunk. Local gate: fmt, clippy on drive and platform-version with warnings as errors, workspace check with all targets, and the fee history, refund credit, group, migration and ranked index tests all pass. 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
|
The Rust workspace failure on dd8516c was this branch meeting the contract moderation work that landed on 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
|
🌳 GroveDB structure This pull request changes the described GroveDB structure. Open it in the structure viewer: new nodes glow, removed ones stay as ghosts, and the tour walks through each change. Changed (28 nodes)
Compared |
|
Second rebase fallout on 7add6b5, one test this time: 2dacc92 gives v1 the same arm as v0 (ephemeral bytes to processing at the ephemeral rate, zero storage, basic removal with no history needed, sectioned removal rejected as corrupted state) and pins v0 == v1 on ephemeral operations with a new test; the twin test's delete of owner-flagged standing bytes now passes a history like its neighbours. 10cd829 routes the structure fixture's group-action close through Local: 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
…or storage refund fee history Protocol version 15 is a copy of 14 whose drive table (DRIVE_VERSION_V10) selects calculate_fee v1 and the new identity update slot credit_storage_refunds_to_owners (DRIVE_IDENTITY_METHOD_VERSIONS_V3). Shipped identity tables backfill the slot with None so every shipped protocol version is behaviour-preserving. Refs #4675, Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…r recorded owners calculate_fee v1 (consume_to_fees_v1) consults the block's fee history for every owner-attributed storage removal and returns CorruptedCodeExecution without one, on every fee version number; v0 stays byte-identical. The new versioned method credit_storage_refunds_to_owners_operations credits each recorded owner that has a balance element without consulting any key or permission and reports the amount whose owner has no balance for the caller to route to the processing pool. Refs #4675, Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ocessing pool add_to_identity_balance_operations clears negative credit before raising a zero balance, and that share never reaches the credit sum trees. The refund primitive now measures it, reports it as repaid_debt beside the unrouted amount, and exposes processing_pool_share() for the caller's single pool write. Tests cover refunds below, equal to and above the outstanding debt with the conservation check, and a positive balance that repays nothing. Refs #4675, Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The balance read that decides whether the owner exists now feeds add_to_previous_balance directly, followed by the balance and negative credit update operations, the same shape the payer's own refund uses. The shipped helper reads the negative credit only from a zero balance, so an owner costs one or two stateful reads instead of up to four, and the repaid debt is derived from the helper's outcome instead of a separate read. A test pins the read count per owner. Refs #4675, Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…duction funnel with fee history The contract moderation tests that landed on the base after this branch was cut remove or shrink moderator-flagged entries through the bare fee-returning wrappers, which pass no fee history. From protocol version 15 pricing such a removal without the history is an error, so those calls now go through apply_drive_operations with a history, the funnel production uses; calls that only insert keep using the wrappers. Refs #4675, Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
consume_to_fees_v1 was written before the time-range TTL work added the ephemeral cost arm to v0 and the rebase kept v1 without it, so a TTL'd index write was billed to storage at protocol version 15. v1 now carries the same arm: added bytes bill to processing at the ephemeral rate, storage stays zero, removal is basic and needs no fee history, a sectioned removal is corrupted state. A test pins v1 equal to v0 on ephemeral operations. The TTL twin test's delete of owner-flagged standing index bytes now passes a fee history. Refs #4675, Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
From protocol version 15 pricing an owner-attributed storage removal without the fee history is an error. The fee-returning entry points that start their own transaction when the caller passes none committed it before pricing, so that error could come back after the write had been persisted. New generations hold the owned transaction until Drive::calculate_fee succeeded and drop it with everything it wrote on an error: apply_drive_operations v2 (finalize tasks still run after the commit), add_group_action v1, and the three moderation writers that can free moderator-flagged bytes (remove_contract_ban, remove_contract_suspension, add_contract_suspension) v1. Drive table v10 selects them through DRIVE_GROUP_METHOD_VERSIONS_V2 and DRIVE_CONTRACT_METHOD_VERSIONS_V5. With a caller transaction nothing changes; shipped generations are byte-identical. Refs #4675, Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… transaction drive owns The six fee-returning document wrappers (add, delete, delete by contract id, update, update by contract id, update with serialization), update_contract and apply_contract_with_serialization committed the transaction they own before pricing, so at protocol version 15 the missing-fee-history error could return with the write persisted. New generations (document wrappers v1, update_contract v3, apply_contract_with_serialization v1) hold the owned transaction until Drive::calculate_fee succeeded; the operation builders, the element writer and the _apply_and_add_to_operations methods production uses are untouched and their dispatchers accept the new numbers. The contract cache keeps its committed path under an owned transaction: contracts are looked up through the caller's transaction and a rewritten copy replaces the global entry only after the commit. DRIVE_DOCUMENT_METHOD_VERSIONS_V5 and DRIVE_CONTRACT_METHOD_VERSIONS_V5 select them in drive table v10. Refs #4675, Refs #4689 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1f2a124 to
1a7bc4b
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
The protocol-15 fee-history guard and recorded-owner refund accounting are correctly versioned in the reviewed paths, and the debt, documentation, and duplicate-read findings are resolved. However, three public Drive writers still use generation 0 and can commit owner-attributed removals before the new missing-history error is returned: add_document, index-only document deletion, and warning replacement.
🔴 1 blocking
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — This large, intricate diff changes consensus fee calculation and storage-refund funds movement in Drive, including owner attribution and crediting behavior inDrive::calculate_fee_v1andcredit_storage_refunds_to_owners_operations. - Phase 1 reviewers: not run (skipped for throughput: 15 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— 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-platform-version/src/version/drive_versions/drive_document_method_versions/v5.rs`:
- [BLOCKING] packages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v5.rs:20-35: Reject missing fee history before committing Drive mutations
The protocol-15 table bumps the six ordinary document wrappers but leaves `add_document`, `delete_index_only_document_for_contract_operations`, and `delete_index_only_document_for_contract` at generation 0. `add_document_v0` accepts owner flags through `OwnedDocumentInfo` and supports `override_document`; its applying helper commits an internally owned transaction before `calculate_fee(None, ...)` returns the new missing-history error. The index-only deletion wrapper explicitly commits its owned transaction at `delete_index_only_document_for_contract_operations/v0/mod.rs:84-86` before pricing at lines 88-95. The same ordering remains in `add_contract_warning_v0`: replacing an existing moderator-flagged warning entry can shrink the stored bytes, while the batch application commits before `calculate_fee` is called with no history. Thus a protocol-15 caller can receive `CorruptedCodeExecution` after the document, index entry, or warning replacement has already been persisted, losing the associated fee/refund result. Add new price-before-commit generations and select them in the protocol-15 tables, or otherwise retain the owned transaction until pricing succeeds; preserve the shipped generations and add unchanged-state regression tests for these paths.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Wire vote-poll cleanup into refund settlement —
clean_up_after_contested_resources_vote_polls_end_v1still applies cleanup operations, including owner-flagged contested data removals, without calculating or settling storage refunds. The PR explicitly defers this lifecycle integration to Part 2, so it should remain a separate change rather than expand this PR.- Follow-up: Complete the planned Part 2 integration by pricing the cleanup with
previous_fee_versionsand atomically applying the processing-pool and pending-refund operations.
- Follow-up: Complete the planned Part 2 integration by pricing the cleanup with
| insert: DriveDocumentInsertMethodVersions { | ||
| add_document_for_contract: 1, | ||
| ..DRIVE_DOCUMENT_METHOD_VERSIONS_V4.insert | ||
| }, | ||
| update: DriveDocumentUpdateMethodVersions { | ||
| update_document_for_contract: 1, | ||
| update_document_for_contract_id: 1, | ||
| update_document_with_serialization_for_contract: 1, | ||
| ..DRIVE_DOCUMENT_METHOD_VERSIONS_V4.update | ||
| }, | ||
| delete: DriveDocumentDeleteMethodVersions { | ||
| delete_document_for_contract: 1, | ||
| delete_document_for_contract_id: 1, | ||
| ..DRIVE_DOCUMENT_METHOD_VERSIONS_V4.delete | ||
| }, | ||
| ..DRIVE_DOCUMENT_METHOD_VERSIONS_V4 |
There was a problem hiding this comment.
🔴 Blocking: Reject missing fee history before committing Drive mutations
The protocol-15 table bumps the six ordinary document wrappers but leaves add_document, delete_index_only_document_for_contract_operations, and delete_index_only_document_for_contract at generation 0. add_document_v0 accepts owner flags through OwnedDocumentInfo and supports override_document; its applying helper commits an internally owned transaction before calculate_fee(None, ...) returns the new missing-history error. The index-only deletion wrapper explicitly commits its owned transaction at delete_index_only_document_for_contract_operations/v0/mod.rs:84-86 before pricing at lines 88-95. The same ordering remains in add_contract_warning_v0: replacing an existing moderator-flagged warning entry can shrink the stored bytes, while the batch application commits before calculate_fee is called with no history. Thus a protocol-15 caller can receive CorruptedCodeExecution after the document, index entry, or warning replacement has already been persisted, losing the associated fee/refund result. Add new price-before-commit generations and select them in the protocol-15 tables, or otherwise retain the owned transaction until pricing succeeds; preserve the shipped generations and add unchanged-state regression tests for these paths.
source: gpt-6-astra (phase2-reviewer: general, platform-versioning)
Issue being fixed or feature implemented
Part 1 of 3 for R12-02 of the smart-contract plan (#4626, section 12, fee workstream #4689, preparation package #4675).
Storage refunds are priced against the fee history a block carries (
previous_fee_versionsin platform state). The shipped fee calculation (Drive::calculate_feev0,consume_to_fees_v0) short-circuits fee version number 1: it prices every owner-attributed removal against an empty history, so a caller that forgets to pass the history silently refunds at the first generation's storage rates instead of failing. The fee version integrity rule in the fee DIP requires the opposite: a refund without the historical context of the removing block is an error, never a fallback to a schedule.The same DIP requires refunds to follow the recorded owner and, for owners that no longer exist, to go to the current processing pool (acceptance Q26: a frozen but existing owner still receives native bookkeeping refunds without any spending permission; a wiped owner's refunds go to the processing pool). Today the only code that credits other owners (
apply_balance_change_from_fee_to_identityv0) halts on an owner without a balance, and block lifecycle paths that free owner-flagged bytes never price or settle refunds at all.This part establishes the explicit version boundary and the primitive the later parts build on:
packages/rs-platform-version/src/version/v15.rs) withDRIVE_VERSION_V10;Drive::calculate_feev1, which requires and consults the fee history for every owner-attributed storage removal;Drive::credit_storage_refunds_to_owners_operations, which credits recorded owners without consulting any key or permission and reports the amount whose owner has no balance element.Part 2 (same base) makes the vote poll end cleanup price and settle its refunds with this primitive. Part 3 (base
v5.0-dev) routes state transition refunds for missing owners to the processing pool and carries the DIP text.What was done?
Version tables (
packages/rs-platform-version)src/version/v15.rs:PROTOCOL_VERSION_15andPLATFORM_V15, a copy of v14 whose only change isdrive: DRIVE_VERSION_V10. Registered insrc/version/mod.rs(LATEST_VERSION) andsrc/version/protocol_version.rs(PLATFORM_VERSIONS,LATEST_PLATFORM_VERSION).LATEST_VERSIONwas 14 at lane start, so the version was introduced rather than amended; the open state sync PR (feat(drive-abci)!: state sync via ABCI snapshots with reduced platform state (protocol v15) #4648) introduces the same number and whoever rebases second merges thev15.rsdoc comment.src/version/drive_versions/v10.rs:DRIVE_VERSION_V10, a copy of v9 withfees.calculate_fee: 1andidentity: DRIVE_IDENTITY_METHOD_VERSIONS_V3.src/version/drive_versions/drive_identity_method_versions/mod.rs:DriveIdentityUpdateMethodVersionsgainscredit_storage_refunds_to_owners: OptionalFeatureVersion. Backfilled asNoneinv1.rsandv2.rs(shipped tables stay behaviour-preserving); the newv3.rssetsSome(0).Fee calculation (
packages/rs-drive/src/fees)op.rs:LowLevelDriveOperation::consume_to_fees_v1beside the untouchedconsume_to_fees_v0. TheSectionedStorageRemovalarm takes the system bucket as before, then requiresprevious_fee_versionson every fee version number and returnsDriveError::CorruptedCodeExecution("a storage refund needs the fee history of the block that removes the bytes")without it. The empty-map shortcut for number 1 is gone in v1.calculate_fee/v1/mod.rs:Drive::calculate_fee_v1, a copy of v0 callingconsume_to_fees_v1.calculate_fee/mod.rsdispatches1 =>and reportsknown_versions: vec![0, 1]. v0 is byte-identical. After the rebase onto the time-range TTL work (feat(drive): time-range index TTL — O(1) flat-drop drainage and ephemeral-bytes fees #4581) v1 carries v0'sCalculatedEphemeralCostOperationarm unchanged: TTL'd index bytes bill to processing at the ephemeral rate, storage stays zero, their removal is basic and needs no history, and a sectioned removal inside an ephemeral batch is corrupted state.Pricing before commit (
packages/rs-drive)From protocol version 15 a missing fee history is an error, and the fee-returning Drive entry points that start their own transaction when the caller passes none committed it before pricing, so that error could come back after the write had been persisted (found by the automated review). New generations hold the owned transaction until
Drive::calculate_feesucceeded and drop it with everything it wrote on an error; with a caller transaction nothing is committed by Drive in either generation and nothing changes:util/batch/drive_op_batch/drive_methods/apply_drive_operations/v2/mod.rs: generation 1 with the price computed before the owned transaction commits; finalize tasks still run after the commit.DRIVE_VERSION_V10setsbatch_operations.apply_drive_operations: 2.drive/group/insert/add_group_action/v1/mod.rs: the fee-returning wrapper owns a transaction and prices before committing;add_group_action_add_to_operationsv1 is a copy of v0.DRIVE_GROUP_METHOD_VERSIONS_V2(insert.add_group_action: 1).drive/contract/moderation/{remove_contract_ban,remove_contract_suspension,remove_contract_warnings,add_contract_suspension}/v1/mod.rs: the four moderation writers that can free moderator-flagged bytes, same shape; their_operationsv1 are copies of v0.drive/document/{delete/delete_document_for_contract,delete/delete_document_for_contract_id,update/update_document_for_contract,update/update_document_for_contract_id,update/update_document_with_serialization_for_contract,insert/add_document_for_contract}/v1/mod.rs: the six fee-returning document wrappers, same shape; the_apply_and_add_to_operationsmethods production uses throughapply_drive_operationsare untouched.DRIVE_DOCUMENT_METHOD_VERSIONS_V5bumps the six slots.drive/contract/update/update_contract/v3/mod.rsanddrive/contract/apply/apply_contract_with_serialization/v1/mod.rs: the two contract writers, same shape; the element writer stays generation 2 and the operation builder generation 0 (their dispatchers accept the wrapper's number). The contract cache keys its block-versus-committed behaviour on whether a transaction is present, so under an owned transaction the contract is looked up through the caller's (none) and the rewritten copy replaces the global entry only after the commit.DRIVE_CONTRACT_METHOD_VERSIONS_V5bumpsupdate_contractto 3,apply_contract_with_serializationto 1 and the four moderation slots to 1.The bare group and moderation wrappers still pass no fee history, so a call that frees flagged bytes still fails at protocol version 15; the document and contract wrappers take the caller's history and fail the same way when it is missing. In every case the failure now comes before anything is written. Tests:
apply_drive_operations::v2::tests(a batch that cannot be priced leaves the action open and the same batch with the history closes it; protocol version 14 still commits without a history),should_reject_closing_a_group_action_through_the_bare_wrapper_without_fee_historynow asserts the action is still active and nothing moved,should_commit_the_owned_transaction_when_pricing_succeeds_at_the_latest_version,should_leave_a_ban_in_place_when_the_bare_wrapper_cannot_price_its_removal(root hash and every list unchanged after four rejected removals; estimation writes nothing; protocol version 14 removes),should_leave_a_document_in_place_when_the_wrapper_cannot_price_its_removal(the document and root hash unchanged after a rejected delete, the same delete with the history refunds the owner and commits, protocol version 14 deletes without one) andshould_leave_a_contract_in_place_when_the_wrapper_cannot_price_its_update(a shrinking update rejected without the history leaves the stored contract, the root hash and the cached copy unchanged; with the history it commits and the cache follows).Recorded-owner credits (
packages/rs-drive/src/drive/identity/update)methods/credit_storage_refunds_to_owners_operations/{mod.rs, v0/mod.rs}: the new versioned method (Some(0)dispatch,None => VersionNotActive). For each owner in aFeeRefundsexcept an optionalskip_owner(the state transition payer, whose own refund folds into its balance change), it sums the per-epoch credits with checked arithmetic, reads the balance element statefully once, feeds that read intoadd_to_previous_balanceand the balance and negative credit update operations (the same shape the payer's own refund uses inapply_balance_change_from_fee_to_identity, so no element is read twice) when it exists, and otherwise adds the amount torouted_to_processing_pool. When the owner's balance is zero the shipped helper first clears its negative credit (identity debt) and only the remainder reaches the balance; the primitive derives that portion from the helper's outcome and reports it asrepaid_debt, because debt lives outside the credit sum trees and is processing fee the pools were short of when it was incurred. No key, signature or permission is read on any route. The primitive neither writes the processing pool nor records pending refunds; the caller writesprocessing_pool_share()(unrouted refunds plus repaid debt) once and records the pending refunds, so a block keeps one pool write and one pending-refund write per batch.structs/storage_refund_credit_outcome/mod.rs:StorageRefundCreditOutcome { credited: BTreeMap<Identifier, Credits>, repaid_debt: Credits, routed_to_processing_pool: Credits }with checkedprocessing_pool_share()andtotal().Tests
fees/op.rs(storage_refund_fee_historymodule): v1 rejects a sectioned removal without history (v0 still prices it); a system-bucket-only sectioned removal is rejected too; unflagged (BasicStorageRemoval) bytes never need the history; the history is consulted for fee version number 1 (a synthetic number-2 schedule with doubled storage rates at epoch 10, bytes stored at epoch 12 and removed at 15: v0 refunds at the first generation's rate, v1 at the doubled rate); for everyPLATFORM_VERSIONSentry and every history the epoch change hook could build, v1'sFeeResults equal v0's; every shipped schedule keeps fee version number 1 and the first generation's storage rates (the premise of that equality); ephemeral TTL operations price identically in v0 and v1 with no history and a sectioned removal in an ephemeral batch is rejected in both.fees/calculate_fee/mod.rs: through the dispatcher,PlatformVersion::get(14)prices an owner-attributed removal without history,PlatformVersion::latest()refuses it, and with a history both produce the same fees.credit_storage_refunds_to_owners_operations/v0/mod.rs: each recorded owner credited by the sum of its epochs; an owner without a balance is reported, not an error, and no balance element is created; an owner whose keys are all disabled is credited;skip_owneris left alone; after the caller writes the pool and records the pending refunds,calculate_total_credits_balancestays balanced (TotalCreditsBalance::ok); refunds below, equal to and above an owner's outstanding debt report the repaid share and leave the credit sum balanced once the caller writes the pool share; a positive balance repays no debt;PlatformVersion::get(14)returnsVersionNotActive.drive/group/mod.rs:should_close_group_action_and_move_signersnow closes the action the way production does (aGroupOperationType::AddGroupAction { closes_group_action: true, .. }throughapply_drive_operationswith the fee history). New:should_refund_signer_bytes_when_a_group_action_closes(the opening signer's flagged items are refunded against their storage epoch; the closing signer, whose items are unflagged, is not) andshould_reject_closing_a_group_action_through_the_bare_wrapper_without_fee_history(CorruptedCodeExecutionat the latest version, success at protocol version 14). The two other closing-branch tests (immediate close with new action info, cost estimation withapply = false) free no flagged bytes and stay on the bare wrapper, which pins that the rule fires only when flagged bytes are actually removed.drive/contract/migration/strip_unknown_document_schema_properties.rs:should_keep_the_protocol_12_schema_strip_frozen_without_refunding_stripped_bytes, a freeze test atPlatformVersion::get(12): a user contract element carrying the owner's storage flags is inflated with a top-level schema property the v1 meta-schema forbids, the migration shrinks it back to its clean bytes, the flags are byte-equal, the owner's balance and the pending refund tree are unchanged, and the credit sum stays balanced. The doc comment carries the replay rationale (below).ranked_index_e2e_tests::estimated_and_actual_update_fees,update::tests::summable_index_update_changes_key_into_new_branch_materializes_aggregate_tree_type); both now pass the same one-entry history the neighbouring tests use. After the rebase onto the contract moderation work (feat(platform)!: contract moderation with a banlist and a suspension list #4830, feat(platform)!: a reason on contract bans and suspensions #4849), the moderation tests indrive/contract/moderation/tests.rsthat unban, unsuspend or replace a suspension (removals of moderator-flagged entries) go throughapply_drive_operationswith a history, the funnel the moderation state transition uses; the bareremove_contract_ban,remove_contract_suspension,remove_contract_warningsand replacingadd_contract_suspensionwrappers pass no history and are otherwise called only from tests that insert. The structure fixture instructure/tests.rscloses its group action the same way, the TTL twin test intime_range_index_e2e_tests.rspasses a history when it deletes owner-flagged standing index bytes, andgrovedb-structure.jsonis regenerated (only the@14fixture labels become@15).Book
book/src/fees/overview.md: a "Fee history and refund ownership (protocol version 15 onward)" subsection under Refunds, and a paragraph onfee_version_numberunder Fee Versioning.Caller audit
Drive::calculate_feehas 129 call sites in 99 files ofpackages/rs-drive/src(14 forward a caller's history; the rest passNone). Grouped by what they can remove:apply_drive_operations,update_contract, document delete, update and add methods, identity balance and revision, token balance updatesprevious_fee_versions. The document wrappers andupdate_contractgained generations (v1, v3) that price before committing the transaction they own when a caller passes none, so a missing history fails before mutation.None, can remove owner-flagged bytesdrive/group/insert/add_group_action(closing an action moves signer-flagged items)NoneGroupOperationType::AddGroupActioninsideapply_drive_operations, which forwards the history; the bare wrapper is called from test sites, of which the closing ones now use the production funnel. From protocol version 15 a closing call through the bare wrapper is a misuse the strict rule surfaces before anything is written (v1 prices before committing its owned transaction; pinned by test).None, can remove owner-flagged bytesdrive/contract/moderation/{remove_contract_ban,remove_contract_suspension,remove_contract_warnings,add_contract_suspension}bare wrappers (entries are flagged with the moderator; a removal or a shorter replacement frees them)NoneContractModerationOperationTypeinsideapply_drive_operations(history forwarded, from the contract moderation transition); the wrappers are called from tests and drive-abci query fixtures, the removing ones now through the funnel. Their v1 generations price before committing an owned transaction.None, can remove owner-flagged bytesdrive/contract/apply/apply_contract_with_serialization(replace path; v1 prices before committing an owned transaction)Nonestorage_flags: None), the protocol 13 and 14 transitions (perform_events_on_first_block_of_protocol_change/v0/mod.rs:683,707re-store DPNS and DashPay over unflagged genesis elements withNoneflags, so the removal isBasicStorageRemoval), andcreate_mn_shares_contract(test-only). A sectioned removal here needs a flagged old element and flagged new flags, which only tests produce. Unchanged.None, elements unflagged todaydisable_identity_keys/v0(its TODO asks this question),update_keywords/v0,update_description/v0(keyword and description documents are stored withDocumentOwnedInfo((doc, None))), token status, price, supply and contract info (Element::Item(.., None),SumItem(.., None)), identity nonce, revision and balance, vote sum items and references,add_document/v0NoneNone, inserts, fetches, proofs and queries onlydrive/document/query/**,query/**, token fetch and prove, identity fetch, contested document inserts, vote registration, group inserts,insert_contractv0/v1)Nonedrive-abcidirectDrive::calculate_feecallers (fetch_contender.rs, the index-only delete and document create state validators) price reads only.Block lifecycle applications in
packages/rs-drive-abci/src/execution/platform_events(non-test code):voting/clean_up_after_contested_resources_vote_polls_end/{v0,v1}: deletes contested documents and contender trees (owner-flagged), the end-date entry (creator-flagged), votes and stored info (unflagged); v1 also empties prefunded balancesapply_batch_low_level_drive_operations(None, ..)voting/award_document_to_winner/v0DocumentAndSerialization((doc, bytes, None)))add_document_for_contract(.., None)Nonevoting/keep_record_of_vote_poll/v0,voting/remove_votes_for_removed_masternodes/v0core_based_updates/update_masternode_identities/v0apply_drive_operationsSome(platform_state.previous_fee_versions())block_processing_end_events/process_block_fees_and_validate_sum_trees/v0apply_drive_operationsSome(block_platform_state.previous_fee_versions())dequeue_and_build_unsigned_withdrawal_transactions/v0,pool_withdrawals_into_transactions_queue/v1,rebroadcast_expired_withdrawal_documents/v1,update_broadcasted_withdrawal_statuses/v0)owner_id: Noneand no flags (identity_credit_withdrawal_transition.rs,address_credit_withdrawal_transition.rs)apply_drive_operationsNoneinitialization/create_genesis_state/{v0,v1}storage_flags: None)apply_drive_operationsNoneprotocol_upgrade/perform_events_on_first_block_of_protocol_change/v0(DPNS re-store at 13, DashPay at 14)apply_contractNoneprotocol_upgrade/.../v0transition_to_version_12callingstrip_unknown_document_schema_propertiesgrove_insertwith a discarded cost vectorcheck_tx/v0,process_raw_state_transitions/v0,execute_event/v0,validate_fees_of_event/v0apply_drive_operationsSome(state.previous_fee_versions())apply_balance_change_from_fee_to_identityConclusion: one production path frees owner-flagged bytes without fee context and without settling refunds (vote poll end cleanup, part 2); one shipped migration discarded refunds and is frozen (pinned here); one test-only wrapper closes group actions without history (its closing test now uses the production funnel); every other path is correct or touches unflagged bytes.
Owner-flagged writes (what the invariant covers)
Documents, index references, contested documents and contested index trees (document owner); the vote poll end-date entry (contest creator); user data contract items (
insert_contractv1: every contract withcan_be_deleted() || !readonly(), contract owner); group action info and signer sum items (signer); token distribution items (token owner, recipient, claimer). Not flagged: identities and their keys, withdrawal documents, keyword-search documents, votes, prefunded balances, epoch pools, withdrawal queue items, shielded anchors, stored vote poll info, and the system contracts at genesis. The document history contract registered at the protocol 13 transition goes throughinsert_contractv1 and therefore carries the system owner's flags; a future transition that re-stores it must pass its flags or hit grovedb'sRemovingFlagsError(noted, no change here).How Has This Been Tested?
Local gate (exit codes captured under the run directory):
The full
driveunit suite passes (3594 passed, 5 ignored); the rest of the workspace is CI's. The workspace check was run in a privateCARGO_TARGET_DIRbecause another lane editingrs-platform-versionshares the default target directory and its fingerprint collided with this tree's new identity slot. Nosrc/verify/**file is touched, so the verify-only cut is unaffected (cargo check --workspace --all-targetscompiles the verify crates).Breaking Changes
Consensus: protocol version 15 is introduced. Under its drive table a storage refund for owner-attributed bytes is priced only with the fee history of the removing block; a missing history is an internal error (a halt on a block execution path) instead of a silent fallback to the first generation's rates. The Drive entry points that own their transaction when a caller passes none (
apply_drive_operationsv2, the six document wrappers v1,update_contractv3,apply_contract_with_serializationv1,add_group_actionv1, four moderation writers v1) price before they commit, so that error never leaves a write persisted without its fee result. Every shipped schedule shares fee version number 1 and the same storage rates, so refund credits are unchanged for every shipped input; every shippedPLATFORM_V*,calculate_feev0,consume_to_fees_v0,add_group_action,apply_balance_change_from_fee_to_identityv0 and the protocol 12 migration are byte-identical. Shipped identity tables gain oneNonefield.API:
DriveIdentityUpdateMethodVersionsgains a field (struct literals outside the version crate: none).Drive::credit_storage_refunds_to_owners_operationsandStorageRefundCreditOutcomeare new. No production path calls the primitive at this head; part 2 wires the vote poll end cleanup to it. No proto, SDK, wasm or FFI change;PlatformVersion::latest()now resolves to 15.Checklist:
For repository code-owners and collaborators only
Decisions taken (provisional values)
add_epoch_processing_credits_for_distribution_operationis a read-modify-write of the epoch's pool sum item, so two of them in one batch collapse last-wins. Returningrouted_to_processing_poolandrepaid_debt(togetherprocessing_pool_share()) lets the caller issue exactly one pool write per batch and keep pending-refund recording with the existing versioned methods. Repaid debt goes to the processing pool because negative credit is processing fee the pools were short of when the identity could not cover it (fee_result_outcomedrops that part from the block fees), so clearing it with a refund returns those credits to the pool they were owed to.add_group_actionwith a history parameter; this PR instead routes the one closing test through the funnel production uses and pins the wrapper's strictness.FeeRefunds::from_storage_removal; it is not onv4.3-devyet, so the test that would pin it is deferred to part 2.LATEST_VERSIONwas 14 at lane start.Part 1 of 3 for R12-02.
Refs #4675
Refs #4689
🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta.
🤖 Generated with Claude Code
PR Hygiene ·
1a7bc4bbook/src/fees/overview.md,packages/rs-platform-version/src/version/drive_versions/drive_contract_method_versions/mod.rs,packages/rs-platform-version/src/version/drive_versions/drive_contract_method_versions/v5.rsand 13 more) — QuantumExplorer or shumkovrs-drive-abci(packages/rs-drive-abci/src/query/document_query/v1/dispatch/chained.rs) — QuantumExplorer or shumkovrs-drive(packages/rs-drive/grovedb-structure.json,packages/rs-drive/src/drive/contract/apply/apply_contract_with_serialization/mod.rs,packages/rs-drive/src/drive/contract/apply/apply_contract_with_serialization/v1/mod.rsand 45 more) — QuantumExplorer or shumkovWhen every box is checked the
PR Hygienecheck passes and this can merge.