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

Empty amount delegation stored after unbonding/redelegation #10216

Closed
4 tasks
RiccardoM opened this issue Sep 23, 2021 · 13 comments · Fixed by #10254
Closed
4 tasks

Empty amount delegation stored after unbonding/redelegation #10216

RiccardoM opened this issue Sep 23, 2021 · 13 comments · Fixed by #10254
Assignees

Comments

@RiccardoM
Copy link
Contributor

Summary of Bug

If you unbond or redelegate all your tokens from a validator, it might happen that a delegation entry is persisted on chain with amount equals to zero.

Version

v0.44

Steps to Reproduce

  1. Delegate some tokens to a validator
  2. Unbond/redelegate those tokens to another validator.

Sometimes, it might happen that the chain stores a delegation entry with amount equals to 0 but with a non-empty shares amount. This results in the following happening:

{
  "delegation_responses": [
    {
      "delegation": {
        "delegator_address": "emoney1952nru9aj2z8cpgew29z7e0q634d7288qyzq7l",
        "validator_address": "emoneyvaloper1zxxd24h25phc744tjgtatafh05vtw6rve4xmwe",
        "shares": "0.754709466067545576"
      },
      "balance": {
        "denom": "ungm",
        "amount": "0"
      }
    },
    {
      "delegation": {
        "delegator_address": "emoney1952nru9aj2z8cpgew29z7e0q634d7288qyzq7l",
        "validator_address": "emoneyvaloper1yfydycc36gm9eftl5krcj5lfjy3w2kzlaqer98",
        "shares": "0.001230708159860757"
      },
      "balance": {
        "denom": "ungm",
        "amount": "0"
      }
    },
    {
      "delegation": {
        "delegator_address": "emoney1952nru9aj2z8cpgew29z7e0q634d7288qyzq7l",
        "validator_address": "emoneyvaloper17e8y0np4gtdhn7mv2uut3v2a06d99f4gajvrrt",
        "shares": "0.820613871096172588"
      },
      "balance": {
        "denom": "ungm",
        "amount": "0"
      }
    }
  ],
  "pagination": {
    "next_key": null,
    "total": "0"
  }
}

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@RiccardoM RiccardoM changed the title Zero amount delegation stored after unbonding/redelegation Empty amount delegation stored after unbonding/redelegation Sep 23, 2021
@alexanderbez
Copy link
Contributor

This is because the shares are non-zero. Recall, shares are an approximation.

	// remove the delegation
	if delegation.Shares.IsZero() {
		err = k.RemoveDelegation(ctx, delegation)
	} else {
		k.SetDelegation(ctx, delegation)
		// call the after delegation modification hook
		err = k.AfterDelegationModified(ctx, delegatorAddress, delegation.GetValidatorAddr())
	}

@RiccardoM
Copy link
Contributor Author

This is because the shares are non-zero. Recall, shares are an approximation.

Yeah that's what I figured out. Does it make sense to have this stored on the state though? Wouldn't it make more sense to delete the delegation entry when the amount is zero even if shares are not zero? If the amount is zero, even a slashing wouldn't affect it so what's the point on leaving it there?

@alexanderbez
Copy link
Contributor

Does it make sense to have this stored on the state though?

That's a great question! I think we can remove it if amount is zero. We just need to double check some edge cases like how distribution rewards work, e.g. do they work based on shares or amount.

Thoughts @ValarDragon

@ValarDragon
Copy link
Contributor

ValarDragon commented Sep 28, 2021

These shares should definitely be removed if amount is 0 imo. Great catch! (Also somewhat concerning for state blowup atm)

The distribution rewards should work based on shares

@ValarDragon
Copy link
Contributor

Should we make a new issue tracking clearing these at upgrade time? (Its not a blocker for anything, but good to track / maybe help lower state size)

@alexanderbez
Copy link
Contributor

I already implemented a live upgrade handler to clear these. Unless you're referring to apps that still rely on export/import, in that case we do not cover this.

@ValarDragon
Copy link
Contributor

oh perfect, I misunderstood.

@alexanderbez
Copy link
Contributor

alexanderbez commented Dec 14, 2021

Re-opened and reverted #10254, see #10750 for reason

@lucaslopezf
Copy link
Contributor

lucaslopezf commented Jul 24, 2024

Hi @RiccardoM ! I know it's been a while, we've been looking into the issue and we believe it might be resolved.

We tried the following scenarios (All scenarios were tested with unbond and redelegate):

  1. Make a delegation with an integer amount
    We made a delegation with an integer amount.
    Then, we unbonded the entire delegated amount.
    Result: The shares and the amount were consistent.

  2. Make two delegations
    We made two consecutive delegations with integer amounts.
    Then, we unbonded the entire delegated amount.
    Result: The shares and the amount were consistent.

  3. Make delegations with decimal amounts
    We made several delegations with amounts that included decimals.
    Then, we unbonded the entire amount in different parts.
    Result: The shares and the amount were consistent.

  4. Make a delegation with an integer amount and then a delegation with decimals
    We made a delegation with an integer amount.
    Then, we made an additional delegation with a decimal amount.
    We unbonded the entire delegated amount.
    Result: The shares and the amount were consistent.

  5. Make delegations with decimal amounts and then partial unbond
    We made several delegations with decimal amounts.
    We partially unbonded the delegated amount.
    Result: The shares and the amount were consistent.

  6. Make delegations with integer and decimal amounts, and then partial and total unbond
    We made several delegations with integer and decimal amounts.
    We partially unbonded the delegated amount.
    Then, we unbonded the remaining amount.
    Result: The shares and the amount were consistent.

  7. Make multiple delegations and unbonds in different orders
    We made multiple delegations and unbonds in different orders and combinations.
    Result: The shares and the amount were consistent in all cases.

In each of the scenarios, it was ensured that there were no remaining shares, meaning that every time the amount was 0, the shares were also 0. If you continue experiencing the same issue, please provide an example of the amount you are delegating and the amount you are unbonding so we can look into it more closely.

Just in case, if you want test it. Script to configure the node

#!/bin/bash

# Remove previous configuration
rm -rf ./private/.simapp

# Initialize the node
./build/simd init issuesnode --chain-id cosmoshub-4 --home ./private/.simapp

# Add keys for validator and users
./build/simd keys add validator1 --keyring-backend test --home ./private/.simapp
./build/simd keys add alice --keyring-backend test --home ./private/.simapp

# Add genesis accounts
./build/simd genesis add-genesis-account $(./build/simd keys show alice -a --keyring-backend test --home ./private/.simapp) 1000000000stake --home ./private/.simapp
./build/simd genesis add-genesis-account $(./build/simd keys show validator1 -a --keyring-backend test --home ./private/.simapp) 10000000000000stake --home ./private/.simapp

# Generate genesis transaction for the validator
./build/simd genesis gentx validator1 5000000000000stake --chain-id cosmoshub-4 --keyring-backend test --home ./private/.simapp --moniker validator1

# Collect genesis transactions
./build/simd genesis collect-gentxs --home ./private/.simapp

# Validate the genesis file
./build/simd genesis validate-genesis --home ./private/.simapp

# Print validator address for verification
echo "Validator 1 address:"
./build/simd keys show validator1 --bech val -a --keyring-backend test --home ./private/.simapp

echo "Node setup complete. You can start the node with the following command:"
echo "./build/simd start --home ./private/.simapp"

And simulate one situation

Ensure your node is already running with:
# ./build/simd start --home ./private/.simapp

# 1. Perform Initial Delegation
./build/simd tx staking delegate $(./build/simd keys show validator1 --bech val -a --keyring-backend test --home ./private/.simapp) 1000000stake --from alice --chain-id cosmoshub-4 --keyring-backend test --home ./private/.simapp

# 2. Check Initial Delegation
./build/simd query staking delegations $(./build/simd keys show alice -a --keyring-backend test --home ./private/.simapp) --home ./private/.simapp

# 3. Perform Partial Unbond (for decimal shares)
./build/simd tx staking unbond $(./build/simd keys show validator1 --bech val -a --keyring-backend test --home ./private/.simapp) 333333stake --from alice --chain-id cosmoshub-4 --keyring-backend test --home ./private/.simapp

# 4. Check Delegation After Partial Unbond
./build/simd query staking delegation $(./build/simd keys show alice -a --keyring-backend test --home ./private/.simapp) $(./build/simd keys show validator1 --bech val -a --keyring-backend test --home ./private/.simapp) --home ./private/.simapp

# 5. Check Unbonding Delegation
./build/simd query staking unbonding-delegation $(./build/simd keys show alice -a --keyring-backend test --home ./private/.simapp) $(./build/simd keys show validator1 --bech val -a --keyring-backend test --home ./private/.simapp) --home ./private/.simapp

# 6. Perform Complete Unbond
./build/simd tx staking unbond $(./build/simd keys show validator1 --bech val -a --keyring-backend test --home ./private/.simapp) 666667stake --from alice --chain-id cosmoshub-4 --keyring-backend test --home ./private/.simapp

# 7. Check Final Delegations
./build/simd query staking delegations $(./build/simd keys show alice -a --keyring-backend test --home ./private/.simapp) --home ./private/.simapp

@educlerici-zondax educlerici-zondax moved this from 📋 Backlog to 👀 Waiting / In review in Cosmos-SDK Jul 24, 2024
@educlerici-zondax
Copy link
Contributor

@RiccardoM were you able to see lucas's answer?

@educlerici-zondax
Copy link
Contributor

@RiccardoM any update with this one?

@RiccardoM
Copy link
Contributor Author

@lucaslopezf @educlerici-zondax Thanks for fixing this!

@lucaslopezf
Copy link
Contributor

We close the Issue because it is already fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants