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

Limit simulation gas #674

Merged
merged 2 commits into from
Nov 17, 2021
Merged

Limit simulation gas #674

merged 2 commits into from
Nov 17, 2021

Conversation

alpe
Copy link
Contributor

@alpe alpe commented Nov 17, 2021

Resolves #670

Adds a new startup param/config and ante handler to limit gas for simulations:

--wasm.simulation_gas_limit string                Set the max gas that can be spent when executing a simulation TX

@alpe alpe requested a review from ethanfrey November 17, 2021 13:52
@codecov
Copy link

codecov bot commented Nov 17, 2021

Codecov Report

Merging #674 (a9ecc3f) into master (090b8e9) will increase coverage by 0.02%.
The diff coverage is 65.38%.

❗ Current head a9ecc3f differs from pull request most recent head 69623aa. Consider uploading reports for the commit 69623aa to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #674      +/-   ##
==========================================
+ Coverage   60.22%   60.24%   +0.02%     
==========================================
  Files          48       48              
  Lines        5340     5361      +21     
==========================================
+ Hits         3216     3230      +14     
- Misses       1896     1902       +6     
- Partials      228      229       +1     
Impacted Files Coverage Δ
x/wasm/module.go 44.31% <0.00%> (-3.83%) ⬇️
x/wasm/types/types.go 52.20% <0.00%> (ø)
app/app.go 84.07% <66.66%> (+0.05%) ⬆️
app/ante.go 100.00% <100.00%> (ø)
x/wasm/keeper/ante.go 100.00% <100.00%> (ø)

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

lgtm. nice fix

return next(ctx, tx, simulate)
}

// apply custom node gas limit
Copy link
Member

Choose a reason for hiding this comment

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

Looks good. Maybe this could be "simplified"? But fine as is.

if d.gasLimit != nil {
  ctx = ctx.WithGasMeter(sdk.NewGasMeter(*d.gasLimit))
} else if maxGas := ctx.ConsensusParams().GetBlock().MaxGas; maxGas > 0 { 
  ctx = ctx.WithGasMeter(sdk.NewGasMeter(sdk.Gas(maxGas)))
}
return next(ctx, tx, simulate)

@@ -107,3 +111,77 @@ func TestCountTxDecorator(t *testing.T) {
})
}
}
func TestLimitSimulationGasDecorator(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

nice tests

@alpe alpe merged commit efe8774 into master Nov 17, 2021
// SimulationGasLimit is the max gas to be used in a tx simulation call.
// When not set the consensus max block gas is used instead
SimulationGasLimit *uint64
// SimulationGasLimit is the max gas to be used in a smart query contract call
Copy link
Contributor

Choose a reason for hiding this comment

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

Just taking a look at this. You mean, SmartQueryGasLimit.

faddat pushed a commit to notional-labs/wasmd that referenced this pull request Nov 25, 2021
* Limit simulation gas

* Put parameters on separate lines
faddat pushed a commit to notional-labs/wasmd that referenced this pull request Nov 27, 2021
* Limit simulation gas

* Put parameters on separate lines
@alpe alpe deleted the 670-simulation-ante branch May 10, 2023 08:52
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.

Prevent infinite gas consumption in simulation queries
3 participants