Fix TxSel sorting, TxManager geth err checking
@@ -2,9 +2,9 @@ package coordinator
import ( "context" - "errors" "fmt" "math/big" + "strings" "time" "github.com/ethereum/go-ethereum"@@ -206,22 +206,27 @@ auth.GasPrice, t.cfg.MaxGasPrice))
} // 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 @@ } else if err != nil {
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():
@@ -324,5 +324,6 @@ GasPrice: tx.GasPrice(),
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) }
@@ -327,7 +327,7 @@ func (c *RollupClient) RollupForgeBatch(args *RollupForgeBatchArgs, auth *bind.TransactOpts) (tx *types.Transaction, err error) {
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 @@ tx, err = c.hermez.ForgeBatch(auth, newLastIdx, args.NewStRoot, args.NewExitRoot,
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 }
@@ -67,6 +67,11 @@ }
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)) }
@@ -18,20 +18,6 @@ "github.com/hermeznetwork/tracerr"
"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 @@ }
} // 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 @@ return false
} // 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 @@ // is needed is to output the l2Txs where the Nonce of l2Txs for each
// 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 }
@@ -658,7 +658,7 @@
tpc := txprocessor.Config{ NLevels: 16, MaxFeeTx: 10, - MaxTx: 20, + MaxTx: 10, MaxL1Tx: 10, ChainID: chainID, }@@ -683,13 +683,17 @@ PoolTransfer(0) A-B: 10 (126) // 5
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 @@ _, _, oL1UserTxs, oL1CoordTxs, oL2Txs, discardedL2Txs, err := txsel.GetL1L2TxSelection(selectionConfig, l1UserTxs)
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) }