review Estimate Gas validations and Formulas (#2856)

jsonrpc/endpoints_eth.go

@@ -24,11 +24,6 @@ import (
 )
 
 const (
-	// DefaultSenderAddress is the address that jRPC will use
-	// to communicate with the state for eth_EstimateGas and eth_Call when
-	// the From field is not specified because it is optional
-	DefaultSenderAddress = "0x1111111111111111111111111111111111111111"
-
 	// maxTopics is the max number of topics a log can have
 	maxTopics = 4
 )
@@ -101,7 +96,7 @@ func (e *EthEndpoints) Call(arg *types.TxArgs, blockArg *types.BlockNumberOrHash
 			arg.Gas = &gas
 		}
 
-		defaultSenderAddress := common.HexToAddress(DefaultSenderAddress)
+		defaultSenderAddress := common.HexToAddress(state.DefaultSenderAddress)
 		sender, tx, err := arg.ToTransaction(ctx, e.state, e.cfg.MaxCumulativeGasUsed, block.Root(), defaultSenderAddress, dbTx)
 		if err != nil {
 			return RPCErrorResponse(types.DefaultErrorCode, "failed to convert arguments into an unsigned transaction", err, false)
@@ -185,7 +180,7 @@ func (e *EthEndpoints) EstimateGas(arg *types.TxArgs, blockArg *types.BlockNumbe
 			}
 		}
 
-		defaultSenderAddress := common.HexToAddress(DefaultSenderAddress)
+		defaultSenderAddress := common.HexToAddress(state.DefaultSenderAddress)
 		sender, tx, err := arg.ToTransaction(ctx, e.state, e.cfg.MaxCumulativeGasUsed, block.Root(), defaultSenderAddress, dbTx)
 		if err != nil {
 			return RPCErrorResponse(types.DefaultErrorCode, "failed to convert arguments into an unsigned transaction", err, false)

jsonrpc/endpoints_eth_test.go

@@ -378,7 +378,7 @@ func TestCall(t *testing.T) {
 				block := state.NewL2BlockWithHeader(state.NewL2Header(&ethTypes.Header{Number: blockNumOne, Root: blockRoot}))
 				m.State.On("GetL2BlockByNumber", context.Background(), blockNumOneUint64, m.DbTx).Return(block, nil).Once()
 				m.State.
-					On("ProcessUnsignedTransaction", context.Background(), txMatchBy, common.HexToAddress(DefaultSenderAddress), nilUint64, true, m.DbTx).
+					On("ProcessUnsignedTransaction", context.Background(), txMatchBy, common.HexToAddress(state.DefaultSenderAddress), nilUint64, true, m.DbTx).
 					Return(&runtime.ExecutionResult{ReturnValue: testCase.expectedResult}, nil).
 					Once()
 			},
@@ -417,7 +417,7 @@ func TestCall(t *testing.T) {
 				block := state.NewL2BlockWithHeader(state.NewL2Header(&ethTypes.Header{Number: blockNumOne, Root: blockRoot}))
 				m.State.On("GetL2BlockByNumber", context.Background(), blockNumOneUint64, m.DbTx).Return(block, nil).Once()
 				m.State.
-					On("ProcessUnsignedTransaction", context.Background(), txMatchBy, common.HexToAddress(DefaultSenderAddress), nilUint64, true, m.DbTx).
+					On("ProcessUnsignedTransaction", context.Background(), txMatchBy, common.HexToAddress(state.DefaultSenderAddress), nilUint64, true, m.DbTx).
 					Return(&runtime.ExecutionResult{ReturnValue: testCase.expectedResult}, nil).
 					Once()
 			},
@@ -726,7 +726,7 @@ func TestEstimateGas(t *testing.T) {
 				m.State.On("GetLastL2Block", context.Background(), m.DbTx).Return(block, nil).Once()
 
 				m.State.
-					On("EstimateGas", txMatchBy, common.HexToAddress(DefaultSenderAddress), nilUint64, m.DbTx).
+					On("EstimateGas", txMatchBy, common.HexToAddress(state.DefaultSenderAddress), nilUint64, m.DbTx).
 					Return(*testCase.expectedResult, nil, nil).
 					Once()
 			},

state/errors.go

@@ -43,9 +43,9 @@ var (
 	// ongoing batch are not in the same order as the transactions stored in the
 	// database for the same batch.
 	ErrOutOfOrderProcessedTx = errors.New("the processed transactions are not in the same order as the stored transactions")
-	// ErrInsufficientFunds is returned if the total cost of executing a transaction
-	// is higher than the balance of the user's account.
-	ErrInsufficientFunds = errors.New("insufficient funds for gas * price + value")
+	// ErrInsufficientFundsForTransfer is returned if the transaction sender doesn't
+	// have enough funds for transfer(topmost call only).
+	ErrInsufficientFundsForTransfer = errors.New("insufficient funds for transfer")
 	// ErrExecutorNil indicates that the method requires an executor that is not nil
 	ErrExecutorNil = errors.New("the method requires an executor that is not nil")
 	// ErrStateTreeNil indicates that the method requires a state tree that is not nil

state/state.go

@@ -18,6 +18,10 @@ import (
 const newL2BlockEventBufferSize = 500
 
 var (
+	// DefaultSenderAddress is the address that jRPC will use
+	// to communicate with the state for eth_EstimateGas and eth_Call when
+	// the From field is not specified because it is optional
+	DefaultSenderAddress = "0x1111111111111111111111111111111111111111"
 	// ZeroHash is the hash 0x0000000000000000000000000000000000000000000000000000000000000000
 	ZeroHash = common.Hash{}
 	// ZeroAddress is the address 0x0000000000000000000000000000000000000000

state/transaction.go

@@ -588,9 +588,6 @@ func CheckSupersetBatchTransactions(existingTxHashes []common.Hash, processedTxs
 func (s *State) EstimateGas(transaction *types.Transaction, senderAddress common.Address, l2BlockNumber *uint64, dbTx pgx.Tx) (uint64, []byte, error) {
 	const ethTransferGas = 21000
 
-	var lowEnd uint64
-	var highEnd uint64
-
 	ctx := context.Background()
 
 	lastBatches, l2BlockStateRoot, err := s.GetLastNBatchesByL2BlockNumber(ctx, l2BlockNumber, 2, dbTx) // nolint:gomnd
@@ -622,63 +619,68 @@ func (s *State) EstimateGas(transaction *types.Transaction, senderAddress common
 		previousBatch = lastBatches[1]
 	}
 
-	lowEnd, err = core.IntrinsicGas(transaction.Data(), transaction.AccessList(), s.isContractCreation(transaction), true, false, false)
-	if err != nil {
-		return 0, nil, err
-	}
-
-	if lowEnd == ethTransferGas && transaction.To() != nil {
-		code, err := s.tree.GetCode(ctx, *transaction.To(), stateRoot.Bytes())
-		if err != nil {
-			log.Warnf("error while getting transaction.to() code %v", err)
-		} else if len(code) == 0 {
-			return lowEnd, nil, nil
-		}
-	}
+	highEnd := s.cfg.MaxCumulativeGasUsed
 
-	if transaction.Gas() != 0 && transaction.Gas() > lowEnd {
-		highEnd = transaction.Gas()
-	} else {
-		highEnd = s.cfg.MaxCumulativeGasUsed
-	}
-
-	var availableBalance *big.Int
-
-	if senderAddress != ZeroAddress {
+	// if gas price is set, set the highEnd to the max amount
+	// of the account afford
+	isGasPriceSet := transaction.GasPrice().BitLen() != 0
+	if isGasPriceSet {
 		senderBalance, err := s.tree.GetBalance(ctx, senderAddress, stateRoot.Bytes())
-		if err != nil {
-			if errors.Is(err, ErrNotFound) {
-				senderBalance = big.NewInt(0)
-			} else {
-				return 0, nil, err
-			}
+		if errors.Is(err, ErrNotFound) {
+			senderBalance = big.NewInt(0)
+		} else if err != nil {
+			return 0, nil, err
 		}
 
-		availableBalance = new(big.Int).Set(senderBalance)
-
+		availableBalance := new(big.Int).Set(senderBalance)
+		// check if the account has funds to pay the transfer value
 		if transaction.Value() != nil {
 			if transaction.Value().Cmp(availableBalance) > 0 {
-				return 0, nil, ErrInsufficientFunds
+				return 0, nil, ErrInsufficientFundsForTransfer
 			}
 
+			// deduct the value from the available balance
 			availableBalance.Sub(availableBalance, transaction.Value())
 		}
-	}
-
-	if transaction.GasPrice().BitLen() != 0 && // Gas price has been set
-		availableBalance != nil && // Available balance is found
-		availableBalance.Cmp(big.NewInt(0)) > 0 { // Available balance > 0
-		gasAllowance := new(big.Int).Div(availableBalance, transaction.GasPrice())
 
 		// Check the gas allowance for this account, make sure high end is capped to it
+		gasAllowance := new(big.Int).Div(availableBalance, transaction.GasPrice())
 		if gasAllowance.IsUint64() && highEnd > gasAllowance.Uint64() {
 			log.Debugf("Gas estimation high-end capped by allowance [%d]", gasAllowance.Uint64())
 			highEnd = gasAllowance.Uint64()
 		}
 	}
 
-	// Run the transaction with the specified gas value.
-	// Returns a status indicating if the transaction failed, if it was reverted and the accompanying error
+	// if the tx gas is set and it is smaller than the highEnd,
+	// limit the highEnd to the maximum allowed by the tx gas
+	if transaction.Gas() != 0 && transaction.Gas() < highEnd {
+		highEnd = transaction.Gas()
+	}
+
+	// set start values for lowEnd and highEnd:
+	lowEnd, err := core.IntrinsicGas(transaction.Data(), transaction.AccessList(), s.isContractCreation(transaction), true, false, false)
+	if err != nil {
+		return 0, nil, err
+	}
+
+	// if the intrinsic gas is the same as the constant value for eth transfer
+	// and the transaction has a receiver address
+	if lowEnd == ethTransferGas && transaction.To() != nil {
+		receiver := *transaction.To()
+		// check if the receiver address is not a smart contract
+		code, err := s.tree.GetCode(ctx, receiver, stateRoot.Bytes())
+		if err != nil {
+			log.Warnf("error while getting code for address %v: %v", receiver.String(), err)
+		} else if len(code) == 0 {
+			// in case it is just an account, we can avoid the execution and return
+			// the transfer constant amount
+			return lowEnd, nil, nil
+		}
+	}
+
+	// testTransaction runs the transaction with the specified gas value.
+	// it returns a status indicating if the transaction has failed, if it
+	// was reverted and the accompanying error
 	testTransaction := func(gas uint64, nonce uint64, shouldOmitErr bool) (failed, reverted bool, gasUsed uint64, returnValue []byte, err error) {
 		tx := types.NewTx(&types.LegacyTx{
 			Nonce:    nonce,
@@ -767,22 +769,23 @@ func (s *State) EstimateGas(transaction *types.Transaction, senderAddress common
 	txExecutions := []time.Duration{}
 	var totalExecutionTime time.Duration
 
-	// Check if the highEnd is a good value to make the transaction pass
-	failed, reverted, gasUsed, returnValue, err := testTransaction(highEnd, nonce, false)
+	// Check if the highEnd is a good value to make the transaction pass, if it fails we
+	// can return immediately.
 	log.Debugf("Estimate gas. Trying to execute TX with %v gas", highEnd)
+	failed, reverted, gasUsed, returnValue, err := testTransaction(highEnd, nonce, false)
 	if failed {
 		if reverted {
 			return 0, returnValue, err
 		}
 
 		// The transaction shouldn't fail, for whatever reason, at highEnd
 		return 0, nil, fmt.Errorf(
-			"unable to apply transaction even for the highest gas limit %d: %w",
+			"gas required exceeds allowance (%d)",
 			highEnd,
-			err,
 		)
 	}
 
+	// sets
 	if lowEnd < gasUsed {
 		lowEnd = gasUsed
 	}
@@ -791,9 +794,14 @@ func (s *State) EstimateGas(transaction *types.Transaction, senderAddress common
 	for (lowEnd < highEnd) && (highEnd-lowEnd) > 4096 {
 		txExecutionStart := time.Now()
 		mid := (lowEnd + highEnd) / 2 // nolint:gomnd
+		if mid > lowEnd*2 {
+			// Most txs don't need much higher gas limit than their gas used, and most txs don't
+			// require near the full block limit of gas, so the selection of where to bisect the
+			// range here is skewed to favor the low side.
+			mid = lowEnd * 2 // nolint:gomnd
+		}
 
 		log.Debugf("Estimate gas. Trying to execute TX with %v gas", mid)
-
 		failed, reverted, _, _, testErr := testTransaction(mid, nonce, true)
 		executionTime := time.Since(txExecutionStart)
 		totalExecutionTime += executionTime

test/benchmarks/sequencer/common/transactions/transactions.go

@@ -113,5 +113,5 @@ func WaitStatusSelected(countByStatusFunc func(ctx context.Context, status ...po
 }
 
 func ShouldRetryError(err error) bool {
-	return errors.Is(err, state.ErrStateNotSynchronized) || errors.Is(err, state.ErrInsufficientFunds) || errors.Is(err, pool.ErrNonceTooHigh)
+	return errors.Is(err, state.ErrStateNotSynchronized) || errors.Is(err, state.ErrInsufficientFundsForTransfer) || errors.Is(err, pool.ErrNonceTooHigh)
 }