From 66c9936455fc0b79629686fe6b5737a020662e5d Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 29 Sep 2023 17:23:48 -0300 Subject: [PATCH 01/12] bench: add coverage for wallet migration process --- src/bench/CMakeLists.txt | 1 + src/bench/wallet_migration.cpp | 80 ++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) create mode 100644 src/bench/wallet_migration.cpp diff --git a/src/bench/CMakeLists.txt b/src/bench/CMakeLists.txt index 8a52980e072..45b65100446 100644 --- a/src/bench/CMakeLists.txt +++ b/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() diff --git a/src/bench/wallet_migration.cpp b/src/bench/wallet_migration.cpp new file mode 100644 index 00000000000..eff6c6b526d --- /dev/null +++ b/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 // IWYU pragma: keep + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#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(); + + 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 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 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 From f2541d09e132ce17a5d166583be98f5cdbf06588 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 30 Sep 2023 08:57:18 -0300 Subject: [PATCH 02/12] wallet: batch MigrateToDescriptor() db transactions Grouping all db writes into a single atomic write operation. Speeding up the flow and preventing inconsistent states. --- src/wallet/scriptpubkeyman.cpp | 25 +++++++++++++++++++------ src/wallet/scriptpubkeyman.h | 1 + 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 46ec5dc1acc..29265365775 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1807,6 +1807,12 @@ std::optional 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 LegacyDataSPKM::MigrateToDescriptor() // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys auto desc_spk_man = std::make_unique(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 LegacyDataSPKM::MigrateToDescriptor() // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys auto desc_spk_man = std::make_unique(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 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,6 +2025,13 @@ std::optional 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; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index cf7b7eaf31d..39bb41d996f 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -582,6 +582,7 @@ class LegacySigningProvider : public SigningProvider class DescriptorScriptPubKeyMan : public ScriptPubKeyMan { + friend class LegacyDataSPKM; private: using ScriptPubKeyMap = std::map; // Map of scripts to descriptor range index using PubKeyMap = std::map; // Map of pubkeys involved in scripts to descriptor range index From 6052c7891dc033e30b342ae5ea1690f8e7cbeb9e Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 29 Sep 2023 16:12:04 -0300 Subject: [PATCH 03/12] wallet: decouple default descriptors creation from external signer setup This will be useful in the following-up commit to batch the entire wallet migration process. --- src/wallet/wallet.cpp | 29 ++++++++++++++++++----------- src/wallet/wallet.h | 3 +++ 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index de565102cc2..75b258895dd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3754,21 +3754,28 @@ void CWallet::SetupDescriptorScriptPubKeyMans(const CExtKey& master_key) if (!batch.TxnCommit()) throw std::runtime_error("Error: cannot commit db transaction for descriptors setup"); } -void CWallet::SetupDescriptorScriptPubKeyMans() +void CWallet::SetupOwnDescriptorScriptPubKeyMans() { 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)); - if (!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); - // Get the extended key - CExtKey master_key; - master_key.SetSeed(seed_key); + SetupDescriptorScriptPubKeyMans(master_key); +} - SetupDescriptorScriptPubKeyMans(master_key); +void CWallet::SetupDescriptorScriptPubKeyMans() +{ + AssertLockHeld(cs_wallet); + + if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { + SetupOwnDescriptorScriptPubKeyMans(); } else { ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); @@ -4102,7 +4109,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) SetupDescriptorScriptPubKeyMans(data.master_key); } else { // Setup with a new seed if we don't. - SetupDescriptorScriptPubKeyMans(); + SetupOwnDescriptorScriptPubKeyMans(); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d3a7208b15e..f70f9ce2cf3 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1021,6 +1021,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati void SetupDescriptorScriptPubKeyMans(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() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + //! Return the DescriptorScriptPubKeyMan for a WalletDescriptor if it is already in the wallet DescriptorScriptPubKeyMan* GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const; From 122d103ca22f347eef7b323910477b72736c64b9 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 29 Sep 2023 16:18:56 -0300 Subject: [PATCH 04/12] wallet: introduce 'SetWalletFlagWithDB' --- src/wallet/wallet.cpp | 8 +++++++- src/wallet/wallet.h | 3 +++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 75b258895dd..6695edc70ec 100644 --- a/src/wallet/wallet.cpp +++ b/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"); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index f70f9ce2cf3..0b759ee56a4 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -422,6 +422,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati // 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, SaltedSipHasher> m_cached_spks; From 055c0532fc8bb3135102590b46e266b68e9d9edc Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 13 Feb 2024 12:08:15 -0300 Subject: [PATCH 05/12] wallet: provide WalletBatch to 'DeleteRecords' So it can be used within an external db txn context. --- src/wallet/scriptpubkeyman.cpp | 8 +++++++- src/wallet/scriptpubkeyman.h | 3 ++- src/wallet/walletdb.cpp | 6 ++---- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 29265365775..62384056dc6 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -2036,9 +2036,15 @@ std::optional LegacyDataSPKM::MigrateToDescriptor() } 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); } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 39bb41d996f..d8b6c90178a 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/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 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 diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 597a4ef9a4c..7f092b44b16 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1335,10 +1335,8 @@ bool WalletBatch::WriteWalletFlags(const uint64_t flags) bool WalletBatch::EraseRecords(const std::unordered_set& types) { - return RunWithinTxn(*this, "erase records", [&types](WalletBatch& self) { - return std::all_of(types.begin(), types.end(), [&self](const std::string& type) { - return self.m_batch->ErasePrefix(DataStream() << type); - }); + return std::all_of(types.begin(), types.end(), [&](const std::string& type) { + return m_batch->ErasePrefix(DataStream() << type); }); } From 91e065ec175e3d40439230d0a70b2c2c73ff024b Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 24 Mar 2024 01:03:51 -0300 Subject: [PATCH 06/12] wallet: remove post-migration signals connection The wallet is isolated during migration and reloaded at the end of the process. There is no benefit on connecting the signals few lines before unloading the wallet. --- src/wallet/wallet.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6695edc70ec..680b4b651b4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4275,10 +4275,6 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) } local_wallet_batch.TxnCommit(); - // Connect the SPKM signals - ConnectScriptPubKeyManNotifiers(); - NotifyCanGetAddressesChanged(); - WalletLogPrintf("Wallet migration complete.\n"); return true; From 57249ff669748d60895c275255dfb9bffaecbb67 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 26 Mar 2024 10:48:30 -0300 Subject: [PATCH 07/12] wallet: introduce active db txn listeners Useful to ensure that the in-memory state is updated only after successfully committing the data to disk. --- src/wallet/bdb.h | 1 + src/wallet/db.h | 1 + src/wallet/migrate.h | 1 + src/wallet/salvage.cpp | 1 + src/wallet/sqlite.h | 1 + src/wallet/test/util.h | 1 + src/wallet/walletdb.cpp | 26 ++++++++++++++++++++++++-- src/wallet/walletdb.h | 14 ++++++++++++++ 8 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index af0c78f0d9e..f3fe8a19c19 100644 --- a/src/wallet/bdb.h +++ b/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; } }; diff --git a/src/wallet/db.h b/src/wallet/db.h index 049af8dd19e..e8790006a4d 100644 --- a/src/wallet/db.h +++ b/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. diff --git a/src/wallet/migrate.h b/src/wallet/migrate.h index 58c8c0adf4e..16eadeb019d 100644 --- a/src/wallet/migrate.h +++ b/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. diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index 04c02b0dcc6..0ac1b66897e 100644 --- a/src/wallet/salvage.cpp +++ b/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. diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h index 6b84f34366f..78a3accf890 100644 --- a/src/wallet/sqlite.h +++ b/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. diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h index ba12f5f6bf1..c8a89c0e642 100644 --- a/src/wallet/test/util.h +++ b/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 diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 7f092b44b16..b09a0e40eb4 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1347,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 MakeDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error) diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index bffcc87202a..32c3c29b5e3 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -180,6 +180,11 @@ class CKeyMetadata } }; +struct DbTxnListener +{ + std::function 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 @@ class WalletBatch 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 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 m_txn_listeners; }; /** From aacaaaa0d3aeb576e01e2b4e6b3b094ab10f602a Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 26 Mar 2024 10:37:12 -0300 Subject: [PATCH 08/12] wallet: provide WalletBatch to 'RemoveTxs' Preparing it to be used within a broader db txn procedure. --- src/wallet/wallet.cpp | 57 +++++++++++++++++++++++-------------------- src/wallet/wallet.h | 1 + 2 files changed, 32 insertions(+), 26 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 680b4b651b4..7f2b4d3f497 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2388,8 +2388,21 @@ DBErrors CWallet::LoadWallet() util::Result CWallet::RemoveTxs(std::vector& txs_to_remove) { AssertLockHeld(cs_wallet); - WalletBatch batch(GetDatabase()); - if (!batch.TxnBegin()) return util::Error{_("Error starting db txn for wallet transactions removal")}; + bilingual_str str_err; // future: make RunWithinTxn return a util::Result + bool was_txn_committed = RunWithinTxn(GetDatabase(), /*process_desc=*/"remove transactions", [&](WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { + util::Result 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 CWallet::RemoveTxs(WalletBatch& batch, std::vector& 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::const_iterator; @@ -2398,38 +2411,30 @@ util::Result CWallet::RemoveTxs(std::vector& 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()); - break; + return util::Error{strprintf(_("Transaction %s does not belong to this wallet"), hash.GetHex())}; } if (!batch.EraseTx(hash)) { - str_err = strprintf(_("Failure removing transaction: %s"), hash.GetHex()); - break; + return util::Error{strprintf(_("Failure removing transaction: %s"), hash.GetHex())}; } erased_txs.emplace_back(it_wtx); } - // Roll back removals in case of an error - if (!str_err.empty()) { - batch.TxnAbort(); - return util::Error{str_err}; - } - - // Dump changes to disk - if (!batch.TxnCommit()) return util::Error{_("Error committing db txn for wallet transactions removal")}; - - // Update the in-memory state and notify upper layers about the removals - 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); - } + // Register callback to update the memory state only when the db txn is actually dumped to disk + batch.RegisterTxnListener({.on_commit=[&, erased_txs]() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { + // Update the in-memory state and notify upper layers about the removals + 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(); + MarkDirty(); + }, .on_abort={}}); - return {}; // all good + return {}; } bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& address, const std::string& strName, const std::optional& new_purpose) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 0b759ee56a4..7f726bfa0bf 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -794,6 +794,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati /** Erases the provided transactions from the wallet. */ util::Result RemoveTxs(std::vector& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + util::Result RemoveTxs(WalletBatch& batch, std::vector& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional& purpose); From 34bf0795fc0fe30bd04a5b3a89b779f502189262 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 13 Feb 2024 12:21:04 -0300 Subject: [PATCH 09/12] wallet: refactor ApplyMigrationData to return util::Result --- src/wallet/wallet.cpp | 45 ++++++++++++++++--------------------------- src/wallet/wallet.h | 2 +- 2 files changed, 18 insertions(+), 29 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7f2b4d3f497..cf54d3c194c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4073,15 +4073,14 @@ std::optional CWallet::GetDescriptorsForLegacy(bilingual_str& err return res; } -bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) +util::Result CWallet::ApplyMigrationData(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 false; + return util::Error{Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"))}; } // Get all invalid or non-watched scripts that will not be migrated @@ -4095,8 +4094,7 @@ 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 false; + return util::Error{_("Error: Duplicate descriptors created during migration. Your wallet may be corrupted.")}; } uint256 id = desc_spkm->GetID(); AddScriptPubKeyMan(id, std::move(desc_spkm)); @@ -4104,7 +4102,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) // Remove the LegacyScriptPubKeyMan from disk if (!legacy_spkm->DeleteRecords()) { - return false; + return util::Error{_("Error: cannot remove legacy wallet records")}; } // Remove the LegacyScriptPubKeyMan from memory @@ -4127,8 +4125,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) // 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)) { - error = _("Error: Unable to read wallet's best block locator record"); - return false; + return util::Error{_("Error: Unable to read wallet's best block locator record")}; } // Check if the transactions in the wallet are still ours. Either they belong here, or they belong in the watchonly wallet. @@ -4143,15 +4140,13 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) 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 false; + return util::Error{_("Error: Unable to write watchonly wallet best block locator record")}; } } if (data.solvable_wallet) { // Write the best block locator to avoid rescanning on reload if (!WalletBatch(data.solvable_wallet->GetDatabase()).WriteBestBlock(best_block_locator)) { - error = _("Error: Unable to write solvable wallet best block locator record"); - return false; + return util::Error{_("Error: Unable to write solvable wallet best block locator record")}; } } for (const auto& [_pos, wtx] : wtxOrdered) { @@ -4170,8 +4165,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 false; + return util::Error{strprintf(_("Error: Could not add watchonly tx %s to watchonly wallet"), wtx->GetHash().GetHex())}; } 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 @@ -4183,16 +4177,14 @@ 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 false; + return util::Error{strprintf(_("Error: Transaction %s in wallet cannot be identified to belong to migrated wallets"), wtx->GetHash().GetHex())}; } } watchonly_batch.reset(); // Flush // Do the removes if (txids_to_delete.size() > 0) { if (auto res = RemoveTxs(txids_to_delete); !res) { - error = _("Error: Could not delete watchonly transactions. ") + util::ErrorString(res); - return false; + return util::Error{_("Error: Could not delete watchonly transactions. ") + util::ErrorString(res)}; } } @@ -4203,8 +4195,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) std::unique_ptr batch = std::make_unique(ext_wallet->GetDatabase()); if (!batch->TxnBegin()) { - error = strprintf(_("Error: database transaction cannot be executed for wallet %s"), ext_wallet->GetName()); - return false; + return util::Error{strprintf(_("Error: database transaction cannot be executed for wallet %s"), ext_wallet->GetName())}; } wallets_vec.emplace_back(ext_wallet, std::move(batch)); } @@ -4254,16 +4245,14 @@ 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 false; + return util::Error{_("Error: Address book data in wallet cannot be identified to belong to migrated wallets")}; } } // 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 false; + return util::Error{strprintf(_("Error: address book copy failed for wallet %s"), wallet->GetName())}; } } @@ -4273,8 +4262,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) 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 false; + return util::Error{_("Error: Unable to remove watchonly address book data")}; } } } @@ -4282,7 +4270,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) WalletLogPrintf("Wallet migration complete.\n"); - return true; + return {}; // all good } bool CWallet::CanGrindR() const @@ -4393,7 +4381,8 @@ 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)) { + if (auto res_migration = wallet.ApplyMigrationData(*data); !res_migration) { + error = util::ErrorString(res_migration); return false; } return true; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 7f726bfa0bf..26d470f4099 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1051,7 +1051,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati //! 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 ApplyMigrationData(MigrationData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Whether the (external) signer performs R-value signature grinding bool CanGrindR() const; From 9ef20e86d7fd5d775c463a7a752c5bba9ef1266b Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 3 Jul 2024 17:22:00 -0300 Subject: [PATCH 10/12] wallet: provide WalletBatch to 'SetupDescriptorScriptPubKeyMans' So it can be used within an external db txn context. --- src/wallet/wallet.cpp | 24 ++++++++++-------------- src/wallet/wallet.h | 4 ++-- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index cf54d3c194c..e00537e9204 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3747,25 +3747,17 @@ 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 - if (!batch.TxnCommit()) throw std::runtime_error("Error: cannot commit db transaction for descriptors setup"); } -void CWallet::SetupOwnDescriptorScriptPubKeyMans() +void CWallet::SetupOwnDescriptorScriptPubKeyMans(WalletBatch& batch) { AssertLockHeld(cs_wallet); assert(!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)); @@ -3778,7 +3770,7 @@ void CWallet::SetupOwnDescriptorScriptPubKeyMans() CExtKey master_key; master_key.SetSeed(seed_key); - SetupDescriptorScriptPubKeyMans(master_key); + SetupDescriptorScriptPubKeyMans(batch, master_key); } void CWallet::SetupDescriptorScriptPubKeyMans() @@ -3786,7 +3778,10 @@ void CWallet::SetupDescriptorScriptPubKeyMans() AssertLockHeld(cs_wallet); if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { - SetupOwnDescriptorScriptPubKeyMans(); + if (!RunWithinTxn(GetDatabase(), /*process_desc=*/"setup descriptors", [&](WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet){ + SetupOwnDescriptorScriptPubKeyMans(batch); + return true; + })) throw std::runtime_error("Error: cannot process db transaction for descriptors setup"); } else { ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); @@ -4113,12 +4108,13 @@ util::Result CWallet::ApplyMigrationData(MigrationData& data) // Setup new descriptors SetWalletFlag(WALLET_FLAG_DESCRIPTORS); if (!IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + WalletBatch local_wallet_batch(GetDatabase()); // 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. - SetupOwnDescriptorScriptPubKeyMans(); + SetupOwnDescriptorScriptPubKeyMans(local_wallet_batch); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 26d470f4099..1d350fb210b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1022,11 +1022,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati //! 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() EXCLUSIVE_LOCKS_REQUIRED(cs_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; From 7c9076a2d2e5dd117c31ca871e81c9170aa0e371 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 13 Feb 2024 12:34:16 -0300 Subject: [PATCH 11/12] wallet: migration, consolidate main wallet db writes Perform a single db write operation for the entire migration procedure. --- src/wallet/wallet.cpp | 29 +++++++++++++---------------- src/wallet/wallet.h | 2 +- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e00537e9204..9c3b05b444d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4068,7 +4068,7 @@ std::optional CWallet::GetDescriptorsForLegacy(bilingual_str& err return res; } -util::Result CWallet::ApplyMigrationData(MigrationData& data) +util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, MigrationData& data) { AssertLockHeld(cs_wallet); @@ -4096,7 +4096,7 @@ util::Result CWallet::ApplyMigrationData(MigrationData& data) } // Remove the LegacyScriptPubKeyMan from disk - if (!legacy_spkm->DeleteRecords()) { + if (!legacy_spkm->DeleteRecordsWithDB(local_wallet_batch)) { return util::Error{_("Error: cannot remove legacy wallet records")}; } @@ -4106,9 +4106,8 @@ util::Result CWallet::ApplyMigrationData(MigrationData& data) 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)) { - WalletBatch local_wallet_batch(GetDatabase()); // Use the existing master key if we have it if (data.master_key.key.IsValid()) { SetupDescriptorScriptPubKeyMans(local_wallet_batch, data.master_key); @@ -4120,7 +4119,7 @@ util::Result CWallet::ApplyMigrationData(MigrationData& data) // 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)) { return util::Error{_("Error: Unable to read wallet's best block locator record")}; } @@ -4179,7 +4178,7 @@ util::Result CWallet::ApplyMigrationData(MigrationData& data) 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) { return util::Error{_("Error: Could not delete watchonly transactions. ") + util::ErrorString(res)}; } } @@ -4253,8 +4252,6 @@ util::Result CWallet::ApplyMigrationData(MigrationData& data) } // 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)) { @@ -4262,9 +4259,6 @@ util::Result CWallet::ApplyMigrationData(MigrationData& data) } } } - local_wallet_batch.TxnCommit(); - - WalletLogPrintf("Wallet migration complete.\n"); return {}; // all good } @@ -4377,11 +4371,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 (auto res_migration = wallet.ApplyMigrationData(*data); !res_migration) { - error = util::ErrorString(res_migration); - return false; - } - return true; + return RunWithinTxn(wallet.GetDatabase(), /*process_desc=*/"apply migration process", [&](WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet){ + if (auto res_migration = wallet.ApplyMigrationData(batch, *data); !res_migration) { + error = util::ErrorString(res_migration); + return false; + } + wallet.WalletLogPrintf("Wallet migration complete.\n"); + return true; + }); } util::Result MigrateLegacyToDescriptor(const std::string& wallet_name, const SecureString& passphrase, WalletContext& context) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 1d350fb210b..d869f031bbf 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1051,7 +1051,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati //! 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 - util::Result ApplyMigrationData(MigrationData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + util::Result ApplyMigrationData(WalletBatch& local_wallet_batch, MigrationData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Whether the (external) signer performs R-value signature grinding bool CanGrindR() const; From c98fc36d094a08d44f3c95431db2c5f34a96cc73 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 3 Jul 2024 18:48:34 -0300 Subject: [PATCH 12/12] wallet: migration, consolidate external wallets db writes Perform a single db write operation for each external wallet (watch-only and solvables) for the entire migration procedure. --- src/wallet/wallet.cpp | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9c3b05b444d..652d6e9f6d8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4129,6 +4129,7 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, std::unique_ptr watchonly_batch; if (data.watchonly_wallet) { watchonly_batch = std::make_unique(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; @@ -4138,9 +4139,12 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, return util::Error{_("Error: Unable to write watchonly wallet best block locator record")}; } } + std::unique_ptr solvables_batch; if (data.solvable_wallet) { + solvables_batch = std::make_unique(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)) { return util::Error{_("Error: Unable to write solvable wallet best block locator record")}; } } @@ -4175,7 +4179,7 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, return util::Error{strprintf(_("Error: Transaction %s in wallet cannot be identified to belong to migrated wallets"), wtx->GetHash().GetHex())}; } } - watchonly_batch.reset(); // Flush + // Do the removes if (txids_to_delete.size() > 0) { if (auto res = RemoveTxs(local_wallet_batch, txids_to_delete); !res) { @@ -4185,15 +4189,8 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, // Pair external wallets with their corresponding db handler std::vector, std::unique_ptr>> wallets_vec; - for (const auto& ext_wallet : {data.watchonly_wallet, data.solvable_wallet}) { - if (!ext_wallet) continue; - - std::unique_ptr batch = std::make_unique(ext_wallet->GetDatabase()); - if (!batch->TxnBegin()) { - return util::Error{strprintf(_("Error: database transaction cannot be executed for wallet %s"), ext_wallet->GetName())}; - } - wallets_vec.emplace_back(ext_wallet, std::move(batch)); - } + if (data.watchonly_wallet) wallets_vec.emplace_back(data.watchonly_wallet, std::move(watchonly_batch)); + if (data.solvable_wallet) wallets_vec.emplace_back(data.solvable_wallet, std::move(solvables_batch)); // Write address book entry to disk auto func_store_addr = [](WalletBatch& batch, const CTxDestination& dest, const CAddressBookData& entry) { @@ -4247,7 +4244,7 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, // Persist external wallets address book entries for (auto& [wallet, batch] : wallets_vec) { if (!batch->TxnCommit()) { - return util::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())}; } }