wallet: Remove WalletDatabase refcounting and enforce only one Batch access the database at a time

src/wallet/bdb.cpp

@@ -808,6 +808,7 @@ void BerkeleyDatabase::AddRef()
     } else {
         m_refcount++;
     }
+    assert(m_refcount <= 1);
 }
 
 void BerkeleyDatabase::RemoveRef()

src/wallet/bdb.h

@@ -113,10 +113,12 @@ class BerkeleyDatabase : public WalletDatabase
      */
     bool Rewrite(const char* pszSkip=nullptr) override;
 
+    //! Counts the number of active database users to be sure that the database is not closed while someone is using it
+    std::atomic<int> m_refcount{0};
     /** Indicate the a new database user has began using the database. */
-    void AddRef() override;
+    void AddRef();
     /** Indicate that database user has stopped using the database and that it could be flushed or closed. */
-    void RemoveRef() override;
+    void RemoveRef();
 
     /** Back up the entire database to a file.
      */

src/wallet/db.h

@@ -110,13 +110,6 @@ class WalletDatabase
     /** Open the database if it is not already opened. */
     virtual void Open() = 0;
 
-    //! Counts the number of active database users to be sure that the database is not closed while someone is using it
-    std::atomic<int> m_refcount{0};
-    /** Indicate the a new database user has began using the database. Increments m_refcount */
-    virtual void AddRef() = 0;
-    /** Indicate that database user has stopped using the database and that it could be flushed or closed. Decrement m_refcount */
-    virtual void RemoveRef() = 0;
-
     /** Rewrite the entire database on disk, with the exception of key pszSkip if non-zero
      */
     virtual bool Rewrite(const char* pszSkip=nullptr) = 0;
@@ -181,8 +174,6 @@ class DummyDatabase : public WalletDatabase
 {
 public:
     void Open() override {};
-    void AddRef() override {}
-    void RemoveRef() override {}
     bool Rewrite(const char* pszSkip=nullptr) override { return true; }
     bool Backup(const std::string& strDest) const override { return true; }
     void Close() override {}

src/wallet/scriptpubkeyman.cpp

@@ -37,7 +37,8 @@ bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestinat
         error = _("Error: Keypool ran out, please call keypoolrefill first").translated;
         return false;
     }
-    LearnRelatedScripts(new_key, type);
+    WalletBatch batch(m_storage.GetDatabase());
+    LearnRelatedScripts(batch, new_key, type);
     dest = GetDestinationForKey(new_key, type);
     return true;
 }
@@ -386,14 +387,13 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
     }
 }
 
-void LegacyScriptPubKeyMan::UpgradeKeyMetadata()
+void LegacyScriptPubKeyMan::UpgradeKeyMetadata(WalletBatch& batch)
 {
     LOCK(cs_KeyStore);
     if (m_storage.IsLocked() || m_storage.IsWalletFlagSet(WALLET_FLAG_KEY_ORIGIN_METADATA)) {
         return;
     }
 
-    std::unique_ptr<WalletBatch> batch = std::make_unique<WalletBatch>(m_storage.GetDatabase());
     for (auto& meta_pair : mapKeyMetadata) {
         CKeyMetadata& meta = meta_pair.second;
         if (!meta.hd_seed_id.IsNull() && !meta.has_key_origin && meta.hdKeypath != "s") { // If the hdKeypath is "s", that's the seed and it doesn't have a key origin
@@ -415,7 +415,7 @@ void LegacyScriptPubKeyMan::UpgradeKeyMetadata()
             // Write meta to wallet
             CPubKey pubkey;
             if (GetPubKey(meta_pair.first, pubkey)) {
-                batch->WriteKeyMetadata(meta, pubkey, true);
+                batch.WriteKeyMetadata(meta, pubkey, true);
             }
         }
     }
@@ -736,11 +736,11 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyWithDB(WalletBatch& batch, const CKey& s
     CScript script;
     script = GetScriptForDestination(PKHash(pubkey));
     if (HaveWatchOnly(script)) {
-        RemoveWatchOnly(script);
+        RemoveWatchOnlyWithDB(batch, script);
     }
     script = GetScriptForRawPubKey(pubkey);
     if (HaveWatchOnly(script)) {
-        RemoveWatchOnly(script);
+        RemoveWatchOnlyWithDB(batch, script);
     }
 
     if (!m_storage.HasEncryptionKeys()) {
@@ -862,6 +862,12 @@ static bool ExtractPubKey(const CScript &dest, CPubKey& pubKeyOut)
 }
 
 bool LegacyScriptPubKeyMan::RemoveWatchOnly(const CScript &dest)
+{
+    WalletBatch batch(m_storage.GetDatabase());
+    return RemoveWatchOnlyWithDB(batch, dest);
+}
+
+bool LegacyScriptPubKeyMan::RemoveWatchOnlyWithDB(WalletBatch& batch, const CScript &dest)
 {
     {
         LOCK(cs_KeyStore);
@@ -876,7 +882,7 @@ bool LegacyScriptPubKeyMan::RemoveWatchOnly(const CScript &dest)
 
     if (!HaveWatchOnly())
         NotifyWatchonlyChanged(false);
-    if (!WalletBatch(m_storage.GetDatabase()).EraseWatchOnly(dest))
+    if (!batch.EraseWatchOnly(dest))
         return false;
 
     return true;
@@ -1217,20 +1223,22 @@ bool LegacyScriptPubKeyMan::NewKeyPool()
     }
     {
         LOCK(cs_KeyStore);
-        WalletBatch batch(m_storage.GetDatabase());
+        {
+            WalletBatch batch(m_storage.GetDatabase());
 
-        for (const int64_t nIndex : setInternalKeyPool) {
-            batch.ErasePool(nIndex);
-        }
-        setInternalKeyPool.clear();
+            for (const int64_t nIndex : setInternalKeyPool) {
+                batch.ErasePool(nIndex);
+            }
+            setInternalKeyPool.clear();
 
-        for (const int64_t nIndex : setExternalKeyPool) {
-            batch.ErasePool(nIndex);
-        }
-        setExternalKeyPool.clear();
+            for (const int64_t nIndex : setExternalKeyPool) {
+                batch.ErasePool(nIndex);
+            }
+            setExternalKeyPool.clear();
 
-        for (const int64_t nIndex : set_pre_split_keypool) {
-            batch.ErasePool(nIndex);
+            for (const int64_t nIndex : set_pre_split_keypool) {
+                batch.ErasePool(nIndex);
+            }
         }
         set_pre_split_keypool.clear();
 
@@ -1315,7 +1323,7 @@ void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex, const OutputType& ty
     CPubKey pubkey;
     bool have_pk = GetPubKey(m_index_to_reserved_key.at(nIndex), pubkey);
     assert(have_pk);
-    LearnRelatedScripts(pubkey, type);
+    LearnRelatedScripts(batch ,pubkey, type);
     m_index_to_reserved_key.erase(nIndex);
     WalletLogPrintf("keypool keep %d\n", nIndex);
 }
@@ -1409,22 +1417,22 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key
     return true;
 }
 
-void LegacyScriptPubKeyMan::LearnRelatedScripts(const CPubKey& key, OutputType type)
+void LegacyScriptPubKeyMan::LearnRelatedScripts(WalletBatch& batch, const CPubKey& key, OutputType type)
 {
     assert(type != OutputType::BECH32M);
     if (key.IsCompressed() && (type == OutputType::P2SH_SEGWIT || type == OutputType::BECH32)) {
         CTxDestination witdest = WitnessV0KeyHash(key.GetID());
         CScript witprog = GetScriptForDestination(witdest);
         // Make sure the resulting program is solvable.
         assert(IsSolvable(*this, witprog));
-        AddCScript(witprog);
+        AddCScriptWithDB(batch, witprog);
     }
 }
 
-void LegacyScriptPubKeyMan::LearnAllRelatedScripts(const CPubKey& key)
+void LegacyScriptPubKeyMan::LearnAllRelatedScripts(WalletBatch& batch, const CPubKey& key)
 {
     // OutputType::P2SH_SEGWIT always adds all necessary scripts for all types.
-    LearnRelatedScripts(key, OutputType::P2SH_SEGWIT);
+    LearnRelatedScripts(batch, key, OutputType::P2SH_SEGWIT);
 }
 
 void LegacyScriptPubKeyMan::MarkReserveKeysAsUsed(int64_t keypool_id)
@@ -1444,7 +1452,7 @@ void LegacyScriptPubKeyMan::MarkReserveKeysAsUsed(int64_t keypool_id)
         if (batch.ReadPool(index, keypool)) { //TODO: This should be unnecessary
             m_pool_key_to_index.erase(keypool.vchPubKey.GetID());
         }
-        LearnAllRelatedScripts(keypool.vchPubKey);
+        LearnAllRelatedScripts(batch, keypool.vchPubKey);
         batch.ErasePool(index);
         WalletLogPrintf("keypool index %d removed\n", index);
         it = setKeyPool->erase(it);
@@ -1933,18 +1941,20 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_
     m_wallet_descriptor = w_desc;
 
     // Store the master private key, and descriptor
-    WalletBatch batch(m_storage.GetDatabase());
-    if (!AddDescriptorKeyWithDB(batch, master_key.key, master_key.key.GetPubKey())) {
-        throw std::runtime_error(std::string(__func__) + ": writing descriptor master private key failed");
-    }
-    if (!batch.WriteDescriptor(GetID(), m_wallet_descriptor)) {
-        throw std::runtime_error(std::string(__func__) + ": writing descriptor failed");
+    {
+        WalletBatch batch(m_storage.GetDatabase());
+        if (!AddDescriptorKeyWithDB(batch, master_key.key, master_key.key.GetPubKey())) {
+            throw std::runtime_error(std::string(__func__) + ": writing descriptor master private key failed");
+        }
+        if (!batch.WriteDescriptor(GetID(), m_wallet_descriptor)) {
+            throw std::runtime_error(std::string(__func__) + ": writing descriptor failed");
+        }
+        m_storage.UnsetBlankWalletFlag(batch);
     }
 
     // TopUp
     TopUp();
 
-    m_storage.UnsetBlankWalletFlag(batch);
     return true;
 }
 
@@ -2268,7 +2278,7 @@ bool DescriptorScriptPubKeyMan::GetDescriptorString(std::string& out) const
     return m_wallet_descriptor.descriptor->ToNormalizedString(provider, out, &m_wallet_descriptor.cache);
 }
 
-void DescriptorScriptPubKeyMan::UpgradeDescriptorCache()
+void DescriptorScriptPubKeyMan::UpgradeDescriptorCache(WalletBatch& batch)
 {
     LOCK(cs_desc_man);
     if (m_storage.IsLocked() || m_storage.IsWalletFlagSet(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED)) {
@@ -2292,7 +2302,7 @@ void DescriptorScriptPubKeyMan::UpgradeDescriptorCache()
 
     // Cache the last hardened xpubs
     DescriptorCache diff = m_wallet_descriptor.cache.MergeAndDiff(temp_cache);
-    if (!WalletBatch(m_storage.GetDatabase()).WriteDescriptorCacheItems(GetID(), diff)) {
+    if (!batch.WriteDescriptorCacheItems(GetID(), diff)) {
         throw std::runtime_error(std::string(__func__) + ": writing cache items failed");
     }
 }

src/wallet/scriptpubkeyman.h

@@ -370,7 +370,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
     void MarkUnusedAddresses(const CScript& script) override;
 
     //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo
-    void UpgradeKeyMetadata();
+    void UpgradeKeyMetadata(WalletBatch& batch);
 
     bool IsHDEnabled() const override;
 
@@ -440,6 +440,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
     bool HaveWatchOnly() const;
     //! Remove a watch only script from the keystore
     bool RemoveWatchOnly(const CScript &dest);
+    bool RemoveWatchOnlyWithDB(WalletBatch& batch, const CScript &dest);
     bool AddWatchOnly(const CScript& dest, int64_t nCreateTime) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore);
 
     //! Fetches a pubkey from mapWatchKeys if it exists there
@@ -483,13 +484,13 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
      * software, as FillableSigningProvider automatically does this implicitly for all
      * keys now.
      */
-    void LearnRelatedScripts(const CPubKey& key, OutputType);
+    void LearnRelatedScripts(WalletBatch& batch, const CPubKey& key, OutputType);
 
     /**
      * Same as LearnRelatedScripts, but when the OutputType is not known (and could
      * be anything).
      */
-    void LearnAllRelatedScripts(const CPubKey& key);
+    void LearnAllRelatedScripts(WalletBatch& batch, const CPubKey& key);
 
     /**
      * Marks all keys in the keypool up to and including reserve_key as used.
@@ -623,7 +624,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
 
     bool GetDescriptorString(std::string& out) const;
 
-    void UpgradeDescriptorCache();
+    void UpgradeDescriptorCache(WalletBatch& batch);
 };
 
 #endif // BITCOIN_WALLET_SCRIPTPUBKEYMAN_H

src/wallet/sqlite.h

@@ -79,10 +79,6 @@ class SQLiteDatabase : public WalletDatabase
     /** Close the database */
     void Close() override;
 
-    /* These functions are unused */
-    void AddRef() override { assert(false); }
-    void RemoveRef() override { assert(false); }
-
     /** Rewrite the entire database on disk */
     bool Rewrite(const char* skip = nullptr) override;
 

src/wallet/wallet.cpp

@@ -359,8 +359,9 @@ const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const
     return &(it->second);
 }
 
-void CWallet::UpgradeKeyMetadata()
+void CWallet::UpgradeKeyMetadata(WalletBatch& batch)
 {
+    AssertLockHeld(cs_wallet);
     if (IsLocked() || IsWalletFlagSet(WALLET_FLAG_KEY_ORIGIN_METADATA)) {
         return;
     }
@@ -370,21 +371,21 @@ void CWallet::UpgradeKeyMetadata()
         return;
     }
 
-    spk_man->UpgradeKeyMetadata();
-    SetWalletFlag(WALLET_FLAG_KEY_ORIGIN_METADATA);
+    spk_man->UpgradeKeyMetadata(batch);
+    SetWalletFlagWithDB(batch, WALLET_FLAG_KEY_ORIGIN_METADATA);
 }
 
-void CWallet::UpgradeDescriptorCache()
+void CWallet::UpgradeDescriptorCache(WalletBatch& batch)
 {
     if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) || IsLocked() || IsWalletFlagSet(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED)) {
         return;
     }
 
     for (ScriptPubKeyMan* spkm : GetAllScriptPubKeyMans()) {
         DescriptorScriptPubKeyMan* desc_spkm = dynamic_cast<DescriptorScriptPubKeyMan*>(spkm);
-        desc_spkm->UpgradeDescriptorCache();
+        desc_spkm->UpgradeDescriptorCache(batch);
     }
-    SetWalletFlag(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED);
+    SetWalletFlagWithDB(batch, WALLET_FLAG_LAST_HARDENED_XPUB_CACHED);
 }
 
 bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool accept_no_keys)
@@ -402,9 +403,10 @@ bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool accept_no_key
                 continue; // try another master key
             if (Unlock(_vMasterKey, accept_no_keys)) {
                 // Now that we've unlocked, upgrade the key metadata
-                UpgradeKeyMetadata();
+                WalletBatch batch(GetDatabase());
+                UpgradeKeyMetadata(batch);
                 // Now that we've unlocked, upgrade the descriptor cache
-                UpgradeDescriptorCache();
+                UpgradeDescriptorCache(batch);
                 return true;
             }
         }
@@ -1379,13 +1381,19 @@ bool CWallet::CanGetAddresses(bool internal) const
     }
     return false;
 }
+void CWallet::SetWalletFlagWithDB(WalletBatch& batch, uint64_t flags)
+{
+    AssertLockHeld(cs_wallet);
+    m_wallet_flags |= flags;
+    if (!batch.WriteWalletFlags(m_wallet_flags))
+        throw std::runtime_error(std::string(__func__) + ": writing wallet flags failed");
+}
 
 void CWallet::SetWalletFlag(uint64_t flags)
 {
     LOCK(cs_wallet);
-    m_wallet_flags |= flags;
-    if (!WalletBatch(GetDatabase()).WriteWalletFlags(m_wallet_flags))
-        throw std::runtime_error(std::string(__func__) + ": writing wallet flags failed");
+    WalletBatch batch(GetDatabase());
+    SetWalletFlagWithDB(batch, flags);
 }
 
 void CWallet::UnsetWalletFlag(uint64_t flag)

src/wallet/wallet.h

@@ -476,10 +476,10 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     double ScanningProgress() const { return fScanningWallet ? (double) m_scanning_progress : 0; }
 
     //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo
-    void UpgradeKeyMetadata() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    void UpgradeKeyMetadata(WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     //! Upgrade DescriptorCaches
-    void UpgradeDescriptorCache() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    void UpgradeDescriptorCache(WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     bool LoadMinVersion(int nVersion) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); nWalletVersion = nVersion; return true; }
 
@@ -797,6 +797,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     void BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(::cs_main) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet);
 
     /** set a single wallet flag */
+    void SetWalletFlagWithDB(WalletBatch& batch, uint64_t flags) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     void SetWalletFlag(uint64_t flags);
 
     /** Unsets a single wallet flag */