remove unnecessary parts of app.go in consumer and provider #98

okwme · 2022-05-12T17:38:30Z

In removing unnecessary parts of the two app.go files it became apparent that there was still a dependence in the Consumer Chain on the staking module. Consumer chain should not have a staking module (at least not until democracy is added back with the ccv/staking module in issue #89 ). It uses the Consumer Module to handle all staking module duties. As such it fulfills the staking module interface and works with slashing. However there are other modules on a consumer chain that consume the staking module and are not happy when you swap in the consumer module instead.

IBCKeeper can't accept app.ConsumerKeeper in place of app.StakingKeeper because it's missing the method GetHistoricalInfo
app.ConsumerKeeper is missing ApplyAndReturnValidatorSetUpdates in order to be used by genutil
- genutil module should be removed from the consumer chain; the init val set comes always from the CCV module (see InitGenesis)
fails when adding to slashing.NewAppModule() without reason listed
fails when adding to distr.NewAppModule without reason listed
fails as return value in GetStakingKeeper
fails within prepForZeroHeightGenesis while trying to use the GetAllDelegations, IterateRedelegations, SetRedelegation, IterateUnbondingDelegations, SetUnbondingDelegation, GetValidator, SetValidator and ApplyAndReturnValidatorSetUpdates methods of stakingKeeper (these may be unnecessary, do we need to export genesis on a child chain?)

Some of these changes are already present in @sainoe 's #97 (which relies on cosmos/cosmos-sdk#11921).

this also removes the liquidity module from the child chain as it should not be considered part of a standard application. It could stay on the provider chain since gaia has it but should also probably just be removed.

Closes #58

Close #65 * Update the CCV slashing logic to handle double-signing evidences. * Add `InfractionType` enum to the `SlashPacketData` fields * Use `InfractionType` enum to distinguish between downtime and double-signing infractions in the logic * Use the provider chain slash fraction and jail duration parameters * Change the evidence keeper instantiation in app.go to use the CCV module instead of the staking module

mpoke · 2022-05-13T11:20:29Z

IBCKeeper can't accept app.ConsumerKeeper in place of app.StakingKeeper because it's missing the method GetHistoricalInfo

I think we can add the TrackHistoricalInfo() method from staking to the consumer ccv module. This would enable the IBCKeeper to use the app.ConsumerKeeper for calling the GetHistoricalInfo method.

mpoke · 2022-05-13T11:22:47Z

app.ConsumerKeeper is missing ApplyAndReturnValidatorSetUpdates in order to be used by genutil

Do we need the genutil module on the consumer? I don't know exactly what's the purpose of it.

okwme · 2022-05-13T12:44:22Z

@mpoke , genutil is used in creating genesis files for the blockchain afaiu.
The answer for whether we can remove it would be the same for whether we need to make it work with prepForZeroHeightGenesis as part of the genesis export.

I think it's probably important to keep. It's necessary for a certain type of network upgrade and is also valuable for data since you can essentially dump the entire state of the blockchain with that command. We should try to preserve it.

mpoke · 2022-05-13T12:56:30Z

@mpoke , genutil is used in creating genesis files for the blockchain afaiu. The answer for whether we can remove it would be the same for whether we need to make it work with prepForZeroHeightGenesis as part of the genesis export.

I think it's probably important to keep. It's necessary for a certain type of network upgrade and is also valuable for data since you can essentially dump the entire state of the blockchain with that command. We should try to preserve it.

Strange though, this means that the InitGenesis of both genutil and staking return a validator set (see here and here). Both of them are calling ApplyAndReturnValidatorSetUpdates. In the case of the consumer chain, the InitGenesis of the CCV module sends the initial validator set to Tendermint.

Still I don't know why DeliverGenTxs must return the validator set to Tendermint, i.e.,

// DeliverGenTxs iterates over all genesis txs, decodes each into a Tx and
// invokes the provided deliverTxfn with the decoded Tx. It returns the result
// of the staking module's ApplyAndReturnValidatorSetUpdates.

rigelrozanski · 2022-05-13T23:18:23Z

Overall this PR looks good to me. Only thing I'll mention is that we should also remove this testing function DisableConsumerDistribution (https://github.com/cosmos/interchain-security/blob/main/x/ccv/provider/provider_test.go#L614)

It's nice to keep the ModuleManager (MM) exposed within the app.go on the provider chain so we can disable distribution on the provider chain to check consumer funds received on the provider chain.

…urity into okwme/app-specific

… renamed consumer Keeper keys to match Staking Keeper

okwme · 2022-05-20T19:24:04Z

ok so the current problem has to do with making all the places in the consumer module that looks for stakingKeeper become replaced with consumerKeeper since that is where our staking is coming from. I was able to add new methods, some of them that needed to actually do something and some that were just no-ops to satisfy most of them.

The issue now is that the IBC testing framework expects a real functioning staking keeper in order to run.

One solution was to try implementing all of the methods within the real stakingKeeper as part of consumerKeeper. I added all of them that were missing to https://github.com/cosmos/interchain-security/blob/okwme/app-specific/x/ccv/consumer/keeper/no-ops.go. In order to confirm whether the consumerKeeper could be used as a stakingKeeper I made this test: https://github.com/cosmos/interchain-security/blob/okwme/app-specific/x/ccv/consumer/keeper/no-ops_test.go. This threw an error though, I'm not sure if it's possible or what's missing to make the consumerKeeper interchangeable with the stakingKeeper.

So instead I went back and realized that the part of the app.go that wants a real stakingKeeper could just be ignored like:
https://github.com/cosmos/interchain-security/blob/okwme/app-specific/app/consumer/app.go#L830-L833

// GetStakingKeeper implements the TestingApp interface.
func (app *App) GetStakingKeeper() stakingkeeper.Keeper {
	return stakingkeeper.Keeper{}
}

since GetStakingKeeper isn't called anywhere in the consumer chain.

However this breaks all of the IBC test framework we're using for testing Interchain Security because the IBC test framework wants to generate a chain with a real validator set.

Seems like even if we could get stakingKeeper and consumerKeeper interchangeable we still won't be able to use the IBC test framework because it has no concept of a Consumer Chain with this limited staking functionality.

Not sure how to proceed, anyone have ideas?
@alexanderbez @rigelrozanski @jtremback ?

okwme · 2022-05-20T19:31:39Z

x/ccv/consumer/keeper/keeper.go

@@ -26,7 +30,7 @@ import (
 type Keeper struct {
 	storeKey          sdk.StoreKey
 	cdc               codec.BinaryCodec
-	paramSpace        paramtypes.Subspace
+	paramstore        paramtypes.Subspace


paramSpace and accountKeeper were renamed to match the stakingKeeper's naming conventions

okwme · 2022-05-20T19:32:11Z

x/ccv/consumer/keeper/keeper.go

+// TrackHistoricalInfo saves the latest historical-info and deletes the oldest
+// heights that are below pruning height
+func (k Keeper) TrackHistoricalInfo(ctx sdk.Context) {
+	entryNum := 4 * 7 * 24 * 60 * 60 // 4 weeks (TODO: confirm safe limit)


should this use types.UnbondingTime ?

okwme · 2022-05-20T19:35:50Z

x/ccv/consumer/module.go

@@ -155,6 +155,8 @@ func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) {
 	blockHeight := uint64(ctx.BlockHeight())
 	vID := am.keeper.GetHeightValsetUpdateID(ctx, blockHeight)
 	am.keeper.SetHeightValsetUpdateID(ctx, blockHeight+1, vID)
+	am.keeper.TrackHistoricalInfo(ctx)


this allows us to satisfy the requirements of IBC in opening new connections from the consumer chain. We're now storing the val set at each block as well as the val set updates.

okwme · 2022-05-20T19:38:52Z

also should note that I merged in @sainoe 's PR (#97) to this one to resolve some of the other interface issues.

his work is mostly present in this PR within x/ccv/consumer/keeper/keeper_test.go, x/ccv/consumer/keeper/relay.go, x/ccv/consumer/keeper/relay_test.go, x/ccv/consumer/keeper/validators.go, x/ccv/provider/keeper/keeper.go, x/ccv/provider/keeper/keeper_test.go, x/ccv/provider/keeper/relay.go, x/ccv/provider/provider_test.go, x/ccv/types/ccv.go, x/ccv/types/expected_keepers.go and should mostly be ignored as part of this PR.

tac0turtle · 2022-05-31T16:20:17Z

feels like there is a bit of scope creep going on in this PR. Ill try to break out a few things into their own prs so we can merge in a timely fashion.

tac0turtle · 2022-06-01T10:19:32Z

x/ccv/consumer/genesis.go

+
+// TODO: export ccv vals as GenesisValidators
+// WriteValidators returns a slice of bonded genesis validators.
+func WriteValidators(ctx sdk.Context, keeper keeper.Keeper) (vals []tmtypes.GenesisValidator, err error) {


@mpoke any chance you can explain the reasoning for this todo?

@marbar3778 Not sure. The entire consumer chain restart is a bit fuzzy to me. IMO, there are two reasons to export the state of a consumer chain:

To restart the chain as a normal chain. I'm not sure how that's going to work. The consumer chain doesn't have a staking module (I'm ignoring here the gov-staking module). Thus, the exported state will contain no staking state and it doesn't require any validator set. This means that to restart as a normal chain, the state needs to be extended with this information.

To restart the chain still as a consumer chain. The CCV channel to the provided is part of the exported state. As long as the restart is quick enough so that the CCV channel doesn't timeout on the provider side, once the consumer chain is restarted, a relayer will relay all the pending VSC packets (with all the validator updates). The consumer needs though an initial validator set on which to apply all these updates. This initial validator set is the last validator set on the consumer, which can be retrieved from the CCV module, see

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

Line 495 in c072e5b

func (k Keeper) GetAllCCValidator(ctx sdk.Context) (validators []types.CrossChainValidator) {

Maybe this is the set of validator the WriteValidators function should return.

tac0turtle · 2022-06-07T21:54:42Z

with cosmos/cosmos-sdk#12181 being merged I think we can close this PR since the reliance on staking the struct is no longer there.

okwme · 2022-06-08T17:13:51Z

closing in light of #120

sainoe and others added 11 commits May 9, 2022 20:30

pass tests after renaming 'UnbondingOp' in sdk

139bc40

use rigel hack to progress in IntTest debugging

612b7a5

small fix

7f024b9

Merge branch 'main' into cis-merge-main

fde338e

change sdk import

21409b5

merge main

6dc0d1e

clean before merge

ea1c6d5

clean tests

f0730f6

fix nits

f298c83

remove unecessary parts of app.go in consumer and provider

60dc6b9

okwme requested review from AdityaSripal, jtremback, mpoke and sainoe as code owners May 12, 2022 17:38

mpoke added the product label May 13, 2022

mpoke requested a review from danwt May 13, 2022 11:13

This was referenced May 13, 2022

Fix broken integration test(s) at 80d146a #79

Closed

Create example of app.go for users, test in real configuration, fix IBC-Go test suite to handle it #58

Closed

okwme added 6 commits May 19, 2022 17:03

fix conflicts from rebase

78eb2bd

begin adding GetHistoricalInfo

7161749

add back historic height

65a70da

remove disables from testing framework

9140d53

Merge branch 'okwme/app-specific' of github.com:cosmos/interchain-sec…

a1ac1f0

…urity into okwme/app-specific

add historic height to beginblock

d34581b

okwme added 9 commits May 20, 2022 15:42

add stub for ApplyAndReturnValidatorSetUpdates

95543f5

add back ApplyAndReturnValidatorSetUpdates to provider

0e0a5f0

added UnbondingTime, stubbed GetLastTotalPower and SetLastTotalPower,…

11ef039

… renamed consumer Keeper keys to match Staking Keeper

Merge branch 'main' into okwme/app-specific

1d0b532

add forked slashing module to allow for staking interface

e576185

merged in sainoe/consumer-initiated-slashing

4d156e9

fixed IBC app interface with forks

174bb09

satisfies IBCKeeper

9d4d457

compiles, no thanks to meticulously adding every keeper method

c072e5b

okwme commented May 20, 2022

View reviewed changes

This was referenced Jun 1, 2022

rename paramSpace and accountKeeper #119

Merged

separate consumer from provider in app.go #120

Merged

tac0turtle reviewed Jun 1, 2022

View reviewed changes

mpoke marked this pull request as draft June 7, 2022 09:30

mpoke assigned mpoke and sainoe and unassigned mpoke Jun 8, 2022

okwme closed this Jun 8, 2022

mpoke reopened this Jun 8, 2022

This was referenced Jun 8, 2022

GetHistoricalInfo in consumer CCV module #142

Closed

Create app.go for minimum viable consumer chain #139

Closed

mpoke closed this Jun 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove unnecessary parts of app.go in consumer and provider #98

remove unnecessary parts of app.go in consumer and provider #98

okwme commented May 12, 2022 •

edited by mpoke

Loading

mpoke commented May 13, 2022

mpoke commented May 13, 2022

okwme commented May 13, 2022

mpoke commented May 13, 2022

rigelrozanski commented May 13, 2022

okwme commented May 20, 2022

okwme May 20, 2022

okwme May 20, 2022

okwme May 20, 2022

okwme commented May 20, 2022

tac0turtle commented May 31, 2022

tac0turtle Jun 1, 2022

mpoke Jun 1, 2022

tac0turtle commented Jun 7, 2022

okwme commented Jun 8, 2022

remove unnecessary parts of app.go in consumer and provider #98

remove unnecessary parts of app.go in consumer and provider #98

Conversation

okwme commented May 12, 2022 • edited by mpoke Loading

mpoke commented May 13, 2022

mpoke commented May 13, 2022

okwme commented May 13, 2022

mpoke commented May 13, 2022

rigelrozanski commented May 13, 2022

okwme commented May 20, 2022

okwme May 20, 2022

Choose a reason for hiding this comment

okwme May 20, 2022

Choose a reason for hiding this comment

okwme May 20, 2022

Choose a reason for hiding this comment

okwme commented May 20, 2022

tac0turtle commented May 31, 2022

tac0turtle Jun 1, 2022

Choose a reason for hiding this comment

mpoke Jun 1, 2022

Choose a reason for hiding this comment

tac0turtle commented Jun 7, 2022

okwme commented Jun 8, 2022

okwme commented May 12, 2022 •

edited by mpoke

Loading