-
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
feat: add custom max gas for block for sim config #16656
Conversation
x/simulation/params.go
Outdated
@@ -176,7 +176,7 @@ func (w WeightedProposalContent) ContentSimulatorFn() simulation.ContentSimulato | |||
// Consensus Params | |||
|
|||
// randomConsensusParams returns random simulation consensus parameters, it extracts the Evidence from the Staking genesis state. | |||
func randomConsensusParams(r *rand.Rand, appState json.RawMessage, cdc codec.JSONCodec) *cmtproto.ConsensusParams { | |||
func randomConsensusParams(r *rand.Rand, appState json.RawMessage, cdc codec.JSONCodec, maxGas *int64) *cmtproto.ConsensusParams { |
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.
why not just pass -1 to indicate to not override?
x/simulation/params.go
Outdated
@@ -197,7 +197,9 @@ func randomConsensusParams(r *rand.Rand, appState json.RawMessage, cdc codec.JSO | |||
MaxAgeDuration: stakingGenesisState.Params.UnbondingTime, | |||
}, | |||
} | |||
|
|||
if maxGas != 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.
if maxGas != nil { | |
if maxGas > 0 { |
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.
seems pointer is easier to understand, since default value of integer is 0, it might be confused to pass -1
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 block max gas semantics are that any non-positive value is indicative of being "set". Why not just stick with that?
x/simulation/simulate.go
Outdated
blockMaxGas := int64(-1) | ||
if config.BlockMaxGas != nil { | ||
blockMaxGas = *config.BlockMaxGas | ||
} |
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.
ditto
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.
The zero value of int64 is 0, but 0 is a valid value for max gas. If we don't use a pointer, we cannot maintain a default behaviour of maxGas with value -1
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.
Why can't the default be -1
w/o using a pointer?
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.
updated, but we can't set maxGas as 0 unless pointer
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!
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.
Thank you @mmsqe!
CHANGELOG.md
Outdated
@@ -101,6 +101,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
* (cli) [#16209](https://github.com/cosmos/cosmos-sdk/pull/16209) Make `StartCmd` more customizable. | |||
* (types) [#16257](https://github.com/cosmos/cosmos-sdk/pull/16257) Allow setting the base denom in the denom registry. | |||
* (genutil) [#16046](https://github.com/cosmos/cosmos-sdk/pull/16046) Add "module-name" flag to genutil add-genesis-account to enable intializing module accounts at genesis. | |||
* (sims) [#16656](https://github.com/cosmos/cosmos-sdk/pull/16656) Add custom max gas for block for sim config with unlimited as default. |
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.
nit, can you place the changelog under unreleased?
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.
updated, is it possible that we backport to v0.46.x as well
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.
utACK
(cherry picked from commit 9b2fd7b)
(cherry picked from commit 9b2fd7b) # Conflicts: # x/simulation/params.go
(cherry picked from commit 9b2fd7b) # Conflicts: # x/simulation/params.go
) (cosmos#16731) Co-authored-by: mmsqe <mavis@crypto.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com>
) (cosmos#16731) Co-authored-by: mmsqe <mavis@crypto.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com>
* build(deps): bump cometbft to v0.34.29 (cosmos#16553) * fix(x/auth): ensure nil .BaseAccounts are reported in ModuleAccount.Validate (backport cosmos#16554) (cosmos#16570) Co-authored-by: Emmanuel T Odeke <emmanuel@orijtech.com> Co-authored-by: Julien Robert <julien@rbrt.fr> * fix: snapshotter's failure is not propogated (backport cosmos#16588) (cosmos#16604) Co-authored-by: yihuang <huang@crypto.com> * feat: add custom max gas for block for sim config (backport cosmos#16656) (cosmos#16731) Co-authored-by: mmsqe <mavis@crypto.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com> * fix: accaddr cachefix (backport cosmos#15433) (cosmos#16823) Co-authored-by: KyleMoser <KyleMoser@users.noreply.github.com> Co-authored-by: HuangYi <huang@crypto.com> * fix(cli): improve `prune` command ux (backport cosmos#16856) (cosmos#16876) Co-authored-by: Julien Robert <julien@rbrt.fr> * fix: query tx events with `>=` and `<=` operators (cosmos#16994) * docs: change bank multi-send command description (backport cosmos#16950) (cosmos#17019) * chore: prepare v0.46.14 (cosmos#16879) * refactor: add MigrateHandler to allow reuse migrate genesis related function (backport cosmos#17296) (cosmos#17301) Co-authored-by: mmsqe <mavis@crypto.com> Co-authored-by: Julien Robert <julien@rbrt.fr> * fix: resolve migration map in MigrateHandler (cosmos#17301) (cosmos#17302) * feat(x/gov): Emit VoterAddr (backport cosmos#17354) (cosmos#17357) Co-authored-by: Devon Bear <itsdevbear@berachain.com> Co-authored-by: Julien Robert <julien@rbrt.fr> * feat(x/gov): add MsgSubmitProposal SetMsgs method (backport cosmos#17387) (cosmos#17388) Co-authored-by: Julien Robert <julien@rbrt.fr> --------- Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Emmanuel T Odeke <emmanuel@orijtech.com> Co-authored-by: yihuang <huang@crypto.com> Co-authored-by: mmsqe <mavis@crypto.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com> Co-authored-by: KyleMoser <KyleMoser@users.noreply.github.com> Co-authored-by: Rootul P <rootulp@gmail.com> Co-authored-by: Devon Bear <itsdevbear@berachain.com>
) (cosmos#16731) Co-authored-by: mmsqe <mavis@crypto.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com>
Description
To avoid sim test fail when check unlimited max gas as invalid consensus params.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change