R4R: Consume Gas Proportional to Tx Size #3447

alexanderbez · 2019-01-30T15:58:25Z

Remove memo (per byte) cost
Introduce tx size (per byte) cost
Update simulation
- BaseApp#Simulate now takes tx bytes because it needs to be set on the context

closes: #3256

Targeted PR against correct branch (see CONTRIBUTING.md)
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)

x/auth/ante.go

x/auth/params.go

codecov · 2019-01-31T15:08:51Z

Codecov Report

Merging #3447 into develop will decrease coverage by 0.02%.
The diff coverage is 71.42%.

@@             Coverage Diff             @@
##           develop    #3447      +/-   ##
===========================================
- Coverage    52.97%   52.94%   -0.03%     
===========================================
  Files          137      137              
  Lines        10719    10741      +22     
===========================================
+ Hits          5678     5687       +9     
- Misses        4698     4710      +12     
- Partials       343      344       +1

client/flags.go

cwgoes

A few questions but mostly looks good.

client/flags.go

x/auth/params.go

x/mock/test_utils.go

x/auth/ante.go

alexanderbez · 2019-01-31T23:42:23Z

address your comments @cwgoes -- lmk if the cost logic makes sense.

cwgoes · 2019-02-01T01:29:18Z

address your comments @cwgoes -- lmk if the cost logic makes sense.

I think that's fine; we should consider the total tx cost but in the broader context of #1008 (comment).

jackzampolin · 2019-02-01T02:00:26Z

Hey @mossid can you take a look at this too?

alessio · 2019-02-02T00:08:09Z

x/auth/ante.go

+		// contain a pubkey, so we must account for tx size of including a
+		// StdSignature (Amino encoding) and simulate gas consumption
+		// (assuming a SECP256k1 simulation key).
+		simSig := StdSignature{PubKey: pubKey}


In the multisig case, gas estimate would be wrong here

we don't support multi-sig for estimation afaik -- correct me if I'm wrong.

Can we with a flag or something?

@cwgoes multisig is tricky nonetheless - signature verification strictly depends on the signature itself, i.e. given a N-of-M multikey and a multisig signature with P sub sigs, all sub signatures get always processed/verified, even when N < P. Knowing the original N-of-M key might help obtaining a more accurate estimate (i.e. you know that you would process at least N signature), though it does not solve the problem.

How about just estimating the upper bound by default?

(I agree that the problem is tricky but we should optimize for the average use case and relatively low cost of paying a bit extra gas)

Updated to account for multisigs.

alessio

gas estimate in multisig txs is controversial - approving

jackzampolin · 2019-02-02T03:06:12Z

Looks like we are still failing some tests here.

alexanderbez · 2019-02-04T22:38:53Z

@cwgoes @alessio @jackzampolin updated to account for multisigs and fixed CI regression.

Aleksandr Bezobchuk added 3 commits January 30, 2019 10:56

add TxSizeCostPerByte to x/auth params

a0688a7

add TxSizeCostPerByte to x/auth simulation params

b769bae

consume gas proportional to tx size in the ante handler

df79cc0

alexanderbez added wip labels Jan 30, 2019

alexanderbez requested review from alessio, jackzampolin and cwgoes January 30, 2019 15:58

alexanderbez requested review from ebuchman and rigelrozanski as code owners January 30, 2019 15:58

alexanderbez commented Jan 30, 2019

View reviewed changes

x/auth/ante.go Outdated Show resolved Hide resolved

Aleksandr Bezobchuk added 2 commits January 30, 2019 11:53

add gas wanted and gas used to OOG logs

cbb6562

remove memo cost per byte

3d781c9

alexanderbez commented Jan 30, 2019

View reviewed changes

x/auth/params.go Show resolved Hide resolved

Aleksandr Bezobchuk added 2 commits January 30, 2019 16:57

update baseapp to handle simulation nuances for tx size gas costs

f4af664

fix linting errors

b4f4cb1

cwgoes mentioned this pull request Jan 30, 2019

Double-check our gas prices #1008

Closed

4 tasks

move GasEstimateResponse to utils and implement Stringer

2a36fb9

Aleksandr Bezobchuk added 2 commits January 31, 2019 10:42

remove XXX simulation in favor of increased gas adjustment

8a3d2f9

add pending log entry

284a206

alexanderbez commented Jan 31, 2019

View reviewed changes

client/flags.go Outdated Show resolved Hide resolved

revert printing gas estimate back to STDERR

9f872bb

alexanderbez changed the title ~~WIP: Consume Gas Proportional to Tx Size~~ R4R: Consume Gas Proportional to Tx Size Jan 31, 2019

alexanderbez added ready-for-review and removed wip labels Jan 31, 2019

cwgoes suggested changes Jan 31, 2019

View reviewed changes

client/flags.go Outdated Show resolved Hide resolved

x/auth/params.go Show resolved Hide resolved

x/mock/test_utils.go Outdated Show resolved Hide resolved

x/auth/ante.go Show resolved Hide resolved

Aleksandr Bezobchuk added 3 commits January 31, 2019 14:55

simulate with tx bytes in x/mock

dc95156

revert gas adjustment change in favor of encoding a dummy sig

79d9779

update pending log entry

9a3cb6f

Aleksandr Bezobchuk added 2 commits January 31, 2019 18:14

update SignCheckDeliver to accept a codec so all txs can be encoded

c07d05a

update fees/gas spec

9e1d5cb

cwgoes approved these changes Feb 1, 2019

View reviewed changes

alessio reviewed Feb 2, 2019

View reviewed changes

Merge branch 'develop' into bez/3256-gas-tx-size

70b17b4

alessio approved these changes Feb 2, 2019

View reviewed changes

Aleksandr Bezobchuk added 3 commits February 4, 2019 13:51

fix sig gas cost and support multi-sigs

2d70732

Merge branch 'develop' into bez/3256-gas-tx-size

7cd2c5f

bump up sig encoding size

3b57468

jackzampolin merged commit 0822951 into develop Feb 4, 2019

jackzampolin deleted the bez/3256-gas-tx-size branch February 4, 2019 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Consume Gas Proportional to Tx Size #3447

R4R: Consume Gas Proportional to Tx Size #3447

alexanderbez commented Jan 30, 2019 •

edited

Loading

codecov bot commented Jan 31, 2019 •

edited

Loading

cwgoes left a comment

alexanderbez commented Jan 31, 2019

cwgoes commented Feb 1, 2019

jackzampolin commented Feb 1, 2019

alessio Feb 2, 2019

alexanderbez Feb 2, 2019

cwgoes Feb 2, 2019

alessio Feb 3, 2019

cwgoes Feb 4, 2019

alexanderbez Feb 4, 2019

alessio left a comment

jackzampolin commented Feb 2, 2019

alexanderbez commented Feb 4, 2019

R4R: Consume Gas Proportional to Tx Size #3447

R4R: Consume Gas Proportional to Tx Size #3447

Conversation

alexanderbez commented Jan 30, 2019 • edited Loading

codecov bot commented Jan 31, 2019 • edited Loading

Codecov Report

cwgoes left a comment

Choose a reason for hiding this comment

alexanderbez commented Jan 31, 2019

cwgoes commented Feb 1, 2019

jackzampolin commented Feb 1, 2019

alessio Feb 2, 2019

Choose a reason for hiding this comment

alexanderbez Feb 2, 2019

Choose a reason for hiding this comment

cwgoes Feb 2, 2019

Choose a reason for hiding this comment

alessio Feb 3, 2019

Choose a reason for hiding this comment

cwgoes Feb 4, 2019

Choose a reason for hiding this comment

alexanderbez Feb 4, 2019

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

jackzampolin commented Feb 2, 2019

alexanderbez commented Feb 4, 2019

alexanderbez commented Jan 30, 2019 •

edited

Loading

codecov bot commented Jan 31, 2019 •

edited

Loading