Merge #19753: p2p: don't add AlreadyHave transactions to recentRejects

d419fde [net processing] Don't add AlreadyHave txs to recentRejects (Troy Giorshev) Pull request description: If we already have a transaction, don't add it to recentRejects Now, we only add a transaction to our recentRejects filter if we didn't already have it, meaning that it is added at most once, as intended. ACKs for top commit: jnewbery: Code review ACK d419fde laanwj: Code review ACK d419fde Tree-SHA512: cff5c1ba36c4700e2d6ab3eec4a3e51e1bef28fb3cc1bc850c84e06d6e5a9f6c32825207c253cc9cdf596b2eaadb6b5be68b3f8ca752b4ef6c31cf85138e3c99
bitcoin-core · Oct 29, 2020 · 6196cf7 · 6196cf7
2 parents 3f512f3 + d419fde
commit 6196cf7
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 20 deletions.
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -2946,13 +2946,9 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
             pfrom.AddKnownTx(txid);
         }
 
-        TxValidationState state;
-
         m_txrequest.ReceivedResponse(pfrom.GetId(), txid);
         if (tx.HasWitness()) m_txrequest.ReceivedResponse(pfrom.GetId(), wtxid);
 
-        std::list<CTransactionRef> lRemovedTxn;
-
         // We do the AlreadyHaveTx() check using wtxid, rather than txid - in the
         // absence of witness malleation, this is strictly better, because the
         // recent rejects filter may contain the wtxid but rarely contains
@@ -2965,8 +2961,25 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
         // already; and an adversary can already relay us old transactions
         // (older than our recency filter) if trying to DoS us, without any need
         // for witness malleation.
-        if (!AlreadyHaveTx(GenTxid(/* is_wtxid=*/true, wtxid), m_mempool) &&
-            AcceptToMemoryPool(m_mempool, state, ptx, &lRemovedTxn, false /* bypass_limits */)) {
+        if (AlreadyHaveTx(GenTxid(/* is_wtxid=*/true, wtxid), m_mempool)) {
+            if (pfrom.HasPermission(PF_FORCERELAY)) {
+                // Always relay transactions received from peers with forcerelay
+                // permission, even if they were already in the mempool, allowing
+                // the node to function as a gateway for nodes hidden behind it.
+                if (!m_mempool.exists(tx.GetHash())) {
+                    LogPrintf("Not relaying non-mempool transaction %s from forcerelay peer=%d\n", tx.GetHash().ToString(), pfrom.GetId());
+                } else {
+                    LogPrintf("Force relaying tx %s from peer=%d\n", tx.GetHash().ToString(), pfrom.GetId());
+                    RelayTransaction(tx.GetHash(), tx.GetWitnessHash(), m_connman);
+                }
+            }
+            return;
+        }
+
+        TxValidationState state;
+        std::list<CTransactionRef> lRemovedTxn;
+
+        if (AcceptToMemoryPool(m_mempool, state, ptx, &lRemovedTxn, false /* bypass_limits */)) {
             m_mempool.check(&::ChainstateActive().CoinsTip());
             // As this version of the transaction was acceptable, we can forget about any
             // requests for it.
@@ -3088,19 +3101,6 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
                     AddToCompactExtraTransactions(ptx);
                 }
             }
-
-            if (pfrom.HasPermission(PF_FORCERELAY)) {
-                // Always relay transactions received from peers with forcerelay permission, even
-                // if they were already in the mempool,
-                // allowing the node to function as a gateway for
-                // nodes hidden behind it.
-                if (!m_mempool.exists(tx.GetHash())) {
-                    LogPrintf("Not relaying non-mempool transaction %s from forcerelay peer=%d\n", tx.GetHash().ToString(), pfrom.GetId());
-                } else {
-                    LogPrintf("Force relaying tx %s from peer=%d\n", tx.GetHash().ToString(), pfrom.GetId());
-                    RelayTransaction(tx.GetHash(), tx.GetWitnessHash(), m_connman);
-                }
-            }
         }
 
         // If a tx has been detected by recentRejects, we will have reached

diff --git a/test/functional/p2p_permissions.py b/test/functional/p2p_permissions.py
@@ -153,11 +153,20 @@ def check_tx_relay(self):
         self.log.debug("Check that node[1] will not send an invalid tx to node[0]")
         tx.vout[0].nValue += 1
         txid = tx.rehash()
+        # Send the transaction twice. The first time, it'll be rejected by ATMP because it conflicts
+        # with a mempool transaction. The second time, it'll be in the recentRejects filter.
         p2p_rebroadcast_wallet.send_txs_and_test(
             [tx],
             self.nodes[1],
             success=False,
-            reject_reason='Not relaying non-mempool transaction {} from forcerelay peer=0'.format(txid),
+            reject_reason='{} from peer=0 was not accepted: txn-mempool-conflict'.format(txid)
+        )
+
+        p2p_rebroadcast_wallet.send_txs_and_test(
+            [tx],
+            self.nodes[1],
+            success=False,
+            reject_reason='Not relaying non-mempool transaction {} from forcerelay peer=0'.format(txid)
         )
 
     def checkpermission(self, args, expectedPermissions, whitelisted):