fix(drive)!: merge repeated writes of one balance in a batch so an action fee no longer loses a purchase price - #4987
Merged
Conversation
Every identity balance and contract fee pot operation computes the new value from the one committed before its batch, and GroveDB keeps only the last write of a key. A document action fee is appended to the transition's own operations, so when those already write the payer's balance one of the two writes was lost: - a purchase that owes a purchase fee kept the price in the buyer's balance while the seller was still paid; - a contested document's creation that owes a creation fee kept the voting fund in the creator's balance; - a purchase from a contract owner who sponsors its gas lost either the sale price or the moderators part of the fee. The credits then no longer add up. apply_drive_operations 1 (selected only by protocol version 14, edited in place) now merges every write of one identity balance or one fee pot in a batch into a single net operation, at the place of the first one. A key written once keeps its operation untouched, so every other batch is applied exactly as before. Tests: merge unit tests in drive; a buyer-pays purchase, a purchase from a sponsoring seller and a contested creation, each owing an action fee, checked through check tx and execution, all three failing without the merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
the
waiting-bots
Contributor
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-09-25T16:48:05.994Z |
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughFee validation now returns the selected payer with its estimate, and execution charges that payer. Drive batches net prefunded specialized-balance writes and refuse repeated writes to a token balance or supply. Tests cover fee, sponsorship, purchase, and contested-domain outcomes. ChangesFee settlement and batch accounting
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant FeeValidation
participant Sponsor
participant Identity
participant execute_event_v1
FeeValidation->>Sponsor: Estimate sponsor gas and action fees
FeeValidation->>Identity: Estimate identity costs if sponsor does not pay
FeeValidation->>execute_event_v1: Return settled payer and estimate
execute_event_v1->>Sponsor: Charge sponsor when selected
execute_event_v1->>Identity: Charge identity when selected
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The identified compatibility and transfer concerns do not block merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change addresses a credit-accounting failure and keeps fee validation and execution aligned on the payer. No introduced security defect was established, but replay, concurrent execution, and recovery behavior were not fully verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Collaborator
|
🕓 Queued for automated review — 4th in line, estimated start in ~1.3 h (commit fe9dfd1)
|
thepastaclaw
suggested changes
Sep 24, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The balance merge is correctly confined to protocol version 14, but it introduces a confirmed fee-estimation regression that rejects an affordable sponsored purchase. The boundary-case test fails at the reviewed head and passes when only estimation retains its previous behavior; the original test also passes after restoring all temporary changes. The alternate payout-path issue is pre-existing and retained only as a separate follow-up, while the mixed-sign precondition finding does not establish a defect in the intended netting behavior.
🔴 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) — DriveOperation::merge_balance_writes and apply_drive_operations_v1 introduce intricate batch-wide netting of identity balances and fee pots, changing consensus-critical funds movement through signed aggregation, cancellation, overflow handling, and operation ordering. - Phase 1 reviewers: not run (skipped for throughput: 18 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-drive/src/util/batch/drive_op_batch/drive_methods/apply_drive_operations/v1/mod.rs`:
- [BLOCKING] packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/apply_drive_operations/v1/mod.rs:62: Keep waived owner fees from invalidating sponsored-purchase estimates
`validate_fees_of_event_v1` estimates the transition together with `action_fee_operations(identity.id, ...)` before deciding whether the contract owner sponsors it. Merging those operations combines the purchase price with hypothetical signer-paid fees, and `remove_from_identity_balance_operations_v0` rejects the combined debit when it exceeds the stateless estimate's `MAX_CREDITS` balance. I independently reproduced this by changing the sponsored-purchase fixture's declared owner fee to `MAX_CREDITS - MODERATORS_PART - 1`, which satisfies the fee limits: `FirstTimeCheck` returns an internal insufficient-balance error for removing 9223372037854775806 from 9223372036854775807. The actual purchase is affordable because the buyer pays only the price and the sponsoring contract owner pays only the moderators part plus gas; the owner part is waived. Keeping execution merging while temporarily restoring the previous estimation behavior makes the same boundary test pass, including its balance assertions. Make fee estimation account for sponsorship without rejecting on this hypothetical signer debit, and add the boundary case as a regression test.
Out-of-scope follow-up suggestions (2)
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.
- Epoch reward payouts can still overwrite additions to a shared recipient —
add_epoch_pool_to_proposers_payout_operations_v0accumulates reward-share and proposer balance additions, lowers them throughconvert_drive_operations_to_grove_operations, and wraps the resulting replacements inGroveDBOpBatch. The conversion path does not merge repeated balance additions, and the reward-share contract's unique index on($ownerId, payToId)permits different masternodes to share a recipient. Those already-lowered replacements remain opaque to the new merge. Both the caller and conversion behavior are present in the base commit and unchanged by this PR, so this is a separate credit-accounting issue rather than a regression required to fix the action-fee scenarios.- Follow-up: Track a separate version-gated repair for shared-recipient epoch payouts, with a test exercising the production conversion path.
- Normalize balances at both Drive batch-lowering entry points — Out of scope as a blocking request. Comparing base 9eb59ec with the reviewed head confirms that the epoch payout caller and alternate conversion path already lower repeated additions independently. This PR neither introduces nor worsens that path, and its action-fee repair does not depend on it. The concrete accounting concern is retained as a separate follow-up.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
Contributor
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
…timated Fee validation estimates the signer paying every action fee before it settles on a gas sponsor, and a contract owner who sponsors a purchase of their own document is never charged the owner part. An estimate prices each identity balance removal against MAX_CREDITS, so merging the purchase price with that hypothetical fee could exceed it and fail the estimate (an internal insufficient-balance error in check tx) for a purchase the sponsor pays in full. An estimate now keeps the writes as they are: separately they cost at least what the merged write does, and none of them exceeds the largest balance on its own. Only an applied batch is merged. Regression test: a sponsored purchase from the contract owner whose owner part is MAX_CREDITS - MODERATORS_PART - 1 passes check tx and executes; it fails check tx when estimates are merged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
waiting-bots
QuantumExplorer
changed the title
…n settles on Fee validation estimated every batch with the signer paying all action fees, owner part included, before deciding whether the contract owner sponsors the gas, so the estimate described a batch execution never applies when the sponsor pays. It now estimates for the payer: - with the sponsor paying first, and decides gas_sponsor_pays on that; - with the signer paying only when the sponsor does not pay, after refusing a signer whose balance cannot fund the principal and the action fees they owe (known without an estimate); - when the sponsor is passed over and the signer's estimate is the smaller, the sponsor's is returned: execution asks gas_sponsor_pays again on the returned estimate and must pass the sponsor over too. With estimates made for the real payer, apply_drive_operations 1 merges estimates as well as applied batches again, so what is estimated has the shape of what is applied. The pre-check keeps every merged removal an estimate makes for the signer within their balance. Regression test: a buyer paying the gas who cannot fund the price and a purchase fee near MAX_CREDITS is refused unpaid for an insufficient balance (40210) in check tx and at execution; without the pre-check the merged estimate fails check tx with an internal error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r's estimate Execution asks gas_sponsor_pays again on the estimate fee validation returns, so when fee validation passes a sponsor over and the signer's estimate is the smaller one, it returns the sponsor's. The new test builds that case: a purchase from a third-party seller owing only a moderators part, whose buyer-paid estimate is lower because the fee merges into the price, with the preferred sponsor settled one credit short of their own estimate. It checks the sponsor pays nothing, the returned estimate is the sponsor's, and the buyer pays price, fee and gas. Without the rule, execution charges the passed-over sponsor. The sponsor's estimate is measured at execution (check tx skips a batch's state validation) with the sponsor paying, and re-measured as their balance is lowered: reading that balance costs less as it shrinks. card_for_sale now takes the seller and the purchase fee; purchase_by takes the buyer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…efunded balances, refuse repeated token writes (PV14) Fee validation hands execution the payer it settled on. Execution used to ask gas_sponsor_pays again on the estimate fee validation returned, which forced fee validation to return the sponsor's estimate whenever it passed the sponsor over and the signer's estimate was the smaller one. settle_fees_of_event_v1 now returns SettledFees (the estimate for the payer and the paying sponsor), validate_fees_of_event_v1 maps it to the estimate for its other callers, and execute_event 1 charges the settled payer. The returned-estimate rule is gone: a passed-over sponsor's batch now returns the signer's own estimate. apply_drive_operations 1 also merges repeated writes of one prefunded specialized balance (a contested document's voting fund), which compute from the committed value like identity balances and fee pots. Token writes do the same, but a transfer writes two balances and a mint or a burn a balance and the supply, so they cannot be netted into operations of their own kinds; a batch writing one token balance or token supply twice is refused instead. No state transition makes such a batch: token operations come only from batch transitions, one per batch, each writing a key at most once. Tests: drive unit tests for the prefunded merge and the token refusal; the passed-over-sponsor test now checks the returned estimate is the buyer's, which the sponsor covers, and fails if execution asks the sponsor question again on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-1d405b # Conflicts: # packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/apply_drive_operations/v1/mod.rs # packages/rs-platform-version/src/version/drive_versions/v9.rs # packages/rs-platform-version/src/version/v14.rs
System credits and address balances are written like identity balances, from the value committed before the batch, but merge_balance_writes does not merge them because no batch repeats their keys today. Say so where the operations are defined and in merge_balance_writes, with what keeps it true (one system credits write per transition or block; no address both input and output, maps for inputs and outputs, input fees in a batch of their own). partially_use_asset_lock deducts each fee strategy step from the input's original balance and sets it absolutely, which holds only because basic structure validation refuses repeated steps; note that next to the loop. Comments only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Document action fees (#4851, protocol version 14) move credits from the payer to the contract's fee pots with operations appended to the transition's own ones. Every identity balance and fee pot operation computes the new value from the one committed before its batch, and GroveDB keeps only the last write of a key. When the transition already writes the payer's balance, one of the two writes is lost and the credits no longer add up:
Only protocol version 14, which no released network runs, is affected.
What was done?
apply_drive_operations1 merges every write of one identity balance, one fee pot or one prefunded specialized balance in a batch into one net operation (DriveOperation::merge_balance_writesinrs-drive/src/util/batch/drive_op_batch/mod.rs):AddToIdentityBalance/RemoveFromIdentityBalanceon the same identity,AddToPot/DeductFromPoton the same contract and pot, andCreateNewPrefundedBalance/DeductFromPrefundedBalanceon the same prefunded balance, become one operation for their net change, at the place of the first one;Token writes are refused, not merged (
DriveOperation::refuse_repeated_token_balance_writes). They compute from the committed value too, but a transfer writes two balances and a mint or a burn a balance and the supply, so they cannot be netted into operations of their own kinds. A batch writing one token balance or one token supply twice now fails instead of losing a write. No state transition makes such a batch: token operations come only from batch transitions, one per batch, and each writes a key at most once.Comments where the same trap waits (no behaviour change). The total system credits and address balances are written the same way, from the committed value, but no batch repeats their keys today, so
merge_balance_writesleaves them out.SystemOperationType,AddressFundsOperationTypeandmerge_balance_writesnow say so and what keeps it true.partially_use_asset_locknotes that its fee loop is only correct because repeated fee strategy steps are refused (FeeStrategyDuplicateError).validate_fees_of_event1 estimates for the payer it settles on. It used to estimate every batch with the signer paying all action fees, owner part included, before deciding whether the contract owner sponsors the gas, so for a sponsored batch the estimate described operations execution never applies. Now:gas_sponsor_paysis decided on that estimate;settle_fees_of_event_v1returnsSettledFees(the estimate for the payer and the paying sponsor),validate_fees_of_event_v1maps it to the estimate for check tx and its other callers, andexecute_event1 charges the settled payer instead of askinggas_sponsor_paysagain on the returned estimate.Example, a purchase from the contract owner, who sponsors the gas and is owed an owner part of
MAX_CREDITS - MODERATORS_PART - 1:Because the fix sits in
apply_drive_operations, it covers every protocol version 14 batch, not only action fees.execute_event1 andvalidate_fees_of_event1 are unchanged apart from a comment: the fees still travel in the transition's batch.Example, a buyer with 1 Dash buying a card priced 0.01 Dash whose purchase fee is 0.0001 (owner) + 0.001 (moderators):
The other two cases, as the new tests see them:
Generation edited in place
apply_drive_operations1 is selected only byDRIVE_VERSION_V9, which only protocol version 14 uses, and that version is unreleased, so it is amended in place. Generation 0 (protocol versions 1 to 13) is untouched. Any change at protocol version 14 still splits a devnet running 4.2.0-beta.4 from nodes running this, hence the!.Docs: the fees chapter of the book, the
action_fee_operationsandAddToPotdoc comments, and the protocol version 14 changelog comments (v14.rs,drive_versions/v9.rs).How Has This Been Tested?
batch/tests/document/action_fees.rs, each run through check tx at both levels (assert_check_tx_valid_at_all_levels, which also guards the committed state) and then executed, checking the payer's and seller's balances, both pots and the total credits:should_charge_the_buyer_both_the_price_and_the_purchase_feeshould_pay_a_seller_who_sponsors_the_gas_the_price_less_the_moderators_part_and_the_gasshould_charge_the_creator_of_a_contested_document_both_the_voting_fund_and_the_creation_feeshould_admit_a_sponsored_purchase_whose_waived_owner_part_no_signer_could_pay: owner partMAX_CREDITS - MODERATORS_PART - 1; passes check tx and executes with the owner part waived; fails check tx with an internal insufficient-balance error when the estimate is made for the signer and merged (found in review)should_charge_the_buyer_when_the_sponsor_covers_only_the_buyers_estimate: a purchase from a third-party seller owing only a moderators part, whose buyer-paid estimate is lower (the fee merges into the price); the preferred sponsor is settled one credit short of their own estimate, measured at execution with them paying. The sponsor pays nothing, fee validation returns the buyer's estimate (which the sponsor would cover), and the buyer pays price, fee and gas. If execution asks the sponsor question again on the returned estimate, it charges the passed-over sponsorshould_refuse_unpaid_a_buyer_who_cannot_fund_the_price_and_the_purchase_fee: the same fee paid by the buyer is refused unpaid with 40210 in check tx and at execution; without the pre-check the merged estimate fails check tx with an internal errorcargo test -p drive-abci --lib: 3530 passed, 0 failed, on the current head.cargo test -p drive --lib -- drive_op_batch fee_pots identity::update prefunded_specialized_balances tokens: 286 passed.cargo clippy -p drive -p drive-abci -p platform-version --all-targets -- -D warningsclean, also after merging the latest v4.2-dev;cargo fmt --all --checkclean.Breaking Changes
Consensus change at protocol version 14 only: a batch that writes one identity balance or fee pot more than once is now applied with the writes merged. No released protocol version changes.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull request (no structure change)For repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
fe9dfd1/skip-botsproceeds without the ones not yet reported/self-reviewedonce the bots are doneWhen every box is checked the
PR Hygienecheck passes and this can merge.Summary by CodeRabbit