fix: dAppStaking - Ledger contract stake count invariant #1569
Conversation
ipapandinas
added
shiden
Dinonard
left a comment
Dinonard
left a comment
There was a problem hiding this comment.
This won't work.
The approach with the cleanup during reward claiming is hacky and very inefficient. Also, what if user locks himself out from being able to stake anything - they won't be able to claim any rewards, and thus will remain locked out.
The fix should address claim calls where ledger isn't updated when contract stake entry is removed - or just identify if contract take entry should have been removed but wasn't.
The cleanup extrinsic can also be modified as suggested in the comments. Ideally, we can just use it to do the cleanup.
pallets/dapp-staking/src/lib.rs
Outdated
|
|
||
| // Remove expired stake entries from the ledger. | ||
| let mut ledger = Ledger::<T>::get(&account); | ||
| ledger |
There was a problem hiding this comment.
Perhaps you should change this to set the stake account to the number of entries that haven't been cleaned up. This should implicitly help cleanup the current mess.
pallets/dapp-staking/src/lib.rs
Outdated
|
|
||
| // Find all entries which are from past periods & don't have claimable bonus rewards. | ||
| // This is bounded by max allowed number of stake entries per account. | ||
| let to_be_deleted: Vec<T::SmartContract> = StakerInfo::<T>::iter_prefix(&account) |
There was a problem hiding this comment.
Please remind me why doesn't this function properly cleanup faulty entries?
If it does, please check my next comment.
pallets/dapp-staking/src/lib.rs
Outdated
| T::StakingRewardHandler::payout_reward(&account, reward_sum) | ||
| .map_err(|_| Error::<T>::RewardPayoutFailed)?; | ||
|
|
||
| let stale_contracts: Vec<T::SmartContract> = StakerInfo::<T>::iter_prefix(&account) |
There was a problem hiding this comment.
Why should reward claiming extrinsic be responsible for cleaning up everything?
|
@Dinonard I misunderstood the issue, now it should be properly fixed using the cleanup extrinsic as you suggested. |
Minimum allowed line rate is |
Pull Request Summary
When a staker claims rewards for a finished period, the ledger's
claim_up_to_erafunction zeros out the staked and staked_future fields once the period end is reached. However, the corresponding StakerInfo entries remain in storage and the ledger's contract_stake_count is never decremented. The next time the user restakes on the same contract (or when the cleanup extrinsic tries to reconcile counts), the pallet treats it as a new entry and bumps contract_stake_count again. Over time, the counter grows out of sync with the actual number of StakerInfo entries and may eventually block further stakes because the ledger appears to have reachedMaxNumberOfStakedContracts, even though storage contains far fewer entries.This PR fixes this bug by syncing the contract_stake_count to the remaining valid entries during a
cleanup_expired_entries. A new test scenario ensures the expected behavior.Check list**