feat: make global min gas price gov modifiable #3094

ninabarbakadze · 2024-02-08T16:24:16Z

Overview

Makes GlobalMinGasPrice hard-coded constant gov modifiable

Fixes - #3092

Co-authored-by: Callum Waters <[email protected]>

Co-authored-by: Rootul P <[email protected]>

ninabarbakadze · 2024-03-19T00:00:24Z

Do you want to update the specs in this PR or a follow-up PR? We have to update CIP-13 in a follow-up PR because separate repo. Do you mind creating an issue or PR in the cips repo to track?

Ref: #3092 (comment)

[question] did we use the Cosmos Hub globalfee module as inspiration for this?

I updated the params.md to include this new param. we still need to update state_machine_modules.md with link to our new modules + minfee module once it's merged. I will create an issue for it.
I tried to follow the pattern in our existing modules.

cmwaters

Great work!

rootulp

+1 great work!

rootulp · 2024-03-19T14:27:11Z

app/ante/fee_checker.go

-// unit of gas is fixed and set globally, and the tx priority is computed from the gas price.
-func CheckTxFeeWithGlobalMinGasPrices(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, int64, error) {
+// CheckTxFeeWithMinGasPrices implements default fee validation logic for transactions.
+// It ensures that the provided transaction fee meets a minimum threshold for the validator


Ahh now I remember that. I think that means we can close #3034 as won't do.

[optional] we could update the comment here to say that the config minimum only applies for CheckTx and the global minimum applies for Prepare/ProcessProposal.

Update: it's described in the body of this function so definitely optional / not blocking.

rootulp · 2024-03-19T14:33:11Z

app/ante/min_fee_test.go

+		{
+			name:       "bad tx; fee below node's required minimum",
+			fee:        sdk.NewCoins(sdk.NewInt64Coin(appconsts.BondDenom, feeAmount-1)),
+			gasLimit:   uint64(float64(feeAmount) / validatorMinGasPrice),
+			appVersion: uint64(1),
+			isCheckTx:  true,
+			expErr:     true,
+		},


rootulp · 2024-03-19T14:34:13Z

specs/src/specs/params.md

@@ -64,5 +64,6 @@ are blocked by the `x/paramfilter` module.
 | staking.MaxValidators                         | 100                                         | Maximum number of validators.                                                                                                                                                                   | True                      |
 | staking.MinCommissionRate                     | 0.05 (5%)                                   | Minimum commission rate used by all validators.                                                                                                                                                 | True                      |
 | staking.UnbondingTime                         | 1814400 (21 days)                           | Duration of time for unbonding in seconds.                                                                                                                                                      | False                     |
+| minfee.GlobalMinGasPrice                      | 0.002                                        | All transactions must have a gas price greater than or equal to this value                                                                                                                             | True                      |


[nit] this param table is alphabetically sorted. Can you please move minfee up?

[tangent] we don't have to figure this out in this PR but I wonder if it would make sense to include the app version that a constant is applicable for in this table. For example, I think some of these params could be modified to be different in different app versions and at that point it's not immediately obvious if this table should reflect the original or updated value. Perhaps both with a new column for app version?

yes, i can move it in a follow up pr. thanks @rootulp

to you second point i think that's a good call. I'll export your comment as an issue and we can discuss it during our grooming session.

evan-forbes · 2024-03-20T18:11:31Z

app/ante/ante.go

+
+// The purpose of this wrapper is to enable the passing of an additional paramKeeper parameter
+// whilst still satisfying the ante.TxFeeChecker type.
+func CheckTxFeeWithGlobalMinGasPricesWrapper(paramKeeper paramkeeper.Keeper) ante.TxFeeChecker {


[optional]

the name CheckTx could be confusing

evan-forbes · 2024-03-20T18:13:12Z

app/ante/fee_checker.go

+	if ctx.IsCheckTx() {
+		minGasPrice := ctx.MinGasPrices().AmountOf(appconsts.BondDenom)
+		if !minGasPrice.IsZero() {
+			err := verifyMinFee(fee, gas, minGasPrice, "insufficient minimum gas price for this validator")


[optoinal]

since all nodes will run this, validator might be confusing to users

evan-forbes · 2024-03-20T18:14:39Z

thanks for testing rigorously!

only had optional naming related things left!

ninabarbakadze · 2024-03-20T18:41:52Z

thanks for testing rigorously!

only had optional naming related things left!

thanks evan! I'll update in a follow up pr.

## Overview follow-up pr addressing some of the comments on [minfee-module](#3094) ---------

Makes GlobalMinGasPrice hard-coded constant gov modifiable Fixes - celestiaorg#3092 ---------

## Overview follow-up pr addressing some of the comments on [minfee-module](celestiaorg#3094) ---------

## Overview follow-up pr addressing some of the comments on [minfee-module](celestiaorg/celestia-app#3094) ---------

ninabarbakadze changed the title ~~Nina/gov modifiable gmgp~~ feat: make GlobalMinGasPrice gov modifiable Feb 8, 2024

ninabarbakadze changed the title ~~feat: make GlobalMinGasPrice gov modifiable~~ feat: make global min gas price gov modifiable Feb 8, 2024

rootulp assigned ninabarbakadze Feb 8, 2024

ninabarbakadze force-pushed the nina/gov-modifiable-gmgp branch 4 times, most recently from d471c49 to 0cfa5bb Compare February 13, 2024 22:45

go sum

29701b5

ninabarbakadze force-pushed the nina/gov-modifiable-gmgp branch 2 times, most recently from 79c9be2 to 90742ce Compare March 13, 2024 19:34

ninabarbakadze added 6 commits March 13, 2024 20:37

feat: make GlobalMinGasPrice gov modifiable

eeb6dc7

fix: make it compile

595a09d

test: min fee test refactor

d6f65f6

feat: set initial value in app

7c47e19

fix: pass correct subspace

6d86876

chore: cleanup

2396e5b

ninabarbakadze force-pushed the nina/gov-modifiable-gmgp branch from 90742ce to 2396e5b Compare March 13, 2024 19:38

ninabarbakadze added 7 commits March 13, 2024 23:12

chore: cleanup

ff550b2

refactor: some tests and nits

258501d

refactor: more cleanup and lint

e1773f8

fix: go work sum

d241821

lint: make linter happy

826be91

fix: the failing test

942f225

chore: merge

efcdbb2

ninabarbakadze force-pushed the nina/gov-modifiable-gmgp branch 2 times, most recently from 1b47b3e to efcdbb2 Compare March 14, 2024 22:04

ninabarbakadze added 4 commits March 15, 2024 00:42

fix: all the remaining tests

73ff45b

chore: merge with main

29dcc6f

Merge branch 'main' into nina/gov-modifiable-gmgp

2d20731

chore: remove local go mod replace

a78e076

Update app/ante/fee_checker.go

fa7f72a

Co-authored-by: Callum Waters <[email protected]>

celestia-bot requested a review from a team March 18, 2024 18:54

ninabarbakadze and others added 4 commits March 18, 2024 19:54

Update app/ante/fee_checker.go

d9cd014

Co-authored-by: Callum Waters <[email protected]>

Update x/minfee/genesis.go

fd36acd

Co-authored-by: Rootul P <[email protected]>

Update x/minfee/genesis.go

5b6e87b

Co-authored-by: Rootul P <[email protected]>

Update app/ante/ante.go

23edbf3

Co-authored-by: Rootul P <[email protected]>

ninabarbakadze marked this pull request as draft March 18, 2024 19:29

ninabarbakadze added 3 commits March 19, 2024 00:21

chore: apply review changes

279b975

docs: add GlobalMinGasPrice to params list

23b2d42

Merge branch 'main' into nina/gov-modifiable-gmgp

85101fc

ninabarbakadze marked this pull request as ready for review March 19, 2024 00:00

ninabarbakadze requested review from liamsi and adlerjohn as code owners March 19, 2024 00:00

cmwaters approved these changes Mar 19, 2024

View reviewed changes

rootulp approved these changes Mar 19, 2024

View reviewed changes

ninabarbakadze mentioned this pull request Mar 19, 2024

Make constants version specific in specs #3191

Closed

rootulp mentioned this pull request Mar 20, 2024

feat: use nodes min gas price in prepare proposal #3034

Closed

evan-forbes approved these changes Mar 20, 2024

View reviewed changes

ninabarbakadze merged commit 8bacf9b into celestiaorg:main Mar 20, 2024
34 checks passed

ninabarbakadze deleted the nina/gov-modifiable-gmgp branch March 20, 2024 18:42

cmwaters mentioned this pull request Mar 21, 2024

make test-e2e fails #3010

Open

This was referenced Mar 21, 2024

Update CIP-13 to add minfee.GlobalMinGasPrice celestiaorg/CIPs#113

Open

refactor: minor naming changes and updates in specs #3204

Merged

ninabarbakadze added a commit that referenced this pull request Mar 21, 2024

refactor: minor naming changes and updates in specs (#3204)

d5a9362

## Overview follow-up pr addressing some of the comments on [minfee-module](#3094) ---------

ninabarbakadze added a commit to ninabarbakadze/celestia-app that referenced this pull request Apr 2, 2024

feat: make global min gas price gov modifiable (celestiaorg#3094)

d493e61

Makes GlobalMinGasPrice hard-coded constant gov modifiable Fixes - celestiaorg#3092 ---------

ninabarbakadze added a commit to ninabarbakadze/celestia-app that referenced this pull request Apr 2, 2024

refactor: minor naming changes and updates in specs (celestiaorg#3204)

6251e58

## Overview follow-up pr addressing some of the comments on [minfee-module](celestiaorg#3094) ---------

0xchainlover pushed a commit to celestia-org/celestia-app that referenced this pull request Aug 1, 2024

refactor: minor naming changes and updates in specs (#3204)

cef771b

## Overview follow-up pr addressing some of the comments on [minfee-module](celestiaorg/celestia-app#3094) ---------

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make global min gas price gov modifiable #3094

feat: make global min gas price gov modifiable #3094

ninabarbakadze commented Feb 8, 2024 •

edited

Loading

ninabarbakadze commented Mar 19, 2024

cmwaters left a comment

rootulp left a comment

rootulp Mar 19, 2024

rootulp Mar 19, 2024

rootulp Mar 19, 2024

rootulp Mar 19, 2024

ninabarbakadze Mar 19, 2024 •

edited

Loading

evan-forbes Mar 20, 2024

evan-forbes Mar 20, 2024

evan-forbes commented Mar 20, 2024 •

edited

Loading

ninabarbakadze commented Mar 20, 2024

feat: make global min gas price gov modifiable #3094

feat: make global min gas price gov modifiable #3094

Conversation

ninabarbakadze commented Feb 8, 2024 • edited Loading

Overview

ninabarbakadze commented Mar 19, 2024

cmwaters left a comment

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

rootulp Mar 19, 2024

Choose a reason for hiding this comment

rootulp Mar 19, 2024

Choose a reason for hiding this comment

rootulp Mar 19, 2024

Choose a reason for hiding this comment

rootulp Mar 19, 2024

Choose a reason for hiding this comment

ninabarbakadze Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

evan-forbes Mar 20, 2024

Choose a reason for hiding this comment

evan-forbes Mar 20, 2024

Choose a reason for hiding this comment

evan-forbes commented Mar 20, 2024 • edited Loading

ninabarbakadze commented Mar 20, 2024

ninabarbakadze commented Feb 8, 2024 •

edited

Loading

ninabarbakadze Mar 19, 2024 •

edited

Loading

evan-forbes commented Mar 20, 2024 •

edited

Loading