R4R: Enforce block maximum gas limit in DeliverTx #2795

rigelrozanski · 2018-11-13T18:10:40Z

Closes #2775

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

server/constructors.go

rigelrozanski · 2018-11-13T21:03:42Z

Cleanup needs to be done as this broke some other tests, but it appears the mechanism works as expected :)

baseapp/baseapp.go

codecov · 2018-11-14T19:25:29Z

Codecov Report

Merging #2795 into develop will increase coverage by 0.03%.
The diff coverage is 73.21%.

@@             Coverage Diff             @@
##           develop    #2795      +/-   ##
===========================================
+ Coverage    56.84%   56.88%   +0.03%     
===========================================
  Files          120      120              
  Lines         8298     8340      +42     
===========================================
+ Hits          4717     4744      +27     
- Misses        3263     3275      +12     
- Partials       318      321       +3

baseapp/baseapp.go

docs/spec/baseapp/WIP_abci_application.md

baseapp/baseapp.go

docs/spec/baseapp/WIP_abci_application.md

cmd/gaia/app/genesis.go

cwgoes

I think there's still a bug in gas handling, and I'm not sure why we need to store consensusParams.

Also looks like test_lint on CI still fails.

baseapp/baseapp.go

cwgoes · 2018-11-21T10:06:22Z

baseapp/baseapp.go

@@ -629,6 +702,12 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk
 			}
 		}

+		// consume block gas whether panic or not.


What happens if this panics again? Won't DeliverTx panic, whereas we want to just return an error?

What would specifically panic again?

The ConsumeGas call in the recover handler here.

If this panics it will be caught by the recover (just above this comment) - this comment simply notes that even if this consume gas panics, it will still affect the state of the BlockGasMeter which is important for ignoring future transactions... updating this comment to reflect this

No, it won't. I've added a demonstrative failing testcase: 2d3e1af.

Run go test ./baseapp/... -v, it panics with:

panic: (types.ErrorOutOfGas) (0xd2f3e0,0xc000909320) [recovered] panic: (types.ErrorOutOfGas) (0xd2f3e0,0xc000909320) goroutine 58 [running]: testing.tRunner.func1(0xc000183100) /usr/lib/go/src/testing/testing.go:792 +0x387 panic(0xd2f3e0, 0xc000909320) /usr/lib/go/src/runtime/panic.go:513 +0x1b9 github.com/cosmos/cosmos-sdk/types.(*basicGasMeter).ConsumeGas(0xc00090dc00, 0x9, 0xe25a9f, 0xf) /home/cwgoes/working/go/src/github.com/cosmos/cosmos-sdk/types/gas.go:82 +0xab github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).runTx.func1(0xc000479358, 0x2, 0xc0004790f0, 0xc0004790c8) /home/cwgoes/working/go/src/github.com/cosmos/cosmos-sdk/baseapp/baseapp.go:711 +0x33a github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).runTx(0xc000914000, 0xc000206302, 0x0, 0x0, 0x0, 0x10396c0, 0xc0009169f0, 0x0, 0x0, 0x0, ...) /home/cwgoes/working/go/src/github.com/cosmos/cosmos-sdk/baseapp/baseapp.go:771 +0x340

baseapp/baseapp_test.go

alexanderbez

Looks solid -- just a few tidbits of feedback 👍

alexanderbez · 2018-11-21T15:23:44Z

baseapp/baseapp.go

+	consensusParamsBz := mainStore.Get(mainConsensusParamsKey)
+	if consensusParamsBz != nil {
+		var consensusParams = &abci.ConsensusParams{}
+		err := proto.Unmarshal(consensusParamsBz, consensusParams)


We don't want to use a codec here?

not sure @jaekwon

switched to codec - Jae can switch back if there is a good reason. Otherwise yeah staying consistent makes sense

It should be proto because abci.ConsensusParams are protobuf messages.

baseapp/baseapp.go

alexanderbez · 2018-11-21T15:26:51Z

baseapp/baseapp.go

@@ -629,6 +702,12 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk
 			}
 		}

+		// consume block gas whether panic or not.


What would specifically panic again?

baseapp/baseapp_test.go

docs/reference/baseapp.md

types/gas.go

baseapp/baseapp.go

cwgoes

The panic/recover logic still isn't quite right, see #2795 (comment). I'd echo @alexanderbez's question on the codec.

baseapp/baseapp.go

rigelrozanski · 2018-11-22T17:42:47Z

@cwgoes addressed the panic statement

cwgoes

Tested ACK

baseapp/baseapp.go

alexanderbez

The changes LGTM from an implementation perspective. I still think there is confusion on the consensus params used in the BaseApp and the one in the context and I think a simple godoc can help remove said confusion...even if it'll be removed soon.

baseapp/baseapp.go

alexanderbez · 2018-11-23T16:42:49Z

baseapp/baseapp.go

@@ -220,6 +241,29 @@ func (app *BaseApp) setDeliverState(header abci.Header) {
 	}
 }

+// setConsensusParams memoizes the consensus params.


I think this further alludes to the statement above. I don't see the direct relationship between these params and the ones used via the context. I think at the very least the godoc should allude to what these consensus params are for.

Good question. The types/Context consensus params were never used. We don't need them in the context for now, and even if we need them we'd want to modify them via a keeper, so for now will delete types/Context.ConsensusParams.

jaekwon · 2018-11-25T01:46:16Z

Reviewing...

Co-Authored-By: jaekwon <jae@tendermint.com>

jaekwon · 2018-11-25T02:16:39Z

@cwgoes we need to store consensusParams because it's provided by InitChain, and never provided again, yet we need it for per-block max gas even upon app restart.

jaekwon · 2018-11-25T06:00:33Z

LGTM now.

ValarDragon · 2018-11-26T05:08:57Z

baseapp/baseapp.go

+	// TODO move this in the future to baseapp param store on main store.
+	consensusParams *abci.ConsensusParams
+
+	// spam prevention


probably good to indicate that its local spam prevention, not consensus

* bump ics to v2.2.0-provider-lsm * add ICS bump to changelog * Update .changelog/unreleased/features/2781-bump-ics.md Co-authored-by: Marius Poke <marius.poke@posteo.de> * Update .changelog/unreleased/api-breaking/2781-bump-ics.md Co-authored-by: Marius Poke <marius.poke@posteo.de> --------- Co-authored-by: Marius Poke <marius.poke@posteo.de>

rigelrozanski added 3 commits November 12, 2018 23:12

block gas meter working

2f73cf4

basic structure in place

956d351

modified app provider to pass genesis

ebaa394

rigelrozanski requested review from cwgoes and ebuchman as code owners November 13, 2018 18:10

jackzampolin mentioned this pull request Nov 13, 2018

Game of Stakes / gaia-9002 release checklist #2754

Closed

27 tasks

rigelrozanski added 3 commits November 13, 2018 14:27

compiling

3bf67b6

default infinite block gas meter

8069b2b

examples installing

2446830

rigelrozanski commented Nov 13, 2018

View reviewed changes

server/constructors.go Outdated Show resolved Hide resolved

rigelrozanski added 2 commits November 13, 2018 15:12

initial test case

eead278

passing test

7e6fcc0

rigelrozanski added 3 commits November 14, 2018 00:57

only use block gas for deliver

68e3b9a

fix baseapp tests

0d4dd87

add init chain block gas for gen-txs (all unit tests fixed)

5249064

rigelrozanski commented Nov 14, 2018

View reviewed changes

baseapp/baseapp.go Show resolved Hide resolved

rigelrozanski added 2 commits November 14, 2018 14:45

Merge remote-tracking branch 'origin/develop' into rigel/deliver-max-gas

7be5179

docs and pending

90217e2

rigelrozanski requested a review from zramsay as a code owner November 14, 2018 20:14

rigelrozanski changed the title ~~WIP: Enforce block maximum gas limit in DeliverTx~~ R4R: Enforce block maximum gas limit in DeliverTx Nov 14, 2018

rigelrozanski added ready-for-review labels Nov 14, 2018

rigelrozanski assigned jaekwon Nov 15, 2018

rigelrozanski requested a review from jaekwon November 15, 2018 03:58

cwgoes reviewed Nov 15, 2018

View reviewed changes

alexanderbez reviewed Nov 15, 2018

View reviewed changes

baseapp/baseapp.go Show resolved Hide resolved

docs/spec/baseapp/WIP_abci_application.md Outdated Show resolved Hide resolved

cmd/gaia/app/genesis.go Show resolved Hide resolved

rigelrozanski added 2 commits November 15, 2018 11:13

basic cwgoes comments

2a594fe

Merge remote-tracking branch 'origin/develop' into rigel/deliver-max-gas

4818e67

cwgoes suggested changes Nov 21, 2018

View reviewed changes

alexanderbez suggested changes Nov 21, 2018

View reviewed changes

lint

972377c

rigelrozanski mentioned this pull request Nov 22, 2018

Baseapp Params Store + Import/Export #2882

Closed

rigelrozanski and others added 3 commits November 22, 2018 00:30

address some comments while reviewing Jaes work

b4b61b8

extra max block gas test at limit

abed373

Add demonstrative failing testcase

2d3e1af

cwgoes suggested changes Nov 22, 2018

View reviewed changes

baseapp/baseapp.go Show resolved Hide resolved

rigelrozanski added 3 commits November 22, 2018 12:34

fix BlockGasRecovery

56fa7dc

Merge remote-tracking branch 'origin/develop' into rigel/deliver-max-gas

0861112

replaced proto with codec in baseapp

ce10ef2

cwgoes approved these changes Nov 22, 2018

View reviewed changes

cwgoes requested a review from sunnya97 November 23, 2018 14:18

alexanderbez reviewed Nov 23, 2018

View reviewed changes

baseapp/baseapp.go Show resolved Hide resolved

alexanderbez reviewed Nov 23, 2018

View reviewed changes

baseapp/baseapp.go Outdated Show resolved Hide resolved

alexanderbez approved these changes Nov 23, 2018

View reviewed changes

alexanderbez and others added 2 commits November 24, 2018 18:10

Apply suggestions from code review

5792e1d

Co-Authored-By: jaekwon <jae@tendermint.com>

Final fixes from review

819af35

rigelrozanski added 2 commits November 25, 2018 23:34

Merge remote-tracking branch 'origin/develop' into rigel/deliver-max-gas

0da2472

dep

f12ac43

rigelrozanski merged commit 8af2eb2 into develop Nov 26, 2018

rigelrozanski deleted the rigel/deliver-max-gas branch November 26, 2018 05:00

ValarDragon reviewed Nov 26, 2018

View reviewed changes

chengwenxi added a commit to chengwenxi/irishub that referenced this pull request Dec 12, 2018

Merge PRs: cosmos/cosmos-sdk#2795

3a426c0

chengwenxi mentioned this pull request Dec 12, 2018

R4R: Merge PRs from Cosmos-SDK 0.27 irisnet/irishub#815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Enforce block maximum gas limit in DeliverTx #2795

R4R: Enforce block maximum gas limit in DeliverTx #2795

rigelrozanski commented Nov 13, 2018 •

edited

Loading

rigelrozanski commented Nov 13, 2018

codecov bot commented Nov 14, 2018 •

edited

Loading

cwgoes left a comment •

edited

Loading

cwgoes Nov 21, 2018

alexanderbez Nov 21, 2018

cwgoes Nov 21, 2018

rigelrozanski Nov 22, 2018

cwgoes Nov 22, 2018

alexanderbez left a comment

alexanderbez Nov 21, 2018

rigelrozanski Nov 22, 2018

rigelrozanski Nov 22, 2018

jaekwon Nov 25, 2018

alexanderbez Nov 21, 2018

cwgoes left a comment

rigelrozanski commented Nov 22, 2018

cwgoes left a comment

alexanderbez left a comment

alexanderbez Nov 23, 2018

jaekwon Nov 25, 2018

jaekwon commented Nov 25, 2018

jaekwon commented Nov 25, 2018

jaekwon commented Nov 25, 2018

ValarDragon Nov 26, 2018

R4R: Enforce block maximum gas limit in DeliverTx #2795

R4R: Enforce block maximum gas limit in DeliverTx #2795

Conversation

rigelrozanski commented Nov 13, 2018 • edited Loading

rigelrozanski commented Nov 13, 2018

codecov bot commented Nov 14, 2018 • edited Loading

Codecov Report

cwgoes left a comment • edited Loading

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

alexanderbez 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

cwgoes left a comment

Choose a reason for hiding this comment

rigelrozanski commented Nov 22, 2018

cwgoes left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaekwon commented Nov 25, 2018

jaekwon commented Nov 25, 2018

jaekwon commented Nov 25, 2018

Choose a reason for hiding this comment

rigelrozanski commented Nov 13, 2018 •

edited

Loading

codecov bot commented Nov 14, 2018 •

edited

Loading

cwgoes left a comment •

edited

Loading