Repository navigation
fix(drive-abci)!: claw a storage refund back from the epochs it was priced for (PV14) - #5013
Conversation
…riced for (PV14) A refund for data removed in epoch E is priced in E and returns the data's shares of epochs E+1 onward. It is taken out of the epoch storage pools at the next epoch change, where add_distribute_storage_fee_to_epochs_operations v0 restored and subtracted it from the epoch after the then current one. The current epoch kept most of its refunded share and later epochs gave back more than theirs; the total still equalled the refund. v1 (DRIVE_ABCI_METHOD_VERSIONS_V10, protocol version 14) restores and subtracts each pending refund from the epoch after the previous block's epoch, the one every pending refund was priced in. Shares of epochs with no block between the two, and the leftovers, come out of the current epoch. The dpp helper subtract_refunds_priced_in_epoch_from_epoch_credits_collection keeps the end-of-storage-window guard. add_process_epoch_change_operations v0 passes the previous epoch index, which only v1 reads. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 57 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
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 |
|
🕓 Queued for automated review — 1st in line, estimated start in ~5 min (commit fc1854d)
|
|
Reviewed |
Issue being fixed or feature implemented
When data is removed in epoch E,
calculate_storage_fee_refund_amount_and_leftoversprices the owner's refund in epoch E: the refund is the data's share of every epoch from E+1 on. The refund is recorded as pending and taken out of the epoch storage pools at the next epoch change. There,add_distribute_storage_fee_to_epochs_operationsv0 callssubtract_refunds_from_epoch_credits_collectionwith the then current epoch, which is E+1 or later. It restores the storage fee as if the refund covered E+2 onward, subtracts per-epoch shares from E+2 on and takes only the leftovers from the current epoch.The total taken out equals the refund, so the credit sum balances, but the shares land on the wrong pools. The current epoch keeps most of its refunded share, and every later epoch gives back more than its own. When epochs were skipped, the offset grows with the number of skipped epochs.
This is an accounting correction between epoch pools. Measured on a chain that stores a document in epoch 1 and removes it in epoch 3, with the refund taken out at the change into epoch 4 (482 bytes removed, refund 12,964,399 credits, 40 epochs per era):
Before, 15,244 credits of the refund came from pools other than the ones it was priced for. After, the only difference is rounding: 1,356 credits (explained below) come from the current epoch instead of a credit each from 1,356 later epochs. With epochs 4 and 5 skipped by a halt (removed in epoch 3, taken out at the change into epoch 6), the misplaced amount was 47,840 credits before and 1,354 after.
What was done?
add_distribute_storage_fee_to_epochs_operationsv1 takes each pending refund back from the epochs it was priced for. Every refund pending at an epoch change was priced in the epoch of the block before it (epoch_info.previous_epoch_index()): a non-change block adds its refunds to the pending ones (fetch_and_add_pending_epoch_refunds_to_collection), while the first block of an epoch keeps only its own (add_delete_pending_epoch_refunds_except_specified_operations, then an insert that replaces the stored values). v1 restores the storage fee from the pricing epoch + 1 and subtracts per epoch from there.start_era >= PERPETUAL_STORAGE_ERASinrefund_storage_fee_to_epochs_map) and is taken whole from the current epoch.subtract_refunds_priced_in_epoch_from_epoch_credits_collection(checked arithmetic). A refund whose storage epoch is after its pricing epoch, which removal cannot produce, is taken whole from the current epoch instead of restored.subtract_refunds_from_epoch_credits_collectionand its v0 call are unchanged.previous_epoch_index: Option<EpochIndex>. v1 returnsCorruptedCodeExecutionif refunds are pending at an epoch change without a previous epoch (only the genesis change has none, and it returns before distributing).DRIVE_ABCI_METHOD_VERSIONS_V10(PV14 only) selects v1, amended in place. PV14 note 47 describes the change.Rounding: the refund is a sum of floored per-epoch shares, and the fee restored from it through the refunded fraction (
restore_original_removed_credits_amount) comes out slightly below the fee it was priced from. So a later epoch gives back its share or one credit less, and the current epoch the rest, under the existing leftover rule. When every share is a whole number of credits and the restored fee is exact, each pool gives back exactly its share (unit tests with a 10,000,000,000 credit fee).In-place changes to shipped generations
add_process_epoch_change_operationsv0 (selected by every protocol version) now passesepoch_info.previous_epoch_index()toadd_distribute_storage_fee_to_epochs_operations. Only v1 reads it, and v1 is selected only by PV14. Up to PV13 the dispatcher selects v0, whose signature and body are unchanged and which never sees the argument, so the operations it produces are byte-identical. The PV13 end-to-end test below pins the old per-epoch amounts.How Has This Been Tested?
End-to-end tests in
block_processing_end_events/tests.rsdrive a chain block by block throughprocess_raw_state_transitionsandprocess_block_fees_and_validate_sum_trees(sum trees verified every block). They store documents in epoch S, remove one in epoch E and cross the epoch change, then compare each pool's change with the shares the refund was priced from:Also:
Ran locally:
cargo test -p dpp --lib(4,820 passed)cargo test -p platform-versioncargo test -p drive-abci --lib(3,562 passed, before rebasing onto #5011; the fee distribution and block end tests again after)identity_and_document_tests,core_height_increaseandcomprehensive_tests(31 tests, including those that pin app hashes and those that delete documents across epoch changes)cargo clippy -p dpp -p drive-abci --all-features --all-targets -- -D warningscargo fmt --all -- --checkBreaking Changes
Consensus change at protocol version 14 (unreleased): from PV14 on, an epoch change distributes pending storage refunds over the epoch pools differently, so nodes must agree on the version. Chains at PV13 and earlier are unchanged.
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 ·
fc1854d/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.