feat!: compute partial sets

[insumity]

Description

Closes: #1606 and #1607

This PR does some cleaning up by removing all state related with to-be-opted-in and to-be-opted-out validators. This state is not needed anymore after we moved to a DiffValidators approach.

In this PR, we make clear the distinction that a validator could be an i) opted in, or a ii) consumer validator. A consumer validator is one that is validating a consumer chain, while an opted-in validator is one that is set to be a consumer validator at the end of an upcoming epoch.
Also, note that a validator can simply opt out by calling MsgOptOut. However, a validator that is a Top N validator on a Top N chain would be automatically opted-in again at the end of an epoch.
We also simplified how a Yes vote works for ConsumerAdditionProposals.

This PR fails the Run Gosec Security Scanner but this is due to test-related code in tests/integration/distribution.go. This is fixed here; we should not be scanning the tests directory for security issues.

Also, the Protobuf checks fail because:

Error: Previously present message "OptedInValidator" was deleted from file.

but this is fine because OptedInValidator never existed in the main branch or some release.

The MBT tests fail due to

Expected an error: Cannot opt out a validator that is in the top N

this is because we changed how opting out form a Top N chain operates as mentioned above.

---

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

[p-offtermatt]

Left some comments. My main concern is that the predicate seems too restrictive to be used for power shaping.

[p-offtermatt]

Is ShouldConsider the thing we would use for what Jehan calls power shaping?
Because I think a predicate (val -> bool) is probably not expressive enough.
In reality, we want to be able to put functions
[]ConsumerValidator -> []ConsumerValidator

The predicate is not enough to express e.g. power caps

[insumity]

We discussed offline: We agree that this can't be used for power shaping. But it is something we can do add/refactor later when we actually implement power shaping, etc.

[p-offtermatt]

Can we de-duplicate this more? Lines 226 to 235 here are still the same as lines 293 to 301 in proposal.go (but it's already much better like this)

[insumity]

There are not exactly the same. In relay.go we do:

if topN, found := k.GetTopN(ctx, chain.ChainId); found {

while in proposal.go we do:

if prop.Top_N > 0 {

Not clear what you have in mind here. It might be simpler to centralize everything in a future iteration of PSS where we add more types of chains and put the logic on what to do if Top N, etc. in one function.

[p-offtermatt]

Yeah, you are right, let's keep this in mind to maybe consolidate more in the future, but leave as is for now

[p-offtermatt]

Hey,

I think there is a bug here.
If topN == 0, topNThreshold will be the maximal power.

However, this is invoked e.g. here

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

Line 65 in 30b99aa

if power >= minPowerToOptIn {

to check whether someone can opt out.
This disallows the val with maximal power from opting out, for example.

I think it should just return something to signify infinity if topN == 0

[p-offtermatt]

When processing proposals, you do check whether topN > 0 (see

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

Line 292 in 30b99aa

if prop.Top_N > 0 {

), but I think this shows that doing this wherever ComputeMinPowerToOptIn is wrong, since it is too easy to miss.

Just make the behaviour of ComputeMinPowerToOptIn to not require the explicit check for topN > 0 wherever it is invoked by returning infinity

[insumity]

Great catch. I fixed the condition on HandleOptOut to:

if topN, found := k.GetTopN(ctx, chainID); found && topN > 0 {

to guarantee that ComputeMinPowerToOptIn does not get topN == 0.
But, also changed ComputeMinPowerToOptIn to return math.MaxInt64 when topN == 0 to avoid potential pitfalls.