From afbde804223436fcd898faebbdb035a33b646f57 Mon Sep 17 00:00:00 2001 From: beer-1 Date: Fri, 1 Nov 2024 12:03:47 +0900 Subject: [PATCH 1/3] make gas refunds error free and panic safe --- app/app.go | 2 +- app/posthandler/gasrefund.go | 47 +++++++++++++++++++++++++--------- app/posthandler/posthandler.go | 6 ++--- x/evm/keeper/txutils.go | 4 +-- 4 files changed, 41 insertions(+), 18 deletions(-) diff --git a/app/app.go b/app/app.go index be76d43..ab89164 100644 --- a/app/app.go +++ b/app/app.go @@ -367,7 +367,7 @@ func (app *MinitiaApp) SetCheckTx(handler blockchecktx.CheckTx) { func (app *MinitiaApp) setPostHandler() { app.SetPostHandler(posthandler.NewPostHandler( - app.AccountKeeper, + app.Logger(), app.EVMKeeper, )) } diff --git a/app/posthandler/gasrefund.go b/app/posthandler/gasrefund.go index d056338..8fc8ecf 100644 --- a/app/posthandler/gasrefund.go +++ b/app/posthandler/gasrefund.go @@ -4,6 +4,7 @@ import ( "fmt" errorsmod "cosmossdk.io/errors" + "cosmossdk.io/log" "cosmossdk.io/math" storetypes "cosmossdk.io/store/types" @@ -17,12 +18,14 @@ import ( var _ sdk.PostDecorator = &GasRefundDecorator{} type GasRefundDecorator struct { - ek EVMKeeper + logger log.Logger + ek EVMKeeper } -func NewGasRefundDecorator(ek EVMKeeper) sdk.PostDecorator { +func NewGasRefundDecorator(logger log.Logger, ek EVMKeeper) sdk.PostDecorator { + logger = logger.With("module", "gas_refund") return &GasRefundDecorator{ - ek, + logger, ek, } } @@ -70,20 +73,40 @@ func (erd *GasRefundDecorator) PostHandle(ctx sdk.Context, tx sdk.Tx, simulate, sdk.NewAttribute(AttributeKeyCoins, coinsRefund.String()), )) - // TODO - should we charge gas for refund? - // - // for now, we use infinite gas meter to prevent out of gas error or inconsistency between - // used gas and refunded gas. - feeCollectorAddr := authtypes.NewModuleAddress(authtypes.FeeCollectorName) - err = erd.ek.ERC20Keeper().SendCoins(ctx.WithGasMeter(storetypes.NewInfiniteGasMeter()), feeCollectorAddr, feePayer, coinsRefund) - if err != nil { - return ctx, err - } + // conduct gas refund + erd.safeRefund(ctx, feePayer, coinsRefund) } return next(ctx, tx, simulate, success) } +func (erd *GasRefundDecorator) safeRefund(ctx sdk.Context, feePayer sdk.AccAddress, coinsRefund sdk.Coins) { + defer func() { + if r := recover(); r != nil { + switch r := r.(type) { + case storetypes.ErrorOutOfGas: + erd.logger.Error("failed to refund gas", "err", r) + default: + panic(r) + } + } + }() + + // prepare context for refund operation + const gasLimitForRefund = 1_000_000 + cacheCtx, commit := ctx.CacheContext() + cacheCtx = cacheCtx.WithGasMeter(storetypes.NewGasMeter(gasLimitForRefund)) + + feeCollectorAddr := authtypes.NewModuleAddress(authtypes.FeeCollectorName) + err := erd.ek.ERC20Keeper().SendCoins(cacheCtx, feeCollectorAddr, feePayer, coinsRefund) + if err != nil { + erd.logger.Error("failed to refund gas", "err", err) + return + } + + commit() +} + const ( EventTypeGasRefund = "gas_refund" AttributeKeyGas = "gas" diff --git a/app/posthandler/posthandler.go b/app/posthandler/posthandler.go index 04132f8..02f302a 100644 --- a/app/posthandler/posthandler.go +++ b/app/posthandler/posthandler.go @@ -3,20 +3,20 @@ package posthandler import ( "context" + "cosmossdk.io/log" "cosmossdk.io/math" sdk "github.com/cosmos/cosmos-sdk/types" - authante "github.com/cosmos/cosmos-sdk/x/auth/ante" evmtypes "github.com/initia-labs/minievm/x/evm/types" ) // NewPostHandler returns a new sdk.PostHandler that is composed of the sdk.ChainPostDecorators func NewPostHandler( - ak authante.AccountKeeper, + logger log.Logger, ek EVMKeeper, ) sdk.PostHandler { return sdk.ChainPostDecorators( - NewGasRefundDecorator(ek), + NewGasRefundDecorator(logger, ek), ) } diff --git a/x/evm/keeper/txutils.go b/x/evm/keeper/txutils.go index 81d526a..ac51787 100644 --- a/x/evm/keeper/txutils.go +++ b/x/evm/keeper/txutils.go @@ -225,7 +225,7 @@ func (u *TxUtils) ConvertCosmosTxToEthereumTx(ctx context.Context, sdkTx sdk.Tx) return nil, nil, err } - if len(fees) > 0 && fees[0].Denom != params.FeeDenom { + if !(len(fees) == 0 || (len(fees) == 1 && fees[0].Denom == params.FeeDenom)) { return nil, nil, nil } @@ -403,7 +403,7 @@ func (u *TxUtils) IsEthereumTx(ctx context.Context, sdkTx sdk.Tx) (bool, error) return false, err } - if len(fees) > 0 && fees[0].Denom != params.FeeDenom { + if !(len(fees) == 0 || (len(fees) == 1 && fees[0].Denom == params.FeeDenom)) { return false, nil } From 872068262ed726c874eda796b308925ad9ead4a8 Mon Sep 17 00:00:00 2001 From: beer-1 Date: Mon, 4 Nov 2024 12:33:05 +0900 Subject: [PATCH 2/3] add test --- app/posthandler/gasrefund_test.go | 126 ++++++++++++++++++++++++++ app/posthandler/posthandler_test.go | 136 ++++++++++++++++++++++++++++ 2 files changed, 262 insertions(+) create mode 100644 app/posthandler/gasrefund_test.go create mode 100644 app/posthandler/posthandler_test.go diff --git a/app/posthandler/gasrefund_test.go b/app/posthandler/gasrefund_test.go new file mode 100644 index 0000000..0c794d0 --- /dev/null +++ b/app/posthandler/gasrefund_test.go @@ -0,0 +1,126 @@ +package posthandler_test + +import ( + "bytes" + "crypto/ecdsa" + "crypto/rand" + "math/big" + + "cosmossdk.io/math" + storetypes "cosmossdk.io/store/types" + sdk "github.com/cosmos/cosmos-sdk/types" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + + "github.com/ethereum/go-ethereum/common" + coretypes "github.com/ethereum/go-ethereum/core/types" + "github.com/ethereum/go-ethereum/crypto" + + "github.com/initia-labs/initia/crypto/ethsecp256k1" + "github.com/initia-labs/minievm/app/posthandler" + evmante "github.com/initia-labs/minievm/x/evm/ante" + "github.com/initia-labs/minievm/x/evm/contracts/erc20_factory" + "github.com/initia-labs/minievm/x/evm/keeper" + "github.com/initia-labs/minievm/x/evm/types" +) + +func (suite *PostHandlerTestSuite) Test_NotSpendingGasForTxWithFeeDenom() { + suite.SetupTest() // setup + suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() + + gasRefundPostHandler := posthandler.NewGasRefundDecorator(suite.app.Logger(), suite.app.EVMKeeper) + + params, err := suite.app.EVMKeeper.Params.Get(suite.ctx) + suite.NoError(err) + + // create fee token + decimals := uint8(18) + feeCollectorAddr := authtypes.NewModuleAddress(authtypes.FeeCollectorName) + suite.app.EVMKeeper.InitializeWithDecimals(suite.ctx, decimals) + err = suite.app.EVMKeeper.ERC20Keeper().CreateERC20(suite.ctx, params.FeeDenom, decimals) + suite.NoError(err) + + // mint fee token to fee collector + gasPrice := math.NewInt(1_000_000_000) + gasLimit := uint64(1_000_000) + paidFeeAmount := sdk.NewCoins(sdk.NewCoin(params.FeeDenom, gasPrice.Mul(math.NewIntFromUint64(gasLimit)))) + err = suite.app.EVMKeeper.ERC20Keeper().MintCoins(suite.ctx, feeCollectorAddr, paidFeeAmount) + suite.NoError(err) + + feeAmount := new(big.Int).Mul( + big.NewInt(int64(gasLimit)), + new(big.Int).Exp(big.NewInt(10), big.NewInt(int64(decimals)-8), nil), // gas price is 1e-8 + ) + + ethFactoryAddr, err := suite.app.EVMKeeper.GetERC20FactoryAddr(suite.ctx) + suite.NoError(err) + + abi, err := erc20_factory.Erc20FactoryMetaData.GetAbi() + suite.NoError(err) + + inputBz, err := abi.Pack("createERC20", "bar", "bar", uint8(6)) + suite.NoError(err) + + gasFeeCap := types.ToEthersUint(decimals, feeAmount) + gasFeeCap = gasFeeCap.Quo(gasFeeCap, new(big.Int).SetUint64(gasLimit)) + value := types.ToEthersUint(decimals, big.NewInt(100)) + + ethChainID := types.ConvertCosmosChainIDToEthereumChainID(suite.ctx.ChainID()) + ethTx := coretypes.NewTx(&coretypes.DynamicFeeTx{ + ChainID: types.ConvertCosmosChainIDToEthereumChainID(suite.ctx.ChainID()), + Nonce: 100, + GasTipCap: big.NewInt(100), + GasFeeCap: gasFeeCap, + Gas: gasLimit, + To: ðFactoryAddr, + Data: inputBz, + Value: value, + AccessList: coretypes.AccessList{ + coretypes.AccessTuple{Address: ethFactoryAddr, + StorageKeys: []common.Hash{ + common.HexToHash("0x0000000000000000000000000000000000000000000000000000000000000000"), + common.HexToHash("0x0000000000000000000000000000000000000000000000000000000000000001"), + common.HexToHash("0x0000000000000000000000000000000000000000000000000000000000000002"), + }}, + }, + }) + + randBytes := make([]byte, 64) + _, err = rand.Read(randBytes) + suite.NoError(err) + reader := bytes.NewReader(randBytes) + privKey, err := ecdsa.GenerateKey(crypto.S256(), reader) + suite.NoError(err) + signer := coretypes.LatestSignerForChainID(ethChainID) + signedTx, err := coretypes.SignTx(ethTx, signer, privKey) + suite.NoError(err) + + // Compute sender address + cosmosKey := ethsecp256k1.PrivKey{ + Key: crypto.FromECDSA(privKey), + } + addrBz := cosmosKey.PubKey().Address() + + // Convert to cosmos tx + sdkTx, err := keeper.NewTxUtils(suite.app.EVMKeeper).ConvertEthereumTxToCosmosTx(suite.ctx, signedTx) + suite.NoError(err) + + // Spend half of the gas + gasMeter := storetypes.NewGasMeter(gasLimit) + gasMeter.ConsumeGas(gasLimit/2-2216 /* 2216 is extra gas for refunds */, "test") + gasPrices := sdk.DecCoins{sdk.NewDecCoin(params.FeeDenom, gasPrice)} + + ctx := sdk.UnwrapSDKContext(suite.ctx).WithValue(evmante.ContextKeyGasPrices, gasPrices) + ctx = ctx.WithGasMeter(gasMeter).WithExecMode(sdk.ExecModeFinalize) + ctx, err = gasRefundPostHandler.PostHandle(ctx, sdkTx, false, true, func(ctx sdk.Context, tx sdk.Tx, simulate, success bool) (newCtx sdk.Context, err error) { + return ctx, nil + }) + suite.NoError(err) + + gasRefundRatio := params.GasRefundRatio + sender := sdk.AccAddress(addrBz.Bytes()) + + // Check the gas refund + amount := suite.app.BankKeeper.GetBalance(ctx, sender, params.FeeDenom) + refunds, _ := gasPrices.MulDec(gasRefundRatio.MulInt64(int64(gasLimit / 2))).TruncateDecimal() + suite.Equal(amount, refunds[0]) +} diff --git a/app/posthandler/posthandler_test.go b/app/posthandler/posthandler_test.go new file mode 100644 index 0000000..4903c20 --- /dev/null +++ b/app/posthandler/posthandler_test.go @@ -0,0 +1,136 @@ +package posthandler_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/suite" + + tmproto "github.com/cometbft/cometbft/proto/tendermint/types" + + "cosmossdk.io/log" + dbm "github.com/cosmos/cosmos-db" + "github.com/cosmos/cosmos-sdk/client" + "github.com/cosmos/cosmos-sdk/client/flags" + "github.com/cosmos/cosmos-sdk/client/tx" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" + "github.com/cosmos/cosmos-sdk/server" + simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" + "github.com/cosmos/cosmos-sdk/testutil/testdata" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/types/tx/signing" + authsign "github.com/cosmos/cosmos-sdk/x/auth/signing" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + simcli "github.com/cosmos/cosmos-sdk/x/simulation/client/cli" + + minievmapp "github.com/initia-labs/minievm/app" + evmconfig "github.com/initia-labs/minievm/x/evm/config" + evmtypes "github.com/initia-labs/minievm/x/evm/types" +) + +const feeDenom = "feetoken" + +// PostHandlerTestSuite is a test suite to be used with ante handler tests. +type PostHandlerTestSuite struct { + suite.Suite + + app *minievmapp.MinitiaApp + ctx sdk.Context + clientCtx client.Context + txBuilder client.TxBuilder +} + +// returns context and app with params set on account keeper +func (suite *PostHandlerTestSuite) createTestApp(tempDir string) (*minievmapp.MinitiaApp, sdk.Context) { + appOptions := make(simtestutil.AppOptionsMap, 0) + appOptions[flags.FlagHome] = tempDir + appOptions[server.FlagInvCheckPeriod] = simcli.FlagPeriodValue + + app := minievmapp.NewMinitiaApp( + log.NewNopLogger(), dbm.NewMemDB(), dbm.NewMemDB(), dbm.NewMemDB(), nil, true, evmconfig.DefaultEVMConfig(), appOptions, + ) + ctx := app.BaseApp.NewUncachedContext(false, tmproto.Header{}) + err := app.AccountKeeper.Params.Set(ctx, authtypes.DefaultParams()) + suite.NoError(err) + + params := evmtypes.DefaultParams() + params.FeeDenom = feeDenom + err = app.EVMKeeper.Params.Set(ctx, params) + suite.NoError(err) + + return app, ctx +} + +// SetupTest setups a new test, with new app, context, and anteHandler. +func (suite *PostHandlerTestSuite) SetupTest() { + tempDir := suite.T().TempDir() + suite.app, suite.ctx = suite.createTestApp(tempDir) + suite.ctx = suite.ctx.WithBlockHeight(1) + + // Set up TxConfig. + encodingConfig := minievmapp.MakeEncodingConfig() + + // We're using TestMsg encoding in some tests, so register it here. + encodingConfig.Amino.RegisterConcrete(&testdata.TestMsg{}, "testdata.TestMsg", nil) + testdata.RegisterInterfaces(encodingConfig.InterfaceRegistry) + + suite.clientCtx = client.Context{}. + WithTxConfig(encodingConfig.TxConfig) +} + +// CreateTestTx is a helper function to create a tx given multiple inputs. +func (suite *PostHandlerTestSuite) CreateTestTx(privs []cryptotypes.PrivKey, accNums []uint64, accSeqs []uint64, chainID string) (authsign.Tx, error) { + defaultSignMode, err := authsign.APISignModeToInternal(suite.clientCtx.TxConfig.SignModeHandler().DefaultMode()) + suite.NoError(err) + + // First round: we gather all the signer infos. We use the "set empty + // signature" hack to do that. + var sigsV2 []signing.SignatureV2 + for i, priv := range privs { + + sigV2 := signing.SignatureV2{ + PubKey: priv.PubKey(), + Data: &signing.SingleSignatureData{ + SignMode: defaultSignMode, + Signature: nil, + }, + Sequence: accSeqs[i], + } + + sigsV2 = append(sigsV2, sigV2) + } + err = suite.txBuilder.SetSignatures(sigsV2...) + if err != nil { + return nil, err + } + + // Second round: all signer infos are set, so each signer can sign. + sigsV2 = []signing.SignatureV2{} + for i, priv := range privs { + signerData := authsign.SignerData{ + Address: sdk.AccAddress(priv.PubKey().Address()).String(), + ChainID: chainID, + AccountNumber: accNums[i], + Sequence: accSeqs[i], + PubKey: priv.PubKey(), + } + sigV2, err := tx.SignWithPrivKey( + context.TODO(), defaultSignMode, signerData, + suite.txBuilder, priv, suite.clientCtx.TxConfig, accSeqs[i]) + if err != nil { + return nil, err + } + + sigsV2 = append(sigsV2, sigV2) + } + err = suite.txBuilder.SetSignatures(sigsV2...) + if err != nil { + return nil, err + } + + return suite.txBuilder.GetTx(), nil +} + +func TestPostHandlerTestSuite(t *testing.T) { + suite.Run(t, new(PostHandlerTestSuite)) +} From f2fdd32fa94a0781ce04bd06ffb3f829d4651676 Mon Sep 17 00:00:00 2001 From: beer-1 Date: Mon, 4 Nov 2024 12:34:16 +0900 Subject: [PATCH 3/3] add logger message --- app/posthandler/gasrefund.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/posthandler/gasrefund.go b/app/posthandler/gasrefund.go index 8fc8ecf..b762a80 100644 --- a/app/posthandler/gasrefund.go +++ b/app/posthandler/gasrefund.go @@ -85,7 +85,7 @@ func (erd *GasRefundDecorator) safeRefund(ctx sdk.Context, feePayer sdk.AccAddre if r := recover(); r != nil { switch r := r.(type) { case storetypes.ErrorOutOfGas: - erd.logger.Error("failed to refund gas", "err", r) + erd.logger.Error("failed to refund gas", "err", r, "feePayer", feePayer, "refudnAmount", coinsRefund) default: panic(r) }