Skip to content

Commit

Permalink
dApp Staking v3 - Fix for Incorrect (Incomplete) Expired Entry Cleanup (
Browse files Browse the repository at this point in the history 
#1137)

* Fix for incorrect add stake

* Minor adjustments

* Adjustments

* Typo & renaming

* More review adjustments

* Cycle Alignment

* dApp staking modificiations

* Fix tests

* Migration logic

* integration tests

* Changes,updated weights

* Minor changes

* Format

* Fixes

* Fix failing UTs

* Improved history cleanup

* Updates & tests

* Fixes, extend test utils

* Benchmarks part1

* Finished benchmarks

* Minor changes

* Upgrade logic reset

* Minor tests

* Additional check
  • Loading branch information
@Dinonard
Dinonard authored Jan 17, 2024
1 parent ada298f commit 9afc68a
Show file tree
Hide file tree
Showing 8 changed files with 248 additions and 146 deletions.
83 changes: 56 additions & 27 deletions pallets/dapp-staking-v3/src/benchmarking/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ mod benchmarks {
#[extrinsic_call]
claim_staker_rewards(RawOrigin::Signed(staker.clone()));

// No need to do precise check of values, but predetermiend amount of 'Reward' events is expected.
// No need to do precise check of values, but predetermined amount of 'Reward' events is expected.
let dapp_staking_events = dapp_staking_events::<T>();
assert_eq!(dapp_staking_events.len(), x as usize);
dapp_staking_events.iter().for_each(|e| {
Expand Down Expand Up @@ -561,7 +561,7 @@ mod benchmarks {
#[extrinsic_call]
claim_staker_rewards(RawOrigin::Signed(staker.clone()));

// No need to do precise check of values, but predetermiend amount of 'Reward' events is expected.
// No need to do precise check of values, but predetermined amount of 'Reward' events is expected.
let dapp_staking_events = dapp_staking_events::<T>();
assert_eq!(dapp_staking_events.len(), x as usize);
dapp_staking_events.iter().for_each(|e| {
Expand Down Expand Up @@ -662,7 +662,7 @@ mod benchmarks {
// Advance enough eras so dApp reward can be claimed.
force_advance_to_next_subperiod::<T>();

// This is a hacky part to ensure we accomodate max number of contracts.
// This is a hacky part to ensure we accommodate max number of contracts.
TierConfig::<T>::mutate(|config| {
let max_number_of_contracts: u16 = T::MaxNumberOfContracts::get().try_into().unwrap();
config.number_of_slots = max_number_of_contracts;
Expand Down Expand Up @@ -840,6 +840,12 @@ mod benchmarks {
// Register & stake contracts, just so we don't have empty stakes.
prepare_contracts_for_tier_assignment::<T>(max_number_of_contracts::<T>());

// Force advance enough periods into the future so we can ensure that history
// cleanup marker will be updated on the next period change.
let period_before_expiry_starts =
ActiveProtocolState::<T>::get().period_number() + T::RewardRetentionInPeriods::get();
force_advance_to_period::<T>(period_before_expiry_starts);

// Advance to build&earn subperiod
force_advance_to_next_subperiod::<T>();
let snapshot_state = ActiveProtocolState::<T>::get();
Expand All @@ -853,7 +859,7 @@ mod benchmarks {
);
run_to_block::<T>(ActiveProtocolState::<T>::get().next_era_start - 1);

// Some sanity checks, we should still be in the build&earn subperiod, and in the first period.
// Some sanity checks, we should still be in the build&earn subperiod, and in the same period as when snapshot was taken.
assert_eq!(
ActiveProtocolState::<T>::get().subperiod(),
Subperiod::BuildAndEarn
Expand All @@ -867,6 +873,8 @@ mod benchmarks {
DappStaking::<T>::on_finalize(new_era_start_block - 1);
System::<T>::set_block_number(new_era_start_block);

let pre_cleanup_marker = HistoryCleanupMarker::<T>::get();

#[block]
{
DappStaking::<T>::era_and_period_handler(new_era_start_block, TierAssignment::Dummy);
Expand All @@ -880,6 +888,9 @@ mod benchmarks {
ActiveProtocolState::<T>::get().period_number(),
snapshot_state.period_number() + 1,
);
assert!(
HistoryCleanupMarker::<T>::get().oldest_valid_era > pre_cleanup_marker.oldest_valid_era
);
}

#[benchmark]
Expand Down Expand Up @@ -928,7 +939,7 @@ mod benchmarks {

// Investigate why the PoV size is so large here, even after removing read of `IntegratedDApps` storage.
// Relevant file: polkadot-sdk/substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs
// UPDATE: after some investigation, it seems that PoV size benchmarks are very unprecise
// UPDATE: after some investigation, it seems that PoV size benchmarks are very imprecise
// - the worst case measured is usually very far off the actual value that is consumed on chain.
// There's an ongoing item to improve it (mentioned on roundtable meeting).
#[benchmark]
Expand Down Expand Up @@ -960,39 +971,57 @@ mod benchmarks {
// Prepare init config (protocol state, tier params & config, etc.)
initial_config::<T>();

// Advance to era just after the last era covered by the first span.
// This is sufficient to completely fill up the first span with entries for the ongoing era.
force_advance_to_era::<T>(T::EraRewardSpanLength::get());

// Advance enough periods to make cleanup feasible.
let retention_period = T::RewardRetentionInPeriods::get();
force_advance_to_period::<T>(
ActiveProtocolState::<T>::get().period_number() + retention_period + 2,
// Hack
// Manually prepare state prior to the cleanup to ensure worst case.
let cleanup_marker = CleanupMarker {
era_reward_index: 0,
dapp_tiers_index: 0,
oldest_valid_era: T::EraRewardSpanLength::get().into(),
};
HistoryCleanupMarker::<T>::put(cleanup_marker);

// Prepare completely filled up reward span and insert it into storage.
let mut reward_span = EraRewardSpan::<_>::new();
(0..T::EraRewardSpanLength::get()).for_each(|era| {
assert_ok!(reward_span.push(
era as EraNumber,
EraReward {
staker_reward_pool: 1_000_000_000_000,
staked: 1_000_000_000_000,
dapp_reward_pool: 1_000_000_000_000,
},
));
});
EraRewards::<T>::insert(&cleanup_marker.era_reward_index, reward_span);

// Prepare completely filled up tier rewards and insert it into storage.
DAppTiers::<T>::insert(
&cleanup_marker.dapp_tiers_index,
DAppTierRewardsFor::<T> {
dapps: (0..T::MaxNumberOfContracts::get())
.map(|dapp_id| (dapp_id as DAppId, 0))
.collect::<BTreeMap<DAppId, TierId>>()
.try_into()
.expect("Using `MaxNumberOfContracts` as length; QED."),
rewards: vec![1_000_000_000_000; T::NumberOfTiers::get() as usize]
.try_into()
.expect("Using `NumberOfTiers` as length; QED."),
period: 1,
},
);

let first_era_span_index = 0;
assert!(
EraRewards::<T>::contains_key(first_era_span_index),
"Sanity check - era reward span entry must exist."
);
let first_period = 1;
assert!(
PeriodEnd::<T>::contains_key(first_period),
"Sanity check - period end info must exist."
);
let block_number = System::<T>::block_number();

#[block]
{
DappStaking::<T>::on_idle(block_number, Weight::MAX);
}

assert!(
!EraRewards::<T>::contains_key(first_era_span_index),
"Entry should have been cleaned up."
!EraRewards::<T>::contains_key(cleanup_marker.era_reward_index),
"Reward span should have been cleaned up."
);
assert!(
!PeriodEnd::<T>::contains_key(first_period),
!DAppTiers::<T>::contains_key(cleanup_marker.dapp_tiers_index),
"Period end info should have been cleaned up."
);
}
Expand Down
33 changes: 5 additions & 28 deletions pallets/dapp-staking-v3/src/benchmarking/utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,29 +77,6 @@ pub(crate) fn force_advance_to_next_era<T: Config>() {
run_for_blocks::<T>(One::one());
}

/// Advance blocks until the specified period has been reached.
///
/// Function has no effect if period is already passed.
pub(super) fn _advance_to_period<T: Config>(period: PeriodNumber) {
assert!(period >= ActiveProtocolState::<T>::get().period_number());
while ActiveProtocolState::<T>::get().period_number() < period {
run_for_blocks::<T>(One::one());
}
}

/// Advance to the specified period, using the `force` approach.
pub(super) fn force_advance_to_period<T: Config>(period: PeriodNumber) {
assert!(period >= ActiveProtocolState::<T>::get().period_number());
while ActiveProtocolState::<T>::get().period_number() < period {
force_advance_to_next_subperiod::<T>();
}
}

/// Advance blocks until next period has been reached.
pub(super) fn _advance_to_next_period<T: Config>() {
_advance_to_period::<T>(ActiveProtocolState::<T>::get().period_number() + 1);
}

/// Advance blocks until next period has been reached.
///
/// Relies on the `force` approach to advance one subperiod per block.
Expand All @@ -114,11 +91,11 @@ pub(super) fn force_advance_to_next_period<T: Config>() {
}
}

/// Advance blocks until next period type has been reached.
pub(super) fn _advance_to_next_subperiod<T: Config>() {
let subperiod = ActiveProtocolState::<T>::get().subperiod();
while ActiveProtocolState::<T>::get().subperiod() == subperiod {
run_for_blocks::<T>(One::one());
/// Advance to the specified period, using the `force` approach.
pub(super) fn force_advance_to_period<T: Config>(period: PeriodNumber) {
assert!(period >= ActiveProtocolState::<T>::get().period_number());
while ActiveProtocolState::<T>::get().period_number() < period {
force_advance_to_next_subperiod::<T>();
}
}

Expand Down
109 changes: 65 additions & 44 deletions pallets/dapp-staking-v3/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1904,6 +1904,10 @@ pub mod pallet {
TierConfig::<T>::get().calculate_new(average_price, &tier_params);
TierConfig::<T>::put(new_tier_config);

// Update historical cleanup marker.
// Must be called with the new period number.
Self::update_cleanup_marker(protocol_state.period_number());

consumed_weight.saturating_accrue(
T::WeightInfo::on_initialize_build_and_earn_to_voting(),
);
Expand Down Expand Up @@ -1963,77 +1967,94 @@ pub mod pallet {
T::Observers::block_before_new_era(next_era)
}

/// Attempt to cleanup some expired entries, if enough remaining weight & applicable entries exist.
/// Updates the cleanup marker with the new oldest valid era if possible.
///
/// Returns consumed weight.
fn expired_entry_cleanup(remaining_weight: &Weight) -> Weight {
// Need to be able to process full pass
if remaining_weight.any_lt(T::WeightInfo::on_idle_cleanup()) {
return Weight::zero();
}

// Get the cleanup marker
let mut cleanup_marker = HistoryCleanupMarker::<T>::get();

// Whitelisted storage, no need to account for the read.
let protocol_state = ActiveProtocolState::<T>::get();
let latest_expired_period = match protocol_state
.period_number()
/// It's possible that the call will be a no-op since we haven't advanced enough periods yet.
fn update_cleanup_marker(new_period_number: PeriodNumber) {
// 1. Find out the latest expired period; rewards can no longer be claimed for it or any older period.
let latest_expired_period = match new_period_number
.checked_sub(T::RewardRetentionInPeriods::get().saturating_add(1))
{
Some(latest_expired_period) => latest_expired_period,
None => {
// Protocol hasn't advanced enough to have any expired entries.
return T::WeightInfo::on_idle_cleanup();
}
Some(period) if !period.is_zero() => period,
// Haven't advanced enough periods to have any expired entries.
_ => return,
};

// Get the oldest valid era - any era before it is safe to be cleaned up.
let oldest_valid_era = match PeriodEnd::<T>::get(latest_expired_period) {
// 2. Find the oldest valid era for which rewards can still be claimed.
// Technically, this will be `Voting` subperiod era but it doesn't matter.
//
// Also, remove the expired `PeriodEnd` entry since it's no longer needed.
let oldest_valid_era = match PeriodEnd::<T>::take(latest_expired_period) {
Some(period_end_info) => period_end_info.final_era.saturating_add(1),
None => {
// Can happen if it's period 0 or if the entry has already been cleaned up.
return T::WeightInfo::on_idle_cleanup();
// Should never happen but nothing we can do if it does.
log::error!(
target: LOG_TARGET,
"No `PeriodEnd` entry for the expired period: {}",
latest_expired_period
);
return;
}
};

// Attempt to cleanup one expired `EraRewards` entry.
if let Some(era_reward) = EraRewards::<T>::get(cleanup_marker.era_reward_index) {
// If oldest valid era comes AFTER this span, it's safe to delete it.
if era_reward.last_era() < oldest_valid_era {
EraRewards::<T>::remove(cleanup_marker.era_reward_index);
// 3. Update the cleanup marker with the new oldest valid era.
HistoryCleanupMarker::<T>::mutate(|marker| {
marker.oldest_valid_era = oldest_valid_era;
});
}

/// Attempt to cleanup some expired entries, if enough remaining weight & applicable entries exist.
///
/// Returns consumed weight.
fn expired_entry_cleanup(remaining_weight: &Weight) -> Weight {
// Need to be able to process one full pass
if remaining_weight.any_lt(T::WeightInfo::on_idle_cleanup()) {
return Weight::zero();
}

// Get the cleanup marker and ensure we have pending cleanups.
let mut cleanup_marker = HistoryCleanupMarker::<T>::get();
if !cleanup_marker.has_pending_cleanups() {
return T::DbWeight::get().reads(1);
}

// 1. Attempt to cleanup one expired `EraRewards` entry.
if cleanup_marker.era_reward_index < cleanup_marker.oldest_valid_era {
if let Some(era_reward) = EraRewards::<T>::get(cleanup_marker.era_reward_index) {
// If oldest valid era comes AFTER this span, it's safe to delete it.
if era_reward.last_era() < cleanup_marker.oldest_valid_era {
EraRewards::<T>::remove(cleanup_marker.era_reward_index);
cleanup_marker
.era_reward_index
.saturating_accrue(T::EraRewardSpanLength::get());
}
} else {
// Can happen if the entry is part of history before dApp staking v3
log::warn!(
target: LOG_TARGET,
"Era rewards span for era {} is missing, but cleanup marker is set.",
cleanup_marker.era_reward_index
);
cleanup_marker
.era_reward_index
.saturating_accrue(T::EraRewardSpanLength::get());
}
} else {
// Should never happen, but if it does, log an error and move on.
log::error!(
target: LOG_TARGET,
"Era rewards span for era {} is missing, but cleanup marker is set.",
cleanup_marker.era_reward_index
);
}

// Attempt to cleanup one expired `DAppTiers` entry.
if cleanup_marker.dapp_tiers_index < oldest_valid_era {
// 2. Attempt to cleanup one expired `DAppTiers` entry.
if cleanup_marker.dapp_tiers_index < cleanup_marker.oldest_valid_era {
DAppTiers::<T>::remove(cleanup_marker.dapp_tiers_index);
cleanup_marker.dapp_tiers_index.saturating_inc();
}

// One extra grace period before we cleanup period end info.
// This so we can always read the `final_era` of that period.
if let Some(period_end_cleanup) = latest_expired_period.checked_sub(1) {
PeriodEnd::<T>::remove(period_end_cleanup);
}

// Store the updated cleanup marker
HistoryCleanupMarker::<T>::put(cleanup_marker);

// We could try & cleanup more entries, but since it's not a critical operation and can happen whenever,
// we opt for the simpler solution where only 1 entry per block is cleaned up.
// It can be changed though.

// It could end up being less than this weight, but this won't occur often enough to be important.
T::WeightInfo::on_idle_cleanup()
}
}
Expand Down
10 changes: 10 additions & 0 deletions pallets/dapp-staking-v3/src/migrations.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,3 +200,13 @@ impl<T: Config> OnRuntimeUpgrade for DappStakingV3TierRewardAsTree<T> {
T::DbWeight::get().reads_writes(counter, counter)
}
}

/// We just set it to default value (all zeros) and let the pallet itself do the history cleanup.
/// Only needed for Shibuya, can be removed later.
pub struct DappStakingV3HistoryCleanupMarkerReset<T>(PhantomData<T>);
impl<T: Config> OnRuntimeUpgrade for DappStakingV3HistoryCleanupMarkerReset<T> {
fn on_runtime_upgrade() -> Weight {
HistoryCleanupMarker::<T>::put(CleanupMarker::default());
T::DbWeight::get().writes(1)
}
}
Loading

0 comments on commit 9afc68a

Please sign in to comment.