Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: deprecate soft opt-out #1964

Merged
merged 13 commits into from
Jun 25, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Remove soft opt-out feature.
([\#1964](https://github.com/cosmos/interchain-security/pull/1964))
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Remove soft opt-out feature.
([\#1964](https://github.com/cosmos/interchain-security/pull/1964))
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Remove soft opt-out feature.
([\#1964](https://github.com/cosmos/interchain-security/pull/1964))
1 change: 0 additions & 1 deletion app/consumer-democracy/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,6 @@ func New(
// CanWithdrawInvariant invariant.
// NOTE: staking module is required if HistoricalEntries param > 0
// NOTE: capability module's beginblocker must come before any modules using capabilities (e.g. IBC)
// NOTE: the soft opt-out requires that the consumer module's beginblocker comes after the slashing module's beginblocker
app.MM.SetOrderBeginBlockers(
// upgrades should be run first
upgradetypes.ModuleName,
Expand Down
1 change: 0 additions & 1 deletion app/consumer/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,6 @@ func New(
// NOTE: Capability module must occur first so that it can initialize any capabilities
// so that other modules that want to create or claim capabilities afterwards in InitChain
// can do so safely.
// NOTE: the soft opt-out requires that the consumer module's beginblocker comes after the slashing module's beginblocker
app.MM.SetOrderInitGenesis(
capabilitytypes.ModuleName,
authtypes.ModuleName,
Expand Down
3 changes: 3 additions & 0 deletions app/consumer/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ func transformToNew(jsonRaw []byte, ctx client.Context) (json.RawMessage, error)
oldConsumerGenesis.Params.RetryDelayPeriod = types.DefaultRetryDelayPeriod
}

// `SoftOptOutThreshold` is deprecated in the current consumer implementation, so set to zero
oldConsumerGenesis.Params.SoftOptOutThreshold = "0"

// Versions before v3.3.x of provider genesis data fills up deprecated fields
// ProviderClientState, ConsensusState and InitialValSet in type GenesisState
newGenesis := types.ConsumerGenesisState{
Expand Down
9 changes: 7 additions & 2 deletions app/consumer/genesis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,9 @@ func TestConsumerGenesisTransformationFromV2ToCurrent(t *testing.T) {
require.EqualValues(t, srcGenesis.Params.ConsumerRedistributionFraction, resultGenesis.Params.ConsumerRedistributionFraction)
require.EqualValues(t, srcGenesis.Params.HistoricalEntries, resultGenesis.Params.HistoricalEntries)
require.EqualValues(t, srcGenesis.Params.UnbondingPeriod, resultGenesis.Params.UnbondingPeriod)
require.EqualValues(t, srcGenesis.Params.SoftOptOutThreshold, resultGenesis.Params.SoftOptOutThreshold)

// `SoftOptOutThreshold` is deprecated, so it should be set to zero the current version
require.EqualValues(t, "0", resultGenesis.Params.SoftOptOutThreshold)
require.EqualValues(t, srcGenesis.Params.RewardDenoms, resultGenesis.Params.RewardDenoms)
require.EqualValues(t, srcGenesis.Params.ProviderRewardDenoms, resultGenesis.Params.ProviderRewardDenoms)

Expand Down Expand Up @@ -565,7 +567,10 @@ func TestConsumerGenesisTransformationV330ToCurrent(t *testing.T) {
require.Equal(t, srcGenesis.Params.ConsumerRedistributionFraction, resultGenesis.Params.ConsumerRedistributionFraction)
require.Equal(t, srcGenesis.Params.HistoricalEntries, resultGenesis.Params.HistoricalEntries)
require.Equal(t, srcGenesis.Params.UnbondingPeriod, resultGenesis.Params.UnbondingPeriod)
require.Equal(t, srcGenesis.Params.SoftOptOutThreshold, resultGenesis.Params.SoftOptOutThreshold)

// `SoftOptOutThreshold` is deprecated, so it should be set to zero the current version
require.Equal(t, "0", resultGenesis.Params.SoftOptOutThreshold)

require.Equal(t, srcGenesis.Params.RewardDenoms, resultGenesis.Params.RewardDenoms)
require.Equal(t, srcGenesis.Params.ProviderRewardDenoms, resultGenesis.Params.ProviderRewardDenoms)

Expand Down
1 change: 0 additions & 1 deletion app/sovereign/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ import (
govkeeper "github.com/cosmos/cosmos-sdk/x/gov/keeper"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
govv1beta1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1"

// add mint
mint "github.com/cosmos/cosmos-sdk/x/mint"
mintkeeper "github.com/cosmos/cosmos-sdk/x/mint/keeper"
Expand Down
5 changes: 3 additions & 2 deletions docs/docs/adrs/adr-009-soft-opt-out.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ title: Soft Opt-Out
## Changelog

* 6/13/23: Initial draft of ADR. Feature already implemented and in production.
* 6/19/24: Change status to deprecated

## Status

Accepted

Deprecated
Deprecated by [Partial Set Security](adr-015-partial-set-security.md)
insumity marked this conversation as resolved.
Show resolved Hide resolved
## Context

Some small validators may not have the resources needed to validate all consumer chains. Therefore a need exists to allow the bottom `x%` of validators to opt-out of validating a consumer chain. Meaning downtime infractions for these validators are dropped without ever reaching the provider.
Expand Down
2 changes: 1 addition & 1 deletion docs/docs/adrs/intro.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ To suggest an ADR, please make use of the [ADR template](./adr-template.md) prov
- [ADR 004: Denom DOS fixes](./adr-004-denom-dos-fixes.md)
- [ADR 005: Cryptographic verification of equivocation evidence](./adr-005-cryptographic-equivocation-verification.md)
- [ADR 008: Throttle with retries](./adr-008-throttle-retries.md)
- [ADR 009: Soft Opt-Out](./adr-009-soft-opt-out.md)
- [ADR 010: Standalone to Consumer Changeover](./adr-010-standalone-changeover.md)
- [ADR 013: Slashing on the provider for consumer equivocation](./adr-013-equivocation-slashing.md)
- [ADR 014: Epochs](./adr-014-epochs.md)
Expand All @@ -57,3 +56,4 @@ To suggest an ADR, please make use of the [ADR template](./adr-template.md) prov
### Deprecated

- [ADR 003: Equivocation governance proposal](./adr-003-equivocation-gov-proposal.md)
- [ADR 009: Soft Opt-Out](./adr-009-soft-opt-out.md)
8 changes: 0 additions & 8 deletions docs/docs/features/proposals.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,6 @@ Minimal example:
}
```

:::warning
Before the introduction of Partial Set Security, consumer chains typically included a "soft opt-out mechanism"
which allows the bottom N% of the provider's validators to not validate the consumer chain, without being jailed for downtime on the provider.
After the introduction of Partial Set Security, the use of the soft opt-out mechanism is discouraged, and consumer chains are
encouraged to use the topN parameter to not force validators with little stake to validate the chain.
:::


## `ConsumerModificationProposal`
Proposal type used to change the power shaping parameters of a running consumer chain, as well as to change a Top N running
consumer chain to an Opt-In chain and vice versa.
Expand Down
5 changes: 4 additions & 1 deletion proto/interchain_security/ccv/consumer/v1/consumer.proto
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ message CrossChainValidator {
(cosmos_proto.accepts_interface) = "cosmos.crypto.PubKey",
(gogoproto.moretags) = "yaml:\"consensus_pubkey\""
];
bool opted_out = 4;

// !!! DEPRECATED !!! opted_out is deprecated because after the introduction of Partial Set Security (PSS)
// we removed the soft opt-out feature.
bool opted_out = 4 [deprecated = true];
}

// A record storing the state of a slash packet sent to the provider chain
Expand Down
7 changes: 2 additions & 5 deletions proto/interchain_security/ccv/v1/shared_consumer.proto
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,8 @@ message ConsumerParams {
google.protobuf.Duration unbonding_period = 9
[ (gogoproto.nullable) = false, (gogoproto.stdduration) = true ];

// The threshold for the percentage of validators at the bottom of the set who
// can opt out of running the consumer chain without being punished. For
// example, a value of 0.05 means that the validators in the bottom 5% of the
// set can opt out
string soft_opt_out_threshold = 10;
// !!! DEPRECATED !!! soft_opt_out_threshold is deprecated. see docs/docs/adrs/adr-015-partial-set-security.md
string soft_opt_out_threshold = 10 [deprecated = true];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConsumerParams is used to send data to the consumer chain "over the wire" - it is used in the consumer genesis.

Setting it to deprecated allows the field to exist but it will always be empty, thus maintaining backward compatibility.

We will not need to write a special parser to go from v4.2.0 ConsumerParams to some future version ConsumerParams.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. This is why we did not use reserved.


// Reward denoms. These are the denominations which are allowed to be sent to
// the provider as rewards.
Expand Down
14 changes: 10 additions & 4 deletions tests/e2e/actions.go
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,6 @@ func (tr TestConfig) submitConsumerModificationProposal(
}

bz, err = cmd.CombinedOutput()

if err != nil {
log.Fatal(err, "\n", string(bz))
}
Expand Down Expand Up @@ -606,10 +605,14 @@ func (tr *TestConfig) startConsumerChain(
verbose bool,
) {
fmt.Println("Starting consumer chain ", action.ConsumerChain)

consumerGenesis := ".app_state.ccvconsumer = " + tr.getConsumerGenesis(action.ProviderChain, action.ConsumerChain, target)
consumerGenesisChanges := tr.chainConfigs[action.ConsumerChain].GenesisChanges
if consumerGenesisChanges != "" {
consumerGenesis = consumerGenesis + " | " + consumerGenesisChanges + " | " + action.GenesisChanges
consumerGenesis = consumerGenesis + " | " + consumerGenesisChanges
}
if action.GenesisChanges != "" {
consumerGenesis = consumerGenesis + " | " + action.GenesisChanges
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added here and in changeoverChain because If action.GenesisChanges was the empty string, consumerGenesis would end with | and as a result the jq transformation here would fail.

}

tr.startChain(StartChainAction{
Expand Down Expand Up @@ -841,7 +844,10 @@ func (tr TestConfig) changeoverChain(
consumerGenesis := ".app_state.ccvconsumer = " + string(bz)
consumerGenesisChanges := tr.chainConfigs[action.SovereignChain].GenesisChanges
if consumerGenesisChanges != "" {
consumerGenesis = consumerGenesis + " | " + consumerGenesisChanges + " | " + action.GenesisChanges
consumerGenesis = consumerGenesis + " | " + consumerGenesisChanges
}
if action.GenesisChanges != "" {
consumerGenesis = consumerGenesis + " | " + action.GenesisChanges
}

tr.startChangeover(ChangeoverChainAction{
Expand Down Expand Up @@ -2065,7 +2071,7 @@ func (tr TestConfig) invokeDoublesignSlash(
if err != nil {
log.Fatal(err, "\n", string(bz))
}
tr.waitBlocks("provi", 10, 2*time.Minute)
tr.waitBlocks("provi", 20, 4*time.Minute)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note for posterity: Increased this due to some flakiness in the test involving double signing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor the use of constants for repeated string literals.

The string "auto" is used multiple times across different functions. It is beneficial to define it as a constant at the beginning of the file to ensure consistency and ease of maintenance.

+ const autoGas = "auto"
- gas := "auto"
+ gas := autoGas

Committable suggestion was skipped due to low confidence.

} else { // tr.useCometMock
validatorPrivateKeyAddress := tr.GetValidatorPrivateKeyAddress(action.Chain, action.Validator)

Expand Down
115 changes: 5 additions & 110 deletions tests/e2e/step_delegation.go
Original file line number Diff line number Diff line change
Expand Up @@ -220,58 +220,6 @@ func stepsCancelUnbond(consumerName string) []Step {
}
}

// stepsRedelegateForOptOut tests redelegation, and sets up voting powers s.t
// alice will have less than 5% of the total voting power. This is needed to
// test opt-out functionality.
func stepsRedelegateForOptOut(consumerName string) []Step {
return []Step{
{
Action: RedelegateTokensAction{
Chain: ChainID("provi"),
Src: ValidatorID("alice"),
Dst: ValidatorID("carol"),
TxSender: ValidatorID("alice"),
Amount: 450000000,
},
State: State{
ChainID("provi"): ChainState{
ValPowers: &map[ValidatorID]uint{
ValidatorID("alice"): 60,
ValidatorID("bob"): 500,
ValidatorID("carol"): 950,
},
},
ChainID(consumerName): ChainState{
ValPowers: &map[ValidatorID]uint{
// Voting power changes not seen by consumer yet
ValidatorID("alice"): 510,
ValidatorID("bob"): 500,
ValidatorID("carol"): 500,
},
},
},
},
{
Action: RelayPacketsAction{
ChainA: ChainID("provi"),
ChainB: ChainID(consumerName),
Port: "provider",
Channel: 0,
},
State: State{
ChainID(consumerName): ChainState{
ValPowers: &map[ValidatorID]uint{
// Now power changes are seen by consumer
ValidatorID("alice"): 60,
ValidatorID("bob"): 500,
ValidatorID("carol"): 950,
},
},
},
},
}
}

// stepsRedelegate tests redelegation and resulting validator power changes.
func stepsRedelegate(consumerName string) []Step {
return []Step{
Expand All @@ -283,68 +231,15 @@ func stepsRedelegate(consumerName string) []Step {
TxSender: ValidatorID("carol"),
// redelegate s.t. alice has majority stake so non-faulty validators maintain more than
// 2/3 voting power during downtime tests below, avoiding chain halt
Amount: 449000000,
Amount: 400000000,
},
State: State{
ChainID("provi"): ChainState{
ValPowers: &map[ValidatorID]uint{
ValidatorID("alice"): 509,
ValidatorID("alice"): 910,
ValidatorID("bob"): 500,
// carol always uses a consumer assigned key
ValidatorID("carol"): 501,
},
},
ChainID(consumerName): ChainState{
ValPowers: &map[ValidatorID]uint{
// Voting power changes not seen by consumer yet
ValidatorID("alice"): 60,
ValidatorID("bob"): 500,
ValidatorID("carol"): 950,
},
},
},
},
{
Action: RelayPacketsAction{
ChainA: ChainID("provi"),
ChainB: ChainID(consumerName),
Port: "provider",
Channel: 0,
},
State: State{
ChainID(consumerName): ChainState{
ValPowers: &map[ValidatorID]uint{
// Now power changes are seen by consumer
ValidatorID("alice"): 509,
ValidatorID("bob"): 500,
ValidatorID("carol"): 501,
},
},
},
},
}
}

// stepsRedelegate tests redelegation and resulting validator power changes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this function because it did not add much because we could use stepsRedelegate in its place.

func stepsRedelegateShort(consumerName string) []Step {
return []Step{
{
Action: RedelegateTokensAction{
Chain: ChainID("provi"),
Src: ValidatorID("alice"),
Dst: ValidatorID("carol"),
TxSender: ValidatorID("alice"),
// Leave alice with majority stake so non-faulty validators maintain more than
// 2/3 voting power during downtime tests below, avoiding chain halt
Amount: 1000000,
},
State: State{
ChainID("provi"): ChainState{
ValPowers: &map[ValidatorID]uint{
ValidatorID("alice"): 509,
ValidatorID("bob"): 500,
// carol always uses a consumer assigned key
ValidatorID("carol"): 501,
ValidatorID("carol"): 100,
},
},
ChainID(consumerName): ChainState{
Expand All @@ -368,9 +263,9 @@ func stepsRedelegateShort(consumerName string) []Step {
ChainID(consumerName): ChainState{
ValPowers: &map[ValidatorID]uint{
// Now power changes are seen by consumer
ValidatorID("alice"): 509,
ValidatorID("alice"): 910,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why has the ratio changed so much?

Are you trying to check that the soft opt-out no longer works?

Copy link
Contributor Author

@insumity insumity Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why has the ratio changed so much?

TL;DR because we removed opt-out related steps that were reducing the ValPower of alice.

Specifically, we removed the stepsRedelegateForOptOut and stepsDowntimeWithOptOut. The stepsRedelegateForOptOut was the one that reduced the ValPower of alice to 60 so it could test the opt out functionality.

Before, in the happyPathSteps we had:

...
stepsRedelegateForOptOut("consu"),
stepsDowntimeWithOptOut("consu"),
stepsRedelegate("consu"),
...

but we removed stepsRedelegateForOptOut and stepsDowntimeWithOptOut and hence now when stepsRedelegate is called, the ValPower of alice is not 60 anymore but 510. Redelegating 400 to alice makes alice have a ValPower 910. This meant that all steps* functions called after stepsRedelegate had to be modified as well to contain the correct ValPowers.

Are you trying to check that the soft opt-out no longer works?

No.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for elaborating. It was not clear to me after going over it a couple times.

ValidatorID("bob"): 500,
ValidatorID("carol"): 501,
ValidatorID("carol"): 100,
},
},
},
Expand Down
16 changes: 7 additions & 9 deletions tests/e2e/steps.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ var compatibilitySteps = concatSteps(
compstepsStartChains([]string{"consu"}, false),
stepsDelegate("consu"),
stepsUnbond("consu"),
stepsRedelegateShort("consu"),
stepsRedelegate("consu"),
stepsDowntime("consu"),
stepsDoubleSignOnProvider("consu"), // carol double signs on provider
stepsStartRelayer(),
Expand All @@ -32,8 +32,6 @@ var happyPathSteps = concatSteps(
stepsAssignConsumerKeyOnStartedChain("consu", "bob"),
stepsUnbond("consu"),
stepsCancelUnbond("consu"),
stepsRedelegateForOptOut("consu"),
stepsDowntimeWithOptOut("consu"),
stepsRedelegate("consu"),
stepsDowntime("consu"),
stepsDoubleSignOnProvider("consu"), // carol double signs on provider
Expand All @@ -46,7 +44,7 @@ var shortHappyPathSteps = concatSteps(
stepsStartChains([]string{"consu"}, false),
stepsDelegate("consu"),
stepsUnbond("consu"),
stepsRedelegateShort("consu"),
stepsRedelegate("consu"),
stepsDowntime("consu"),
stepsDoubleSignOnProvider("consu"), // carol double signs on provider
stepsStartRelayer(),
Expand All @@ -58,12 +56,12 @@ var lightClientAttackSteps = concatSteps(
stepsStartChains([]string{"consu"}, false),
stepsDelegate("consu"),
stepsUnbond("consu"),
stepsRedelegateShort("consu"),
stepsRedelegate("consu"),
stepsDowntime("consu"),
stepsLightClientAttackOnProviderAndConsumer("consu"), // carol double signs on provider, bob double signs on consumer
stepsStartRelayer(),
stepsConsumerRemovalPropNotPassing("consu", 3), // submit removal prop but vote no on it - chain should stay
stepsStopChain("consu", 4), // stop chain
stepsConsumerRemovalPropNotPassing("consu", 2), // submit removal prop but vote no on it - chain should stay
stepsStopChain("consu", 3), // stop chain
)

var slashThrottleSteps = concatSteps(
Expand Down Expand Up @@ -121,7 +119,7 @@ var changeoverSteps = concatSteps(

var consumerMisbehaviourSteps = concatSteps(
// start provider and consumer chain
stepsStartChainsWithSoftOptOut("consu"),
stepsStartChainsForConsumerMisbehaviour("consu"),
// make a consumer validator to misbehave and get jailed
stepsCauseConsumerMisbehaviour("consu"),
)
Expand All @@ -137,6 +135,6 @@ var consumerDoubleDowntimeSteps = concatSteps(
stepsStartChains([]string{"consu"}, false),
stepsDelegate("consu"),
stepsUnbond("consu"),
stepsRedelegateShort("consu"),
stepsRedelegate("consu"),
stepsDoubleDowntime("consu"),
)
6 changes: 0 additions & 6 deletions tests/e2e/steps_active_set_changes.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,6 @@ func stepsActiveSetChanges() []Step {
{Id: ValidatorID("bob"), Stake: 200000000, Allocation: 10000000000},
{Id: ValidatorID("carol"), Stake: 700000000, Allocation: 10000000000},
},
// For consumers that're launching with the provider being on an earlier version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test removes the threshold while the ones above have it stil

Copy link
Contributor Author

@insumity insumity Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thresholds are removed from the individual StarConsumerChainAction and ChangeOverChainAction actions and are moved to startConsumerChain and changeoverChain so we do not have to set the threshold in a multitude of other places. This was just part of small refactoring.

// of ICS before the soft opt-out threshold was introduced, we need to set the
// soft opt-out threshold to 0.05 in the consumer genesis to ensure that the
// consumer binary doesn't panic. Sdk requires that all params are set to valid
// values from the genesis file.
GenesisChanges: ".app_state.ccvconsumer.params.soft_opt_out_threshold = \"0.05\"",
},
State: State{},
},
Expand Down
Loading
Loading