Onchain upgrade to consumer chain from sovereign chain #697

jstr1121 · 2023-02-02T01:36:57Z

Description

This PR is supposed to upgrade intechain-security module for on-chain upgrade from sovereign to consumer chain.
Applying the changes mentioned in cosmos/ibc#840

Changes

Protobuf modification
democracy app to use consumer keeper's params to see if consumer is enabled.
PreCCV check on consumer InitGenesis to consider using staking module
Unit test for changes on Endblocker
Slashing sovereign validators for the fault made before consumer chain transition

Type of change

If you've checked more than one of the first three boxes, consider splitting this PR into multiple PRs!

Feature: Changes and/or adds code behavior, irrelevant to bug fixes
Fix: Changes and/or adds code behavior, specifically to fix a bug
Refactor: Changes existing code style, naming, structure, etc.
Testing: Adds testing
Docs: Adds documentation

Regression tests

If Refactor, describe the new or existing tests that verify no behavior was changed or added where refactors were introduced.

New behavior tests

If Feature or Fix, describe the new or existing tests that verify the new behavior is correct and expected.

…sion height

…nsumer chain upgrade height

yaruwangway · 2023-02-02T11:53:34Z

app/consumer-democracy/app.go

-		storeUpgrades := store.StoreUpgrades{}
+		storeUpgrades := store.StoreUpgrades{
+			Added: []string{ibcconsumertypes.ModuleName},
+		}



if IsSkipHeight(upgradeInfo.Height) { return }

yaruwangway · 2023-02-02T12:02:13Z

app/sovereign/export.go

+		if err != nil {
+			panic(err)
+		}
+		_, _ = app.DistrKeeper.WithdrawDelegationRewards(ctx, delAddr, valAddr)


why not check err and panic here ?

yaruwangway · 2023-02-02T12:04:58Z

app/sovereign/export.go

+	_, err := app.StakingKeeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	if err != nil {
+		log.Fatal(err)
+	}


Hi, about store iterator, please check this issue in gaia, cosmos/gaia#1924
please let me know your opinion.

app/sovereign/upgrades/v3/upgrades.go

shaspitz · 2023-02-17T23:11:48Z

Hi @jstr1121, thanks again for your code! We've merged a lot of PRs over the past few weeks, so it looks like this branch needs to catch up to main. Let's eventually work together on making an ADR for the implementation of this feature. It would also be useful to setup a 30-60 minute code walkthrough of how the implementation works, and why certain design decisions were made. Would sometime next week work for you? Let's chat on slack

jstr1121 · 2023-02-21T14:43:37Z

Hi @jstr1121, thanks again for your code! We've merged a lot of PRs over the past few weeks, so it looks like this branch needs to catch up to main. Let's eventually work together on making an ADR for the implementation of this feature. It would also be useful to setup a 30-60 minute code walkthrough of how the implementation works, and why certain design decisions were made. Would sometime next week work for you? Let's chat on slack

Will get the conflict fixed soon - I am fine for 30-60 mins call this week, we can schedule a time on slack.

…o jstr/onchain_upgrade_to_consumer_chain_840

…merge conflict fix issue

Co-authored-by: yaruwangway <[email protected]>

shaspitz

Great stuff!

shaspitz · 2023-02-23T00:46:59Z

app/consumer-democracy/app.go

@@ -506,6 +509,8 @@ func New(
 		distrtypes.ModuleName,
 		slashingtypes.ModuleName,
 		evidencetypes.ModuleName,
+		consumertypes.ModuleName, // Note: consumer beginblocker before staking module


I think changes like this would need to be upstreamed to all consumer chains that pattern match this file. No action item for the Stride team, just commenting for awareness on our team

shaspitz · 2023-02-23T00:58:06Z

app/consumer/app.go

@@ -334,6 +334,7 @@ func New(
 		&app.IBCKeeper.PortKeeper,
 		app.IBCKeeper.ConnectionKeeper,
 		app.IBCKeeper.ClientKeeper,
+		nil,


See

interchain-security/x/ccv/consumer/keeper/keeper.go

Line 85 in afd1b2b

func (k Keeper) mustValidateFields() {

on this keeper being nil. Maybe we can add a flag likeisDemocConsumer to make things very clear

tests/sovereign_consumer_upgrade_local/hermes-0.15.0-old-connection/start_provider.sh

shaspitz · 2023-02-23T01:38:00Z

x/ccv/consumer/keeper/keeper.go

+	return false
+}
+
+func (k Keeper) SetPreCCV(ctx sdk.Context, state *types.GenesisState) {


We usually make quick unit tests for CRUD operations like these, especially because this method is not just setting a flag

shaspitz · 2023-02-23T01:47:28Z

x/ccv/consumer/keeper/validators.go

-	panic("unimplemented on CCV keeper")
+	if k.IsPreCCV(ctx) || ctx.BlockHeight() <= k.LastSovereignHeight(ctx) {
+		if k.stakingKeeper == nil {
+			panic("empty staking keeper")


Should we panic or return in cases like this?

sainoe · 2023-02-24T13:30:06Z

x/ccv/consumer/keeper/validators.go

+		}
+
+		return k.stakingKeeper.ValidatorByConsAddr(ctx, consAddr)
+	}
 	return stakingtypes.Validator{}
 }


nit: it'd be cleaner to pass a stakingtypes.InfractionEmpty infraction type here.

sainoe · 2023-02-24T13:40:50Z

x/ccv/consumer/keeper/validators.go

+		if k.stakingKeeper == nil {
+			return
+		}
+


This error message is hard to understand without context imo.

shaspitz · 2023-02-27T17:19:03Z

app/consumer/app.go

@@ -344,7 +345,7 @@ func New(

 	// register slashing module Slashing hooks to the consumer keeper
 	app.ConsumerKeeper = *app.ConsumerKeeper.SetHooks(app.SlashingKeeper.Hooks())
-	consumerModule := ibcconsumer.NewAppModule(app.ConsumerKeeper)
+	consumerModule := ibcconsumer.NewAppModule(app.ConsumerKeeper, nil)


Could use a comment here

shaspitz · 2023-02-27T17:28:14Z

x/ccv/consumer/keeper/genesis.go


+	if state.PreCCV {
+		return []abci.ValidatorUpdate{}


Could use comment that ccv module shouldn't be doing anything yet because staking keeper is still active

shaspitz · 2023-02-27T17:31:01Z

x/ccv/consumer/keeper/validators.go

-func (k Keeper) IterateValidators(sdk.Context, func(index int64, validator stakingtypes.ValidatorI) (stop bool)) {
+// IterateValidators iterate democracy staking validators when the block height is before the last sovereign height
+func (k Keeper) IterateValidators(ctx sdk.Context, f func(index int64, validator stakingtypes.ValidatorI) (stop bool)) {
+	if k.IsPreCCV(ctx) || ctx.BlockHeight() <= k.LastSovereignHeight(ctx) {


Could use comment on what edge case this covers

The idea is that all the calls to the staking module are directed to the consumer CCV module as it plays the role of the staking module, even though until the chain fully transitions to be a consumer chain the staking module should be the original staking module.

So basically, once the consumer CCV module is added, the democracy-staking module is also added. In the first block after the upgrade, the democracy-staking acts exactly as the original staking module as the preCCV flag is set (see democracy/staking/module.go). In the EndBlock of the first block, the preCCV flag is unset, which means that the democracy-staking module stops sending updates to TM and the consumer CCV module starts sending these updates.

However, the provider validators are producing blocks two heights later due to the +1 delay of TM (see ABCI spec). Thus, all the conditions for this edge case are wrong.

shaspitz · 2023-02-27T17:31:33Z

x/ccv/consumer/keeper/validators.go

 }

 // IsJailed returns the outstanding slashing flag for the given validator adddress
 func (k Keeper) IsValidatorJailed(ctx sdk.Context, addr sdk.ConsAddress) bool {
+	if k.IsPreCCV(ctx) || ctx.BlockHeight() <= k.LastSovereignHeight(ctx) {


Same here on edge case comment

shaspitz · 2023-02-27T17:31:43Z

x/ccv/consumer/keeper/validators.go

@@ -89,16 +108,32 @@ func (k Keeper) ValidatorByConsAddr(sdk.Context, sdk.ConsAddress) stakingtypes.V
 		Also, the slashing module will cal lthis function when it observes downtime. In that case
 		the only requirement on the returned value is that it isn't null.
 	*/
+	if k.IsPreCCV(ctx) || ctx.BlockHeight() <= k.LastSovereignHeight(ctx) {


shaspitz · 2023-02-27T17:31:56Z

x/ccv/consumer/keeper/validators.go

 	if infraction == stakingtypes.InfractionEmpty {
 		return
 	}

+	if k.IsPreCCV(ctx) || ctx.BlockHeight() <= k.LastSovereignHeight(ctx) {


same, maybe we just make this a func to be more readable

shaspitz · 2023-03-01T00:40:25Z

Slashing sovereign validators for the fault made before consumer chain transition

I'm having trouble finding the section of code this is located at

mpoke · 2023-03-02T18:14:42Z

x/ccv/consumer/module.go

+		// populate cross chain validators states with initial valset
+		am.keeper.ApplyCCValidatorChanges(ctx, initialValSet)
+
+		initialSetFlag := make(map[string]bool)


@smarshall-spitzbart please add a comment here explaining the logic.

I'll add "Add validator updates to initialValSet, such that the "old" validators returned from GetLastValidators are given zero power, and the "new" validators are given their full power."

mpoke · 2023-03-02T18:33:43Z

x/ccv/consumer/keeper/validators.go

 	if infraction == stakingtypes.InfractionEmpty {
 		return
 	}

+	if k.IsPreCCV(ctx) || ctx.BlockHeight() <= k.LastSovereignHeight(ctx) {


Can never be the case that ctx.BlockHeight() < k.LastSovereignHeight(ctx) as the CCV module was not on the chain.

This is not the correct way to deal with slashing. This slash request could be for one of the following cases:

An infraction committed by a sovereign chain validator. This can be detected via the infraction height. The sovereign chain validator set is active until the height h (inclusive). Note that h != LastSovereignHeight. LastSovereignHeight is the height the provider validator set was sent to TM. According to the ABCI spec, h = LastSovereignHeight + 1. In this case, the validator should be slashed if the unbonding period didn't pass yet. Still need to implement a way in democracy-staking that not slash validators after unbonding period elapsed after the switch.

An infraction committed by a provider validator. Again, the infraction height can be used for detection.

mpoke · 2023-03-02T18:43:17Z

x/ccv/consumer/module.go

 	return AppModule{
 		keeper: k,
+		sk:     sk,


Why do we need this? The DemocracyStakingKeeper can be access via am.keeper.stakingKeeper

This has to do with the capabilities model, am.keeper.stakingKeeper is non-exported and not accessible.

mpoke · 2023-03-02T18:46:04Z

app/consumer-democracy/app.go

@@ -506,6 +509,8 @@ func New(
 		distrtypes.ModuleName,
 		slashingtypes.ModuleName,
 		evidencetypes.ModuleName,
+		consumertypes.ModuleName, // Note: consumer beginblocker before staking module


@jstr1121 Why is this order necessary?

Beginblocker order shouldn't matter AFAIK

mpoke · 2023-03-02T18:47:11Z

app/consumer-democracy/app.go

 	)
 	app.MM.SetOrderEndBlockers(
 		crisistypes.ModuleName,
 		govtypes.ModuleName,
+		consumertypes.ModuleName, // Note: consumer endblocker before staking module


@jstr1121 Why is this order necessary?

With 1269163, the order doesn't matter AFAIK

mpoke · 2023-03-02T18:48:46Z

app/consumer-democracy/app.go

@@ -550,6 +554,7 @@ func New(
 		capabilitytypes.ModuleName,
 		authtypes.ModuleName,
 		banktypes.ModuleName,
+		consumertypes.ModuleName, // Note: consumer initiation before staking module


@jstr1121 Why is this order necessary?

Same here, InitGenesis shouldn't matter unless I'm missing something

app/consumer-democracy/app.go

mpoke · 2023-03-02T18:53:54Z

app/consumer-democracy/app.go

+			// TODO: should have a way to read from current node home
+			userHomeDir, err := os.UserHomeDir()
+			if err != nil {
+				stdlog.Println("Failed to get home dir %2", err)
+			}
+			nodeHome := userHomeDir + "/.sovereign/config/genesis.json"
+			appState, _, err := genutiltypes.GenesisStateFromGenFile(nodeHome)
+			if err != nil {
+				return fromVM, fmt.Errorf("failed to unmarshal genesis state: %w", err)
+			}
+
+			var consumerGenesis = consumertypes.GenesisState{}
+			appCodec.MustUnmarshalJSON(appState[consumertypes.ModuleName], &consumerGenesis)
+
+			consumerGenesis.PreCCV = true
+			app.ConsumerKeeper.InitGenesis(ctx, &consumerGenesis)
+


This is needed only because the provider doesn't create a correct init genesis for the consumer CCV module with preCCV set to true. See the spec for more details. @smarshall-spitzbart please open an issue to address this.

mpoke · 2023-03-02T18:55:44Z

app/consumer-democracy/app.go

+		storeUpgrades := store.StoreUpgrades{
+			Added: []string{consumertypes.ModuleName},
+		}



@jstr1121 What are these changes for?

mpoke · 2023-03-02T18:57:34Z

app/sovereign/app.go

@@ -0,0 +1,808 @@
+package app


Why do we need to have the app.go for the sovereign chain in this repo?

mpoke · 2023-03-02T18:58:40Z

x/ccv/consumer/keeper/genesis.go

+	// PreCCV is true when consumer chain used to be running on non-consumer chain, and when it is in the progress of upgrading
+	// to consumer chain, where consumer chain upgrade is done.
+	if state.PreCCV {
+		k.SetPreCCV(ctx, state)


@smarshall-spitzbart please store the init val set explicitly.

mpoke · 2023-03-02T19:11:30Z

x/ccv/consumer/keeper/validators.go

-func (k Keeper) Jail(ctx sdk.Context, addr sdk.ConsAddress) {}
+// Jail performs jail operation on democracy staking validator if block height after last sovereign height
+func (k Keeper) Jail(ctx sdk.Context, addr sdk.ConsAddress) {
+	if k.IsPreCCV(ctx) || ctx.BlockHeight() <= k.LastSovereignHeight(ctx) {


ditto: see comments from Slash

mpoke · 2023-03-02T19:13:29Z

x/ccv/consumer/keeper/validators.go

+
+// Unjail performs unjail operation on democracy staking validator if block height after last sovereign height
+func (k Keeper) Unjail(ctx sdk.Context, addr sdk.ConsAddress) {
+	if k.IsPreCCV(ctx) || ctx.BlockHeight() <= k.LastSovereignHeight(ctx) {


ditto: see comments from Slash

mpoke · 2023-03-02T19:14:51Z

x/ccv/consumer/keeper/validators.go

-	panic("unimplemented on CCV keeper")
+// MaxValidators returns max number of validators on democracy staking if block height after last sovereign height
+func (k Keeper) MaxValidators(ctx sdk.Context) uint32 {
+	if k.IsPreCCV(ctx) || ctx.BlockHeight() <= k.LastSovereignHeight(ctx) {


mpoke · 2023-03-02T19:14:59Z

x/ccv/consumer/keeper/validators.go

-func (k Keeper) Unjail(sdk.Context, sdk.ConsAddress) {}
+// Delegation returns delegation from an address to a democracy staking validator if block height after last sovereign height
+func (k Keeper) Delegation(ctx sdk.Context, addr sdk.AccAddress, valAddr sdk.ValAddress) stakingtypes.DelegationI {
+	if k.IsPreCCV(ctx) || ctx.BlockHeight() <= k.LastSovereignHeight(ctx) {


mpoke · 2023-03-02T19:15:23Z

x/ccv/consumer/keeper/validators.go

@@ -89,16 +108,32 @@ func (k Keeper) ValidatorByConsAddr(sdk.Context, sdk.ConsAddress) stakingtypes.V
 		Also, the slashing module will cal lthis function when it observes downtime. In that case
 		the only requirement on the returned value is that it isn't null.
 	*/
+	if k.IsPreCCV(ctx) || ctx.BlockHeight() <= k.LastSovereignHeight(ctx) {


shaspitz · 2023-03-06T19:53:49Z

@jstr1121 your contributions are much appreciated! We're going to work off your commits in a branch native to the parent repository, tracked as #757

Consequently I'll close this PR in favor of the new one. Thanks again!

jstr1121 added 10 commits December 19, 2022 09:52

on-chain upgrade to consumer chain wip

40f3fdd

add preCCV store and use it on democracy staking

aa566c9

add TODOs and one more packet possibility

dc9768d

status update

473e467

Resolve hermes start issue for trusted validator set by changing revi…

c6803f1

…sion height

remove intermediary logs

15265ad

remove further unused codebase

2806d6f

updates for endblocker test, existing test fixes, get last validators

c3ae9a4

update for slashing sovereign validators for the fault made before co…

67aa39b

…nsumer chain upgrade height

resolve comments on github and slack communication

09f7b61

jstr1121 requested review from jtremback, mpoke, sainoe, shaspitz, MSalopek, mmulji-ic, glnro and yaruwangway as code owners February 2, 2023 01:36

yaruwangway reviewed Feb 2, 2023

View reviewed changes

app/sovereign/upgrades/v3/upgrades.go Outdated Show resolved Hide resolved

jstr1121 and others added 3 commits February 21, 2023 23:05

Merge branch 'main' of github.com:Stride-Labs/interchain-security int…

d7b5a7f

…o jstr/onchain_upgrade_to_consumer_chain_840

update sovereign app to use v4 ibc from v3 & resolve consumer module …

8aea5bf

…merge conflict fix issue

Update app/sovereign/upgrades/v3/upgrades.go

62da0d2

Co-authored-by: yaruwangway <[email protected]>

shaspitz reviewed Feb 23, 2023

View reviewed changes

sainoe reviewed Feb 24, 2023

View reviewed changes

shaspitz reviewed Mar 1, 2023

View reviewed changes

mpoke reviewed Mar 2, 2023

View reviewed changes

app/consumer-democracy/app.go Show resolved Hide resolved

mpoke reviewed Mar 2, 2023

View reviewed changes

app/sovereign/app.go

@@ -0,0 +1,808 @@

package app

Copy link

Contributor

mpoke Mar 2, 2023

Choose a reason for hiding this comment

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

Why do we need to have the app.go for the sovereign chain in this repo?

mpoke reviewed Mar 2, 2023

View reviewed changes

This was referenced Mar 6, 2023

EPIC: Standalone to consumer changeover #756

Closed

feat: standalone to consumer changeover part 1 #757

Merged

shaspitz closed this Mar 6, 2023

Onchain upgrade to consumer chain from sovereign chain #697

Onchain upgrade to consumer chain from sovereign chain #697

Conversation

jstr1121 commented Feb 2, 2023

Description

Changes

Type of change

Regression tests

New behavior tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaspitz commented Feb 17, 2023

jstr1121 commented Feb 21, 2023

shaspitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaspitz commented Mar 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpoke Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaspitz commented Mar 6, 2023

mpoke Mar 2, 2023 •

edited

Loading