Drop duplicate spawn/stop proposals

[danwt]

Edit: see #155 (comment)

When a proposal gets handled it will be added to a pending list if spawnTime has not yet passed.

func (k Keeper) CreateConsumerChainProposal(ctx sdk.Context, p *types.CreateConsumerChainProposal) error {
	if ctx.BlockTime().After(p.SpawnTime) {
		return k.CreateConsumerClient(ctx, p.ChainId, p.InitialHeight)
	}

	k.SetPendingClientInfo(ctx, p.SpawnTime, p.ChainId, p.InitialHeight)
	return nil
}

I'm suspicious of what might happen if we allow multiple entries in pending for a given chain. I cannot think of any situation where this would be beneficial. It could only lead to strange behavior.

I think we should ensure that SetPendingClientInfo only has at most one entry per chainID and updates the existing entry if it already exists instead of creating a new entry.

[mpoke]

@danwt This should be the same behavior regardless of the outcome of the if condition currentTimestamp() > p.spawnTime. This means that this logic should be implemented in CreateConsumerClient. For now, we could just drop here all proposals that are for an existing chainID (i.e., there is already a client stored for that chainID, see

interchain-security/x/ccv/provider/keeper/proposal.go

Line 120 in 0ae85e8

func (k Keeper) SetConsumerClient(ctx sdk.Context, chainID, clientID string) {

).

This brings me to another potential issue. What happens if GetConsumerClient is called for a non-existing chainID (see here)? What does string(nil) return?

[danwt]

Nice catch re.

interchain-security/x/ccv/provider/keeper/proposal.go

Line 126 in 0ae85e8

func (k Keeper) GetConsumerClient(ctx sdk.Context, chainID string) string {

[mpoke]

Yeah, we need to have a look through all the setters and getters and make sure that they can handle edge cases. I've opened an issue #160

[mpoke]

For now, we could just drop all proposals that are for an existing chainID

Of course, this means that if for some reason a consumer chain is removed and the state is not cleaned, that chain cannot join as a consumer chain with the same chainID. This should be handled by #125 though.

[mpoke]

The fix was specified in cosmos/ibc#775

[jtremback]

For future reference, the final fix proposed in this thread is to disable creation of a duplicate client for an existing consumer chainId. We may also want to error when someone tries to create a governance proposal for new consumer chain with the same chainId, to avoid getting into this state in the first place.

[mpoke]

We may also want to error when someone tries to create a governance proposal for new consumer chain with the same chainId, to avoid getting into this state in the first place.

Is there a way to check gov proposals when they are submitted, like a handler or something?