feat!: Permissionless ICS

[mpoke]

Description

Closes: #2108

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• Included the correct type prefix in the PR title
• Added ! to the type prefix if the change is state-machine breaking
• Confirmed this PR does not introduce changes requiring state migrations, OR migration code has been added to consumer and/or provider modules
• Targeted the correct branch (see PR Targeting)
• Provided a link to the relevant issue or specification
• Followed the guidelines for building SDK modules
• Included the necessary unit and integration tests
• Added a changelog entry to CHANGELOG.md
• Included comments for documenting Go code
• Updated the relevant documentation or specification
• Reviewed "Files changed" and left comments if necessary
• Confirmed all CI checks have passed
• If this PR is library API breaking, bump the go.mod version string of the repo, and follow through on a new major release

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! the type prefix if the change is state-machine breaking
• confirmed this PR does not introduce changes requiring state migrations, OR confirmed migration code has been added to consumer and/or provider modules
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage

[mpoke]

@bermuell This entire file needs to be removed. This will affect the e2e tests though.

[insumity]

As discussed with @mpoke. I did not review but accepting (✅) to unblock cutting. I'll review at a later stage.

[MSalopek]

Admin approval.
Review will follow.

[MSalopek]

Make a comment that this is just a string. It can be a JSON string etc. The chain shouldnot care if it's a URL or not.

[p-offtermatt]

Also outdated here:

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

Lines 14 to 18 in 5b6252c

	// GetTemplateClient returns the template client for provider proposals
	func (k Keeper) GetTemplateClient(ctx sdk.Context) *ibctmtypes.ClientState {
	params := k.GetParams(ctx)
	return params.TemplateClient
	}

[mpoke]

@p-offtermatt I addressed comments until here in #2237

[p-offtermatt]

Note: Already discussed with @insumity about this one

There is a bug around the spawntime in UpdateConsumer; essentially it's possible fo a user to set a consumer chain that will attempt to launch in every block.

Assume we have a Registered consumer chain with a SpawnTime.
Now, a user sends a MsgUpdateConsumer with InitializationParameters where SpawnTime==0.
(This is not validated against in ValidateBasic

interchain-security/x/ccv/provider/types/msg.go

Line 524 in 5b6252c

func ValidateInitializationParameters(initializationParameters ConsumerInitializationParameters) error {

)

The new InitializationParameters get set here:

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

Line 507 in 5b6252c

if err = k.Keeper.SetConsumerInitializationParameters(ctx, msg.ConsumerId, *msg.InitializationParameters); err != nil {

However, the InitializeConsumer call here

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

Line 567 in 5b6252c

if spawnTime, initialized := k.Keeper.InitializeConsumer(ctx, consumerId); initialized {

will fail because the SpawnTime is zero, see this check

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

Line 54 in 5b6252c

if initializationParameters.SpawnTime.IsZero() {

Thus, the PrepareConsumerForLaunch call here

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

Line 568 in 5b6252c

if err := k.Keeper.PrepareConsumerForLaunch(ctx, consumerId, previousSpawnTime, spawnTime); err != nil {

does not update the SpawnTime in the ConsumerToBeLaunched store.

So the InitializationParameters will say SpawnTime==0, but the store will have SpawnTime=oldSpawnTime.

During BeginBlock, when the consumers are launched, this RemoveConsumerToBeLaunched call here will thus not remove the consumer

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

Line 76 in 5b6252c

err = k.RemoveConsumerToBeLaunched(ctx, consumerId, initializationParameters.SpawnTime)

and error, which causes the loop to continue.

In the extreme, someone can create 200 such consumers, and those will be re-tried to be launched in every block and no other consumers will ever launch

[p-offtermatt]

Another find:
The owner can update launched consumer chains; in particular the owner can arbitrarily modify the InitializationParameters of the already launched chain. afaict this doesn't have any further impact, but it means the InitializationParameters returned by QueryConsumerChain can be arbitrary, and it would look like the chain launched with different parameters than it actually launched with (i.e. could make it look like the chain had a very large ConsumerRedistributionFraction)

I think a fix would be to just not let the InitializationParameters of launched consumer chains be modified

[mpoke]

So the InitializationParameters will say SpawnTime==0, but the store will have SpawnTime=oldSpawnTime.

During BeginBlock, when the consumers are launched, this RemoveConsumerToBeLaunched call here will thus not remove the consumer

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

Line 76 in 5b6252c

err = k.RemoveConsumerToBeLaunched(ctx, consumerId, initializationParameters.SpawnTime)

and error, which causes the loop to continue.
In the extreme, someone can create 200 such consumers, and those will be re-tried to be launched in every block and no other consumers will ever launch

Nice catch @p-offtermatt. I do think that #2236 somehow fixed this, but we should probably not have inconsistencies between the initialization params and the time queues used to store the chains to be launched.