Skip to content
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

epoching: simulation infra for integration testing #68

Merged
merged 7 commits into from
Jul 21, 2022
Merged

Conversation

SebastianElvis
Copy link
Member

@SebastianElvis SebastianElvis commented Jul 20, 2022

Fixes BM-83

This PR adds the simulation support for the epoching module. It also makes minor modifications on some DB schemas for serialisation and simulation support.

Most of the code is adapted from https://github.com/cosmos/cosmos-sdk/blob/v0.45.5/x/staking/simulation and is based on my understanding below.

Background of Cosmos SDK simulation and integration testing

Cosmos SDK supports integration tests by using the "simulation" technique, in which the simulator program generates the Cosmos-SDK-based application with randomised parameters and genesis states, sends random messages to the application, and verifies the system's correctness properties specified by the developers. See https://docs.cosmos.network/master/core/simulation.html for more details on Cosmos blockchain simulator.

To support simulation, the Cosmo-SDK-based blockchain has to implement a simulation submodule for each module. For each module, its simulation submodule allows to generate random parameters, genesis states and messages of this module. See https://docs.cosmos.network/master/building-modules/simulator.html for details on the module simulation.

As an example, Osmosis has submodule simulation for each of its module, and implements the simulation program by themselves https://github.com/osmosis-labs/osmosis/tree/main/simulation.

@SebastianElvis SebastianElvis changed the title epoching: simulation infra for SimApp-based integration test epoching: simulation infra for integration testing Jul 20, 2022
@SebastianElvis SebastianElvis marked this pull request as ready for review July 20, 2022 06:19
Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

I believe this is good, although I only have the faintest idea of how it fits together. You did tons of research into how Osmosis does it, impressive!

Comment on lines 36 to 38
func(_ *rand.Rand) {
weightMsgDelegate = simappparams.DefaultWeightMsgDelegate
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand what's happening with this callback. The address of an weightMsgDelegate is passed to GetOrGenerate, and we're setting the value as well to a default... Why not just set it to that default?

I should read the docs you linked to, but perhaps some comments would help anyone coming across this.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, weightMsgDelegate is always the default value in this case and is not randomised w.r.t. *rand.Rand. This is also taken from the staking module. Sometimes Cosmos SDK uses randomised weights, e.g., https://github.com/cosmos/cosmos-sdk/blob/v0.45.5/x/staking/simulation/genesis.go#L48-L61

In fact I think it's good to randomise its value and the other two weights, in order to simulate all possible distributions of delegation/undelegation/redelegation. Not sure why Cosmos SDK does not do this. Added some TODOs here and will consider randomising these weights in the future.

Comment on lines 86 to 88
if val.InvalidExRate() {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgWrappedDelegate, "validator's invalid echange rate"), nil, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if val.InvalidExRate() {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgWrappedDelegate, "validator's invalid echange rate"), nil, nil
}
if val.InvalidExRate() {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgWrappedDelegate, "validator's invalid exchange rate"), nil, nil
}

Not sure, just see ExRate.

What does this mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Judging from https://github.com/cosmos/cosmos-sdk/blob/v0.45.5/x/staking/types/validator.go#L296-L301 it means that the validator has zero token but has delegations with positive token amount, which is impossible and thus invalid.

srcAddr := srcVal.GetOperator()
delegations := stk.GetValidatorDelegations(ctx, srcAddr)
if delegations == nil {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgWrappedBeginRedelegate, "keeper does have any delegation entries"), nil, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgWrappedBeginRedelegate, "keeper does have any delegation entries"), nil, nil
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgWrappedBeginRedelegate, "keeper does not have any delegation entries"), nil, nil

delAddr := delegation.GetDelegatorAddr()

if stk.HasReceivingRedelegation(ctx, delAddr, srcAddr) {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgWrappedBeginRedelegate, "receveing redelegation is not allowed"), nil, nil // skip
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgWrappedBeginRedelegate, "receveing redelegation is not allowed"), nil, nil // skip
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgWrappedBeginRedelegate, "receiving redelegation is not allowed"), nil, nil // skip

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, there are many typos in the original staking module's implementation 😢 Thanks for catching all of them

for i, p := range paramChanges {
require.Equal(t, expected[i].composedKey, p.ComposedKey())
require.Equal(t, expected[i].key, p.Key())
require.Equal(t, expected[i].simValue, p.SimValue()(r))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "82" the random value it changed to with that seed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. If we change rand.NewSource(1) to rand.NewSource(2) then simValue will no longer be 82.

Comment on lines +14 to +16
// NewDecodeStore returns a decoder function closure that unmarshals the KVPair's
// Value to the corresponding epoching type.
func NewDecodeStore(cdc codec.Codec) func(kvA, kvB kv.Pair) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this used? What are these newline separated strings for?

Copy link
Member Author

Choose a reason for hiding this comment

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

The decoder function returned by NewDecodeStore will be a part of the module (via RegisterStoreDecoder at https://github.com/cosmos/cosmos-sdk/blob/v0.45.5/x/staking/module.go#L194-L197 for staking module). The decoder function allows the simulation program to test importing/exporting the entire app, which includes parameters, genesis states, and DB (in the GetSimulationLog function at https://github.com/cosmos/cosmos-sdk/blob/v0.45.5/simapp/sim_test.go#L180-L189).

@SebastianElvis SebastianElvis merged commit 0049ecd into main Jul 21, 2022
@SebastianElvis SebastianElvis deleted the simapp-impl branch July 21, 2022 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants