From 34fe2613d7a8a261ef552c0f4893a8bd920106c2 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Thu, 4 Jun 2026 17:40:56 +0200 Subject: [PATCH 1/2] fix(dpp): return error instead of panicking on storage-fee refund div-by-zero `original_removed_credits_multiplier_from` computed `dec!(1) / ratio_used`. `FEE_DISTRIBUTION_TABLE` has exactly `PERPETUAL_STORAGE_ERAS` (50) entries, so once a refund's original storage epoch is at least that whole window behind the repayment epoch (`current_era >= 50`), every table era is `Less`, the iterator yields nothing, and `ratio_used` sums to zero. `rust_decimal::Decimal`'s `/` operator PANICS on a zero divisor (unlike integer/`f64` division and unlike its own `checked_div`). On the consensus path (`process_block_fees_and_validate_sum_trees` at epoch change) this would abort every node simultaneously and halt the chain. Not reachable today, but a guaranteed liveness failure as the chain ages. The function now returns `Result`: it returns `ProtocolError::DivideByZero` when the window is fully elapsed and guards the `paid_epochs` subtraction with `checked_sub` (`ProtocolError::Overflow`) so a repayment epoch before the original storage epoch can't underflow either. The sole production caller (`restore_original_removed_credits_amount`) already returns a Result and propagates with `?`. Test call sites updated to `.expect(...)`; adds tests for both error paths. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/rs-dpp/src/fee/epoch/distribution.rs | 115 +++++++++++++++--- 1 file changed, 95 insertions(+), 20 deletions(-) diff --git a/packages/rs-dpp/src/fee/epoch/distribution.rs b/packages/rs-dpp/src/fee/epoch/distribution.rs index 22a0cec9111..4564cf52c0c 100644 --- a/packages/rs-dpp/src/fee/epoch/distribution.rs +++ b/packages/rs-dpp/src/fee/epoch/distribution.rs @@ -165,8 +165,17 @@ fn original_removed_credits_multiplier_from( start_epoch_index: EpochIndex, start_repayment_from_epoch_index: EpochIndex, epochs_per_era: u16, -) -> Decimal { - let paid_epochs = start_repayment_from_epoch_index - start_epoch_index; +) -> Result { + // `start_repayment_from_epoch_index` is `current_epoch_index + 1` and + // `start_epoch_index` is the (earlier) epoch the storage was originally + // paid in, so this subtraction normally cannot underflow. Guard it anyway + // so corrupted/unexpected inputs return an error rather than panicking + // (debug) or wrapping (release) on the consensus path. + let paid_epochs = start_repayment_from_epoch_index + .checked_sub(start_epoch_index) + .ok_or(ProtocolError::Overflow( + "start repayment epoch is before the original storage epoch", + ))?; let current_era = (paid_epochs / epochs_per_era) as usize; @@ -186,7 +195,21 @@ fn original_removed_credits_multiplier_from( }) .sum(); - dec!(1) / ratio_used + // `FEE_DISTRIBUTION_TABLE` has exactly `PERPETUAL_STORAGE_ERAS` entries. + // Once the refund's original storage epoch is at least that whole window + // behind the repayment epoch (`current_era >= PERPETUAL_STORAGE_ERAS`), + // every table era compares `Ordering::Less`, the iterator yields nothing, + // and `ratio_used` sums to zero. `rust_decimal::Decimal`'s `/` operator + // PANICS on a zero divisor (unlike integer/`f64` division and unlike its + // own `checked_div`), which on the consensus path would abort every node + // simultaneously and halt the chain. Return a propagable error instead. + if ratio_used.is_zero() { + return Err(ProtocolError::DivideByZero( + "storage fee refund is older than the entire perpetual storage window", + )); + } + + Ok(dec!(1) / ratio_used) } /// Let's imagine that we are refunding something from epoch 5 @@ -203,7 +226,7 @@ fn restore_original_removed_credits_amount( start_epoch_index, start_repayment_from_epoch_index, epochs_per_era, - ); + )?; refund_amount .checked_mul(multiplier) @@ -347,7 +370,8 @@ mod tests { let epoch_0_cost = dec!(0.05000) / dec!(20.0); let multiplier_should_be = dec!(1.0) / (dec!(1.0) - epoch_0_cost); - let multiplier = original_removed_credits_multiplier_from(0, 1, 20); + let multiplier = original_removed_credits_multiplier_from(0, 1, 20) + .expect("multiplier within perpetual storage window"); assert_eq!(multiplier_should_be, multiplier); } @@ -359,7 +383,8 @@ mod tests { let multiplier_should_be = dec!(1.0) / (dec!(1.0) - epoch_0_cost); - let multiplier = original_removed_credits_multiplier_from(24, 43, 20); + let multiplier = original_removed_credits_multiplier_from(24, 43, 20) + .expect("multiplier within perpetual storage window"); assert_eq!(multiplier_should_be, multiplier); } @@ -733,7 +758,8 @@ mod tests { start_epoch_index, current_epoch_index_where_refund_occurred + 1, 20, - ); + ) + .expect("multiplier within perpetual storage window"); // it's not going to be completely perfect but it's good enough // there were 24 epochs, on average we would be 12 off @@ -927,7 +953,8 @@ mod tests { SECOND_START_EPOCH_INDEX, CURRENT_EPOCH_INDEX_WHERE_REFUND_OCCURRED + 1, 20, - ); + ) + .expect("multiplier within perpetual storage window"); // it's not going to be completely perfect but it's good enough // there were 24 epochs, on average we would be 12 off @@ -1154,15 +1181,19 @@ mod tests { fn should_create_multiplier_of_one_when_no_epochs_have_passed() { // When start_repayment == start, paid_epochs = 0, ratio_used = full table sum = 1.0 // So multiplier = 1/1 = 1 - let multiplier = original_removed_credits_multiplier_from(0, 0, 20); + let multiplier = original_removed_credits_multiplier_from(0, 0, 20) + .expect("multiplier within perpetual storage window"); assert_eq!(multiplier, dec!(1)); } #[test] fn should_increase_multiplier_as_more_epochs_pass() { - let m1 = original_removed_credits_multiplier_from(0, 5, 20); - let m2 = original_removed_credits_multiplier_from(0, 10, 20); - let m3 = original_removed_credits_multiplier_from(0, 19, 20); + let m1 = original_removed_credits_multiplier_from(0, 5, 20) + .expect("multiplier within perpetual storage window"); + let m2 = original_removed_credits_multiplier_from(0, 10, 20) + .expect("multiplier within perpetual storage window"); + let m3 = original_removed_credits_multiplier_from(0, 19, 20) + .expect("multiplier within perpetual storage window"); // More paid epochs means less ratio remaining, so multiplier increases assert!(m1 < m2); @@ -1172,9 +1203,12 @@ mod tests { #[test] fn should_handle_era_boundary_crossing() { // paid_epochs = 20 means we enter the second era exactly - let m_at_boundary = original_removed_credits_multiplier_from(0, 20, 20); - let m_before_boundary = original_removed_credits_multiplier_from(0, 19, 20); - let m_after_boundary = original_removed_credits_multiplier_from(0, 21, 20); + let m_at_boundary = original_removed_credits_multiplier_from(0, 20, 20) + .expect("multiplier within perpetual storage window"); + let m_before_boundary = original_removed_credits_multiplier_from(0, 19, 20) + .expect("multiplier within perpetual storage window"); + let m_after_boundary = original_removed_credits_multiplier_from(0, 21, 20) + .expect("multiplier within perpetual storage window"); // At the boundary, the entire first era (0.05) is consumed assert!(m_at_boundary > m_before_boundary); @@ -1184,9 +1218,11 @@ mod tests { #[test] fn should_handle_different_epochs_per_era() { // With 40 epochs per era (the default), 40 paid epochs = 1 full era - let m_40 = original_removed_credits_multiplier_from(0, 40, 40); + let m_40 = original_removed_credits_multiplier_from(0, 40, 40) + .expect("multiplier within perpetual storage window"); // With 20 epochs per era, 20 paid epochs = 1 full era - let m_20 = original_removed_credits_multiplier_from(0, 20, 20); + let m_20 = original_removed_credits_multiplier_from(0, 20, 20) + .expect("multiplier within perpetual storage window"); // Both consume exactly one full era of 0.05, so multipliers should be equal assert_eq!(m_40, m_20); @@ -1195,13 +1231,52 @@ mod tests { #[test] fn should_produce_same_multiplier_regardless_of_absolute_epoch_offset() { // The multiplier depends only on the difference, not absolute indices - let m1 = original_removed_credits_multiplier_from(0, 15, 20); - let m2 = original_removed_credits_multiplier_from(100, 115, 20); - let m3 = original_removed_credits_multiplier_from(5000, 5015, 20); + let m1 = original_removed_credits_multiplier_from(0, 15, 20) + .expect("multiplier within perpetual storage window"); + let m2 = original_removed_credits_multiplier_from(100, 115, 20) + .expect("multiplier within perpetual storage window"); + let m3 = original_removed_credits_multiplier_from(5000, 5015, 20) + .expect("multiplier within perpetual storage window"); assert_eq!(m1, m2); assert_eq!(m2, m3); } + + #[test] + fn should_return_error_instead_of_panicking_when_window_fully_elapsed() { + // PERPETUAL_STORAGE_ERAS == 50 and the distribution table has 50 + // entries. With epochs_per_era = 20, `current_era` reaches 50 (the + // full window) at paid_epochs = 50 * 20 = 1000, at which point every + // table era compares `Less`, `ratio_used` sums to zero, and the old + // code panicked on `dec!(1) / 0`. It must now return a DivideByZero + // error so the consensus path can propagate it instead of aborting. + let result = original_removed_credits_multiplier_from(0, 1000, 20); + assert!( + matches!(result, Err(ProtocolError::DivideByZero(_))), + "expected DivideByZero error once the window is fully elapsed, got {:?}", + result + ); + + // The caller propagates the error rather than panicking. + let restored = restore_original_removed_credits_amount(dec!(1_000_000), 0, 1000, 20); + assert!( + matches!(restored, Err(ProtocolError::DivideByZero(_))), + "restore_original_removed_credits_amount must propagate the error, got {:?}", + restored + ); + } + + #[test] + fn should_return_error_when_repayment_epoch_precedes_start() { + // Defensive guard: a repayment epoch before the original storage + // epoch must not underflow the `paid_epochs` subtraction. + let result = original_removed_credits_multiplier_from(10, 5, 20); + assert!( + matches!(result, Err(ProtocolError::Overflow(_))), + "expected Overflow error on underflowing epoch difference, got {:?}", + result + ); + } } mod additional_restore_original_removed_credits_amount { From b4f2b1c5cce34f1618439a9f1bcb194a0fc535b6 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Sun, 7 Jun 2026 23:06:41 +0200 Subject: [PATCH 2/2] fix(dpp): restore liveness when storage-fee refund spans the full window The panic->error change alone relocates the halt rather than preventing it: returning DivideByZero from the refund clawback propagates up the consensus path to a Tenderdash ResponseException, which CometBFT panics on, so every validator still halts at the same block. Add the actual liveness fix one frame up, in refund_storage_fee_to_epochs_map: when the distribution epoch is at or past the end of the perpetual-storage window (start_era >= PERPETUAL_STORAGE_ERAS) there are no future epoch pools to claw back from, so route the whole refund through the existing leftovers path to the current epoch instead of computing an unused 1/ratio_used multiplier that divides by zero. This is reachable because the refund amount is computed at the removal epoch but the clawback runs at the next epoch change against the then-current epoch, so a non-zero refund for data removed just before expiry can be distributed just after the window boundary is crossed. Add a regression test reproducing that decoupled-epoch path, which the existing self-consistent tests never exercised. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/rs-dpp/src/fee/epoch/distribution.rs | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/packages/rs-dpp/src/fee/epoch/distribution.rs b/packages/rs-dpp/src/fee/epoch/distribution.rs index 4564cf52c0c..3a9f72d7d82 100644 --- a/packages/rs-dpp/src/fee/epoch/distribution.rs +++ b/packages/rs-dpp/src/fee/epoch/distribution.rs @@ -310,6 +310,26 @@ where let start_era: u16 = (skip_until_epoch_index - start_epoch_index) / epochs_per_era; + // The perpetual storage window for this data ends `PERPETUAL_STORAGE_ERAS` + // eras after `start_epoch_index`. Once the distribution epoch reaches or + // passes that end (`start_era >= PERPETUAL_STORAGE_ERAS`) there are no future + // epoch pools left to claw the refund back from: the per-era loop below is + // empty, and `original_removed_credits_multiplier_from` returns + // `DivideByZero` (`ratio_used == 0`) computing a multiplier that is never + // used. Returning that error here still halts the chain — it propagates up + // the consensus path to a Tenderdash `ResponseException`. Instead treat the + // whole refund as leftovers so the caller removes it from the current + // epoch's pool, keeping the chain live. + // + // This is reachable on the consensus path: the refund amount is computed at + // the removal epoch (`FeeRefunds::from_storage_removal`) but this clawback + // runs at least one epoch later, at the next epoch change, against the + // then-current epoch. So a non-zero refund for data removed just before + // expiry can be distributed just after the window boundary is crossed. + if start_era >= PERPETUAL_STORAGE_ERAS { + return Ok(storage_fee); + } + // Let's imagine that we are refunding something from epoch 5 // We are at Epoch 12 // The refund amount is from Epoch 13 (current + 1) to Epoch 1005 (5 + 1000) @@ -737,6 +757,98 @@ mod tests { assert_eq!(total_distributed.to_unsigned(), refund_amount); } + /// Regression test for the decoupled-epoch storage-fee refund + /// divide-by-zero (chain-halt) bug. + /// + /// In production the refund AMOUNT and the clawback DISTRIBUTION are + /// computed at two different epochs: + /// * `FeeRefunds::from_storage_removal` computes the amount at the + /// removal epoch (`c_store`) and persists it keyed by the original + /// write epoch — the removal epoch is then discarded. + /// * one epoch later, at the next epoch change, + /// `add_distribute_storage_fee_to_epochs_operations_v0` consumes it + /// via `subtract_refunds_from_epoch_credits_collection` using the + /// *new* current epoch (`c_consume = c_store + 1`), which re-derives + /// the `1 / ratio_used` multiplier against a different window + /// position. + /// + /// Every other test in this module feeds the SAME epoch to both calls + /// (the self-consistent case), which is exactly why this was never + /// exercised. Here we reproduce the real wiring: data written at epoch + /// 0 is removed two epochs before its 50-era window expires — so the + /// refund is legitimately non-zero (it is the share of the single + /// remaining in-window epoch) — and then distributed one epoch later, + /// precisely as the window boundary is crossed. At that point + /// `current_era == PERPETUAL_STORAGE_ERAS`, so `ratio_used == 0`. + /// Without the `start_era >= PERPETUAL_STORAGE_ERAS` guard in + /// `refund_storage_fee_to_epochs_map`, this returns `DivideByZero` (or, + /// before that error existed, panicked) and halts every node. + #[test] + fn should_not_halt_when_refund_distributed_as_window_boundary_is_crossed() { + const EPOCHS_PER_ERA: u16 = 40; // production default + const WRITE_EPOCH: EpochIndex = 0; + + // The perpetual storage window for data written at WRITE_EPOCH spans + // epochs [0, EPOCHS_PER_ERA * PERPETUAL_STORAGE_ERAS) = [0, 2000). + let window_end_epoch = WRITE_EPOCH + EPOCHS_PER_ERA * PERPETUAL_STORAGE_ERAS; // 2000 + + // Remove the data two epochs before the window fully elapses, so the + // refund still covers exactly one in-window epoch (the final one, + // 1999) and is therefore NON-ZERO. + let removal_epoch = window_end_epoch - 2; // c_store = 1998 + + // One epoch change later the refund is distributed with the THEN + // current epoch index (the natural +1-epoch lag). This value flows + // into original_removed_credits_multiplier_from via skip_until = + // current + 1, which now spans the full window. + let distribution_epoch = removal_epoch + 1; // c_consume = 1999 + + let original_storage_fee: Credits = 10_000_000_000; + + let (refund_amount, _leftovers) = calculate_storage_fee_refund_amount_and_leftovers( + original_storage_fee, + WRITE_EPOCH, + removal_epoch, + EPOCHS_PER_ERA, + ) + .expect("refund amount computation should succeed"); + + assert!( + refund_amount > 0, + "removing data before the final in-window epoch ({}) must leave a \ + non-zero refund; got {refund_amount}", + window_end_epoch - 1, + ); + + let mut credits_per_epochs = SignedCreditsPerEpoch::default(); + + // Without the guard this returns Err(DivideByZero) (which still + // halts the chain via Tenderdash) — or panicked before that error + // existed — inside original_removed_credits_multiplier_from, because + // the multiplier is recomputed at the distribution epoch and now + // spans the entire perpetual-storage window. + subtract_refunds_from_epoch_credits_collection( + &mut credits_per_epochs, + refund_amount, + WRITE_EPOCH, + distribution_epoch, + EPOCHS_PER_ERA, + ) + .expect("refund distribution must not halt the chain at the window boundary"); + + // With the window fully elapsed there are no future epoch pools left + // to claw the refund back from, so the entire refund must come out of + // the current (distribution) epoch's pool — and nothing else should + // be touched. + let entries: Vec<(EpochIndex, SignedCredits)> = + credits_per_epochs.into_iter().collect(); + assert_eq!( + entries, + vec![(distribution_epoch, -(refund_amount as SignedCredits))], + "the full refund should be clawed back from only the current epoch", + ); + } + #[test] fn should_deduct_refunds_from_collection_start_epoch_doesnt_matter_check() { for start_epoch_index in 0..150 {