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(server/v2): add min gas price and check with tx fee #21173

Merged
merged 38 commits into from
Aug 29, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
39d9a69
feat(server/v2): add min gas price and check with tx fee
akhilkumarpilli Aug 5, 2024
a00a866
address comment
akhilkumarpilli Aug 6, 2024
1fe5674
Merge branch 'main' into akhil/min-gas-price-config
akhilkumarpilli Aug 6, 2024
36e4aed
address comment
akhilkumarpilli Aug 6, 2024
bc8ad35
WIP: add min-gas-prices in main server component
akhilkumarpilli Aug 7, 2024
d943ad8
clean and integrate min gas prices in auth
akhilkumarpilli Aug 8, 2024
68f5d6f
Merge branch 'main' into akhil/min-gas-price-config
akhilkumarpilli Aug 8, 2024
b1593b5
revert typo
akhilkumarpilli Aug 8, 2024
650bfef
fix lint
akhilkumarpilli Aug 8, 2024
a1d39b7
Merge branch 'main' into akhil/min-gas-price-config
akhilkumarpilli Aug 8, 2024
f5c1272
WIP: update DeductFeeDecorator
akhilkumarpilli Aug 9, 2024
e68de22
Merge branch 'main' into akhil/min-gas-price-config
akhilkumarpilli Aug 9, 2024
7cad0b8
address comments, fix tests
akhilkumarpilli Aug 12, 2024
a6e7be5
Merge branch 'main' into akhil/min-gas-price-config
akhilkumarpilli Aug 12, 2024
25528b0
move min-gas-price check to feegrant TxValidator
akhilkumarpilli Aug 12, 2024
2bdda62
revert change and comment
akhilkumarpilli Aug 12, 2024
84fed9e
WIP: return feeTxValidator from auth/tx and revert feegrant changes
akhilkumarpilli Aug 12, 2024
0499d82
Merge branch 'main' into akhil/min-gas-price-config
akhilkumarpilli Aug 16, 2024
dc59cae
update approach
akhilkumarpilli Aug 20, 2024
ca0a801
fix lint and small changes
akhilkumarpilli Aug 21, 2024
aad3d1d
Merge branch 'main' into akhil/min-gas-price-config
akhilkumarpilli Aug 21, 2024
3157565
add todo
akhilkumarpilli Aug 21, 2024
3680c95
Merge branch 'main' into akhil/min-gas-price-config
akhilkumarpilli Aug 22, 2024
44a7c0f
fix lint
akhilkumarpilli Aug 22, 2024
76cbd83
address comments
akhilkumarpilli Aug 23, 2024
166f17e
Merge branch 'main' into akhil/min-gas-price-config
akhilkumarpilli Aug 26, 2024
98d5914
update approach
akhilkumarpilli Aug 26, 2024
93bebd4
emit v2 event and pass feegrant keeper
akhilkumarpilli Aug 27, 2024
50e95fe
address comments
akhilkumarpilli Aug 27, 2024
e8365f5
address comments
akhilkumarpilli Aug 27, 2024
331731c
Merge branch 'main' into akhil/min-gas-price-config
julienrbrt Aug 28, 2024
5071eac
remove state struct
julienrbrt Aug 28, 2024
ba0d199
updates
julienrbrt Aug 28, 2024
8d9dc75
correct testnet flag
julienrbrt Aug 28, 2024
8e966e6
consistent config placement
julienrbrt Aug 28, 2024
2c3b9d8
fix
julienrbrt Aug 28, 2024
46cef74
ok
julienrbrt Aug 28, 2024
0c5f774
done
julienrbrt Aug 28, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions server/v2/cometbft/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"cosmossdk.io/core/transaction"
errorsmod "cosmossdk.io/errors"
"cosmossdk.io/log"
sdkmath "cosmossdk.io/math"
"cosmossdk.io/server/v2/appmanager"
"cosmossdk.io/server/v2/cometbft/client/grpc/cmtservice"
"cosmossdk.io/server/v2/cometbft/handlers"
Expand All @@ -27,6 +28,7 @@ import (
"cosmossdk.io/server/v2/streaming"
"cosmossdk.io/store/v2/snapshots"
consensustypes "cosmossdk.io/x/consensus/types"
sdk "github.com/cosmos/cosmos-sdk/types"
)

var _ abci.Application = (*Consensus[transaction.Tx])(nil)
Expand Down Expand Up @@ -123,6 +125,14 @@ func (c *Consensus[T]) CheckTx(ctx context.Context, req *abciproto.CheckTxReques
return nil, err
}

// check tx fee with validator's minimum-gas-price config
if err := c.checkTxFeeWithMinGasPrices(decodedTx); err != nil {
return &abciproto.CheckTxResponse{
Code: 1,
Log: err.Error(),
}, nil
}

resp, err := c.app.ValidateTx(ctx, decodedTx)
if err != nil {
return nil, err
Expand All @@ -145,6 +155,40 @@ func (c *Consensus[T]) CheckTx(ctx context.Context, req *abciproto.CheckTxReques
return cometResp, nil
}

// checkTxFeeWithMinGasPrices ensure that the provided fees meet a minimum threshold for the validator,
// if this is a CheckTx. This is only for local mempool purposes, and thus
// is only ran on check tx.
func (c *Consensus[T]) checkTxFeeWithMinGasPrices(tx transaction.Tx) error {
akhilkumarpilli marked this conversation as resolved.
Show resolved Hide resolved
feeTx, ok := tx.(sdk.FeeTx)
if !ok {
return nil // don't force users to implement fee tx
}

feeCoins := feeTx.GetFee()
gas := feeTx.GetGas()

minGasPrices := c.cfg.GetMinGasPrices()
if minGasPrices.IsZero() {
return nil
}

requiredFees := make(sdk.Coins, len(minGasPrices))

// Determine the required fees by multiplying each required minimum gas
// price by the gas limit, where fee = ceil(minGasPrice * gasLimit).
glDec := sdkmath.LegacyNewDec(int64(gas))
for i, gp := range minGasPrices {
fee := gp.Amount.Mul(glDec)
requiredFees[i] = sdk.NewCoin(gp.Denom, fee.Ceil().RoundInt())
}

if !feeCoins.IsAnyGTE(requiredFees) {
return errorsmod.Wrapf(cometerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, requiredFees)
}

return nil
}

// Info implements types.Application.
func (c *Consensus[T]) Info(ctx context.Context, _ *abciproto.InfoRequest) (*abciproto.InfoResponse, error) {
version, _, err := c.store.StateLatest()
Expand Down
29 changes: 29 additions & 0 deletions server/v2/cometbft/config.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
package cometbft

import (
"fmt"

cmtcfg "github.com/cometbft/cometbft/config"
"github.com/spf13/viper"

serverv2 "cosmossdk.io/server/v2"
cometerrors "cosmossdk.io/server/v2/cometbft/types/errors"
sdk "github.com/cosmos/cosmos-sdk/types"
)

// Config is the configuration for the CometBFT application
Expand All @@ -27,6 +31,7 @@ func DefaultAppTomlConfig() *AppTomlConfig {
}

type AppTomlConfig struct {
MinGasPrices string `mapstructure:"minimum-gas-prices" toml:"minimum-gas-prices" comment:"minimum-gas-prices defines the price which a validator is willing to accept for processing a transaction. A transaction's fees must meet the minimum of any denomination specified in this config (e.g. 0.25token1;0.0001token2)."`
MinRetainBlocks uint64 `mapstructure:"min-retain-blocks" toml:"min-retain-blocks" comment:"min-retain-blocks defines the minimum block height offset from the current block being committed, such that all blocks past this offset are pruned from CometBFT. A value of 0 indicates that no blocks should be pruned."`
IndexEvents []string `mapstructure:"index-events" toml:"index-events" comment:"index-events defines the set of events in the form {eventType}.{attributeKey}, which informs CometBFT what to index. If empty, all events will be indexed."`
HaltHeight uint64 `mapstructure:"halt-height" toml:"halt-height" comment:"halt-height contains a non-zero block height at which a node will gracefully halt and shutdown that can be used to assist upgrades and testing."`
Expand Down Expand Up @@ -64,3 +69,27 @@ func getConfigTomlFromViper(v *viper.Viper) *cmtcfg.Config {

return conf.SetRoot(rootDir)
}

// GetMinGasPrices returns the validator's minimum gas prices based on the set configuration.
func (c Config) GetMinGasPrices() sdk.DecCoins {
minGasPricesStr := c.AppTomlConfig.MinGasPrices
if minGasPricesStr == "" {
return sdk.DecCoins{}
}

gasPrices, err := sdk.ParseDecCoins(minGasPricesStr)
if err != nil {
panic(fmt.Sprintf("invalid minimum gas prices: %v", err))
}

return gasPrices
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure proper error handling in GetMinGasPrices.

The GetMinGasPrices method is well-implemented, but consider handling errors more gracefully rather than panicking.

-  panic(fmt.Sprintf("invalid minimum gas prices: %v", err))
+  return sdk.DecCoins{}, fmt.Errorf("invalid minimum gas prices: %v", err)

Committable suggestion was skipped due to low confidence.


// ValidateBasic returns an error if min-gas-prices field is empty in Config. Otherwise, it returns nil.
func (c Config) ValidateBasic() error {
if c.AppTomlConfig.MinGasPrices == "" {
return cometerrors.ErrAppConfig.Wrap("set minimum-gas-prices in app.toml or flag or env variable")
}

return nil
}
13 changes: 7 additions & 6 deletions server/v2/cometbft/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,11 @@ func prefix(f string) string {

// Server flags
var (
Standalone = prefix("standalone")
FlagAddress = prefix("address")
FlagTransport = prefix("transport")
FlagHaltHeight = prefix("halt-height")
FlagHaltTime = prefix("halt-time")
FlagTrace = prefix("trace")
Standalone = prefix("standalone")
FlagAddress = prefix("address")
FlagTransport = prefix("transport")
FlagHaltHeight = prefix("halt-height")
FlagHaltTime = prefix("halt-time")
FlagTrace = prefix("trace")
FlagMinGasPrices = prefix("minimum-gas-prices")
)
2 changes: 1 addition & 1 deletion server/v2/cometbft/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ require (
cosmossdk.io/core v0.12.1-0.20240725072823-6a2d039e1212
cosmossdk.io/errors v1.0.1
cosmossdk.io/log v1.3.1
cosmossdk.io/math v1.3.0
cosmossdk.io/server/v2 v2.0.0-00010101000000-000000000000
cosmossdk.io/server/v2/appmanager v0.0.0-00010101000000-000000000000
cosmossdk.io/store/v2 v2.0.0-00010101000000-000000000000
Expand All @@ -50,7 +51,6 @@ require (
cosmossdk.io/collections v0.4.0 // indirect
cosmossdk.io/depinject v1.0.0 // indirect
cosmossdk.io/errors/v2 v2.0.0-20240731132947-df72853b3ca5 // indirect
cosmossdk.io/math v1.3.0 // indirect
cosmossdk.io/store v1.1.1-0.20240418092142-896cdf1971bc // indirect
cosmossdk.io/x/auth v0.0.0-00010101000000-000000000000 // indirect
cosmossdk.io/x/bank v0.0.0-20240226161501-23359a0b6d91 // indirect
Expand Down
6 changes: 6 additions & 0 deletions server/v2/cometbft/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,11 @@ func (s *CometBFTServer[T]) Name() string {
}

func (s *CometBFTServer[T]) Start(ctx context.Context) error {
// validate cometbft config
if err := s.config.ValidateBasic(); err != nil {
return err
}

wrappedLogger := cometlog.CometLoggerWrapper{Logger: s.logger}
if s.config.AppTomlConfig.Standalone {
svr, err := abciserver.NewServer(s.config.AppTomlConfig.Address, s.config.AppTomlConfig.Transport, s.Consensus)
Expand Down Expand Up @@ -218,6 +223,7 @@ func (s *CometBFTServer[T]) StartCmdFlags() *pflag.FlagSet {
flags.String(FlagTransport, "socket", "Transport protocol: socket, grpc")
flags.Uint64(FlagHaltHeight, 0, "Block height at which to gracefully halt the chain and shutdown the node")
flags.Uint64(FlagHaltTime, 0, "Minimum block time (in Unix seconds) at which to gracefully halt the chain and shutdown the node")
flags.String(FlagMinGasPrices, "", "Minimum gas prices to accept for transactions; Any fee in a tx must meet this minimum (e.g. 0.01photino;0.0001stake)")
flags.Bool(FlagTrace, false, "Provide full stack traces for errors in ABCI Log")
flags.Bool(Standalone, false, "Run app without CometBFT")

Expand Down
9 changes: 9 additions & 0 deletions server/v2/cometbft/types/errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,13 @@ var (
// ErrInvalidRequest defines an ABCI typed error where the request contains
// invalid data.
ErrInvalidRequest = errorsmod.Register(RootCodespace, 2, "invalid request")

// ErrTxDecode is returned if we cannot parse a transaction
ErrTxDecode = errorsmod.Register(RootCodespace, 3, "tx parse error")

// ErrInsufficientFee is returned if provided fee in tx is less than required fee
ErrInsufficientFee = errorsmod.Register(RootCodespace, 4, "insufficient fee")

// ErrAppConfig defines an error occurred if application configuration is misconfigured
ErrAppConfig = errorsmod.Register(RootCodespace, 5, "error in app.toml")
)
23 changes: 22 additions & 1 deletion simapp/v2/simdv2/cmd/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,33 @@ func initRootCmd[T transaction.Tx](
offchain.OffChain(),
)

// Optionally allow the chain developer to overwrite the cometbft default
// app toml config.
cometAppTomlCfg := cometbft.DefaultAppTomlConfig()
// The cometbft server's default minimum gas price is set to "" (empty value) inside
// app.toml. If left empty by validators, the node will halt on startup.
// However, the chain developer can set a default app.toml value for their
// validators here.
//
// In summary:
// - if you leave cometAppTomlCfg.MinGasPrices = "", all validators MUST tweak their
// own app.toml config,
// - if you set cometAppTomlCfg.MinGasPrices non-empty, validators CAN tweak their
// own app.toml to override, or use this default value.
//
// In simapp, we set the min gas prices to 0.
cometAppTomlCfg.MinGasPrices = "0stake"

// wire server commands
if err = serverv2.AddCommands(
rootCmd,
newApp,
logger,
cometbft.New(&genericTxDecoder[T]{txConfig}, cometbft.DefaultServerOptions[T]()),
cometbft.New(
&genericTxDecoder[T]{txConfig},
cometbft.DefaultServerOptions[T](),
cometbft.OverwriteDefaultAppTomlConfig(cometAppTomlCfg),
),
grpc.New[T](),
store.New[T](),
); err != nil {
Expand Down
4 changes: 4 additions & 0 deletions simapp/v2/simdv2/cmd/testnet.go
Original file line number Diff line number Diff line change
Expand Up @@ -337,11 +337,15 @@ func initTestnetFiles[T transaction.Tx](
return err
}

cometAppTomlCfg := cometbft.DefaultAppTomlConfig()
cometAppTomlCfg.MinGasPrices = args.minGasPrices

// Write server config
cometServer := cometbft.New[T](
&genericTxDecoder[T]{clientCtx.TxConfig},
cometbft.ServerOptions[T]{},
cometbft.OverwriteDefaultConfigTomlConfig(nodeConfig),
cometbft.OverwriteDefaultAppTomlConfig(cometAppTomlCfg),
)
grpcServer := grpc.New[T](grpc.OverwriteDefaultConfig(grpcConfig))
storeServer := store.New[T]()
Expand Down
2 changes: 2 additions & 0 deletions tools/confix/data/v2-app.toml
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
[comet]
# minimum-gas-prices defines the price which a validator is willing to accept for processing a transaction. A transaction's fees must meet the minimum of any denomination specified in this config (e.g. 0.25token1;0.0001token2).
minimum-gas-prices = '0stake'
# min-retain-blocks defines the minimum block height offset from the current block being committed, such that all blocks past this offset are pruned from CometBFT. A value of 0 indicates that no blocks should be pruned.
min-retain-blocks = 0
# index-events defines the set of events in the form {eventType}.{attributeKey}, which informs CometBFT what to index. If empty, all events will be indexed.
Expand Down
11 changes: 6 additions & 5 deletions tools/confix/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,12 @@ type v2KeyChangesMap map[string][]string

// list all the keys which are need to be modified in v2
var v2KeyChanges = v2KeyChangesMap{
"min-retain-blocks": []string{"comet.min-retain-blocks"},
"index-events": []string{"comet.index-events"},
"halt-height": []string{"comet.halt-height"},
"halt-time": []string{"comet.halt-time"},
"app-db-backend": []string{"store.app-db-backend"},
"minimum-gas-prices": []string{"comet.minimum-gas-prices"},
"min-retain-blocks": []string{"comet.min-retain-blocks"},
"index-events": []string{"comet.index-events"},
"halt-height": []string{"comet.halt-height"},
"halt-time": []string{"comet.halt-time"},
"app-db-backend": []string{"store.app-db-backend"},
"pruning-keep-recent": []string{
"store.options.ss-pruning-option.keep-recent",
"store.options.sc-pruning-option.keep-recent",
Expand Down
Loading