Backport PR14897, PR15834 and PR16196

Summary:
They all are backported at once to avoid leaving master in a buggy state.

This is Core PR14897: bitcoin/bitcoin#14897

* Change in transaction pull scheduling to prevent InvBlock-related attacks

Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>

This is Core PR15834: bitcoin/bitcoin#15834

 * Remove NOTFOUND transactions from in-flight data structures

This prevents a bug where the in-flight queue for our peers will not be
drained, resulting in not downloading any new transactions from our peers.

Thanks to ajtowns for reporting this bug.

 * Add an explicit memory bound to m_tx_process_time

Previously there was an implicit bound based on the handling of m_tx_announced,
but that approach is error-prone (particularly if we start automatically
removing things from that set).

 * Improve NOTFOUND comment

 * Expire old entries from the in-flight tx map

If a peer hasn't responded to a getdata request, eventually time out the request
and remove it from the in-flight data structures.  This is to prevent any bugs in
our handling of those in-flight data structures from filling up the in-flight
map and preventing us from requesting more transactions (such as the NOTFOUND
bug, fixed in a previous commit).

Co-authored-by: Anthony Towns <aj@erisian.com.au>

 * Fix bug around transaction requests

If a transaction is already in-flight when a peer announces a new tx to us, we
schedule a time in the future to reconsider whether to download. At that future
time, there was a bug that would prevent transactions from being rescheduled
for potential download again (ie if the transaction was still in-flight at the
time of reconsideration, such as from some other peer). Fix this.

This is Core PR16196: bitcoin/bitcoin#16196

 * doc: Add release notes for 14897 & 15834

Test Plan:
  make check
  ./test/functional/test_runner.py --extended

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D4574

doc/release-notes.md

@@ -47,3 +47,13 @@ Here are the changes to RPC methods:
 
  - `getlabeladdress` has been removed and replaced with `getaccountaddress`
    until v0.21 at which time `getaccountaddress` will also be removed.  To
+
+Network
+-------
+ - When fetching a transaction announced by multiple peers, previous versions of
+   Bitcoin ABC would sequentially attempt to download the transaction from each
+   announcing peer until the transaction is received, in the order that those
+   peers' announcements were received.  In this release, the download logic has
+   changed to randomize the fetch order across peers and to prefer sending
+   download requests to outbound peers over inbound peers. This fixes an issue
+   where inbound peers can prevent a node from getting a transaction.

src/net.cpp

@@ -93,8 +93,6 @@ CCriticalSection cs_mapLocalHost;
 std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(cs_mapLocalHost);
 static bool vfLimited[NET_MAX] GUARDED_BY(cs_mapLocalHost) = {};
 
-limitedmap<uint256, int64_t> mapAlreadyAskedFor(MAX_INV_SZ);
-
 void CConnman::AddOneShot(const std::string &strDest) {
     LOCK(cs_vOneShots);
     vOneShots.push_back(strDest);
@@ -2706,47 +2704,6 @@ CNode::~CNode() {
     CloseSocket(hSocket);
 }
 
-void CNode::AskFor(const CInv &inv) {
-    if (mapAskFor.size() > MAPASKFOR_MAX_SZ ||
-        setAskFor.size() > SETASKFOR_MAX_SZ) {
-        return;
-    }
-
-    // a peer may not have multiple non-responded queue positions for a single
-    // inv item.
-    if (!setAskFor.insert(inv.hash).second) {
-        return;
-    }
-
-    // We're using mapAskFor as a priority queue, the key is the earliest time
-    // the request can be sent.
-    int64_t nRequestTime;
-    auto it = mapAlreadyAskedFor.find(inv.hash);
-    if (it != mapAlreadyAskedFor.end()) {
-        nRequestTime = it->second;
-    } else {
-        nRequestTime = 0;
-    }
-    LogPrint(BCLog::NET, "askfor %s  %d (%s) peer=%d\n", inv.ToString(),
-             nRequestTime, FormatISO8601DateTime(nRequestTime / 1000000), id);
-
-    // Make sure not to reuse time indexes to keep things in the same order
-    int64_t nNow = GetTimeMicros() - 1000000;
-    static int64_t nLastTime;
-    ++nLastTime;
-    nNow = std::max(nNow, nLastTime);
-    nLastTime = nNow;
-
-    // Each retry is 2 minutes after the last
-    nRequestTime = std::max(nRequestTime + 2 * 60 * 1000000, nNow);
-    if (it != mapAlreadyAskedFor.end()) {
-        mapAlreadyAskedFor.update(it, nRequestTime);
-    } else {
-        mapAlreadyAskedFor.insert(std::make_pair(inv.hash, nRequestTime));
-    }
-    mapAskFor.insert(std::make_pair(nRequestTime, inv));
-}
-
 bool CConnman::NodeFullyConnected(const CNode *pnode) {
     return pnode && pnode->fSuccessfullyConnected && !pnode->fDisconnect;
 }

src/net.h

@@ -77,10 +77,6 @@ static const bool DEFAULT_UPNP = USE_UPNP;
 #else
 static const bool DEFAULT_UPNP = false;
 #endif
-/** The maximum number of entries in mapAskFor */
-static const size_t MAPASKFOR_MAX_SZ = MAX_INV_SZ;
-/** The maximum number of entries in setAskFor (larger due to getdata latency)*/
-static const size_t SETASKFOR_MAX_SZ = 2 * MAX_INV_SZ;
 /** The maximum number of peer connections to maintain. */
 static const unsigned int DEFAULT_MAX_PEER_CONNECTIONS = 125;
 /** The default for -maxuploadtarget. 0 = Unlimited */
@@ -506,8 +502,6 @@ extern bool fDiscover;
 extern bool fListen;
 extern bool fRelayTxes;
 
-extern limitedmap<uint256, int64_t> mapAlreadyAskedFor;
-
 struct LocalServiceInfo {
     int nScore;
     int nPort;
@@ -711,8 +705,6 @@ class CNode {
     // requested.
     std::vector<uint256> vInventoryBlockToSend GUARDED_BY(cs_inventory);
     CCriticalSection cs_inventory;
-    std::set<uint256> setAskFor;
-    std::multimap<int64_t, CInv> mapAskFor;
     int64_t nNextInvSend{0};
     // Used for headers announcements - unfiltered blocks to relay.
     std::vector<uint256> vBlockHashesToAnnounce GUARDED_BY(cs_inventory);
@@ -838,8 +830,6 @@ class CNode {
         vBlockHashesToAnnounce.push_back(hash);
     }
 
-    void AskFor(const CInv &inv);
-
     void CloseSocketDisconnect();
 
     void copyStats(CNodeStats &stats);