-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix(simulation): Fix all problems make test-sim-custom-genesis-fast
for simulation test.
#17911
Conversation
…sFileFn does not have ConsKey causing SimulateMsgCreateValidator fail
…sgCreateGroupPolicy when the group does not exist in the chain during simulation test
testutil/sims/state_helpers.go
Outdated
return genesis, nil, err | ||
// NOTE: cometbft uses a custom JSON decoder for GenesisDoc | ||
// On some machines, the above will go wrong, so try using cbftjson for Unmarshal again. | ||
if err = cbftjson.Unmarshal(bytes, &genesis); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a helper for that un x/genutil (
cosmos-sdk/x/genutil/types/genesis.go
Line 92 in 0a253f3
func AppGenesisFromReader(reader io.Reader) (*AppGenesis, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, have used the AppGenesisFromReader function to Unmarshal the genesis.json file
baseapp/baseapp.go
Outdated
@@ -31,6 +31,7 @@ import ( | |||
sdk "github.com/cosmos/cosmos-sdk/types" | |||
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | |||
"github.com/cosmos/cosmos-sdk/types/mempool" | |||
simcli "github.com/cosmos/cosmos-sdk/x/simulation/client/cli" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels not right to import a x/
path in baseapp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but as I mentioned above: "Add the SigverifyTx flag in the simulation test whether to check the transaction signature.", in order to achieve this requirement, I followed the previous implementation and added a FlagSigverifyTx global variable(x/simulation/client/cli/flags.go), and during in the simulation test,whether check the user's signature based on this global variable. In order to achieve this requirement, I have not thought of a better implementation for the time being. Can you give me any more suggestions or tips?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels not right to import a
x/
path in baseapp.
@julienrbrt A private variable sigverifyTx has been added to the type BaseApp struct
to solve this problem. Please help me review the code again.
…er to sigverify check for transaction
baseapp/test_helpers.go
Outdated
errorsmod "cosmossdk.io/errors" | ||
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rn make lint-fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
x/auth/ante/sigverify.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test in sigverify_test to harden this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done! Please help me review the code again.
baseapp/baseapp.go
Outdated
@@ -89,6 +89,7 @@ type BaseApp struct { | |||
addrPeerFilter sdk.PeerFilter // filter peers by address and port | |||
idPeerFilter sdk.PeerFilter // filter peers by node ID | |||
fauxMerkleMode bool // if true, IAVL MountStores uses MountStoresDB for simulation speed. | |||
sigverifyTx bool // whether to sigverify check for transaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we precise here, as in context what it is used for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
@julienrbrt Could we get those fixes in the next CosmosSDK 0.50.x version? The fix in |
Hey @julienrbrt, I'm sorry for poking you again but I want to make sure you saw my post above. |
Perfectly on time for our monthly patch release (this week), thanks for pinging me |
@Mergifyio backport release/v0.50.x |
✅ Backports have been created
|
* build(deps): Bump github.com/cosmos/gogoproto from 1.4.12 to 1.5.0 (cosmos#20567) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * refactor(x/authz,x/feegrant): provide updated keeper in depinject (cosmos#20590) * docs: Update high level overview and introduction (backport cosmos#20535) (cosmos#20627) Co-authored-by: samricotta <37125168+samricotta@users.noreply.github.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com> * fix: Properly parse json in the wait-tx command. (backport cosmos#20631) (cosmos#20660) Co-authored-by: Daniel Wedul <github@wedul.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com> * docs: remove Ineffective code block (backport cosmos#20703) (cosmos#20711) * feat(client): Add flag & reading mnemonic from file (backport cosmos#20690) (cosmos#20712) Co-authored-by: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com> * fix: nested multisig signatures using CLI (backport cosmos#20438) (cosmos#20692) Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com> Co-authored-by: Facundo <facundomedica@gmail.com> * feat(client/v2): get keyring from context (backport cosmos#19646) (cosmos#20727) Co-authored-by: Julien Robert <julien@rbrt.fr> * docs(x/group): orm codespace comment (backport cosmos#20749) (cosmos#20751) * feat: parse home flag earlier (backport cosmos#20771) (cosmos#20777) Co-authored-by: Julien Robert <julien@rbrt.fr> * build(deps): Bump github.com/cometbft/cometbft from 0.38.7 to 0.38.8 (cosmos#20805) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * build(deps): Bump github.com/cometbft/cometbft from 0.38.8 to 0.38.9 (cosmos#20836) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * fix(simulation): fix the problem of `validator set is empty after InitGenesis` in simulation test (backport cosmos#18196) (cosmos#20897) Co-authored-by: Chenqun Lu <luchenqun@qq.com> Co-authored-by: Julien Robert <julien@rbrt.fr> * fix(simulation): Fix all problems `make test-sim-custom-genesis-fast` for simulation test. (backport cosmos#17911) (cosmos#20909) Co-authored-by: Chenqun Lu <luchenqun@qq.com> Co-authored-by: Julien Robert <julien@rbrt.fr> * chore: prepare v0.50.8 (cosmos#20910) --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: samricotta <37125168+samricotta@users.noreply.github.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com> Co-authored-by: Daniel Wedul <github@wedul.com> Co-authored-by: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Co-authored-by: Facundo <facundomedica@gmail.com> Co-authored-by: Chenqun Lu <luchenqun@qq.com>
cosmos sdk provides a powerful simulation testing capability, and one of the simulation modes
TestFullAppSimulation
allows for simulation testing using data fromgenesis.json
. The original implementation can be found at #3428. However, with the release of v0.40.0, there have been significant changes to the code andTestFullAppSimulation
is no longer being maintained.The following are my steps:
To begin, the chain-related data can be initialized using the following command, at least 2 accounts must be added for genesis account, otherwise when executing SimulateMsgSend, two accounts will be selected to meet the conditions from != to and it will fall into an infinite loop.
Then, in the latest
main
branch, execute the commandmake test-sim-custom-genesis-fast
(note: you need to modify-Genesis=${HOME}/.gaiad/config/genesis.json
to-Genesis=${HOME}/.simapp/config/genesis.json
in the Makefile first). You may encounter the following error:Based on the errors encountered during each execution of
make test-sim-custom-genesis-fast
, I have identified and attempted to resolve three potential issues:Issue 1: Fix the problem where the account randomly generated by
AppStateFromGenesisFileFn
does not have aConsKey
, causingSimulateMsgCreateValidator
to fail.This issue primarily arises when executing
SimulateMsgCreateValidator
. The accounts generated byAppStateFromGenesisFileFn
do not have theConsKey
field specified, leading to a null pointer exception when callingsimAccount.ConsKey.PubKey()
.Issue 2: Fix the problem of operating on a group, such as
SimulateMsgCreateGroupPolicy
, when the group does not exist in the chain during simulation testing.The problem is encountered when executing the simulation test of the group module. It occurs when there is no group created, meaning there is no group in the chain. Some operations are performed on an empty group, such as executing
SimulateMsgCreateGroupPolicy
.Issue 3: Add the SigverifyTx flag in the simulation test whether to check the transaction signature.
During the execution of
AppStateFromGenesisFileFn
, the private keys corresponding to addresses are generated randomly. Therefore, when executing transactions, we need to skip the signature verification check.Of course, if the genesis data is also randomly generated, such as the simulation test executed by the
make test-sim-nondeterminism
command, the private key corresponding to the address generated at this time is correct, and the signature needs to be verified at this time.Or when we also read the data initialization chain from the genesis.json file. If the private keys corresponding to all addresses are passed in, we can also verify the signature at this time.
So I added a
SigverifyTx
flag that leaves it up to the user to decide whether signature verification of the transaction is required. For example, if I want to skip signature verification when executing simulation, then I can rungo test -mod=readonly -run TestFullAppSimulation -Genesis=genesis.json -SigverifyTx=false