fix: dAppStaking - Ledger contract stake count invariant#1569
fix: dAppStaking - Ledger contract stake count invariant#1569ipapandinas merged 12 commits intomasterfrom
Conversation
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
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
| @@ -1237,9 +1237,7 @@ pub mod pallet { | |||
| // 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. |
Dinonard
left a comment
There was a problem hiding this comment.
The logic for the cleanup looks good to me now.
The part that is missing though is the fix for why this error can even happen, no?
|
/bench astar,shibuya,shiden pallet_dapp_staking |
|
Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/20239643390. |
|
Benchmarks have been finished. |
pallets/dapp-staking/src/lib.rs
Outdated
|
|
||
| Self::update_ledger(&account, ledger)?; | ||
|
|
||
| if ledger_was_cleared { |
There was a problem hiding this comment.
Same question again.
There was a problem hiding this comment.
Yes true, this is not claim responsibility, it doesn't create the inconsistency itself in first place. In that case I suppose the new check on staking to properly replace an existing key is enough. I can add a test for it.
There was a problem hiding this comment.
That's correct, claim staker call should not care for it.
What needs to be done:
- clean-up call for inconsistent ledgers - ✅
- logic updates to ensure inconsistency doesn't happen - ✅ (from what I can see)
- tests that reproduce the reported issue (should fail without your changes, and pass after the
claim_stakeris fixed)
This changes shouldn't impact performance/benchmarks in any way.
This reverts commit 11cfc70.
| // Bounded by max allowed number of stake entries per account. | ||
| for (smart_contract, stake_info) in StakerInfo::<T>::iter_prefix(&account) { | ||
| let stake_period = stake_info.period_number(); | ||
|
|
||
| // Check if this entry should be kept | ||
| let should_keep = stake_period == current_period | ||
| || (stake_period >= threshold_period | ||
| && stake_period < current_period | ||
| && stake_info.is_bonus_eligible()); | ||
|
|
||
| if should_keep { | ||
| remaining = remaining.saturating_add(1); | ||
| } else { | ||
| to_be_deleted.push(smart_contract); | ||
| } | ||
| } | ||
| let entries_to_delete = to_be_deleted.len(); |
There was a problem hiding this comment.
Please add a test for this, and then it can be merged.
You shouldn't be able to create this scenario using regular calls (if you can, it means there is still a bug), so just adding values directly into the storage should do the trick.
There was a problem hiding this comment.
Good idea, the new cleanup_expired_entries_correctly_with_ledger_counter_reconciliation test covers the proper cleanup and the counter alignment.
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**