Merge pull request #5787 from PastaPastaPasta/develop-trivial-2023-12-22

backport: trivial backports Dec 23 2023

src/node/transaction.cpp

@@ -96,8 +96,6 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
 
 CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock)
 {
-    LOCK(cs_main);
-
     if (mempool && !block_index) {
         CTransactionRef ptx = mempool->get(hash);
         if (ptx) return ptx;

src/test/fuzz/banman.cpp

@@ -107,8 +107,7 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
             BanMan ban_man_read{banlist_file, /* client_interface */ nullptr, /* default_ban_time */ 0};
             banmap_t banmap_read;
             ban_man_read.GetBanned(banmap_read);
-            // Temporarily disabled to allow the remainder of the fuzz test to run while a fix is being worked on:
+            assert(banmap == banmap_read);
-            // assert(banmap == banmap_read);
         }
     }
     fs::remove(banlist_file.string() + ".json");

src/util/epochguard.h

@@ -40,6 +40,9 @@ public:
     Epoch() = default;
     Epoch(const Epoch&) = delete;
     Epoch& operator=(const Epoch&) = delete;
+    Epoch(Epoch&&) = delete;
+    Epoch& operator=(Epoch&&) = delete;
+    ~Epoch() = default;
 
     bool guarded() const { return m_guarded; }
 
@@ -51,6 +54,13 @@ public:
         // only allow modification via Epoch member functions
         friend class Epoch;
         Marker& operator=(const Marker&) = delete;
+
+    public:
+        Marker() = default;
+        Marker(const Marker&) = default;
+        Marker(Marker&&) = delete;
+        Marker& operator=(Marker&&) = delete;
+        ~Marker() = default;
     };
 
     class SCOPED_LOCKABLE Guard

src/wallet/rpcdump.cpp

@@ -917,7 +917,7 @@ UniValue dumpwallet(const JSONRPCRequest& request)
     // the user could have gotten from another RPC command prior to now
     wallet.BlockUntilSyncedToCurrentChain();
 
-    LOCK2(wallet.cs_wallet, spk_man.cs_KeyStore);
+    LOCK(wallet.cs_wallet);
 
     EnsureWalletIsUnlocked(&wallet);
 
@@ -939,9 +939,16 @@ UniValue dumpwallet(const JSONRPCRequest& request)
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file");
 
     std::map<CKeyID, int64_t> mapKeyBirth;
-    const std::map<CKeyID, int64_t>& mapKeyPool = spk_man.GetAllReserveKeys();
     wallet.GetKeyBirthTimes(mapKeyBirth);
 
+    int64_t block_time = 0;
+    CHECK_NONFATAL(wallet.chain().findBlock(wallet.GetLastBlockHash(), FoundBlock().time(block_time)));
+
+    // Note: To avoid a lock order issue, access to cs_main must be locked before cs_KeyStore.
+    // So we do the two things in this function that lock cs_main first: GetKeyBirthTimes, and findBlock.
+    LOCK(spk_man.cs_KeyStore);
+
+    const std::map<CKeyID, int64_t>& mapKeyPool = spk_man.GetAllReserveKeys();
     std::set<CScriptID> scripts = spk_man.GetCScripts();
 
     // sort time/key pairs
@@ -956,8 +963,6 @@ UniValue dumpwallet(const JSONRPCRequest& request)
     file << strprintf("# Wallet dump created by Dash Core %s\n", CLIENT_BUILD);
     file << strprintf("# * Created on %s\n", FormatISO8601DateTime(GetTime()));
     file << strprintf("# * Best block at time of backup was %i (%s),\n", wallet.GetLastBlockHeight(), wallet.GetLastBlockHash().ToString());
-    int64_t block_time = 0;
-    CHECK_NONFATAL(wallet.chain().findBlock(wallet.GetLastBlockHash(), FoundBlock().time(block_time)));
     file << strprintf("#   mined on %s\n", FormatISO8601DateTime(block_time));
     file << "\n";
 

src/wallet/wallet.cpp

@@ -4321,6 +4321,13 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t>& mapKeyBirth) const {
     AssertLockHeld(cs_wallet);
     mapKeyBirth.clear();
 
+    // map in which we'll infer heights of other keys
+    std::map<CKeyID, const CWalletTx::Confirmation*> mapKeyFirstBlock;
+    CWalletTx::Confirmation max_confirm;
+    max_confirm.block_height = GetLastBlockHeight() > 144 ? GetLastBlockHeight() - 144 : 0; // the tip can be reorganized; use a 144-block safety margin
+    CHECK_NONFATAL(chain().findAncestorByHeight(GetLastBlockHash(), max_confirm.block_height, FoundBlock().hash(max_confirm.hashBlock)));
+
+    {
         LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan();
         assert(spk_man != nullptr);
         LOCK(spk_man->cs_KeyStore);
@@ -4332,11 +4339,7 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t>& mapKeyBirth) const {
             }
         }
 
-    // map in which we'll infer heights of other keys
+        // Prepare to infer birth heights for keys without metadata
-    std::map<CKeyID, const CWalletTx::Confirmation*> mapKeyFirstBlock;
-    CWalletTx::Confirmation max_confirm;
-    max_confirm.block_height = GetLastBlockHeight() > 144 ? GetLastBlockHeight() - 144 : 0; // the tip can be reorganized; use a 144-block safety margin
-    CHECK_NONFATAL(chain().findAncestorByHeight(GetLastBlockHash(), max_confirm.block_height, FoundBlock().hash(max_confirm.hashBlock)));
         for (const CKeyID &keyid : spk_man->GetKeys()) {
             if (mapKeyBirth.count(keyid) == 0)
                 mapKeyFirstBlock[keyid] = &max_confirm;
@@ -4364,6 +4367,7 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t>& mapKeyBirth) const {
                 }
             }
         }
+    }
 
     // Extract block timestamps for those keys
     for (const auto& entry : mapKeyFirstBlock) {

test/functional/wallet_dump.py

@@ -195,6 +195,15 @@ class WalletDumpTest(BitcoinTestFramework):
             with self.nodes[0].assert_debug_log(['Flushing wallet.dat'], timeout=20):
                 self.nodes[0].getnewaddress()
 
+        # Make sure that dumpwallet doesn't have a lock order issue when there is an unconfirmed tx and it is reloaded
+        # See https://github.com/bitcoin/bitcoin/issues/22489
+        self.nodes[0].createwallet("w3")
+        w3 = self.nodes[0].get_wallet_rpc("w3")
+        w3.importprivkey(privkey=self.nodes[0].get_deterministic_priv_key().key, label="coinbase_import")
+        w3.sendtoaddress(w3.getnewaddress(), 10)
+        w3.unloadwallet()
+        self.nodes[0].loadwallet("w3")
+        w3.dumpwallet(os.path.join(self.nodes[0].datadir, "w3.dump"))
 
 if __name__ == '__main__':
     WalletDumpTest().main()

test/functional/wallet_multiwallet.py

@@ -100,7 +100,7 @@ class MultiWalletTest(BitcoinTestFramework):
         #   w8         - to verify existing wallet file is loaded correctly. Not tested for SQLite wallets as this is a deprecated BDB behavior.
         #   ''         - to verify default wallet file is created correctly
         to_create = ['w1', 'w2', 'w3', 'w', 'sub/w5', 'w7_symlink']
-        in_wallet_dir = to_create.copy() # Wallets in the wallet dir
+        in_wallet_dir = [w.replace('/', os.path.sep) for w in to_create]  # Wallets in the wallet dir
         in_wallet_dir.append('w7')  # w7 is not loaded or created, but will be listed by listwalletdir because w7_symlink
         to_create.append(os.path.join(self.options.tmpdir, 'extern/w6'))  # External, not in the wallet dir, so we need to avoid adding it to in_wallet_dir
         to_load = [self.default_wallet_name]
@@ -398,5 +398,6 @@ class MultiWalletTest(BitcoinTestFramework):
         self.nodes[0].unloadwallet(wallet)
         self.nodes[1].loadwallet(wallet)
 
+
 if __name__ == '__main__':
     MultiWalletTest().main()