feat(sim): port simulator to run on top of testutil/network #17938
Conversation
elias-orijtech
force-pushed
the
simulator-rewrite
branch
from
55437d9 to
42e2d96
Compare
There was a problem hiding this comment.
Nice work and thank you @elias-orijtech! Just an initial round of review.
| network *network.Network | ||
| } | ||
|
|
||
| func (s *IntegrationTestSuite) SetupSuite() { | ||
| s.T().Log("setting up integration test suite") | ||
|
|
||
| s.rand = rand.New(rand.NewSource(42)) |
There was a problem hiding this comment.
Let's add a comment here
// Using a hardcoded seed for predictability.
// TODO(@elias-orijtech): allow the seed to be passed in.There was a problem hiding this comment.
For AppHash tests, the seed has to be hardcoded. I prefer to leave this for when MockComet support is added.
| } | ||
| return fixture | ||
| }) | ||
| s.cfg.NumValidators = s.rand.Intn(10) |
There was a problem hiding this comment.
// TODO(@elias-orijtech): allow number of validators to
// be programmatically passed in perhaps via cli argumentsThere was a problem hiding this comment.
There has to be some limit? I assume this particular parameter will be revisited once testutil runs on top of MockComet.
elias-orijtech
force-pushed
the
simulator-rewrite
branch
2 times, most recently
from
c1f6201 to
79dc580
Compare
elias-orijtech
force-pushed
the
simulator-rewrite
branch
from
79dc580 to
ff0cd00
Compare
WalkthroughWalkthroughThe changes across various files in the Cosmos SDK-based application suggest a significant refactoring of the simulation framework. The Changes
TipsChat with CodeRabbit Bot (
|
elias-orijtech
force-pushed
the
simulator-rewrite
branch
from
ff0cd00 to
9ee6aec
Compare
elias-orijtech
force-pushed
the
simulator-rewrite
branch
2 times, most recently
from
90a9e2e to
7fed617
Compare
|
I've rebased and pushed the current progress of the simulator, and described the approach and future direction in the last commit and the PR description. The first commit removes the simulator tests that lock in message content, which I don't think are worth their weight. The PR is ready for review, but I've left it a draft because someone should decide whether to revert deletion of the replaced generators. |
elias-orijtech
force-pushed
the
simulator-rewrite
branch
3 times, most recently
from
2b49651 to
42d2ff1
Compare
|
Not sure what to do about the linter errors: They seem to be caused by a mismatch between the v1 and v2 |
elias-orijtech
force-pushed
the
simulator-rewrite
branch
3 times, most recently
from
65ef1d6 to
8e83401
Compare
github-prbot
requested review from
a team,
facundomedica and
samricotta
and removed request for
a team
|
@tac0turtle I've rebased the PR on top of the recent testutil changes and re-enabled genesis state generation for the old simulator for compatibility. The missing piece is the v1 vs v2 simapp difference in the |
…ction The tests don't carry their maintenance weight, because they merely enforce particular messages for a particular random generator state. This is part of the simulator rewrite which is going to replace the generators.
This change implements a replacement for the current simulator based on testutil/network. Most of the changes are porting the module specific message generators to no longer rely on SimulationState, and to generate "real" messages, not simulator messages. The simulator driver is in simapp, as part of the IntegationTestSuite. The new approach aims to improve simulation in two important ways: - Simulation should more closely mimic a real network. The current simulator message delivery is implemented parallel to non-simulator message delivery, leading to loss of fidelity and higher maintenance. One symptom is cosmos#13843. - Simulation should be layered on top of modules, not part of modules. This means that modules should not import simulation packages, nor refer to its generator package (x/module/simulation). This should eventually fix cosmos#7622. There are also downsides, however. Where the current simulator is too high level, testutil/network is too low level: it runs a real network of validators which is difficult to control. For example: - AppHashes differ between runs, because modules may depend on non- deterministic state such as block header timestamps. - The validators runs in separate goroutines, which makes it hard to query app state without introducing race conditions. - Blocks are produced according tot time, and not under control by the test driver. This makes it hard to trigger processing of messages in particular blocks, which ruins determinism. Some of the issues may be worked around, for example by forcing the block headers to be deterministic; however, the real fix is to make testutil/network itself deterministic, providing the goldilock level of simulation: close enough to a real network, yet deterministic enough to generate the same chain state for a given random seed. A deterministic testutil/network is part of cosmos#18145. Future work includes: - Porting of the remaining module message generators. - Generating (and verifying) deterministic AppHashes, allowing reliable replay when a problematic message is detected. Depends on cosmos#18145. - Save/reload of state for faster debugging cycles. - Removal of the old simulator, most importantly the reference to it from module code. Updates cosmos#14753 (Simulator rewrite epic) Updates cosmos#7622 (reducing imports from modules to simulator) Updates cosmos#13843 (using real message delivery for simulation)
elias-orijtech
force-pushed
the
simulator-rewrite
branch
from
84f15c3 to
dc967dd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (20)
- simapp/app.go (3 hunks)
- simapp/app_v2.go (2 hunks)
- simapp/testutil_network_test.go (2 hunks)
- tests/integration/bank/keeper/deterministic_test.go (2 hunks)
- tests/integration/distribution/keeper/msg_server_test.go (2 hunks)
- tests/integration/evidence/keeper/infraction_test.go (2 hunks)
- tests/integration/example/example_test.go (3 hunks)
- tests/integration/gov/keeper/keeper_test.go (2 hunks)
- tests/integration/staking/keeper/common_test.go (2 hunks)
- tests/integration/staking/keeper/deterministic_test.go (2 hunks)
- testutil/network/network.go (3 hunks)
- x/auth/module.go (6 hunks)
- x/auth/simulation/genesis.go (2 hunks)
- x/auth/simulation/proposals.go (1 hunks)
- x/bank/module.go (2 hunks)
- x/bank/simulation/genesis.go (2 hunks)
- x/bank/simulation/operations.go (3 hunks)
- x/gov/module.go (2 hunks)
- x/gov/simulation/genesis.go (2 hunks)
- x/gov/simulation/operations.go (3 hunks)
Files skipped from review due to trivial changes (4)
- simapp/app.go
- tests/integration/bank/keeper/deterministic_test.go
- tests/integration/example/example_test.go
- tests/integration/staking/keeper/common_test.go
Additional comments: 43
tests/integration/gov/keeper/keeper_test.go (1)
- 120-126: Ensure that all calls to
auth.NewAppModule,bank.NewAppModule,staking.NewAppModule, andgov.NewAppModulethroughout the codebase have been updated to match the new function signatures.tests/integration/evidence/keeper/infraction_test.go (1)
- 129-135: The
auth.NewAppModulefunction call no longer includesauthsims.RandomGenesisAccountsas an argument. Ensure that this change does not affect the functionality of theauthModuleand that all necessary adjustments have been made in the codebase.x/bank/simulation/genesis.go (2)
56-85: The changes in this hunk are well done. The function signatures have been updated to accept the necessary parameters directly, instead of relying on a
simStateobject. This improves the modularity and testability of the functions. The removal of the print statement also cleans up the code.60-70: The suggestion to use
makewith a capacity hint fromlen(accs)was declined in a previous review. The reasoning was that genesis generation is called once per genesis, so the performance improvement would be negligible. This reasoning is sound, and the current implementation is acceptable.x/bank/module.go (2)
164-168: The
GenerateGenesisStatefunction now callssimulation.RandomizedGenStatewith specific parameters. Ensure that the parameters passed are correct and that the function behaves as expected.181-183: The
WeightedOperationsfunction now returnsnilinstead of callingsimulation.WeightedOperations. This change might affect the behavior of the function. Ensure that this change is intentional and does not cause any issues.simapp/app_v2.go (1)
- 231-237: The creation of a simulation manager with specific module overrides has been replaced with a new simulation manager created from app modules. This change seems to be in line with the goal of reducing module dependencies on the simulation package. However, ensure that all necessary modules are included in
app.ModuleManager.Modules.- app.sm = module.NewSimulationManagerFromAppModules(app.ModuleManager.Modules, nil) + app.sm = module.NewSimulationManagerFromAppModules(app.ModuleManager.Modules, <necessary modules>)x/auth/simulation/proposals.go (1)
- 8-17: The function
GenerateMsgUpdateParamshas been updated to remove unused parameters and renamed fromSimulateMsgUpdateParams. Ensure that all calls to this function throughout the codebase have been updated to match the new signature and name.x/auth/simulation/genesis.go (2)
17-51: The
RandomGenesisAccountsfunction has been significantly modified. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the new logic for creatingBaseAccount,ContinuousVestingAccount, andDelayedVestingAccountis correct and aligns with the intended behavior.86-101: The
RandomizedGenStatefunction has been significantly modified. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the new logic for generating a randomGenesisStatefor auth is correct and aligns with the intended behavior.x/gov/module.go (4)
322-327: The
RandomizedGenStatefunction now takes four parameters instead of one. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.330-332: The
ProposalContentsfunction now returnsnilinstead of callingsimulation.ProposalContents(). This change might affect the simulation of governance proposals. Ensure that this change is intentional and does not affect the expected behavior.334-337: The
ProposalMsgsfunction now returnsnilinstead of callingsimulation.ProposalMsgs(). This change might affect the simulation of governance proposals. Ensure that this change is intentional and does not affect the expected behavior.344-347: The
WeightedOperationsfunction now returnsnilinstead of callingsimulation.WeightedOperations(). This change might affect the simulation of governance proposals. Ensure that this change is intentional and does not affect the expected behavior.tests/integration/distribution/keeper/msg_server_test.go (2)
15-20: The import of the package "cosmossdk.io/x/auth/simulation" has been removed. Ensure that this does not affect any other parts of the code that might be using it.
117-120: The initialization of the
authModulein theinitFixturefunction has been updated to use theauth.NewAppModulefunction without theauthsims.RandomGenesisAccountsargument. Make sure that this change is reflected whereverinitFixtureis used.simapp/testutil_network_test.go (4)
40-40: The hardcoded seed for the random number generator is still being used. This is fine for testing purposes as it ensures predictability and repeatability of tests. However, consider allowing the seed to be passed in for more varied testing in the future.
55-55: The number of validators is randomly generated with a maximum of 10. This is fine for testing purposes, but consider allowing this number to be programmatically passed in for more varied testing in the future.
64-66: Error handling is done correctly here. If an error occurs while generating a new mnemonic, the test fails immediately.
152-176: The simulation loop generates and broadcasts transactions, and handles future votes. Error handling is done correctly. If an error occurs while encoding a transaction or broadcasting it, the test fails immediately.
x/gov/simulation/genesis.go (5)
2-7: Imports are well organized and follow the standard Go conventions.
17-21: The constants are well defined and documented.
81-108: The
RandomizedGenStatefunction has been significantly refactored. It now uses therand.Randinstance for random number generation and takes additional parameters forgenState,cdc, andbondDenom. The function no longer relies on themodule.SimulationStatefor random number generation and parameter retrieval. The function also no longer prints the randomly generated governance parameters to the console. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.102-105: The
v1.NewGenesisStatefunction is called with the correct parameters. The parameters are well defined and the function call is correct.107-107: The
govGenesisis correctly marshaled into JSON and stored in thegenStatemap.testutil/network/network.go (3)
19-19: The new import
github.com/cometbft/cometbft/configis used for theFuzzConnConfigtype. Ensure that this package is maintained and reliable.136-136: The new field
FuzzConnConfigis added to theConfigstruct. This field is used to conditionally setTestFuzzandTestFuzzConfigincmtCfg.P2P.346-349: The
Newfunction is modified to conditionally setTestFuzzandTestFuzzConfigincmtCfg.P2Pbased on the presence ofFuzzConnConfigin the inputcfg. This change is consistent with the PR's goal of improving simulation fidelity.x/bank/simulation/operations.go (1)
- 16-19: The constant
DistributionModuleNameis defined but not used anywhere in the code. If it's not needed, consider removing it to avoid confusion.x/auth/module.go (6)
93-97: The
randGenAccountsFnfield has been removed from theAppModulestruct. Ensure that this does not break any dependencies that might be using this field.106-111: The
NewAppModulefunction no longer accepts therandGenAccountsFnparameter. Make sure all calls to this function have been updated accordingly.157-159: The
GenerateGenesisStatefunction now uses a different method signature forsimulation.RandomizedGenState. Verify that the new parameters are correctly passed and handled.190-195: The
RandomGenesisAccountsFnfield has been removed from theModuleInputsstruct. Ensure that this does not break any dependencies that might be using this field.213-218: The
ProvideModulefunction has been updated to remove the logic for handling theRandomGenesisAccountsFnfield. Make sure this change is consistent with the overall design and does not introduce any issues.223-229: The
ProvideModulefunction has been updated to remove the logic for handling theRandomGenesisAccountsFnfield. Make sure this change is consistent with the overall design and does not introduce any issues.x/gov/simulation/operations.go (7)
- 10-15: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [19-15]
The import paths have been updated to reflect the new location of the packages. Ensure that the new paths are correct and the packages exist at those locations.
22-59: The
GenerateMsgSubmitProposalfunction has been updated to include a newexpeditedvariable which is used to determine the type of deposit. Ensure that this change is consistent with the rest of the codebase and that all calls to this function have been updated accordingly.73-268: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [61-126]
A new
VoteGeneratortype has been introduced along with associated methods for generating votes. This is a significant change and should be thoroughly tested to ensure it works as expected.
128-161: The
GenerateMsgDepositfunction has been updated to include a newexpeditedvariable which is used to determine the type of deposit. Ensure that this change is consistent with the rest of the codebase and that all calls to this function have been updated accordingly.163-202: The
GenerateMsgVotefunction has been updated to include a newproposalIDIntvariable which is used to determine the proposal ID. Ensure that this change is consistent with the rest of the codebase and that all calls to this function have been updated accordingly.204-234: The
GenerateMsgVoteWeightedfunction has been introduced. This is a significant change and should be thoroughly tested to ensure it works as expected.236-266: The
GenerateMsgCancelProposalfunction has been introduced. This is a significant change and should be thoroughly tested to ensure it works as expected.tests/integration/staking/keeper/deterministic_test.go (1)
- 110-110: The
auth.NewAppModulefunction call has been updated to exclude theauthsims.RandomGenesisAccountsargument. Ensure that this change does not affect the functionality of theauthModuleand that all dependencies are correctly handled.
| @@ -405,7 +404,7 @@ func NewSimApp( | |||
| txConfig, | |||
| ), | |||
| accounts.NewAppModule(app.AccountsKeeper), | |||
| auth.NewAppModule(appCodec, app.AuthKeeper, authsims.RandomGenesisAccounts), | |||
There was a problem hiding this comment.
does this break existing simulator from running?
|
i think we may need to revert the removal of things in modules as we want the current simulator to still run, I believe there is still some work we need to do to make it sufficient to replace things. I can take ownership of this and push it over the finish line |
Thank you very much. Another option is to leave this PR in draft and start working on a deterministic testutil/network based on your conversion to interfaces and integration of MockComet. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- simapp/app.go (3 hunks)
- tests/integration/distribution/keeper/msg_server_test.go (2 hunks)
- testutil/network/network.go (3 hunks)
Additional comments: 6
simapp/app.go (3)
29-34: The removal of the import
cosmossdk.io/x/auth/simulationis not visible within the provided hunk. It's important to ensure that this import is no longer used within the file and that any associated functionality has been refactored or removed accordingly.412-418: The call to
auth.NewAppModuledoes not show the removal of theauthsims.RandomGenesisAccountsargument within the provided hunk. It's important to ensure that the call has been updated as per the PR objectives and that any necessary adjustments have been made to accommodate this change.509-515: The addition of the simulation manager and the registration of store decoders are consistent with the PR objectives of implementing a new simulator that runs on top of
testutil/network. This change should help in achieving the goal of making simulations more realistic and deterministic.testutil/network/network.go (3)
19-19: The import of
cmtcfgis correctly added to support the newFuzzConnConfigfield in theConfigstruct.345-348: The conditional logic to set
cmtCfg.P2P.TestFuzzandcmtCfg.P2P.TestFuzzConfigbased on the presence ofFuzzConnConfigis correctly implemented.132-137: The addition of
FuzzConnConfigto theConfigstruct is consistent with the PR's objectives to improve the realism and determinism of simulations by allowing network fuzzing configurations.
| @@ -131,6 +132,8 @@ type Config struct { | |||
| AddressCodec address.Codec // address codec | |||
| ValidatorAddressCodec runtime.ValidatorAddressCodec // validator address codec | |||
| ConsensusAddressCodec runtime.ConsensusAddressCodec // consensus address codec | |||
|
|
|||
| FuzzConnConfig *cmtcfg.FuzzConnConfig | |||
There was a problem hiding this comment.
The new field FuzzConnConfig has been added to the Config struct. Ensure that this field is properly documented to explain its purpose and usage within the network configuration.
|
hmm i cant get this test to fail locally, @elias-orijtech are you able to do it locally? |
|
Sorry for dropping this ball. Do you mean the failing simapp v1 test? If so, did you test with the |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This change implements a replacement for the current simulator based
on testutil/network. Most of the changes are porting the module specific
message generators to no longer rely on SimulationState, and to generate
"real" messages, not simulator messages. The simulator driver is in
simapp, as part of the IntegationTestSuite.
The new approach aims to improve simulation in two important ways:
simulator message delivery is implemented parallel to non-simulator
message delivery, leading to loss of fidelity and higher maintenance.
One symptom is Add support for
Prepare/ProcessProposalin simulation testing #13843.This means that modules should not import simulation packages, nor refer
to its generator package (x/module/simulation). This should eventually
fix Refactor simulation packages & subpackages to reduce frequency of import cycles #7622.
There are also downsides, however. Where the current simulator is too
high level, testutil/network is too low level: it runs a real network
of validators which is difficult to control. For example:
deterministic state such as block header timestamps.
query app state without introducing race conditions.
test driver. This makes it hard to trigger processing of messages in
particular blocks, which ruins determinism.
Some of the issues may be worked around, for example by forcing the
block headers to be deterministic; however, the real fix is to make
testutil/network itself deterministic, providing the goldilock level
of simulation: close enough to a real network, yet deterministic enough
to generate the same chain state for a given random seed.
A deterministic testutil/network is part of #18145.
Future work includes:
replay when a problematic message is detected. Depends on [Feature]: modular testutil.Network #18145.
module code.
Updates #14753 (Simulator rewrite epic)
Updates #7622 (reducing imports from modules to simulator)
Updates #13843 (using real message delivery for simulation)
CC @odeke-em @tac0turtle @alexanderbez
Summary by CodeRabbit
Refactor
authmodule across various test suites.authmodule's initialization process by excluding specific arguments related to random account generation.New Features
testutil_network_test.goto enhance network and application configuration for testing.testutil/network/network.go.Bug Fixes
RandomGenesisAccountsandRandomizedGenStatefunctions in thex/auth/simulationpackage to accept new parameters and return updated types, ensuring correct initialization of genesis states.Documentation
x/auth/simulationandx/bank/simulationpackages to better reflect their purpose.Chores