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!: added E2E test and docs for ConsumerModificationProposal #1949

Merged
merged 5 commits into from
Jun 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions .github/workflows/nightly-e2e.yml
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,22 @@ jobs:
go-version: "1.21" # The Go version to download (if necessary) and use.
- name: E2E partial set security denylist
run: go run ./tests/e2e/... --tc partial-set-security-validators-denylisted
partial-set-security-modification-proposal:
runs-on: ubuntu-latest
timeout-minutes: 20
steps:
- uses: actions/setup-go@v5
with:
go-version: "1.21"
- uses: actions/checkout@v4
- name: Checkout LFS objects
run: git lfs checkout
- name: Setup Go
uses: actions/setup-go@v5
with:
go-version: "1.21" # The Go version to download (if necessary) and use.
- name: E2E partial set security modification proposal
run: go run ./tests/e2e/... --tc partial-set-security-modification-proposal

nightly-test-fail:
needs:
Expand All @@ -295,6 +311,7 @@ jobs:
- partial-set-security-validators-power-cap-test
- partial-set-security-validators-allowlisted-test
- partial-set-security-validators-denylisted-test
- partial-set-security-modification-proposal
if: ${{ failure() }}
runs-on: ubuntu-latest
steps:
Expand Down
1 change: 1 addition & 0 deletions app/provider/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ var (
ibcclientclient.UpgradeProposalHandler,
ibcproviderclient.ConsumerAdditionProposalHandler,
ibcproviderclient.ConsumerRemovalProposalHandler,
ibcproviderclient.ConsumerModificationProposalHandler,
ibcproviderclient.ChangeRewardDenomsProposalHandler,
},
),
Expand Down
5 changes: 5 additions & 0 deletions docs/docs/features/partial-set-security.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,8 @@ For Top N chains, this is also the long term vision for how they are launched.

For Opt In chains, this is a temporary measure to prevent issues around chain ID squatting, i.e. someone could spuriously register many desirable chain IDs of upcoming consumer chain and simply deny legitimate consumer chains from using them. Eventually, the plan is to allow launching Opt In chains permissionlessly without going through governance, with quality control being handled by the market of validators deciding which chains they would like to validate on.
:::

:::tip
A running Top N consumer chain might want to become an Opt-In chain or vice versa. This can be achieved by issuing
a [`ConsumerModificationProposal`](./proposals.md#consumermodificationproposal).
:::
6 changes: 5 additions & 1 deletion docs/docs/features/power-shaping.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,8 @@ an allowlist that is too short can very quickly become outdated and leave too fe
the power distribution on the provider shifts and the denylisted validators gain more power.

In general, when setting these parameters, consider that the voting power distribution in the future might be very different from the one right now,
and that the chain should be secure even if the power distribution changes significantly.
and that the chain should be secure even if the power distribution changes significantly.

:::tip
The power shaping parameters of a running consumer chain can be changed through a [`ConsumerModificationProposal`](./proposals.md#consumermodificationproposal).
:::
34 changes: 33 additions & 1 deletion docs/docs/features/proposals.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ sidebar_position: 3

# ICS Provider Proposals

Interchain security module introduces 3 new proposal types to the provider.
Interchain security module introduces new proposal types to the provider.

The proposals are used to propose upcoming interchain security events through governance.

Expand Down Expand Up @@ -84,6 +84,38 @@ After the introduction of Partial Set Security, the use of the soft opt-out mech
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.

When a `ConsumerModificationProposal` passes for a running consumer chain, the consumer chain would change all its
parameters to the ones passed in the `ConsumerModificationProposal`.

Assume, a `chain-1` is a Top N chain. If the following `ConsumerModificationProposal` passes, then `chain-1` would become
an Opt-In chain with a 40% validators power cap, a maximum number of 30 validators, and one denylisted validator.
```js
{
"title": "Modify consumer chain",
"description": ".md description of your chain and all other relevant information",
"chain_id": "chain-1",
"top_N": 0,
"validators_power_cap": 40,
"validator_set_cap": 30,
"allowlist": [],
"denylist": ["cosmosvalcons1qmq08eruchr5sf5s3rwz7djpr5a25f7xw4mceq"]
}
```

:::warning
If `top_N`, `validators_power_cap`, or some other argument is not included in the proposal, then it is considered
that the default value is set for this argument. For example, if a Top 50% chain wants to only modify `validators_power_cap`
from 35 to 40, then the `ConsumerModificationProposal` would still need to include that `top_N` is 50. Otherwise
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comma after 'Otherwise' for grammatical correctness.

- Otherwise `top_N` would be set to its default value of 0, and the chain would become an Opt-In chain.
+ Otherwise, `top_N` would be set to its default value of 0, and the chain would become an Opt-In chain.
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
from 35 to 40, then the `ConsumerModificationProposal` would still need to include that `top_N` is 50. Otherwise
from 35 to 40, then the `ConsumerModificationProposal` would still need to include that `top_N` is 50. Otherwise, `top_N` would be set to its default value of 0, and the chain would become an Opt-In chain.
Tools
LanguageTool

[uncategorized] ~113-~113: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Context: ...ill need to include that top_N is 50. Otherwise top_N would be set to its default val...

`top_N` would be set to its default value of 0, and the chain would become an Opt-In chain.

To be **safe**, always include `top_N` and all the power shaping parameters in your `ConsumerModificationProposal`.
:::

## ChangeRewardDenomProposal

Proposal type used to mutate the set of denoms accepted by the provider as rewards.
Expand Down
6 changes: 6 additions & 0 deletions docs/docs/frequently-asked-questions.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,3 +129,9 @@ Yes, the consumer chain will halt with an ERR CONSENSUS FAILURE error after the
## Can validators set a commission rate for chains they have not opted in to?
Yes, and this is useful for validators that are not in the top N% of the provider chain, but might move into the top N% in the future.
By setting the commission rate ahead of time, they can make sure that they immediately have a commission rate of their choosing as soon as they are in the top N%.

## Can a consumer chain modify its power shaping parameters?
Yes, by issuing a [`ConsumerModificationProposal`](./features/proposals.md#consumermodificationproposal).

## Can a Top N consumer chain become Opt-In or vice versa?
Yes, by issuing a [`ConsumerModificationProposal`](./features/proposals.md#consumermodificationproposal).
Comment on lines +133 to +137
Copy link
Contributor

Choose a reason for hiding this comment

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

Add FAQ entries about modifying power shaping parameters and transitioning between Top N and Opt-In chains using ConsumerModificationProposal.

Consider adding blank lines around headings for better markdown formatting.

- ## Can a Top N consumer chain become Opt-In or vice versa?
+ 
+ ## Can a Top N consumer chain become Opt-In or vice versa?
+ 
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## Can a consumer chain modify its power shaping parameters?
Yes, by issuing a [`ConsumerModificationProposal`](./features/proposals.md#consumermodificationproposal).
## Can a Top N consumer chain become Opt-In or vice versa?
Yes, by issuing a [`ConsumerModificationProposal`](./features/proposals.md#consumermodificationproposal).
## Can a consumer chain modify its power shaping parameters?
Yes, by issuing a [`ConsumerModificationProposal`](./features/proposals.md#consumermodificationproposal).
## Can a Top N consumer chain become Opt-In or vice versa?
Yes, by issuing a [`ConsumerModificationProposal`](./features/proposals.md#consumermodificationproposal).
Tools
Markdownlint

136-136: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces


133-133: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines


136-136: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines


137-137: null (MD047, single-trailing-newline)
Files should end with a single newline character

4 changes: 2 additions & 2 deletions docs/docs/validators/joining-testnet.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ The experience gained in the testnet will prepare you for validating interchain
:::tip
Provider and consumer chain represent distinct networks and infrastructures operated by the same validator set.

For general information about running cosmos-sdk based chains check out the [validator basics](https://hub.cosmos.network/validators/validator-setup) and [Running a Node section](https://docs.cosmos.network/main/run-node/run-node) of Cosmos SDK docs
For general information about running cosmos-sdk based chains check out the [validator basics](https://hub.cosmos.network/main/validators/validator-setup) and [Running a Node section](https://docs.cosmos.network/main/run-node/run-node) of Cosmos SDK docs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this PR was touching docs as well, fixed those stale links.

Copy link
Contributor

Choose a reason for hiding this comment

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

Update URLs to point to the correct resources.

Consider adding a comma after "chains" for better readability.

- For general information about running cosmos-sdk based chains check out the [validator basics](https://hub.cosmos.network/main/validators/validator-setup) and [Running a Node section](https://docs.cosmos.network/main/run-node/run-node) of Cosmos SDK docs
+ For general information about running cosmos-sdk based chains, check out the [validator basics](https://hub.cosmos.network/main/validators/validator-setup) and [Running a Node section](https://docs.cosmos.network/main/run-node/run-node) of Cosmos SDK docs
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
For general information about running cosmos-sdk based chains check out the [validator basics](https://hub.cosmos.network/main/validators/validator-setup) and [Running a Node section](https://docs.cosmos.network/main/run-node/run-node) of Cosmos SDK docs
For general information about running cosmos-sdk based chains, check out the [validator basics](https://hub.cosmos.network/main/validators/validator-setup) and [Running a Node section](https://docs.cosmos.network/main/run-node/run-node) of Cosmos SDK docs
Tools
LanguageTool

[uncategorized] ~15-~15: A comma might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
Context: ...ormation about running cosmos-sdk based chains check out the [validator basics](https:...

:::

## Joining the provider chain
Expand Down Expand Up @@ -79,7 +79,7 @@ gaiad tx staking create-validator \
```

:::tip
Check this [guide](https://hub.cosmos.network/validators/validator-setup#edit-validator-description) to edit your validator.
Check this [guide](https://hub.cosmos.network/main/validators/validator-setup#edit-validator-description) to edit your validator.
:::

After this step, your validator is created and you can start your node and catch up to the rest of the network. It is recommended that you use `statesync` to catch up to the rest of the network.
Expand Down
8 changes: 4 additions & 4 deletions docs/docs/validators/overview.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ At present, the consumer chain can report evidence about downtime infractions to
:::info
Causing a downtime infraction on any consumer chain will not incur a slash penalty. Instead, the offending validator will be jailed on the provider chain and consequently on all consumer chains.

To unjail, the validator must wait for the jailing period to elapse on the provider chain and [submit an unjail transaction](https://hub.cosmos.network/validators/validator-setup#unjail-validator) on the provider chain. After unjailing on the provider, the validator will be unjailed on all consumer chains.
To unjail, the validator must wait for the jailing period to elapse on the provider chain and [submit an unjail transaction](https://hub.cosmos.network/main/validators/validator-setup#unjail-validator) on the provider chain. After unjailing on the provider, the validator will be unjailed on all consumer chains.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this PR was touching docs as well, fixed those stale links.


More information is available in [Downtime Slashing documentation](../features/slashing.md#downtime-infractions)
:::
Expand All @@ -99,7 +99,7 @@ Validators can use different consensus keys on the provider and each of the cons
For more information check out the [Key assignment overview and guide](../features/key-assignment.md)

## References:
- [Cosmos Hub Validators FAQ](https://hub.cosmos.network/validators/validator-faq)
- [Cosmos Hub Running a validator](https://hub.cosmos.network/validators/validator-setup)
- [Cosmos Hub Validators FAQ](https://hub.cosmos.network/main/validators/validator-faq)
- [Cosmos Hub Running a validator](https://hub.cosmos.network/main/validators/validator-setup)
- [Startup Sequence](https://github.com/cosmos/testnets/blob/master/interchain-security/CONSUMER_LAUNCH_GUIDE.md#chain-launch)
- [Submit Unjailing Transaction](https://hub.cosmos.network/validators/validator-setup#unjail-validator)
- [Submit Unjailing Transaction](https://hub.cosmos.network/main/validators/validator-setup#unjail-validator)
73 changes: 73 additions & 0 deletions tests/e2e/actions.go
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,79 @@ func (tr TestConfig) submitConsumerRemovalProposal(
tr.waitBlocks(ChainID("provi"), 2, 20*time.Second)
}

type SubmitConsumerModificationProposalAction struct {
Chain ChainID
From ValidatorID
Deposit uint
ConsumerChain ChainID
TopN uint32
ValidatorsPowerCap uint32
ValidatorSetCap uint32
Allowlist []string
Denylist []string
}

func (tr TestConfig) submitConsumerModificationProposal(
action SubmitConsumerModificationProposalAction,
target ExecutionTarget,
verbose bool,
) {
prop := client.ConsumerModificationProposalJSON{
Title: "Propose the modification of the PSS parameters of a chain",
Summary: "summary of a modification proposal",
ChainId: string(tr.chainConfigs[action.ConsumerChain].ChainId),
Deposit: fmt.Sprint(action.Deposit) + `stake`,
TopN: action.TopN,
ValidatorsPowerCap: action.ValidatorsPowerCap,
ValidatorSetCap: action.ValidatorSetCap,
Allowlist: action.Allowlist,
Denylist: action.Denylist,
}

bz, err := json.Marshal(prop)
if err != nil {
log.Fatal(err)
}

jsonStr := string(bz)
if strings.Contains(jsonStr, "'") {
log.Fatal("prop json contains single quote")
}

//#nosec G204 -- bypass unsafe quoting warning (no production code)
bz, err = target.ExecCommand(
"/bin/bash", "-c", fmt.Sprintf(`echo '%s' > %s`, jsonStr, "/temp-proposal.json"),
Fixed Show fixed Hide fixed
Dismissed Show dismissed Hide dismissed
Copy link
Contributor

Choose a reason for hiding this comment

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

Address potential security vulnerability with unsafe quoting.

The use of fmt.Sprintf to dynamically generate a bash command with user-controlled input can lead to command injection vulnerabilities. Consider using more secure alternatives or ensuring that inputs are properly sanitized.

- fmt.Sprintf(`echo '%s' > %s`, jsonStr, "/temp-proposal.json"),
+ fmt.Sprintf(`echo '%s' > %s`, jsonStr, "/temp-proposal.json"), // Ensure jsonStr is sanitized

Committable suggestion was skipped due to low confidence.

Tools
GitHub Check: CodeQL

[failure] 443-443: Potentially unsafe quoting
If this JSON value contains a single quote, it could break out of the enclosing quotes.

).CombinedOutput()
if err != nil {
log.Fatal(err, "\n", string(bz))
}

// CONSUMER MODIFICATION PROPOSAL
cmd := target.ExecCommand(
tr.chainConfigs[action.Chain].BinaryName,
"tx", "gov", "submit-legacy-proposal", "consumer-modification", "/temp-proposal.json",
`--from`, `validator`+fmt.Sprint(action.From),
`--chain-id`, string(tr.chainConfigs[action.Chain].ChainId),
`--home`, tr.getValidatorHome(action.Chain, action.From),
`--gas`, `900000`,
`--node`, tr.getValidatorNode(action.Chain, action.From),
`--keyring-backend`, `test`,
`-y`,
)
if verbose {
log.Println("submitConsumerModificationProposal cmd: ", cmd.String())
}

bz, err = cmd.CombinedOutput()

if err != nil {
log.Fatal(err, "\n", string(bz))
}

// wait for inclusion in a block -> '--broadcast-mode block' is deprecated
tr.waitBlocks(ChainID("provi"), 2, 10*time.Second)
}
Comment on lines +402 to +473
Copy link
Contributor

Choose a reason for hiding this comment

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

Review the implementation of SubmitConsumerModificationProposalAction.

The function submitConsumerModificationProposal handles the submission of a consumer modification proposal. However, there are several issues and improvements to consider:

  1. Error Handling: The use of log.Fatal for error handling is not recommended in production code as it will stop the execution of the program. Consider returning errors to the caller instead.
  2. Potential Security Issue: The use of fmt.Sprintf with user-controlled input can lead to command injection vulnerabilities. Ensure that inputs are properly sanitized before use.
  3. Code Duplication: The JSON marshaling and command execution logic is repeated multiple times in the file. Consider refactoring this into a helper function to improve code maintainability.
- log.Fatal(err, "\n", string(bz))
+ return err

Committable suggestion was skipped due to low confidence.

Tools
golangci-lint

466-466: File is not gofumpt-ed with -extra (gofumpt)

GitHub Check: CodeQL

[failure] 443-443: Potentially unsafe quoting
If this JSON value contains a single quote, it could break out of the enclosing quotes.


type SubmitParamChangeLegacyProposalAction struct {
Chain ChainID
From ValidatorID
Expand Down
7 changes: 7 additions & 0 deletions tests/e2e/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,12 @@ var stepChoices = map[string]StepChoice{
description: "test partial set security for an Opt-In chain that has a validator denylisted",
testConfig: DefaultTestCfg,
},
"partial-set-security-modification-proposal": {
name: "partial-set-security-modification-proposal",
steps: stepsModifyChain(),
description: "test partial set security parameters can be changed through a modification proposal",
testConfig: DefaultTestCfg,
},
Comment on lines +195 to +200
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure comprehensive test coverage for partial-set-security-modification-proposal.

The new test case should thoroughly test all aspects of the security modifications it aims to cover. Consider adding more detailed steps or checks to ensure comprehensive coverage.

}

func getTestCaseUsageString() string {
Expand Down Expand Up @@ -280,6 +286,7 @@ func getTestCases(selectedPredefinedTests, selectedTestFiles TestSet, providerVe
"consumer-double-downtime", "partial-set-security-opt-in", "partial-set-security-top-n",
"partial-set-security-validator-set-cap", "partial-set-security-validators-power-cap",
"partial-set-security-validators-allowlisted", "partial-set-security-validators-denylisted",
"partial-set-security-modification-proposal",
}
if includeMultiConsumer != nil && *includeMultiConsumer {
selectedPredefinedTests = append(selectedPredefinedTests, "multiconsumer")
Expand Down
24 changes: 24 additions & 0 deletions tests/e2e/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,14 @@ type ConsumerRemovalProposal struct {

func (p ConsumerRemovalProposal) isProposal() {}

type ConsumerModificationProposal struct {
Deposit uint
Chain ChainID
Status string
}

func (p ConsumerModificationProposal) isProposal() {}

type Rewards struct {
IsRewarded map[ValidatorID]bool
// if true it will calculate if the validator/delegator is rewarded between 2 successive blocks,
Expand Down Expand Up @@ -482,6 +490,22 @@ func (tr TestConfig) getProposal(chain ChainID, proposal uint) Proposal {
Chain: chain,
StopTime: int(stopTime.Milliseconds()),
}
case "/interchain_security.ccv.provider.v1.ConsumerModificationProposal":
chainId := gjson.Get(string(bz), `messages.0.content.chain_id`).String()

var chain ChainID
for i, conf := range tr.chainConfigs {
if string(conf.ChainId) == chainId {
chain = i
break
}
}
Dismissed Show dismissed Hide dismissed

return ConsumerModificationProposal{
Deposit: uint(deposit),
Status: status,
Chain: chain,
}
Comment on lines +493 to +508
Copy link
Contributor

Choose a reason for hiding this comment

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

Address potential non-determinism in map iteration.

The iteration over tr.chainConfigs map in the switch case for ConsumerModificationProposal could lead to non-deterministic behavior as the order of map iteration in Go is not guaranteed. Consider sorting the keys or using a slice if the order is important for the application logic.

- for i, conf := range tr.chainConfigs {
+ keys := make([]string, 0, len(tr.chainConfigs))
+ for k := range tr.chainConfigs {
+     keys = append(keys, k)
+ }
+ sort.Strings(keys)  // Ensure the keys are sorted if order is important
+ for _, k := range keys {
+     conf := tr.chainConfigs[k]
      if string(conf.ChainId) == chainId {
          chain = i
          break
      }
  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
case "/interchain_security.ccv.provider.v1.ConsumerModificationProposal":
chainId := gjson.Get(string(bz), `messages.0.content.chain_id`).String()
var chain ChainID
for i, conf := range tr.chainConfigs {
if string(conf.ChainId) == chainId {
chain = i
break
}
}
return ConsumerModificationProposal{
Deposit: uint(deposit),
Status: status,
Chain: chain,
}
case "/interchain_security.ccv.provider.v1.ConsumerModificationProposal":
chainId := gjson.Get(string(bz), `messages.0.content.chain_id`).String()
var chain ChainID
keys := make([]string, 0, len(tr.chainConfigs))
for k := range tr.chainConfigs {
keys = append(keys, k)
}
sort.Strings(keys) // Ensure the keys are sorted if order is important
for _, k := range keys {
conf := tr.chainConfigs[k]
if string(conf.ChainId) == chainId {
chain = i
break
}
}
return ConsumerModificationProposal{
Deposit: uint(deposit),
Status: status,
Chain: chain,
}
Tools
GitHub Check: CodeQL

[warning] 497-502: Iteration over map
Iteration over map may be a possible source of non-determinism

case "/cosmos.params.v1beta1.ParameterChangeProposal":
return ParamsProposal{
Deposit: uint(deposit),
Expand Down
Loading
Loading