fix(platform): resolve fee versions by registered number and price refunds at the storage epoch rate - #4703
fix(platform): resolve fee versions by registered number and price refunds at the storage epoch rate#4703DCG-Claude wants to merge 7 commits into
Conversation
The fee version registry resolved a fee_version_number by array position and as_static aborted on an unknown number. Lookup is now by number on every entry point, zero and unregistered numbers are errors, as_static is fallible, and a compile time assertion keeps the registry numbered contiguously from one. Tests pin that every schedule a platform version references is registered and agrees with the registered generation on every group the fee history serves, and that the number one storage rates stay frozen for replay. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e stored A refund is the unpaid remainder of the storage fee originally charged, and that fee was priced with the storage table active at the storage epoch. The refund now resolves the rate through the fee history at the storage epoch instead of the removal epoch. Every shipped schedule carries the same storage table under fee version number 1, so no reachable input changes; the tests pin both the boundary behaviour and the shipped-input equivalence. Also adds tests for the epoch fee-history resolution and for agreement between every platform version's schedule and its registered generation on every known cost item. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Pins the legacy empty-history path for fee version number one, the fee history requirement for any other number, storage-epoch rate resolution across a history boundary, and the calculate_fee dispatcher forwarding the platform schedule and history. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…n number Restoring PlatformState from the version 1 saving format resolved every stored fee version number with expect, so a state written by a newer build aborted the node at start. The conversion is now TryFrom and returns a CorruptedCachedState error naming the number and the epoch. The struct and its encoding are unchanged; the legacy version 0 mapping to the first generation is unchanged. Tests cover both fixture formats resolving to number 1, every registered number round-tripping through saved state, in-memory versus reloaded fee history agreeing on every known cost item, and the unknown-number rejection. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…th a mock generation Adds a mock fee-history generation under the mock-versions feature whose number lives above the test protocol version shift, so it is never a position in the shipped registry and can never collide with a persisted number. The registry consults it after the shipped entries. Lookup by carried number is factored into a helper and tested against a registry whose numbers do not line up with positions. Drive::calculate_fee is now driven through the dispatcher with a platform version carrying the mock generation: refunds across a storage-rate boundary are priced at the storage-epoch rate, and a missing fee history is rejected. Saved state round-trips the mock number and prices storage identically after reload. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-09-12T05:41:59.206Z |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change resolves fee versions by carried numbers, validates registry numbering, prices storage refunds at storage epochs, and makes saved-state restoration fallible for unknown versions. Tests cover fee-history boundaries, dispatch paths, round trips, legacy states, and invalid numbers. ChangesFee generation and refund handling
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Drive
participant LowLevelDriveOperation
participant FeeRefunds
participant FeeHistory
Drive->>LowLevelDriveOperation: calculate fee for storage removal
LowLevelDriveOperation->>FeeRefunds: consume storage removal
FeeRefunds->>FeeHistory: resolve rate at each storage epoch
FeeHistory-->>FeeRefunds: return fee generation
FeeRefunds-->>Drive: return refunds and fee result
Merge Risk: ⚪ Minimal · up to The fee-version lookup, storage-epoch refund pricing, and saved-state error paths are covered without an identified merge-blocking issue. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 79.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 68 functions across 10 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.3-dev #4703 +/- ##
============================================
+ Coverage 82.94% 84.12% +1.18%
============================================
Files 2767 2798 +31
Lines 376906 380743 +3837
============================================
+ Hits 312618 320301 +7683
+ Misses 64288 60442 -3846
🚀 New features to boost your workflow:
|
|
✅ Final review complete — no blockers (commit 078c446) · triage: critical · Phase 2 only (queue backlog) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Verified the supplied finding against the exact head and confirmed one architectural blocker: the refund algorithm changes through a shared helper without a new method-version boundary. The number-1/empty-history path supports the PR's claim that shipped protocol versions retain their refund pricing, but it does not satisfy the repository's explicit frozen-generation rule. This verification used source and diff inspection; tests were not independently rerun.
🔴 1 blocking
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 is a large, intricate cross-cutting change that directly alters consensus fee resolution and funds movement through storage-removal refund pricing, with additional persisted-state loading behavior in platform state. - Phase 1 reviewers: not run (skipped for throughput: 13 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-dpp/src/fee/fee_result/refunds.rs`:
- [BLOCKING] packages/rs-dpp/src/fee/fee_result/refunds.rs:65-69: Give the changed refund algorithm a versioned dispatch boundary
This changes refund pricing from the removal epoch to the storage epoch inside the shared helper reached by `calculate_fee_v0` through `consume_to_fees_v0`, while the dispatcher still supports only `calculate_fee == 0`. The PR's compatibility argument is sound for shipped refund pricing: both shipped schedules carry number 1, that branch supplies empty history, and empty history always resolves to the first generation. However, `book/src/contributing/coding-conventions.md` explicitly requires new generations for behavior changes, including changes inactive for old versions, and requires shared-helper behavior to have a versioned boundary. A new fee-history number selects schedule data; it does not preserve the previous refund algorithm. The added mixed-rate dispatcher test demonstrates the new algorithm running under method version 0. Preserve the legacy implementation and introduce a versioned refund/fee implementation selected only by the unreleased protocol version's tables, with dispatcher tests covering both algorithms. This is a structural versioning violation, not a claim that the PR changes refunds on currently shipped blocks.
…rsion 1 Review pointed out that changing FeeRefunds::from_storage_removal in place edits a shipped generation even though no released protocol version can observe the difference. The shipped helper, consume_to_fees_v0 and calculate_fee_v0 are restored byte-identical to the base branch. The storage-epoch rule now lives in from_storage_removal_v1, consume_to_fees_v1 and calculate_fee_v1, reachable only through calculate_fee version 1, which no drive version table selects yet. The unreleased protocol version that registers a schedule under a new fee version number flips the slot. Tests run both generations: the shipped rule is pinned to current-epoch pricing across a boundary, generation 1 prices at the storage epoch, both agree whenever every history entry is number 1, and the dispatcher covers both arms plus an unknown slot. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
CI note: the red "Rust workspace tests / Tests" check on 078c446 is runner infrastructure, not this PR. Both attempts landed on the self-hosted runner The same runner failed the 🤖 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 PR preserves the shipped generation-0 refund behavior and places the storage-epoch refund correction behind the new calculate_fee version-1 dispatch boundary. Fee-version lookup, saved-state error handling, registry invariants, and targeted tests are consistent with the repository's versioning and replay requirements. No additional actionable issues were found.
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 is a large, intricate change that directly alters consensus fee-version resolution and storage-removal refund pricing inFeeRefunds::from_storage_removalandcalculate_feegeneration dispatch, affecting funds movement and persisted-state compatibility. - 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
|
Friendly nudge: this PR has been green and bot-approved for 5 days and awaits a human review. 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
Issue being fixed or feature implemented
Part of the smart-contract plan in #4626 (task R12-01, fees workstream). The fee version registry and its consumers had four gaps that the plan's fee-version integrity work requires closed before new fee schedules can be introduced:
FeeVersion::getandFeeVersion::get_optionalresolved afee_version_numberby array position (FEE_VERSIONS[n - 1]), not by the number the entry carries. That only works while the registry happens to be numbered contiguously, and nothing enforced it.FeeVersion::as_staticcalledexpecton the lookup, and the saved-state loader (PlatformStateForSavingV1intoPlatformState) did the same, so a persisted state holding a fee version number this build does not know aborted the node at start instead of failing with a descriptive error.FeeRefunds::from_storage_removalprices every removed byte at the storage rate active at the removal epoch. A refund is the unpaid remainder of the fee originally charged, and that fee was priced at the rate active when the bytes were written, so the rate must be resolved at the storage epoch through the fee history. The corrected rule is a new generation behind thecalculate_feemethod version, not an edit to the shipped one.Refs #4675
What was done?
packages/rs-platform-version/src/version/fee/mod.rsFeeVersion::getandget_optionalresolve by matchingfee_version_numberinFEE_VERSIONS; zero and any unregistered number arePlatformVersionError::UnknownVersionError(same message text as before).FeeVersion::as_staticnow returnsResult<&'static FeeVersion, PlatformVersionError>.first()andlatest()cannot panic) and the entries are numbered 1, 2, ... in order (registry_numbers_are_contiguous_from_one, aconst fn). A mis-numbered or duplicated entry no longer compiles.FEE_VERSIONSdefining a fee-history generation and when a schedule needs a new number.find_registered, and with themock-versionsfeatureget_optionalconsultsFEE_TEST_VERSIONSafter the shipped registry.find_registeredagainst a registry whose numbers do not line up with positions (number 3 at position 0, number 1 at position 1), the mock generation resolves throughget,get_optionalandas_staticalthough no registry position corresponds to its number, zero and unregistered numbers are rejected on every entry point, the numbering guard, every schedule referenced byPLATFORM_VERSIONS(andTEST_PLATFORM_V2/TEST_PLATFORM_V3undermock-versions) is registered and agrees with the registered generation on the storage, processing, hashing and signature groups, the number 1 storage rates are frozen with a replay rationale, andas_staticon both sides.packages/rs-platform-version/src/version/mocks/fee_test.rs(new,mock-versionsfeature only)TEST_FEE_VERSION_DOUBLED_STORAGE_RATE: a fee-history generation with twice the shipped disk usage rate, numbered(1 << TEST_PROTOCOL_VERSION_SHIFT_BYTES) + 1so it can never collide with a number a released network persisted and is never a position in the shipped registry. A compile-time assertion keeps every mock number above the shift.mock-versionsis a dev-dependency feature ofdrive,drive-abciandstrategy-tests; release builds never enable it.packages/rs-dpp/src/fee/fee_result/refunds.rsFeeRefunds::from_storage_removal(generation 0) is byte-identical to the shipped body; its doc comment now states it is frozen and selected bycalculate_feeversion 0.FeeRefunds::from_storage_removal_v1: the same function with the storage rate resolved withEpoch::new(epoch_index)(the storage epoch, the key of each removal entry) instead ofEpoch::new(current_epoch_index). The dust filter, era arithmetic and error mapping are unchanged. Doc comment explains the rule and which method version selects it.packages/rs-dpp/src/fee/default_costs/mod.rs(tests only)EpochCosts::active_fee_version: empty history resolves to the first registered generation (the genesis-epoch fallback the epoch-change hook relies on), exact epoch match, nearest lower entry, before the earliest entry, and a consumer-level check that for everyPlatformVersionand everyKnownCostItemvariant the referenced schedule and the registered generation return the same cost.packages/rs-drive/src/fees/calculate_fee/v1/mod.rs(new),calculate_fee/mod.rsandpackages/rs-drive/src/fees/op.rsDrive::calculate_fee_v1: a copy ofcalculate_fee_v0that callsLowLevelDriveOperation::consume_to_fees_v1. The dispatcher gains the1 =>arm andknown_versions: vec![0, 1].calculate_fee_v0andconsume_to_fees_v0are byte-identical to the base branch.LowLevelDriveOperation::consume_to_fees_v1: a copy ofconsume_to_fees_v0whose two refund arms callfrom_storage_removal_v1. The fee version number 1 arm still prices against an empty history, so both generations agree on every shipped schedule.DRIVE_VERSION_V*table selectscalculate_fee: 1yet. EveryPLATFORM_V*keepscalculate_fee: 0, and the mock versions too. The unreleased protocol version that registers a schedule under a new fee version number setscalculate_fee: 1in its drive table; that is the explicit version boundary.op.rsrun both generations: number 1 refunds through the legacy empty-history path withNonehistory and ignores a supplied history (both generations); any other number requires the history (DriveError::CorruptedCodeExecution, both generations); with a boundary history, generation 0 prices every epoch at the current epoch's rate and generation 1 at the storage-epoch rate; system bytes land inremoved_bytes_from_system.calculate_fee/mod.rsgo through the public dispatcher with aTEST_PLATFORM_V2clone carrying the mock generation and an explicitcalculate_feeslot: generation 0 and generation 1 produce their respective pricing across a boundary, both agree when every history entry is number 1, both reject a missing history, and slot 2 isUnknownVersionMismatch.packages/rs-drive-abci/src/platform_types/platform_state/platform_state_for_saving/v1/mod.rsandplatform_state/mod.rsFrom<PlatformStateForSavingV1> for PlatformStatebecameTryFromwithError. An unknown stored number producesExecutionError::CorruptedCachedState("platform state stores fee version {n} for epoch {e}, which this build does not know"). The struct, its derive stack and its bincode encoding are unchanged. The V1 arm ofTryFromPlatformVersioned<PlatformStateForSaving>callstry_from. The V0 arm (legacy pre-1.4 format, every entry mapped to the first generation) is unchanged.fetch_platform_state_v0, the checkpoint loader and theverifysubcommand all go through this path.serialize_to_bytesandversioned_deserialize, that the mock generation's number (not a registry position) is what gets stored and comes back resolved to the mock entry with storage priced identically before and after reload, that a map built the way the epoch-change hook builds it (a reference intoPLATFORM_VERSIONS) and the reloaded map agree on everyKnownCostItemfor epochs 0 to 3, and that a V1 state carrying number 99 for epoch 3 fails to load with an error naming both.packages/rs-drive-abci/src/execution/platform_events/block_processing_end_events/tests.rsas_static()call site uses the fallible signature.book/src/fees/overview.mdfee_version_numbernames, lookup by number, unknown numbers are load errors, whyFEE_VERSION1andFEE_VERSION2share number 1, and why the empty-history fallback is deliberate.Not changed: every
FEE_VERSION*and fee-group constant, everyPLATFORM_V*and mock version,consume_to_fees_v0,EpochCosts::active_fee_version,upgrade_protocol_version_on_epoch_change_v0, bothPlatformStateForSavinglayouts, the V0 legacy mapping,SystemLimits, proto and SDK surfaces.How Has This Been Tested?
Local gate, each command's output captured to a log file and its exit code checked (macOS, Rust 1.92):
Compile-time guard verified negatively: registering
FEE_VERSION1twice failscargo check -p platform-versionwith "fee version numbers must be registered in order, starting at 1 and without gaps".No verify-only cut needed (no
src/verify/**change). Fulldrive-abcisuite and strategy tests are left to CI.Breaking Changes
None on any shipped protocol version, so no
!.The refund rule correction (rate at the storage epoch instead of the removal epoch) lives in a new generation:
FeeRefunds::from_storage_removal_v1,consume_to_fees_v1andDrive::calculate_fee_v1, selected only bycalculate_fee: 1, which noDRIVE_VERSION_V*table sets. Generation 0 is byte-identical to the base branch. Every released protocol version therefore runs exactly the code it shipped with. The tests also show the two generations agree on every input reachable today (every registered schedule shares one storage table), so switching the slot to 1 in the unreleased protocol version alongside a new fee version number is where the correction first becomes observable.In-workspace Rust API:
FeeVersion::as_staticis now fallible andPlatformStateForSavingV1converts toPlatformStatethroughTryFrom. Each had one in-tree caller, both updated. No client crate uses either.Decisions taken (provisional values)
FEE_VERSION2keepsfee_version_number: 1and is not registered separately.fee_version_numbernames a fee-history generation: the set of valuesKnownCostItemcan read (storage, processing, hashing, signature). The two schedules differ only indata_contract_registration, which the history never serves, so both resolve to the same generation. Renumbering would flip protocol versions 9 to 14 onto the map-driven refund branch ofconsume_to_fees_v0for historical blocks, and several block-lifecycle callers (withdrawal cleanup, epoch change, masternode identity updates at init chain) passNonehistory to Drive and would hitCorruptedCodeExecution. That caller audit belongs to the next task in this workstream; it is recorded here so the reviewer sees it was considered.ExecutionError::CorruptedCachedState, surfaced throughProtocolError::Genericbyversioned_deserialize, which is the existing path for load failures.expectinfirst()andlatest()has a proof.mock-versions, not in the shipped registry. The review asked for a lookup that cannot be satisfied by array position. Adding a real second generation would be a consensus change (it needs a protocol version and the caller audit). The mock generation follows the existing test protocol version pattern: its number sits aboveTEST_PROTOCOL_VERSION_SHIFT_BYTES, it is consulted after the shipped registry, and release builds never compile it.calculate_feegeneration, not an in-place edit. The first revision changedfrom_storage_removalin place on the argument that the change is unobservable on shipped versions; review pointed out the conventions freeze shipped generations regardless, so generation 0 is restored byte-identical and the corrected rule iscalculate_fee_v1/consume_to_fees_v1/from_storage_removal_v1. No version table selects it in this PR: the slot flips to 1 in the unreleased protocol version that also introduces a new fee version number, so both changes share one boundary. The registry change is a lookup repair with identical results for every currently registered input, and the saved-state change only turns an abort into an error; neither alters versioned behaviour.Findings recorded for follow-up tasks in the workstream (no code change here):
Nonefee history to Drive are safe while every registered number is 1; the tests inop.rspin that a later number requires the history on those paths.TEST_FEE_VERSION_DOUBLED_STORAGE_RATEon a mock protocol version throughreplace_test_versions; this PR covers the boundary at the Drive dispatcher and the saved-state layers.test_document_refund_after_10_epochs_on_different_fee_version_increasing_feescannot observe a boundary today because its "higher fees" version is a clone of the latest schedule (number 1).Checklist:
For repository code-owners and collaborators only
Dash-Tasks: R12-01
🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta.
🤖 Generated with Claude Code