Repository navigation
refactor(platform): price storage refunds from the block's fee schedule and drop the fee history from Drive - #5798
Open
QuantumExplorer wants to merge 1 commit into
Open
QuantumExplorer wants to merge 1 commit into
QuantumExplorer wants to merge 1 commit into
Conversation
…le and drop the fee history from Drive Drive::calculate_fee and everything above it took previous_fee_versions, a map from epoch to fee schedule. Its only reader was the refund branch of consume_to_fees_v0, which ignored it for fee_version_number 1, the number of every shipped schedule. Every shipped schedule also has the same storage rate, so no block on any network reads the map. FeeRefunds::from_storage_removal now takes the block's FeeVersion and the parameter is gone from 45 functions and about 860 calls, along with KnownCostItem and EpochCosts, which only read through the map. Platform state still records and saves the map in the same format. No fee or refund changes at any protocol version. should_price_a_refund_the_same_at_every_protocol_version checks that, and should_keep_one_storage_rate_on_every_protocol_version fails on a schedule that changes the rate, since refunds would then need the rate of the epoch that stored the bytes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Important Review skippedToo many files! This PR contains 336 files, which is 236 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files.
 |
Contributor
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-11-07T07:29:25.984Z |
Contributor
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
Collaborator
|
✅ Final review complete — no blockers (commit 3e41c7f) · triage: critical |
thepastaclaw
reviewed
Nov 7, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
At head 3e41c7f, the refund-pricing refactor preserves results for all configured protocol versions because their storage rates match. One non-blocking regression remains: the public refund helper loses its current-epoch range validation. This was a static review; current CI reports passing code checks, including Rust workspace and wasm32 tests, with PR Hygiene and automated review still pending.
🟡 1 suggestion(s)
1 finding(s) not shown inline (GitHub refused the PR diff as too large)
🟡 Suggestion: Preserve epoch validation in the public refund helper
packages/rs-dpp/src/fee/fee_result/refunds.rs:60-62
Removing Epoch::new(current_epoch_index)? also removes the range validation previously performed by this public, fallible API. EpochIndex is a u16 alias, so callers can supply values above MAX_EPOCH. For example, refunding 100 bytes stored in epoch 1 with current_epoch_index = u16::MAX, epochs_per_era = 20, and the first fee schedule previously returned ProtocolError::Overflow. It now reaches current_epoch_index + 1 in calculate_storage_fee_refund_amount_and_leftovers, which panics with overflow checks enabled or wraps to zero and produces an incorrect positive refund without them. The existing Drive execution path supplies a validated epoch, so this is not a current consensus blocker. Retain the epoch validation independently of the removed history lookup and add a regression test asserting that an invalid current epoch returns an error.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The cross-cutting refactor directly changes funds-refund pricing in FeeRefunds::from_storage_removal and its consensus-execution callers, requiring verification that historical protocol versions retain identical monetary results. - 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 quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— tenderdash-consensus-security (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-dpp/src/fee/fee_result/refunds.rs`:
- [SUGGESTION] packages/rs-dpp/src/fee/fee_result/refunds.rs:60-62: Preserve epoch validation in the public refund helper
Removing `Epoch::new(current_epoch_index)?` also removes the range validation previously performed by this public, fallible API. `EpochIndex` is a `u16` alias, so callers can supply values above `MAX_EPOCH`. For example, refunding 100 bytes stored in epoch 1 with `current_epoch_index = u16::MAX`, `epochs_per_era = 20`, and the first fee schedule previously returned `ProtocolError::Overflow`. It now reaches `current_epoch_index + 1` in `calculate_storage_fee_refund_amount_and_leftovers`, which panics with overflow checks enabled or wraps to zero and produces an incorrect positive refund without them. The existing Drive execution path supplies a validated epoch, so this is not a current consensus blocker. Retain the epoch validation independently of the removed history lookup and add a regression test asserting that an invalid current epoch returns an error.
Contributor
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
This branch has not been deployed
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
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.
Basic explanation
What this does: when stored data is deleted, its owner gets part of the storage fee back. To work out that refund, the code passes around a map saying which fee schedule each past epoch used. No block on any network reads that map, because every fee schedule has the same number and the same storage price, and the refund code skips the map for that number. This PR removes the map from the fee code. Refunds now read the storage price from the current block's fee schedule.
Value: about 1,200 fewer lines, and one less argument on 45 functions and about 860 calls. A caller can no longer forget the map or pass the wrong one, so there is nothing left for a "fee history is required" rule to check. The saved map also can't make nodes disagree any more (#4647), since block execution no longer reads it.
Risks: Low. No fee or refund changes at any protocol version, for the reason given under "In-place changes to shipped generations" below, and a new test checks this at every protocol version. Two things to watch:
Issue being fixed or feature implemented
Drive::calculate_feeand everything above it tookprevious_fee_versions, a map from epoch to fee schedule that platform state keeps. Its only reader was the refund branch ofLowLevelDriveOperation::consume_to_fees_v0, and that branch never read it in production:FEE_VERSION1toFEE_VERSION4) hasfee_version_number: 1;consume_to_fees_v0priced refunds against an empty map instead of the one it was given;FEE_STORAGE_VERSION1, 27,000 credits per byte).So the argument was threaded through 45 functions without affecting any result.
What was done?
Refund pricing.
FeeRefunds::from_storage_removaltakes the block's&FeeVersioninstead of the map and readsstorage.storage_disk_usage_credit_per_bytefrom it.consume_to_fees_v0loses itsfee_version_number == 1branch.Before:
After:
For 1,000 bytes stored in epoch 12 and removed in epoch 15, the refund is the same amount before and after, at every protocol version.
The parameter. Removed from
Drive::calculate_fee,apply_drive_operations,update_contract, the document, identity and token balance methods that forwarded it, andexecute_event,validate_fees_of_eventandprocess_validation_resultin drive-abci.check_tx,process_raw_state_transitions,process_block_fees_and_validate_sum_treesandupdate_masternode_identitiesstop reading it from platform state.Before:
After:
The read helpers.
dpp::fee::default_costs::{KnownCostItem, EpochCosts}existed only to look prices up through the map, and after this change only tests used them, so they are gone. Those tests read the rate fromFeeVersion::first(), the schedule their one-entry maps resolved to.Platform state is unchanged. It still records the map at each protocol upgrade and saves it in the same format, so saved states load as before. It is now history that nothing in block execution reads, and it is what a future storage price change could build on.
Book. Signatures and code samples updated in
drive/cost-tracking.md,drive/batch-operations.md,drive/finalize-tasks.md,state-transitions/drive-operations.md,versioning/versioned-dispatch.mdanddata-model/key-limits.md.fees/overview.mdandcontributing/coding-conventions.mdnow state the refund pricing rule, and the conventions chapter no longer lists this parameter as the exception to "platform_versionis the last parameter".A renamed test.
test_document_refund_after_10_epochs_on_different_fee_version_increasing_feesused an unchanged copy of the latest version, so it never had different fees. Its map entry is gone and it is nowtest_document_refund_after_10_epochs, with the same assertions.If the storage price ever changes
A refund is priced at the rate of the block that removes the bytes, while the owner paid the rate of the epoch that stored them. That is correct only while the rate never changes, which is true today.
should_keep_one_storage_rate_on_every_protocol_versionfails on a schedule that changes the rate, so that change has to come with a refund lookup by storing epoch. This is not a new limit: the removed code also looked up the removal epoch, not the storing epoch.Open PRs affected
calculate_feev1 and its price-before-commit generations have no purpose after this. Its recorded-owner refund credits are unaffected but will conflict.KnownCostItem, which this removes. Mechanical conflicts only.How Has This Been Tested?
New tests:
should_price_a_refund_the_same_at_every_protocol_version(rs-drive,fees/calculate_fee): runs a removal with owner and system bytes throughDrive::calculate_feeat every entry ofPLATFORM_VERSIONS, and compares it with the amount computed fromFeeVersion::first(), the schedule the old code always used.should_keep_one_storage_rate_on_every_protocol_version(rs-platform-version,fee/mod.rs): fails if any protocol version changes the storage rate.should_price_refunds_at_the_storage_rate_of_the_given_schedule(rs-dpp,fee_result/refunds.rs): a doubled-rate schedule gives the refund for the doubled rate.Local checks, all passing:
cargo fmt --all,cargo clippy -p platform-version -p dpp -p drive -p drive-abci --all-targets -- -D warnings,cargo check --workspace --all-targetsandcargo check -p drive --no-default-features --features verify.platform-versionfee (6),dppfee (71),drivefees, document insert, update and delete, and moderation (307),drive-abciblock end events, fee validation, event execution, masternode identity updates, shielded identity creation and protocol upgrade (127).The
driveintegration tests and the strategy tests were only compiled locally, so CI is their first run.Existing tests with hard-coded fee and refund amounts (document insert, update and delete, the refund and clawback tests in
block_processing_end_events) were changed only to drop the argument, so any changed amount would fail them.In-place changes to shipped generations
All edits are in place and none changes an output:
LowLevelDriveOperation::consume_to_fees_v0,Drive::calculate_fee_v0, andFeeRefunds::from_storage_removal(unversioned)FeeVersion::first()'s rate because every shipped schedule has number 1. The new code reads the block schedule's rate. All four schedules useFEE_STORAGE_VERSION1, so it is the same number. Theelsebranch and itsCorruptedCodeExecutionneeded a number other than 1, so they were unreachable.apply_drive_operationsv0, v1, v2;update_contractv0, v1, v2add_document_for_contract,update_document_for_contract,update_document_for_contract_id,update_document_with_serialization_for_contract,delete_document_for_contract,delete_document_for_contract_id,delete_index_only_document_for_contract,remove_from_identity_balance,update_identity_revision,remove_from_identity_token_balance,add_to_identity_token_balanceexecute_eventv0, v1;validate_fees_of_eventv0, v1;process_validation_resultv0, v1;update_masternode_identitiesv0, v1;process_block_fees_and_validate_sum_treesv0;process_raw_state_transitionsv0;check_txv0Every other edited generation only stops passing the argument at a call site.
from_storage_removalalso losesEpoch::new(current_epoch_index)?. That can only fail aboveMAX_EPOCH(65,279), and the index it receives comes from an existingEpoch.Breaking Changes
None for the protocol, the saved platform state or any client. Rust API only: the signatures above change, and
dpp::fee::default_costs::{KnownCostItem, EpochCosts}are removed. No other crate in this workspace uses them.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
3e41c7f/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.