Merge pull request #550 from hermeznetwork/fix/txselectornoncessorting2

Fix TxSel sorting, TxManager geth err checking

coordinator/txmanager.go

@@ -2,9 +2,9 @@ package coordinator
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"math/big"
+	"strings"
 	"time"
 
 	"github.com/ethereum/go-ethereum"
@@ -206,22 +206,27 @@ func (t *TxManager) sendRollupForgeBatch(ctx context.Context, batchInfo *BatchIn
 		}
 		// RollupForgeBatch() calls ethclient.SendTransaction()
 		ethTx, err = t.ethClient.RollupForgeBatch(batchInfo.ForgeBatchArgs, auth)
-		if errors.Is(err, core.ErrNonceTooLow) {
+		// We check the errors via strings because we match the
+		// definition of the error from geth, with the string returned
+		// via RPC obtained by the client.
+		if err == nil {
+			break
+		} else if strings.Contains(err.Error(), core.ErrNonceTooLow.Error()) {
 			log.Warnw("TxManager ethClient.RollupForgeBatch incrementing nonce",
 				"err", err, "nonce", auth.Nonce, "batchNum", batchInfo.BatchNum)
 			auth.Nonce.Add(auth.Nonce, big.NewInt(1))
 			attempt--
-		} else if errors.Is(err, core.ErrNonceTooHigh) {
+		} else if strings.Contains(err.Error(), core.ErrNonceTooHigh.Error()) {
 			log.Warnw("TxManager ethClient.RollupForgeBatch decrementing nonce",
 				"err", err, "nonce", auth.Nonce, "batchNum", batchInfo.BatchNum)
 			auth.Nonce.Sub(auth.Nonce, big.NewInt(1))
 			attempt--
-		} else if errors.Is(err, core.ErrUnderpriced) {
+		} else if strings.Contains(err.Error(), core.ErrReplaceUnderpriced.Error()) {
 			log.Warnw("TxManager ethClient.RollupForgeBatch incrementing gasPrice",
 				"err", err, "gasPrice", auth.GasPrice, "batchNum", batchInfo.BatchNum)
 			auth.GasPrice = addPerc(auth.GasPrice, 10)
 			attempt--
-		} else if errors.Is(err, core.ErrReplaceUnderpriced) {
+		} else if strings.Contains(err.Error(), core.ErrUnderpriced.Error()) {
 			log.Warnw("TxManager ethClient.RollupForgeBatch incrementing gasPrice",
 				"err", err, "gasPrice", auth.GasPrice, "batchNum", batchInfo.BatchNum)
 			auth.GasPrice = addPerc(auth.GasPrice, 10)
@@ -230,8 +235,6 @@ func (t *TxManager) sendRollupForgeBatch(ctx context.Context, batchInfo *BatchIn
 			log.Errorw("TxManager ethClient.RollupForgeBatch",
 				"attempt", attempt, "err", err, "block", t.stats.Eth.LastBlock.Num+1,
 				"batchNum", batchInfo.BatchNum)
-		} else {
-			break
 		}
 		select {
 		case <-ctx.Done():

eth/ethereum.go

@@ -324,5 +324,6 @@ func (c *EthereumClient) EthCall(ctx context.Context, tx *types.Transaction,
 		Value:    tx.Value(),
 		Data:     tx.Data(),
 	}
-	return c.client.CallContract(ctx, msg, blockNum)
+	result, err := c.client.CallContract(ctx, msg, blockNum)
+	return result, tracerr.Wrap(err)
 }

eth/rollup.go

@@ -327,7 +327,7 @@ func (c *RollupClient) RollupForgeBatch(args *RollupForgeBatchArgs, auth *bind.T
 	if auth == nil {
 		auth, err = c.client.NewAuth()
 		if err != nil {
-			return nil, err
+			return nil, tracerr.Wrap(err)
 		}
 		auth.GasLimit = 1000000
 	}
@@ -393,7 +393,7 @@ func (c *RollupClient) RollupForgeBatch(args *RollupForgeBatchArgs, auth *bind.T
 		l1CoordinatorBytes, l1l2TxData, feeIdxCoordinator, args.VerifierIdx, args.L1Batch,
 		args.ProofA, args.ProofB, args.ProofC)
 	if err != nil {
-		return nil, tracerr.Wrap(fmt.Errorf("Failed Hermez.ForgeBatch: %w", err))
+		return nil, tracerr.Wrap(fmt.Errorf("Hermez.ForgeBatch: %w", err))
 	}
 	return tx, nil
 }

log/log.go

@@ -67,6 +67,11 @@ func Init(levelStr string, outputs []string) {
 
 func sprintStackTrace(st []tracerr.Frame) string {
 	builder := strings.Builder{}
+	// Skip deepest frame because it belongs to the go runtime and we don't
+	// care about it.
+	if len(st) > 0 {
+		st = st[:len(st)-1]
+	}
 	for _, f := range st {
 		builder.WriteString(fmt.Sprintf("\n%s:%d %s()", f.Path, f.Line, f.Func))
 	}

txselector/txselector.go

@@ -18,20 +18,6 @@ import (
 	"github.com/iden3/go-iden3-crypto/babyjub"
 )
 
-// txs implements the interface Sort for an array of Tx, and sorts the txs by
-// absolute fee
-type txs []common.PoolL2Tx
-
-func (t txs) Len() int {
-	return len(t)
-}
-func (t txs) Swap(i, j int) {
-	t[i], t[j] = t[j], t[i]
-}
-func (t txs) Less(i, j int) bool {
-	return t[i].AbsoluteFee > t[j].AbsoluteFee
-}
-
 // CoordAccount contains the data of the Coordinator account, that will be used
 // to create new transactions of CreateAccountDeposit type to add new TokenID
 // accounts for the Coordinator to receive the fees.
@@ -191,14 +177,16 @@ func (txsel *TxSelector) GetL1L2TxSelection(selectionConfig *SelectionConfig,
 	}
 
 	// discardedL2Txs contains an array of the L2Txs that have not been selected in this Batch
-	var discardedL2Txs []common.PoolL2Tx
 
 	var l1CoordinatorTxs []common.L1Tx
 	positionL1 := len(l1UserTxs)
 	var accAuths [][]byte
 
 	// sort l2TxsRaw (cropping at MaxTx at this point)
-	l2Txs0 := txsel.getL2Profitable(l2TxsRaw, selectionConfig.TxProcessorConfig.MaxTx)
+	l2Txs0, discardedL2Txs := txsel.getL2Profitable(l2TxsRaw, selectionConfig.TxProcessorConfig.MaxTx)
+	for i := range discardedL2Txs {
+		discardedL2Txs[i].Info = "Tx not selected due to low absolute fee"
+	}
 
 	noncesMap := make(map[common.Idx]common.Nonce)
 	var l2Txs []common.PoolL2Tx
@@ -590,11 +578,22 @@ func checkAlreadyPendingToCreate(l1CoordinatorTxs []common.L1Tx, tokenID common.
 }
 
 // getL2Profitable returns the profitable selection of L2Txssorted by Nonce
-func (txsel *TxSelector) getL2Profitable(l2Txs []common.PoolL2Tx, max uint32) []common.PoolL2Tx {
-	// Sort by absolute fee
-	sort.Sort(txs(l2Txs))
+func (txsel *TxSelector) getL2Profitable(l2Txs []common.PoolL2Tx, max uint32) ([]common.PoolL2Tx,
+	[]common.PoolL2Tx) {
+	// First sort by nonce so that txs from the same account are sorted so
+	// that they could be applied in succession.
+	sort.Slice(l2Txs, func(i, j int) bool {
+		return l2Txs[i].Nonce < l2Txs[j].Nonce
+	})
+	// Sort by absolute fee with SliceStable, so that txs with same
+	// AbsoluteFee are not rearranged and nonce order is kept in such case
+	sort.SliceStable(l2Txs, func(i, j int) bool {
+		return l2Txs[i].AbsoluteFee > l2Txs[j].AbsoluteFee
+	})
 
+	discardedL2Txs := []common.PoolL2Tx{}
 	if len(l2Txs) > int(max) {
+		discardedL2Txs = l2Txs[max:]
 		l2Txs = l2Txs[:max]
 	}
 
@@ -603,9 +602,9 @@ func (txsel *TxSelector) getL2Profitable(l2Txs []common.PoolL2Tx, max uint32) []
 	// Account is sorted, but the l2Txs can not be grouped by sender Account
 	// neither by Fee. This is because later on the Nonces will need to be
 	// sequential for the zkproof generation.
-	sort.SliceStable(l2Txs, func(i, j int) bool {
+	sort.Slice(l2Txs, func(i, j int) bool {
 		return l2Txs[i].Nonce < l2Txs[j].Nonce
 	})
 
-	return l2Txs
+	return l2Txs, discardedL2Txs
 }

txselector/txselector_test.go

@@ -658,7 +658,7 @@ func TestTransferManyFromSameAccount(t *testing.T) {
 	tpc := txprocessor.Config{
 		NLevels:  16,
 		MaxFeeTx: 10,
-		MaxTx:    20,
+		MaxTx:    10,
 		MaxL1Tx:  10,
 		ChainID:  chainID,
 	}
@@ -683,13 +683,17 @@ func TestTransferManyFromSameAccount(t *testing.T) {
 	PoolTransfer(0) A-B: 10 (126) // 6
 	PoolTransfer(0) A-B: 10 (126) // 7
 	PoolTransfer(0) A-B: 10 (126) // 8
+	PoolTransfer(0) A-B: 10 (126) // 9
+	PoolTransfer(0) A-B: 10 (126) // 10
+	PoolTransfer(0) A-B: 10 (126) // 11
 	`
 	poolL2Txs, err := tc.GeneratePoolL2Txs(batchPoolL2)
 	require.NoError(t, err)
+	require.Equal(t, 11, len(poolL2Txs))
 
 	// reorder poolL2Txs so that nonces are not sorted
 	poolL2Txs[0], poolL2Txs[7] = poolL2Txs[7], poolL2Txs[0]
-	poolL2Txs[1], poolL2Txs[6] = poolL2Txs[6], poolL2Txs[1]
+	poolL2Txs[1], poolL2Txs[10] = poolL2Txs[10], poolL2Txs[1]
 
 	// add the PoolL2Txs to the l2DB
 	addL2Txs(t, txsel, poolL2Txs)
@@ -699,8 +703,9 @@ func TestTransferManyFromSameAccount(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, 3, len(oL1UserTxs))
 	require.Equal(t, 0, len(oL1CoordTxs))
-	assert.Equal(t, 8, len(oL2Txs))
-	assert.Equal(t, 0, len(discardedL2Txs))
+	assert.Equal(t, 7, len(oL2Txs))
+	assert.Equal(t, 1, len(discardedL2Txs))
+
 	err = txsel.l2db.StartForging(common.TxIDsFromPoolL2Txs(oL2Txs), txsel.localAccountsDB.CurrentBatch())
 	require.NoError(t, err)
 }