From 7807722fb9fa11d09ac26cd2f8bac29cf15e29ab Mon Sep 17 00:00:00 2001 From: Callum Waters Date: Mon, 24 Jul 2023 15:10:22 +0200 Subject: [PATCH] feat: backport features to allow node to handle gas price estimation (#2134) Backports #2122 --------- Co-authored-by: Rootul P --- app/app.go | 8 ++ app/errors/errors.go | 80 ++++++++++++++++++++ app/errors/errors_test.go | 145 +++++++++++++++++++++++++++++++++++++ test/txsim/blob.go | 21 ++---- test/txsim/sequence.go | 6 +- x/blob/ante/ante.go | 13 +--- x/blob/keeper/keeper.go | 9 +-- x/blob/types/payforblob.go | 51 +++++++++++++ 8 files changed, 296 insertions(+), 37 deletions(-) create mode 100644 app/errors/errors.go create mode 100644 app/errors/errors_test.go diff --git a/app/app.go b/app/app.go index 4472ce4aae..651c54c0d2 100644 --- a/app/app.go +++ b/app/app.go @@ -10,6 +10,7 @@ import ( minttypes "github.com/celestiaorg/celestia-app/x/mint/types" "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/client" + nodeservice "github.com/cosmos/cosmos-sdk/client/grpc/node" "github.com/cosmos/cosmos-sdk/client/grpc/tmservice" "github.com/cosmos/cosmos-sdk/codec" "github.com/cosmos/cosmos-sdk/codec/types" @@ -676,6 +677,9 @@ func (app *App) RegisterAPIRoutes(apiSvr *api.Server, _ config.APIConfig) { // Register new tendermint queries routes from grpc-gateway. tmservice.RegisterGRPCGatewayRoutes(clientCtx, apiSvr.GRPCGatewayRouter) + // Register node gRPC service for grpc-gateway. + nodeservice.RegisterGRPCGatewayRoutes(clientCtx, apiSvr.GRPCGatewayRouter) + // Register the ModuleBasics.RegisterGRPCGatewayRoutes(clientCtx, apiSvr.GRPCGatewayRouter) } @@ -690,6 +694,10 @@ func (app *App) RegisterTendermintService(clientCtx client.Context) { tmservice.RegisterTendermintService(clientCtx, app.BaseApp.GRPCQueryRouter(), app.interfaceRegistry, nil) } +func (app *App) RegisterNodeService(clientCtx client.Context) { + nodeservice.RegisterNodeService(clientCtx, app.GRPCQueryRouter()) +} + func (app *App) setPostHanders() { postHandler, err := posthandler.NewPostHandler( posthandler.HandlerOptions{}, diff --git a/app/errors/errors.go b/app/errors/errors.go new file mode 100644 index 0000000000..c45658f664 --- /dev/null +++ b/app/errors/errors.go @@ -0,0 +1,80 @@ +package errors + +import ( + "errors" + "fmt" + "regexp" + "strconv" + + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" +) + +var ( + // This is relatively brittle. It would be better if going below the min gas price + // had a specific error type. + regexpMinGasPrice = regexp.MustCompile(`insufficient fees; got: \d+utia required: \d+utia`) + regexpInt = regexp.MustCompile(`[0-9]+`) +) + +// ParseInsufficientMinGasPrice checks if the error is due to the gas price being too low. +// Given the previous gas price and gas limit, it returns the new minimum gas price that +// the node should accept. +// If the error is not due to the gas price being too low, it returns 0, nil +func ParseInsufficientMinGasPrice(err error, gasPrice float64, gasLimit uint64) (float64, error) { + // first work out if the error is ErrInsufficientFunds + if err == nil || !sdkerrors.ErrInsufficientFee.Is(err) { + return 0, nil + } + + // As there are multiple cases of ErrInsufficientFunds, we need to check the error message + // matches the regexp + substr := regexpMinGasPrice.FindAllString(err.Error(), -1) + if len(substr) != 1 { + return 0, nil + } + + // extract the first and second numbers from the error message (got and required) + numbers := regexpInt.FindAllString(substr[0], -1) + if len(numbers) != 2 { + return 0, fmt.Errorf("expected two numbers in error message got %d", len(numbers)) + } + + // attempt to parse them into float64 values + got, err := strconv.ParseFloat(numbers[0], 64) + if err != nil { + return 0, err + } + required, err := strconv.ParseFloat(numbers[1], 64) + if err != nil { + return 0, err + } + + // catch rare condition that required is zero. This should theoretically + // never happen as a min gas price of zero should always be accepted. + if required == 0 { + return 0, errors.New("unexpected case: required gas price is zero (why was an error returned)") + } + + // calculate the actual min gas price of the node based on the difference + // between the got and required values. If gas price was zero, we need to use + // the gasLimit to infer this. + if gasPrice == 0 || got == 0 { + if gasLimit == 0 { + return 0, fmt.Errorf("gas limit and gas price cannot be zero") + } + return required / float64(gasLimit), nil + } + return required / got * gasPrice, nil +} + +// IsInsufficientMinGasPrice checks if the error is due to the gas price being too low. +func IsInsufficientMinGasPrice(err error) bool { + // first work out if the error is ErrInsufficientFunds + if err == nil || !sdkerrors.ErrInsufficientFee.Is(err) { + return false + } + + // As there are multiple cases of ErrInsufficientFunds, we need to check the error message + // matches the regexp + return regexpMinGasPrice.MatchString(err.Error()) +} diff --git a/app/errors/errors_test.go b/app/errors/errors_test.go new file mode 100644 index 0000000000..071f110aac --- /dev/null +++ b/app/errors/errors_test.go @@ -0,0 +1,145 @@ +package errors_test + +import ( + "fmt" + "testing" + + "cosmossdk.io/errors" + "github.com/celestiaorg/celestia-app/app" + apperr "github.com/celestiaorg/celestia-app/app/errors" + "github.com/celestiaorg/celestia-app/pkg/appconsts" + "github.com/celestiaorg/celestia-app/pkg/namespace" + testutil "github.com/celestiaorg/celestia-app/test/util" + blob "github.com/celestiaorg/celestia-app/x/blob/types" + sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/cosmos/cosmos-sdk/x/auth/ante" + "github.com/stretchr/testify/require" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" +) + +// This will detect any changes to the DeductFeeDecorator which may cause a +// different error message that does not match the regexp. +func TestInsufficientMinGasPriceIntegration(t *testing.T) { + var ( + gasLimit uint64 = 1_000_000 + feeAmount int64 = 10 + gasPrice = float64(feeAmount) / float64(gasLimit) + ) + account := "test" + testApp, kr := testutil.SetupTestAppWithGenesisValSet(app.DefaultConsensusParams(), account) + minGasPrice, err := sdk.ParseDecCoins(fmt.Sprintf("%v%s", appconsts.DefaultMinGasPrice, app.BondDenom)) + require.NoError(t, err) + ctx := testApp.NewContext(true, tmproto.Header{}).WithMinGasPrices(minGasPrice) + signer := blob.NewKeyringSigner(kr, account, testutil.ChainID) + builder := signer.NewTxBuilder() + builder.SetGasLimit(gasLimit) + builder.SetFeeAmount(sdk.NewCoins(sdk.NewCoin(app.BondDenom, sdk.NewInt(feeAmount)))) + + address, err := signer.GetSignerInfo().GetAddress() + require.NoError(t, err) + + _, err = sdk.AccAddressFromBech32(address.String()) + require.NoError(t, err, address) + + b, err := blob.NewBlob(namespace.RandomNamespace(), []byte("hello world"), 0) + require.NoError(t, err) + + pfb, err := blob.NewMsgPayForBlobs(address.String(), b) + require.NoError(t, err, address) + + tx, err := signer.BuildSignedTx(builder, pfb) + require.NoError(t, err) + + decorator := ante.NewDeductFeeDecorator(testApp.AccountKeeper, testApp.BankKeeper, testApp.FeeGrantKeeper, nil) + anteHandler := sdk.ChainAnteDecorators(decorator) + + _, err = anteHandler(ctx, tx, false) + require.True(t, apperr.IsInsufficientMinGasPrice(err)) + actualGasPrice, err := apperr.ParseInsufficientMinGasPrice(err, gasPrice, gasLimit) + require.NoError(t, err) + require.Equal(t, appconsts.DefaultMinGasPrice, actualGasPrice, err) +} + +func TestInsufficientMinGasPriceTable(t *testing.T) { + testCases := []struct { + name string + err error + inputGasPrice float64 + inputGasLimit uint64 + isInsufficientMinGasPriceErr bool + expectParsingError bool + expectedGasPrice float64 + }{ + { + name: "nil error", + err: nil, + isInsufficientMinGasPriceErr: false, + }, + { + name: "not insufficient fee error", + err: errors.Wrap(sdkerrors.ErrInsufficientFee, "not enough gas to pay for blobs (minimum: 1000000, got: 100000)"), + isInsufficientMinGasPriceErr: false, + }, + { + name: "not insufficient fee error 2", + err: errors.Wrap(sdkerrors.ErrInsufficientFunds, "not enough gas to pay for blobs (got: 1000000, required: 100000)"), + isInsufficientMinGasPriceErr: false, + }, + { + name: "insufficient fee error", + err: errors.Wrap(sdkerrors.ErrInsufficientFee, "insufficient fees; got: 10utia required: 100utia"), + inputGasPrice: 0.01, + expectedGasPrice: 0.1, + isInsufficientMinGasPriceErr: true, + }, + { + name: "insufficient fee error with zero gas price", + err: errors.Wrap(sdkerrors.ErrInsufficientFee, "insufficient fees; got: 0utia required: 100utia"), + inputGasPrice: 0, + inputGasLimit: 100, + expectedGasPrice: 1, + isInsufficientMinGasPriceErr: true, + }, + { + name: "insufficient fee error with zero gas price and zero gas limit", + err: errors.Wrap(sdkerrors.ErrInsufficientFee, "insufficient fees; got: 0utia required: 100utia"), + inputGasPrice: 0, + inputGasLimit: 0, + isInsufficientMinGasPriceErr: true, + expectParsingError: true, + }, + { + name: "incorrectly formatted error", + err: errors.Wrap(sdkerrors.ErrInsufficientFee, "insufficient fees; got: 0uatom required: 100uatom"), + isInsufficientMinGasPriceErr: false, + }, + { + name: "error with zero required gas price", + err: errors.Wrap(sdkerrors.ErrInsufficientFee, "insufficient fees; got: 10utia required: 0utia"), + isInsufficientMinGasPriceErr: true, + expectParsingError: true, + }, + { + name: "error with extra wrapping", + err: errors.Wrap(errors.Wrap(sdkerrors.ErrInsufficientFee, "insufficient fees; got: 10utia required: 100utia"), "extra wrapping"), + inputGasPrice: 0.01, + expectedGasPrice: 0.1, + isInsufficientMinGasPriceErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.isInsufficientMinGasPriceErr, apperr.IsInsufficientMinGasPrice(tc.err)) + actualGasPrice, err := apperr.ParseInsufficientMinGasPrice(tc.err, tc.inputGasPrice, tc.inputGasLimit) + if tc.expectParsingError { + require.Error(t, err) + require.Zero(t, actualGasPrice) + } else { + require.NoError(t, err) + require.Equal(t, tc.expectedGasPrice, actualGasPrice) + } + }) + } +} diff --git a/test/txsim/blob.go b/test/txsim/blob.go index db8d081024..33f2a03db2 100644 --- a/test/txsim/blob.go +++ b/test/txsim/blob.go @@ -5,7 +5,6 @@ import ( "fmt" "math/rand" - "github.com/celestiaorg/celestia-app/pkg/appconsts" ns "github.com/celestiaorg/celestia-app/pkg/namespace" "github.com/celestiaorg/celestia-app/test/util/blobfactory" blob "github.com/celestiaorg/celestia-app/x/blob/types" @@ -86,7 +85,7 @@ func (s *BlobSequence) Next(_ context.Context, _ grpc.ClientConn, rand *rand.Ran return Operation{ Msgs: []types.Msg{msg}, Blobs: blobs, - GasLimit: EstimateGas(sizes), + GasLimit: estimateGas(sizes), }, nil } @@ -107,18 +106,12 @@ func (r Range) Rand(rand *rand.Rand) int { return rand.Intn(r.Max-r.Min) + r.Min } -const ( - perByteGasTolerance = 2 - pfbGasFixedCost = 80000 -) - -// EstimateGas estimates the gas required to pay for a set of blobs in a PFB. -func EstimateGas(blobSizes []int) uint64 { - totalByteCount := 0 - for _, size := range blobSizes { - totalByteCount += size +// estimateGas estimates the gas required to pay for a set of blobs in a PFB. +func estimateGas(blobSizes []int) uint64 { + size := make([]uint32, len(blobSizes)) + for i, s := range blobSizes { + size[i] = uint32(s) } - variableGasAmount := (appconsts.DefaultGasPerBlobByte + perByteGasTolerance) * totalByteCount - return uint64(variableGasAmount + pfbGasFixedCost) + return blob.DefaultEstimateGas(size) } diff --git a/test/txsim/sequence.go b/test/txsim/sequence.go index 126be3c985..b3206fe73b 100644 --- a/test/txsim/sequence.go +++ b/test/txsim/sequence.go @@ -42,9 +42,9 @@ type Operation struct { } const ( - // set default gas limit to cover the costs of most transactions - // At 0.001 utia per gas, this equates to 1000utia per transaction - DefaultGasLimit = 1000000 + // Set the default gas limit to cover the costs of most transactions. + // At 0.1 utia per gas, this equates to 20_000utia per transaction. + DefaultGasLimit = 200_000 ) // ErrEndOfSequence is a special error which indicates that the sequence has been terminated diff --git a/x/blob/ante/ante.go b/x/blob/ante/ante.go index 1893db35e2..c5eb82de1b 100644 --- a/x/blob/ante/ante.go +++ b/x/blob/ante/ante.go @@ -1,8 +1,6 @@ package ante import ( - "github.com/celestiaorg/celestia-app/pkg/appconsts" - "github.com/celestiaorg/celestia-app/pkg/shares" "github.com/celestiaorg/celestia-app/x/blob/types" "cosmossdk.io/errors" @@ -38,7 +36,7 @@ func (d MinGasPFBDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool // lazily fetch the gas per byte param gasPerByte = d.k.GasPerBlobByte(ctx) } - gasToConsume := gasToConsume(pfb, gasPerByte) + gasToConsume := pfb.Gas(gasPerByte) if gasToConsume > txGas { return ctx, errors.Wrapf(sdkerrors.ErrInsufficientFee, "not enough gas to pay for blobs (minimum: %d, got: %d)", gasToConsume, txGas) } @@ -48,15 +46,6 @@ func (d MinGasPFBDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool return next(ctx, tx, simulate) } -func gasToConsume(pfb *types.MsgPayForBlobs, gasPerByte uint32) uint64 { - var totalSharesUsed uint64 - for _, size := range pfb.BlobSizes { - totalSharesUsed += uint64(shares.SparseSharesNeeded(size)) - } - - return totalSharesUsed * appconsts.ShareSize * uint64(gasPerByte) -} - type BlobKeeper interface { GasPerBlobByte(ctx sdk.Context) uint32 } diff --git a/x/blob/keeper/keeper.go b/x/blob/keeper/keeper.go index af40268318..34ad7f5e01 100644 --- a/x/blob/keeper/keeper.go +++ b/x/blob/keeper/keeper.go @@ -4,8 +4,6 @@ import ( "context" "fmt" - "github.com/celestiaorg/celestia-app/pkg/appconsts" - "github.com/celestiaorg/celestia-app/pkg/shares" "github.com/celestiaorg/celestia-app/x/blob/types" "github.com/cosmos/cosmos-sdk/codec" storetypes "github.com/cosmos/cosmos-sdk/store/types" @@ -52,12 +50,7 @@ func (k Keeper) Logger(ctx sdk.Context) log.Logger { func (k Keeper) PayForBlobs(goCtx context.Context, msg *types.MsgPayForBlobs) (*types.MsgPayForBlobsResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) - var totalSharesUsed uint64 - for _, size := range msg.BlobSizes { - totalSharesUsed += uint64(shares.SparseSharesNeeded(size)) - } - - gasToConsume := totalSharesUsed * appconsts.ShareSize * uint64(k.GasPerBlobByte(ctx)) + gasToConsume := types.GasToConsume(msg.BlobSizes, k.GasPerBlobByte(ctx)) ctx.GasMeter().ConsumeGas(gasToConsume, payForBlobGasDescriptor) err := ctx.EventManager().EmitTypedEvent( diff --git a/x/blob/types/payforblob.go b/x/blob/types/payforblob.go index 5d15bd8957..1685e03c31 100644 --- a/x/blob/types/payforblob.go +++ b/x/blob/types/payforblob.go @@ -12,6 +12,7 @@ import ( "github.com/celestiaorg/nmt" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx" + auth "github.com/cosmos/cosmos-sdk/x/auth/types" "github.com/tendermint/tendermint/crypto/merkle" tmproto "github.com/tendermint/tendermint/proto/tendermint/types" coretypes "github.com/tendermint/tendermint/types" @@ -21,6 +22,26 @@ import ( const ( URLMsgPayForBlobs = "/celestia.blob.v1.MsgPayForBlobs" ShareSize = appconsts.ShareSize + + // PFBGasFixedCost is a rough estimate for the "fixed cost" in the gas cost + // formula: gas cost = gas per byte * bytes per share * shares occupied by + // blob + "fixed cost". In this context, "fixed cost" accounts for the gas + // consumed by operations outside the blob's GasToConsume function (i.e. + // signature verification, tx size, read access to accounts). + // + // Since the gas cost of these operations is not easy to calculate, linear + // regression was performed on a set of observed data points to derive an + // approximate formula for gas cost. Assuming gas per byte = 8 and bytes per + // share = 512, we can solve for "fixed cost" and arrive at 65,000. gas cost + // = 8 * 512 * number of shares occupied by the blob + 65,000 has a + // correlation coefficient of 0.996. To be conservative, we round up "fixed + // cost" to 75,000 because the first tx always takes up 10,000 more gas than + // subsequent txs. + PFBGasFixedCost = 75000 + + // BytesPerBlobInfo is a rough estimation for the amount of extra bytes in + // information a blob adds to the size of the underlying transaction. + BytesPerBlobInfo = 70 ) // MsgPayForBlobs implements the `LegacyMsg` interface. @@ -130,6 +151,36 @@ func (msg *MsgPayForBlobs) ValidateBasic() error { return nil } +func (msg *MsgPayForBlobs) Gas(gasPerByte uint32) uint64 { + return GasToConsume(msg.BlobSizes, gasPerByte) +} + +// GasToConsume works out the extra gas charged to pay for a set of blobs in a PFB. +// Note that tranasctions will incur other gas costs, such as the signature verification +// and reads to the user's account. +func GasToConsume(blobSizes []uint32, gasPerByte uint32) uint64 { + var totalSharesUsed uint64 + for _, size := range blobSizes { + totalSharesUsed += uint64(appshares.SparseSharesNeeded(size)) + } + + return totalSharesUsed * appconsts.ShareSize * uint64(gasPerByte) +} + +// EstimateGas estimates the total gas required to pay for a set of blobs in a PFB. +// It is based on a linear model that is dependent on the governance parameters: +// gasPerByte and txSizeCost. It assumes other variables are constant. This includes +// assuming the PFB is the only message in the transaction. +func EstimateGas(blobSizes []uint32, gasPerByte uint32, txSizeCost uint64) uint64 { + return GasToConsume(blobSizes, gasPerByte) + (txSizeCost * BytesPerBlobInfo * uint64(len(blobSizes))) + PFBGasFixedCost +} + +// DefaultEstimateGas runs EstimateGas with the system defaults. The network may change these values +// through governance, thus this function should predominantly be used in testing. +func DefaultEstimateGas(blobSizes []uint32) uint64 { + return EstimateGas(blobSizes, appconsts.DefaultGasPerBlobByte, auth.DefaultTxSizeCostPerByte) +} + // ValidateBlobNamespace returns an error if the provided namespace is reserved, // parity shares, or tail padding. func ValidateBlobNamespace(ns appns.Namespace) error {