Skip to content

Fixes TotalValueLocked out of sync in nomination pools#3052

Merged
gpestana merged 18 commits intomasterfrom
gpestana/nom-pools-auto-withdraw
Feb 8, 2024
Merged

Fixes TotalValueLocked out of sync in nomination pools#3052
gpestana merged 18 commits intomasterfrom
gpestana/nom-pools-auto-withdraw

Conversation

@gpestana
Copy link
Copy Markdown
Contributor

@gpestana gpestana commented Jan 24, 2024 •
edited
Loading

The TotalLockedValue storage value in nomination pools pallet may get out of sync if the staking pallet does implicit withdrawal of unlocking chunks belonging to a bonded pool stash. This fix is based on a new method in the OnStakingUpdate traits, on_withdraw, which allows the nomination pools pallet to adjust the TotalLockedValue every time there is an implicit or explicit withdrawal from a bonded pool's stash.

This PR also adds a migration that checks and updates the on-chain TVL if it got out of sync due to the bug this PR fixes.

Changes to trait OnStakingUpdate

In order for staking to notify the nomination pools pallet that chunks where withdrew, we add a new method, on_withdraw to the OnStakingUpdate trait. The nomination pools pallet filters the withdraws that are related to bonded pool accounts and updates the TotalValueLocked accordingly.

Others

  • Adds try-state checks to the EPM/staking e2e tests
  • Adds tests for auto withdrawing in the context of nomination pools

To-do

  • check if we need a migration to fix the current TotalValueLocked (run try-runtime)
  • migrations to fix the current on-chain TVL value

✅ Kusama:

TotalValueLocked: 99.4559 kKSM
TotalValueLocked (calculated) 99.4559 kKSM

⚠️ Westend:

TotalValueLocked: 18.4060 kWND
TotalValueLocked (calculated) 18.4050 kWND

Polkadot: TVL not released yet.

Closes #3055

@gpestana gpestana added the T10-tests This PR/Issue is related to tests. label Jan 24, 2024
@gpestana gpestana self-assigned this Jan 24, 2024
@gpestana gpestana requested review from a team January 24, 2024 18:43
@gpestana gpestana marked this pull request as draft January 24, 2024 18:43
@gpestana gpestana changed the title Adds e2e tests for auto withdrawing in the context of pools Fixes TotalValueLocked out of sync in nomination pools Jan 24, 2024
@gpestana gpestana marked this pull request as ready for review January 24, 2024 23:45
@gpestana gpestana added T1-FRAME This PR/Issue is related to core FRAME, the framework. T8-polkadot This PR/Issue is related to/affects the Polkadot network. labels Jan 24, 2024
@gpestana
Copy link
Copy Markdown
Contributor Author

gpestana commented Jan 25, 2024

bot fmt

@command-bot
Copy link
Copy Markdown

command-bot bot commented Jan 25, 2024 •
edited
Loading

@gpestana https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5024240 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 21-b3c66b89-5687-4693-a834-052339423958 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link
Copy Markdown

command-bot bot commented Jan 25, 2024

@gpestana Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5024240 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5024240/artifacts/download.

rossbulat
rossbulat reviewed Jan 25, 2024
View reviewed changes
rossbulat
rossbulat reviewed Jan 25, 2024
View reviewed changes
Copy link
Copy Markdown

@rossbulat rossbulat left a comment

Choose a reason for hiding this comment

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

This warning comment for EventListeners should probably be updated to reflect the additional on_withdraw use case.

rossbulat
rossbulat reviewed Jan 25, 2024
View reviewed changes
rossbulat
rossbulat reviewed Jan 25, 2024
View reviewed changes
rossbulat
rossbulat reviewed Jan 25, 2024
View reviewed changes
rossbulat
rossbulat reviewed Jan 25, 2024
View reviewed changes
rossbulat
rossbulat reviewed Jan 25, 2024
View reviewed changes
Copy link
Copy Markdown

@rossbulat rossbulat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Neat solution of fixing the issue and removing a function on the pools side. Looks like a good approval once the migration has been determined.

kianenigma
kianenigma reviewed Jan 26, 2024
View reviewed changes
substrate/frame/nomination-pools/src/lib.rs
// Before calculating the `balance_to_unbond`, we call withdraw unbonded to ensure the
// `transferrable_balance` is correct.
let stash_killed = bonded_pool.withdraw_from_staking(num_slashing_spans)?;
let stash_killed =
Copy link
Copy Markdown
Contributor

@kianenigma kianenigma Jan 26, 2024

Choose a reason for hiding this comment

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

Can we not update the TVL here, instead of needing the on_withdraw?

Copy link
Copy Markdown
Contributor Author

@gpestana gpestana Jan 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's what the fn withdraw_from_staking was doing. However that's insufficient because staking may withdraw unlocking chunks from a bonded pool account without an explicit request from pools. If that happens, the TVL in the pool is not updated.

This is the culprit code on the staking side: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/staking/src/pallet/mod.rs#L1029-L1039.

@paritytech-cicd-pr
Copy link
Copy Markdown

paritytech-cicd-pr commented Feb 7, 2024

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5142739

@paritytech-cicd-pr
Copy link
Copy Markdown

paritytech-cicd-pr commented Feb 7, 2024

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5142738

@gpestana gpestana requested a review from rossbulat February 7, 2024 21:36
liamaharon
liamaharon approved these changes Feb 8, 2024
View reviewed changes
seadanda
seadanda approved these changes Feb 8, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@seadanda seadanda left a comment

Choose a reason for hiding this comment

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

LGTM - sorry for the grammatical tweaks 😀

@gpestana gpestana added this pull request to the merge queue Feb 8, 2024
Merged via the queue into master with commit aac07af Feb 8, 2024
@gpestana gpestana deleted the gpestana/nom-pools-auto-withdraw branch February 8, 2024 18:51
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@gpestana @seadanda
Fixes TotalValueLocked out of sync in nomination pools (paritytech#…
62e553e 
…3052)

The `TotalLockedValue` storage value in nomination pools pallet may get
out of sync if the staking pallet does implicit withdrawal of unlocking
chunks belonging to a bonded pool stash. This fix is based on a new
method in the `OnStakingUpdate` traits, `on_withdraw`, which allows the
nomination pools pallet to adjust the `TotalLockedValue` every time
there is an implicit or explicit withdrawal from a bonded pool's stash.

This PR also adds a migration that checks and updates the on-chain TVL
if it got out of sync due to the bug this PR fixes.

**Changes to `trait OnStakingUpdate`**

In order for staking to notify the nomination pools pallet that chunks
where withdrew, we add a new method, `on_withdraw` to the
`OnStakingUpdate` trait. The nomination pools pallet filters the
withdraws that are related to bonded pool accounts and updates the
`TotalValueLocked` accordingly.

**Others**
- Adds try-state checks to the EPM/staking e2e tests
- Adds tests for auto withdrawing in the context of nomination pools

**To-do**
- [x] check if we need a migration to fix the current `TotalValueLocked`
(run try-runtime)
- [x] migrations to fix the current on-chain TVL value 

 ✅ **Kusama**:
```
TotalValueLocked: 99.4559 kKSM
TotalValueLocked (calculated) 99.4559 kKSM
```
⚠️ **Westend**:
```
TotalValueLocked: 18.4060 kWND
TotalValueLocked (calculated) 18.4050 kWND
```
**Polkadot**: TVL not released yet.

Closes paritytech#3055

---------

Co-authored-by: command-bot <>
Co-authored-by: Ross Bulat <[email protected]>
Co-authored-by: Dónal Murray <[email protected]>
This was referenced Jun 5, 2024
Closed
Closed
Closed
Closed
@Aideepakchaudhary Aideepakchaudhary mentioned this pull request Oct 14, 2024
Closed
aurexav added a commit to darwinia-network/darwinia that referenced this pull request Dec 18, 2024
@aurexav
Companion of paritytech/polkadot-sdk#3052
ad06f78
@LayNath242 LayNath242 mentioned this pull request Dec 3, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seadanda seadanda seadanda approved these changes

@kianenigma kianenigma kianenigma approved these changes

@rossbulat rossbulat Awaiting requested review from rossbulat

+1 more reviewer

@liamaharon liamaharon liamaharon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gpestana gpestana

Labels

T1-FRAME This PR/Issue is related to core FRAME, the framework. T8-polkadot This PR/Issue is related to/affects the Polkadot network. T10-tests This PR/Issue is related to tests.

Projects

(Nominated) Proof of Stake
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Nomination Pools] TotalValueLocked is not properly updated when staking performs auto-withdraw

6 participants

@gpestana @paritytech-cicd-pr @kianenigma @rossbulat @seadanda @liamaharon