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

Failing a transaction with a profile results in never-ending balance withdraws #416

Closed
RiccardoM opened this issue May 6, 2021 · 1 comment · Fixed by #417
Closed

Failing a transaction with a profile results in never-ending balance withdraws #416

RiccardoM opened this issue May 6, 2021 · 1 comment · Fixed by #417
Assignees
Labels
kind/bug Something isn't working x/profiles Module that allows to create and manage decentralized social profiles
Milestone

Comments

@RiccardoM
Copy link
Contributor

Bug description

Inside Apollo, if you perform a transaction with a fee after you create a profile, and that transaction fails, your balance will start loosing tokens every block until there are no more funds to get.

Steps to reproduce

  1. Create a new account
  2. Send that account some tokens
  3. Create a profile for that account
  4. Perform a failing transaction with that account (eg. a send with amount greater than the overall balance) and set a fee
  5. Watch the balance continuously dropping

Expected behavior

After a transaction fails, the balance should not continue to drop.

@RiccardoM RiccardoM added the kind/bug Something isn't working label May 6, 2021
@RiccardoM RiccardoM self-assigned this May 6, 2021
@RiccardoM RiccardoM added the x/profiles Module that allows to create and manage decentralized social profiles label May 6, 2021
@RiccardoM
Copy link
Contributor Author

RiccardoM commented May 6, 2021

After a lot of investigations, and thanks to @dimiandre and @AmauryM, we found out what was causing this.

Apparently, inside Cosmos when a transaction fails the sequence of the account is incremented anyway. This results, on the next block, to make the transaction invalid as the account will have a different sequence from the one used to sign it. Being invalid, the transaction will be removed from the mempool and not be included in the next block.

In this case, when a transaction is performed by a Profile-owning account, if it's invalid it is not removed from the mempool. This results in it being constantly included in later blocks and trigger the DeductFee AnteHandler, which results in a slow depletion of account funds due to the constant payment of fees associated with a transaction that will fail. In short, this is what is happening:

  1. The transaction is processed, and fees are payed lowering the balance.
  2. The transaction fails.
  3. The transaction is included in the next block.
  4. Back to 1.

We found out this is caused by the fact that for a Profile instance the sequence is not increasing. This is due to how we currently handle SetX methods inside the Profile type:

// GetAccount returns the underlying account as an authtypes.AccountI instance
func (p *Profile) GetAccount() authtypes.AccountI {
	return p.Account.GetCachedValue().(authtypes.AccountI)
}

// SetSequence implements authtypes.AccountI
func (p *Profile) SetSequence(sequence uint64) error {
	return p.GetAccount().SetSequence(sequence)
}

As you can see, the SetSequence method relies on the GetAccount one, which returns the underlying cached account. Following, the SetSequence is called on that cached account.

The problem with this approach is that we're only updating the cached account, and not the real one. To solve this, we need to change all SetX implementations to something like the following:

// SetSequence implements authtypes.AccountI
func (p *Profile) SetSequence(sequence uint64) error {
    acc := p.GetAccount()
    err := acc.SetSequence(sequence)
    if err != nil {
        return err
    }

    accAny, err := codectypes.NewAnyWithValue(acc)
    if err != nil {
        return err
    }

    p.Account = accAny

    return nil
}

This will make sure that changes are also stored inside the underlying Any and not only it's cached value.

A test should also be added to make sure that this bug does not happen anymore.

@RiccardoM RiccardoM mentioned this issue May 6, 2021
7 tasks
@RiccardoM RiccardoM added this to the v0.16.2 milestone May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working x/profiles Module that allows to create and manage decentralized social profiles
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant