From 85cfcc56b7ded20c9d95c39d98b892d824e90eec Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Tue, 26 Mar 2024 15:21:09 +0100 Subject: [PATCH 01/10] Make the same validator assigning the same key a noop instead of an error --- x/ccv/provider/keeper/key_assignment.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/x/ccv/provider/keeper/key_assignment.go b/x/ccv/provider/keeper/key_assignment.go index 983b92cb26..bc2dfed47b 100644 --- a/x/ccv/provider/keeper/key_assignment.go +++ b/x/ccv/provider/keeper/key_assignment.go @@ -371,12 +371,19 @@ 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 + 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 From ec9d2722c124d300835fc097f8bcd66b384eec13 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Tue, 26 Mar 2024 15:38:44 +0100 Subject: [PATCH 02/10] Adjust test --- x/ccv/provider/handler_test.go | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/x/ccv/provider/handler_test.go b/x/ccv/provider/handler_test.go index 8cefa3f949..243bd053ba 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 := providerCryptoId.ProviderConsAddress() + consumerCryptoId := testcrypto.NewCryptoIdentityFromIntSeed(1) consumerConsAddr := consumerCryptoId.ConsumerConsAddress() consumerKeyBz := base64.StdEncoding.EncodeToString(consumerCryptoId.ConsensusSDKPubKey().Bytes()) @@ -101,7 +105,31 @@ 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 + k.SetValidatorByConsumerAddr(ctx, "chainid", consumerConsAddr, providerConsAddr2) + + gomock.InOrder( + mocks.MockStakingKeeper.EXPECT().GetValidator( + ctx, providerCryptoId.SDKValOpAddress(), + // Return a valid validator, found! + ).Return(providerCryptoId2.SDKStakingValidator(), true).Times(1), + mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, + consumerConsAddr.ToSdkConsAddr(), + ).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 +149,7 @@ func TestAssignConsensusKeyForConsumerChain(t *testing.T) { ).Return(stakingtypes.Validator{}, false), ) }, - expError: true, + expError: false, chainID: "chainid", }, } From 28eb93bb0a7297319d777d94c80bbf47236379d0 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Wed, 27 Mar 2024 09:35:57 +0100 Subject: [PATCH 03/10] Update tests --- tests/e2e/steps_compatibility.go | 12 ------ tests/e2e/steps_start_chains.go | 16 ++++++-- tests/integration/key_assignment.go | 57 ++++++++++++++++++++++++++--- 3 files changed, 64 insertions(+), 21 deletions(-) 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/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 From 6d05156767598b8021127de218673510e313e7c0 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Wed, 27 Mar 2024 09:42:34 +0100 Subject: [PATCH 04/10] Fix newline warning --- tests/e2e/main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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() } From 170e4ea10db75431698d38203bb1ea019017c231 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Wed, 27 Mar 2024 09:42:46 +0100 Subject: [PATCH 05/10] Regenerate traces --- .../consumer-double-sign.json | 26 ++++++++-- .../e2e/tracehandler_testdata/democracy.json | 26 ++++++++-- .../democracyRewardsSteps.json | 26 ++++++++-- .../e2e/tracehandler_testdata/happyPath.json | 26 ++++++++-- .../multipleConsumers.json | 52 ++++++++++++++++--- .../e2e/tracehandler_testdata/shorthappy.json | 26 ++++++++-- .../tracehandler_testdata/slashThrottle.json | 26 ++++++++-- 7 files changed, 184 insertions(+), 24 deletions(-) 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", From 6f982ea0e549c3712d87eb160d2f2ff1a434dbf6 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Wed, 27 Mar 2024 09:53:05 +0100 Subject: [PATCH 06/10] Add key assignment change to changelog --- .../provider/1732-assigning-already-assigned-key-fix.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/state-breaking/provider/1732-assigning-already-assigned-key-fix.md 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 From b17faeeb391956dee74d520ff141a1de491801d3 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Wed, 27 Mar 2024 14:03:02 +0100 Subject: [PATCH 07/10] Add info log for same key same validator assignments --- x/ccv/provider/keeper/key_assignment.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/x/ccv/provider/keeper/key_assignment.go b/x/ccv/provider/keeper/key_assignment.go index bc2dfed47b..67f4d89fdd 100644 --- a/x/ccv/provider/keeper/key_assignment.go +++ b/x/ccv/provider/keeper/key_assignment.go @@ -376,6 +376,11 @@ func (k Keeper) AssignConsumerKey( 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 From 4eb736b5351f12bc023a46947ebc58b458ae1409 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Thu, 28 Mar 2024 09:44:01 +0100 Subject: [PATCH 08/10] Add changelog entry to api-breaking --- .../provider/1732-assigning-already-assigned-key-fix.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/api-breaking/provider/1732-assigning-already-assigned-key-fix.md 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 From cec40b1326b169588cade41d9f3c239af1d891d7 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt <57488781+p-offtermatt@users.noreply.github.com> Date: Thu, 28 Mar 2024 10:40:51 +0100 Subject: [PATCH 09/10] Update x/ccv/provider/handler_test.go Co-authored-by: insumity --- x/ccv/provider/handler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/ccv/provider/handler_test.go b/x/ccv/provider/handler_test.go index 243bd053ba..11150d52b3 100644 --- a/x/ccv/provider/handler_test.go +++ b/x/ccv/provider/handler_test.go @@ -112,7 +112,7 @@ func TestAssignConsensusKeyForConsumerChain(t *testing.T) { k.SetPendingConsumerAdditionProp(ctx, &providertypes.ConsumerAdditionProposal{ ChainId: "chainid", }) - // Use the consumer key already + // Use the consumer key already used by some other validator k.SetValidatorByConsumerAddr(ctx, "chainid", consumerConsAddr, providerConsAddr2) gomock.InOrder( From d9d4a20d96a7415b9da15980c168a77e90283e78 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Tue, 2 Apr 2024 09:14:47 +0200 Subject: [PATCH 10/10] Add more comments to test and return right validator --- x/ccv/provider/handler_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/x/ccv/provider/handler_test.go b/x/ccv/provider/handler_test.go index 11150d52b3..6e13511e3b 100644 --- a/x/ccv/provider/handler_test.go +++ b/x/ccv/provider/handler_test.go @@ -36,7 +36,7 @@ func TestAssignConsensusKeyForConsumerChain(t *testing.T) { // a different providerConsAddr, to simulate different validators having assigned keys providerCryptoId2 := testcrypto.NewCryptoIdentityFromIntSeed(10) - providerConsAddr2 := providerCryptoId.ProviderConsAddress() + providerConsAddr2 := providerCryptoId2.ProviderConsAddress() consumerCryptoId := testcrypto.NewCryptoIdentityFromIntSeed(1) consumerConsAddr := consumerCryptoId.ConsumerConsAddress() @@ -118,10 +118,11 @@ func TestAssignConsensusKeyForConsumerChain(t *testing.T) { gomock.InOrder( mocks.MockStakingKeeper.EXPECT().GetValidator( ctx, providerCryptoId.SDKValOpAddress(), - // Return a valid validator, found! - ).Return(providerCryptoId2.SDKStakingValidator(), true).Times(1), + // 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), ) },