Refactor ChainTip wallet locks

src/main.h

@@ -113,6 +113,10 @@ static const unsigned int DATABASE_WRITE_INTERVAL = 60 * 60;
 static const unsigned int DATABASE_FLUSH_INTERVAL = 24 * 60 * 60;
 /** Maximum length of reject messages. */
 static const unsigned int MAX_REJECT_MESSAGE_LENGTH = 111;
+/** Time to wait (in seconds) between writing wallet witness data to disk. */
+static const unsigned int WITNESS_WRITE_INTERVAL = 10 * 60;
+/** Number of updates between writing wallet witness data to disk. */
+static const unsigned int WITNESS_WRITE_UPDATES = 10000;
 static const int64_t DEFAULT_MAX_TIP_AGE = 24 * 60 * 60;
 
 // NSPV enabled?

src/wallet/rpcwallet.cpp

@@ -2335,7 +2335,7 @@ UniValue keypoolrefill(const UniValue& params, bool fHelp, const CPubKey& mypk)
 
 static void LockWallet(CWallet* pWallet)
 {
-    LOCK(cs_nWalletUnlockTime);
+    LOCK2(pWallet->cs_wallet, cs_nWalletUnlockTime);
     nWalletUnlockTime = 0;
     pWallet->Lock();
 }

src/wallet/wallet.cpp

@@ -472,6 +472,7 @@ void CWallet::ChainTip(const CBlockIndex *pindex,
                        const CBlock *pblock,
                        boost::optional<std::pair<SproutMerkleTree, SaplingMerkleTree>> added)
 {
+    LOCK2(cs_main, cs_wallet);
     if (added) {
         bool initialDownloadCheck = IsInitialBlockDownload();
         // Prevent witness cache building && consolidation transactions
@@ -504,13 +505,28 @@ void CWallet::ChainTip(const CBlockIndex *pindex,
         DecrementNoteWitnesses(pindex);
         UpdateNullifierNoteMapForBlock(pblock);
     }
+
+    // SetBestChain() can be expensive for large wallets, so do only
+    // this sometimes; the wallet state will be brought up to date
+    // during rescanning on startup.
+    int64_t nNow = GetTimeMicros();
+    if (nLastSetChain == 0) {
+        // Don't flush during startup.
+        nLastSetChain = nNow;
+    }
+    // TODO: SetBestChain if DeleteWalletTransactions deletes txs
+    if (++nSetChainUpdates >= WITNESS_WRITE_UPDATES || nLastSetChain + (int64_t)WITNESS_WRITE_INTERVAL * 1000000 < nNow) {
+        nLastSetChain = nNow;
+        nSetChainUpdates = 0;
+        // The locator must be derived from the pindex used to increment
+        // the witnesses above; pindex can be behind chainActive.Tip().
+        CBlockLocator loc = chainActive.GetLocator(pindex);
+        SetBestChain(loc);
+        LogPrintf("%s: wrote bestchain to wallet at height=%d\n", __func__, pindex->GetHeight() );
+    }
 }
 
 void CWallet::RunSaplingSweep(int blockHeight) {
-    // Sapling is always active since height=1 of HUSH+HSCs
-    // if (!NetworkUpgradeActive(blockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) {
-    //     return;
-    // }
     AssertLockHeld(cs_wallet);
     if (!fSweepEnabled) {
         return;
@@ -548,12 +564,7 @@ void CWallet::RunSaplingSweep(int blockHeight) {
 }
 
 void CWallet::RunSaplingConsolidation(int blockHeight) {
-    // Sapling is always active on HUSH+HSCs
-    //if (!NetworkUpgradeActive(blockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) {
-    //    return;
-    //}
-
-    LOCK(cs_wallet);
+    AssertLockHeld(cs_wallet);
 
     if (!fSaplingConsolidationEnabled) {
         return;
@@ -592,6 +603,7 @@ bool CWallet::CommitAutomatedTx(const CTransaction& tx) {
 
 void CWallet::SetBestChain(const CBlockLocator& loc)
 {
+    AssertLockHeld(cs_wallet);
     CWalletDB walletdb(strWalletFile);
     SetBestChainINTERNAL(walletdb, loc);
 }
@@ -974,7 +986,7 @@ void CWallet::ClearNoteWitnessCache()
 
 void CWallet::DecrementNoteWitnesses(const CBlockIndex* pindex)
 {
-    LOCK(cs_wallet);
+    AssertLockHeld(cs_wallet);
 
     extern int32_t HUSH_REWIND;
 
@@ -1016,7 +1028,8 @@ int CWallet::SaplingWitnessMinimumHeight(const uint256& nullifier, int nWitnessH
 
 int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessOnly)
 {
-  LOCK2(cs_main, cs_wallet);
+  AssertLockHeld(cs_main);
+  AssertLockHeld(cs_wallet);
 
   int nWitnessTxIncrement = 0;
   int nWitnessTotalTxCount = mapWallet.size();
@@ -1131,8 +1144,8 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO
 
 void CWallet::BuildWitnessCache(const CBlockIndex* pindex, bool witnessOnly)
 {
-
-  LOCK2(cs_main, cs_wallet);
+  AssertLockHeld(cs_main);
+  AssertLockHeld(cs_wallet);
 
   int startHeight = VerifyAndSetInitialWitness(pindex, witnessOnly) + 1;
 
@@ -1758,7 +1771,8 @@ void CWallet::RescanWallet()
  */
 std::pair<mapSaplingNoteData_t, SaplingIncomingViewingKeyMap> CWallet::FindMySaplingNotes(const CTransaction &tx) const
 {
-    LOCK(cs_SpendingKeyStore);
+    // LOCK(cs_SpendingKeyStore);
+    LOCK(cs_wallet);
     uint256 hash = tx.GetHash();
     uint32_t nZouts = tx.vShieldedOutput.size();
     if(fDebug && (nZouts > 0)) {
@@ -2428,7 +2442,8 @@ bool CWalletTx::WriteToDisk(CWalletDB *pwalletdb)
 
 void CWallet::ReorderWalletTransactions(std::map<std::pair<int,int>, CWalletTx*> &mapSorted, int64_t &maxOrderPos)
 {
-    LOCK2(cs_main, cs_wallet);
+    AssertLockHeld(cs_main);
+    AssertLockHeld(cs_wallet);
     if(fZdebug)
         fprintf(stderr,"%s: maxOrderPos=%li\n",__func__, maxOrderPos);
 
@@ -2453,11 +2468,11 @@ void CWallet::ReorderWalletTransactions(std::map<std::pair<int,int>, CWalletTx*>
     if(fZdebug)
         fprintf(stderr,"%s: mapSorted.size=%lu\n",__func__, mapSorted.size());
 }
- /**Update the nOrderPos with passed in ordered map.
- */
 
+ /**Update the nOrderPos with passed in ordered map.  */
 void CWallet::UpdateWalletTransactionOrder(std::map<std::pair<int,int>, CWalletTx*> &mapSorted, bool resetOrder) {
-  LOCK2(cs_main, cs_wallet);
+  AssertLockHeld(cs_main);
+  AssertLockHeld(cs_wallet);
 
   int64_t previousPosition = 0;
   std::map<const uint256, CWalletTx*> mapUpdatedTxs;
@@ -2504,11 +2519,9 @@ void CWallet::UpdateWalletTransactionOrder(std::map<std::pair<int,int>, CWalletT
   LogPrintf("%s: Total Transactions Reordered %i, Next Position %i\n ", __func__, mapUpdatedTxs.size(), nOrderPosNext);
 }
 
-/**
- * Delete transactions from the Wallet
- */
+// Delete transactions from the Wallet
 void CWallet::DeleteTransactions(std::vector<uint256> &removeTxs) {
-    LOCK(cs_wallet);
+    AssertLockHeld(cs_wallet);
 
     int numTx = removeTxs.size();
     if(fZdebug)
@@ -2537,7 +2550,8 @@ void CWallet::DeleteTransactions(std::vector<uint256> &removeTxs) {
 }
 
 void CWallet::DeleteWalletTransactions(const CBlockIndex* pindex) {
-      LOCK2(cs_main, cs_wallet);
+      AssertLockHeld(cs_main);
+      AssertLockHeld(cs_wallet);
 
       int nDeleteAfter = (int)fDeleteTransactionsAfterNBlocks;
       bool runCompact = false;

src/wallet/wallet.h

@@ -764,6 +764,10 @@ private:
     int64_t nNextResend;
     int64_t nLastResend;
     bool fBroadcastTransactions;
+    // The time we last did a SetBestChain
+    int64_t nLastSetChain;
+    // How many chain tips we have added since the last time we did a SetBestChain
+    int nSetChainUpdates;
 
     template <class T>
     using TxSpendMap = std::multimap<T, uint256>;

src/wallet/walletdb.cpp

@@ -855,13 +855,13 @@ static bool IsKeyType(string strType)
 
 DBErrors CWalletDB::LoadWallet(CWallet* pwallet)
 {
+    LOCK2(cs_main, pwallet->cs_wallet);
     pwallet->vchDefaultKey = CPubKey();
     CWalletScanState wss;
     bool fNoncriticalErrors = false;
     DBErrors result = DB_LOAD_OK;
 
     try {
-        LOCK(pwallet->cs_wallet);
         int nMinVersion = 0;
         if (Read((string)"minversion", nMinVersion))
         {