fix: verify consumer proposals execution to prevent provider halts

[sainoe]

Description

This PR prevents consumer proposal executions to halt the provider in BeginBlock.

Proposals are now always executed in a cached context in order to either write or rollback the states, depending on the returned errors and the calling methods.

Therefore, the provider's BeginBlocker logic either writes or discards the proposals resulting states preventing the chain to halt due to invalid proposals.

The behavior of the proposal handlers has been changed in two ways: first, proposal execution is always verified, and second, successfully checked proposals are added to the pending proposals store regardless of their spawn/stop time.

Linked issues

Closes: #463, #458

Type of change

If you've checked more than one of the first three boxes, consider splitting this PR into multiple PRs!

• Fix: Changes and/or adds code behavior, specifically to fix a bug

New behavior tests

TestHandleConsumerAdditionProposal & TestHandleConsumerAdditionProposal UTs are changed to test that only valid proposals can be added to the pending proposals regardless of their timestamp.

A new test case is added to TestBeginBlockInit & TestBeginBlockCCR in order to check that invalid proposals are dropped as expected.

[MSalopek]

Is there any significant performance impact when a new cached ctx is created for every prop inside the loop?

Is there a problem with using

cacheCtx, writeCacheFunc = ctx.CacheContext()
for _, props := range propsToExecute {
    	err := k.CreateConsumerClient(cacheCtx, prop)
        if err != nil {
            continue
        }
        writeCacheFunc()
}

[sainoe]

I don't see any, good catch
EDIT: Actually there is, see @danwt's comment below

[danwt]

Re your snippet @MSalopek I'm not sure if I understand the ctx caching exactly, but isn't it a problem if you continue and then afterwards writeCacheFunc() in a later iteration? The bogus data from the iteration when you continued will still be in the cacheCtx and then it will be written.

[MSalopek]

I'm not sure how this works, but what you state might be why there was a separate cache context for each iteration.

Good catch!

[jtremback]

That would just undo all props, not just the one that failed

[MSalopek]

I think this naming scheme is clear, but I was wondering if it would be beneficial to call this something like ConsumerClientDryRun or VerifyCreateConsumerClient?

There's only 1 use case where writeCache is used and that's in BeginBlock, so maybe this should only return an error?

// VerifyCreateConsumerClient uses a cache context to create a consumer client in isolation.
// If consumer client cannot be created an error is returned.
func (k Keeper) VerifyCreateConsumerClient(ctx sdk.Context, p types.ConsumerAdditionProposal) error {
	cc, _ := ctx.CacheContext()
	return  k.CreateConsumerClient(cc, &p)
}

[sainoe]

Valid point but I prefer the ...CachedCtx naming scheme bc it's more generic so that it can be used in BeginBlockInit and HandleConsumerAdditionProposal.

[danwt]

Looks good!

[danwt]

Re your snippet @MSalopek I'm not sure if I understand the ctx caching exactly, but isn't it a problem if you continue and then afterwards writeCacheFunc() in a later iteration? The bogus data from the iteration when you continued will still be in the cacheCtx and then it will be written.

[sainoe]

That's correct @danwt we need a new cached context for each prop execution

[jtremback]

LGTM