From a082434d122754ec1a10e0e08a35bdb1f47989e6 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 18 Apr 2023 11:19:53 -0300 Subject: [PATCH 1/2] refactor: single method to append new spkm to the wallet --- src/wallet/wallet.cpp | 22 +++++++++++++++------- src/wallet/wallet.h | 4 ++++ 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b3eed8abc7a..dc8112c0817 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3498,6 +3498,11 @@ LegacyScriptPubKeyMan* CWallet::GetOrCreateLegacyScriptPubKeyMan() return GetLegacyScriptPubKeyMan(); } +void CWallet::AddScriptPubKeyMan(const uint256& id, std::unique_ptr spkm_man) +{ + m_spk_managers[id] = std::move(spkm_man); +} + void CWallet::SetupLegacyScriptPubKeyMan() { if (!m_internal_spk_managers.empty() || !m_external_spk_managers.empty() || !m_spk_managers.empty() || IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { @@ -3509,7 +3514,8 @@ void CWallet::SetupLegacyScriptPubKeyMan() m_internal_spk_managers[type] = spk_manager.get(); m_external_spk_managers[type] = spk_manager.get(); } - m_spk_managers[spk_manager->GetID()] = std::move(spk_manager); + uint256 id = spk_manager->GetID(); + AddScriptPubKeyMan(id, std::move(spk_manager)); } const CKeyingMaterial& CWallet::GetEncryptionKey() const @@ -3534,10 +3540,10 @@ void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc) { if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { auto spk_manager = std::unique_ptr(new ExternalSignerScriptPubKeyMan(*this, desc, m_keypool_size)); - m_spk_managers[id] = std::move(spk_manager); + AddScriptPubKeyMan(id, std::move(spk_manager)); } else { auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size)); - m_spk_managers[id] = std::move(spk_manager); + AddScriptPubKeyMan(id, std::move(spk_manager)); } } @@ -3558,7 +3564,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans(const CExtKey& master_key) } spk_manager->SetupDescriptorGeneration(master_key, t, internal); uint256 id = spk_manager->GetID(); - m_spk_managers[id] = std::move(spk_manager); + AddScriptPubKeyMan(id, std::move(spk_manager)); AddActiveScriptPubKeyMan(id, t, internal); } } @@ -3606,7 +3612,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans() auto spk_manager = std::unique_ptr(new ExternalSignerScriptPubKeyMan(*this, m_keypool_size)); spk_manager->SetupDescriptor(std::move(desc)); uint256 id = spk_manager->GetID(); - m_spk_managers[id] = std::move(spk_manager); + AddScriptPubKeyMan(id, std::move(spk_manager)); AddActiveScriptPubKeyMan(id, t, internal); } } @@ -3723,7 +3729,8 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat spk_man = new_spk_man.get(); // Save the descriptor to memory - m_spk_managers[new_spk_man->GetID()] = std::move(new_spk_man); + uint256 id = new_spk_man->GetID(); + AddScriptPubKeyMan(id, std::move(new_spk_man)); } // Add the private keys to the descriptor @@ -3866,7 +3873,8 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) error = _("Error: Duplicate descriptors created during migration. Your wallet may be corrupted."); return false; } - m_spk_managers[desc_spkm->GetID()] = std::move(desc_spkm); + uint256 id = desc_spkm->GetID(); + AddScriptPubKeyMan(id, std::move(desc_spkm)); } // Remove the LegacyScriptPubKeyMan from disk diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 79f4c434564..6aa67737e86 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -380,6 +380,10 @@ private: // ScriptPubKeyMan::GetID. In many cases it will be the hash of an internal structure std::map> m_spk_managers; + // Appends spk managers into the main 'm_spk_managers'. + // Must be the only method adding data to it. + void AddScriptPubKeyMan(const uint256& id, std::unique_ptr spkm_man); + /** * Catch wallet up to current chain, scanning new blocks, updating the best * block locator and m_last_block_processed, and registering for From 82bb7831fa6052620998c7eef47e48ed594248a8 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 13 Apr 2023 22:14:41 -0300 Subject: [PATCH 2/2] wallet: skip block scan if block was created before wallet birthday To avoid wasting processing power, we can skip blocks that occurred before the wallet's creation time, since these blocks are guaranteed not to contain any relevant wallet data. This has direct implications (an speed improvement) on the underlying blockchain synchronization process as well. The reason is that the validation interface queue is limited to 10 tasks per time. This means that no more than 10 blocks can be waiting for the wallet(s) to be processed while we are synchronizing the chain (activating the best chain to be more precise). Which can be a bottleneck if blocks arrive and are processed faster from the network than what they are processed by the wallet(s). --- src/bench/wallet_balance.cpp | 4 ++++ src/interfaces/chain.h | 3 +++ src/kernel/chain.cpp | 1 + src/wallet/scriptpubkeyman.cpp | 4 ++++ src/wallet/scriptpubkeyman.h | 3 +++ src/wallet/wallet.cpp | 34 ++++++++++++++++++++++++++++------ src/wallet/wallet.h | 7 +++++++ 7 files changed, 50 insertions(+), 6 deletions(-) diff --git a/src/bench/wallet_balance.cpp b/src/bench/wallet_balance.cpp index 2c77b9b22b2..6cf71f4b83b 100644 --- a/src/bench/wallet_balance.cpp +++ b/src/bench/wallet_balance.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -28,6 +29,9 @@ static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const b const auto& ADDRESS_WATCHONLY = ADDRESS_BCRT1_UNSPENDABLE; + // Set clock to genesis block, so the descriptors/keys creation time don't interfere with the blocks scanning process. + // The reason is 'generatetoaddress', which creates a chain with deterministic timestamps in the past. + SetMockTime(test_setup->m_node.chainman->GetParams().GenesisBlock().nTime); CWallet wallet{test_setup->m_node.chain.get(), "", CreateMockableWalletDatabase()}; { LOCK(wallet.cs_wallet); diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index a3fa753a980..40bf0b680c2 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -87,6 +87,9 @@ struct BlockInfo { unsigned data_pos = 0; const CBlock* data = nullptr; const CBlockUndo* undo_data = nullptr; + // The maximum time in the chain up to and including this block. + // A timestamp that can only move forward. + unsigned int chain_time_max{0}; BlockInfo(const uint256& hash LIFETIMEBOUND) : hash(hash) {} }; diff --git a/src/kernel/chain.cpp b/src/kernel/chain.cpp index 82e77125d7f..1c877866d0a 100644 --- a/src/kernel/chain.cpp +++ b/src/kernel/chain.cpp @@ -16,6 +16,7 @@ interfaces::BlockInfo MakeBlockInfo(const CBlockIndex* index, const CBlock* data if (index) { info.prev_hash = index->pprev ? index->pprev->phashBlock : nullptr; info.height = index->nHeight; + info.chain_time_max = index->GetBlockTimeMax(); LOCK(::cs_main); info.file_number = index->nFile; info.data_pos = index->nDataPos; diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 953fdb63b6f..1c8d65c5834 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -710,6 +710,8 @@ void LegacyScriptPubKeyMan::UpdateTimeFirstKey(int64_t nCreateTime) } else if (!nTimeFirstKey || nCreateTime < nTimeFirstKey) { nTimeFirstKey = nCreateTime; } + + NotifyFirstKeyTimeChanged(this, nTimeFirstKey); } bool LegacyScriptPubKeyMan::LoadKey(const CKey& key, const CPubKey &pubkey) @@ -2736,6 +2738,8 @@ void DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descrip m_map_script_pub_keys.clear(); m_max_cached_index = -1; m_wallet_descriptor = descriptor; + + NotifyFirstKeyTimeChanged(this, m_wallet_descriptor.creation_time); } bool DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor(const WalletDescriptor& descriptor, std::string& error) diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 22b67c88e95..5c7240823f6 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -256,6 +256,9 @@ public: /** Keypool has new keys */ boost::signals2::signal NotifyCanGetAddressesChanged; + + /** Birth time changed */ + boost::signals2::signal NotifyFirstKeyTimeChanged; }; /** OutputTypes supported by the LegacyScriptPubKeyMan */ diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index dc8112c0817..d5268afbf64 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1436,6 +1436,12 @@ void CWallet::blockConnected(const interfaces::BlockInfo& block) m_last_block_processed_height = block.height; m_last_block_processed = block.hash; + + // No need to scan block if it was created before the wallet birthday. + // Uses chain max time and twice the grace period to adjust time for block time variability. + if (block.chain_time_max < m_birth_time.load() - (TIMESTAMP_WINDOW * 2)) return; + + // Scan block for (size_t index = 0; index < block.data->vtx.size(); index++) { SyncTransaction(block.data->vtx[index], TxStateConfirmed{block.hash, block.height, static_cast(index)}); transactionRemovedFromMempool(block.data->vtx[index], MemPoolRemovalReason::BLOCK); @@ -1779,6 +1785,14 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set CWallet::Create(WalletContext& context, const std::stri // Try to top up keypool. No-op if the wallet is locked. walletInstance->TopUpKeyPool(); + // Cache the first key time + std::optional time_first_key; + for (auto spk_man : walletInstance->GetAllScriptPubKeyMans()) { + int64_t time = spk_man->GetTimeFirstKey(); + if (!time_first_key || time < *time_first_key) time_first_key = time; + } + if (time_first_key) walletInstance->m_birth_time = *time_first_key; + if (chain && !AttachChain(walletInstance, *chain, rescan_required, error, warnings)) { return nullptr; } @@ -3182,11 +3204,7 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf { // No need to read and scan block if block was created before // our wallet birthday (as adjusted for block time variability) - std::optional time_first_key; - for (auto spk_man : walletInstance->GetAllScriptPubKeyMans()) { - int64_t time = spk_man->GetTimeFirstKey(); - if (!time_first_key || time < *time_first_key) time_first_key = time; - } + std::optional time_first_key = walletInstance->m_birth_time.load(); if (time_first_key) { FoundBlock found = FoundBlock().height(rescan_height); chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, found); @@ -3500,7 +3518,10 @@ LegacyScriptPubKeyMan* CWallet::GetOrCreateLegacyScriptPubKeyMan() void CWallet::AddScriptPubKeyMan(const uint256& id, std::unique_ptr spkm_man) { - m_spk_managers[id] = std::move(spkm_man); + const auto& spkm = m_spk_managers[id] = std::move(spkm_man); + + // Update birth time if needed + FirstKeyTimeChanged(spkm.get(), spkm->GetTimeFirstKey()); } void CWallet::SetupLegacyScriptPubKeyMan() @@ -3533,6 +3554,7 @@ void CWallet::ConnectScriptPubKeyManNotifiers() for (const auto& spk_man : GetActiveScriptPubKeyMans()) { spk_man->NotifyWatchonlyChanged.connect(NotifyWatchonlyChanged); spk_man->NotifyCanGetAddressesChanged.connect(NotifyCanGetAddressesChanged); + spk_man->NotifyFirstKeyTimeChanged.connect(std::bind(&CWallet::FirstKeyTimeChanged, this, std::placeholders::_1, std::placeholders::_2)); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 6aa67737e86..e74eeb0a99d 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -299,6 +299,10 @@ private: // Local time that the tip block was received. Used to schedule wallet rebroadcasts. std::atomic m_best_block_time {0}; + // First created key time. Used to skip blocks prior to this time. + // 'std::numeric_limits::max()' if wallet is blank. + std::atomic m_birth_time{std::numeric_limits::max()}; + /** * Used to keep track of spent outpoints, and * detect and report conflicts (double-spends or @@ -645,6 +649,9 @@ public: bool ImportPubKeys(const std::vector& ordered_pubkeys, const std::map& pubkey_map, const std::map>& key_origins, const bool add_keypool, const bool internal, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool ImportScriptPubKeys(const std::string& label, const std::set& script_pub_keys, const bool have_solving_data, const bool apply_label, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Updates wallet birth time if 'new_birth_time' is below it */ + void FirstKeyTimeChanged(const ScriptPubKeyMan* spkm, int64_t new_birth_time); + CFeeRate m_pay_tx_fee{DEFAULT_PAY_TX_FEE}; unsigned int m_confirm_target{DEFAULT_TX_CONFIRM_TARGET}; /** Allow Coin Selection to pick unconfirmed UTXOs that were sent from our own wallet if it