Merge #2150: Revamping block validation interface interaction with wa…

…llet

98d770f CScheduler boost->std::function, use millisecs for times, not secs (Matt Corallo)
67e068c Remove now unneeded ChainTip signal (furszy)
bcdd3e9 Move ChainTip sapling update witnesses and nullifiers to BlockConnected/BlockDisconnected. (furszy)
b799070 Take a CTransactionRef in AddToWalletIfInvolvingMe to avoid a copy (Matt Corallo)
d77244c Remove dead-code tracking of requests for blocks we generated (Matt Corallo)
10ccbbf Hold cs_wallet for whole block [dis]connection processing (Matt Corallo)
1a45036 fix compiler warning member functions not marked as 'override' (furszy)
d3867a7 An adaptation of Corallo's btc@461e49fee2935b1eb4d4ea7bae3023e655c0a6d8 (Matt Corallo)
f5ac648 [Refactor] Move Sapling ChainTip signal notification loop to use the latest connectTrace class structure (furszy)
8704d9d Handle SyncTransaction in ActivateBestChain instead of ConnectTrace (Matt Corallo)
6dcb6b0 Keep conflictedTxs in ConnectTrace per-block (Matt Corallo)
50d3e0e Handle conflicted transactions directly in ConnectTrace (furszy)
27fb897 Make ConnectTrace::blocksConnected private, hide behind accessors (Matt Corallo)
60329da Add pblock to connectTrace at the end of ConnectTip, not start (Matt Corallo)
e7c2789 Include missing #include in zmqnotificationinterface.h (Matt Corallo)
1b396b8 Move `NotifyEntryRemoved` to use the general interfaces::MakeHandler (furszy)
4cb5820 Better document usage of SyncTransaction (Alex Morcos)
21be4e2 Introduce MemPoolConflictRemovalTracker (Alex Morcos)
7326acb mempool: add notification for added/removed entries (Wladimir J. van der Laan)
a8605d2 Clean up tx prioritization when conflict mined (Casey Rodarmor)
e7db9ff remove internal tracking of mempool conflicts for reporting to wallet (Alex Morcos)
76c72c6 remove external usage of mempool conflict tracking (Alex Morcos)
ef429af tests: Stop node before removing the notification file (furszy)
15a21c2 tests: write the notification to different files to avoid race condition (Chun Kuan Lee)
466e97a [Wallet] Simplify InMempool (furszy)
85e18f0 Rename FlushWalletDB -> CompactWalletDB, add function description (Matt Corallo)
00f36ea Use CScheduler for wallet flushing, remove ThreadFlushWalletDB (Matt Corallo)

Pull request description:

  Revamped the validation interface interaction with the wallet, encapsulating and improving the mempool and block signaling and each of the wallet signals handler.
  Restructured the Sapling witnesses and nullifiers update under the new signals.
  Solved several bugs that found on the way as well (Check each commit description).

  This PR concludes with #1726 long road :). Pushing our repository around 2 years ahead in btc time in the mempool and validation interface areas (without including RBF).
  The new validation -> wallet interaction architecture will enable further, and much more user facing important, improvements for the syncing process, overall software responsiveness and multithreading locking issues.

  Adapted backports:
  bitcoin#6464 --> Always clean up manual transaction prioritization (mempool)
  bitcoin#9240 --> Remove txConflicted (mempool)
  bitcoin#9371 --> Notify on removal (mempool)
  bitcoin#9605 --> Use CScheduler for wallet flushing, remove ThreadFlushWalletDB (walletdb)
  bitcoin#9725 --> CValidationInterface Cleanups (wallet, validation and validation interface)

ACKs for top commit:
  random-zebra:
    utACK 98d770f
  Fuzzbawls:
    utACK 98d770f

Tree-SHA512: 84c86567c2bff36b859b2ae73c558956a70dff0fffb4f73208708d92165b851bf42d35246410238c66c7a4b77e5bf51ec93885234a75fa48901fd182d2f70a28

src/init.cpp

@@ -1908,7 +1908,7 @@ bool AppInitMain()
 
 #ifdef ENABLE_WALLET
     if (pwalletMain) {
-        pwalletMain->postInitProcess(threadGroup);
+        pwalletMain->postInitProcess(scheduler);
 
         // StakeMiner thread disabled by default on regtest
         if (gArgs.GetBoolArg("-staking", !Params().IsRegTestNet() && DEFAULT_STAKING)) {

src/miner.cpp

@@ -521,9 +521,6 @@ bool ProcessBlockFound(const std::shared_ptr<const CBlock>& pblock, CWallet& wal
     if (reservekey)
         reservekey->KeepKey();
 
-    // Inform about the new block
-    GetMainSignals().BlockFound(pblock->GetHash());
-
     // Process this block the same as if we had received it from another node
     CValidationState state;
     if (!ProcessNewBlock(state, nullptr, pblock, nullptr)) {

src/net.cpp

@@ -2142,7 +2142,7 @@ bool CConnman::Start(CScheduler& scheduler, std::string& strNodeError, Options c
     threadMessageHandler = std::thread(&TraceThread<std::function<void()> >, "msghand", std::function<void()>(std::bind(&CConnman::ThreadMessageHandler, this)));
 
     // Dump network addresses
-    scheduler.scheduleEvery(std::bind(&CConnman::DumpData, this), DUMP_ADDRESSES_INTERVAL);
+    scheduler.scheduleEvery(std::bind(&CConnman::DumpData, this), DUMP_ADDRESSES_INTERVAL * 1000);
 
     return true;
 }

src/net_processing.h

@@ -41,8 +41,8 @@ class PeerLogicValidation : public CValidationInterface {
     PeerLogicValidation(CConnman* connmanIn);
     ~PeerLogicValidation() = default;
 
-    virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload);
-    virtual void BlockChecked(const CBlock& block, const CValidationState& state);
+    void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override;
+    void BlockChecked(const CBlock& block, const CValidationState& state) override;
 };
 
 struct CNodeStateStats {

src/sapling/saplingscriptpubkeyman.cpp

@@ -300,11 +300,11 @@ void DecrementNoteWitnesses(NoteDataMap& noteDataMap, int indexHeight, int64_t n
     }
 }
 
-void SaplingScriptPubKeyMan::DecrementNoteWitnesses(const CBlockIndex* pindex)
+void SaplingScriptPubKeyMan::DecrementNoteWitnesses(int nChainHeight)
 {
     LOCK(wallet->cs_wallet);
     for (std::pair<const uint256, CWalletTx>& wtxItem : wallet->mapWallet) {
-        ::DecrementNoteWitnesses(wtxItem.second.mapSaplingNoteData, pindex->nHeight, nWitnessCacheSize);
+        ::DecrementNoteWitnesses(wtxItem.second.mapSaplingNoteData, nChainHeight, nWitnessCacheSize);
     }
     nWitnessCacheSize -= 1;
     nWitnessCacheNeedsUpdate = true;

src/sapling/saplingscriptpubkeyman.h

@@ -72,7 +72,7 @@ class SaplingNoteData
      * Block height corresponding to the most current witness.
      *
      * When we first create a SaplingNoteData in SaplingScriptPubKeyMan::FindMySaplingNotes, this is set to
-     * -1 as a placeholder. The next time CWallet::ChainTip is called, we can
+     * -1 as a placeholder. The next time CWallet::BlockConnected/CWallet::BlockDisconnected is called, we can
      * determine what height the witness cache for this note is valid for (even
      * if no witnesses were cached), and so can set the correct value in
      * SaplingScriptPubKeyMan::IncrementNoteWitnesses and SaplingScriptPubKeyMan::DecrementNoteWitnesses.
@@ -163,9 +163,9 @@ class SaplingScriptPubKeyMan {
                                 const CBlock* pblock,
                                 SaplingMerkleTree& saplingTree);
     /**
-     * pindex is the old tip being disconnected.
+     * nChainHeight is the old tip height being disconnected.
      */
-    void DecrementNoteWitnesses(const CBlockIndex* pindex);
+    void DecrementNoteWitnesses(int nChainHeight);
 
     /**
      * Update mapSaplingNullifiersToNotes

src/scheduler.cpp

@@ -93,20 +93,20 @@ void CScheduler::schedule(CScheduler::Function f, boost::chrono::system_clock::t
     newTaskScheduled.notify_one();
 }
 
-void CScheduler::scheduleFromNow(CScheduler::Function f, int64_t deltaSeconds)
+void CScheduler::scheduleFromNow(CScheduler::Function f, int64_t deltaMilliSeconds)
 {
-    schedule(f, boost::chrono::system_clock::now() + boost::chrono::seconds(deltaSeconds));
+    schedule(f, boost::chrono::system_clock::now() + boost::chrono::milliseconds(deltaMilliSeconds));
 }
 
-static void Repeat(CScheduler* s, CScheduler::Function f, int64_t deltaSeconds)
+static void Repeat(CScheduler* s, CScheduler::Function f, int64_t deltaMilliSeconds)
 {
     f();
-    s->scheduleFromNow(std::bind(&Repeat, s, f, deltaSeconds), deltaSeconds);
+    s->scheduleFromNow(std::bind(&Repeat, s, f, deltaMilliSeconds), deltaMilliSeconds);
 }
 
-void CScheduler::scheduleEvery(CScheduler::Function f, int64_t deltaSeconds)
+void CScheduler::scheduleEvery(CScheduler::Function f, int64_t deltaMilliSeconds)
 {
-    scheduleFromNow(std::bind(&Repeat, this, f, deltaSeconds), deltaSeconds);
+    scheduleFromNow(std::bind(&Repeat, this, f, deltaMilliSeconds), deltaMilliSeconds);
 }
 
 size_t CScheduler::getQueueInfo(boost::chrono::system_clock::time_point &first,

src/scheduler.h

@@ -44,15 +44,15 @@ class CScheduler
     // Call func at/after time t
     void schedule(Function f, boost::chrono::system_clock::time_point t);
 
-    // Convenience method: call f once deltaSeconds from now
-    void scheduleFromNow(Function f, int64_t deltaSeconds);
+    // Convenience method: call f once deltaMilliSeconds from now
+    void scheduleFromNow(Function f, int64_t deltaMilliSeconds);
 
     // Another convenience method: call f approximately
-    // every deltaSeconds forever, starting deltaSeconds from now.
+    // every deltaMilliSeconds forever, starting deltaMilliSeconds from now.
     // To be more precise: every time f is finished, it
-    // is rescheduled to run deltaSeconds later. If you
+    // is rescheduled to run deltaMilliSeconds later. If you
     // need more accurate scheduling, don't use this method.
-    void scheduleEvery(Function f, int64_t deltaSeconds);
+    void scheduleEvery(Function f, int64_t deltaMilliSeconds);
 
     // To keep things as simple as possible, there is no unschedule.
 

src/test/mempool_tests.cpp

@@ -53,18 +53,18 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
 
 
     CTxMemPool testPool(CFeeRate(0));
-    std::vector<CTransactionRef> removed;
 
     // Nothing in pool, remove should do nothing:
-    testPool.removeRecursive(txParent, &removed);
-    BOOST_CHECK_EQUAL(removed.size(), 0);
+    unsigned int poolSize = testPool.size();
+    testPool.removeRecursive(txParent);
+    BOOST_CHECK_EQUAL(testPool.size(), poolSize);
 
     // Just the parent:
     testPool.addUnchecked(txParent.GetHash(), entry.FromTx(txParent));
-    testPool.removeRecursive(txParent, &removed);
-    BOOST_CHECK_EQUAL(removed.size(), 1);
-    removed.clear();
-    
+    poolSize = testPool.size();
+    testPool.removeRecursive(txParent);
+    BOOST_CHECK_EQUAL(testPool.size(), poolSize - 1);
+
     // Parent, children, grandchildren:
     testPool.addUnchecked(txParent.GetHash(), entry.FromTx(txParent));
     for (int i = 0; i < 3; i++)
@@ -73,19 +73,21 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
         testPool.addUnchecked(txGrandChild[i].GetHash(), entry.FromTx(txGrandChild[i]));
     }
     // Remove Child[0], GrandChild[0] should be removed:
-    testPool.removeRecursive(txChild[0], &removed);
-    BOOST_CHECK_EQUAL(removed.size(), 2);
-    removed.clear();
+    poolSize = testPool.size();
+    testPool.removeRecursive(txChild[0]);
+    BOOST_CHECK_EQUAL(testPool.size(), poolSize - 2);
     // ... make sure grandchild and child are gone:
-    testPool.removeRecursive(txGrandChild[0], &removed);
-    BOOST_CHECK_EQUAL(removed.size(), 0);
-    testPool.removeRecursive(txChild[0], &removed);
-    BOOST_CHECK_EQUAL(removed.size(), 0);
+    poolSize = testPool.size();
+    testPool.removeRecursive(txGrandChild[0]);
+    BOOST_CHECK_EQUAL(testPool.size(), poolSize);
+    poolSize = testPool.size();
+    testPool.removeRecursive(txChild[0]);
+    BOOST_CHECK_EQUAL(testPool.size(), poolSize);
     // Remove parent, all children/grandchildren should go:
-    testPool.removeRecursive(txParent, &removed);
-    BOOST_CHECK_EQUAL(removed.size(), 5);
+    poolSize = testPool.size();
+    testPool.removeRecursive(txParent);
+    BOOST_CHECK_EQUAL(testPool.size(), poolSize - 5);
     BOOST_CHECK_EQUAL(testPool.size(), 0);
-    removed.clear();
 
     // Add children and grandchildren, but NOT the parent (simulate the parent being in a block)
     for (int i = 0; i < 3; i++)
@@ -95,10 +97,10 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest)
     }
     // Now remove the parent, as might happen if a block-re-org occurs but the parent cannot be
     // put into the mempool (maybe because it is non-standard):
-    testPool.removeRecursive(txParent, &removed);
-    BOOST_CHECK_EQUAL(removed.size(), 6);
+    poolSize = testPool.size();
+    testPool.removeRecursive(txParent);
+    BOOST_CHECK_EQUAL(testPool.size(), poolSize - 6);
     BOOST_CHECK_EQUAL(testPool.size(), 0);
-    removed.clear();
 }
 
 template<typename name>
@@ -415,7 +417,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest)
     /* after tx6 is mined, tx7 should move up in the sort */
     std::vector<CTransactionRef> vtx;
     vtx.emplace_back(MakeTransactionRef(tx6));
-    pool.removeForBlock(vtx, 1, nullptr, false);
+    pool.removeForBlock(vtx, 1);
 
     sortedOrder.erase(sortedOrder.begin()+1);
     // Ties are broken by hash

src/txmempool.cpp

@@ -368,13 +368,12 @@ void CTxMemPool::AddTransactionsUpdated(unsigned int n)
     nTransactionsUpdated += n;
 }
 
-
 bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool fCurrentEstimate)
 {
+    NotifyEntryAdded(entry.GetSharedTx());
     // Add to memory pool without checking anything.
     // Used by AcceptToMemoryPool(), which DOES do all the appropriate checks.
     LOCK(cs);
-
     indexed_transaction_set::iterator newit = mapTx.insert(entry).first;
     mapLinks.insert(make_pair(newit, TxLinks()));
 
@@ -433,8 +432,9 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry,
     return true;
 }
 
-void CTxMemPool::removeUnchecked(txiter it)
+void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
 {
+    NotifyEntryRemoved(it->GetSharedTx(), reason);
     AssertLockHeld(cs);
     const CTransaction& tx = it->GetTx();
     for (const CTxIn& txin : tx.vin)
@@ -483,7 +483,7 @@ void CTxMemPool::CalculateDescendants(txiter entryit, setEntries &setDescendants
     }
 }
 
-void CTxMemPool::removeRecursive(const CTransaction& origTx, std::vector<CTransactionRef>* removed)
+void CTxMemPool::removeRecursive(const CTransaction& origTx, MemPoolRemovalReason reason)
 {
     // Remove transaction from memory pool
     {
@@ -510,12 +510,8 @@ void CTxMemPool::removeRecursive(const CTransaction& origTx, std::vector<CTransa
         for (const txiter& it : txToRemove) {
             CalculateDescendants(it, setAllRemoves);
         }
-        if (removed) {
-            for (const txiter& it : setAllRemoves) {
-                removed->emplace_back(it->GetSharedTx());
-            }
-        }
-        RemoveStaged(setAllRemoves, false);
+
+        RemoveStaged(setAllRemoves, false, reason);
     }
 }
 
@@ -547,7 +543,7 @@ void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMem
     for (txiter it : txToRemove) {
         CalculateDescendants(it, setAllRemoves);
     }
-    RemoveStaged(setAllRemoves, false);
+    RemoveStaged(setAllRemoves, false, MemPoolRemovalReason::REORG);
 }
 
 void CTxMemPool::removeWithAnchor(const uint256& invalidRoot)
@@ -574,7 +570,7 @@ void CTxMemPool::removeWithAnchor(const uint256& invalidRoot)
     }
 }
 
-void CTxMemPool::removeConflicts(const CTransaction& tx, std::vector<CTransactionRef>* removed)
+void CTxMemPool::removeConflicts(const CTransaction& tx)
 {
     // Remove transactions which depend on inputs of tx, recursively
     std::list<CTransaction> result;
@@ -584,7 +580,8 @@ void CTxMemPool::removeConflicts(const CTransaction& tx, std::vector<CTransactio
         if (it != mapNextTx.end()) {
             const CTransaction& txConflict = *it->second;
             if (txConflict != tx) {
-                removeRecursive(txConflict, removed);
+                removeRecursive(txConflict, MemPoolRemovalReason::CONFLICT);
+                ClearPrioritisation(txConflict.GetHash());
             }
         }
     }
@@ -595,7 +592,8 @@ void CTxMemPool::removeConflicts(const CTransaction& tx, std::vector<CTransactio
             if (it != mapSaplingNullifiers.end()) {
                 const CTransaction& txConflict = *it->second;
                 if (txConflict != tx) {
-                    removeRecursive(txConflict, removed);
+                    removeRecursive(txConflict, MemPoolRemovalReason::CONFLICT);
+                    ClearPrioritisation(txConflict.GetHash());
                 }
             }
         }
@@ -606,7 +604,7 @@ void CTxMemPool::removeConflicts(const CTransaction& tx, std::vector<CTransactio
  * Called when a block is connected. Removes from mempool and updates the miner fee estimator.
  */
 void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight,
-                                std::vector<CTransactionRef>* conflicts, bool fCurrentEstimate)
+                                bool fCurrentEstimate)
 {
     LOCK(cs);
     std::vector<CTxMemPoolEntry> entries;
@@ -621,9 +619,9 @@ void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigne
         if (it != mapTx.end()) {
             setEntries stage;
             stage.insert(it);
-            RemoveStaged(stage, true);
+            RemoveStaged(stage, true, MemPoolRemovalReason::BLOCK);
         }
-        removeConflicts(*tx, conflicts);
+        removeConflicts(*tx);
         ClearPrioritisation(tx->GetHash());
     }
     // After the txs in the new block have been removed from the mempool, update policy estimates
@@ -1064,12 +1062,12 @@ size_t CTxMemPool::DynamicMemoryUsage() const
             memusage::DynamicUsage(mapSaplingNullifiers);
 }
 
-void CTxMemPool::RemoveStaged(setEntries &stage, bool updateDescendants)
+void CTxMemPool::RemoveStaged(setEntries &stage, bool updateDescendants, MemPoolRemovalReason reason)
 {
     AssertLockHeld(cs);
     UpdateForRemoveFromMempool(stage, updateDescendants);
     for (const txiter& it : stage) {
-        removeUnchecked(it);
+        removeUnchecked(it, reason);
     }
 }
 
@@ -1086,7 +1084,7 @@ int CTxMemPool::Expire(int64_t time)
     for (const txiter& removeit : toremove) {
         CalculateDescendants(removeit, stage);
     }
-    RemoveStaged(stage, false);
+    RemoveStaged(stage, false, MemPoolRemovalReason::EXPIRY);
     return stage.size();
 }
 
@@ -1197,7 +1195,7 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector<COutPoint>* pvNoSpends
             for (txiter it: stage)
                 txn.push_back(it->GetTx());
         }
-        RemoveStaged(stage, false);
+        RemoveStaged(stage, false, MemPoolRemovalReason::SIZELIMIT);
         if (pvNoSpendsRemaining) {
             for (const CTransaction& tx: txn) {
                 for (const CTxIn& txin: tx.vin) {