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 5 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")
)
24 changes: 24 additions & 0 deletions server/v2/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,30 @@ import (
"github.com/spf13/viper"
)

// ServerConfig defines configuration for the main server.
type ServerConfig 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)."`
}

// ValidateBasic returns an error if min-gas-prices field is empty in Config. Otherwise, it returns nil.
func (c ServerConfig) ValidateBasic() error {
Copy link
Member

Choose a reason for hiding this comment

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

Let's maybe delete this to stay consistent. IMHO we should just have good defaults, so we don't this this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean we can allow config even with minimum-gas-prices as empty?

Copy link
Member

Choose a reason for hiding this comment

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

Nah the default config would just give 0stake as of today.

if c.MinGasPrices == "" {
return fmt.Errorf("error in app.toml: set minimum-gas-prices in app.toml or flag or env variable")
}

return nil
}

// DefaultMainServerConfig returns the default config of main server component
func DefaultMainServerConfig() ServerConfig {
Copy link
Member

Choose a reason for hiding this comment

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

Can we drop the Main

return ServerConfig{}
Copy link
Member

Choose a reason for hiding this comment

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

Can the default config be 0stake?

Copy link
Contributor Author

@akhilkumarpilli akhilkumarpilli Aug 12, 2024

Choose a reason for hiding this comment

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

We will be setting 0stake in simapp/v2 cmd. I think we cannot set it here as denom might be different for different networks. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

That's true but they can overwrite it then right? This just avoids having an empty value by default

}

// OverwriteDefaultConfig overwrites main server config with given config
func (s *Server[T]) OverwriteDefaultConfig(cfg ServerConfig) {
s.config = cfg
}

// ReadConfig returns a viper instance of the config file
func ReadConfig(configPath string) (*viper.Viper, error) {
v := viper.New()
Expand Down
11 changes: 11 additions & 0 deletions server/v2/flags.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,17 @@
// Package serverv2 defines constants for server configuration flags and output formats.
package serverv2

import "fmt"

// start flags are prefixed with the server name
// as the config in prefixed with the server name
// this allows viper to properly bind the flags
func prefix(f string) string {
return fmt.Sprintf("%s.%s", serverName, f)
}

var FlagMinGasPrices = prefix("minimum-gas-prices")

const (
// FlagHome specifies the home directory flag.
FlagHome = "home"
Expand Down
41 changes: 40 additions & 1 deletion server/v2/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,16 @@ type CLIConfig struct {
Txs []*cobra.Command
}

const (
serverName = "server"
)

var _ ServerComponent[transaction.Tx] = (*Server[transaction.Tx])(nil)

type Server[T transaction.Tx] struct {
logger log.Logger
components []ServerComponent[T]
config ServerConfig
}

func NewServer[T transaction.Tx](
Expand All @@ -74,11 +79,16 @@ func NewServer[T transaction.Tx](
}

func (s *Server[T]) Name() string {
return "server"
return serverName
}

// Start starts all components concurrently.
func (s *Server[T]) Start(ctx context.Context) error {
// validate main server config
if err := s.config.ValidateBasic(); err != nil {
return err
}

s.logger.Info("starting servers...")

g, ctx := errgroup.WithContext(ctx)
Expand Down Expand Up @@ -151,9 +161,19 @@ func (s *Server[T]) CLICommands() CLIConfig {
return commands
}

// Config returns config of the main server component
func (s *Server[T]) Config() ServerConfig {
return s.config
}

// Configs returns all configs of all server components.
func (s *Server[T]) Configs() map[string]any {
cfgs := make(map[string]any)

// add main server component config
cfgs[s.Name()] = s.config

// add other components' config
for _, mod := range s.components {
if configmod, ok := mod.(HasConfig); ok {
cfg := configmod.Config()
Expand All @@ -164,9 +184,22 @@ func (s *Server[T]) Configs() map[string]any {
return cfgs
}

func (s *Server[T]) StartCmdFlags() *pflag.FlagSet {
flags := pflag.NewFlagSet(s.Name(), pflag.ExitOnError)
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)")
return flags
}

// Init initializes all server components with the provided application, configuration, and logger.
// It returns an error if any component fails to initialize.
func (s *Server[T]) Init(appI AppI[T], v *viper.Viper, logger log.Logger) error {
cfg := s.config
if v != nil {
if err := UnmarshalSubConfig(v, s.Name(), &cfg); err != nil {
return fmt.Errorf("failed to unmarshal config: %w", err)
}
}

var components []ServerComponent[T]
for _, mod := range s.components {
mod := mod
Expand All @@ -177,6 +210,7 @@ func (s *Server[T]) Init(appI AppI[T], v *viper.Viper, logger log.Logger) error
components = append(components, mod)
}

s.config = cfg
s.components = components
return nil
}
Expand Down Expand Up @@ -217,6 +251,11 @@ func (s *Server[T]) WriteConfig(configPath string) error {
// StartFlags returns all flags of all server components.
func (s *Server[T]) StartFlags() []*pflag.FlagSet {
flags := []*pflag.FlagSet{}

// add main server component flags
flags = append(flags, s.StartCmdFlags())

// add other components' start cmd flags
for _, mod := range s.components {
if startmod, ok := mod.(HasStartFlags); ok {
flags = append(flags, startmod.StartCmdFlags())
Expand Down
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,6 +337,9 @@ func initTestnetFiles[T transaction.Tx](
return err
}

serverCfg := serverv2.DefaultMainServerConfig()
serverCfg.MinGasPrices = args.minGasPrices

// Write server config
cometServer := cometbft.New[T](
&genericTxDecoder[T]{clientCtx.TxConfig},
Expand All @@ -346,6 +349,7 @@ func initTestnetFiles[T transaction.Tx](
grpcServer := grpc.New[T](grpc.OverwriteDefaultConfig(grpcConfig))
storeServer := store.New[T]()
server := serverv2.NewServer(log.NewNopLogger(), cometServer, grpcServer, storeServer)
server.OverwriteDefaultConfig(serverCfg)
err = server.WriteConfig(filepath.Join(nodeDir, "config"))
if err != nil {
return err
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
Loading
Loading