Test concurrent feecurrency tx handling in tx pool (#2333)

Add test to verify lack of race conditions in txpool

When handling fee currency transactions.

Also fixes a race condition in istanbul core when
accessing the current round state.

consensus/istanbul/core/handler.go

@@ -36,7 +36,9 @@ func (c *core) Start() error {
 		return err
 	}
 
+	c.currentMu.Lock()
 	c.current = roundState
+	c.currentMu.Unlock()
 	c.roundChangeSetV2 = newRoundChangeSetV2(c.current.ValidatorSet())
 
 	// Reset the Round Change timer for the current round to timeout.

e2e_test/e2e_test.go

@@ -732,7 +732,7 @@ func testRPCDynamicTxGasPriceWithoutState(t *testing.T, afterGingerbread, altern
 	gasTipCap := big.NewInt(0).Mul(suggestedGasPrice, big.NewInt(2))
 
 	// Send one celo from external account 0 to 1 via node 0.
-	tx, err := accounts[0].SendValueWithDynamicFee(ctx, accounts[1].Address, 1, feeCurrency, gasFeeCap, gasTipCap, network[0])
+	tx, err := accounts[0].SendValueWithDynamicFee(ctx, accounts[1].Address, 1, feeCurrency, gasFeeCap, gasTipCap, network[0], 0)
 	require.NoError(t, err)
 
 	// Wait for the whole network to process the transaction.
@@ -1044,3 +1044,66 @@ func TestGetFinalizedBlock(t *testing.T) {
 	// Check latest and finalzed block are the same
 	require.Equal(t, h.Hash(), h2.Hash())
 }
+
+// TestManyFeeCurrencyTransactions is intended to test that we don't have race conditions in the tx pool when handling
+// fee currency transactions. It does this by submitting many fee currency transactions from 3 different goroutines over
+// a period of roughly 5 seconds which with the configured block time of 1 second means that the transactions should
+// span multiple block boundaries with the goal of ensuring that both runReorg and AddRemotes are being called
+// concurrently in the txPool. This issue https://github.com/celo-org/celo-blockchain/issues/2318 is occurring somewhat
+// randomly and could be the result of some race condition in tx pool handling for fee currency transactions. However
+// this test seems to run fairly reliably with the race flag enabled, which seems to indicate that the problem is not a
+// result of racy behavior in the tx pool.
+func TestManyFeeCurrencyTransactions(t *testing.T) {
+	ac := test.AccountConfig(3, 3)
+	gingerbreadBlock := common.Big0
+	gc, ec, err := test.BuildConfig(ac, gingerbreadBlock, nil)
+	require.NoError(t, err)
+	ec.Istanbul.BlockPeriod = 1
+	network, shutdown, err := test.NewNetwork(ac, gc, ec)
+	require.NoError(t, err)
+	defer shutdown()
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second*200)
+	defer cancel()
+
+	cUSD := common.HexToAddress("0x000000000000000000000000000000000000d008")
+	cEUR := common.HexToAddress("0x000000000000000000000000000000000000D024")
+	cREAL := common.HexToAddress("0x000000000000000000000000000000000000d026")
+
+	accounts := test.Accounts(ac.DeveloperAccounts(), gc.ChainConfig())
+
+	time.Sleep(2 * time.Second)
+	txsChan := make(chan []*types.Transaction, 3)
+	for nodeIndex := 0; nodeIndex < len(network); nodeIndex++ {
+		go func(nodeIndex int) {
+			txs := make([]*types.Transaction, 0, 3000)
+			for i := 0; i < 100; i++ {
+				for _, feeCurrency := range []*common.Address{&cUSD, &cEUR, &cREAL} {
+					baseFee, err := network[nodeIndex].WsClient.SuggestGasPriceInCurrency(ctx, feeCurrency)
+					require.NoError(t, err)
+					tip, err := network[nodeIndex].WsClient.SuggestGasTipCapInCurrency(ctx, feeCurrency)
+					require.NoError(t, err)
+
+					// Send one celo from external account 0 to 1 via node 0.
+					tx, err := accounts[nodeIndex].SendValueWithDynamicFee(ctx, accounts[nodeIndex].Address, 1, feeCurrency, baseFee.Add(baseFee, tip), tip, network[nodeIndex], 71000)
+					require.NoError(t, err)
+					txs = append(txs, tx)
+					time.Sleep(16 * time.Millisecond)
+				}
+			}
+			txsChan <- txs
+		}(nodeIndex)
+	}
+
+	allTxs := make([]*types.Transaction, 0, 3000*len(network))
+	count := 0
+	for txs := range txsChan {
+		allTxs = append(allTxs, txs...)
+		count++
+		if count == len(network) {
+			break
+		}
+	}
+
+	err = network.AwaitTransactions(ctx, allTxs...)
+	require.NoError(t, err)
+}

ethclient/ethclient.go

@@ -546,7 +546,7 @@ func (ec *Client) SuggestGasPrice(ctx context.Context) (*big.Int, error) {
 	return (*big.Int)(&hex), nil
 }
 
-// SuggestGasPrice retrieves the currently suggested gas price to allow a timely
+// SuggestGasPriceInCurrency retrieves the currently suggested gas price to allow a timely
 // execution of a transaction.
 func (ec *Client) SuggestGasPriceInCurrency(ctx context.Context, feeCurrency *common.Address) (*big.Int, error) {
 	var hex hexutil.Big
@@ -566,6 +566,16 @@ func (ec *Client) SuggestGasTipCap(ctx context.Context) (*big.Int, error) {
 	return (*big.Int)(&hex), nil
 }
 
+// SuggestGasTipCapInCurrency retrieves the currently suggested gas tip cap after 1559 to
+// allow a timely execution of a transaction.
+func (ec *Client) SuggestGasTipCapInCurrency(ctx context.Context, feeCurrency *common.Address) (*big.Int, error) {
+	var hex hexutil.Big
+	if err := ec.c.CallContext(ctx, &hex, "eth_maxPriorityFeePerGas", feeCurrency); err != nil {
+		return nil, err
+	}
+	return (*big.Int)(&hex), nil
+}
+
 // EstimateGas tries to estimate the gas needed to execute a specific transaction based on
 // the current pending state of the backend blockchain. There is no guarantee that this is
 // the true gas limit requirement as other transactions may be added or removed by miners,

test/account.go

@@ -70,12 +70,13 @@ func (a *Account) SendCelo(ctx context.Context, recipient common.Address, value
 // SendCeloWithDynamicFee submits a value transfer transaction via the provided Node to send
 // celo to the recipient. The submitted transaction is returned.
 func (a *Account) SendCeloWithDynamicFee(ctx context.Context, recipient common.Address, value int64, gasFeeCap *big.Int, gasTipCap *big.Int, node *Node) (*types.Transaction, error) {
-	return a.SendValueWithDynamicFee(ctx, recipient, value, nil, gasFeeCap, gasTipCap, node)
+	return a.SendValueWithDynamicFee(ctx, recipient, value, nil, gasFeeCap, gasTipCap, node, 0)
 }
 
-// SendValueWithDynamicFee submits a value transfer transaction via the provided Node to send
-// celo to the recipient. The submitted transaction is returned.
-func (a *Account) SendValueWithDynamicFee(ctx context.Context, recipient common.Address, value int64, feeCurrency *common.Address, gasFeeCap, gasTipCap *big.Int, node *Node) (*types.Transaction, error) {
+// SendValueWithDynamicFee submits a value transfer transaction via the provided Node to send celo to the recipient. The
+// submitted transaction is returned. Note that gasLimit is optional and if 0 is provided the estimate gas will be
+// called to determine the gas limit.
+func (a *Account) SendValueWithDynamicFee(ctx context.Context, recipient common.Address, value int64, feeCurrency *common.Address, gasFeeCap, gasTipCap *big.Int, node *Node, gasLimit uint64) (*types.Transaction, error) {
 	var err error
 	// Lazy set nonce
 	if a.Nonce == nil {
@@ -101,7 +102,8 @@ func (a *Account) SendValueWithDynamicFee(ctx context.Context, recipient common.
 		feeCurrency,
 		gasFeeCap,
 		gasTipCap,
-		signer)
+		signer,
+		gasLimit)
 
 	if err != nil {
 		return nil, err

test/node.go

@@ -665,14 +665,18 @@ func ValueTransferTransactionWithDynamicFee(
 	gasFeeCap *big.Int,
 	gasTipCap *big.Int,
 	signer types.Signer,
+	gasLimit uint64,
 ) (*types.Transaction, error) {
 	ctx, cancel := context.WithTimeout(context.Background(), time.Second*2)
 	defer cancel()
 
-	msg := ethereum.CallMsg{From: sender, To: &recipient, Value: value, FeeCurrency: feeCurrency}
-	gasLimit, err := client.EstimateGas(ctx, msg)
-	if err != nil {
-		return nil, fmt.Errorf("failed to estimate gas needed: %v", err)
+	if gasLimit == 0 {
+		msg := ethereum.CallMsg{From: sender, To: &recipient, Value: value, FeeCurrency: feeCurrency}
+		var err error
+		gasLimit, err = client.EstimateGas(ctx, msg)
+		if err != nil {
+			return nil, fmt.Errorf("failed to estimate gas needed: %v", err)
+		}
 	}
 	// Create the transaction and sign it
 	rawTx := types.NewTx(&types.CeloDynamicFeeTx{