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

validatorUnbondingCanComplete must take into account (re)bonded validators #232

Closed
Tracked by #12578
danwt opened this issue Jul 13, 2022 · 4 comments
Closed
Tracked by #12578
Assignees
Labels
scope: cosmos-sdk Integration with Cosmos SDK type: bug Issues that need priority attention -- something isn't working

Comments

@danwt
Copy link
Contributor

danwt commented Jul 13, 2022

By the time a mature VSC is received on the provider side mapping to a validator unbonding operation the validator can already be rebonded!

We must take this into account in the code. Changes will be needed at least here

https://github.com/cosmos/cosmos-sdk/blob/c783aea68fbd856c2b188b2d467a7fa5cb4df1e6/x/staking/keeper/unbonding.go#L326-L346

func (k Keeper) validatorUnbondingCanComplete(ctx sdk.Context, id uint64) (found bool, err error) {
	val, found := k.GetValidatorByUnbondingId(ctx, id)
	if !found {
		return false, nil
	}


	if !val.IsMature(ctx.BlockTime(), ctx.BlockHeight()) {
		val.UnbondingOnHold = false
		k.SetValidator(ctx, val)
	} else {
		// If unbonding is mature complete it
		val = k.UnbondingToUnbonded(ctx, val)
		if val.GetDelegatorShares().IsZero() {
			k.RemoveValidator(ctx, val.GetOperator())
		}


		k.DeleteUnbondingIndex(ctx, id)
	}


	return true, nil
}

In the above, if the validator is already rebonded k.UnbondingToUnbonded can be called triggering a panic.

I am really confirmed about the following case, in addition to what is explicit above. Consider:

  1. A validator unbonds which causes VSC 0 to go out
  2. The validator rebonds
  3. The validator unbonds which causes VSC 1 to go out
  4. Maturity for 0 comes in, and the if !val.IsMature(ctx.BlockTime(), ctx.BlockHeight()) branch is entered
  5. val.UnbondingOnHold is set false
  6. The validator unbonding for 1 completes BEFORE maturity for 1 is received

I will make a PR soon.

Found with diff testing - although, it's interesting because in the diff model I do not model explicity all the data structures. Because of that, the diff model will search the validator unbonding queue for a validator with opID == opID before doing anything, which avoids the problem. Still though, the model is able to generate the example case.

@danwt danwt self-assigned this Jul 13, 2022
@danwt danwt added the type: bug Issues that need priority attention -- something isn't working label Jul 13, 2022
@danwt danwt moved this to Todo in Replicated Security Jul 13, 2022
@danwt
Copy link
Contributor Author

danwt commented Jul 13, 2022

Any fix must take care of state cleanup, which is a bit nuanced. See also this issue

for taking care of leaks.

@danwt danwt added scope: cosmos-sdk Integration with Cosmos SDK ccv-core labels Jul 13, 2022
@danwt danwt moved this from Todo to In Progress in Replicated Security Jul 14, 2022
danwt pushed a commit that referenced this issue Jul 14, 2022
danwt pushed a commit that referenced this issue Jul 18, 2022
@danwt
Copy link
Contributor Author

danwt commented Jul 28, 2022

Was fixed by

@danwt danwt closed this as completed Jul 28, 2022
Repository owner moved this from In Progress to Done in Replicated Security Jul 28, 2022
@mpoke mpoke reopened this Aug 1, 2022
Repository owner moved this from Done to In Progress in Replicated Security Aug 1, 2022
@mpoke mpoke self-assigned this Aug 2, 2022
@mpoke mpoke moved this from In Progress to Waiting for review in Replicated Security Aug 2, 2022
@mpoke
Copy link
Contributor

mpoke commented Aug 2, 2022

Was fixed by

cosmos/cosmos-sdk@67c8163

Fixed by cosmos/cosmos-sdk#12796

@mpoke mpoke moved this from Waiting for review to Done in Replicated Security Aug 5, 2022
danwt pushed a commit that referenced this issue Aug 15, 2022
@danwt
Copy link
Contributor Author

danwt commented Aug 15, 2022

Closing for now as fixed by cosmos/cosmos-sdk#12796

@danwt danwt closed this as completed Aug 15, 2022
danwt pushed a commit that referenced this issue Aug 16, 2022
danwt pushed a commit that referenced this issue Aug 17, 2022
danwt pushed a commit that referenced this issue Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: cosmos-sdk Integration with Cosmos SDK type: bug Issues that need priority attention -- something isn't working
Projects
No open projects
Status: Done
Development

No branches or pull requests

2 participants