diff --git a/.changelog/unreleased/api-breaking/provider/1732-assigning-already-assigned-key-fix.md b/.changelog/unreleased/api-breaking/provider/1732-assigning-already-assigned-key-fix.md new file mode 100644 index 0000000000..667a481d3f --- /dev/null +++ b/.changelog/unreleased/api-breaking/provider/1732-assigning-already-assigned-key-fix.md @@ -0,0 +1,2 @@ +- Assigning a key that is already assigned by the same validator will now be a no-op instead of throwing an error. + ([\#1732](https://github.com/cosmos/interchain-security/pull/1732)) \ No newline at end of file diff --git a/.changelog/unreleased/state-breaking/provider/1732-assigning-already-assigned-key-fix.md b/.changelog/unreleased/state-breaking/provider/1732-assigning-already-assigned-key-fix.md new file mode 100644 index 0000000000..667a481d3f --- /dev/null +++ b/.changelog/unreleased/state-breaking/provider/1732-assigning-already-assigned-key-fix.md @@ -0,0 +1,2 @@ +- Assigning a key that is already assigned by the same validator will now be a no-op instead of throwing an error. + ([\#1732](https://github.com/cosmos/interchain-security/pull/1732)) \ No newline at end of file diff --git a/tests/e2e/main.go b/tests/e2e/main.go index 7b2ba26278..00a9d0ea15 100644 --- a/tests/e2e/main.go +++ b/tests/e2e/main.go @@ -430,13 +430,13 @@ TEST RESULTS } } if len(passedTests) > 0 { - report += fmt.Sprintln("\n\nPASSED TESTS:\n") + report += fmt.Sprintln("\n\nPASSED TESTS:") for _, t := range passedTests { report += t.Report() } } if len(remainingTests) > 0 { - report += fmt.Sprintln("\n\nREMAINING TESTS:\n") + report += fmt.Sprintln("\n\nREMAINING TESTS:") for _, t := range remainingTests { report += t.Report() } diff --git a/tests/e2e/steps_compatibility.go b/tests/e2e/steps_compatibility.go index ef3cab4e73..000287d640 100644 --- a/tests/e2e/steps_compatibility.go +++ b/tests/e2e/steps_compatibility.go @@ -85,18 +85,6 @@ func compstepsStartConsumerChain(consumerName string, proposalIndex, chainIndex }, }, }, - { - // op should fail - key already assigned by the same validator - Action: AssignConsumerPubKeyAction{ - Chain: ChainID(consumerName), - Validator: ValidatorID("carol"), - ConsumerPubkey: getDefaultValidators()[ValidatorID("carol")].ConsumerValPubKey, - ReconfigureNode: false, - ExpectError: true, - ExpectedError: "a validator has assigned the consumer key already: consumer key is already in use by a validator", - }, - State: State{}, - }, { // op should fail - key already assigned by another validator Action: AssignConsumerPubKeyAction{ diff --git a/tests/e2e/steps_start_chains.go b/tests/e2e/steps_start_chains.go index 08732e3f37..8dcbd7d06f 100644 --- a/tests/e2e/steps_start_chains.go +++ b/tests/e2e/steps_start_chains.go @@ -82,16 +82,24 @@ func stepsStartConsumerChain(consumerName string, proposalIndex, chainIndex uint }, }, { - // op should fail - key already assigned by the same validator + // op should be a noop - key already assigned, but by the same validator Action: AssignConsumerPubKeyAction{ Chain: ChainID(consumerName), Validator: ValidatorID("carol"), ConsumerPubkey: getDefaultValidators()[ValidatorID("carol")].ConsumerValPubKey, ReconfigureNode: false, - ExpectError: true, - ExpectedError: "a validator has assigned the consumer key already: consumer key is already in use by a validator", + ExpectError: false, + }, + State: State{ + ChainID(consumerName): ChainState{ + AssignedKeys: &map[ValidatorID]string{ + ValidatorID("carol"): getDefaultValidators()[ValidatorID("carol")].ConsumerValconsAddressOnProvider, + }, + ProviderKeys: &map[ValidatorID]string{ + ValidatorID("carol"): getDefaultValidators()[ValidatorID("carol")].ValconsAddress, + }, + }, }, - State: State{}, }, { // op should fail - key already assigned by another validator diff --git a/tests/e2e/tracehandler_testdata/consumer-double-sign.json b/tests/e2e/tracehandler_testdata/consumer-double-sign.json index e93049cf8b..8a5203757b 100644 --- a/tests/e2e/tracehandler_testdata/consumer-double-sign.json +++ b/tests/e2e/tracehandler_testdata/consumer-double-sign.json @@ -134,10 +134,30 @@ "Validator": "carol", "ConsumerPubkey": "{\"@type\":\"/cosmos.crypto.ed25519.PubKey\",\"key\":\"Ui5Gf1+mtWUdH8u3xlmzdKID+F3PK0sfXZ73GZ6q6is=\"}", "ReconfigureNode": false, - "ExpectError": true, - "ExpectedError": "a validator has assigned the consumer key already: consumer key is already in use by a validator" + "ExpectError": false, + "ExpectedError": "" }, - "State": {} + "State": { + "consu": { + "ValBalances": null, + "ProposedConsumerChains": null, + "ValPowers": null, + "StakedTokens": null, + "Params": null, + "Rewards": null, + "ConsumerChains": null, + "AssignedKeys": { + "carol": "cosmosvalcons1kswr5sq599365kcjmhgufevfps9njf43e4lwdk" + }, + "ProviderKeys": { + "carol": "cosmosvalcons1ezyrq65s3gshhx5585w6mpusq3xsj3ayzf4uv6" + }, + "ConsumerPendingPacketQueueSize": null, + "RegisteredConsumerRewardDenoms": null, + "ClientsFrozenHeights": null, + "Proposals": null + } + } }, { "ActionType": "main.AssignConsumerPubKeyAction", diff --git a/tests/e2e/tracehandler_testdata/democracy.json b/tests/e2e/tracehandler_testdata/democracy.json index 10c9838122..3f425a2a92 100644 --- a/tests/e2e/tracehandler_testdata/democracy.json +++ b/tests/e2e/tracehandler_testdata/democracy.json @@ -134,10 +134,30 @@ "Validator": "carol", "ConsumerPubkey": "{\"@type\":\"/cosmos.crypto.ed25519.PubKey\",\"key\":\"Ui5Gf1+mtWUdH8u3xlmzdKID+F3PK0sfXZ73GZ6q6is=\"}", "ReconfigureNode": false, - "ExpectError": true, - "ExpectedError": "a validator has assigned the consumer key already: consumer key is already in use by a validator" + "ExpectError": false, + "ExpectedError": "" }, - "State": {} + "State": { + "democ": { + "ValBalances": null, + "ProposedConsumerChains": null, + "ValPowers": null, + "StakedTokens": null, + "Params": null, + "Rewards": null, + "ConsumerChains": null, + "AssignedKeys": { + "carol": "cosmosvalcons1kswr5sq599365kcjmhgufevfps9njf43e4lwdk" + }, + "ProviderKeys": { + "carol": "cosmosvalcons1ezyrq65s3gshhx5585w6mpusq3xsj3ayzf4uv6" + }, + "ConsumerPendingPacketQueueSize": null, + "RegisteredConsumerRewardDenoms": null, + "ClientsFrozenHeights": null, + "Proposals": null + } + } }, { "ActionType": "main.AssignConsumerPubKeyAction", diff --git a/tests/e2e/tracehandler_testdata/democracyRewardsSteps.json b/tests/e2e/tracehandler_testdata/democracyRewardsSteps.json index 7e6d90cace..1235c47a22 100644 --- a/tests/e2e/tracehandler_testdata/democracyRewardsSteps.json +++ b/tests/e2e/tracehandler_testdata/democracyRewardsSteps.json @@ -134,10 +134,30 @@ "Validator": "carol", "ConsumerPubkey": "{\"@type\":\"/cosmos.crypto.ed25519.PubKey\",\"key\":\"Ui5Gf1+mtWUdH8u3xlmzdKID+F3PK0sfXZ73GZ6q6is=\"}", "ReconfigureNode": false, - "ExpectError": true, - "ExpectedError": "a validator has assigned the consumer key already: consumer key is already in use by a validator" + "ExpectError": false, + "ExpectedError": "" }, - "State": {} + "State": { + "democ": { + "ValBalances": null, + "ProposedConsumerChains": null, + "ValPowers": null, + "StakedTokens": null, + "Params": null, + "Rewards": null, + "ConsumerChains": null, + "AssignedKeys": { + "carol": "cosmosvalcons1kswr5sq599365kcjmhgufevfps9njf43e4lwdk" + }, + "ProviderKeys": { + "carol": "cosmosvalcons1ezyrq65s3gshhx5585w6mpusq3xsj3ayzf4uv6" + }, + "ConsumerPendingPacketQueueSize": null, + "RegisteredConsumerRewardDenoms": null, + "ClientsFrozenHeights": null, + "Proposals": null + } + } }, { "ActionType": "main.AssignConsumerPubKeyAction", diff --git a/tests/e2e/tracehandler_testdata/happyPath.json b/tests/e2e/tracehandler_testdata/happyPath.json index 5b9505e848..04afc707a6 100644 --- a/tests/e2e/tracehandler_testdata/happyPath.json +++ b/tests/e2e/tracehandler_testdata/happyPath.json @@ -134,10 +134,30 @@ "Validator": "carol", "ConsumerPubkey": "{\"@type\":\"/cosmos.crypto.ed25519.PubKey\",\"key\":\"Ui5Gf1+mtWUdH8u3xlmzdKID+F3PK0sfXZ73GZ6q6is=\"}", "ReconfigureNode": false, - "ExpectError": true, - "ExpectedError": "a validator has assigned the consumer key already: consumer key is already in use by a validator" + "ExpectError": false, + "ExpectedError": "" }, - "State": {} + "State": { + "consu": { + "ValBalances": null, + "ProposedConsumerChains": null, + "ValPowers": null, + "StakedTokens": null, + "Params": null, + "Rewards": null, + "ConsumerChains": null, + "AssignedKeys": { + "carol": "cosmosvalcons1kswr5sq599365kcjmhgufevfps9njf43e4lwdk" + }, + "ProviderKeys": { + "carol": "cosmosvalcons1ezyrq65s3gshhx5585w6mpusq3xsj3ayzf4uv6" + }, + "ConsumerPendingPacketQueueSize": null, + "RegisteredConsumerRewardDenoms": null, + "ClientsFrozenHeights": null, + "Proposals": null + } + } }, { "ActionType": "main.AssignConsumerPubKeyAction", diff --git a/tests/e2e/tracehandler_testdata/multipleConsumers.json b/tests/e2e/tracehandler_testdata/multipleConsumers.json index fdb69d1e47..610e8a786f 100644 --- a/tests/e2e/tracehandler_testdata/multipleConsumers.json +++ b/tests/e2e/tracehandler_testdata/multipleConsumers.json @@ -134,10 +134,30 @@ "Validator": "carol", "ConsumerPubkey": "{\"@type\":\"/cosmos.crypto.ed25519.PubKey\",\"key\":\"Ui5Gf1+mtWUdH8u3xlmzdKID+F3PK0sfXZ73GZ6q6is=\"}", "ReconfigureNode": false, - "ExpectError": true, - "ExpectedError": "a validator has assigned the consumer key already: consumer key is already in use by a validator" + "ExpectError": false, + "ExpectedError": "" }, - "State": {} + "State": { + "consu": { + "ValBalances": null, + "ProposedConsumerChains": null, + "ValPowers": null, + "StakedTokens": null, + "Params": null, + "Rewards": null, + "ConsumerChains": null, + "AssignedKeys": { + "carol": "cosmosvalcons1kswr5sq599365kcjmhgufevfps9njf43e4lwdk" + }, + "ProviderKeys": { + "carol": "cosmosvalcons1ezyrq65s3gshhx5585w6mpusq3xsj3ayzf4uv6" + }, + "ConsumerPendingPacketQueueSize": null, + "RegisteredConsumerRewardDenoms": null, + "ClientsFrozenHeights": null, + "Proposals": null + } + } }, { "ActionType": "main.AssignConsumerPubKeyAction", @@ -399,10 +419,30 @@ "Validator": "carol", "ConsumerPubkey": "{\"@type\":\"/cosmos.crypto.ed25519.PubKey\",\"key\":\"Ui5Gf1+mtWUdH8u3xlmzdKID+F3PK0sfXZ73GZ6q6is=\"}", "ReconfigureNode": false, - "ExpectError": true, - "ExpectedError": "a validator has assigned the consumer key already: consumer key is already in use by a validator" + "ExpectError": false, + "ExpectedError": "" }, - "State": {} + "State": { + "densu": { + "ValBalances": null, + "ProposedConsumerChains": null, + "ValPowers": null, + "StakedTokens": null, + "Params": null, + "Rewards": null, + "ConsumerChains": null, + "AssignedKeys": { + "carol": "cosmosvalcons1kswr5sq599365kcjmhgufevfps9njf43e4lwdk" + }, + "ProviderKeys": { + "carol": "cosmosvalcons1ezyrq65s3gshhx5585w6mpusq3xsj3ayzf4uv6" + }, + "ConsumerPendingPacketQueueSize": null, + "RegisteredConsumerRewardDenoms": null, + "ClientsFrozenHeights": null, + "Proposals": null + } + } }, { "ActionType": "main.AssignConsumerPubKeyAction", diff --git a/tests/e2e/tracehandler_testdata/shorthappy.json b/tests/e2e/tracehandler_testdata/shorthappy.json index 607c1d6d7c..1be75865aa 100644 --- a/tests/e2e/tracehandler_testdata/shorthappy.json +++ b/tests/e2e/tracehandler_testdata/shorthappy.json @@ -134,10 +134,30 @@ "Validator": "carol", "ConsumerPubkey": "{\"@type\":\"/cosmos.crypto.ed25519.PubKey\",\"key\":\"Ui5Gf1+mtWUdH8u3xlmzdKID+F3PK0sfXZ73GZ6q6is=\"}", "ReconfigureNode": false, - "ExpectError": true, - "ExpectedError": "a validator has assigned the consumer key already: consumer key is already in use by a validator" + "ExpectError": false, + "ExpectedError": "" }, - "State": {} + "State": { + "consu": { + "ValBalances": null, + "ProposedConsumerChains": null, + "ValPowers": null, + "StakedTokens": null, + "Params": null, + "Rewards": null, + "ConsumerChains": null, + "AssignedKeys": { + "carol": "cosmosvalcons1kswr5sq599365kcjmhgufevfps9njf43e4lwdk" + }, + "ProviderKeys": { + "carol": "cosmosvalcons1ezyrq65s3gshhx5585w6mpusq3xsj3ayzf4uv6" + }, + "ConsumerPendingPacketQueueSize": null, + "RegisteredConsumerRewardDenoms": null, + "ClientsFrozenHeights": null, + "Proposals": null + } + } }, { "ActionType": "main.AssignConsumerPubKeyAction", diff --git a/tests/e2e/tracehandler_testdata/slashThrottle.json b/tests/e2e/tracehandler_testdata/slashThrottle.json index e99e7e2973..352f7cc06d 100644 --- a/tests/e2e/tracehandler_testdata/slashThrottle.json +++ b/tests/e2e/tracehandler_testdata/slashThrottle.json @@ -134,10 +134,30 @@ "Validator": "carol", "ConsumerPubkey": "{\"@type\":\"/cosmos.crypto.ed25519.PubKey\",\"key\":\"Ui5Gf1+mtWUdH8u3xlmzdKID+F3PK0sfXZ73GZ6q6is=\"}", "ReconfigureNode": false, - "ExpectError": true, - "ExpectedError": "a validator has assigned the consumer key already: consumer key is already in use by a validator" + "ExpectError": false, + "ExpectedError": "" }, - "State": {} + "State": { + "consu": { + "ValBalances": null, + "ProposedConsumerChains": null, + "ValPowers": null, + "StakedTokens": null, + "Params": null, + "Rewards": null, + "ConsumerChains": null, + "AssignedKeys": { + "carol": "cosmosvalcons1kswr5sq599365kcjmhgufevfps9njf43e4lwdk" + }, + "ProviderKeys": { + "carol": "cosmosvalcons1ezyrq65s3gshhx5585w6mpusq3xsj3ayzf4uv6" + }, + "ConsumerPendingPacketQueueSize": null, + "RegisteredConsumerRewardDenoms": null, + "ClientsFrozenHeights": null, + "Proposals": null + } + } }, { "ActionType": "main.AssignConsumerPubKeyAction", diff --git a/tests/integration/key_assignment.go b/tests/integration/key_assignment.go index ab6cb63146..799109c0d4 100644 --- a/tests/integration/key_assignment.go +++ b/tests/integration/key_assignment.go @@ -79,7 +79,7 @@ func (s *CCVTestSuite) TestKeyAssignment() { }, false, 2, }, { - "double same-key assignment in same block", func(pk *providerkeeper.Keeper) error { + "double same-key assignment in same block by different vals", func(pk *providerkeeper.Keeper) error { // establish CCV channel s.SetupCCVChannel(s.path) @@ -90,8 +90,9 @@ func (s *CCVTestSuite) TestKeyAssignment() { return err } - // same key assignment - err = pk.AssignConsumerKey(s.providerCtx(), s.consumerChain.ChainID, validator, consumerKey) + // same key assignment, but different validator + validator2, _ := generateNewConsumerKey(s, 1) + err = pk.AssignConsumerKey(s.providerCtx(), s.consumerChain.ChainID, validator2, consumerKey) if err != nil { return err } @@ -100,6 +101,28 @@ func (s *CCVTestSuite) TestKeyAssignment() { return nil }, true, 2, }, + { + "double same-key assignment in same block by same val", func(pk *providerkeeper.Keeper) error { + // establish CCV channel + s.SetupCCVChannel(s.path) + + // key assignment + validator, consumerKey := generateNewConsumerKey(s, 0) + err := pk.AssignConsumerKey(s.providerCtx(), s.consumerChain.ChainID, validator, consumerKey) + if err != nil { + return err + } + + // same key assignment, but different validator + err = pk.AssignConsumerKey(s.providerCtx(), s.consumerChain.ChainID, validator, consumerKey) + if err != nil { + return err + } + s.nextEpoch() + + return nil + }, false, 2, + }, { "double key assignment in same block", func(pk *providerkeeper.Keeper) error { // establish CCV channel @@ -124,7 +147,7 @@ func (s *CCVTestSuite) TestKeyAssignment() { }, false, 2, }, { - "double same-key assignment in different blocks", func(pk *providerkeeper.Keeper) error { + "double same-key assignment in different blocks by different vals", func(pk *providerkeeper.Keeper) error { // establish CCV channel s.SetupCCVChannel(s.path) @@ -137,7 +160,8 @@ func (s *CCVTestSuite) TestKeyAssignment() { s.nextEpoch() // same key assignment - err = pk.AssignConsumerKey(s.providerCtx(), s.consumerChain.ChainID, validator, consumerKey) + validator2, _ := generateNewConsumerKey(s, 1) + err = pk.AssignConsumerKey(s.providerCtx(), s.consumerChain.ChainID, validator2, consumerKey) if err != nil { return err } @@ -146,6 +170,29 @@ func (s *CCVTestSuite) TestKeyAssignment() { return nil }, true, 2, }, + { + "double same-key assignment in different blocks by same val", func(pk *providerkeeper.Keeper) error { + // establish CCV channel + s.SetupCCVChannel(s.path) + + // key assignment + validator, consumerKey := generateNewConsumerKey(s, 0) + err := pk.AssignConsumerKey(s.providerCtx(), s.consumerChain.ChainID, validator, consumerKey) + if err != nil { + return err + } + s.nextEpoch() + + // same key assignment + err = pk.AssignConsumerKey(s.providerCtx(), s.consumerChain.ChainID, validator, consumerKey) + if err != nil { + return err + } + s.nextEpoch() + + return nil + }, false, 2, + }, { "double key assignment in different blocks", func(pk *providerkeeper.Keeper) error { // establish CCV channel diff --git a/x/ccv/provider/handler_test.go b/x/ccv/provider/handler_test.go index 8cefa3f949..6e13511e3b 100644 --- a/x/ccv/provider/handler_test.go +++ b/x/ccv/provider/handler_test.go @@ -34,6 +34,10 @@ func TestAssignConsensusKeyForConsumerChain(t *testing.T) { providerCryptoId := testcrypto.NewCryptoIdentityFromIntSeed(0) providerConsAddr := providerCryptoId.ProviderConsAddress() + // a different providerConsAddr, to simulate different validators having assigned keys + providerCryptoId2 := testcrypto.NewCryptoIdentityFromIntSeed(10) + providerConsAddr2 := providerCryptoId2.ProviderConsAddress() + consumerCryptoId := testcrypto.NewCryptoIdentityFromIntSeed(1) consumerConsAddr := consumerCryptoId.ConsumerConsAddress() consumerKeyBz := base64.StdEncoding.EncodeToString(consumerCryptoId.ConsensusSDKPubKey().Bytes()) @@ -101,7 +105,32 @@ func TestAssignConsensusKeyForConsumerChain(t *testing.T) { chainID: "chainid", }, { - name: "fail: consumer key in use", + name: "fail: consumer key in use by other validator", + setup: func(ctx sdk.Context, + k keeper.Keeper, mocks testkeeper.MockedKeepers, + ) { + k.SetPendingConsumerAdditionProp(ctx, &providertypes.ConsumerAdditionProposal{ + ChainId: "chainid", + }) + // Use the consumer key already used by some other validator + k.SetValidatorByConsumerAddr(ctx, "chainid", consumerConsAddr, providerConsAddr2) + + gomock.InOrder( + mocks.MockStakingKeeper.EXPECT().GetValidator( + ctx, providerCryptoId.SDKValOpAddress(), + // validator should not be missing + ).Return(providerCryptoId.SDKStakingValidator(), true).Times(1), + mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, + consumerConsAddr.ToSdkConsAddr(), + // return false - no other validator uses the consumer key to validate *on the provider* + ).Return(stakingtypes.Validator{}, false), + ) + }, + expError: true, + chainID: "chainid", + }, + { + name: "success: consumer key in use, but by the same validator", setup: func(ctx sdk.Context, k keeper.Keeper, mocks testkeeper.MockedKeepers, ) { @@ -121,7 +150,7 @@ func TestAssignConsensusKeyForConsumerChain(t *testing.T) { ).Return(stakingtypes.Validator{}, false), ) }, - expError: true, + expError: false, chainID: "chainid", }, } diff --git a/x/ccv/provider/keeper/key_assignment.go b/x/ccv/provider/keeper/key_assignment.go index 983b92cb26..67f4d89fdd 100644 --- a/x/ccv/provider/keeper/key_assignment.go +++ b/x/ccv/provider/keeper/key_assignment.go @@ -371,12 +371,24 @@ func (k Keeper) AssignConsumerKey( } } - if _, found := k.GetValidatorByConsumerAddr(ctx, chainID, consumerAddr); found { + if existingProviderAddr, found := k.GetValidatorByConsumerAddr(ctx, chainID, consumerAddr); found { // consumer key is already in use - // prevent multiple validators from assigning the same key - return errorsmod.Wrapf( - types.ErrConsumerKeyInUse, "a validator has assigned the consumer key already", - ) + if providerAddr.Address.Equals(existingProviderAddr.Address) { + // the validator itself is the one already using the consumer key, + // just do a noop + k.Logger(ctx).Info("tried to assign a consumer key that is already assigned to the validator", + "consumer chainID", chainID, + "validator", providerAddr.String(), + "consumer consensus addr", consumerAddr.String(), + ) + return nil + } else { + // the validators are different -> throw an error to + // prevent multiple validators from assigning the same key + return errorsmod.Wrapf( + types.ErrConsumerKeyInUse, "a validator has assigned the consumer key already", + ) + } } // get the previous key assigned for this validator on this consumer chain