From b06b000186e6bb84bd97a7fae2d8764ee51cd338 Mon Sep 17 00:00:00 2001 From: Goran Rojovic Date: Thu, 19 Oct 2023 12:21:04 +0200 Subject: [PATCH 1/3] Fix --- jsonrpc/eth_endpoint.go | 38 +++++++++++++++------------------ jsonrpc/eth_state_test.go | 44 +++++++++++++++++++++++++++++++++------ types/transaction.go | 8 +++++++ 3 files changed, 63 insertions(+), 27 deletions(-) diff --git a/jsonrpc/eth_endpoint.go b/jsonrpc/eth_endpoint.go index ece4f03c38..79002409b9 100644 --- a/jsonrpc/eth_endpoint.go +++ b/jsonrpc/eth_endpoint.go @@ -551,23 +551,28 @@ func (e *Eth) EstimateGas(arg *txnArgs, rawNum *BlockNumber) (interface{}, error return nil, err } + forksInTime := e.store.GetForksInTime(header.Number) + + if transaction.IsValueTransfer() || transaction.IsContractCreation() { + // if it is a simple value transfer or a contract creation, + // we already know what is the transaction gas cost, no need to apply transaction + gasCost, err := state.TransactionGasCost(transaction, forksInTime.Homestead, forksInTime.Istanbul) + if err != nil { + return nil, err + } + + return argUint64(gasCost), nil + } + // Force transaction gas price if empty if err = e.fillTransactionGasPrice(transaction); err != nil { return nil, err } - forksInTime := e.store.GetForksInTime(header.Number) - - var standardGas uint64 - if transaction.IsContractCreation() && forksInTime.Homestead { - standardGas = state.TxGasContractCreation - } else { - standardGas = state.TxGas - } - var ( - lowEnd = standardGas - highEnd uint64 + standardGas = state.TxGas + lowEnd = standardGas + highEnd uint64 ) // If the gas limit was passed in, use it as a ceiling @@ -579,7 +584,6 @@ func (e *Eth) EstimateGas(arg *txnArgs, rawNum *BlockNumber) (interface{}, error } gasPriceInt := new(big.Int).Set(transaction.GasPrice) - valueInt := new(big.Int).Set(transaction.Value) var availableBalance *big.Int @@ -602,14 +606,6 @@ func (e *Eth) EstimateGas(arg *txnArgs, rawNum *BlockNumber) (interface{}, error } availableBalance = new(big.Int).Set(accountBalance) - - if transaction.Value != nil { - if valueInt.Cmp(availableBalance) > 0 { - return 0, ErrInsufficientFunds - } - - availableBalance.Sub(availableBalance, valueInt) - } } // Recalculate the gas ceiling based on the available funds (if any) @@ -663,7 +659,7 @@ func (e *Eth) EstimateGas(arg *txnArgs, rawNum *BlockNumber) (interface{}, error transaction.Gas = gas - result, applyErr := e.store.ApplyTxn(header, transaction, nil, false) + result, applyErr := e.store.ApplyTxn(header, transaction, nil, true) if result != nil { data = []byte(hex.EncodeToString(result.ReturnValue)) diff --git a/jsonrpc/eth_state_test.go b/jsonrpc/eth_state_test.go index a027ad8507..7c6f81aab6 100644 --- a/jsonrpc/eth_state_test.go +++ b/jsonrpc/eth_state_test.go @@ -772,7 +772,7 @@ func TestEth_EstimateGas_Reverts(t *testing.T) { } } -func TestEth_EstimateGas_Errors(t *testing.T) { +func TestEth_EstimateGas_ValueTransfer(t *testing.T) { store := getExampleStore() ethEndpoint := newTestEthEndpoint(store) @@ -780,19 +780,51 @@ func TestEth_EstimateGas_Errors(t *testing.T) { store.account.account.Balance = big.NewInt(0) // The transaction has a value > 0 + from := types.StringToAddress("0xSenderAddress") + to := types.StringToAddress("0xReceiverAddress") mockTx := constructMockTx(nil, nil) mockTx.Value = argBytesPtr([]byte{0x1}) + mockTx.From = &from + mockTx.To = &to // Run the estimation - estimate, estimateErr := ethEndpoint.EstimateGas( + estimate, err := ethEndpoint.EstimateGas( mockTx, nil, ) - assert.Equal(t, 0, estimate) + assert.NotNil(t, estimate) + estimateUint64, ok := estimate.(argUint64) + assert.True(t, ok) + assert.NoError(t, err) + assert.Equal(t, state.TxGas, uint64(estimateUint64)) // simple value transfers are 21000wei always +} + +func TestEth_EstimateGas_ContractCreation(t *testing.T) { + store := getExampleStore() + ethEndpoint := newTestEthEndpoint(store) + + // Account doesn't have any balance + store.account.account.Balance = big.NewInt(0) + + // The transaction has a value > 0 + from := types.StringToAddress("0xSenderAddress") + mockTx := constructMockTx(nil, nil) + mockTx.From = &from + mockTx.Input = argBytesPtr([]byte{}) + mockTx.To = nil + + // Run the estimation + estimate, err := ethEndpoint.EstimateGas( + mockTx, + nil, + ) - // Make sure the insufficient funds error message is contained - assert.ErrorIs(t, estimateErr, ErrInsufficientFunds) + assert.NotNil(t, estimate) + estimateUint64, ok := estimate.(argUint64) + assert.True(t, ok) + assert.NoError(t, err) + assert.Equal(t, state.TxGasContractCreation, uint64(estimateUint64)) } type mockSpecialStore struct { @@ -867,7 +899,7 @@ func (m *mockSpecialStore) GetCode(root types.Hash, addr types.Address) ([]byte, } func (m *mockSpecialStore) GetForksInTime(blockNumber uint64) chain.ForksInTime { - return chain.ForksInTime{} + return chain.AllForksEnabled.At(0) } func (m *mockSpecialStore) ApplyTxn(header *types.Header, txn *types.Transaction, _ types.StateOverride, _ bool) (*runtime.ExecutionResult, error) { diff --git a/types/transaction.go b/types/transaction.go index f19a79b9ee..53f8380ac8 100644 --- a/types/transaction.go +++ b/types/transaction.go @@ -74,6 +74,14 @@ func (t *Transaction) IsContractCreation() bool { return t.To == nil } +// IsValueTransfer checks if tx is a value transfer +func (t *Transaction) IsValueTransfer() bool { + return t.Value != nil && + t.Value.Sign() != 0 && + len(t.Input) == 0 && + !t.IsContractCreation() +} + // ComputeHash computes the hash of the transaction func (t *Transaction) ComputeHash(blockNumber uint64) *Transaction { GetTransactionHashHandler(blockNumber).ComputeHash(t) From 1063ef9252b0d0356de813b981cc849d061cb055 Mon Sep 17 00:00:00 2001 From: Goran Rojovic Date: Thu, 19 Oct 2023 12:54:46 +0200 Subject: [PATCH 2/3] Remove unnecessary variable --- jsonrpc/eth_endpoint.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/jsonrpc/eth_endpoint.go b/jsonrpc/eth_endpoint.go index 79002409b9..c035683722 100644 --- a/jsonrpc/eth_endpoint.go +++ b/jsonrpc/eth_endpoint.go @@ -570,13 +570,12 @@ func (e *Eth) EstimateGas(arg *txnArgs, rawNum *BlockNumber) (interface{}, error } var ( - standardGas = state.TxGas - lowEnd = standardGas - highEnd uint64 + lowEnd = state.TxGas + highEnd uint64 ) // If the gas limit was passed in, use it as a ceiling - if transaction.Gas != 0 && transaction.Gas >= standardGas { + if transaction.Gas != 0 && transaction.Gas >= lowEnd { highEnd = transaction.Gas } else { // If not, use the referenced block number From 8ac42d4926cc6c1feb5830c0040bec8dfd264d59 Mon Sep 17 00:00:00 2001 From: Goran Rojovic Date: Thu, 19 Oct 2023 14:35:05 +0200 Subject: [PATCH 3/3] Add new e2e test --- e2e-polybft/e2e/jsonrpc_test.go | 35 +++++++++++++++++++++++++++++++++ jsonrpc/eth_endpoint.go | 13 +++++++++--- types/transaction.go | 2 +- 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/e2e-polybft/e2e/jsonrpc_test.go b/e2e-polybft/e2e/jsonrpc_test.go index 4ece515fce..6735904dec 100644 --- a/e2e-polybft/e2e/jsonrpc_test.go +++ b/e2e-polybft/e2e/jsonrpc_test.go @@ -13,6 +13,7 @@ import ( "github.com/0xPolygon/polygon-edge/consensus/polybft/contractsapi" "github.com/0xPolygon/polygon-edge/e2e-polybft/framework" "github.com/0xPolygon/polygon-edge/helper/hex" + "github.com/0xPolygon/polygon-edge/state" "github.com/0xPolygon/polygon-edge/types" ) @@ -126,6 +127,40 @@ func TestE2E_JsonRPC(t *testing.T) { require.Equal(t, uint64(0x56a3), resp) }) + t.Run("eth_estimateGas by zero-balance account - simple value transfer", func(t *testing.T) { + acctZeroBalance, err := wallet.GenerateKey() + require.NoError(t, err) + + fundedAccountAddress := acct.Address() + nonFundedAccountAddress := acctZeroBalance.Address() + + estimateGasFn := func(value *big.Int) { + resp, err := client.EstimateGas(ðgo.CallMsg{ + From: nonFundedAccountAddress, + To: &fundedAccountAddress, + Value: value, + }) + + require.NoError(t, err) + require.Equal(t, state.TxGas, resp) + } + + estimateGasFn(ethgo.Gwei(1)) + + // transfer some funds to zero balance account + valueTransferTxn := cluster.SendTxn(t, acct, ðgo.Transaction{ + From: fundedAccountAddress, + To: &nonFundedAccountAddress, + Value: ethgo.Gwei(10), + }) + + require.NoError(t, valueTransferTxn.Wait()) + require.True(t, valueTransferTxn.Succeed()) + + // now call estimate gas again for the now funded account + estimateGasFn(ethgo.Gwei(1)) + }) + t.Run("eth_getBalance", func(t *testing.T) { key1, err := wallet.GenerateKey() require.NoError(t, err) diff --git a/jsonrpc/eth_endpoint.go b/jsonrpc/eth_endpoint.go index c035683722..455d5d94ba 100644 --- a/jsonrpc/eth_endpoint.go +++ b/jsonrpc/eth_endpoint.go @@ -553,7 +553,7 @@ func (e *Eth) EstimateGas(arg *txnArgs, rawNum *BlockNumber) (interface{}, error forksInTime := e.store.GetForksInTime(header.Number) - if transaction.IsValueTransfer() || transaction.IsContractCreation() { + if transaction.IsValueTransfer() { // if it is a simple value transfer or a contract creation, // we already know what is the transaction gas cost, no need to apply transaction gasCost, err := state.TransactionGasCost(transaction, forksInTime.Homestead, forksInTime.Istanbul) @@ -569,13 +569,20 @@ func (e *Eth) EstimateGas(arg *txnArgs, rawNum *BlockNumber) (interface{}, error return nil, err } + var standardGas uint64 + if transaction.IsContractCreation() && forksInTime.Homestead { + standardGas = state.TxGasContractCreation + } else { + standardGas = state.TxGas + } + var ( - lowEnd = state.TxGas + lowEnd = standardGas highEnd uint64 ) // If the gas limit was passed in, use it as a ceiling - if transaction.Gas != 0 && transaction.Gas >= lowEnd { + if transaction.Gas != 0 && transaction.Gas >= standardGas { highEnd = transaction.Gas } else { // If not, use the referenced block number diff --git a/types/transaction.go b/types/transaction.go index 53f8380ac8..87dbea350a 100644 --- a/types/transaction.go +++ b/types/transaction.go @@ -77,7 +77,7 @@ func (t *Transaction) IsContractCreation() bool { // IsValueTransfer checks if tx is a value transfer func (t *Transaction) IsValueTransfer() bool { return t.Value != nil && - t.Value.Sign() != 0 && + t.Value.Sign() > 0 && len(t.Input) == 0 && !t.IsContractCreation() }