From cfa595b409acb78aa445b30c35e293797b9d1ce9 Mon Sep 17 00:00:00 2001 From: Stefan Iliev <46542846+StefanIliev545@users.noreply.github.com> Date: Wed, 26 Feb 2025 14:16:14 +0200 Subject: [PATCH] Fix gas price (#2351) * Some fixes. * Revert removal of gas price normalization. * Updates. --------- Co-authored-by: StefanIliev545 Co-authored-by: StefanIliev545 --- contracts/scripts/test-eth-call.ts | 194 +++++++++++++++++++++++++++++ go/enclave/rpc/EstimateGas.go | 75 ++++++----- go/enclave/rpc/TenEthCall.go | 3 + 3 files changed, 240 insertions(+), 32 deletions(-) create mode 100644 contracts/scripts/test-eth-call.ts diff --git a/contracts/scripts/test-eth-call.ts b/contracts/scripts/test-eth-call.ts new file mode 100644 index 000000000..44df42eb3 --- /dev/null +++ b/contracts/scripts/test-eth-call.ts @@ -0,0 +1,194 @@ +import { ethers } from "hardhat"; +import { HardhatRuntimeEnvironment } from "hardhat/types"; +async function main(hre: HardhatRuntimeEnvironment) { + // Get network information + const network = await ethers.provider.getNetwork(); + console.log(`Connected to network: ${network.name} (chainId: ${network.chainId})`); + + // Get signers - first is funded, create an unfunded one + const signers = await ethers.getSigners(); + const fundedWallet = signers[0]!; + const unfundedWallet = signers[2]!; + + // Print wallet addresses + console.log(`Funded Wallet: ${fundedWallet.address}`); + console.log(`Unfunded Wallet: ${unfundedWallet.address}`); + + // Set gas price for testing + const gasPrice = ethers.parseUnits("100", "gwei"); + console.log(`Gas price for tests: ${ethers.formatUnits(gasPrice, 'gwei')} gwei`); + + // Get balance of funded wallet + const fundedBalance = await ethers.provider.getBalance(fundedWallet.address); + console.log(`Funded Wallet Balance: ${ethers.formatEther(fundedBalance)} ETH`); + + console.log("\n=== Testing eth_call ==="); + + // Test 1: eth_call with funded account (basic call to check ETH balance) + try { + console.log("\nTest 1: eth_call with funded account, no gas price"); + const result1 = await ethers.provider.call({ + from: fundedWallet.address, + to: fundedWallet.address, + value: 0 + }); + console.log(`Result: ${result1}`); + } catch (error) { + console.error(`Error: ${error.message}`); + } + + // Test 2: eth_call with funded account with gas price + try { + console.log("\nTest 2: eth_call with funded account, with gas price"); + const result2 = await ethers.provider.call({ + from: fundedWallet.address, + to: fundedWallet.address, + value: 0, + gasPrice + }); + console.log(`Result: ${result2}`); + } catch (error) { + console.error(`Error: ${error.message}`); + } + + // Test 3: eth_call with unfunded account, no gas price + try { + console.log("\nTest 3: eth_call with unfunded account, no gas price"); + const result3 = await ethers.provider.call({ + from: unfundedWallet.address, + to: fundedWallet.address, + value: 0, + gasPrice: null + }); + console.log(`Result: ${result3}`); + } catch (error) { + console.error(`Error: ${error.message}`); + } + + // Test 4: eth_call with unfunded account, with gas price (should fail) + try { + console.log("\nTest 4: eth_call with unfunded account, with gas price"); + const result4 = await ethers.provider.call({ + from: unfundedWallet.address, + to: fundedWallet.address, + value: 0, + gasPrice + }); + console.log(`Result: ${result4}`); + } catch (error) { + console.error(`Error: ${error.message}`); + } + + console.log("\n=== Testing eth_estimateGas ==="); + + // Test 5: Estimate gas - Funded account with no gas price + try { + console.log("\nTest 5: estimateGas with funded account, no gas price"); + const gasEstimate = await ethers.provider.estimateGas({ + from: fundedWallet.address, + to: fundedWallet.address, + value: ethers.parseEther("0.001"), + gasPrice: null + }); + console.log(`Gas estimate: ${gasEstimate.toString()}`); + } catch (error) { + console.error(`Error: ${error.message}`); + } + + // Test 6: Estimate gas - Funded account with gas price + try { + console.log("\nTest 6: estimateGas with funded account, with gas price"); + const gasEstimate = await ethers.provider.estimateGas({ + from: fundedWallet.address, + to: fundedWallet.address, + value: ethers.parseEther("0.001"), + gasPrice + }); + console.log(`Gas estimate: ${gasEstimate.toString()}`); + } catch (error) { + console.error(`Error: ${error.message}`); + } + + // Test 7: Estimate gas - Unfunded account with no gas price + try { + console.log("\nTest 7: estimateGas with unfunded account, no gas price"); + const gasEstimate = await ethers.provider.estimateGas({ + from: unfundedWallet.address, + to: fundedWallet.address, + value: ethers.parseEther("0.001"), + gasPrice: null + }); + console.log(`Gas estimate: ${gasEstimate.toString()}`); + } catch (error) { + console.error(`Error: ${error.message}`); + } + + // Test 8: Estimate gas - Unfunded account with gas price (should fail) + try { + console.log("\nTest 8: estimateGas with unfunded account, with gas price"); + const gasEstimate = await ethers.provider.estimateGas({ + from: unfundedWallet.address, + to: fundedWallet.address, + value: ethers.parseEther("0.001"), + gasPrice + }); + console.log(`Gas estimate: ${gasEstimate.toString()}`); + } catch (error) { + console.error(`Error: ${error.message}`); + } + + // Test 9: Direct RPC call to eth_estimateGas with unfunded account + try { + console.log("\nTest 9: Direct RPC call to eth_estimateGas with unfunded account"); + const result = await ethers.provider.send("eth_estimateGas", [ + { + from: unfundedWallet.address, + to: fundedWallet.address, + value: ethers.parseEther("0.001").toString(), + gasPrice: gasPrice.toString() + } + ]); + console.log(`Result: ${result}`); + } catch (error) { + console.log("Error object structure:"); + console.log(JSON.stringify(error, null, 2)); + console.error(`Error: ${error.message}`); + + // If there's an error.data field, it might contain the revert reason + if (error.data) { + console.error(`Error data: ${error.data}`); + } + } + + // Test 10: Direct RPC call to eth_call with unfunded account + try { + console.log("\nTest 10: Direct RPC call to eth_call with unfunded account"); + const result = await ethers.provider.send("eth_call", [ + { + from: unfundedWallet.address, + to: fundedWallet.address, + value: ethers.parseEther("0.001").toString(), + gasPrice: gasPrice.toString() + }, + "latest" + ]); + console.log(`Result: ${result}`); + } catch (error) { + console.log("Error object structure:"); + console.log(JSON.stringify(error, null, 2)); + console.error(`Error: ${error.message}`); + + // If there's an error.data field, it might contain the revert reason + if (error.data) { + console.error(`Error data: ${error.data}`); + } + } +} + +// Run the script +main() + .then(() => process.exit(0)) + .catch((error) => { + console.error(error); + process.exit(1); + }); diff --git a/go/enclave/rpc/EstimateGas.go b/go/enclave/rpc/EstimateGas.go index 39be2dedf..c93f3087f 100644 --- a/go/enclave/rpc/EstimateGas.go +++ b/go/enclave/rpc/EstimateGas.go @@ -58,31 +58,33 @@ func EstimateGasValidate(reqParams []any, builder *CallBuilder[CallParamsWithBlo // EstimateGasExecute - performs the gas estimation based on the provided parameters and the local environment configuration. // Will accommodate l1 gas cost and stretch the final gas estimation. +// Note that setting gas price on the external call affects behaviour - this is due to a change geth implemented; If the account has +// no balance with the gas price the estimation might zero out. func EstimateGasExecute(builder *CallBuilder[CallParamsWithBlock, hexutil.Uint64], rpc *EncryptionManager) error { err := authenticateFrom(builder.VK, builder.From) if err != nil { builder.Err = err - return nil //nolint:nilerr + return nil } txArgs := builder.Param.callParams blockNumber := builder.Param.block block, err := rpc.l1BlockProcessor.GetHead(builder.ctx) if err != nil { - return err + return fmt.Errorf("failed to get head block from L1: %w", err) } headBatchSeq := rpc.registry.HeadBatchSeq() batch, err := rpc.storage.FetchBatchHeaderBySeqNo(builder.ctx, headBatchSeq.Uint64()) if err != nil { - return err + return fmt.Errorf("failed to fetch batch header: %w", err) } // The message is run through the l1 publishing cost estimation for the current // known head BlockHeader. l1Cost, err := rpc.gasOracle.EstimateL1CostForMsg(txArgs, block, batch) if err != nil { - return err + return fmt.Errorf("failed to estimate L1 cost: %w", err) } // We divide the total estimated l1 cost by the l2 fee per gas in order to convert @@ -100,7 +102,11 @@ func EstimateGasExecute(builder *CallBuilder[CallParamsWithBlock, hexutil.Uint64 // Notice that unfortunately, some slots might ve considered warm, which skews the estimation. // The single pass will run once at the highest gas cap and return gas used. Not completely reliable, // but is quick. - executionGasEstimate, revert, gasPrice, userErr, err := estimateGasSinglePass(builder.ctx, rpc, txArgs, blockNumber, rpc.config.GasLocalExecutionCapFlag) + executionGasEstimate, revert, gasPrice, userErr, sysErr := estimateGasSinglePass(builder.ctx, rpc, txArgs, blockNumber, rpc.config.GasLocalExecutionCapFlag) + if sysErr != nil { + return fmt.Errorf("system error during gas estimation: %w", sysErr) + } + if len(revert) > 0 { builder.Err = newRevertError(revert) rpc.logger.Debug("revert error", "err", builder.Err) @@ -112,20 +118,15 @@ func EstimateGasExecute(builder *CallBuilder[CallParamsWithBlock, hexutil.Uint64 return nil } - // if there was a system error return it - if err != nil { - return err - } - totalGasEstimateUint64 := publishingGas.Uint64() + uint64(executionGasEstimate) totalGasEstimate := hexutil.Uint64(totalGasEstimateUint64) balance, err := rpc.chain.GetBalanceAtBlock(builder.ctx, *txArgs.From, blockNumber) if err != nil { - return err + return fmt.Errorf("failed to get account balance: %w", err) } if balance.ToInt().Cmp(big.NewInt(0).Mul(gasPrice, big.NewInt(0).SetUint64(totalGasEstimateUint64))) < 0 { - builder.Err = fmt.Errorf("insufficient funds for gas estimate") + builder.Err = errors.New("insufficient funds for gas estimate") return nil } rpc.logger.Debug("Estimation breakdown", "gasPrice", gasPrice, "executionGasEstimate", uint64(executionGasEstimate), "publishingGas", publishingGas, "totalGasEstimate", uint64(totalGasEstimate)) @@ -133,12 +134,11 @@ func EstimateGasExecute(builder *CallBuilder[CallParamsWithBlock, hexutil.Uint64 return nil } -func calculateMaxGasCap(ctx context.Context, rpc *EncryptionManager, gasCap uint64, argsGas *hexutil.Uint64) uint64 { +func calculateMaxGasCap(ctx context.Context, rpc *EncryptionManager, gasCap uint64, argsGas *hexutil.Uint64) (uint64, error) { // Fetch the current batch header to get the batch gas limit batchHeader, err := rpc.storage.FetchHeadBatchHeader(ctx) if err != nil { - rpc.logger.Error("Failed to fetch batch header", "error", err) - return gasCap + return 0, fmt.Errorf("failed to fetch batch header: %w", err) } // Determine the gas limit based on the batch header @@ -160,7 +160,7 @@ func calculateMaxGasCap(ctx context.Context, rpc *EncryptionManager, gasCap uint } } - return gasCap + return gasCap, nil } // This adds a bit of an overhead to gas estimation. Fixes issues when calling proxies, but needs more investigation. @@ -196,19 +196,27 @@ func calculateProxyOverhead(txArgs *gethapi.TransactionArgs) uint64 { // for the head batch might not be fully clean in terms of the running call. Cold storage slots cost far more than warm ones to // read and write. func estimateGasSinglePass(ctx context.Context, rpc *EncryptionManager, args *gethapi.TransactionArgs, blkNumber *gethrpc.BlockNumber, globalGasCap uint64) (hexutil.Uint64, []byte, *big.Int, error, common.SystemError) { - maxGasCap := calculateMaxGasCap(ctx, rpc, globalGasCap, args.Gas) - // allowance will either be the maxGasCap or the balance allowance. - // If the users funds are floaty, this might cause issues combined with the l1 pricing. - allowance, feeCap, err := normalizeFeeCapAndAdjustGasLimit(ctx, rpc, args, blkNumber, maxGasCap) + maxGasCap, err := calculateMaxGasCap(ctx, rpc, globalGasCap, args.Gas) if err != nil { return 0, nil, nil, nil, err } + // allowance will either be the maxGasCap or the balance allowance. + // If the users funds are floaty, this might cause issues combined with the l1 pricing. + allowance, feeCap, userErr, sysErr := normalizeFeeCapAndAdjustGasLimit(ctx, rpc, args, blkNumber, maxGasCap) + if sysErr != nil { + return 0, nil, nil, nil, sysErr + } + + if userErr != nil { + return 0, nil, nil, userErr, nil + } + // Perform a single gas estimation pass using isGasEnough - failed, result, userErr, err := isGasEnough(ctx, rpc, args, allowance, blkNumber) - if err != nil { + failed, result, userErr, sysErr := isGasEnough(ctx, rpc, args, allowance, blkNumber) + if sysErr != nil { // Return zero values and the encountered error if estimation fails - return 0, nil, nil, nil, err + return 0, nil, nil, nil, sysErr } if failed { @@ -242,11 +250,11 @@ func estimateGasSinglePass(ctx context.Context, rpc *EncryptionManager, args *ge return gasUsed, nil, feeCap, nil, nil } -func normalizeFeeCapAndAdjustGasLimit(ctx context.Context, rpc *EncryptionManager, args *gethapi.TransactionArgs, blkNumber *gethrpc.BlockNumber, hi uint64) (uint64, *big.Int, error) { +func normalizeFeeCapAndAdjustGasLimit(ctx context.Context, rpc *EncryptionManager, args *gethapi.TransactionArgs, blkNumber *gethrpc.BlockNumber, hi uint64) (uint64, *big.Int, error, common.SystemError) { // Normalize the max fee per gas the call is willing to spend. var feeCap *big.Int if args.GasPrice != nil && (args.MaxFeePerGas != nil || args.MaxPriorityFeePerGas != nil) { - return 0, gethcommon.Big0, errors.New("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified") + return 0, gethcommon.Big0, errors.New("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified"), nil } else if args.GasPrice != nil { feeCap = args.GasPrice.ToInt() } else if args.MaxFeePerGas != nil { @@ -259,13 +267,13 @@ func normalizeFeeCapAndAdjustGasLimit(ctx context.Context, rpc *EncryptionManage if feeCap.BitLen() != 0 { //nolint:nestif balance, err := rpc.chain.GetBalanceAtBlock(ctx, *args.From, blkNumber) if err != nil { - return 0, gethcommon.Big0, fmt.Errorf("unable to fetch account balance - %w", err) + return 0, gethcommon.Big0, nil, fmt.Errorf("unable to fetch account balance: %w", err) } available := new(big.Int).Set(balance.ToInt()) if args.Value != nil { if args.Value.ToInt().Cmp(available) >= 0 { - return 0, gethcommon.Big0, errors.New("insufficient funds for transfer") + return 0, gethcommon.Big0, errors.New("insufficient funds for transfer"), nil } available.Sub(available, args.Value.ToInt()) } @@ -287,8 +295,7 @@ func normalizeFeeCapAndAdjustGasLimit(ctx context.Context, rpc *EncryptionManage hi = allowance.Uint64() } } - - return hi, feeCap, nil + return hi, feeCap, nil, nil } // Create a helper to check if a gas allowance results in an executable transaction @@ -297,9 +304,13 @@ func isGasEnough(ctx context.Context, rpc *EncryptionManager, args *gethapi.Tran defer core.LogMethodDuration(rpc.logger, measure.NewStopwatch(), "enclave.go:IsGasEnough") args.Gas = (*hexutil.Uint64)(&gas) result, userErr, sysErr := rpc.chain.ObsCallAtBlock(ctx, args, blkNumber) - if sysErr != nil || userErr != nil { - // since we estimate gas in a single pass, any error is just returned - return true, nil, userErr, sysErr // Bail out + if sysErr != nil { + return true, nil, nil, sysErr } + + if userErr != nil { + return true, nil, userErr, nil + } + return result.Failed(), result, nil, nil } diff --git a/go/enclave/rpc/TenEthCall.go b/go/enclave/rpc/TenEthCall.go index 605f6aaa1..a4b0c918b 100644 --- a/go/enclave/rpc/TenEthCall.go +++ b/go/enclave/rpc/TenEthCall.go @@ -39,6 +39,9 @@ func TenCallValidate(reqParams []any, builder *CallBuilder[CallParamsWithBlock, return nil } +// TenCallExecute - executes the eth_call call according to the new geth specifications; Balance is checked and gas price affects behaviour. +// When gas price is zero the sender does not need to be funded as gas would be free, but otherwise eth call would accurately represent what a transaction +// would be doing and potentially fail due to insufficient balance. func TenCallExecute(builder *CallBuilder[CallParamsWithBlock, string], rpc *EncryptionManager) error { err := authenticateFrom(builder.VK, builder.From) if err != nil {