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!: use harcoded global min gas price check in ProcessProposal #2985

Merged
merged 30 commits into from
Jan 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
10352cd
Use harcoded global min gas price check in ProcessProposal
cmwaters Jan 5, 2024
f382ad6
test: trying to build tx
ninabarbakadze Jan 5, 2024
617e7ff
refactor: fix rounding error and make the function public
ninabarbakadze Jan 8, 2024
2f89d0c
test: add unit test for the new global minimum fee logic
ninabarbakadze Jan 8, 2024
4136c46
style: lint
ninabarbakadze Jan 8, 2024
d20fe1a
test: revert changes in unrelated test file
ninabarbakadze Jan 8, 2024
488bb7f
test: fix insufficient tx fee in accounts tx in genesis
ninabarbakadze Jan 9, 2024
fae5c79
feat: add version control
ninabarbakadze Jan 11, 2024
070c51e
test: increase the fee in remaining test
ninabarbakadze Jan 11, 2024
189f19b
style: gofumpt-ed
ninabarbakadze Jan 11, 2024
6640341
merge
ninabarbakadze Jan 17, 2024
4f804cd
test: refactor
ninabarbakadze Jan 11, 2024
6c62e68
docs: update the fee market docs
ninabarbakadze Jan 11, 2024
9156b77
docs: nit
ninabarbakadze Jan 11, 2024
d805575
docs: remove last few words
ninabarbakadze Jan 11, 2024
ad1912f
test: make test case more explicit
ninabarbakadze Jan 12, 2024
37ca150
docs: make it more explicit
ninabarbakadze Jan 12, 2024
4302fa4
chore: cleanup
ninabarbakadze Jan 17, 2024
34c9628
merge
ninabarbakadze Jan 17, 2024
cb0d51b
Merge remote-tracking branch 'celestia-app/main' into nina/min-gas-price
ninabarbakadze Jan 17, 2024
9d5739c
go mod tidy
ninabarbakadze Jan 17, 2024
a07b827
fix: docs
ninabarbakadze Jan 17, 2024
f65ce02
style: lint
ninabarbakadze Jan 17, 2024
c476541
chore: revert go.work.sum
ninabarbakadze Jan 17, 2024
a480db0
docs: cleanup
ninabarbakadze Jan 17, 2024
97616e0
Update pkg/appconsts/initial_consts.go
ninabarbakadze Jan 17, 2024
91f1b42
refactor: review suggestions
ninabarbakadze Jan 17, 2024
9f6a379
test: remove unnecessary test
ninabarbakadze Jan 17, 2024
1e2bd44
Merge branch 'main' into nina/min-gas-price
ninabarbakadze Jan 18, 2024
9aeccb9
Merge branch 'main' into nina/min-gas-price
ninabarbakadze Jan 19, 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
2 changes: 1 addition & 1 deletion app/ante/ante.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] for @cmwaters @evan-forbes for single binary syncs and rolling upgrades, do we need to make the NewAnteHandler invocation version aware?

Copy link
Member

@evan-forbes evan-forbes Jan 17, 2024

Choose a reason for hiding this comment

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

I don't think we will ever be able to to remove an antehandler, so I don't think that we need to.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

// Set public keys in the context for fee-payer and all signers.
// Contract: must be called before all signature verification decorators.
ante.NewSetPubKeyDecorator(accountKeeper),
Expand Down
44 changes: 21 additions & 23 deletions app/ante/fee_checker.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package ante

import (
"fmt"

errors "cosmossdk.io/errors"
"github.com/celestiaorg/celestia-app/pkg/appconsts"
v1 "github.com/celestiaorg/celestia-app/pkg/appconsts/v1"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerror "github.com/cosmos/cosmos-sdk/types/errors"
)
Expand All @@ -11,41 +15,35 @@ const (
priorityScalingFactor = 1_000_000
)

// checkTxFeeWithValidatorMinGasPrices implements the default fee logic, where the minimum price per
// unit of gas is fixed and set by each validator, and the tx priority is computed from the gas price.
func checkTxFeeWithValidatorMinGasPrices(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, int64, error) {
// CheckTxFeeWithGlobalMinGasPrices implements the default fee logic, where the minimum price per
// unit of gas is fixed and set globally, and the tx priority is computed from the gas price.
func CheckTxFeeWithGlobalMinGasPrices(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, int64, error) {
feeTx, ok := tx.(sdk.FeeTx)
if !ok {
return nil, 0, errors.Wrap(sdkerror.ErrTxDecode, "Tx must be a FeeTx")
}

feeCoins := feeTx.GetFee()
fee := feeTx.GetFee().AmountOf(appconsts.BondDenom)
gas := feeTx.GetGas()

// 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.
if ctx.IsCheckTx() {
minGasPrices := ctx.MinGasPrices()
if !minGasPrices.IsZero() {
requiredFees := make(sdk.Coins, len(minGasPrices))
// global minimum fee only applies to app versions greater than one
if ctx.BlockHeader().Version.App > v1.Version {
// convert the global minimum gas price to a big.Int
globalMinGasPrice, err := sdk.NewDecFromStr(fmt.Sprintf("%f", appconsts.GlobalMinGasPrice))
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, 0, errors.Wrap(err, "invalid GlobalMinGasPrice")
}

// 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())
}
gasInt := sdk.NewIntFromUint64(gas)
minFee := globalMinGasPrice.MulInt(gasInt).RoundInt()

if !feeCoins.IsAnyGTE(requiredFees) {
return nil, 0, errors.Wrapf(sdkerror.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, requiredFees)
}
if !fee.GTE(minFee) {
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
return nil, 0, errors.Wrapf(sdkerror.ErrInsufficientFee, "insufficient fees; got: %s required: %s", fee, minFee)
}
}

priority := getTxPriority(feeCoins, int64(gas))
return feeCoins, priority, nil
priority := getTxPriority(feeTx.GetFee(), int64(gas))
return feeTx.GetFee(), priority, nil
}

// getTxPriority returns a naive tx priority based on the amount of the smallest denomination of the gas price
Expand Down
File renamed without changes.
111 changes: 111 additions & 0 deletions app/ante/min_fee_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package ante_test
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved

import (
"math"
"testing"

"github.com/celestiaorg/celestia-app/app"
"github.com/celestiaorg/celestia-app/app/ante"
"github.com/celestiaorg/celestia-app/app/encoding"
"github.com/celestiaorg/celestia-app/pkg/appconsts"
"github.com/celestiaorg/celestia-app/test/util/testnode"
sdk "github.com/cosmos/cosmos-sdk/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
"github.com/stretchr/testify/require"
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
version "github.com/tendermint/tendermint/proto/tendermint/version"
)

func TestCheckTxFeeWithGlobalMinGasPrices(t *testing.T) {
encCfg := encoding.MakeConfig(app.ModuleEncodingRegisters...)

builder := encCfg.TxConfig.NewTxBuilder()
err := builder.SetMsgs(banktypes.NewMsgSend(
testnode.RandomAddress().(sdk.AccAddress),
testnode.RandomAddress().(sdk.AccAddress),
sdk.NewCoins(sdk.NewInt64Coin(appconsts.BondDenom, 10))),
)
require.NoError(t, err)

feeAmount := int64(1000)

ctx := sdk.Context{}

testCases := []struct {
name string
fee sdk.Coins
gasLimit uint64
appVersion uint64
expErr bool
}{
{
name: "bad tx; fee below required minimum",
fee: sdk.NewCoins(sdk.NewInt64Coin(appconsts.BondDenom, feeAmount-1)),
gasLimit: uint64(float64(feeAmount) / appconsts.GlobalMinGasPrice),
appVersion: uint64(2),
expErr: true,
},
{
name: "good tx; fee equal to required minimum",
fee: sdk.NewCoins(sdk.NewInt64Coin(appconsts.BondDenom, feeAmount)),
gasLimit: uint64(float64(feeAmount) / appconsts.GlobalMinGasPrice),
appVersion: uint64(2),
expErr: false,
},
{
name: "good tx; fee above required minimum",
fee: sdk.NewCoins(sdk.NewInt64Coin(appconsts.BondDenom, feeAmount+1)),
gasLimit: uint64(float64(feeAmount) / appconsts.GlobalMinGasPrice),
appVersion: uint64(2),
expErr: false,
},
{
name: "good tx; with no fee (v1)",
fee: sdk.NewCoins(sdk.NewInt64Coin(appconsts.BondDenom, feeAmount)),
gasLimit: uint64(float64(feeAmount) / appconsts.GlobalMinGasPrice),
appVersion: uint64(1),
expErr: false,
},
{
name: "good tx; gas limit and fee are maximum values",
fee: sdk.NewCoins(sdk.NewInt64Coin(appconsts.BondDenom, math.MaxInt64)),
gasLimit: math.MaxUint64,
appVersion: uint64(2),
expErr: false,
},
{
name: "bad tx; gas limit and fee are 0",
fee: sdk.NewCoins(sdk.NewInt64Coin(appconsts.BondDenom, 0)),
gasLimit: 0,
appVersion: uint64(2),
expErr: false,
},
{
name: "good tx; minFee = 0.8, rounds up to 1",
fee: sdk.NewCoins(sdk.NewInt64Coin(appconsts.BondDenom, feeAmount)),
gasLimit: 400,
appVersion: uint64(2),
expErr: false,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
builder.SetGasLimit(tc.gasLimit)
builder.SetFeeAmount(tc.fee)
tx := builder.GetTx()

ctx = ctx.WithBlockHeader(tmproto.Header{
Version: version.Consensus{
App: tc.appVersion,
},
})
_, _, err := ante.CheckTxFeeWithGlobalMinGasPrices(ctx, tx)
if tc.expErr {
require.Error(t, err)
} else {
require.NoError(t, err)
}
})
}
}
2 changes: 1 addition & 1 deletion app/test/max_total_blob_size_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

do you suggest I put that helper in a separate package or do you have a specific spot in mind?

ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(t, err)
subCtx, cancel := context.WithTimeout(s.cctx.GoContext(), 30*time.Second)
defer cancel()
Expand Down
2 changes: 1 addition & 1 deletion app/test/prepare_proposal_context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func TestTimeInPrepareProposalContext(t *testing.T) {
addr := testfactory.GetAddress(cctx.Keyring, account)
signer, err := user.SetupSigner(cctx.GoContext(), cctx.Keyring, cctx.GRPCClient, addr, ecfg)
require.NoError(t, err)
res, err := signer.SubmitTx(cctx.GoContext(), msgs, user.SetGasLimit(1000000), user.SetFee(1))
res, err := signer.SubmitTx(cctx.GoContext(), msgs, user.SetGasLimit(1000000), user.SetFee(2000))
if tt.expectedCode != abci.CodeTypeOK {
require.Error(t, err)
} else {
Expand Down
Loading
Loading