Merge #18861: Do not answer GETDATA for to-be-announced tx

2896c41 Do not answer GETDATA for to-be-announced tx (Pieter Wuille)
f2f32a3 Push down use of cs_main into FindTxForGetData (Pieter Wuille)
c6131bf Abstract logic to determine whether to answer tx GETDATA (Pieter Wuille)

Pull request description:

  This PR intends to improve transaction-origin privacy.

  In general, we should try to not leak information about what transactions we have (recently) learned about before deciding to announce them to our peers. There is a controlled transaction dissemination process that reveals our transactions to peers that has various safeguards for privacy (it's rate-limited, delayed & batched, deterministically sorted, ...), and ideally there is no way to test which transactions we have before that controlled process reveals them. The handling of the `mempool` BIP35 message has protections in this regard as well, as it would be an obvious way to bypass these protections (handled asynchronously after a delay, also deterministically sorted).

  However, currently, if we receive a GETDATA for a transaction that we have not yet announced to the requester, we will still respond to it if it was announced to *some* other peer already (because it needs to be in `mapRelay`, which only happens on the first announcement). This is a slight privacy leak.

  Thankfully, this seems easy to solve: `setInventontoryTxToSend` keeps track of the txids we have yet to announce to a peer - which almost(*) exactly corresponds to the transactions we know of that we haven't revealed to that peer. By checking whether a txid is in that set before responding to a GETDATA, we can filter these out.

  (*) Locally resubmitted or rebroadcasted transactions may end up in setInventoryTxToSend while the peer already knows we have them, which could result in us incorrectly claiming we don't have such transactions if coincidentally requested right after we schedule reannouncing them, but before they're actually INVed. This is made even harder by the fact that filterInventoryKnown will generally keep known reannouncements out of setInventoryTxToSend unless it overflows (which needs 50000 INVs in either direction before it happens).

  The condition for responding now becomes:

  ```
    (not in setInventoryTxToSend) AND
    (
      (in relay map) OR
      (
        (in mempool) AND
        (old enough that it could have expired from relay map) AND
        (older than our last getmempool response)
      )
    )
  ```

ACKs for top commit:
  naumenkogs:
    utACK 2896c41
  ajtowns:
    ACK 2896c41
  amitiuttarwar:
    code review ACK 2896c41
  jonatack:
    ACK 2896c41 per `git diff 2b3f101 2896c41` only change since previous review is moving the recency check up to be verified first in `FindTxForGetData`, as it was originally in 353a391 (good catch), before looking up the transaction in the relay pool.
  jnewbery:
    code review ACK 2896c41

Tree-SHA512: e7d5bc006e626f60a2c108a9334f3bbb67205ace04a7450a1e4d4db1d85922a7589e0524500b7b4953762cf70554c4a08eec62c7b38b486cbca3d86321600868

Author: fanquake

src/net_processing.cpp

@@ -1608,6 +1608,37 @@ void static ProcessGetBlockData(CNode* pfrom, const CChainParams& chainparams, c
     }
 }
 
+//! Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed).
+CTransactionRef static FindTxForGetData(CNode* peer, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds longlived_mempool_time) LOCKS_EXCLUDED(cs_main)
+{
+    // Check if the requested transaction is so recent that we're just
+    // about to announce it to the peer; if so, they certainly shouldn't
+    // know we already have it.
+    {
+        LOCK(peer->m_tx_relay->cs_tx_inventory);
+        if (peer->m_tx_relay->setInventoryTxToSend.count(txid)) return {};
+    }
+
+    {
+        LOCK(cs_main);
+        // Look up transaction in relay pool
+        auto mi = mapRelay.find(txid);
+        if (mi != mapRelay.end()) return mi->second;
+    }
+
+    auto txinfo = mempool.info(txid);
+    if (txinfo.tx) {
+        // To protect privacy, do not answer getdata using the mempool when
+        // that TX couldn't have been INVed in reply to a MEMPOOL request,
+        // or when it's too recent to have expired from mapRelay.
+        if ((mempool_req.count() && txinfo.m_time <= mempool_req) || txinfo.m_time <= longlived_mempool_time) {
+            return txinfo.tx;
+        }
+    }
+
+    return {};
+}
+
 void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnman* connman, CTxMemPool& mempool, const std::atomic<bool>& interruptMsgProc) LOCKS_EXCLUDED(cs_main)
 {
     AssertLockNotHeld(cs_main);
@@ -1622,58 +1653,31 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm
     const std::chrono::seconds mempool_req = pfrom->m_tx_relay != nullptr ? pfrom->m_tx_relay->m_last_mempool_req.load()
                                                                           : std::chrono::seconds::min();
 
-    {
-        LOCK(cs_main);
+    // Process as many TX items from the front of the getdata queue as
+    // possible, since they're common and it's efficient to batch process
+    // them.
+    while (it != pfrom->vRecvGetData.end() && (it->type == MSG_TX || it->type == MSG_WITNESS_TX)) {
+        if (interruptMsgProc) return;
+        // The send buffer provides backpressure. If there's no space in
+        // the buffer, pause processing until the next call.
+        if (pfrom->fPauseSend) break;
 
-        // Process as many TX items from the front of the getdata queue as
-        // possible, since they're common and it's efficient to batch process
-        // them.
-        while (it != pfrom->vRecvGetData.end() && (it->type == MSG_TX || it->type == MSG_WITNESS_TX)) {
-            if (interruptMsgProc)
-                return;
-            // The send buffer provides backpressure. If there's no space in
-            // the buffer, pause processing until the next call.
-            if (pfrom->fPauseSend)
-                break;
-
-            const CInv &inv = *it++;
+        const CInv &inv = *it++;
 
-            if (pfrom->m_tx_relay == nullptr) {
-                // Ignore GETDATA requests for transactions from blocks-only peers.
-                continue;
-            }
+        if (pfrom->m_tx_relay == nullptr) {
+            // Ignore GETDATA requests for transactions from blocks-only peers.
+            continue;
+        }
 
-            // Send stream from relay memory
-            bool push = false;
-            auto mi = mapRelay.find(inv.hash);
+        CTransactionRef tx = FindTxForGetData(pfrom, inv.hash, mempool_req, longlived_mempool_time);
+        if (tx) {
             int nSendFlags = (inv.type == MSG_TX ? SERIALIZE_TRANSACTION_NO_WITNESS : 0);
-            if (mi != mapRelay.end()) {
-                connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *mi->second));
-                push = true;
-            } else {
-                auto txinfo = mempool.info(inv.hash);
-                // To protect privacy, do not answer getdata using the mempool when
-                // that TX couldn't have been INVed in reply to a MEMPOOL request,
-                // or when it's too recent to have expired from mapRelay.
-                if (txinfo.tx && (
-                     (mempool_req.count() && txinfo.m_time <= mempool_req)
-                      || (txinfo.m_time <= longlived_mempool_time)))
-                {
-                    connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *txinfo.tx));
-                    push = true;
-                }
-            }
-
-            if (push) {
-                // We interpret fulfilling a GETDATA for a transaction as a
-                // successful initial broadcast and remove it from our
-                // unbroadcast set.
-                mempool.RemoveUnbroadcastTx(inv.hash);
-            } else {
-                vNotFound.push_back(inv);
-            }
+            connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *tx));
+            mempool.RemoveUnbroadcastTx(inv.hash);
+        } else {
+            vNotFound.push_back(inv);
         }
-    } // release cs_main
+    }
 
     // Only process one BLOCK item per call, since they're uncommon and can be
     // expensive to process.