Drop duplicate spawn/stop proposals

[danwt]

Edit: see https://github.com/cosmos/interchain-security/issues/155#issuecomment-1180511416

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 https://github.com/cosmos/interchain-security/blob/0ae85e89e7f030c5f24e9503fe1f2e822949bdaf/x/ccv/provider/keeper/proposal.go#L120).

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.

https://github.com/cosmos/interchain-security/blob/0ae85e89e7f030c5f24e9503fe1f2e822949bdaf/x/ccv/provider/keeper/proposal.go#L126

[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 https://github.com/cosmos/interchain-security/issues/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 https://github.com/cosmos/interchain-security/issues/125 though.

[mpoke]

The fix was specified in https://github.com/cosmos/ibc/pull/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?