Merge #2118: [NET] Inv, mempool and validation improvements

9b9c616 Fix missing zapwallettxes mode in wallet_hd.py functional test (furszy)
d6d0ad9 [logs] fix zapwallettxes startup logs (John Newbery)
006c503 [wallet] fix zapwallettxes interaction with persistent mempool (John Newbery)
c6d45c6 Adapting and connecting mempool_persist.py functional test to the test runner. (furszy)
4f26a4e Control mempool persistence using a command line parameter. (John Newbery)
5d949de [Qt] Do proper shutdown (Jonas Schnelli)
e60da98 Allow shutdown during LoadMempool, dump only when necessary (Jonas Schnelli)
c0a0e81 Moving TxMempoolInfo tx member to CTransactionRef (furszy)
f0c2255 Add mempool.dat to doc/files.md (furszy)
8e52226 Add DumpMempool and LoadMempool (Pieter Wuille)
44c635d Add AcceptToMemoryPoolWithTime function (Pieter Wuille)
6bbc6a9 Add feedelta to TxMempoolInfo (Pieter Wuille)
9979f3d [mempool] move removed and conflicts transaction ref list to vector. (furszy)
4f672c2 Make removed and conflicted arguments optional to remove (Pieter Wuille)
e51c4b8 Bypass removeRecursive in removeForReorg (Pieter Wuille)
54cf7c0 Get rid of CTxMempool::lookup() entirely (furszy)
35bc2a9 Finished the switch CTransaction storage in mempool to CTransactionRef and introduced the timeLastMempoolReq coming from btc#8080. (furszy)
d10583b An adapted version of btc@b5599147533103efea896a1fc4ff51f2d3ad5808 (furszy)
cb4fc6c An adapted version of btc@ed7068302c7490e8061cb3a558a0f83a465beeea (furszy)
9645775 Split up and optimize transaction and block inv queues (furszy)
68bc68f Don't do mempool lookups for "mempool" command without a filter (furszy)
7624823 mapNextTx: use pointer as key, simplify value (furszy)
191c62e Return mempool queries in dependency order (Pieter Wuille)
23c9f3e Eliminate TX trickle bypass, sort TX invs for privacy and priority. (furszy)
6ebfd17 tiny test fix for mempool_tests (Alex Morcos)
8c0016e Check all ancestor state in CTxMemPool::check() (furszy)
91c6096 Add ancestor feerate index to mempool (Suhas Daftuar)
64e84e2 Add ancestor tracking to mempool  This implements caching of ancestor state to each mempool entry, similar to descendant tracking, but also including caching sigops-with-ancestors (as that metric will be helpful to future code that implements better transaction selection in CreatenewBlock). (furszy)
8325bb4 Fix mempool limiting for PrioritiseTransaction Redo the feerate index to be based on mining score, rather than fee. (furszy)
1fa40ac Remove work limit in UpdateForDescendants()  The work limit served to prevent the descendant walking algorithm from doing too much work by marking the parent transaction as dirty. However to implement ancestor tracking, it's not possible to similarly mark those descendant transactions as dirty without having to calculate them to begin with.  This commit removes the work limit altogether. With appropriate chain limits (-limitdescendantcount) the concern about doing too much work inside this function should be mitigated. (furszy)
ba32375 Rename CTxMemPool::remove -> removeRecursive  remove is no longer called non-recursively, so simplify the logic and eliminate an unnecessary parameter (furszy)
c30fa16 CTxMemPool::removeForBlock now uses RemoveStaged (furszy)

Pull request description:

  Ending up 2020 with a large PR :).

  Included a good number of performance and privacy improvements over the mempool and inv processing/sending areas + added mempool cache dump/load.
  Almost finishing with #1726 work, getting closer to 9725, and getting closer to be able to decouple the message processing thread <-> wallet and the wallet <-> GUI locks dependencies (which will be another long story.. but well, step by step).

  The final goal is clear, a much faster syncing process using pivx-qt (a good speed up for pivxd as well), smoother visual navigation when big wallets are syncing, less thread synchronization issues, among all of the improvements that will be coming with the backports adaptations.

  Adapted the following PRs to our sources:

  bitcoin#7062 —> only eb30666.
  bitcoin#7174 —> complete.
  bitcoin#7562 —> only c5d746a.
  bitcoin#7594 —> complete.
  bitcoin#7840 —> complete.
  bitcoin#7997 —> complete.
  bitcoin#8080 —> complete
  bitcoin#8126 —> except e9b4780 (we don't have the mapRelay) and c2a4724 (we don't have the relay expiration vector).
  bitcoin#8448 —> complete
  bitcoin#8515 —> complete
  bitcoin#9408 —> complete
  bitcoin#9966 —> complete
  bitcoin#10330 —> complete

ACKs for top commit:
  random-zebra:
    ACK 9b9c616
  Fuzzbawls:
    ACK 9b9c616

Tree-SHA512: 186bd09bbb19b55ec0d46dc27291663a0df2d60d8238c661a8c9b8cbf3677b6f8a92fa56bd7346a3cb5293712eeccc5d6542ee3c441d35a4f61e7d12e2ce489a

CMakeLists.txt

@@ -193,6 +193,7 @@ set(SERVER_SOURCES
         ./src/checkpoints.cpp
         ./src/httprpc.cpp
         ./src/httpserver.cpp
+        ./src/indirectmap.h
         ./src/init.cpp
         ./src/interfaces/handler.cpp
         ./src/interfaces/wallet.cpp

doc/files.md

@@ -11,6 +11,7 @@ database/*          | BDB database environment; only used for wallet since 0.8.0
 db.log              | wallet database log file; moved to wallets/ directory on new installs since 0.16.0
 debug.log           | contains debug information and general logging generated by pivxd or pivx-qt
 fee_estimates.dat   | stores statistics used to estimate minimum transaction fees and priorities required for confirmation; since 0.10.0
+mempool.dat         | dump of the mempool's transactions; since 5.0.2
 budget.dat          | stores data for budget objects
 masternode.conf     | contains configuration settings for remote masternodes
 mncache.dat         | stores data for masternode list

src/Makefile.am

@@ -189,6 +189,7 @@ BITCOIN_CORE_H = \
   hash.h \
   httprpc.h \
   httpserver.h \
+  indirectmap.h \
   init.h \
   interfaces/handler.h \
   interfaces/wallet.h \

src/indirectmap.h

@@ -0,0 +1,58 @@
+// Copyright (c) 2016-2020 The Bitcoin developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_INDIRECTMAP_H
+#define BITCOIN_INDIRECTMAP_H
+
+#include <map>
+
+template <class T>
+struct DereferencingComparator { bool operator()(const T a, const T b) const { return *a < *b; } };
+
+/* Map whose keys are pointers, but are compared by their dereferenced values.
+ *
+ * Differs from a plain std::map<const K*, T, DereferencingComparator<K*> > in
+ * that methods that take a key for comparison take a K rather than taking a K*
+ * (taking a K* would be confusing, since it's the value rather than the address
+ * of the object for comparison that matters due to the dereferencing comparator).
+ *
+ * Objects pointed to by keys must not be modified in any way that changes the
+ * result of DereferencingComparator.
+ */
+template <class K, class T>
+class indirectmap {
+private:
+    typedef std::map<const K*, T, DereferencingComparator<const K*> > base;
+    base m;
+public:
+    typedef typename base::iterator iterator;
+    typedef typename base::const_iterator const_iterator;
+    typedef typename base::size_type size_type;
+    typedef typename base::value_type value_type;
+
+    // passthrough (pointer interface)
+    std::pair<iterator, bool> insert(const value_type& value) { return m.insert(value); }
+
+    // pass address (value interface)
+    iterator find(const K& key)                     { return m.find(&key); }
+    const_iterator find(const K& key) const         { return m.find(&key); }
+    iterator lower_bound(const K& key)              { return m.lower_bound(&key); }
+    const_iterator lower_bound(const K& key) const  { return m.lower_bound(&key); }
+    size_type erase(const K& key)                   { return m.erase(&key); }
+    size_type count(const K& key) const             { return m.count(&key); }
+
+    // passthrough
+    bool empty() const              { return m.empty(); }
+    size_type size() const          { return m.size(); }
+    size_type max_size() const      { return m.max_size(); }
+    void clear()                    { m.clear(); }
+    iterator begin()                { return m.begin(); }
+    iterator end()                  { return m.end(); }
+    const_iterator begin() const    { return m.begin(); }
+    const_iterator end() const      { return m.end(); }
+    const_iterator cbegin() const   { return m.cbegin(); }
+    const_iterator cend() const     { return m.cend(); }
+};
+
+#endif // BITCOIN_INDIRECTMAP_H

src/init.cpp

@@ -155,6 +155,7 @@ CClientUIInterface uiInterface;  // Declared but not defined in guiinterface.h
 //
 
 volatile bool fRequestShutdown = false;
+std::atomic<bool> fDumpMempoolLater(false);
 
 void StartShutdown()
 {
@@ -245,6 +246,9 @@ void PrepareShutdown()
     DumpBudgets(g_budgetman);
     DumpMasternodePayments();
     UnregisterNodeSignals(GetNodeSignals());
+    if (fDumpMempoolLater && gArgs.GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) {
+        DumpMempool();
+    }
 
     // After everything has been shut down, but before things get flushed, stop the
     // CScheduler/checkqueue threadGroup
@@ -418,6 +422,7 @@ std::string HelpMessage(HelpMessageMode mode)
     strUsage += HelpMessageOpt("-maxorphantx=<n>", strprintf(_("Keep at most <n> unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS));
     strUsage += HelpMessageOpt("-maxmempool=<n>", strprintf(_("Keep the transaction memory pool below <n> megabytes (default: %u)"), DEFAULT_MAX_MEMPOOL_SIZE));
     strUsage += HelpMessageOpt("-mempoolexpiry=<n>", strprintf(_("Do not keep transactions in the mempool longer than <n> hours (default: %u)"), DEFAULT_MEMPOOL_EXPIRY));
+    strUsage += HelpMessageOpt("-persistmempool", strprintf(_("Whether to save the mempool on shutdown and load on restart (default: %u)"), DEFAULT_PERSIST_MEMPOOL));
     strUsage += HelpMessageOpt("-par=<n>", strprintf(_("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)"), -GetNumCores(), MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_THREADS));
 #ifndef WIN32
     strUsage += HelpMessageOpt("-pid=<file>", strprintf(_("Specify pid file (default: %s)"), PIVX_PID_FILENAME));
@@ -700,6 +705,11 @@ void ThreadImport(std::vector<fs::path> vImportFiles)
         LogPrintf("Stopping after block import\n");
         StartShutdown();
     }
+
+    if (gArgs.GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) {
+        LoadMempool();
+        fDumpMempoolLater = !fRequestShutdown;
+    }
 }
 
 /** Sanity checks
@@ -899,10 +909,16 @@ void InitParameterInteraction()
             LogPrintf("%s : parameter interaction: -salvagewallet=1 -> setting -rescan=1\n", __func__);
     }
 
-    // -zapwallettx implies a rescan
-    if (gArgs.GetBoolArg("-zapwallettxes", false)) {
+    int zapwallettxes = gArgs.GetArg("-zapwallettxes", 0);
+    // -zapwallettxes implies dropping the mempool on startup
+    if (zapwallettxes != 0 && gArgs.SoftSetBoolArg("-persistmempool", false)) {
+        LogPrintf("%s: parameter interaction: -zapwallettxes=%s -> setting -persistmempool=0\n", __func__, zapwallettxes);
+    }
+
+    // -zapwallettxes implies a rescan
+    if (zapwallettxes != 0) {
         if (gArgs.SoftSetBoolArg("-rescan", true))
-            LogPrintf("%s : parameter interaction: -zapwallettxes=<mode> -> setting -rescan=1\n", __func__);
+            LogPrintf("%s : parameter interaction: -zapwallettxes=%s -> setting -rescan=1\n", __func__, zapwallettxes);
     }
 }
 

src/memusage.h

@@ -5,6 +5,7 @@
 #ifndef BITCOIN_MEMUSAGE_H
 #define BITCOIN_MEMUSAGE_H
 
+#include "indirectmap.h"
 #include "prevector.h"
 
 #include <stdlib.h>
@@ -185,6 +186,20 @@ static inline size_t DynamicUsage(const std::shared_ptr<X>& p)
     return p ? MallocUsage(sizeof(X)) + MallocUsage(sizeof(stl_shared_counter)) : 0;
 }
 
+// indirectmap has underlying map with pointer as key
+
+template<typename X, typename Y>
+static inline size_t DynamicUsage(const indirectmap<X, Y>& m)
+{
+    return MallocUsage(sizeof(stl_tree_node<std::pair<const X*, Y> >)) * m.size();
+}
+
+template<typename X, typename Y>
+static inline size_t IncrementalDynamicUsage(const indirectmap<X, Y>& m)
+{
+    return MallocUsage(sizeof(stl_tree_node<std::pair<const X*, Y> >));
+}
+
 // Boost data structures
 
 template<typename X>

src/net.cpp

@@ -2438,12 +2438,14 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
     hashContinue = UINT256_ZERO;
     nStartingHeight = -1;
     filterInventoryKnown.reset();
+    fSendMempool = false;
     fGetAddr = false;
     nNextLocalAddrSend = 0;
     nNextAddrSend = 0;
     nNextInvSend = 0;
     fRelayTxes = false;
     pfilter = new CBloomFilter();
+    timeLastMempoolReq = 0;
     nPingNonceSent = 0;
     nPingUsecStart = 0;
     nPingUsecTime = 0;

src/net.h

@@ -580,7 +580,7 @@ class CNode
     // a) it allows us to not relay tx invs before receiving the peer's version message
     // b) the peer may tell us in their version message that we should not relay tx invs
     //    until they have initialized their bloom filter.
-    bool fRelayTxes;
+    bool fRelayTxes; //protected by cs_filter
     CSemaphoreGrant grantOutbound;
     RecursiveMutex cs_filter;
     CBloomFilter* pfilter;
@@ -610,11 +610,24 @@ class CNode
 
     // inventory based relay
     CRollingBloomFilter filterInventoryKnown;
-    std::vector<CInv> vInventoryToSend;
+    // Set of transaction ids we still have to announce.
+    // They are sorted by the mempool before relay, so the order is not important.
+    std::set<uint256> setInventoryTxToSend;
+    // List of block ids we still have announce.
+    // There is no final sorting before sending, as they are always sent immediately
+    // and in the order requested.
+    std::vector<uint256> vInventoryBlockToSend;
+    // Set of tier two messages ids we still have to announce.
+    std::vector<CInv> vInventoryTierTwoToSend;
     RecursiveMutex cs_inventory;
     std::multimap<int64_t, CInv> mapAskFor;
     std::vector<uint256> vBlockRequested;
     int64_t nNextInvSend;
+    // Used for BIP35 mempool sending, also protected by cs_inventory
+    bool fSendMempool;
+
+    // Last time a "MEMPOOL" request was serviced.
+    std::atomic<int64_t> timeLastMempoolReq{0};
 
     // Ping time measurement:
     // The pong reply we're expecting, or 0 if no pong expected.
@@ -735,11 +748,15 @@ class CNode
 
     void PushInventory(const CInv& inv)
     {
-        {
-            LOCK(cs_inventory);
-            if (inv.type == MSG_TX && filterInventoryKnown.contains(inv.hash))
-                return;
-            vInventoryToSend.push_back(inv);
+        LOCK(cs_inventory);
+        if (inv.type == MSG_TX) {
+            if (!filterInventoryKnown.contains(inv.hash)) {
+                setInventoryTxToSend.insert(inv.hash);
+            }
+        } else if (inv.type == MSG_BLOCK) {
+            vInventoryBlockToSend.push_back(inv.hash);
+        } else {
+            vInventoryTierTwoToSend.emplace_back(inv);
         }
     }