Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(testnet): add two new parameters --base-fee and --min-gas-price to the command evmosd testnet init-files #1864

Merged
merged 20 commits into from Nov 2, 2023

Conversation

luchenqun
Copy link
Contributor

@luchenqun luchenqun commented Oct 14, 2023

Description

Use the command evmosd testnet init-files --v 1 --output-dir ./nodes --chain-id evmos_9000-1 --keyring-backend test to generate the genesis.json file for the initial block. The feemarket params are as follows:

{
    "no_base_fee":false,
    "base_fee_change_denominator":8,
    "elasticity_multiplier":2,
    "enable_height":"0",
    "base_fee":"1000000000",
    "min_gas_price":"0.000000000000000000",
    "min_gas_multiplier":"0.500000000000000000"
}

In the genutil module, there is at least one genesis transaction for creating a validator, with the following content:

{
    "body":{
        "messages":[
            {
                "@type":"/cosmos.staking.v1beta1.MsgCreateValidator",
                "validator_address":"evmosvaloper1hajh...ur4y3khljfx82"
            }
        ]
    },
    "auth_info":{
        "fee":{
            "amount":[],
            "gas_limit":"0"
        }
    }
}

Based on the params in feemarket, The gasPrice for the transaction executed in block 0 must be max(base_fee, min_gas_price) = max(1000000000, 0.000000000000000000) = 1gwei

However, the MsgCreateValidator transaction does not meet the requirements of the feemarket params because the amount array in the fee is empty and the gas_limit is 0.

So why can this transaction be executed correctly? First, regarding the gasPrice check, when checking the transaction fee in NewDynamicFeeChecker, if the block height is 0, the check does not use the global fee, which is the feemarket related params. Instead, it uses the local fee, which is set in the app.toml as minimum-gas-prices. The default value for this parameter is 0aevmos. You can find the relevant code here: https://github.com/evmos/evmos/blob/v14.1.0/app/ante/evm/fee_checker.go#L27-L93

One possible issue that may arise from this is that when modifying the local fee, it may not be possible to start the chain using the genesis block. However, when I changed the local fee to minimum-gas-prices = "6aevmos" before start the chain using the genesis block, you can still use the genesis block to start the chain. In other words, although the checkTxFeeWithValidatorMinGasPrices function has been executed, it did not take effect.

After analyzing, this is because when executing transactions in the genutil module at block height 0, it directly skips the CheckTx stage and goes into DeliverTx. In the DeliverTx stage, ctx.minGasPrice is always an empty array. Only in the CheckTx stage is ctx.minGasPrice the one we set as 6aevmos. Therefore, checking the transaction fees with checkTxFeeWithValidatorMinGasPricesfor the transactions in the genutil module at block 0 has no meaning.

In #1760, I mentioned that we can treat transactions in genutil as special transactions. This way, I can modify the min_gas_price parameter in the genesis block's feemarket to be greater than 0, allowing the execution of transactions within the genutil module even when the transaction fee is 0. However, this suggestion was rejected. There was a suggestion to manually modify the transactions in the genutil module, but I find it inconvenient. I hope that the genesis.json file generated by evmosd testnet init-files can directly run without manual intervention.

I think the best solution would be to treat the transactions in the genutil module as regular transactions without any special handling, meaning that they must also comply with the feemarket params. When generating the genesis block using the evmosd testnet init-files command, the generated feemarket params would have a default min_gas_price of 0, which is not suitable for real scenarios. The solution is to specify the --base-fee and --min-gas-price parameters when executing the evmosd testnet init-files command to adjust the fees in the genutil module transactions based on the base fee and min gas price.

Ultimately, after implementing this feature, if you run the evmosd testnet init-files --v 1 --output-dir ./nodes --chain-id evmos_9000-1 --keyring-backend test --base-fee 10000000000 --min-gas-price 800000000 command, the generated feemarket parameters would be as follows:

{
    "no_base_fee":false,
    "base_fee_change_denominator":8,
    "elasticity_multiplier":2,
    "enable_height":"0",
    "base_fee":"10000000000",
    "min_gas_price":"800000000.000000000000000000",
    "min_gas_multiplier":"0.500000000000000000"
}

genutil generates the genesis block transaction based on the feemarket params. The content is roughly as follows:

{
    "body":{
        "messages":[
            {
                "@type":"/cosmos.staking.v1beta1.MsgCreateValidator",
                "validator_address":"evmosvaloper1hajh6rh....3khljfx82"
            }
        ]
    },
    "auth_info":{
        "fee":{
            "amount":[
                {
                    "denom":"aevmos",
                    "amount":"3000000000000000"
                }
            ],
            "gas_limit":"300000"
        }
    }
}

Note: fee = max(baseFee, minGasPrice)*gasLimit. The gasLimit is declared as a constant in the code as const CreateValidatorTxGasLimit = 300000.

Also, the evmosd testnet init-files command has an optional parameter called--minimum-gas-prices. This parameter is used to set the --minimum-gas-prices in the app.toml file, which affects the local fee. It should be noted that the -min-gas-price option mentioned in this commit modifies the global fee for feemarket params.


Closes #XXX

@luchenqun luchenqun requested a review from a team as a code owner October 14, 2023 05:33
@luchenqun luchenqun requested review from Vvaradinov and facs95 and removed request for a team October 14, 2023 05:33
@luchenqun luchenqun changed the title add two new parameters --base-fee and --min-gas-price to the command evmosd testnet init-files feat(testnet): add two new parameters --base-fee and --min-gas-price to the command evmosd testnet init-files Oct 15, 2023
@fedekunze
Copy link
Contributor

@luchenqun can you fix the conflicts?

cmd/evmosd/testnet.go Fixed Show fixed Hide fixed
@@ -148,6 +154,18 @@
args.startingIPAddress, _ = cmd.Flags().GetString(flagStartingIPAddress)
args.numValidators, _ = cmd.Flags().GetInt(flagNumValidators)
args.algo, _ = cmd.Flags().GetString(flags.FlagKeyType)
baseFee, _ := cmd.Flags().GetString(flagBaseFee)

Check warning

Code scanning / gosec

Returned error is not propagated up the stack. Warning

Returned error is not propagated up the stack.
@luchenqun
Copy link
Contributor Author

@fedekunze The conflict has been resolved. I was using v14 code at the time, and it is now v15. In addition, I saw that lint did not pass, and it seems was not my relevant code.

Copy link
Contributor

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @luchenqun!!

Left a few comments

app/ante/evm/fee_checker.go Outdated Show resolved Hide resolved
cmd/evmosd/testnet.go Outdated Show resolved Hide resolved
cmd/evmosd/testnet.go Outdated Show resolved Hide resolved
cmd/evmosd/testnet.go Outdated Show resolved Hide resolved
cmd/evmosd/testnet.go Outdated Show resolved Hide resolved
cmd/evmosd/testnet.go Outdated Show resolved Hide resolved
cmd/evmosd/testnet.go Outdated Show resolved Hide resolved
@GAtom22
Copy link
Contributor

GAtom22 commented Oct 25, 2023

Actually, the default value for minimum_gas_prices flag is not zero

--minimum-gas-prices string    Minimum gas prices to accept for transactions; All fees in a tx must meet this minimum (e.g. 0.01photino,0.001stake) (default "0.000006aevmos")

However, for some reason, this is not stored properly in the app.toml file.

@luchenqun
Copy link
Contributor Author

Actually, the default value for minimum_gas_prices flag is not zero

--minimum-gas-prices string    Minimum gas prices to accept for transactions; All fees in a tx must meet this minimum (e.g. 0.01photino,0.001stake) (default "0.000006aevmos")

However, for some reason, this is not stored properly in the app.toml file.

You are right, it does not take effect. You can directly modify the app.toml file manually after production configuration and change --minimum-gas-prices to the value you need.

Copy link
Contributor

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the comments @luchenqun!!

Left a few more

cmd/evmosd/testnet.go Outdated Show resolved Hide resolved
cmd/evmosd/testnet.go Outdated Show resolved Hide resolved
cmd/evmosd/testnet.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
cmd/evmosd/testnet.go Outdated Show resolved Hide resolved
cmd/evmosd/testnet.go Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Oct 25, 2023

CLA assistant check
All committers have signed the CLA.

cmd/evmosd/testnet.go Fixed Show fixed Hide fixed
cmd/evmosd/testnet.go Outdated Show resolved Hide resolved
@GAtom22
Copy link
Contributor

GAtom22 commented Oct 26, 2023

Please address the lint issue

@@ -148,6 +160,22 @@
args.startingIPAddress, _ = cmd.Flags().GetString(flagStartingIPAddress)
args.numValidators, _ = cmd.Flags().GetInt(flagNumValidators)
args.algo, _ = cmd.Flags().GetString(flags.FlagKeyType)
baseFee, _ := cmd.Flags().GetString(flagBaseFee)
minGasPrice, _ := cmd.Flags().GetString(flagMinGasPrice)

Check warning

Code scanning / gosec

Returned error is not propagated up the stack. Warning

Returned error is not propagated up the stack.
@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #1864 (88da29b) into main (b826687) will increase coverage by 0.27%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 88da29b differs from pull request most recent head 81699c1. Consider uploading reports for the commit 81699c1 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1864      +/-   ##
==========================================
+ Coverage   69.70%   69.98%   +0.27%     
==========================================
  Files         331      327       -4     
  Lines       24514    24271     -243     
==========================================
- Hits        17088    16985     -103     
+ Misses       6543     6415     -128     
+ Partials      883      871      -12     
Files Coverage Δ
app/ante/evm/fee_checker.go 100.00% <100.00%> (ø)

... and 8 files with indirect coverage changes

@luchenqun
Copy link
Contributor Author

Actually, the default value for minimum_gas_prices flag is not zero

--minimum-gas-prices string    Minimum gas prices to accept for transactions; All fees in a tx must meet this minimum (e.g. 0.01photino,0.001stake) (default "0.000006aevmos")

However, for some reason, this is not stored properly in the app.toml file.

I have found the reason. It is because there is probably $HOME/.evmosd in your computer, and the minimum-gas-prices configured in app.toml has a higher priority than the fmt.Sprintf("0.000006%s" configured in the code. ", cmdcfg.BaseDenom)

@GAtom22 GAtom22 enabled auto-merge (squash) November 2, 2023 18:02
@GAtom22 GAtom22 merged commit d726ae0 into evmos:main Nov 2, 2023
23 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants