refactor: bypass-msgs #2218

yaruwangway · 2023-02-17T12:36:18Z

closes: #2213
Issue description please check #2213

please note this is not the final refactor of bypass-msg, bypass-msg will go to store as global params.

codecov · 2023-02-17T12:40:21Z

Codecov Report

Merging #2218 (4c991c3) into main (53a07ee) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2218      +/-   ##
==========================================
+ Coverage   83.83%   83.84%   +0.01%     
==========================================
  Files          22       22              
  Lines        1540     1541       +1     
==========================================
+ Hits         1291     1292       +1     
  Misses        201      201              
  Partials       48       48

Impacted Files	Coverage Δ
ante/ante.go	`50.00% <100.00%> (ø)`
app/app.go	`75.65% <100.00%> (+0.21%)`	⬆️
x/globalfee/ante/fee.go	`87.87% <100.00%> (-0.13%)`	⬇️
x/globalfee/ante/fee_utils.go	`88.79% <100.00%> (ø)`

yaruwangway · 2023-02-21T14:49:04Z

x/globalfee/ante/fee.go

 	// Otherwise, minimum fees and global fees are checked to prevent spam.
 	containsOnlyBypassMinFeeMsgs := mfd.bypassMinFeeMsgs(msgs)
-	doesNotExceedMaxGasUsage := gas <= uint64(len(msgs))*mfd.MaxBypassMinFeeMsgGasUsage
+	doesNotExceedMaxGasUsage := gas <= mfd.MaxTotalBypassMinFeeMsgGasUsage
 	allowedToBypassMinFee := containsOnlyBypassMinFeeMsgs && doesNotExceedMaxGasUsage


Hi @alexanderbez , can you please help confirm here, it is ok to refactor uint64(len(msgs))*mfd.MaxBypassMinFeeMsgGasUsage to MaxTotalBypassMinFeeMsgGasUsage ?
without upgrade

MaxBypassMinFeeMsgGasUsage is 200_000,
MaxTotalBypassMinFeeMsgGasUsage is 1_000_000

No, you cannot change gas consumed in a soft upgrade (i.e. a point release). If a node is replaying or catching up, it will yield in a consensus fault (e.g. you said gas was X but it's Y for me)

Even if MaxTotalBypassMinFeeMsgGasUsage isn't checked in DeliverTx ?

gaia/x/globalfee/ante/fee.go

Lines 79 to 84 in 44d5e05

// Only check for minimum fees and global fee if the execution mode is CheckTx

if !ctx.IsCheckTx() || simulate {

return next(ctx, tx, simulate)

}

if !allowedToBypassMinFee {

yaruwangway · 2023-03-01T11:56:11Z

docs/modules/globalfee.md

-
-Nodes created using Gaiad `v7.0.1` or earlier do not have `bypass-min-fee-msg-types` configured in `config/app.toml` - they are also using default values. The `bypass-min-fee-msg-types` config option can be added to `config/app.toml` before the `[telemetry]` field.
+- Nodes created using Gaiad `v7.0.2` or later use `["/ibc.core.channel.v1.MsgRecvPacket", "/ibc.core.channel.v1.MsgAcknowledgement","/ibc.applications.transfer.v1.MsgTransfer"]` as defaults. 
+- Nodes created using Gaiad `v9.0.1` or later use `["/ibc.core.channel.v1.MsgRecvPacket", "/ibc.core.channel.v1.MsgAcknowledgement","/ibc.applications.transfer.v1.MsgTransfer", "/ibc.core.channel.v1.MsgTimeout", "/ibc.core.channel.v1.MsgTimeoutOnClose"]` as defaults. 


This version need to be updated, it will not be included in v9.0.1. This PR is consensus breaking.

robert-zaremba

You can see our (Umee) ante handler implementation for bypassing fee for certain type (and max gas) of messages: https://github.com/umee-network/umee/blob/main/ante/fee.go#L22

mergify · 2023-03-15T14:37:32Z

⚠️ The sha of the head commit of this PR conflicts with #2259. Mergify cannot evaluate rules on this PR. ⚠️

ante/ante.go

sainoe

LGTM

x/globalfee/ante/antetest/fee_test.go

MSalopek

Looks good but some changes are necessary. We should not drop NewGovPreventSpamDecorator.

ante/ante.go

docs/modules/globalfee.md

x/globalfee/ante/antetest/fee_test.go

Co-authored-by: MSalopek <35486649+MSalopek@users.noreply.github.com>

docs/modules/globalfee.md

MSalopek

Looks good!

Thank you for making the changes.

Yaru Wang added 3 commits February 17, 2023 13:07

refactor: maxBypassMinFeeMsgGasUsage -> maxTotalBypassMinFeeMsgGasUsage

b26588a

add more bypass-msg-types to default bypass-msg

9ace1a4

docs: update bypass-msg in globalfee.md

34459c9

test: add tests for bypass-msg

ed407c0

yaruwangway commented Feb 21, 2023

View reviewed changes

yaruwangway mentioned this pull request Feb 21, 2023

refactor bypass-min-fee-msg-types #2213

Closed

11 tasks

yaruwangway changed the title ~~refactor: bypass-msgs~~ refactor: bypass-msgs for v10 Feb 21, 2023

yaruwangway mentioned this pull request Feb 22, 2023

Reformat global fee #2229

Closed

refactor: containsOnlyBypassMinFeeMsgs

7be1e72

yaruwangway changed the title ~~refactor: bypass-msgs for v10~~ refactor: bypass-msgs Mar 1, 2023

yaruwangway mentioned this pull request Mar 1, 2023

Refactor: global fee #2259

Closed

yaruwangway requested a review from ancazamfir March 1, 2023 11:54

yaruwangway commented Mar 1, 2023

View reviewed changes

robert-zaremba reviewed Mar 15, 2023

View reviewed changes

merge main

b5ccab3

yaruwangway marked this pull request as ready for review March 15, 2023 14:56

yaruwangway requested review from zmanian, crodriguezvega, jackzampolin, mmulji-ic, glnro, jtremback, mpoke, sainoe, MSalopek and shaspitz as code owners March 15, 2023 14:56

sainoe reviewed Mar 16, 2023

View reviewed changes

ante/ante.go Show resolved Hide resolved

sainoe approved these changes Mar 16, 2023

View reviewed changes

sainoe reviewed Mar 16, 2023

View reviewed changes

x/globalfee/ante/antetest/fee_test.go Show resolved Hide resolved

MSalopek requested changes Mar 16, 2023

View reviewed changes

ante/ante.go Show resolved Hide resolved

docs/modules/globalfee.md Outdated Show resolved Hide resolved

docs/modules/globalfee.md Outdated Show resolved Hide resolved

x/globalfee/ante/antetest/fee_test.go Show resolved Hide resolved

Yaru Wang and others added 3 commits March 16, 2023 11:29

docs: update changelog

3d0b902

Apply suggestions from code review

82a7274

Co-authored-by: MSalopek <35486649+MSalopek@users.noreply.github.com>

docs: update comments on bypass-min-fee-msgs

21ff8b1

yaruwangway commented Mar 16, 2023

View reviewed changes

docs/modules/globalfee.md Outdated Show resolved Hide resolved

Update docs/modules/globalfee.md

508f1a4

yaruwangway requested a review from MSalopek March 16, 2023 17:12

MSalopek approved these changes Mar 16, 2023

View reviewed changes

Yaru Wang added 8 commits March 16, 2023 23:34

merge main

f9964c3

workflow: increase upgrade test timeout

7b04c97

workflow: increase upgrade test timeout

f708f6e

workflow: use latest cosmovisor in upgrade test

52ac3fc

fix typo

9bf484b

fix: upgrade test

480b2fb

fix: cosmovisor start in upgrade test

8ed9ab7

change timeout for upgrade test in workflow

4c991c3

yaruwangway merged commit 483fb4d into main Mar 17, 2023

yaruwangway deleted the yaru/bypass-msg branch March 17, 2023 13:25

sainoe mentioned this pull request May 2, 2023

Plan: Global Fee module refactor #2270

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: bypass-msgs #2218

refactor: bypass-msgs #2218

yaruwangway commented Feb 17, 2023 •

edited

Loading

codecov bot commented Feb 17, 2023 •

edited

Loading

yaruwangway Feb 21, 2023

alexanderbez Feb 21, 2023 •

edited

Loading

sainoe Mar 16, 2023

yaruwangway Mar 1, 2023

robert-zaremba left a comment

mergify bot commented Mar 15, 2023

sainoe left a comment

MSalopek left a comment

MSalopek left a comment

	// Only check for minimum fees and global fee if the execution mode is CheckTx
	if !ctx.IsCheckTx() \|\| simulate {
	return next(ctx, tx, simulate)
	}

	if !allowedToBypassMinFee {

refactor: bypass-msgs #2218

refactor: bypass-msgs #2218

Conversation

yaruwangway commented Feb 17, 2023 • edited Loading

codecov bot commented Feb 17, 2023 • edited Loading

Codecov Report

yaruwangway Feb 21, 2023

Choose a reason for hiding this comment

alexanderbez Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

sainoe Mar 16, 2023

Choose a reason for hiding this comment

yaruwangway Mar 1, 2023

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 15, 2023

sainoe left a comment

Choose a reason for hiding this comment

MSalopek left a comment

Choose a reason for hiding this comment

MSalopek left a comment

Choose a reason for hiding this comment

yaruwangway commented Feb 17, 2023 •

edited

Loading

codecov bot commented Feb 17, 2023 •

edited

Loading

alexanderbez Feb 21, 2023 •

edited

Loading