Use sync calls to add txs to pool in tests

[piersy]

Without the use of sync calls there is no guarantee that txpoool reorg will run between adding txs and chekcking some condition, this causes tests to fail randomly.

This PR ensures the use of sync calls.

Tested

To verify my suspicion that the lack of reorg running was the cause of the error I was seeing in TestTransactionPoolUnderpricing i used the following patch to reliably recreate the error.

diff --git a/core/tx_pool.go b/core/tx_pool.go
index 9a3003d6f..0a35c8a76 100644
--- a/core/tx_pool.go
+++ b/core/tx_pool.go
@@ -263,6 +263,8 @@ type txPoolContext struct {
 // current state) and future transactions. Transactions move between those
 // two states over time as they are received and processed.
 type TxPool struct {
+   reorgSkip   int32
+   skipLock    sync.Mutex
    config      TxPoolConfig
    chainconfig *params.ChainConfig
    chain       blockChain
@@ -1248,6 +1250,17 @@ func (pool *TxPool) scheduleReorgLoop() {

 // runReorg runs reset and promoteExecutables on behalf of scheduleReorgLoop.
 func (pool *TxPool) runReorg(done chan struct{}, reset *txpoolResetRequest, dirtyAccounts *accountSet, events map[common.Address]*txSortedMap) {
+   pool.skipLock.Lock()
+   pool.reorgSkip++
+   if pool.reorgSkip >= 3 {
+       pool.skipLock.Unlock()
+       close(done)
+       return
+   }
+   pool.skipLock.Unlock()
+
+   println("reorging")
+
    defer func(t0 time.Time) {
        reorgDurationTimer.Update(time.Since(t0))
    }(time.Now())
diff --git a/core/tx_pool_test.go b/core/tx_pool_test.go
index 2a2ae348f..e48733f05 100644
--- a/core/tx_pool_test.go
+++ b/core/tx_pool_test.go
@@ -1899,7 +1899,9 @@ func TestTransactionPoolUnderpricing(t *testing.T) {
    ltx := pricedTransaction(0, 100000, big.NewInt(1), keys[2])

    // Import the batch and that both pending and queued transactions match up
+   println("adding remote")
    pool.AddRemotes(txs)
+   println("adding local")
    pool.AddLocal(ltx)

    pending, queued := pool.Stats()
@@ -1915,17 +1917,21 @@ func TestTransactionPoolUnderpricing(t *testing.T) {
    if err := validateTxPoolInternals(pool); err != nil {
        t.Fatalf("pool internal state corrupted: %v", err)
    }
+   println("adding remote")
    // Ensure that adding an underpriced transaction on block limit fails
    if err := pool.AddRemote(pricedTransaction(0, 100000, big.NewInt(1), keys[1])); err != ErrUnderpriced {
        t.Fatalf("adding underpriced pending transaction error mismatch: have %v, want %v", err, ErrUnderpriced)
    }
+   println("adding remote")
    // Ensure that adding high priced transactions drops cheap ones, but not own
    if err := pool.AddRemote(pricedTransaction(0, 100000, big.NewInt(3), keys[1])); err != nil { // +K1:0 => -K1:1 => Pend K0:0, K0:1, K1:0, K2:0; Que -
        t.Fatalf("failed to add well priced transaction: %v", err)
    }
+   println("adding remote")
    if err := pool.AddRemote(pricedTransaction(2, 100000, big.NewInt(4), keys[1])); err != nil { // +K1:2 => -K0:0 => Pend K1:0, K2:0; Que K0:1 K1:2
        t.Fatalf("failed to add well priced transaction: %v", err)
    }
+   println("adding remote")
    if err := pool.AddRemote(pricedTransaction(3, 100000, big.NewInt(5), keys[1])); err != nil { // +K1:3 => -K0:1 => Pend K1:0, K2:0; Que K1:2 K1:3
        t.Fatalf("failed to add well priced transaction: %v", err)
    }

Related issues

• Partial fix of #2276

[github-actions[bot]]

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 2aa6d4603436a95ab6ac9631d35c822cfcde426a

coverage: 46.9% of statements across all listed packages

coverage:  57.2% of statements in consensus/istanbul
coverage:  23.7% of statements in consensus/istanbul/announce
coverage:  54.4% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  24.3% of statements in consensus/istanbul/backend/internal/replica
coverage:  64.9% of statements in consensus/istanbul/core
coverage:  45.0% of statements in consensus/istanbul/db
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  64.2% of statements in consensus/istanbul/uptime
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random

[github-actions[bot]]

[piersy]

This sounds reasonable for at least some of the cases. I'm not sure we need the sync everywhere but I also don't see any harm in it (it can only slow things down, but not to a degree that matters, AFAICS).

As so often, upstream already solved the issue in at least some cases, see ethereum/go-ethereum@c866dfd#diff-6083272455b210c86aaa70e7a348d038d2867870cedb4d402b4909abef8fa23b .

Yep, I did check upstream but it seems unlikely we will do any more upstream merges, so this shouldn't cause us any trouble.