feat!: use harcoded global min gas price check in ProcessProposal #2985
Conversation
ninabarbakadze
requested review from
liamsi,
adlerjohn,
evan-forbes,
rootulp and
cmwaters
as code owners
WalkthroughThe changes primarily introduce a global minimum gas price affecting transaction fee calculations and prioritizations, replacing the previous node-specific minimum gas prices. This new consensus rule aims to standardize transaction costs and prevent undercutting in the blockspace market. Tests are updated to reflect these changes, and a new Multisig feature is documented. The project's Go version is also updated across various configurations and Dockerfiles. Changes
Related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| @@ -78,7 +78,7 @@ func (s *MaxTotalBlobSizeSuite) TestErrTotalBlobSizeTooLarge() { | |||
|
|
|||
| for _, tc := range testCases { | |||
| s.Run(tc.name, func() { | |||
| blobTx, err := signer.CreatePayForBlob([]*blob.Blob{tc.blob}, user.SetGasLimit(1e9)) | |||
| blobTx, err := signer.CreatePayForBlob([]*blob.Blob{tc.blob}, user.SetGasLimit(1e9), user.SetFee(2000000)) | |||
There was a problem hiding this comment.
nit: Since the minimum gas price is hardcoded, you could probably create a function that takes the gas limit and returns the minimum fee instead of having to calculate it yourself
There was a problem hiding this comment.
do you suggest I put that helper in a separate package or do you have a specific spot in mind?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
had one blocking comment, but otherwise LGTM!! great work and debugging 👍
its an optimization and therefore optional, and can be done in a different PR, but if we wanted to fail even faster we could account for the new min fee in the user package.
There was a problem hiding this comment.
Overall LGTM! Great work. I have a few nits about wording in the specs that are optional and one question about whether we need to preserve the v1 behavior to support single binary syncs.
For context: #1568
Update: disregard I don't think we need to preserve the v1 behavior because when a v2 binary is syncing from genesis and processing v1 blocks, it doesn't have access to the min gas price of various block proposers and their is no validity rule on what the min gas price should be for those blocks.
|
some of the nits will be addressed in a different pr when I continue working on the second part of this feature. #3016 |
ninabarbakadze
force-pushed
the
nina/min-gas-price
branch
from
72ccac4
to
912d5fb
Compare
ninabarbakadze
force-pushed
the
nina/min-gas-price
branch
2 times, most recently
from
b317f19
to
5843780
Compare
Co-authored-by: nina / ნინა <[email protected]>
ninabarbakadze
force-pushed
the
nina/min-gas-price
branch
from
988f986
to
cb0d51b
Compare
ninabarbakadze
force-pushed
the
nina/min-gas-price
branch
2 times, most recently
from
0996fd5
to
a07b827
Compare
| // GlobalMinGasPrice is used in the processProposal to ensure | ||
| // that all transactions have a gas price greater than or equal to this value. | ||
| GlobalMinGasPrice = DefaultMinGasPrice |
There was a problem hiding this comment.
[blocking][question] for @cmwaters. The breakdown of files in pkg/appconsts has me wondering where this constant should be defined. Candidates:
consensus_consts.gobecause this is a new "consensus constant"v2/app_consts.gobecause this is a new param introduced in app version v2
If we do keep this in initial_consts.go then I think we need to update the comment at the top of this file because this constant can't be modified via on-chain governance or the node's local config.
// The following defaults correspond to initial parameters of the network that can be changed, not via app versions
// but other means such as on-chain governance, or the nodes local config
There was a problem hiding this comment.
This should be viewed as a temporary home for now as we will want to either make it a governance parameter or have some other mechanism which picks this value. I don't think v2 will simply have this hardcoded value.
If we want to move it, we can shift it to the v2 directory. I'm easy either way
There was a problem hiding this comment.
Sounds good, I like v2/app_consts.go.
I think I'd rather it not be a governance modifiable param but I think we can discuss that as a follow up. Mostly b/c I don't think we should have any governance params long term and instead we should modify params via hard-forks after reaching rough consensus off-chain. So eventually we would move from governance params to versioned params.
There was a problem hiding this comment.
Since this PR already has two approvals, I don't think we need to block on just this constant move. We can move the constant in a follow up PR if that's easier
| @@ -39,7 +39,7 @@ func NewAnteHandler( | |||
| ante.NewConsumeGasForTxSizeDecorator(accountKeeper), | |||
| // Ensure the feepayer (fee granter or first signer) has enough funds to pay for the tx. | |||
| // Side effect: deducts fees from the fee payer. Sets the tx priority in context. | |||
| ante.NewDeductFeeDecorator(accountKeeper, bankKeeper, feegrantKeeper, checkTxFeeWithValidatorMinGasPrices), | |||
| ante.NewDeductFeeDecorator(accountKeeper, bankKeeper, feegrantKeeper, CheckTxFeeWithGlobalMinGasPrices), | |||
There was a problem hiding this comment.
[question] for @cmwaters @evan-forbes for single binary syncs and rolling upgrades, do we need to make the NewAnteHandler invocation version aware?
There was a problem hiding this comment.
I don't think we will ever be able to to remove an antehandler, so I don't think that we need to.
There was a problem hiding this comment.
I was thinking about having a generic version wrapper that wraps the decorator and only executes it for certain versions. This might be more cleaner than having version checks sporadically added to every decorator
app/ante/fee_checker.go
Outdated
| // convert the global minimum gas price to a big.Int | ||
| globalMinGasPrice, err := sdk.NewDecFromStr(fmt.Sprintf("%f", appconsts.GlobalMinGasPrice)) | ||
| if err != nil { | ||
| return nil, 0, errors.Wrap(err, "invalid GlobalMinGasPrice") | ||
| } | ||
|
|
||
| gasInt := sdk.NewIntFromUint64(gas) | ||
| minFee := globalMinGasPrice.MulInt(gasInt).TruncateInt() | ||
|
|
||
| // Determine the required fees by multiplying each required minimum gas | ||
| // price by the gas limit, where fee = ceil(minGasPrice * gasLimit). | ||
| glDec := sdk.NewDec(int64(gas)) | ||
| for i, gp := range minGasPrices { | ||
| fee := gp.Amount.Mul(glDec) | ||
| requiredFees[i] = sdk.NewCoin(gp.Denom, fee.Ceil().RoundInt()) | ||
| } | ||
| // if min fee truncates to zero, set it to one | ||
| if minFee.LT(sdk.NewInt(appconsts.MinFee)) { | ||
| minFee = sdk.NewInt(appconsts.MinFee) | ||
| } | ||
|
|
||
| if !feeCoins.IsAnyGTE(requiredFees) { | ||
| return nil, 0, errors.Wrapf(sdkerror.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, requiredFees) | ||
| } | ||
| if !fee.GTE(minFee) { |
There was a problem hiding this comment.
[no change needed] we may want to compare / contrast this implementation with https://github.com/skip-mev/feemarket/blob/e6f642fbeac30f78ec7061e2f7b23a6371bf0e85/x/feemarket/ante/fee.go#L76-L103
There was a problem hiding this comment.
I think the main thing we should be wary of is this implementation assumes only a single token whereas skip's implementation allows for multiple native tokens. At the moment we only have one token and I don't see that changing in the foreseeable future but maybe we want to avoid the footgun in the future
Co-authored-by: Rootul P <[email protected]>
Overview
Fixes - #1621
Introducing a global minimum gas price, a consensus change enforcing a lower bound for all transactions.
sidenote - updated the fee markets section in the docs but since I haven't yet been able to thoroughly read the specs section please do point out if I missed something.
Checklist