feat: protocol level dynamic gas price #2544

piux2 · 2024-07-08T08:39:53Z

BREAKING CHANGE: addition block header, state value and block parameters are added. It may fail on playing back the blocks produced in previous implementation.

Main idea

This is Inspired by EIP 1559, it adjusts gas price based on ratio of gas used last bock and the target block gas.

the compressor is used to reduce the impact on price due to sudden changes on

When the last gas used in a block is above the target gas, we increase the gas price

When the last gas used in a block is below the target gas, we decrease the gas price until it returns to the initial gas price in the block.

Impact

The Cosmos SDK has an optional setting for a minimum gas price. Each validator can configure their own values to only accept transactions with a gas price that meets their setting in the mempool. When a user submits a transaction on-chain, the gas price is calculated as gas-fee / gas-wanted.

With the addition of the block gas price, a network-wide minimum gas price is enforced for every validator. Users will need to provide a gas price that meets the requirements set by both the validator and the network.

Implementation flow

Play back block and Commit block:

The consensus engine sends the last gas price in the block header and block parameters to the app during InitChain().
In EndBlock(), the base app calculates the updated gas price using the formula, updates the value in the header, and sends the header back to the consensus engine.

Accept transactions in the mempool

The consensus mempool sends the last gas price in the block header to the app at checkTx()
The app checks the received transaction's gas fee and gas wanted against the last block's gas price and the validator's minimum gas setting in the ante handler to decide if the transaction is accepted in the mempool and propagated to other nodes.

Changes

Added Initial GasPrice, Target Gas, and Price Change Compressor in Block Parameters, Consensus.Block.
Added GasPrice in the Block Header.
Updated the ante handler to check both the minimum gas price set by the validator and the gas price required by the network.
Added logic to calculate the updated gas price at EndBlock() in the base app.
Included block header in ResponseEndBlock
Updated the integration testing to allow changes to the genesisDoc at the start.
Added a blockparams command in txtar to allow testing different block parameter configurations.
Added telemetry metrics to monitor gas price changes.
Added unit testing cases and integration testing cases.

This is the initial implementation, we need more thorough testing and cross checking on different spamming scenarios

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
[ X] No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

codecov · 2024-07-08T08:42:36Z

Codecov Report

Attention: Patch coverage is 58.54545% with 114 lines in your changes missing coverage. Please review.

Project coverage is 60.19%. Comparing base (5f28803) to head (2fa574b).
Report is 67 commits behind head on master.

Files with missing lines	Patch %	Lines
gno.land/pkg/integration/testing_integration.go	44.44%	44 Missing and 6 partials ⚠️
tm2/pkg/sdk/baseapp.go	63.82%	13 Missing and 4 partials ⚠️
tm2/pkg/std/gasprice.go	0.00%	15 Missing ⚠️
tm2/pkg/bft/types/params.go	56.25%	9 Missing and 5 partials ⚠️
tm2/pkg/telemetry/metrics/metrics.go	0.00%	6 Missing ⚠️
gno.land/pkg/gnoland/node_inmemory.go	0.00%	4 Missing ⚠️
gno.land/pkg/integration/testing_node.go	75.00%	3 Missing and 1 partial ⚠️
tm2/pkg/bft/types/block.go	75.00%	3 Missing ⚠️
tm2/pkg/bft/consensus/state.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2544      +/-   ##
==========================================
+ Coverage   60.14%   60.19%   +0.04%     
==========================================
  Files         560      560              
  Lines       74738    75128     +390     
==========================================
+ Hits        44950    45222     +272     
- Misses      26400    26485      +85     
- Partials     3388     3421      +33

Flag	Coverage Δ
contribs/gnodev	`61.40% <ø> (+0.81%)`	⬆️
contribs/gnofaucet	`14.46% <ø> (ø)`
gno.land	`63.56% <51.26%> (-0.60%)`	⬇️
gnovm	`64.28% <ø> (ø)`
misc/genstd	`80.54% <ø> (ø)`
misc/logos	`20.23% <ø> (+0.35%)`	⬆️
tm2	`62.02% <63.15%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

deelawn

I've made some suggestions and asked a few questions. Looks mostly good to me, but I'm not the most familiar with the tm2 code.

deelawn · 2024-08-07T20:16:18Z

tm2/pkg/bft/types/params.go

+	// PriceChangeCompressor is to reduce the impact of gas changes to gas price.
+	PriceChangeCompressor int64 = 8 // 1/8 = 0.125
+
+	// BlockTimeIotaMS is the block time iota (in ms)


Update comment

tm2/pkg/bft/types/params.go

deelawn · 2024-08-07T20:24:37Z

tm2/pkg/std/coin.go

@@ -623,6 +623,9 @@ var (
 	reCoin      = regexp.MustCompile(fmt.Sprintf(`^(%s)%s(%s)$`, reAmt, reSpc, reDnmString))
 )

+// export validateDenom
+var ValidateDenom func(string) error = validateDenom


I know this is probably done to reduce the number of changes, but there are only five other places this function is called, so might as well just export it directly.

deelawn · 2024-08-07T20:28:32Z

gno.land/cmd/gnoland/genesis_generate.go

@@ -145,9 +145,15 @@ func execGenerate(cfg *generateCfg, io commands.IO) error {

 // getDefaultGenesis returns the default genesis config
 func getDefaultGenesis() *types.GenesisDoc {
+	bp := types.DefaultBlockParams()
+	bp.InitialGasPriceAmount = 0


Why is this set to zero? I notice this happens in most of the cases after types.DefaultBlockParams is called.

deelawn · 2024-08-07T20:40:28Z

tm2/pkg/std/gasprice.go

I think it would be better to name this file gas_price.go.

tm2/pkg/sdk/auth/ante.go

deelawn · 2024-08-08T00:30:20Z

tm2/pkg/sdk/auth/ante.go

@@ -379,43 +379,54 @@ func DeductFees(bank BankKeeperI, ctx sdk.Context, acc std.Account, fees std.Coi
 // consensus.
 func EnsureSufficientMempoolFees(ctx sdk.Context, fee std.Fee) sdk.Result {
 	minGasPrices := ctx.MinGasPrices()
+	blockHeader := ctx.BlockHeader().(*bft.Header)


If we want to make this tm2 package to be useful for others in the future, I'm not sure this should be doing a type assertion on this interface. Instead could the BlockHeader interface be modified to include a method that returns this gas price data?

deelawn · 2024-08-08T00:39:11Z

tm2/pkg/sdk/baseapp.go

+
+// it calculates the minGasPrice for the txs to be included in the next block.
+// minGasPrice = lastPrice + lastPrice*(gasUsed-TargetBlockGas)/TargetBlockGas/GasCompressor)
+func (app *BaseApp) calBlockMinPriceAmount(gasUsed int64, lastPrice int64) int64 {


Can you name this calcBlockMinPriceAmount instead?

deelawn · 2024-08-08T00:40:42Z

tm2/pkg/sdk/baseapp.go

+		num = max(num, big.NewInt(minGasPrice))
+	}
+
+	if !num.IsInt64() {


I have a bunch of questions about this.

Should this really panic here?

Would this halt the chain?

Can it be set to the max int64 value in this case?

Is there an attack vector that could easily exploit this?

deelawn · 2024-08-08T01:06:27Z

tm2/pkg/sdk/baseapp.go

+	c := app.consensusParams.Block.PriceChangeCompressor
+
+	lastMinPrice := big.NewInt(lastPrice)
+	if gasUsed > targetGas { // increase gas price


Could this calculation be done in a way that makes it a bit easier to follow based on the formula definition? Something like this?

// Declare big integers for the calculation. minGasPriceBig := big.NewInt(0) // result value lastPriceBig := big.NewInt(lastPrice) gasUsedBig := big.NewInt(gasUsed) targetGasBig := big.NewInt(targetGas) gasCompressorBig := big.NewInt(app.consensusParams.Block.PriceChangeCompressor) // Formula: // minGasPrice = lastPrice + lastPrice*(gasUsed-TargetBlockGas)/TargetBlockGas/GasCompressor) minGasPriceBig.Sub(gasUsedBig, targetGasBig) minGasPriceBig.Mul(minGasPriceBig, lastPriceBig) minGasPriceBig.Div(minGasPriceBig, targetGasBig) minGasPriceBig.Div(minGasPriceBig, gasCompressorBig) minGasPriceBig.Add(minGasPriceBig, lastPriceBig)

The math formula is just an abstraction of a simple solution for the underlying problem we're trying to solve.

What do we do if the gas used is less than the target gas in a block?

How do we bring the gas used back to the target level, if gas used is more than the target?

We simplify the solution with a one-line formula to explain the idea. However, in reality, we need to treat two scenarios differently. For example, in the first case, we need to increase the gas by at least 1 unit, instead of round down for the integer divisions, and in the second case, we should set a floor as the target gas price.

This is just a starting point. Down the line, the solution might not be even representable by one simple formula

jaekwon · 2024-08-15T23:19:42Z

tm2/pkg/bft/abci/types/types.go

+	PriceChangeCompressor int64  // must be >0
+	InitialGasPriceAmount int64  // must be >=0
+	InitialGasPriceDenom  string // must not be ""
+	InitialGasPriceGas    int64  // must be >=1


to remove; instead add these to GnoGenesisState.Auth.

feat: protocol level dynamic gas price

2d015b2

piux2 requested review from gfanton, zivkovicmilos, a team, jaekwon and moul as code owners July 8, 2024 08:39

github-actions bot assigned piux2 Jul 8, 2024

github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Jul 8, 2024

piux2 changed the title ~~feat: protocol level dynamic gas price~~ DRAFT: feat: protocol level dynamic gas price Jul 8, 2024

piux2 changed the title ~~DRAFT: feat: protocol level dynamic gas price~~ WIP: feat: protocol level dynamic gas price Jul 8, 2024

more tests

205b701

zivkovicmilos added the breaking change Functionality that contains breaking changes label Jul 22, 2024

Kouteki mentioned this pull request Jul 26, 2024

Minutes: Core Staff Weekly Syncs [every Monday] gnolang/meetings#36

Open

piux2 added 4 commits July 28, 2024 19:41

fix: updated state

48681ff

Merge branch 'master' of https://github.com/piux2/gno into feat_gasprice

fec6f5a

updated integration test and telemetry

f712345

added txtar files

0cd7cb3

piux2 changed the title ~~WIP: feat: protocol level dynamic gas price~~ feat: protocol level dynamic gas price Jul 29, 2024

piux2 added 4 commits July 29, 2024 03:17

added new testing in memory node API

708a565

fix update block params test

ec55ca8

lint

57dc456

Merge remote-tracking branch 'origin/master' into feat_gasprice

2fa574b

michelleellen mentioned this pull request Jul 31, 2024

Minutes: Public Dev Calls [every Wednesday] gnolang/meetings#37

Open

deelawn reviewed Aug 8, 2024

View reviewed changes

jaekwon reviewed Aug 15, 2024

View reviewed changes

piux2 mentioned this pull request Sep 24, 2024

feat: dynamic gas price, keeper implementation #2838

Open

7 tasks

piux2 closed this Sep 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: protocol level dynamic gas price #2544

feat: protocol level dynamic gas price #2544

piux2 commented Jul 8, 2024 •

edited

Loading

codecov bot commented Jul 8, 2024 •

edited

Loading

deelawn left a comment

deelawn Aug 7, 2024

deelawn Aug 7, 2024

deelawn Aug 7, 2024

deelawn Aug 7, 2024

deelawn Aug 8, 2024

deelawn Aug 8, 2024

deelawn Aug 8, 2024

deelawn Aug 8, 2024

piux2 Aug 15, 2024 •

edited

Loading

jaekwon Aug 15, 2024

feat: protocol level dynamic gas price #2544

feat: protocol level dynamic gas price #2544

Conversation

piux2 commented Jul 8, 2024 • edited Loading

Main idea

Impact

Implementation flow

Play back block and Commit block:

Accept transactions in the mempool

Changes

codecov bot commented Jul 8, 2024 • edited Loading

Codecov Report

deelawn 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

piux2 Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piux2 commented Jul 8, 2024 •

edited

Loading

codecov bot commented Jul 8, 2024 •

edited

Loading

piux2 Aug 15, 2024 •

edited

Loading