From 44d940a011156a3ed97635a529b4c6cafe6ea672 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrei=20B=C4=83ncioiu?= Date: Tue, 12 Nov 2024 09:09:38 +0200 Subject: [PATCH] Fix handling of lower nonces (act only when nonce is known). Refactor, rename, add tests. --- txcache/eviction.go | 2 +- txcache/txCache.go | 8 +- txcache/txCache_test.go | 13 ++- txcache/txListBySenderMap.go | 12 +-- txcache/txListBySenderMap_test.go | 8 +- txcache/txListForSender.go | 14 +++- txcache/txListForSender_test.go | 134 ++++++++++++++++++------------ 7 files changed, 116 insertions(+), 75 deletions(-) diff --git a/txcache/eviction.go b/txcache/eviction.go index adb6bb2e..d82da786 100644 --- a/txcache/eviction.go +++ b/txcache/eviction.go @@ -160,7 +160,7 @@ func (cache *TxCache) evictLeastLikelyToSelectTransactions() *evictionJournal { // Remove those transactions from "txListBySender". for sender, nonce := range lowestToEvictBySender { - cache.txListBySender.evictTransactionsWithHigherOrEqualNonces([]byte(sender), nonce) + cache.txListBySender.removeTransactionsWithHigherOrEqualNonce([]byte(sender), nonce) } // Remove those transactions from "txByHash". diff --git a/txcache/txCache.go b/txcache/txCache.go index 140adbef..2c494831 100644 --- a/txcache/txCache.go +++ b/txcache/txCache.go @@ -131,18 +131,18 @@ func (cache *TxCache) getSenders() []*txListForSender { return cache.txListBySender.getSenders() } -// RemoveTxByHash removes tx by hash +// RemoveTxByHash removes transactions with nonces lower or equal to the given transaction's nonce func (cache *TxCache) RemoveTxByHash(txHash []byte) bool { cache.mutTxOperation.Lock() defer cache.mutTxOperation.Unlock() tx, foundInByHash := cache.txByHash.removeTx(string(txHash)) if !foundInByHash { - // Transaction might have been removed in the meantime (e.g. due to NotifyAccountNonce). + // Transaction might have been removed in the meantime. return false } - evicted := cache.txListBySender.removeTxReturnEvicted(tx) + evicted := cache.txListBySender.removeTransactionsWithLowerOrEqualNonceReturnHashes(tx) if len(evicted) > 0 { cache.txByHash.RemoveTxsBulk(evicted) } @@ -277,6 +277,8 @@ func (cache *TxCache) UnRegisterHandler(string) { // NotifyAccountNonce should be called by external components (such as interceptors and transactions processor) // in order to inform the cache about initial nonce gap phenomena func (cache *TxCache) NotifyAccountNonce(accountKey []byte, nonce uint64) { + log.Trace("TxCache.NotifyAccountNonce", "account", accountKey, "nonce", nonce) + cache.txListBySender.notifyAccountNonce(accountKey, nonce) } diff --git a/txcache/txCache_test.go b/txcache/txCache_test.go index ed14a6c4..e9038543 100644 --- a/txcache/txCache_test.go +++ b/txcache/txCache_test.go @@ -152,7 +152,12 @@ func Test_RemoveByTxHash(t *testing.T) { removed := cache.RemoveTxByHash([]byte("hash-1")) require.True(t, removed) - cache.Remove([]byte("hash-2")) + + removed = cache.RemoveTxByHash([]byte("hash-2")) + require.True(t, removed) + + removed = cache.RemoveTxByHash([]byte("hash-3")) + require.False(t, removed) foundTx, ok := cache.GetByTxHash([]byte("hash-1")) require.False(t, ok) @@ -161,6 +166,8 @@ func Test_RemoveByTxHash(t *testing.T) { foundTx, ok = cache.GetByTxHash([]byte("hash-2")) require.False(t, ok) require.Nil(t, foundTx) + + require.Equal(t, uint64(0), cache.CountTx()) } func Test_CountTx_And_Len(t *testing.T) { @@ -216,7 +223,7 @@ func Test_RemoveByTxHash_RemovesFromByHash_WhenMapsInconsistency(t *testing.T) { cache.AddTx(tx) // Cause an inconsistency between the two internal maps (theoretically possible in case of misbehaving eviction) - _ = cache.txListBySender.removeTxReturnEvicted(tx) + _ = cache.txListBySender.removeTransactionsWithLowerOrEqualNonceReturnHashes(tx) _ = cache.RemoveTxByHash(txHash) require.Equal(t, 0, cache.txByHash.backingMap.Count()) @@ -281,7 +288,7 @@ func Test_GetTransactionsPoolForSender(t *testing.T) { txs = cache.GetTransactionsPoolForSender(txSender2) require.Equal(t, wrappedTxs2, txs) - cache.RemoveTxByHash(txHashes2[0]) + _ = cache.RemoveTxByHash(txHashes2[0]) expectedTxs := wrappedTxs2[1:] txs = cache.GetTransactionsPoolForSender(txSender2) require.Equal(t, expectedTxs, txs) diff --git a/txcache/txListBySenderMap.go b/txcache/txListBySenderMap.go index 72bfdec3..a1afbf10 100644 --- a/txcache/txListBySenderMap.go +++ b/txcache/txListBySenderMap.go @@ -75,8 +75,8 @@ func (txMap *txListBySenderMap) addSender(sender string) *txListForSender { return listForSender } -// removeTxReturnEvicted removes a transaction from the map -func (txMap *txListBySenderMap) removeTxReturnEvicted(tx *WrappedTransaction) [][]byte { +// removeTransactionsWithLowerOrEqualNonceReturnHashes removes transactions with nonces lower or equal to the given transaction's nonce. +func (txMap *txListBySenderMap) removeTransactionsWithLowerOrEqualNonceReturnHashes(tx *WrappedTransaction) [][]byte { sender := string(tx.Tx.GetSndAddr()) listForSender, ok := txMap.getListForSender(sender) @@ -87,7 +87,7 @@ func (txMap *txListBySenderMap) removeTxReturnEvicted(tx *WrappedTransaction) [] return nil } - evicted := listForSender.evictTransactionsWithLowerOrEqualNonces(tx.Tx.GetNonce()) + evicted := listForSender.removeTransactionsWithLowerOrEqualNonceReturnHashes(tx.Tx.GetNonce()) txMap.removeSenderIfEmpty(listForSender) return evicted } @@ -133,16 +133,16 @@ func (txMap *txListBySenderMap) notifyAccountNonce(accountKey []byte, nonce uint listForSender.notifyAccountNonce(nonce) } -// evictTransactionsWithHigherOrEqualNonces removes transactions with nonces higher or equal to the given nonce. +// removeTransactionsWithHigherOrEqualNonce removes transactions with nonces higher or equal to the given nonce. // Useful for the eviction flow. -func (txMap *txListBySenderMap) evictTransactionsWithHigherOrEqualNonces(accountKey []byte, nonce uint64) { +func (txMap *txListBySenderMap) removeTransactionsWithHigherOrEqualNonce(accountKey []byte, nonce uint64) { sender := string(accountKey) listForSender, ok := txMap.getListForSender(sender) if !ok { return } - listForSender.evictTransactionsWithHigherOrEqualNonces(nonce) + listForSender.removeTransactionsWithHigherOrEqualNonce(nonce) txMap.removeSenderIfEmpty(listForSender) } diff --git a/txcache/txListBySenderMap_test.go b/txcache/txListBySenderMap_test.go index 3fda916b..083925fb 100644 --- a/txcache/txListBySenderMap_test.go +++ b/txcache/txListBySenderMap_test.go @@ -19,7 +19,7 @@ func TestSendersMap_AddTx_IncrementsCounter(t *testing.T) { require.Equal(t, int64(2), myMap.counter.Get()) } -func TestSendersMap_RemoveTx_AlsoRemovesSenderWhenNoTransactionLeft(t *testing.T) { +func TestSendersMap_removeTransactionsWithLowerOrEqualNonceReturnHashes_alsoRemovesSenderWhenNoTransactionLeft(t *testing.T) { myMap := newSendersMapToTest() txAlice1 := createTx([]byte("a1"), "alice", 1) @@ -33,16 +33,16 @@ func TestSendersMap_RemoveTx_AlsoRemovesSenderWhenNoTransactionLeft(t *testing.T require.Equal(t, uint64(2), myMap.testGetListForSender("alice").countTx()) require.Equal(t, uint64(1), myMap.testGetListForSender("bob").countTx()) - _ = myMap.removeTxReturnEvicted(txAlice1) + _ = myMap.removeTransactionsWithLowerOrEqualNonceReturnHashes(txAlice1) require.Equal(t, int64(2), myMap.counter.Get()) require.Equal(t, uint64(1), myMap.testGetListForSender("alice").countTx()) require.Equal(t, uint64(1), myMap.testGetListForSender("bob").countTx()) - _ = myMap.removeTxReturnEvicted(txAlice2) + _ = myMap.removeTransactionsWithLowerOrEqualNonceReturnHashes(txAlice2) // All alice's transactions have been removed now require.Equal(t, int64(1), myMap.counter.Get()) - _ = myMap.removeTxReturnEvicted(txBob) + _ = myMap.removeTransactionsWithLowerOrEqualNonceReturnHashes(txBob) // Also Bob has no more transactions require.Equal(t, int64(0), myMap.counter.Get()) } diff --git a/txcache/txListForSender.go b/txcache/txListForSender.go index 1767b277..b6b43e12 100644 --- a/txcache/txListForSender.go +++ b/txcache/txListForSender.go @@ -205,7 +205,7 @@ func (listForSender *txListForSender) getSequentialTxs() []*WrappedTransaction { if isFirstTx { // Handle lower nonces. - if accountNonce > nonce { + if accountNonceKnown && accountNonce > nonce { log.Trace("txListForSender.getSequentialTxs, lower nonce", "sender", listForSender.sender, "nonce", nonce, "accountNonce", accountNonce) continue } @@ -253,8 +253,14 @@ func (listForSender *txListForSender) notifyAccountNonce(nonce uint64) { _ = listForSender.accountNonceKnown.SetReturningPrevious() } -// evictTransactionsWithLowerOrEqualNonces removes transactions with nonces lower or equal to the given nonce -func (listForSender *txListForSender) evictTransactionsWithLowerOrEqualNonces(targetNonce uint64) [][]byte { +// forgetAccountNonce resets the known account nonce +func (listForSender *txListForSender) forgetAccountNonce() { + listForSender.accountNonce.Set(0) + listForSender.accountNonceKnown.Reset() +} + +// removeTransactionsWithLowerOrEqualNonceReturnHashes removes transactions with nonces lower or equal to the given nonce +func (listForSender *txListForSender) removeTransactionsWithLowerOrEqualNonceReturnHashes(targetNonce uint64) [][]byte { evictedTxHashes := make([][]byte, 0) // We don't allow concurrent goroutines to mutate a given sender's list @@ -281,7 +287,7 @@ func (listForSender *txListForSender) evictTransactionsWithLowerOrEqualNonces(ta return evictedTxHashes } -func (listForSender *txListForSender) evictTransactionsWithHigherOrEqualNonces(givenNonce uint64) { +func (listForSender *txListForSender) removeTransactionsWithHigherOrEqualNonce(givenNonce uint64) { listForSender.mutex.Lock() defer listForSender.mutex.Unlock() diff --git a/txcache/txListForSender_test.go b/txcache/txListForSender_test.go index 3f8b075a..0b8892db 100644 --- a/txcache/txListForSender_test.go +++ b/txcache/txListForSender_test.go @@ -119,7 +119,7 @@ func TestListForSender_NotifyAccountNonce(t *testing.T) { require.True(t, list.accountNonceKnown.IsSet()) } -func TestListForSender_evictTransactionsWithLowerOrEqualNonces(t *testing.T) { +func TestListForSender_removeTransactionsWithLowerOrEqualNonceReturnHashes(t *testing.T) { list := newUnconstrainedListToTest() list.AddTx(createTx([]byte("tx-42"), ".", 42)) @@ -129,70 +129,96 @@ func TestListForSender_evictTransactionsWithLowerOrEqualNonces(t *testing.T) { require.Equal(t, 4, list.items.Len()) - _ = list.evictTransactionsWithLowerOrEqualNonces(43) + _ = list.removeTransactionsWithLowerOrEqualNonceReturnHashes(43) require.Equal(t, 2, list.items.Len()) - _ = list.evictTransactionsWithLowerOrEqualNonces(44) + _ = list.removeTransactionsWithLowerOrEqualNonceReturnHashes(44) require.Equal(t, 1, list.items.Len()) - _ = list.evictTransactionsWithLowerOrEqualNonces(99) + _ = list.removeTransactionsWithLowerOrEqualNonceReturnHashes(99) require.Equal(t, 0, list.items.Len()) } func TestListForSender_getTxs(t *testing.T) { - list := newUnconstrainedListToTest() - list.notifyAccountNonce(42) + t.Run("no transactions", func(t *testing.T) { + list := newUnconstrainedListToTest() + list.notifyAccountNonce(42) - // No transaction, no gap - require.Len(t, list.getTxs(), 0) - require.Len(t, list.getTxsReversed(), 0) - require.Len(t, list.getSequentialTxs(), 0) + require.Len(t, list.getTxs(), 0) + require.Len(t, list.getTxsReversed(), 0) + require.Len(t, list.getSequentialTxs(), 0) + }) - // One gap - list.AddTx(createTx([]byte("tx-43"), ".", 43)) - require.Len(t, list.getTxs(), 1) - require.Len(t, list.getTxsReversed(), 1) - require.Len(t, list.getSequentialTxs(), 0) + t.Run("one transaction, one gap", func(t *testing.T) { + list := newUnconstrainedListToTest() + list.notifyAccountNonce(42) - // Resolve gap - list.AddTx(createTx([]byte("tx-42"), ".", 42)) - require.Len(t, list.getTxs(), 2) - require.Len(t, list.getTxsReversed(), 2) - require.Len(t, list.getSequentialTxs(), 2) - - require.Equal(t, []byte("tx-42"), list.getTxs()[0].TxHash) - require.Equal(t, []byte("tx-43"), list.getTxs()[1].TxHash) - require.Equal(t, list.getTxs(), list.getSequentialTxs()) - - require.Equal(t, []byte("tx-43"), list.getTxsReversed()[0].TxHash) - require.Equal(t, []byte("tx-42"), list.getTxsReversed()[1].TxHash) - - // With nonce duplicates - list.AddTx(createTx([]byte("tx-42++"), ".", 42).withGasPrice(1.1 * oneBillion)) - list.AddTx(createTx([]byte("tx-43++"), ".", 43).withGasPrice(1.1 * oneBillion)) - require.Len(t, list.getTxs(), 4) - require.Len(t, list.getTxsReversed(), 4) - require.Len(t, list.getSequentialTxs(), 2) - - require.Equal(t, []byte("tx-42++"), list.getSequentialTxs()[0].TxHash) - require.Equal(t, []byte("tx-43++"), list.getSequentialTxs()[1].TxHash) - - require.Equal(t, []byte("tx-42++"), list.getTxs()[0].TxHash) - require.Equal(t, []byte("tx-42"), list.getTxs()[1].TxHash) - require.Equal(t, []byte("tx-43++"), list.getTxs()[2].TxHash) - require.Equal(t, []byte("tx-43"), list.getTxs()[3].TxHash) - - require.Equal(t, []byte("tx-43"), list.getTxsReversed()[0].TxHash) - require.Equal(t, []byte("tx-43++"), list.getTxsReversed()[1].TxHash) - require.Equal(t, []byte("tx-42"), list.getTxsReversed()[2].TxHash) - require.Equal(t, []byte("tx-42++"), list.getTxsReversed()[3].TxHash) - - // With lower nonces - list.notifyAccountNonce(43) - require.Len(t, list.getTxs(), 4) - require.Len(t, list.getTxsReversed(), 4) - require.Len(t, list.getSequentialTxs(), 1) - require.Equal(t, []byte("tx-43++"), list.getSequentialTxs()[0].TxHash) + // Gap + list.AddTx(createTx([]byte("tx-43"), ".", 43)) + require.Len(t, list.getTxs(), 1) + require.Len(t, list.getTxsReversed(), 1) + require.Len(t, list.getSequentialTxs(), 0) + + // Resolve gap + list.AddTx(createTx([]byte("tx-42"), ".", 42)) + require.Len(t, list.getTxs(), 2) + require.Len(t, list.getTxsReversed(), 2) + require.Len(t, list.getSequentialTxs(), 2) + + require.Equal(t, []byte("tx-42"), list.getTxs()[0].TxHash) + require.Equal(t, []byte("tx-43"), list.getTxs()[1].TxHash) + require.Equal(t, list.getTxs(), list.getSequentialTxs()) + + require.Equal(t, []byte("tx-43"), list.getTxsReversed()[0].TxHash) + require.Equal(t, []byte("tx-42"), list.getTxsReversed()[1].TxHash) + }) + + t.Run("with nonce duplicates", func(t *testing.T) { + list := newUnconstrainedListToTest() + list.notifyAccountNonce(42) + + list.AddTx(createTx([]byte("tx-42"), ".", 42)) + list.AddTx(createTx([]byte("tx-43"), ".", 43)) + + list.AddTx(createTx([]byte("tx-42++"), ".", 42).withGasPrice(1.1 * oneBillion)) + list.AddTx(createTx([]byte("tx-43++"), ".", 43).withGasPrice(1.1 * oneBillion)) + + require.Len(t, list.getTxs(), 4) + require.Len(t, list.getTxsReversed(), 4) + require.Len(t, list.getSequentialTxs(), 2) + + require.Equal(t, []byte("tx-42++"), list.getSequentialTxs()[0].TxHash) + require.Equal(t, []byte("tx-43++"), list.getSequentialTxs()[1].TxHash) + + require.Equal(t, []byte("tx-42++"), list.getTxs()[0].TxHash) + require.Equal(t, []byte("tx-42"), list.getTxs()[1].TxHash) + require.Equal(t, []byte("tx-43++"), list.getTxs()[2].TxHash) + require.Equal(t, []byte("tx-43"), list.getTxs()[3].TxHash) + + require.Equal(t, []byte("tx-43"), list.getTxsReversed()[0].TxHash) + require.Equal(t, []byte("tx-43++"), list.getTxsReversed()[1].TxHash) + require.Equal(t, []byte("tx-42"), list.getTxsReversed()[2].TxHash) + require.Equal(t, []byte("tx-42++"), list.getTxsReversed()[3].TxHash) + }) + + t.Run("with lower nonces", func(t *testing.T) { + list := newUnconstrainedListToTest() + list.notifyAccountNonce(43) + + list.AddTx(createTx([]byte("tx-42"), ".", 42)) + list.AddTx(createTx([]byte("tx-43"), ".", 43)) + + require.Len(t, list.getTxs(), 2) + require.Len(t, list.getTxsReversed(), 2) + require.Len(t, list.getSequentialTxs(), 1) + require.Equal(t, []byte("tx-43"), list.getSequentialTxs()[0].TxHash) + + list.forgetAccountNonce() + + require.Len(t, list.getTxs(), 2) + require.Len(t, list.getTxsReversed(), 2) + require.Len(t, list.getSequentialTxs(), 2) + }) } func TestListForSender_DetectRaceConditions(t *testing.T) {