ConsumerAdditionProposal must contain most consumer params (such as consumer unbonding period)

[mpoke]

JEHANS EDIT: I've expanded the definition of this issue to encompass all the consumer params which should be added to the consumer creation proposal.

Problem

Currently, the consumer client on the provider chain is created with the default consumer unbonding period.

Closing criteria

The consumer unbonding period must be passed through the ConsumerAdditionProposal gov proposal.

Problem details

Creating a client requires a client state. The client state for Tendermint has a field UnbondingPeriod that must match the unbonding period of the targeting chain.

TODOs

• Labels have been added for issue
• Issue has been added to the ICS project

[jtremback]

Does this mean it is not possible to create a consumer with an unbonding period that is different than the default, even if you edit the consumer genesis section?

[jtremback]

Currently we have a pattern where any consumer parameter that needs to be changed needs to either be changed with consumer governance after it is running, or changed by editing the consumer genesis section. Neither of these are really very good, and in this case have led to unforeseen issues. We should make it so that any consumer parameter that could be changed appears in the consumer governance proposal. Here are the consumer params:

// Params defines the parameters for CCV consumer module
message Params {
  // TODO: Remove enabled flag and find a better way to setup e2e tests
  // See: https://github.com/cosmos/interchain-security/issues/339
  bool enabled = 1;

  ///////////////////////
  // Distribution Params
  // Number of blocks between ibc-token-transfers from the consumer chain to
  // the provider chain. Note that at this transmission event a fraction of
  // the accumulated tokens are divided and sent consumer redistribution
  // address.
  int64 blocks_per_distribution_transmission = 2;

  // Channel, and provider-chain receiving address to send distribution token
  // transfers over. These parameters is auto-set during the consumer <->
  // provider handshake procedure.
  string distribution_transmission_channel = 3;
  string provider_fee_pool_addr_str = 4;
  
// Sent CCV related IBC packets will timeout after this duration
  google.protobuf.Duration ccv_timeout_period = 5
      [(gogoproto.nullable) = false, (gogoproto.stdduration) = true];

  // Sent transfer related IBC packets will timeout after this duration
  google.protobuf.Duration transfer_timeout_period = 6
      [(gogoproto.nullable) = false, (gogoproto.stdduration) = true];

  // The fraction of tokens allocated to the consumer redistribution address
  // during distribution events. The fraction is a string representing a
  // decimal number. For example "0.75" would represent 75%.
  string consumer_redistribution_fraction = 7;

  // The number of historical info entries to persist in store.
  // This param is a part of the cosmos sdk staking module. In the case of 
  // a ccv enabled consumer chain, the ccv module acts as the staking module.
  int64 historical_entries = 8;

  // Unbonding period for the consumer,
  // which should be smaller than that of the provider in general.
  google.protobuf.Duration unbonding_period = 9
      [(gogoproto.nullable) = false, (gogoproto.stdduration) = true];
}

These need to be go into the proposal

• blocks_per_distribution_transmission
• ccv_timeout_period
• transfer_timeout_period
• consumer_redistribution_fraction
• historical_entries
• unbonding_period

These do not

• enabled is a weird vestige
• distribution_transmission_channel and provider_fee_pool_addr_str are set in the handshake

[danwt]

See relevant

• Consumer Unbonding As Param #410

[danwt]

Age of evidence params will need to be set and sanity checked too

https://github.com/cosmos/cosmos-sdk/blob/interchain-security-rebase/x/evidence/keeper/infraction.go#L26-L41

Specifically, it should be impossible to send evidence old than unbondingPeriod

[mpoke]

Age of evidence params will need to be set and sanity checked too

https://github.com/cosmos/cosmos-sdk/blob/interchain-security-rebase/x/evidence/keeper/infraction.go#L26-L41

Nice catch. We should update the ICS param matrix with Tendermint params.

Specifically, it should be impossible to send evidence old than unbondingPeriod

I think it's better to drop SlashPackets with SlashPacketData.vscID already unbonded on that chain (i.e., if a VSCMaturedPacket was already received from that consumer). @danwt do you agree?

[danwt]

I think it's better to drop SlashPackets with SlashPacketData.vscID already unbonded on that chain (i.e., if a VSCMaturedPacket was already received from that consumer). @danwt do you agree?

This is more inline with the mindset we are moving towards: not trusting the consumer. So I agree 👍

[mpoke]

I think it's better to drop SlashPackets with SlashPacketData.vscID already unbonded on that chain (i.e., if a VSCMaturedPacket was already received from that consumer). @danwt do you agree?

This is more inline with the mindset we are moving towards: not trusting the consumer. So I agree 👍

I've opened an issue #544

[MSalopek]

Age of evidence params will need to be set and sanity checked too

https://github.com/cosmos/cosmos-sdk/blob/interchain-security-rebase/x/evidence/keeper/infraction.go#L26-L41

Specifically, it should be impossible to send evidence old than unbondingPeriod

@danwt Thanks for pointing this out.
Could you provide some more information about how would you treat this in the context of ConsumerAdditionProposal?
Do we need to add fields for evidence age or are we alright with just dropping faulty slash packets?

[jtremback]

@MSalopek I think it was decided above that this check would not need to be done because it would be taken care of in another part of the code

[danwt]

I would suggest to keep this tracked as otherwise the age of evidence params will be left hanging and won't make sense on the consumer chain. Ie. if they are effectively useless on the consumer it should be documented as such.