From 6e382ac9425cfa7c1f259f34dc4028dea41f72e3 Mon Sep 17 00:00:00 2001 From: Shane Bammel Date: Fri, 24 Mar 2023 22:53:42 -0500 Subject: [PATCH] Add padding to gas estimations Adds a 20% buffer to gas estimations to reduce out-of-gas errors. --- eth/gasestimator/gasestimator.go | 34 +++++++++++++++++++++++--------- ethclient/ethclient_test.go | 4 ++-- graphql/graphql_test.go | 2 +- internal/ethapi/api_test.go | 18 ++++++++--------- 4 files changed, 37 insertions(+), 21 deletions(-) diff --git a/eth/gasestimator/gasestimator.go b/eth/gasestimator/gasestimator.go index f07f98956..7044375b6 100644 --- a/eth/gasestimator/gasestimator.go +++ b/eth/gasestimator/gasestimator.go @@ -69,6 +69,8 @@ func Estimate(ctx context.Context, call *core.Message, opts *Options, gasCap uin } else { feeCap = common.Big0 } + // Track the maximum gas based on the account's available funds and the txn feeCap. + var accountGasLimit uint64 // Recap the highest gas limit with account's available balance. if feeCap.BitLen() != 0 { balance := opts.State.GetBalance(call.From).ToBig() @@ -83,14 +85,17 @@ func Estimate(ctx context.Context, call *core.Message, opts *Options, gasCap uin allowance := new(big.Int).Div(available, feeCap) // If the allowance is larger than maximum uint64, skip checking - if allowance.IsUint64() && hi > allowance.Uint64() { - transfer := call.Value - if transfer == nil { - transfer = new(big.Int) + if allowance.IsUint64() { + accountGasLimit = allowance.Uint64() + if hi > allowance.Uint64() { + transfer := call.Value + if transfer == nil { + transfer = new(big.Int) + } + log.Debug("Gas estimation capped by limited funds", "original", hi, "balance", balance, + "sent", transfer, "maxFeePerGas", feeCap, "fundable", allowance) + hi = allowance.Uint64() } - log.Debug("Gas estimation capped by limited funds", "original", hi, "balance", balance, - "sent", transfer, "maxFeePerGas", feeCap, "fundable", allowance) - hi = allowance.Uint64() } } // Recap the highest gas allowance with specified gascap. @@ -98,6 +103,17 @@ func Estimate(ctx context.Context, call *core.Message, opts *Options, gasCap uin log.Debug("Caller gas above allowance, capping", "requested", hi, "cap", gasCap) hi = gasCap } + + // Adds a 20% pad to the estimated gas usage, not exceeding account gas limit + // to help mitigate gas underestimations + padResult := func(gas uint64) uint64 { + gas = gas + gas/5 + if accountGasLimit != 0 && gas > accountGasLimit { + gas = accountGasLimit + } + return gas + } + // If the transaction is a plain value transfer, short circuit estimation and // directly try 21000. Returning 21000 without any execution is dangerous as // some tx field combos might bump the price up even for plain transfers (e.g. @@ -106,7 +122,7 @@ func Estimate(ctx context.Context, call *core.Message, opts *Options, gasCap uin if call.To != nil && opts.State.GetCodeSize(*call.To) == 0 { failed, _, err := execute(ctx, call, opts, params.TxGas) if !failed && err == nil { - return params.TxGas, nil, nil + return padResult(params.TxGas), nil, nil } } } @@ -178,7 +194,7 @@ func Estimate(ctx context.Context, call *core.Message, opts *Options, gasCap uin hi = mid } } - return hi, nil, nil + return padResult(hi), nil, nil } // execute is a helper that executes the transaction under a given gas limit and diff --git a/ethclient/ethclient_test.go b/ethclient/ethclient_test.go index 0d2675f8d..b6836164b 100644 --- a/ethclient/ethclient_test.go +++ b/ethclient/ethclient_test.go @@ -553,7 +553,7 @@ func testCallContractAtHash(t *testing.T, client *rpc.Client) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if gas != 21000 { + if gas != 25200 { t.Fatalf("unexpected gas price: %v", gas) } block, err := ec.HeaderByNumber(context.Background(), big.NewInt(1)) @@ -580,7 +580,7 @@ func testCallContract(t *testing.T, client *rpc.Client) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if gas != 21000 { + if gas != 25200 { t.Fatalf("unexpected gas price: %v", gas) } // CallContract diff --git a/graphql/graphql_test.go b/graphql/graphql_test.go index 1dda10205..f95523d30 100644 --- a/graphql/graphql_test.go +++ b/graphql/graphql_test.go @@ -139,7 +139,7 @@ func TestGraphQLBlockSerialization(t *testing.T) { // should return `estimateGas` as decimal { body: `{"query": "{block{ estimateGas(data:{}) }}"}`, - want: `{"data":{"block":{"estimateGas":"0xd221"}}}`, + want: `{"data":{"block":{"estimateGas":"0xfc27"}}}`, code: 200, }, // should return `status` as decimal diff --git a/internal/ethapi/api_test.go b/internal/ethapi/api_test.go index a6f7405eb..0101c32fb 100644 --- a/internal/ethapi/api_test.go +++ b/internal/ethapi/api_test.go @@ -663,7 +663,7 @@ func TestEstimateGas(t *testing.T) { Value: (*hexutil.Big)(big.NewInt(1000)), }, expectErr: nil, - want: 21000, + want: 25200, }, // simple transfer with insufficient funds on latest block { @@ -674,14 +674,14 @@ func TestEstimateGas(t *testing.T) { Value: (*hexutil.Big)(big.NewInt(1000)), }, expectErr: core.ErrInsufficientFunds, - want: 21000, + want: 25200, }, // empty create { blockNumber: rpc.LatestBlockNumber, call: TransactionArgs{}, expectErr: nil, - want: 53000, + want: 64551, }, { blockNumber: rpc.LatestBlockNumber, @@ -690,7 +690,7 @@ func TestEstimateGas(t *testing.T) { randomAccounts[0].addr: OverrideAccount{Balance: newRPCBalance(new(big.Int).Mul(big.NewInt(1), big.NewInt(params.Ether)))}, }, expectErr: nil, - want: 53000, + want: 64551, }, { blockNumber: rpc.LatestBlockNumber, @@ -722,7 +722,7 @@ func TestEstimateGas(t *testing.T) { GasPrice: (*hexutil.Big)(big.NewInt(1_000_000_000)), // Legacy as pricing }, expectErr: nil, - want: 67617, + want: 82171, }, { blockNumber: rpc.LatestBlockNumber, @@ -732,7 +732,7 @@ func TestEstimateGas(t *testing.T) { MaxFeePerGas: (*hexutil.Big)(big.NewInt(1_000_000_000)), // 1559 gas pricing }, expectErr: nil, - want: 67617, + want: 82171, }, { blockNumber: rpc.LatestBlockNumber, @@ -743,7 +743,7 @@ func TestEstimateGas(t *testing.T) { MaxFeePerGas: nil, // No 1559 gas pricing }, expectErr: nil, - want: 67595, + want: 82144, }, // Blobs should have no effect on gas estimate { @@ -755,7 +755,7 @@ func TestEstimateGas(t *testing.T) { BlobHashes: []common.Hash{common.Hash{0x01, 0x22}}, BlobFeeCap: (*hexutil.Big)(big.NewInt(1)), }, - want: 21000, + want: 25200, }, } for i, tc := range testSuite { @@ -1008,7 +1008,7 @@ func TestSignTransaction(t *testing.T) { if err != nil { t.Fatal(err) } - expect := `{"type":"0x2","chainId":"0x1","nonce":"0x0","to":"0x703c4b2bd70c169f5717101caee543299fc946c7","gas":"0x5208","gasPrice":null,"maxPriorityFeePerGas":"0x0","maxFeePerGas":"0x684ee180","value":"0x1","input":"0x","accessList":[],"v":"0x0","r":"0x8fabeb142d585dd9247f459f7e6fe77e2520c88d50ba5d220da1533cea8b34e1","s":"0x582dd68b21aef36ba23f34e49607329c20d981d30404daf749077f5606785ce7","yParity":"0x0","hash":"0x93927839207cfbec395da84b8a2bc38b7b65d2cb2819e9fef1f091f5b1d4cc8f"}` + expect := `{"type":"0x2","chainId":"0x1","nonce":"0x0","to":"0x703c4b2bd70c169f5717101caee543299fc946c7","gas":"0x6270","gasPrice":null,"maxPriorityFeePerGas":"0x0","maxFeePerGas":"0x684ee180","value":"0x1","input":"0x","accessList":[],"v":"0x0","r":"0xecde8d8a090f99fa06adcb5dacd9ca415cd31b002355b8285639e78f227fe56e","s":"0x91b92ca049b354ab795430e0d465d9901ef34fdb12a28a598be126441761dc6","yParity":"0x0","hash":"0x79368920659e6e4a5b1fca57cbe28657b4263fa447a8412bdb474347447d2ceb"}` if !bytes.Equal(tx, []byte(expect)) { t.Errorf("result mismatch. Have:\n%s\nWant:\n%s\n", tx, expect) }