Skip to content

fix(dpp): return error instead of panicking on storage-fee refund div-by-zero #3799

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
QuantumExplorer merged 2 commits into from
Jun 7, 2026
Merged

fix(dpp): return error instead of panicking on storage-fee refund div-by-zero #3799

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
34fe261
fix(dpp): return error instead of panicking on storage-fee refund div…
QuantumExplorer Jun 4, 2026
b4f2b1c
fix(dpp): restore liveness when storage-fee refund spans the full window
QuantumExplorer Jun 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
227 changes: 207 additions & 20 deletions packages/rs-dpp/src/fee/epoch/distribution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Decimal, ProtocolError> {
// `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;

Expand All @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -287,6 +310,26 @@ where

let start_era: u16 = (skip_until_epoch_index - start_epoch_index) / epochs_per_era;

@thepastaclaw thepastaclaw Jun 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Unchecked skip_until_epoch_index - start_epoch_index bypasses (and now interacts badly with) the new defensive guards

This PR deliberately hardened original_removed_credits_multiplier_from with checked_sub (lines 174-178) so corrupted/unexpected inputs return ProtocolError::Overflow rather than panic on the consensus path. The same subtraction is performed unchecked one frame higher at line 311 while computing start_era, so if skip_until_epoch_index < start_epoch_index ever held, debug builds still panic and release builds wrap before the helper's checked subtraction can run — defeating the hardening on its only production caller.

The new liveness guard at line 329 (start_era >= PERPETUAL_STORAGE_ERAS) actually makes this slightly worse in release builds: a wrapped start_era divided by epochs_per_era will almost certainly land >= 50, so the guard returns Ok(storage_fee) and silently clawback-routes a fictitious refund from the current epoch instead of erroring. Not reachable on the production path today (the sole caller passes current_epoch_index + 1 >= start_epoch_index by construction, and FeeRefunds::from_storage_removal enforces the invariant), and the PR description explicitly defers it. Flagging it so the two paths stay symmetric and so the new guard cannot silently mask future corruption.

Suggested change
let start_era: u16 = (skip_until_epoch_index - start_epoch_index) / epochs_per_era;
let start_era: u16 = skip_until_epoch_index
.checked_sub(start_epoch_index)
.ok_or(ProtocolError::Overflow(
"skip_until_epoch_index is before the original storage epoch",
))?
/ epochs_per_era;

source: ['claude', 'codex']


// 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)
Expand Down Expand Up @@ -347,7 +390,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);
}
Expand All @@ -359,7 +403,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);
}
Expand Down Expand Up @@ -712,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 {
Expand All @@ -733,7 +870,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
Expand Down Expand Up @@ -927,7 +1065,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
Expand Down Expand Up @@ -1154,15 +1293,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);
Expand All @@ -1172,9 +1315,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);
Expand All @@ -1184,9 +1330,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);
Expand All @@ -1195,13 +1343,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 {
Expand Down
Loading