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

[Feature]: simulate nested messages #15809

Closed
tac0turtle opened this issue Apr 12, 2023 · 7 comments · Fixed by #20291
Closed

[Feature]: simulate nested messages #15809

tac0turtle opened this issue Apr 12, 2023 · 7 comments · Fixed by #20291

Comments

@tac0turtle
Copy link
Member

Summary

Currently users are able to simulate transactions, this is useful to make sure the tx will pass, but with the adoption of delayed execution on nested messages the simulations dont check the nested message. We should adapt the simulation check for transactions to also check nested messages.

Problem Definition

Nested messages dont get checked in transaction simulations if the message will be queued for delayed execution

Proposal

Extend simulations to check nested messages if they exist within a transaction

@alexanderbez
Copy link
Contributor

You mean if a transaction contains a message that gets put into a queue or something to be processed at (Begin|End)Block? If so, how would this even be possible to fully simulate?

@tac0turtle
Copy link
Member Author

the simulate would be more of a check of validation then gas consumption if thats what you mean

@lucaslopezf
Copy link
Contributor

Hi @tac0turtle, I have some doubts:

What do you mean by nested messages? I saw a proto-definition (but it seems to be only for testing) and the DescriptorProto.
What do you mean by simulate transactions? Is it when a user uses the simulate flag on the client?

@tac0turtle
Copy link
Member Author

What do you mean by nested messages? I saw a proto-definition (but it seems to be only for testing) and the DescriptorProto.

messages can embed other messages in them that will be executed at a later time (gov, etc..). These messages are not simulated so they could fail at a later date when the state machine goes to execute them.

simulation is exposed via an endpoint or cli. here is a link to the code that handles this. Simulating a transaction is used in order to see if a transaction will pass or fail when executing it.

cosmos-sdk/baseapp/abci.go

Lines 988 to 1010 in b4e7df2

case "simulate":
txBytes := req.Data
gInfo, res, err := app.Simulate(txBytes)
if err != nil {
return sdkerrors.QueryResult(errorsmod.Wrap(err, "failed to simulate tx"), app.trace)
}
simRes := &sdk.SimulationResponse{
GasInfo: gInfo,
Result: res,
}
bz, err := codec.ProtoMarshalJSON(simRes, app.interfaceRegistry)
if err != nil {
return sdkerrors.QueryResult(errorsmod.Wrap(err, "failed to JSON encode simulation response"), app.trace)
}
return &abci.ResponseQuery{
Codespace: sdkerrors.RootCodespace,
Height: req.Height,
Value: bz,
}
.

Lmk if you have any more questions, happy to hop on a quick call to explain things if needed as well

@lucaslopezf
Copy link
Contributor

lucaslopezf commented Apr 10, 2024

What do you mean by nested messages? I saw a proto-definition (but it seems to be only for testing) and the DescriptorProto.

messages can embed other messages in them that will be executed at a later time (gov, etc..). These messages are not simulated so they could fail at a later date when the state machine goes to execute them.

simulation is exposed via an endpoint or cli. here is a link to the code that handles this. Simulating a transaction is used in order to see if a transaction will pass or fail when executing it.

cosmos-sdk/baseapp/abci.go

Lines 988 to 1010 in b4e7df2

case "simulate":
txBytes := req.Data
gInfo, res, err := app.Simulate(txBytes)
if err != nil {
return sdkerrors.QueryResult(errorsmod.Wrap(err, "failed to simulate tx"), app.trace)
}
simRes := &sdk.SimulationResponse{
GasInfo: gInfo,
Result: res,
}
bz, err := codec.ProtoMarshalJSON(simRes, app.interfaceRegistry)
if err != nil {
return sdkerrors.QueryResult(errorsmod.Wrap(err, "failed to JSON encode simulation response"), app.trace)
}
return &abci.ResponseQuery{
Codespace: sdkerrors.RootCodespace,
Height: req.Height,
Value: bz,
}

.
Lmk if you have any more questions, happy to hop on a quick call to explain things if needed as well

Ok, I understand what the simulation is and the meaning of a nested message (if it's possible I'd like to see an example, I still have doubts about what type the embedded messages have, I'd expect something like a

type msg struct {
 ...
 nested []msg
})

But then, what do you mean by the validation check of the nested message? What validation would be done in the simulation? Could you explain that? If not, I'd be happy to have a quick call

@tac0turtle
Copy link
Member Author

yes, gov submit proposal is a good example:

// MsgSubmitProposal defines an sdk.Msg type that supports submitting arbitrary
// proposal Content.
type MsgSubmitProposal struct {
// messages are the arbitrary messages to be executed if proposal passes.
Messages []*any.Any `protobuf:"bytes,1,rep,name=messages,proto3" json:"messages,omitempty"`
// initial_deposit is the deposit value that must be paid at proposal submission.
InitialDeposit github_com_cosmos_cosmos_sdk_types.Coins `protobuf:"bytes,2,rep,name=initial_deposit,json=initialDeposit,proto3,castrepeated=github.com/cosmos/cosmos-sdk/types.Coins" json:"initial_deposit"`
// proposer is the account address of the proposer.
Proposer string `protobuf:"bytes,3,opt,name=proposer,proto3" json:"proposer,omitempty"`
// metadata is any arbitrary metadata attached to the proposal.
Metadata string `protobuf:"bytes,4,opt,name=metadata,proto3" json:"metadata,omitempty"`
// title is the title of the proposal.
//
// Since: cosmos-sdk 0.47
Title string `protobuf:"bytes,5,opt,name=title,proto3" json:"title,omitempty"`
// summary is the summary of the proposal
//
// Since: cosmos-sdk 0.47
Summary string `protobuf:"bytes,6,opt,name=summary,proto3" json:"summary,omitempty"`
// expedited defines if the proposal is expedited or not
//
// Since: cosmos-sdk 0.50
// Deprecated: Use the PROPOSAL_TYPE_EXPEDITED proposal type instead.
// When this field is set and no proposal_type is set, the proposal_type
// will be set to PROPOSAL_TYPE_EXPEDITED for backwards compatibility.
Expedited bool `protobuf:"varint,7,opt,name=expedited,proto3" json:"expedited,omitempty"` // Deprecated: Do not use.
// proposal_type defines the type of proposal
// When not set defaults to PROPOSAL_TYPE_STANDARD
//
// Since: x/gov v1.0.0
ProposalType ProposalType `protobuf:"varint,8,opt,name=proposal_type,json=proposalType,proto3,enum=cosmos.gov.v1.ProposalType" json:"proposal_type,omitempty"`
}
. Here simulation runs on the outer message but the messages located in the submit proposal are not validated. When governance passes it could fail execution due to a misconfigured message. This has happened on Osmosis with 2 proposals. This issue is an attempt to allow to simulate messages within messages to have the message within run successfully at a later time.

@educlerici-zondax educlerici-zondax moved this from 📋 Backlog to 🤸‍♂️ In Progress in Cosmos-SDK Apr 11, 2024
@JulianToledano
Copy link
Contributor

After taking a look at this, I came to the conclusion that nested messages should be checked in their parent MsgHandler. The handler should always check:

  1. can route any nested message.
  2. all messages are valid.

In the case of SubmitProposal, messages are not checked deliberately:

// Only if it's a MsgExecLegacyContent we try to execute the
// proposal in a cached context.
// For other Msgs, we do not verify the proposal messages any further.
// They may fail upon execution.
// ref: https://github.com/cosmos/cosmos-sdk/pull/10868#discussion_r784872842
msg, ok := msg.(*v1.MsgExecLegacyContent)
if !ok {
continue
}

Here is the discussion about it. Maybe a cli command to simulate a proposals makes sense as pointed in the discussion.

@tac0turtle

@JulianToledano JulianToledano linked a pull request May 6, 2024 that will close this issue
12 tasks
@educlerici-zondax educlerici-zondax moved this from 🤸‍♂️ In Progress to 👀 Waiting / In review in Cosmos-SDK May 6, 2024
@github-project-automation github-project-automation bot moved this from 👀 Waiting / In review to 🥳 Done in Cosmos-SDK Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🥳 Done
Development

Successfully merging a pull request may close this issue.

5 participants