R4R: Support min fees-based anti spam strategy #2328

alessio · 2018-09-13T09:32:38Z

Create cosmos.toml configuration file and handle minimum_fees
setting/flag to provide validators with a simple and flexible
anti-spam mechanism.

Closes: #1921

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)

codecov · 2018-09-14T10:52:14Z

Codecov Report

Merging #2328 into develop will decrease coverage by 0.24%.
The diff coverage is 45.33%.

@@             Coverage Diff             @@
##           develop    #2328      +/-   ##
===========================================
- Coverage    64.97%   64.73%   -0.25%     
===========================================
  Files          135      137       +2     
  Lines         8418     8467      +49     
===========================================
+ Hits          5470     5481      +11     
- Misses        2587     2622      +35     
- Partials       361      364       +3

alexanderbez

Thanks @alessio -- left some minor initial feedback 👍

baseapp/baseapp.go

baseapp/options.go

alexanderbez · 2018-09-14T13:42:47Z

server/config/config.go

+func (c *Config) MinimumFees() sdk.Coins {
+	fees, err := sdk.ParseCoins(c.MinFees)
+	if err != nil {
+		panic(err)


panic(fmt.Sprintf("invalid minimum fees: %v", err))

server/config/config.go

server/config/toml.go

server/start.go

types/context.go

x/auth/ante.go

alexanderbez · 2018-09-14T13:52:04Z

x/auth/ante.go

+				if ctx.MinimumFees().Minus(fee.Amount).IsNotNegative() {
+					fee = NewStdFee(fee.Gas, ctx.MinimumFees()...)
+				}
+				// Validators reject any tx from the mempool with less than the required fee per gas


validators

Where does the rejection actually occur? If the fee amount is zero?

validators

ACKd.

Where does the rejection actually occur? If the fee amount is zero?

Currently the rejection occurs at deductFees().

zmanian · 2018-09-15T13:02:16Z

I think the way min-fees should related to gas prices is some absolute minimium value + a factor * gas consumed

baseapp/baseapp.go

types/context.go

alessio · 2018-09-17T09:04:02Z

@alexanderbez your comments were addressed, please review

alexanderbez · 2018-09-17T14:11:58Z

Hmmm I don't recall test_sim_gaia_nondeterminism failing on develop before (recently). Any chance this PR introduces any non-determinism?

alexanderbez

Left a few more minor questions and suggestions, but otherwise, it looks good.

server/config/config.go

types/context.go

x/auth/ante.go

cwgoes

Structure looks good, see comments.

PENDING.md

baseapp/baseapp.go

docs/getting-started/full-node.md

server/config/config.go

types/context.go

x/auth/ante.go

cwgoes · 2018-09-17T15:00:33Z

x/auth/ante.go

+			if ctx.IsCheckTx() && !simulate {
+				// required fees must be greater than the minimum set by the validator
+				if ctx.MinimumFees().Minus(fee.Amount).IsNotNegative() {
+					fee = NewStdFee(fee.Gas, ctx.MinimumFees()...)


Why are we setting fee here? Do we only want users to pay the minimum fee even if they set a higher one?

When MinimumFees > Fee (MinimumFees - Fee > 0), we pick MinimumFees

What if the user supplies Fee > MinimumFees?

We shouldn't auto set the fee to min fee if the provided fee isn't enough. There are several issues with that. It will cause a consensus fault, since every validator would subtract it differently. Even if this didn't happen, it provides a validator a way to drain your entire account.

What if the user supplies Fee > MinimumFees?

That just works.

We shouldn't auto set the fee to min fee if the provided fee isn't enough.

I thourougly agree, I've just stumbled upon such a case while performing some testing.

We should do neither - if the user didn't sign a transaction with high-enough fees, we should reject the transaction, not subtract fees from their account which they didn't sign. The only check we should perform is that the fees provided in the transaction are greater than the minimum fee - the user should always pay exactly the fee they specified.

Fixed. Please review @cwgoes @ValarDragon @alexanderbez

Mhmmm, what about refunds in the case where they specify a greater amount? Are we relying on the user to know a safe estimate ahead of time?

Are we relying on the user to know a safe estimate ahead of time?

Short answer: yes. A solution may be returning an estimate of the fees along with the gas in --dry-run mode

alessio · 2018-09-18T08:52:48Z

@alexanderbez I noticed that too - @cwgoes any help/clues are appreciated

x/auth/ante.go

x/auth/ante_test.go

Create cosmos.toml configuration file and handle minimum_fees setting/flag to provide validators with a simple and flexible anti-spam mechanism. Closes: #1921

cwgoes

Tested ACK

alexanderbez

One small minor remark, otherwise utACK 👍

x/auth/ante.go

alexanderbez

utACK -- thanks @alessio

alessio added the wip label Sep 13, 2018

alessio requested review from cwgoes, ebuchman and rigelrozanski as code owners September 13, 2018 09:32

alessio force-pushed the alessio/1921-minimum-fee branch from a333407 to e5edb2a Compare September 13, 2018 09:42

alessio added the help wanted label Sep 13, 2018

alessio force-pushed the alessio/1921-minimum-fee branch from e5edb2a to a986ac0 Compare September 14, 2018 10:46

alessio removed the help wanted label Sep 14, 2018

alessio requested review from alexanderbez and zmanian September 14, 2018 13:39

alexanderbez suggested changes Sep 14, 2018

View reviewed changes

alessio force-pushed the alessio/1921-minimum-fee branch 2 times, most recently from 227830b to b2ce856 Compare September 15, 2018 12:59

alessio force-pushed the alessio/1921-minimum-fee branch 2 times, most recently from d470c25 to a2c7116 Compare September 15, 2018 14:55

alessio requested a review from zramsay as a code owner September 15, 2018 16:03

alessio changed the title ~~WIP: Support min fees-based anti spam strategy~~ R4R: Support min fees-based anti spam strategy Sep 15, 2018

alessio added ready-for-review and removed wip labels Sep 15, 2018

alessio force-pushed the alessio/1921-minimum-fee branch from 2141405 to c6034f3 Compare September 15, 2018 16:05

ValarDragon reviewed Sep 15, 2018

View reviewed changes

baseapp/baseapp.go Outdated Show resolved Hide resolved

ValarDragon reviewed Sep 15, 2018

View reviewed changes

types/context.go Show resolved Hide resolved

alessio force-pushed the alessio/1921-minimum-fee branch from dd6a0a5 to 6e89aaa Compare September 17, 2018 09:04

alexanderbez suggested changes Sep 17, 2018

View reviewed changes

server/config/config.go Show resolved Hide resolved

types/context.go Outdated Show resolved Hide resolved

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

cwgoes suggested changes Sep 17, 2018

View reviewed changes

alessio force-pushed the alessio/1921-minimum-fee branch 3 times, most recently from b166630 to 0d354a6 Compare September 18, 2018 08:50

alexanderbez reviewed Sep 18, 2018

View reviewed changes

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

x/auth/ante_test.go Show resolved Hide resolved

Support min fees-based anti spam mechanism

62387d9

Create cosmos.toml configuration file and handle minimum_fees setting/flag to provide validators with a simple and flexible anti-spam mechanism. Closes: #1921

alessio force-pushed the alessio/1921-minimum-fee branch from 67e4a28 to 62387d9 Compare September 19, 2018 10:20

Amend PENDING.md

2ff2072

cwgoes approved these changes Sep 19, 2018

View reviewed changes

alexanderbez suggested changes Sep 19, 2018

View reviewed changes

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

ACK bez comment

04c6e6a

alexanderbez approved these changes Sep 19, 2018

View reviewed changes

Implement types.Coin{,s}.IsLT()

1cdf1b7

cwgoes merged commit b74a6a9 into develop Sep 19, 2018

cwgoes deleted the alessio/1921-minimum-fee branch September 19, 2018 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Support min fees-based anti spam strategy #2328

R4R: Support min fees-based anti spam strategy #2328

alessio commented Sep 13, 2018 •

edited

Loading

codecov bot commented Sep 14, 2018 •

edited

Loading

alexanderbez left a comment

alexanderbez Sep 14, 2018

alexanderbez Sep 14, 2018

alexanderbez Sep 14, 2018

alessio Sep 14, 2018

zmanian commented Sep 15, 2018

alessio commented Sep 17, 2018

alexanderbez commented Sep 17, 2018 •

edited

Loading

alexanderbez left a comment

cwgoes left a comment

cwgoes Sep 17, 2018

alessio Sep 17, 2018

alexanderbez Sep 17, 2018

ValarDragon Sep 17, 2018

alessio Sep 17, 2018

cwgoes Sep 18, 2018

alessio Sep 18, 2018

alexanderbez Sep 18, 2018

alessio Sep 18, 2018 •

edited

Loading

alessio commented Sep 18, 2018

cwgoes left a comment

alexanderbez left a comment

alexanderbez left a comment

R4R: Support min fees-based anti spam strategy #2328

R4R: Support min fees-based anti spam strategy #2328

Conversation

alessio commented Sep 13, 2018 • edited Loading

codecov bot commented Sep 14, 2018 • edited Loading

Codecov Report

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

zmanian commented Sep 15, 2018

alessio commented Sep 17, 2018

alexanderbez commented Sep 17, 2018 • edited Loading

alexanderbez left a comment

Choose a reason for hiding this comment

cwgoes 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

alessio Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

alessio commented Sep 18, 2018

cwgoes left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alessio commented Sep 13, 2018 •

edited

Loading

codecov bot commented Sep 14, 2018 •

edited

Loading

alexanderbez commented Sep 17, 2018 •

edited

Loading

alessio Sep 18, 2018 •

edited

Loading