wallettool, test: Remove BDB/ legacy wallets dump feature #33161

Disable dump feature in wallet-tool (bitcoin-wallet) for legacy wallets, which are no longer supported.

Currently, in master, it's still possible to dump legacy wallets, but not to createfromdump on them, which creates an inconsistency. This PR aligns the behavior by rejecting unsupported legacy wallets in the dump command as well.

<details> <summary><i><B>Note:</B></i>&ensp;There's still another reference to <code>DBD</code> in <code>dump.cpp</code> which I wanted to update returning an error when the format is not supported (<code>!= sqlite</code>) but there's a <code>fuzz</code> (<code>/src/wallet/test/fuzz/wallet_bdb_parser.cpp</code>) that tests <code>BerkeleyRODatabase</code> dumping that fails with my change and since we still use this DB for legacy wallets migration I think we should keep the <code>fuzz</code>.</summary>

--- a/src/wallet/dump.cpp
+++ b/src/wallet/dump.cpp
@@ -23,6 +23,13 @@ uint32_t DUMP_VERSION = 1;
 
 bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& error)
 {
+    // Chek first that DB format is supported
+    std::string format = db.Format();
+    if (format != "sqlite") {
+        error = strprintf(_("Error: Wallet specifies an unsupported database format (%s). Only sqlite database dumps are supported"), format);
+        return false;
+    }
+
     // Get the dumpfile
     std::string dump_filename = args.GetArg("-dumpfile", "");
     if (dump_filename.empty()) {
@@ -60,12 +67,6 @@ bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& erro
     hasher << std::span{line};
 
     // Write out the file format
-    std::string format = db.Format();
-    // BDB files that are opened using BerkeleyRODatabase have its format as "bdb_ro"
-    // We want to override that format back to "bdb"
-    if (format == "bdb_ro") {
-        format = "bdb";
-    }
     line = strprintf("%s,%s\n", "format", format);
     dump_file.write(line.data(), line.size());
     hasher << std::span{line};

--- a/test/functional/tool_wallet.py
+++ b/test/functional/tool_wallet.py
@@ -443,12 +443,7 @@ class ToolWalletTest(BitcoinTestFramework):
         shutil.copytree(legacy_node.wallets_path / wallet_name, master_node.wallets_path / wallet_name)
 
         wallet_dump = master_node.datadir_path / (wallet_name + ".dump")
-        self.assert_raises_tool_error(
-            "Failed to open database path '{}'. The wallet appears to be a Legacy wallet, " \
-            "please use the wallet migration tool (migratewallet RPC or the GUI option).".format(master_node.wallets_path / wallet_name),
-            f"-wallet={wallet_name}", f"-dumpfile={wallet_dump}",
-            "dump"
-        )
+        self.assert_raises_tool_error("Error: Wallet specifies an unsupported database format (bdb_ro). Only sqlite database dumps are supported", f"-wallet={wallet_name}", f"-dumpfile={wallet_dump}", "dump")
         assert not wallet_dump.exists()
 
         self.log.info("Test that legacy wallets could be dumped in releases prior to v30.0")

</details>

Testing

The new functional test test_legacy_dump_is_no_longer_allowed in test/functional/tool_wallet.py will fail in master if the changes on src/wallet/wallettool.cpp are rolled back.

<details> <summary>In order to test it manually you need to create a legacy wallet with a previous release version.</summary>

• Get previous releases with test/get_previous_releases.py.
• Start a node with an older release:
  • ./releases/v28.2/bin/bitcoind -regtest -datadir=/tmp/btc -deprecatedrpc=create_bdb
• On a separate terminal create a legacy wallet:
  • ./releases/v28.2/bin/bitcoind -regtest -datadir=/tmp/btc createwallet legacy_1 false false "" false false
• Stop the node.
• To verify that the legacy wallet can be dumped either previous wallet-tool version or master could be used:
  • In older wallet-tool:
    • ./releases/v28.2/bin/bitcoin-wallet -regtest -datadir=/tmp/btc -wallet=legacy_1 -dumpfile=legacy1.dump dump
  • In master branch (assuming /build is the build directory):
    • ./build/bin/bitcoin-wallet -regtest -datadir=/tmp/btc -wallet=legacy_1 -dumpfile=legacy1.dump dump
• To verify that the legacy wallet cannot be dumped anymore after this change: build this PR branch and run the latest command above (delete the .dump file first if you already ran it).

  </details>

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33161.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Removing this functionality does not meaningfully reduce the amount of code that we have since it is relying on things that we must keep for migration anyways. I think it is also useful to continue to be able to dump legacy wallets with current tooling. Also being able to dump a BDB wallet is the only way to migrate a bdb-descriptor wallet (these could only be created for a few months in 2020).

being able to dump a BDB wallet is the only way to migrate a bdb-descriptor wallet (these could only be created for a few months in 2020).

Should we document this? Perhaps in the "migrating" section on managing-wallets.md...

Should we document this?

No, bdb-descriptor was never a officially supported configuration.