Merge bitcoin/bitcoin#28574: wallet: optimize migration process, batch db transactions

c98fc36 wallet: migration, consolidate external wallets db writes (furszy)
7c9076a wallet: migration, consolidate main wallet db writes (furszy)
9ef20e8 wallet: provide WalletBatch to 'SetupDescriptorScriptPubKeyMans' (furszy)
34bf079 wallet: refactor ApplyMigrationData to return util::Result<void> (furszy)
aacaaaa wallet: provide WalletBatch to 'RemoveTxs' (furszy)
57249ff wallet: introduce active db txn listeners (furszy)
91e065e wallet: remove post-migration signals connection (furszy)
055c053 wallet: provide WalletBatch to 'DeleteRecords' (furszy)
122d103 wallet: introduce 'SetWalletFlagWithDB' (furszy)
6052c78 wallet: decouple default descriptors creation from external signer setup (furszy)
f2541d0 wallet: batch MigrateToDescriptor() db transactions (furszy)
66c9936 bench: add coverage for wallet migration process (furszy)

Pull request description:

  Last step in a chain of PRs (#26836, #28894, #28987, #29403).

  #### Detailed Description:
  The current wallet migration process performs only individual db writes. Accessing disk to
  delete all legacy records, clone and clean each address book entry for every created wallet,
  create each new descriptor (with their corresponding master key, caches and key pool), and
  also clone and delete each transaction that requires to be transferred to a different wallet.

  This work consolidates all individual disk writes into two batch operations. One for the descriptors
  creation from the legacy data and a second one for the execution of the migration process itself.
  Efficiently dumping all the information to disk at once atomically at the end of each process.

  This represent a speed up and also a consistency improvement. During migration, we either
  want to succeed or fail. No other outcomes should be accepted. We should never leave a
  partially migrated wallet on disk and request the user to manually restore the previous wallet from
  a backup (at least not if we can avoid it).

  Since the speedup depends on the storage device, benchmark results can vary significantly.
  Locally, I have seen a 15% speedup on a USB 3.2 pendrive.

  #### Note for Testers:
  The first commit introduces a benchmark for the migration process. This one can be
  cherry-picked on top of master to compare results pre and post changes.

  Please note that the benchmark setup may take some time (~70 seconds here) due to the absence
  of a batching mechanism for the address generation process (`GetNewDestination()` calls).

ACKs for top commit:
  achow101:
    ACK c98fc36
  theStack:
    re-ACK c98fc36
  pablomartin4btc:
    re-ACK c98fc36

Tree-SHA512: a52d5f2eef27811045d613637c0a9d0b7e180256ddc1c893749d98ba2882b570c45f28cc7263cadd4710f2c10db1bea33d88051f29c6b789bc6180c85b5fd8f6

Author: achow101

src/bench/CMakeLists.txt

@@ -71,6 +71,7 @@ if(ENABLE_WALLET)
       wallet_create_tx.cpp
       wallet_loading.cpp
       wallet_ismine.cpp
+      wallet_migration.cpp
   )
   target_link_libraries(bench_bitcoin bitcoin_wallet)
 endif()

src/bench/wallet_migration.cpp

@@ -0,0 +1,80 @@
+// Copyright (c) 2024 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://www.opensource.org/licenses/mit-license.php.
+
+#include <bitcoin-build-config.h> // IWYU pragma: keep
+
+#include <bench/bench.h>
+#include <interfaces/chain.h>
+#include <node/context.h>
+#include <test/util/mining.h>
+#include <test/util/setup_common.h>
+#include <wallet/test/util.h>
+#include <wallet/context.h>
+#include <wallet/receive.h>
+#include <wallet/wallet.h>
+
+#include <optional>
+
+#if defined(USE_BDB) && defined(USE_SQLITE) // only enable benchmark when bdb and sqlite are enabled
+
+namespace wallet{
+
+static void WalletMigration(benchmark::Bench& bench)
+{
+    const auto test_setup = MakeNoLogFileContext<TestingSetup>();
+
+    WalletContext context;
+    context.args = &test_setup->m_args;
+    context.chain = test_setup->m_node.chain.get();
+
+    // Number of imported watch only addresses
+    int NUM_WATCH_ONLY_ADDR = 20;
+
+    // Setup legacy wallet
+    DatabaseOptions options;
+    options.use_unsafe_sync = true;
+    options.verify = false;
+    DatabaseStatus status;
+    bilingual_str error;
+    auto database = MakeWalletDatabase(fs::PathToString(test_setup->m_path_root / "legacy"), options, status, error);
+    uint64_t create_flags = 0;
+    auto wallet = TestLoadWallet(std::move(database), context, create_flags);
+
+    // Add watch-only addresses
+    std::vector<CScript> scripts_watch_only;
+    for (int w = 0; w < NUM_WATCH_ONLY_ADDR; ++w) {
+        CKey key = GenerateRandomKey();
+        LOCK(wallet->cs_wallet);
+        const CScript& script = scripts_watch_only.emplace_back(GetScriptForDestination(GetDestinationForKey(key.GetPubKey(), OutputType::LEGACY)));
+        bool res = wallet->ImportScriptPubKeys(strprintf("watch_%d", w), {script},
+                                    /*have_solving_data=*/false, /*apply_label=*/true, /*timestamp=*/1);
+        assert(res);
+    }
+
+    // Generate transactions and local addresses
+    for (int j = 0; j < 400; ++j) {
+        CMutableTransaction mtx;
+        mtx.vout.emplace_back(COIN, GetScriptForDestination(*Assert(wallet->GetNewDestination(OutputType::BECH32, strprintf("bench_%d", j)))));
+        mtx.vout.emplace_back(COIN, GetScriptForDestination(*Assert(wallet->GetNewDestination(OutputType::LEGACY, strprintf("legacy_%d", j)))));
+        mtx.vout.emplace_back(COIN, scripts_watch_only.at(j % NUM_WATCH_ONLY_ADDR));
+        mtx.vin.resize(2);
+        wallet->AddToWallet(MakeTransactionRef(mtx), TxStateInactive{}, /*update_wtx=*/nullptr, /*fFlushOnClose=*/false, /*rescanning_old_block=*/true);
+    }
+
+    // Unload so the migration process loads it
+    TestUnloadWallet(std::move(wallet));
+
+    bench.epochs(/*numEpochs=*/1).run([&] {
+        util::Result<MigrationResult> res = MigrateLegacyToDescriptor(fs::PathToString(test_setup->m_path_root / "legacy"), "", context);
+        assert(res);
+        assert(res->wallet);
+        assert(res->watchonly_wallet);
+    });
+}
+
+BENCHMARK(WalletMigration, benchmark::PriorityLevel::LOW);
+
+} // namespace wallet
+
+#endif // end USE_SQLITE && USE_BDB

src/wallet/bdb.h

@@ -208,6 +208,7 @@ class BerkeleyBatch : public DatabaseBatch
     bool TxnBegin() override;
     bool TxnCommit() override;
     bool TxnAbort() override;
+    bool HasActiveTxn() override { return activeTxn != nullptr; }
     DbTxn* txn() const { return activeTxn; }
 };
 

src/wallet/db.h

@@ -122,6 +122,7 @@ class DatabaseBatch
     virtual bool TxnBegin() = 0;
     virtual bool TxnCommit() = 0;
     virtual bool TxnAbort() = 0;
+    virtual bool HasActiveTxn() = 0;
 };
 
 /** An instance of this class represents one database.

src/wallet/migrate.h

@@ -115,6 +115,7 @@ class BerkeleyROBatch : public DatabaseBatch
     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.

src/wallet/salvage.cpp

@@ -44,6 +44,7 @@ class DummyBatch : public DatabaseBatch
     bool TxnBegin() override { return true; }
     bool TxnCommit() override { return true; }
     bool TxnAbort() override { return true; }
+    bool HasActiveTxn() override { return false; }
 };
 
 /** A dummy WalletDatabase that does nothing and never fails. Only used by salvage.

src/wallet/scriptpubkeyman.cpp

@@ -1807,6 +1807,12 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
         keyid_it++;
     }
 
+    WalletBatch batch(m_storage.GetDatabase());
+    if (!batch.TxnBegin()) {
+        LogPrintf("Error generating descriptors for migration, cannot initialize db transaction\n");
+        return std::nullopt;
+    }
+
     // keyids is now all non-HD keys. Each key will have its own combo descriptor
     for (const CKeyID& keyid : keyids) {
         CKey key;
@@ -1837,8 +1843,8 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
 
         // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
         auto desc_spk_man = std::make_unique<DescriptorScriptPubKeyMan>(m_storage, w_desc, /*keypool_size=*/0);
-        desc_spk_man->AddDescriptorKey(key, key.GetPubKey());
-        desc_spk_man->TopUp();
+        WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, key, key.GetPubKey()));
+        desc_spk_man->TopUpWithDB(batch);
         auto desc_spks = desc_spk_man->GetScriptPubKeys();
 
         // Remove the scriptPubKeys from our current set
@@ -1883,8 +1889,8 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
 
             // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
             auto desc_spk_man = std::make_unique<DescriptorScriptPubKeyMan>(m_storage, w_desc, /*keypool_size=*/0);
-            desc_spk_man->AddDescriptorKey(master_key.key, master_key.key.GetPubKey());
-            desc_spk_man->TopUp();
+            WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, master_key.key, master_key.key.GetPubKey()));
+            desc_spk_man->TopUpWithDB(batch);
             auto desc_spks = desc_spk_man->GetScriptPubKeys();
 
             // Remove the scriptPubKeys from our current set
@@ -1950,9 +1956,9 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
                 if (!GetKey(keyid, key)) {
                     continue;
                 }
-                desc_spk_man->AddDescriptorKey(key, key.GetPubKey());
+                WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, key, key.GetPubKey()));
             }
-            desc_spk_man->TopUp();
+            desc_spk_man->TopUpWithDB(batch);
             auto desc_spks_set = desc_spk_man->GetScriptPubKeys();
             desc_spks.insert(desc_spks.end(), desc_spks_set.begin(), desc_spks_set.end());
 
@@ -2019,13 +2025,26 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
 
     // Make sure that we have accounted for all scriptPubKeys
     assert(spks.size() == 0);
+
+    // Finalize transaction
+    if (!batch.TxnCommit()) {
+        LogPrintf("Error generating descriptors for migration, cannot commit db transaction\n");
+        return std::nullopt;
+    }
+
     return out;
 }
 
 bool LegacyDataSPKM::DeleteRecords()
+{
+    return RunWithinTxn(m_storage.GetDatabase(), /*process_desc=*/"delete legacy records", [&](WalletBatch& batch){
+        return DeleteRecordsWithDB(batch);
+    });
+}
+
+bool LegacyDataSPKM::DeleteRecordsWithDB(WalletBatch& batch)
 {
     LOCK(cs_KeyStore);
-    WalletBatch batch(m_storage.GetDatabase());
     return batch.EraseRecords(DBKeys::LEGACY_TYPES);
 }
 

src/wallet/scriptpubkeyman.h

@@ -366,8 +366,9 @@ class LegacyDataSPKM : public ScriptPubKeyMan, public FillableSigningProvider
     /** Get the DescriptorScriptPubKeyMans (with private keys) that have the same scriptPubKeys as this LegacyScriptPubKeyMan.
      * Does not modify this ScriptPubKeyMan. */
     std::optional<MigrationData> MigrateToDescriptor();
-    /** Delete all the records ofthis LegacyScriptPubKeyMan from disk*/
+    /** Delete all the records of this LegacyScriptPubKeyMan from disk*/
     bool DeleteRecords();
+    bool DeleteRecordsWithDB(WalletBatch& batch);
 };
 
 // Implements the full legacy wallet behavior
@@ -582,6 +583,7 @@ class LegacySigningProvider : public SigningProvider
 
 class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
 {
+    friend class LegacyDataSPKM;
 private:
     using ScriptPubKeyMap = std::map<CScript, int32_t>; // Map of scripts to descriptor range index
     using PubKeyMap = std::map<CPubKey, int32_t>; // Map of pubkeys involved in scripts to descriptor range index

src/wallet/sqlite.h

@@ -95,6 +95,7 @@ class SQLiteBatch : public DatabaseBatch
     bool TxnBegin() override;
     bool TxnCommit() override;
     bool TxnAbort() override;
+    bool HasActiveTxn() override { return m_txn; }
 };
 
 /** An instance of this class represents one SQLite3 database.

src/wallet/test/util.h

@@ -95,6 +95,7 @@ class MockableBatch : public DatabaseBatch
     bool TxnBegin() override { return m_pass; }
     bool TxnCommit() override { return m_pass; }
     bool TxnAbort() override { return m_pass; }
+    bool HasActiveTxn() override { return false; }
 };
 
 /** A WalletDatabase whose contents and return values can be modified as needed for testing