Skip to content
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.

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

chore: staking python script #847

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

chore: staking python script #847

wants to merge 1 commit into from

Conversation

sander2
Copy link
Member

@sander2 sander2 commented Jan 12, 2023

This PR adds a python script that models the staking pallet, but with only a single vault, and 2 nominators. It illustrates a fundamental problem with our approach to slashing (which is required for nomination).

The problem occurs when a vault gets slashed twice, and in between the two slashes another nominator deposits or withdraws stake. So

  1. Alice deposits 50 stake
  2. Vault gets slashed for 30
  3. Bob deposits 20 stake
  4. Vault gets slashed for 10

What actually happens when a vault gets slashed is that the SlashedPerToken value increases. The stake of the individual nominators is only updated lazily based on this value - every time a nominator changes its stake or withdraws rewards, its stake gets updated based on its stake, SlashPerToken and SlashTally.

Let's take a closer look at the example. The state prior to this is as follows:

total_stake: 70.0
total_current_stake: 40
stake: {'alice': 50, 'bob': 20.0}
slash_tally: {'alice': 0, 'bob': 12.0}
slash_per_token: 0.6

The compute_stake function correctly returns:

staking.compute_stake(alice) = 50 - (50 * 0.6  - 0) = 20.0
staking.compute_stake(bob) = 20 - (20 * 0.6 - 12) = 20.0

Everything good so far. Now we execute step 4. The slash_stake function sets slash_per_token to 0.6 + 10 / 70 = 0.7428571428571429. The compute_stake function returns incorrect values (expected output would be 15):

staking.compute_stake(alice) = 50 - (50 * 0.7428571428571429  - 0) = 12.86
staking.compute_stake(bob) = 20 - (20 * 0.7428571428571429 - 12) = 17.14

In total, 10 stake is indeed slashed, but the slash that Alice receives is disproportionate. This is because its stake storage value is higher (50, compared to bob's 20). The system does not take into account that the effective stake of Alice is actually lower due to the previous slash.

@sander2
Copy link
Member Author

sander2 commented Jan 13, 2023

@nud3l tagging you at your request

@nud3l nud3l added this to the Capacity Model milestone Jan 17, 2023
@nud3l
Copy link
Member

nud3l commented Jan 17, 2023

Thanks for this excellent summary!

We got two options:

  1. We find a fix for the model we use for slashing.
  2. We find a suitable workaround.

Fix

I need to think a bit about that one. @daniel-savu or @gregdhill do you have any ideas? Something for us to work on once we wrap up AMM, lending, and the capacity model.

Workaround

I think there are two options/directions:

  1. We iterate through the nominators to update their stake after slashing. This requires that we introduce an upper bound on the nominators to make sure that we don't have an unbounded loop. It would ensure that the nominator stake is updated together with slashing. However, we might run into issues when multiple vaults and their nominators are slashed at the same time as I'm not sure how our on_initialize hooks behave when the block is full (something to test anyway).
  2. We add a function callable by anyone that updates the stake. We could still have an unbounded number of nominators. However, there's no way to enforce that all stake updates would fit in a single block and also, there's no way to ensure that the update stake call would be executed right after the slashing and so disproportionate slashing might still happen.

Option 1 would be the only viable workaround to me. Overall, I think it's also an OK solution to have an upper bound on nominators. Worst case, vault operators would just need to spin up multiple vaults to get more nominators.

@alexeiZamyatin
Copy link
Member

alexeiZamyatin commented Jan 19, 2023

Option 1 does not work well imho because it would require introducing a minimum delegation amount per Vault. Otherwise, if we set the bound to 10 delegators per Vault, the Vault might only get 10 * 1 DOT delegations (instead of e.g. 10 * 100k DOT).

As discussed with Sander, a better option 3 is:
Option 2 + forbid new delegations while slash_per_token > 0

Meaning, we do not allow new delegations until the previous slashing is completely processed.

We can then have the Vault operator call the function (in his own interest to unlock new delegations), Bob himself, and as a backup we have a Bot running.

On the UI, we would show Bob: "this Vault is processing a slashing event, come back a bit later". Plus, we can show Bob a "Speed up (cost: x INTR)" button which iterates off-chain and makes n calls via a batchAll (or split over multiple blocks, if necessary) to update the stake of each delegator.

Imho this is the better solution.

@nud3l
Copy link
Member

nud3l commented Jan 20, 2023

Option 3 would mean that the existing nominators can neither deposit nor withdraw stake. If the vault client is offline, that would mean stake gets locked (which is anyway sort of an issue but leaving that aside for now).

I think option 3 works well if we do it as follows:

  • When there's a slashing event for a vault, anyone can update the stake of all the vaults nominators manually (via an extrinsic).
  • Until someone has updated the stake of the nominators, nominators cannot change their stake (deposit/withdraw).
  • By allowing anyone to update the stake, nominators can also update it in case the vault client is offline.
  • Implications for the vault client: we should add an automatic stake update function that is executed after being slashed.
  • Implications for the UI: we need to gracefully handle when vaults are processing the slashing. I think we could even just batch a call that would update the stake and then do whatever action (deposit/withdraw) the nominator wanted to do.

@alexeiZamyatin
Copy link
Member

Option 3 would mean that the existing nominators can neither deposit nor withdraw stake. If the vault client is offline, that would mean stake gets locked (which is anyway sort of an issue but leaving that aside for now).

I think option 3 works well if we do it as follows:

  • When there's a slashing event for a vault, anyone can update the stake of all the vaults nominators manually (via an extrinsic).
  • Until someone has updated the stake of the nominators, nominators cannot change their stake (deposit/withdraw).
  • By allowing anyone to update the stake, nominators can also update it in case the vault client is offline.
  • Implications for the vault client: we should add an automatic stake update function that is executed after being slashed.
  • Implications for the UI: we need to gracefully handle when vaults are processing the slashing. I think we could even just batch a call that would update the stake and then do whatever action (deposit/withdraw) the nominator wanted to do.

Yes, that makes sense. The extrinsic should be callable by anyone.

This was referenced Jan 23, 2023
@sander2
Copy link
Member Author

sander2 commented Jan 25, 2023

To add to that, I think we'd need to have a process_slashes function that takes a vaultid and a vector of nominators. This way, we can make the weight dependent on the length of the vector, and in the very unlikely event that it doesn't fit into a single block, it can be split up into multiple calls. It'll require a bit of logic in the lib side though.

@nud3l
Copy link
Member

nud3l commented Jan 26, 2023

I think in full-blown weights v2, we also need an upper limit on that vector. But yes, needs some more work in the lib but I don't see a good way around that.

@gregdhill
Copy link
Member

gregdhill commented Feb 2, 2023

Also agree with the proposed solution (option 3), this should allow us to revise the staking architecture (i.e. remove slashTally, slashPerToken, etc.) since the nominator's collateral can be calculated as a share of the current total.

@nud3l nud3l removed this from the Kintsugi De-Fi Hub milestone Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants