Gas management, estimation, limitation #963

cwgoes · 2018-05-07T17:14:31Z

See questions on issue.

codecov · 2018-05-08T15:42:11Z

Codecov Report

Merging #963 into develop will increase coverage by 0.44%.
The diff coverage is 70%.

@@             Coverage Diff             @@
##           develop     #963      +/-   ##
===========================================
+ Coverage    60.28%   60.73%   +0.44%     
===========================================
  Files           74       75       +1     
  Lines         3591     3716     +125     
===========================================
+ Hits          2165     2257      +92     
- Misses        1256     1288      +32     
- Partials       170      171       +1

cwgoes · 2018-05-08T23:39:08Z

Basic gas accounting implemented. Module structure could be cleaned up a bit.

Currently gas costs are fixed constants in files, probably should be governance parameters modifiable with ParameterChangeProposals.

ebuchman · 2018-05-10T02:55:43Z

baseapp/baseapp.go

+			if err != nil {
+				result = err.Result()
+			}
+			result = app.runTx(true, true, txBytes, tx)


I think we can do this through Query, but we can't just use runTx as is because we want a fresh throw-away state to run the simulation against.

If we do it as is, it will update our checktx state, which sounds appealing, but might not be good because then when we try to add it to the tendermint mempool it will call check tx and possibly fail (since the tx was already executed against the checktx state in the app without tendermint knowing).

So we probably need to refactor runTx to take a ctx so we can pass in the ctx we want and create something fresh every time simulate is called that just gets thrown away after.

Of course this also means that we cant run muiltiple dependent simulate in a row without waiting for block commits.

An alternative option would be to open another connection between Tendermint and App and expose that over the Tendermitn RPC but that would be much more involved ...

@jaekwon

Presently, runTx with the simulateDeliver flag doesn't write to the multistore -94c00a3.

cwgoes · 2018-05-10T18:23:18Z

Ref #979, now also addressed (not p2p queries though)

cwgoes · 2018-05-10T18:51:23Z

To-do:

Fix cross-dependency issue (why gaskvstore is in /types)
Add testcases
Update docs

cwgoes · 2018-05-11T18:28:17Z

Ready for first-pass review @rigelrozanski @ebuchman

cwgoes · 2018-05-11T18:31:54Z

Also @sunnya97 all the gas costs should be governance-controlled parameters once governance is merged (glad to help).

rigelrozanski · 2018-05-11T19:55:30Z

baseapp/baseapp.go

@@ -27,6 +28,7 @@ type BaseApp struct {
 	// initialized on creation
 	Logger     log.Logger
 	name       string               // application name from abci.Info
+	codec      *wire.Codec          // Amino codec


let's stick to cdc ? that's everywhere really

Updated to cdc.

cwgoes · 2018-05-14T16:27:06Z

Would be nice to add crypto wrapper operations with gas costs - however, a bit complex because the go-crypto PubKey interface abstracts over the underlying type. We could modify PubKey to include gas, or just have modules set gas costs themselves - the latter is done now, as only x/auth performs crypto operations.

cwgoes · 2018-05-15T14:25:15Z

Now includes p2p query functions, ref #979

ebuchman

Thanks Chris. This looks great - most comments can be later TODOs, but I think it would be nice if we didn't add gasLimit to NewBaseApp

ebuchman · 2018-05-15T22:52:46Z

baseapp/baseapp.go

-		msg := "application doesn't support queries"
-		return sdk.ErrUnknownRequest(msg).QueryResult()
+	path := req.Path
+	// "/app" prefix for special application queries


What if we ran strings.Split on / and switched on it ? Would be nice not to slice strings with magic numbers everywhere

Agreed, updated.

ebuchman · 2018-05-15T22:56:56Z

baseapp/baseapp.go

 }

 // txBytes may be nil in some cases, eg. in tests.
 // Also, in the future we may support "internal" transactions.
-func (app *BaseApp) runTx(isCheckTx bool, txBytes []byte, tx sdk.Tx) (result sdk.Result) {
+func (app *BaseApp) runTx(isCheckTx bool, simulateDeliver bool, txBytes []byte, tx sdk.Tx) (result sdk.Result) {


maybe runTx should take a runMode, an enum for checkTx, simulateDeliver, etc. Though probably premature until we know an actual third kind of runTx we need to support - ?

We already have three: check, simulate, deliver. Updated to use an enum.

ebuchman · 2018-05-15T23:05:18Z

store/gaskvstore.go

+func (gi *gasKVStore) Get(key []byte) (value []byte) {
+	gi.gasMeter.ConsumeGas(ReadCostFlat, "GetFlat")
+	value = gi.parent.Get(key)
+	// TODO overflow-safe math?


lets open an issue for it

ebuchman · 2018-05-15T23:08:18Z

x/bank/keeper.go

@@ -108,6 +108,7 @@ func (keeper ViewKeeper) HasCoins(ctx sdk.Context, addr sdk.Address, amt sdk.Coi
 //______________________________________________________________________________________________

 func getCoins(ctx sdk.Context, am sdk.AccountMapper, addr sdk.Address) sdk.Coins {
+	ctx.GasMeter().ConsumeGas(10, "getCoins")


let's get a list of constants for these ... possibly parameters one day ...

Switched to constants at top of file. Also, these gas costs are probably wrong / may be unnecessary with the GasKVStore.

ebuchman · 2018-05-15T23:15:37Z

baseapp/baseapp.go

@@ -398,6 +485,14 @@ func (app *BaseApp) runTx(isCheckTx bool, txBytes []byte, tx sdk.Tx) (result sdk
 		ctx = app.deliverState.ctx.WithTxBytes(txBytes)
 	}

+	// Create a new zeroed gas meter
+	ctx = ctx.WithGasMeter(sdk.NewGasMeter(app.txGasLimit))


doesn't the gas meter need to use the gas provided in the tx, rather than this app level limit ? Perhaps we need to do this set up in the anteHandler

Yes, this is wrong, thanks. Switched to ante handler.

Do we also need to check that tx.Fee.Gas <= app.txGasLimit, or will Tendermint do that for us?

we'll need to #1007

ebuchman · 2018-05-15T23:26:02Z

baseapp/baseapp.go

@@ -57,15 +62,17 @@ var _ abci.Application = (*BaseApp)(nil)

 // Create and name new BaseApp
 // NOTE: The db is used to store the version number for now.
-func NewBaseApp(name string, cdc *wire.Codec, logger log.Logger, db dbm.DB) *BaseApp {
+func NewBaseApp(name string, cdc *wire.Codec, logger log.Logger, db dbm.DB, txGasLimit sdk.Gas) *BaseApp {


Seems strange that txGasLimit would become so first class as to be an arg to NewBaseApp - is there a good reason for this ?

Also, BaseApp should probably have a ConsensusParams that mirrors Tendermint's - MaxGasPerTx is part of that , gets initialized by InitChain, and can be updated by EndBlock. Then we shouldnt need to pass txGasLimit in everywhere.

In the meantime anyways I'd rather use setters and defaults than add function args

Agreed on ConsensusParams, made an issue: #1007

Removed txGasLimit, if Tendermint enforces that we may not need it at all actually.

cwgoes · 2018-05-16T00:08:43Z

Ref #1008

ebuchman · 2018-05-16T01:59:11Z

baseapp/baseapp.go

+	if len(path) > 0 && path[0] == "" {
+		path = path[1:]
+	}
+	fmt.Sprintf("Path: %v\n", path)


ebuchman · 2018-05-16T02:04:37Z

baseapp/baseapp.go

@@ -210,9 +233,9 @@ func (app *BaseApp) initFromStore(mainKey sdk.StoreKey) error {
 // NewContext returns a new Context with the correct store, the given header, and nil txBytes.
 func (app *BaseApp) NewContext(isCheckTx bool, header abci.Header) sdk.Context {
 	if isCheckTx {
-		return sdk.NewContext(app.checkState.ms, header, true, nil, app.Logger)
+		return sdk.NewContext(app.checkState.ms, header, true, nil, app.Logger, 0)


can we remove passing the gaslimit here too ? Seems like we're not using it anymore (InfiniteGasMeter ?)

ebuchman · 2018-05-16T02:05:43Z

I can finish up and merge this, thanks!

there is no version 4.6.2, 4.2.1 is the latest 4.x release

cwgoes requested a review from ebuchman as a code owner May 7, 2018 17:14

cwgoes force-pushed the cwgoes/gas-guzzling branch from 5dadbed to d1e2465 Compare May 8, 2018 15:33

cwgoes force-pushed the cwgoes/gas-guzzling branch from e33ca82 to 94c00a3 Compare May 9, 2018 19:57

ebuchman reviewed May 10, 2018

View reviewed changes

cwgoes force-pushed the cwgoes/gas-guzzling branch from bad5413 to 48bad69 Compare May 10, 2018 18:23

cwgoes force-pushed the cwgoes/gas-guzzling branch from 552dfda to 1086e35 Compare May 11, 2018 15:34

cwgoes changed the title ~~WIP: Gas management, estimation, limitation~~ Gas management, estimation, limitation May 11, 2018

cwgoes requested a review from rigelrozanski May 11, 2018 18:28

rigelrozanski reviewed May 11, 2018

View reviewed changes

cwgoes changed the title ~~Gas management, estimation, limitation~~ WIP: Gas management, estimation, limitation May 14, 2018

cwgoes force-pushed the cwgoes/gas-guzzling branch from 79c248e to 3505ce7 Compare May 15, 2018 12:56

cwgoes changed the title ~~WIP: Gas management, estimation, limitation~~ Gas management, estimation, limitation May 15, 2018

cwgoes added 10 commits May 16, 2018 01:13

Add gas limit / gas consumed to context

46f9445

GasMeter & context updates

2699180

Changes to bank keeper for gas

efc7843

Basic gas impl, quick testcase

f0e4d24

Pass gas consumed back in result struct

ddb3b36

Linter fixes

af379b6

Swap to panic/recover version

1f8ef62

Catch out-of-gas panics

0951705

Add baseapp test for out-of-gas handling

ca4ef9a

GasKVStore WIP

c410ceb

cwgoes added 13 commits May 16, 2018 01:13

SimulateTx through Query

a240554

New store query prefixes (ref #979)

8c1c40b

Update changelog

2147203

Rebase

702ffaf

Move GasKVStore to /store

147cf9f

Minor fix, testcases

ce38d8f

PR comment: codec => cdc

a801874

ConsumeGas for pubkey.VerifyBytes

38716d5

Move to new version in changelog

4cfa99e

Add p2p filter functions & tests

d55ba2c

Update changelog

396bf74

Unexport verifyCost

4775437

Rebase changelog

0cc1c52

cwgoes force-pushed the cwgoes/gas-guzzling branch from be054dc to 0cc1c52 Compare May 15, 2018 23:16

ebuchman previously approved these changes May 15, 2018

View reviewed changes

cwgoes mentioned this pull request May 16, 2018

Add "ConsensusParams" to baseapp #1007

Closed

Address PR comments

4134bf9

cwgoes dismissed ebuchman’s stale review via 4134bf9 May 16, 2018 00:06

cwgoes added 2 commits May 16, 2018 02:18

Unexport RunTxMode (fix linter)

03e2207

Remove txGasLimit, update tests

3d5b048

ebuchman reviewed May 16, 2018

View reviewed changes

remove gasLimit from NewContext

4bdcad5

ebuchman approved these changes May 16, 2018

View reviewed changes

ebuchman merged commit e6d21c6 into develop May 16, 2018

ebuchman deleted the cwgoes/gas-guzzling branch May 16, 2018 02:19

This was referenced May 16, 2018

CheckTx gas estimation #962

Closed

Fixup query prefixes #979

Closed

chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this pull request Mar 1, 2024

Update join-mainnet.md (cosmos#963)

7a1b89b

there is no version 4.6.2, 4.2.1 is the latest 4.x release

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gas management, estimation, limitation #963

Gas management, estimation, limitation #963

cwgoes commented May 7, 2018 •

edited

Loading

codecov bot commented May 8, 2018 •

edited

Loading

cwgoes commented May 8, 2018

ebuchman May 10, 2018

cwgoes May 10, 2018

cwgoes commented May 10, 2018 •

edited

Loading

cwgoes commented May 10, 2018 •

edited

Loading

cwgoes commented May 11, 2018

cwgoes commented May 11, 2018

rigelrozanski May 11, 2018

cwgoes May 11, 2018

cwgoes commented May 14, 2018 •

edited

Loading

cwgoes commented May 15, 2018

ebuchman left a comment

ebuchman May 15, 2018

cwgoes May 15, 2018

ebuchman May 15, 2018

cwgoes May 15, 2018

ebuchman May 15, 2018

cwgoes May 15, 2018

ebuchman May 15, 2018

cwgoes May 15, 2018

cwgoes May 16, 2018

ebuchman May 15, 2018

cwgoes May 16, 2018

ebuchman May 16, 2018

ebuchman May 15, 2018

cwgoes May 16, 2018

cwgoes May 16, 2018

cwgoes commented May 16, 2018

ebuchman May 16, 2018

ebuchman May 16, 2018

ebuchman commented May 16, 2018

Gas management, estimation, limitation #963

Gas management, estimation, limitation #963

Conversation

cwgoes commented May 7, 2018 • edited Loading

codecov bot commented May 8, 2018 • edited Loading

Codecov Report

cwgoes commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented May 10, 2018 • edited Loading

cwgoes commented May 10, 2018 • edited Loading

cwgoes commented May 11, 2018

cwgoes commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented May 14, 2018 • edited Loading

cwgoes commented May 15, 2018

ebuchman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebuchman commented May 16, 2018

cwgoes commented May 7, 2018 •

edited

Loading

codecov bot commented May 8, 2018 •

edited

Loading

cwgoes commented May 10, 2018 •

edited

Loading

cwgoes commented May 10, 2018 •

edited

Loading

cwgoes commented May 14, 2018 •

edited

Loading