-
Notifications
You must be signed in to change notification settings - Fork 136
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!: only allow chains with at least one active validator to launch #2399
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
- `[x/provider]` Prevent Opt-In chains from launching, unless at least one active validator has opted-in to them. | ||
([\#2101](https://github.com/cosmos/interchain-security/pull/2399)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,7 @@ func (s *ProviderSuite) TestProviderCreateConsumer() { | |
testAcc := s.Provider.TestWallets[0].FormattedAddress() | ||
testAccKey := s.Provider.TestWallets[0].KeyName() | ||
|
||
// Confirm that a chain can be create with the minimum params (metadata) | ||
// Confirm that a chain can be created with the minimum params (metadata) | ||
chainName := "minParamAddConsumer" | ||
createConsumerMsg := msgCreateConsumer(chainName, nil, nil, testAcc) | ||
consumerId, err := s.Provider.CreateConsumer(s.GetContext(), createConsumerMsg, testAccKey) | ||
|
@@ -101,6 +101,52 @@ func (s *ProviderSuite) TestProviderCreateConsumerRejection() { | |
s.Require().Error(err) | ||
} | ||
|
||
// TestOptInChainCanOnlyStartIfActiveValidatorOptedIn tests that only if an active validator opts in to an Opt-In chain, the chain can launch. | ||
// Scenario 1: Inactive validators opts in, the chain does not launch. | ||
// Scenario 2: Active validator opts in, the chain launches. | ||
func (s *ProviderSuite) TestOptInChainCanOnlyStartIfActiveValidatorOptedIn() { | ||
testAcc := s.Provider.TestWallets[2].FormattedAddress() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll create a separate PR so that we don't have to be aware of which index is in use during the tests, and it will be handled under the hood. For now, I would use an account that isn't used in other tests, for example, account s.Provider.TestWallets[12] (just to avoid possible an error where an invalid account number is sent if 2 tests send the tx in the same time). I'll try to fix this as soon as possible since the current solution isn't the most elegant. |
||
testAccKey := s.Provider.TestWallets[2].KeyName() | ||
|
||
activeValIndex := 0 | ||
inactiveValIndex := 1 | ||
|
||
// Scenario 1: Inactive validators opts in, the chain does not launch. | ||
chainName := "optInScenario1" | ||
spawnTime := time.Now().Add(time.Hour) | ||
consumerInitParams := consumerInitParamsTemplate(&spawnTime) | ||
createConsumerMsg := msgCreateConsumer(chainName, consumerInitParams, powerShapingParamsTemplate(), testAcc) | ||
consumerId, err := s.Provider.CreateConsumer(s.GetContext(), createConsumerMsg, testAccKey) | ||
s.Require().NoError(err) | ||
consumerChain, err := s.Provider.GetConsumerChain(s.GetContext(), consumerId) | ||
s.Require().NoError(err) | ||
// inactive validator opts in | ||
s.Require().NoError(s.Provider.OptIn(s.GetContext(), consumerChain.ConsumerID, inactiveValIndex)) | ||
consumerInitParams.SpawnTime = time.Now() | ||
upgradeMsg := &providertypes.MsgUpdateConsumer{ | ||
Owner: testAcc, | ||
ConsumerId: consumerChain.ConsumerID, | ||
NewOwnerAddress: testAcc, | ||
InitializationParameters: consumerInitParams, | ||
PowerShapingParameters: powerShapingParamsTemplate(), | ||
} | ||
s.Require().NoError(s.Provider.UpdateConsumer(s.GetContext(), upgradeMsg, testAccKey)) | ||
s.Require().NoError(testutil.WaitForBlocks(s.GetContext(), 1, s.Provider)) | ||
consumerChain, err = s.Provider.GetConsumerChain(s.GetContext(), consumerId) | ||
s.Require().NoError(err) | ||
s.Require().Equal(providertypes.CONSUMER_PHASE_REGISTERED.String(), consumerChain.Phase) | ||
|
||
// Scenario 2: Active validator opts in, the chain launches. | ||
// active validator opts in | ||
s.Require().NoError(s.Provider.OptIn(s.GetContext(), consumerChain.ConsumerID, activeValIndex)) | ||
|
||
s.Require().NoError(s.Provider.UpdateConsumer(s.GetContext(), upgradeMsg, testAccKey)) | ||
s.Require().NoError(testutil.WaitForBlocks(s.GetContext(), 1, s.Provider)) | ||
consumerChain, err = s.Provider.GetConsumerChain(s.GetContext(), consumerId) | ||
s.Require().NoError(err) | ||
s.Require().Equal(providertypes.CONSUMER_PHASE_LAUNCHED.String(), consumerChain.Phase) | ||
} | ||
|
||
// Test Opting in validators to a chain (MsgOptIn) | ||
// Confirm that a chain can be created and validators can be opted in | ||
// Scenario 1: Validators opted in, MsgUpdateConsumer called to set spawn time in the past -> chain should start. | ||
|
@@ -403,12 +449,12 @@ func (s *ProviderSuite) TestProviderTransformTopNtoOptIn() { | |
s.Require().Equal(testAcc, optInChain.OwnerAddress) | ||
} | ||
|
||
// TestOptOut tests removin validator from consumer-opted-in-validators | ||
// TestOptOut tests removing validator from consumer-opted-in-validators | ||
func (s *ProviderSuite) TestOptOut() { | ||
testAcc := s.Provider.TestWallets[7].FormattedAddress() | ||
testAccKey := s.Provider.TestWallets[7].KeyName() | ||
|
||
// Add consume chain | ||
// Add consumer chain | ||
chainName := "TestOptOut" | ||
spawnTime := time.Now().Add(time.Hour) | ||
consumerInitParams := consumerInitParamsTemplate(&spawnTime) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this done for al the tests or only for the new one introduced? We should avoid adding complexity to all tests. If you need multiple validators for a certain test, we should just add a new configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will affect all tests in the suite that use this specification, i.e., the entire ProviderSuite. We could create a separate suite to support running tests on a suite with multiple nodes while keeping the existing suite where the provider chain has only one node (ProviderSuite, ProviderMultiNodeSuite).
Another option is to keep it as Karolos implemented and handle the optional nodes in the suite setup, so tests that need additional nodes can simply start the extra validator(s) as the interchaintest framework supports that.
The difference is that with separate suites, we don’t need to start/stop nodes dynamically, but the provider chain will be started twice. I'm not sure what the optimal solution would be.
If you'd like, we can leave it as Karolos implemented in this PR, and I can split it or add dynamic node management in a follow-up PR. I also plan to improve how "independent" test accounts are fetched, so I can handle everything in that PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Let's do that.
Regarding what option to choose, go with the one easier to implement.