Merge ElementsProject/elements#1546: Fix createfromdump deletion

b6369329ce wallettool: do not use fs::remove_all in createfromdump cleanup (Ava Chow)
086e185a8d wallet: introduce method to return all db created files (furszy)
552ce5ca2c use break-system-packages for pip (Tom Trevethan)
53cef282f4 change test order in feature_discount_ct to fix intermittent failure (Tom Trevethan)

Pull request description:

  Cherry pick of https://github.com/bitcoin/bitcoin/pull/34215

  Required new Files() method to return all db created files for sqlite and bdb (1de423e0a0)

ACKs for top commit:
  delta1:
    ACK b6369329ce; tested locally

Tree-SHA512: effa9fbaf2c2c2381b1e0f937da4761ded7b017afd0e5d308182767848806df31a318545cd132e0c0f1959b3a16f43d98f72825f4b46285257abfacd52278cea

ci/test/00_setup_env_mac_native_arm64.sh

@@ -7,7 +7,9 @@
 export LC_ALL=C.UTF-8
 
 export HOST=arm64-apple-darwin
-export PIP_PACKAGES="zmq"
+# Homebrew's python@3.12 is marked as externally managed (PEP 668).
+# Therefore, `--break-system-packages` is needed.
+export PIP_PACKAGES="--break-system-packages zmq"
 export GOAL="install"
 # ELEMENTS: add -fno-stack-check to work around clang bug on macos
 export BITCOIN_CONFIG="--with-gui --with-miniupnpc --with-natpmp --enable-reduce-exports CXXFLAGS=-fno-stack-check"

ci/test/04_install.sh

@@ -11,7 +11,7 @@ if [[ $QEMU_USER_CMD == qemu-s390* ]]; then
 fi
 
 if [ "$CI_OS_NAME" == "macos" ]; then
-  sudo -H pip3 install --upgrade --ignore-installed pip
+  sudo -H pip3 install --upgrade --break-system-packages --ignore-installed pip
   # shellcheck disable=SC2086
   IN_GETOPT_BIN="$(brew --prefix gnu-getopt)/bin/getopt" ${CI_RETRY_EXE} pip3 install --user $PIP_PACKAGES
 fi

src/wallet/bdb.h

@@ -144,6 +144,21 @@ public:
     /** Return path to main database filename */
     std::string Filename() override { return fs::PathToString(env->Directory() / strFile); }
 
+    std::vector<fs::path> Files() override
+    {
+        std::vector<fs::path> files;
+        files.emplace_back(env->Directory() / strFile);
+        if (env->m_databases.size() == 1) {
+            files.emplace_back(env->Directory() / "db.log");
+            files.emplace_back(env->Directory() / ".walletlock");
+            files.emplace_back(env->Directory() / "database" / "log.0000000001");
+            files.emplace_back(env->Directory() / "database");
+            // Note that this list is not exhaustive as BDB may create more log files, and possibly other ones too
+            // However it should be good enough for the only calls to Files()
+        }
+        return files;
+    }
+
     std::string Format() override { return "bdb"; }
     /**
      * Pointer to shared database environment.

src/wallet/db.h

@@ -144,6 +144,9 @@ public:
     /** Return path to main database file for logs and error messages. */
     virtual std::string Filename() = 0;
 
+    /** Return paths to all database created files */
+    virtual std::vector<fs::path> Files() = 0;
+
     virtual std::string Format() = 0;
 
     std::atomic<unsigned int> nUpdateCounter;
@@ -192,6 +195,7 @@ public:
     void IncrementUpdateCounter() override { ++nUpdateCounter; }
     void ReloadDbEnv() override {}
     std::string Filename() override { return "dummy"; }
+    std::vector<fs::path> Files() override { return {}; }
     std::string Format() override { return "dummy"; }
     std::unique_ptr<DatabaseBatch> MakeBatch(bool flush_on_close = true) override { return std::make_unique<DummyBatch>(); }
 };

src/wallet/dump.cpp

@@ -284,11 +284,17 @@ bool CreateFromDump(const std::string& name, const fs::path& wallet_path, biling
 
         dump_file.close();
     }
+    // On failure, gather the paths to remove
+    std::vector<fs::path> paths_to_remove = wallet->GetDatabase().Files();
+    if (!name.empty()) paths_to_remove.push_back(wallet_path);
+
     wallet.reset(); // The pointer deleter will close the wallet for us.
 
     // Remove the wallet dir if we have a failure
     if (!ret) {
-        fs::remove_all(wallet_path);
+        for (const auto& p : paths_to_remove) {
+            fs::remove(p);
+        }
     }
 
     return ret;

src/wallet/migrate.h

@@ -0,0 +1,110 @@
+// Copyright (c) 2021-present The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_WALLET_MIGRATE_H
+#define BITCOIN_WALLET_MIGRATE_H
+
+#include <wallet/db.h>
+
+#include <optional>
+
+namespace wallet {
+
+using BerkeleyROData = std::map<SerializeData, SerializeData, std::less<>>;
+
+/**
+ * A class representing a BerkeleyDB file from which we can only read records.
+ * This is used only for migration of legacy to descriptor wallets
+ */
+class BerkeleyRODatabase : public WalletDatabase
+{
+private:
+    const fs::path m_filepath;
+
+public:
+    /** Create DB handle */
+    BerkeleyRODatabase(const fs::path& filepath, bool open = true) : WalletDatabase(), m_filepath(filepath)
+    {
+        if (open) Open();
+    }
+    ~BerkeleyRODatabase() = default;
+
+    BerkeleyROData m_records;
+
+    /** Open the database if it is not already opened. */
+    void Open() override;
+
+    /** Rewrite the entire database on disk, with the exception of key pszSkip if non-zero
+     */
+    bool Rewrite(const char* pszSkip = nullptr) override { return false; }
+
+    /** Back up the entire database to a file.
+     */
+    bool Backup(const std::string& strDest) const override;
+
+    /** Flush to the database file and close the database.
+     *  Also close the environment if no other databases are open in it.
+     */
+    void Close() override {}
+
+    /** Return path to main database file for logs and error messages. */
+    std::string Filename() override { return fs::PathToString(m_filepath); }
+    std::vector<fs::path> Files() override { return {m_filepath}; }
+
+    std::string Format() override { return "bdb_ro"; }
+
+    /** Make a DatabaseBatch connected to this database */
+    std::unique_ptr<DatabaseBatch> MakeBatch() override;
+};
+
+class BerkeleyROCursor : public DatabaseCursor
+{
+private:
+    const BerkeleyRODatabase& m_database;
+    BerkeleyROData::const_iterator m_cursor;
+    BerkeleyROData::const_iterator m_cursor_end;
+
+public:
+    explicit BerkeleyROCursor(const BerkeleyRODatabase& database, std::span<const std::byte> prefix = {});
+    ~BerkeleyROCursor() = default;
+
+    Status Next(DataStream& key, DataStream& value) override;
+};
+
+/** RAII class that provides access to a BerkeleyRODatabase */
+class BerkeleyROBatch : public DatabaseBatch
+{
+private:
+    const BerkeleyRODatabase& m_database;
+
+    bool ReadKey(DataStream&& key, DataStream& value) override;
+    // WriteKey returns true since various automatic upgrades for older wallets will expect writing to not fail.
+    // It is okay for this batch type to not actually write anything as those automatic upgrades will occur again after migration.
+    bool WriteKey(DataStream&& key, DataStream&& value, bool overwrite = true) override { return true; }
+    bool EraseKey(DataStream&& key) override { return false; }
+    bool HasKey(DataStream&& key) override;
+    bool ErasePrefix(std::span<const std::byte> prefix) override { return false; }
+
+public:
+    explicit BerkeleyROBatch(const BerkeleyRODatabase& database) : m_database(database) {}
+    ~BerkeleyROBatch() = default;
+
+    BerkeleyROBatch(const BerkeleyROBatch&) = delete;
+    BerkeleyROBatch& operator=(const BerkeleyROBatch&) = delete;
+
+    void Close() override {}
+
+    std::unique_ptr<DatabaseCursor> GetNewCursor() override { return std::make_unique<BerkeleyROCursor>(m_database); }
+    std::unique_ptr<DatabaseCursor> GetNewPrefixCursor(std::span<const std::byte> prefix) override;
+    bool TxnBegin() override { return false; }
+    bool TxnCommit() override { return false; }
+    bool TxnAbort() override { return false; }
+    bool HasActiveTxn() override { return false; }
+};
+
+//! Return object giving access to Berkeley Read Only database at specified path.
+std::unique_ptr<BerkeleyRODatabase> MakeBerkeleyRODatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
+} // namespace wallet
+
+#endif // BITCOIN_WALLET_MIGRATE_H

src/wallet/sqlite.h

@@ -107,6 +107,14 @@ public:
     void IncrementUpdateCounter() override { ++nUpdateCounter; }
 
     std::string Filename() override { return m_file_path; }
+    /** Return paths to all database created files */
+    std::vector<fs::path> Files() override
+    {
+        std::vector<fs::path> files;
+        files.emplace_back(m_dir_path / fs::PathFromString(m_file_path));
+        files.emplace_back(m_dir_path / fs::PathFromString(m_file_path + "-journal"));
+        return files;
+    }
     std::string Format() override { return "sqlite"; }
 
     /** Make a SQLiteBatch connected to this database */

test/functional/feature_discount_ct.py

@@ -89,26 +89,6 @@ class CTTest(BitcoinTestFramework):
         assert_approx(tx['discountweight'], 1301.5, 0.5)
         assert_equal(tx['discountvsize'], 326)
 
-        self.log.info("Send confidential tx to node 0")
-        addr = node0.getnewaddress()
-        info = node0.getaddressinfo(addr)
-        txid = node0.sendtoaddress(info['confidential'], 1.0, "", "", False, None, None, None, None, None, None, feerate)
-        tx = node0.gettransaction(txid, True, True)
-        decoded = tx['decoded']
-        vin = decoded['vin']
-        vout = decoded['vout']
-        assert_equal(len(vin), 2)
-        assert_equal(len(vout), 3)
-        assert_equal(tx['fee']['bitcoin'], Decimal('-0.00002575'))
-        assert_equal(decoded['vsize'], 2575)
-        # tx weight can be 10299 or 10300, accept both
-        assert_approx(decoded['weight'], 10299.5, 0.5)
-        self.generate(node0, 1)
-        tx = node1.getrawtransaction(txid, True)
-        # tx discountweight can be 1301 or 1302, accept both
-        assert_approx(tx['discountweight'], 1301.5, 0.5)
-        assert_equal(tx['discountvsize'], 326) # node1 has discountvsize
-
         self.log.info("Send explicit tx to node 1")
         addr = node1.getnewaddress()
         info = node1.getaddressinfo(addr)
@@ -129,6 +109,26 @@ class CTTest(BitcoinTestFramework):
         assert_approx(tx['discountweight'], 1301.5, 0.5)
         assert_equal(tx['discountvsize'], 326)
 
+        self.log.info("Send confidential tx to node 0")
+        addr = node0.getnewaddress()
+        info = node0.getaddressinfo(addr)
+        txid = node0.sendtoaddress(info['confidential'], 1.0, "", "", False, None, None, None, None, None, None, feerate)
+        tx = node0.gettransaction(txid, True, True)
+        decoded = tx['decoded']
+        vin = decoded['vin']
+        vout = decoded['vout']
+        assert_equal(len(vin), 2)
+        assert_equal(len(vout), 3)
+        assert_equal(tx['fee']['bitcoin'], Decimal('-0.00002575'))
+        assert_equal(decoded['vsize'], 2575)
+        # tx weight can be 10299 or 10300, accept both
+        assert_approx(decoded['weight'], 10299.5, 0.5)
+        self.generate(node0, 1)
+        tx = node1.getrawtransaction(txid, True)
+        # tx discountweight can be 1301 or 1302, accept both
+        assert_approx(tx['discountweight'], 1301.5, 0.5)
+        assert_equal(tx['discountvsize'], 326) # node1 has discountvsize
+
         self.log.info("Send confidential (undiscounted) tx to node 1")
         addr = node1.getnewaddress()
         info = node1.getaddressinfo(addr)

test/functional/tool_wallet.py

@@ -396,7 +396,14 @@ class ToolWalletTest(BitcoinTestFramework):
         self.write_dump(dump_data, bad_sum_wallet_dump)
         self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=badload', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump')
         assert not os.path.isdir(os.path.join(self.nodes[0].datadir, "elementsregtest/wallets", "badload"))
+        if not self.options.descriptors:
+            os.rename(self.nodes[0].datadir + "/" + "elementsregtest/wallets/wallet.dat", self.nodes[0].datadir + "/" + "elementsregtest/wallets/default.wallet.dat")
+        self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump')
+        assert os.path.exists(self.nodes[0].datadir + "/" + "elementsregtest/wallets")
+        assert not os.path.exists(self.nodes[0].datadir + "/" + "elementsregtest/wallets/wallet.dat")
 
+        self.log.info('Checking createfromdump with an unnamed wallet')
+        self.do_tool_createfromdump("", "wallet.dump")
 
     def run_test(self):
         self.wallet_path = os.path.join(self.nodes[0].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename)