From 2547d5dce7802509ebdb5d65d5b3f89c32448abe Mon Sep 17 00:00:00 2001 From: Eduard S Date: Thu, 18 Feb 2021 11:18:30 +0100 Subject: [PATCH] Fix TxSel sorting, TxManager geth err checking --- coordinator/txmanager.go | 17 +++++++++------ eth/ethereum.go | 3 ++- eth/rollup.go | 4 ++-- log/log.go | 5 +++++ txselector/txselector.go | 41 +++++++++++++++++------------------ txselector/txselector_test.go | 13 +++++++---- 6 files changed, 48 insertions(+), 35 deletions(-) diff --git a/coordinator/txmanager.go b/coordinator/txmanager.go index b7f6bcb..ad05962 100644 --- a/coordinator/txmanager.go +++ b/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(): diff --git a/eth/ethereum.go b/eth/ethereum.go index d1d798a..c2075f6 100644 --- a/eth/ethereum.go +++ b/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) } diff --git a/eth/rollup.go b/eth/rollup.go index 5a77bd2..c093b29 100644 --- a/eth/rollup.go +++ b/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 } diff --git a/log/log.go b/log/log.go index 80da362..4ae331b 100644 --- a/log/log.go +++ b/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)) } diff --git a/txselector/txselector.go b/txselector/txselector.go index 862cbc2..bf5ffa6 100644 --- a/txselector/txselector.go +++ b/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 } diff --git a/txselector/txselector_test.go b/txselector/txselector_test.go index 9c513da..13f3303 100644 --- a/txselector/txselector_test.go +++ b/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) }