x/authz charge gas for expensive authorizations #8995

aleem1314 · 2021-03-25T10:49:05Z

Description

closes: #8984

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

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.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

orijbot · 2021-03-25T11:07:52Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=2bbe2f2a49de4f28a5198a34120bfae0

orijbot · 2021-03-25T16:50:20Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=26826fee5aca4443b536e70f38d6d6f6

orijbot · 2021-03-25T16:50:28Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=5b616c1269a8a2c1dea6631642610e30882496a8

orijbot · 2021-03-25T17:03:08Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=730121ea62695792cb283c95b7c674b5a5dbbc61

x/staking/types/authz.go

orijbot · 2021-03-25T17:13:56Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=fc020eed5c0e41348eb494bdad2c40e1

orijbot · 2021-03-25T17:14:05Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=be5c12fd5ada7674ac3ff619e3bc8bbe828b510f

x/staking/types/authz.go

orijbot · 2021-03-25T20:15:03Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=e7a3636723596a95812cafc8bba634e42a6d2d93

aaronc

Looks like a good start. Is this R4R?

…os-sdk into aleem/8984-authz-gas

orijbot · 2021-03-26T05:17:45Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=7a2b815380c885b58bd95b8157fcc1a0e62200fe

codecov · 2021-03-26T05:57:58Z

Codecov Report

Merging #8995 (f497bcc) into master (6e26ee9) will increase coverage by 0.12%.
The diff coverage is 51.85%.

❗ Current head f497bcc differs from pull request most recent head b1eebfc. Consider uploading reports for the commit b1eebfc to get more accurate results

@@            Coverage Diff             @@
##           master    #8995      +/-   ##
==========================================
+ Coverage   58.97%   59.10%   +0.12%     
==========================================
  Files         576      574       -2     
  Lines       32377    32241     -136     
==========================================
- Hits        19095    19055      -40     
+ Misses      11041    10950      -91     
+ Partials     2241     2236       -5

Impacted Files	Coverage Δ
x/bank/types/send_authorization.go	`61.11% <20.00%> (+61.11%)`	⬆️
x/authz/types/msgs.go	`53.19% <50.00%> (-0.66%)`	⬇️
x/staking/types/authz.go	`84.93% <57.14%> (-2.95%)`	⬇️
x/authz/simulation/operations.go	`80.32% <60.00%> (-0.79%)`	⬇️
x/authz/types/generic_authorization.go	`71.42% <60.00%> (+71.42%)`	⬆️
x/authz/keeper/keeper.go	`71.29% <100.00%> (ø)`
x/auth/keeper/keeper.go	`48.61% <0.00%> (-0.69%)`	⬇️
x/bank/keeper/keeper.go	`70.64% <0.00%> (-0.29%)`	⬇️
types/query/pagination.go	`80.00% <0.00%> (ø)`
x/upgrade/keeper/keeper.go	`76.51% <0.00%> (ø)`
... and 12 more

orijbot · 2021-03-26T07:24:49Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=bdfd3eeb81e13d81d329e281d8ab64fe04eeee0a

amaury1093

nits left, so pre-approving 🎉

types/tx/types.go

x/authz/types/msgs.go

x/staking/types/authz.go

Co-authored-by: Amaury <[email protected]>

orijbot · 2021-03-29T23:11:37Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=e80dbd0b663446c1aa75c6ace083a66f

orijbot · 2021-03-29T23:11:38Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=dd22b432b8ca40aca44d3a1deafc92ab

orijbot · 2021-03-29T23:11:40Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=71aa27c4700a44289a7a7e7e51f26018

orijbot · 2021-03-29T23:12:43Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=71900e768fb28de493d5472dffed1353271dc550

orijbot · 2021-03-29T23:13:16Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=75b4ef85f35f6f6f04cdc320788364160c538330

orijbot · 2021-03-29T23:13:27Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=82aed7737681416f5a0943e201daa7a467b985ee

tac0turtle · 2021-03-30T08:15:12Z

x/staking/types/authz.go

@@ -68,13 +80,16 @@ func (authorization StakeAuthorization) Accept(msg sdk.ServiceMsg, block tmproto
 	isValidatorExists := false
 	allowedList := authorization.GetAllowList().GetAddress()
 	for _, validator := range allowedList {
+		ctx.GasMeter().ConsumeGas(gasCostPerIteration, "stake authorization")


how do external clients know how much this number is? Are they required to dive into the code?

They are not able to know exact number of gas. It would require a simulated run.
You can't do it, because one message can trigger many operations. And I don't think it would make sense to give exact numbers to clients. However, we should provide some estimates based on simulations.

Could you possibly help with that @robert-zaremba ?

tac0turtle · 2021-03-30T08:15:25Z

x/staking/types/authz.go

 	denyList := authorization.GetDenyList().GetAddress()
 	for _, validator := range denyList {
+		ctx.GasMeter().ConsumeGas(gasCostPerIteration, "stake authorization")


tac0turtle · 2021-03-30T08:18:37Z

Thank you for the PR. I like where this is going, but it's hard to justify a hard coded value for gas like this. I may have missed something, but it doesn't seem there is a way for clients to know the cost of this transaction. What is wrong with adding a new method to Module.BasicManager that allows modules to register custom gas costs of a tx allowing clients to see the cost?

I may also be conflating gas and fees here.

clevinson · 2021-04-07T16:40:15Z

@robert-zaremba suggesting on our call that we go with 10 (per iteration) for gas charge.

atheeshp

LGTM, couple of nits.

x/staking/types/authz_test.go

cyberbono3 · 2021-04-09T01:35:50Z

x/authz/types/generic_authorization.go

+
+// ValidateBasic implements Authorization.ValidateBasic.
+func (authorization GenericAuthorization) ValidateBasic() error {
+	if !msgservice.IsServiceMsg(authorization.MessageName) {


I am not sure if counting of "/" in IsServiceMsg is sufficient to check if typeURL satisfies service method name type e.g. i.e. /cosmos.bank.Msg/Send

WIP

42f2949

Add consume gas

5b616c1

aleem1314 added 2 commits March 25, 2021 22:32

update authz.go

6d40f03

Merge branch 'master' of into aleem/8984-authz-gas

730121e

aleem1314 added the C:x/authz label Mar 25, 2021

add build flag

e7a3636

aleem1314 commented Mar 25, 2021

View reviewed changes

x/staking/types/authz.go Outdated Show resolved Hide resolved

Update x/staking/types/authz.go

be5c12f

aleem1314 commented Mar 25, 2021

View reviewed changes

x/staking/types/authz.go Outdated Show resolved Hide resolved

Update x/staking/types/authz.go

bdfd3ee

aleem1314 requested a review from aaronc March 25, 2021 17:19

add tests docs

f40136b

aaronc reviewed Mar 25, 2021

View reviewed changes

Merge branch 'aleem/8984-authz-gas' of https://github.com/cosmos/cosm…

7a2b815

…os-sdk into aleem/8984-authz-gas

aleem1314 marked this pull request as ready for review March 26, 2021 05:19

aleem1314 requested review from alessio and alexanderbez as code owners March 26, 2021 05:19

aleem1314 requested review from amaury1093, anilcse and robert-zaremba March 26, 2021 05:19

amaury1093 approved these changes Mar 29, 2021

View reviewed changes

types/tx/types.go Outdated Show resolved Hide resolved

x/authz/types/msgs.go Outdated Show resolved Hide resolved

x/staking/types/authz.go Show resolved Hide resolved

aleem1314 and others added 3 commits March 29, 2021 19:21

Update x/authz/types/msgs.go

82aed77

Co-authored-by: Amaury <[email protected]>

review changes

71900e7

Merge branch 'master' into aleem/8984-authz-gas

75b4ef8

tac0turtle reviewed Mar 30, 2021

View reviewed changes

amaury1093 requested a review from cyberbono3 March 31, 2021 17:18

aleem1314 mentioned this pull request Apr 6, 2021

x/authz & x/feegrant extra gas fee #9054

Closed

4 tasks

aleem1314 and others added 2 commits April 8, 2021 11:17

Merge branch 'master' into aleem/8984-authz-gas

5f75011

update gas cost

f497bcc

amaury1093 approved these changes Apr 8, 2021

View reviewed changes

atheeshp self-requested a review April 8, 2021 10:48

atheeshp approved these changes Apr 8, 2021

View reviewed changes

x/staking/types/authz_test.go Outdated Show resolved Hide resolved

x/staking/types/authz_test.go Outdated Show resolved Hide resolved

review changes

71b7093

cyberbono3 reviewed Apr 9, 2021

View reviewed changes

cyberbono3 approved these changes Apr 9, 2021

View reviewed changes

Merge branch 'master' into aleem/8984-authz-gas

b1eebfc

robert-zaremba approved these changes Apr 9, 2021

View reviewed changes

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 9, 2021

mergify bot merged commit 8a376a4 into master Apr 9, 2021

mergify bot deleted the aleem/8984-authz-gas branch April 9, 2021 12:33

technicallyty mentioned this pull request Apr 15, 2021

x/authz & x/feegrant: documentation for gas consumption #9118

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/authz charge gas for expensive authorizations #8995

x/authz charge gas for expensive authorizations #8995

aleem1314 commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

aaronc left a comment

orijbot commented Mar 26, 2021

codecov bot commented Mar 26, 2021 •

edited

Loading

orijbot commented Mar 26, 2021

amaury1093 left a comment

orijbot commented Mar 29, 2021

orijbot commented Mar 29, 2021

orijbot commented Mar 29, 2021

orijbot commented Mar 29, 2021

orijbot commented Mar 29, 2021

orijbot commented Mar 29, 2021

tac0turtle Mar 30, 2021

robert-zaremba Mar 30, 2021

aaronc Mar 30, 2021

tac0turtle Mar 30, 2021

tac0turtle commented Mar 30, 2021

clevinson commented Apr 7, 2021

atheeshp left a comment

cyberbono3 Apr 9, 2021 •

edited

Loading

x/authz charge gas for expensive authorizations #8995

x/authz charge gas for expensive authorizations #8995

Conversation

aleem1314 commented Mar 25, 2021

Description

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

aaronc left a comment

Choose a reason for hiding this comment

orijbot commented Mar 26, 2021

codecov bot commented Mar 26, 2021 • edited Loading

Codecov Report

orijbot commented Mar 26, 2021

amaury1093 left a comment

Choose a reason for hiding this comment

orijbot commented Mar 29, 2021

orijbot commented Mar 29, 2021

orijbot commented Mar 29, 2021

orijbot commented Mar 29, 2021

orijbot commented Mar 29, 2021

orijbot commented Mar 29, 2021

tac0turtle Mar 30, 2021

Choose a reason for hiding this comment

robert-zaremba Mar 30, 2021

Choose a reason for hiding this comment

aaronc Mar 30, 2021

Choose a reason for hiding this comment

tac0turtle Mar 30, 2021

Choose a reason for hiding this comment

tac0turtle commented Mar 30, 2021

clevinson commented Apr 7, 2021

atheeshp left a comment

Choose a reason for hiding this comment

cyberbono3 Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Mar 26, 2021 •

edited

Loading

cyberbono3 Apr 9, 2021 •

edited

Loading