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

c98fc36d09 wallet: migration, consolidate external wallets db writes (furszy)
7c9076a2d2 wallet: migration, consolidate main wallet db writes (furszy)
9ef20e86d7 wallet: provide WalletBatch to 'SetupDescriptorScriptPubKeyMans' (furszy)
34bf0795fc wallet: refactor ApplyMigrationData to return util::Result<void> (furszy)
aacaaaa0d3 wallet: provide WalletBatch to 'RemoveTxs' (furszy)
57249ff669 wallet: introduce active db txn listeners (furszy)
91e065ec17 wallet: remove post-migration signals connection (furszy)
055c0532fc wallet: provide WalletBatch to 'DeleteRecords' (furszy)
122d103ca2 wallet: introduce 'SetWalletFlagWithDB' (furszy)
6052c7891d wallet: decouple default descriptors creation from external signer setup (furszy)
f2541d09e1 wallet: batch MigrateToDescriptor() db transactions (furszy)
66c9936455 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 c98fc36d09
  theStack:
    re-ACK c98fc36d09
  pablomartin4btc:
    re-ACK c98fc36d09

Tree-SHA512: a52d5f2eef27811045d613637c0a9d0b7e180256ddc1c893749d98ba2882b570c45f28cc7263cadd4710f2c10db1bea33d88051f29c6b789bc6180c85b5fd8f6

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 @@ public:
     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 @@ public:
     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 @@ public:
     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 @@ public:
     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());
+        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 = 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());
+            WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, master_key.key, master_key.key.GetPubKey()));
-            desc_spk_man->TopUp();
+            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 @@ public:
     /** 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 @@ public:
 
 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 @@ public:
     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 @@ public:
     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

src/wallet/wallet.cpp

@@ -1701,10 +1701,16 @@ bool CWallet::CanGetAddresses(bool internal) const
 }
 
 void CWallet::SetWalletFlag(uint64_t flags)
+{
+    WalletBatch batch(GetDatabase());
+    return SetWalletFlagWithDB(batch, flags);
+}
+
+void CWallet::SetWalletFlagWithDB(WalletBatch& batch, uint64_t flags)
 {
     LOCK(cs_wallet);
     m_wallet_flags |= flags;
-    if (!WalletBatch(GetDatabase()).WriteWalletFlags(m_wallet_flags))
+    if (!batch.WriteWalletFlags(m_wallet_flags))
         throw std::runtime_error(std::string(__func__) + ": writing wallet flags failed");
 }
 
@@ -2382,8 +2388,21 @@ DBErrors CWallet::LoadWallet()
 util::Result<void> CWallet::RemoveTxs(std::vector<uint256>& txs_to_remove)
 {
     AssertLockHeld(cs_wallet);
-    WalletBatch batch(GetDatabase());
+    bilingual_str str_err;  // future: make RunWithinTxn return a util::Result
-    if (!batch.TxnBegin()) return util::Error{_("Error starting db txn for wallet transactions removal")};
+    bool was_txn_committed = RunWithinTxn(GetDatabase(), /*process_desc=*/"remove transactions", [&](WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
+        util::Result<void> result{RemoveTxs(batch, txs_to_remove)};
+        if (!result) str_err = util::ErrorString(result);
+        return result.has_value();
+    });
+    if (!str_err.empty()) return util::Error{str_err};
+    if (!was_txn_committed) return util::Error{_("Error starting/committing db txn for wallet transactions removal process")};
+    return {}; // all good
+}
+
+util::Result<void> CWallet::RemoveTxs(WalletBatch& batch, std::vector<uint256>& txs_to_remove)
+{
+    AssertLockHeld(cs_wallet);
+    if (!batch.HasActiveTxn()) return util::Error{strprintf(_("The transactions removal process can only be executed within a db txn"))};
 
     // Check for transaction existence and remove entries from disk
     using TxIterator = std::unordered_map<uint256, CWalletTx, SaltedTxidHasher>::const_iterator;
@@ -2392,38 +2411,30 @@ util::Result<void> CWallet::RemoveTxs(std::vector<uint256>& txs_to_remove)
     for (const uint256& hash : txs_to_remove) {
         auto it_wtx = mapWallet.find(hash);
         if (it_wtx == mapWallet.end()) {
-            str_err = strprintf(_("Transaction %s does not belong to this wallet"), hash.GetHex());
+            return util::Error{strprintf(_("Transaction %s does not belong to this wallet"), hash.GetHex())};
-            break;
         }
         if (!batch.EraseTx(hash)) {
-            str_err = strprintf(_("Failure removing transaction: %s"), hash.GetHex());
+            return util::Error{strprintf(_("Failure removing transaction: %s"), hash.GetHex())};
-            break;
         }
         erased_txs.emplace_back(it_wtx);
     }
 
-    // Roll back removals in case of an error
+    // Register callback to update the memory state only when the db txn is actually dumped to disk
-    if (!str_err.empty()) {
+    batch.RegisterTxnListener({.on_commit=[&, erased_txs]() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
-        batch.TxnAbort();
+        // Update the in-memory state and notify upper layers about the removals
-        return util::Error{str_err};
+        for (const auto& it : erased_txs) {
-    }
+            const uint256 hash{it->first};
+            wtxOrdered.erase(it->second.m_it_wtxOrdered);
+            for (const auto& txin : it->second.tx->vin)
+                mapTxSpends.erase(txin.prevout);
+            mapWallet.erase(it);
+            NotifyTransactionChanged(hash, CT_DELETED);
+        }
 
-    // Dump changes to disk
+        MarkDirty();
-    if (!batch.TxnCommit()) return util::Error{_("Error committing db txn for wallet transactions removal")};
+    }, .on_abort={}});
 
-    // Update the in-memory state and notify upper layers about the removals
+    return {};
-    for (const auto& it : erased_txs) {
-        const uint256 hash{it->first};
-        wtxOrdered.erase(it->second.m_it_wtxOrdered);
-        for (const auto& txin : it->second.tx->vin)
-            mapTxSpends.erase(txin.prevout);
-        mapWallet.erase(it);
-        NotifyTransactionChanged(hash, CT_DELETED);
-    }
-
-    MarkDirty();
-
-    return {}; // all good
 }
 
 bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& new_purpose)
@@ -3736,22 +3747,30 @@ DescriptorScriptPubKeyMan& CWallet::SetupDescriptorScriptPubKeyMan(WalletBatch&
     return *out;
 }
 
-void CWallet::SetupDescriptorScriptPubKeyMans(const CExtKey& master_key)
+void CWallet::SetupDescriptorScriptPubKeyMans(WalletBatch& batch, const CExtKey& master_key)
 {
     AssertLockHeld(cs_wallet);
-
-    // Create single batch txn
-    WalletBatch batch(GetDatabase());
-    if (!batch.TxnBegin()) throw std::runtime_error("Error: cannot create db transaction for descriptors setup");
-
     for (bool internal : {false, true}) {
         for (OutputType t : OUTPUT_TYPES) {
             SetupDescriptorScriptPubKeyMan(batch, master_key, t, internal);
         }
     }
+}
 
-    // Ensure information is committed to disk
+void CWallet::SetupOwnDescriptorScriptPubKeyMans(WalletBatch& batch)
-    if (!batch.TxnCommit()) throw std::runtime_error("Error: cannot commit db transaction for descriptors setup");
+{
+    AssertLockHeld(cs_wallet);
+    assert(!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER));
+    // Make a seed
+    CKey seed_key = GenerateRandomKey();
+    CPubKey seed = seed_key.GetPubKey();
+    assert(seed_key.VerifyPubKey(seed));
+
+    // Get the extended key
+    CExtKey master_key;
+    master_key.SetSeed(seed_key);
+
+    SetupDescriptorScriptPubKeyMans(batch, master_key);
 }
 
 void CWallet::SetupDescriptorScriptPubKeyMans()
@@ -3759,16 +3778,10 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
     AssertLockHeld(cs_wallet);
 
     if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) {
-        // Make a seed
+        if (!RunWithinTxn(GetDatabase(), /*process_desc=*/"setup descriptors", [&](WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet){
-        CKey seed_key = GenerateRandomKey();
+            SetupOwnDescriptorScriptPubKeyMans(batch);
-        CPubKey seed = seed_key.GetPubKey();
+            return true;
-        assert(seed_key.VerifyPubKey(seed));
+        })) throw std::runtime_error("Error: cannot process db transaction for descriptors setup");
-
-        // Get the extended key
-        CExtKey master_key;
-        master_key.SetSeed(seed_key);
-
-        SetupDescriptorScriptPubKeyMans(master_key);
     } else {
         ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner();
 
@@ -4055,15 +4068,14 @@ std::optional<MigrationData> CWallet::GetDescriptorsForLegacy(bilingual_str& err
     return res;
 }
 
-bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
+util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, MigrationData& data)
 {
     AssertLockHeld(cs_wallet);
 
     LegacyDataSPKM* legacy_spkm = GetLegacyDataSPKM();
     if (!Assume(legacy_spkm)) {
         // This shouldn't happen
-        error = Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"));
+        return util::Error{Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"))};
-        return false;
     }
 
     // Get all invalid or non-watched scripts that will not be migrated
@@ -4077,16 +4089,15 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
 
     for (auto& desc_spkm : data.desc_spkms) {
         if (m_spk_managers.count(desc_spkm->GetID()) > 0) {
-            error = _("Error: Duplicate descriptors created during migration. Your wallet may be corrupted.");
+            return util::Error{_("Error: Duplicate descriptors created during migration. Your wallet may be corrupted.")};
-            return false;
         }
         uint256 id = desc_spkm->GetID();
         AddScriptPubKeyMan(id, std::move(desc_spkm));
     }
 
     // Remove the LegacyScriptPubKeyMan from disk
-    if (!legacy_spkm->DeleteRecords()) {
+    if (!legacy_spkm->DeleteRecordsWithDB(local_wallet_batch)) {
-        return false;
+        return util::Error{_("Error: cannot remove legacy wallet records")};
     }
 
     // Remove the LegacyScriptPubKeyMan from memory
@@ -4095,22 +4106,21 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
     m_internal_spk_managers.clear();
 
     // Setup new descriptors
-    SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
+    SetWalletFlagWithDB(local_wallet_batch, WALLET_FLAG_DESCRIPTORS);
     if (!IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
         // Use the existing master key if we have it
         if (data.master_key.key.IsValid()) {
-            SetupDescriptorScriptPubKeyMans(data.master_key);
+            SetupDescriptorScriptPubKeyMans(local_wallet_batch, data.master_key);
         } else {
             // Setup with a new seed if we don't.
-            SetupDescriptorScriptPubKeyMans();
+            SetupOwnDescriptorScriptPubKeyMans(local_wallet_batch);
         }
     }
 
     // Get best block locator so that we can copy it to the watchonly and solvables
     CBlockLocator best_block_locator;
-    if (!WalletBatch(GetDatabase()).ReadBestBlock(best_block_locator)) {
+    if (!local_wallet_batch.ReadBestBlock(best_block_locator)) {
-        error = _("Error: Unable to read wallet's best block locator record");
+        return util::Error{_("Error: Unable to read wallet's best block locator record")};
-        return false;
     }
 
     // Check if the transactions in the wallet are still ours. Either they belong here, or they belong in the watchonly wallet.
@@ -4119,21 +4129,23 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
     std::unique_ptr<WalletBatch> watchonly_batch;
     if (data.watchonly_wallet) {
         watchonly_batch = std::make_unique<WalletBatch>(data.watchonly_wallet->GetDatabase());
+        if (!watchonly_batch->TxnBegin()) return util::Error{strprintf(_("Error: database transaction cannot be executed for wallet %s"), data.watchonly_wallet->GetName())};
         // Copy the next tx order pos to the watchonly wallet
         LOCK(data.watchonly_wallet->cs_wallet);
         data.watchonly_wallet->nOrderPosNext = nOrderPosNext;
         watchonly_batch->WriteOrderPosNext(data.watchonly_wallet->nOrderPosNext);
         // Write the best block locator to avoid rescanning on reload
         if (!watchonly_batch->WriteBestBlock(best_block_locator)) {
-            error = _("Error: Unable to write watchonly wallet best block locator record");
+            return util::Error{_("Error: Unable to write watchonly wallet best block locator record")};
-            return false;
         }
     }
+    std::unique_ptr<WalletBatch> solvables_batch;
     if (data.solvable_wallet) {
+        solvables_batch = std::make_unique<WalletBatch>(data.solvable_wallet->GetDatabase());
+        if (!solvables_batch->TxnBegin()) return util::Error{strprintf(_("Error: database transaction cannot be executed for wallet %s"), data.solvable_wallet->GetName())};
         // Write the best block locator to avoid rescanning on reload
-        if (!WalletBatch(data.solvable_wallet->GetDatabase()).WriteBestBlock(best_block_locator)) {
+        if (!solvables_batch->WriteBestBlock(best_block_locator)) {
-            error = _("Error: Unable to write solvable wallet best block locator record");
+            return util::Error{_("Error: Unable to write solvable wallet best block locator record")};
-            return false;
         }
     }
     for (const auto& [_pos, wtx] : wtxOrdered) {
@@ -4152,8 +4164,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
                     ins_wtx.CopyFrom(to_copy_wtx);
                     return true;
                 })) {
-                    error = strprintf(_("Error: Could not add watchonly tx %s to watchonly wallet"), wtx->GetHash().GetHex());
+                    return util::Error{strprintf(_("Error: Could not add watchonly tx %s to watchonly wallet"), wtx->GetHash().GetHex())};
-                    return false;
                 }
                 watchonly_batch->WriteTx(data.watchonly_wallet->mapWallet.at(hash));
                 // Mark as to remove from the migrated wallet only if it does not also belong to it
@@ -4165,31 +4176,21 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
         }
         if (!is_mine) {
             // Both not ours and not in the watchonly wallet
-            error = strprintf(_("Error: Transaction %s in wallet cannot be identified to belong to migrated wallets"), wtx->GetHash().GetHex());
+            return util::Error{strprintf(_("Error: Transaction %s in wallet cannot be identified to belong to migrated wallets"), wtx->GetHash().GetHex())};
-            return false;
         }
     }
-    watchonly_batch.reset(); // Flush
+
     // Do the removes
     if (txids_to_delete.size() > 0) {
-        if (auto res = RemoveTxs(txids_to_delete); !res) {
+        if (auto res = RemoveTxs(local_wallet_batch, txids_to_delete); !res) {
-            error = _("Error: Could not delete watchonly transactions. ") + util::ErrorString(res);
+            return util::Error{_("Error: Could not delete watchonly transactions. ") + util::ErrorString(res)};
-            return false;
         }
     }
 
     // Pair external wallets with their corresponding db handler
     std::vector<std::pair<std::shared_ptr<CWallet>, std::unique_ptr<WalletBatch>>> wallets_vec;
-    for (const auto& ext_wallet : {data.watchonly_wallet, data.solvable_wallet}) {
+    if (data.watchonly_wallet) wallets_vec.emplace_back(data.watchonly_wallet, std::move(watchonly_batch));
-        if (!ext_wallet) continue;
+    if (data.solvable_wallet) wallets_vec.emplace_back(data.solvable_wallet, std::move(solvables_batch));
-
-        std::unique_ptr<WalletBatch> batch = std::make_unique<WalletBatch>(ext_wallet->GetDatabase());
-        if (!batch->TxnBegin()) {
-            error = strprintf(_("Error: database transaction cannot be executed for wallet %s"), ext_wallet->GetName());
-            return false;
-        }
-        wallets_vec.emplace_back(ext_wallet, std::move(batch));
-    }
 
     // Write address book entry to disk
     auto func_store_addr = [](WalletBatch& batch, const CTxDestination& dest, const CAddressBookData& entry) {
@@ -4236,39 +4237,27 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
                 continue;
             }
 
-            error = _("Error: Address book data in wallet cannot be identified to belong to migrated wallets");
+            return util::Error{_("Error: Address book data in wallet cannot be identified to belong to migrated wallets")};
-            return false;
         }
     }
 
     // Persist external wallets address book entries
     for (auto& [wallet, batch] : wallets_vec) {
         if (!batch->TxnCommit()) {
-            error = strprintf(_("Error: address book copy failed for wallet %s"), wallet->GetName());
+            return util::Error{strprintf(_("Error: Unable to write data to disk for wallet %s"), wallet->GetName())};
-            return false;
         }
     }
 
     // Remove the things to delete in this wallet
-    WalletBatch local_wallet_batch(GetDatabase());
-    local_wallet_batch.TxnBegin();
     if (dests_to_delete.size() > 0) {
         for (const auto& dest : dests_to_delete) {
             if (!DelAddressBookWithDB(local_wallet_batch, dest)) {
-                error = _("Error: Unable to remove watchonly address book data");
+                return util::Error{_("Error: Unable to remove watchonly address book data")};
-                return false;
             }
         }
     }
-    local_wallet_batch.TxnCommit();
 
-    // Connect the SPKM signals
+    return {}; // all good
-    ConnectScriptPubKeyManNotifiers();
-    NotifyCanGetAddressesChanged();
-
-    WalletLogPrintf("Wallet migration complete.\n");
-
-    return true;
 }
 
 bool CWallet::CanGrindR() const
@@ -4379,10 +4368,14 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
     }
 
     // Add the descriptors to wallet, remove LegacyScriptPubKeyMan, and cleanup txs and address book data
-    if (!wallet.ApplyMigrationData(*data, error)) {
+    return RunWithinTxn(wallet.GetDatabase(), /*process_desc=*/"apply migration process", [&](WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet){
-        return false;
+        if (auto res_migration = wallet.ApplyMigrationData(batch, *data); !res_migration) {
-    }
+            error = util::ErrorString(res_migration);
-    return true;
+            return false;
+        }
+        wallet.WalletLogPrintf("Wallet migration complete.\n");
+        return true;
+    });
 }
 
 util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& wallet_name, const SecureString& passphrase, WalletContext& context)

src/wallet/wallet.h

@@ -422,6 +422,9 @@ private:
     // Same as 'AddActiveScriptPubKeyMan' but designed for use within a batch transaction context
     void AddActiveScriptPubKeyManWithDb(WalletBatch& batch, uint256 id, OutputType type, bool internal);
 
+    /** Store wallet flags */
+    void SetWalletFlagWithDB(WalletBatch& batch, uint64_t flags);
+
     //! Cache of descriptor ScriptPubKeys used for IsMine. Maps ScriptPubKey to set of spkms
     std::unordered_map<CScript, std::vector<ScriptPubKeyMan*>, SaltedSipHasher> m_cached_spks;
 
@@ -791,6 +794,7 @@ public:
 
     /** Erases the provided transactions from the wallet. */
     util::Result<void> RemoveTxs(std::vector<uint256>& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    util::Result<void> RemoveTxs(WalletBatch& batch, std::vector<uint256>& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& purpose);
 
@@ -1018,9 +1022,12 @@ public:
     //! Create new DescriptorScriptPubKeyMan and add it to the wallet
     DescriptorScriptPubKeyMan& SetupDescriptorScriptPubKeyMan(WalletBatch& batch, const CExtKey& master_key, const OutputType& output_type, bool internal) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     //! Create new DescriptorScriptPubKeyMans and add them to the wallet
-    void SetupDescriptorScriptPubKeyMans(const CExtKey& master_key) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    void SetupDescriptorScriptPubKeyMans(WalletBatch& batch, const CExtKey& master_key) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     void SetupDescriptorScriptPubKeyMans() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
+    //! Create new seed and default DescriptorScriptPubKeyMans for this wallet
+    void SetupOwnDescriptorScriptPubKeyMans(WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+
     //! Return the DescriptorScriptPubKeyMan for a WalletDescriptor if it is already in the wallet
     DescriptorScriptPubKeyMan* GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const;
 
@@ -1044,7 +1051,7 @@ public:
 
     //! Adds the ScriptPubKeyMans given in MigrationData to this wallet, removes LegacyScriptPubKeyMan,
     //! and where needed, moves tx and address book entries to watchonly_wallet or solvable_wallet
-    bool ApplyMigrationData(MigrationData& data, bilingual_str& error) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    util::Result<void> ApplyMigrationData(WalletBatch& local_wallet_batch, MigrationData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     //! Whether the (external) signer performs R-value signature grinding
     bool CanGrindR() const;

src/wallet/walletdb.cpp

@@ -1335,10 +1335,8 @@ bool WalletBatch::WriteWalletFlags(const uint64_t flags)
 
 bool WalletBatch::EraseRecords(const std::unordered_set<std::string>& types)
 {
-    return RunWithinTxn(*this, "erase records", [&types](WalletBatch& self) {
+    return std::all_of(types.begin(), types.end(), [&](const std::string& type) {
-        return std::all_of(types.begin(), types.end(), [&self](const std::string& type) {
+        return m_batch->ErasePrefix(DataStream() << type);
-            return self.m_batch->ErasePrefix(DataStream() << type);
-        });
     });
 }
 
@@ -1349,12 +1347,34 @@ bool WalletBatch::TxnBegin()
 
 bool WalletBatch::TxnCommit()
 {
-    return m_batch->TxnCommit();
+    bool res = m_batch->TxnCommit();
+    if (res) {
+        for (const auto& listener : m_txn_listeners) {
+            listener.on_commit();
+        }
+        // txn finished, clear listeners
+        m_txn_listeners.clear();
+    }
+    return res;
 }
 
 bool WalletBatch::TxnAbort()
 {
-    return m_batch->TxnAbort();
+    bool res = m_batch->TxnAbort();
+    if (res) {
+        for (const auto& listener : m_txn_listeners) {
+            listener.on_abort();
+        }
+        // txn finished, clear listeners
+        m_txn_listeners.clear();
+    }
+    return res;
+}
+
+void WalletBatch::RegisterTxnListener(const DbTxnListener& l)
+{
+    assert(m_batch->HasActiveTxn());
+    m_txn_listeners.emplace_back(l);
 }
 
 std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error)

src/wallet/walletdb.h

@@ -180,6 +180,11 @@ public:
     }
 };
 
+struct DbTxnListener
+{
+    std::function<void()> on_commit, on_abort;
+};
+
 /** Access to the wallet database.
  * Opens the database and provides read and write access to it. Each read and write is its own transaction.
  * Multiple operation transactions can be started using TxnBegin() and committed using TxnCommit()
@@ -292,9 +297,18 @@ public:
     bool TxnCommit();
     //! Abort current transaction
     bool TxnAbort();
+    bool HasActiveTxn() { return m_batch->HasActiveTxn(); }
+
+    //! Registers db txn callback functions
+    void RegisterTxnListener(const DbTxnListener& l);
+
 private:
     std::unique_ptr<DatabaseBatch> m_batch;
     WalletDatabase& m_database;
+
+    // External functions listening to the current db txn outcome.
+    // Listeners are cleared at the end of the transaction.
+    std::vector<DbTxnListener> m_txn_listeners;
 };
 
 /**