From 456e350926adde5dabdbc85fc0f017fb29bdadb3 Mon Sep 17 00:00:00 2001 From: S3RK <1466284+S3RK@users.noreply.github.com> Date: Tue, 5 Oct 2021 09:45:45 +0200 Subject: wallet: resolve ambiguity of two ScriptPubKey managers providing same script --- src/wallet/rpcwallet.cpp | 7 +++++-- src/wallet/wallet.cpp | 31 ++++++++++--------------------- src/wallet/wallet.h | 7 ++----- 3 files changed, 17 insertions(+), 28 deletions(-) (limited to 'src') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 7bc2dc7b21..1a872323be 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3982,8 +3982,12 @@ RPCHelpMan getaddressinfo() ret.pushKV("solvable", false); } + const auto& spk_mans = pwallet->GetScriptPubKeyMans(scriptPubKey); + // In most cases there is only one matching ScriptPubKey manager and we can't resolve ambiguity in a better way + ScriptPubKeyMan* spk_man{nullptr}; + if (spk_mans.size()) spk_man = *spk_mans.begin(); - DescriptorScriptPubKeyMan* desc_spk_man = dynamic_cast(pwallet->GetScriptPubKeyMan(scriptPubKey)); + DescriptorScriptPubKeyMan* desc_spk_man = dynamic_cast(spk_man); if (desc_spk_man) { std::string desc_str; if (desc_spk_man->GetDescriptorString(desc_str, /* priv */ false)) { @@ -3998,7 +4002,6 @@ RPCHelpMan getaddressinfo() ret.pushKV("ischange", ScriptIsChange(*pwallet, scriptPubKey)); - ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan(scriptPubKey); if (spk_man) { if (const std::unique_ptr meta = spk_man->GetMetadata(dest)) { ret.pushKV("timestamp", meta->nCreateTime); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6885499be2..a58f09b953 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2253,16 +2253,15 @@ void ReserveDestination::ReturnDestination() bool CWallet::DisplayAddress(const CTxDestination& dest) { CScript scriptPubKey = GetScriptForDestination(dest); - const auto spk_man = GetScriptPubKeyMan(scriptPubKey); - if (spk_man == nullptr) { - return false; - } - auto signer_spk_man = dynamic_cast(spk_man); - if (signer_spk_man == nullptr) { - return false; + for (const auto& spk_man : GetScriptPubKeyMans(scriptPubKey)) { + auto signer_spk_man = dynamic_cast(spk_man); + if (signer_spk_man == nullptr) { + continue; + } + ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); + return signer_spk_man->DisplayAddress(scriptPubKey, signer); } - ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); - return signer_spk_man->DisplayAddress(scriptPubKey, signer); + return false; } bool CWallet::LockCoin(const COutPoint& output, WalletBatch* batch) @@ -3035,9 +3034,10 @@ ScriptPubKeyMan* CWallet::GetScriptPubKeyMan(const OutputType& type, bool intern return it->second; } -std::set CWallet::GetScriptPubKeyMans(const CScript& script, SignatureData& sigdata) const +std::set CWallet::GetScriptPubKeyMans(const CScript& script) const { std::set spk_mans; + SignatureData sigdata; for (const auto& spk_man_pair : m_spk_managers) { if (spk_man_pair.second->CanProvide(script, sigdata)) { spk_mans.insert(spk_man_pair.second.get()); @@ -3046,17 +3046,6 @@ std::set CWallet::GetScriptPubKeyMans(const CScript& script, S return spk_mans; } -ScriptPubKeyMan* CWallet::GetScriptPubKeyMan(const CScript& script) const -{ - SignatureData sigdata; - for (const auto& spk_man_pair : m_spk_managers) { - if (spk_man_pair.second->CanProvide(script, sigdata)) { - return spk_man_pair.second.get(); - } - } - return nullptr; -} - ScriptPubKeyMan* CWallet::GetScriptPubKeyMan(const uint256& id) const { if (m_spk_managers.count(id) > 0) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 15a5933424..ff6229b919 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -805,14 +805,11 @@ public: //! Get the ScriptPubKeyMan for the given OutputType and internal/external chain. ScriptPubKeyMan* GetScriptPubKeyMan(const OutputType& type, bool internal) const; - //! Get the ScriptPubKeyMan for a script - ScriptPubKeyMan* GetScriptPubKeyMan(const CScript& script) const; + //! Get all the ScriptPubKeyMans for a script + std::set GetScriptPubKeyMans(const CScript& script) const; //! Get the ScriptPubKeyMan by id ScriptPubKeyMan* GetScriptPubKeyMan(const uint256& id) const; - //! Get all of the ScriptPubKeyMans for a script given additional information in sigdata (populated by e.g. a psbt) - std::set GetScriptPubKeyMans(const CScript& script, SignatureData& sigdata) const; - //! Get the SigningProvider for a script std::unique_ptr GetSolvingProvider(const CScript& script) const; std::unique_ptr GetSolvingProvider(const CScript& script, SignatureData& sigdata) const; -- cgit v1.2.3 From 03840c20640685295a65ed8c82456e877f668b9b Mon Sep 17 00:00:00 2001 From: S3RK <1466284+S3RK@users.noreply.github.com> Date: Wed, 8 Sep 2021 09:58:53 +0200 Subject: Add CWallet::IsInternalScriptPubKeyMan --- src/wallet/rpcdump.cpp | 9 ++++----- src/wallet/wallet.cpp | 24 ++++++++++++++++++++++++ src/wallet/wallet.h | 5 +++++ 3 files changed, 33 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 382e8b6116..16cd2d0e3c 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -1819,11 +1819,10 @@ RPCHelpMan listdescriptors() } spk.pushKV("desc", descriptor); spk.pushKV("timestamp", wallet_descriptor.creation_time); - const bool active = active_spk_mans.count(desc_spk_man) != 0; - spk.pushKV("active", active); - const auto& type = wallet_descriptor.descriptor->GetOutputType(); - if (active && type) { - spk.pushKV("internal", wallet->GetScriptPubKeyMan(*type, true) == desc_spk_man); + spk.pushKV("active", active_spk_mans.count(desc_spk_man) != 0); + const auto internal = wallet->IsInternalScriptPubKeyMan(desc_spk_man); + if (internal.has_value()) { + spk.pushKV("internal", *internal); } if (wallet_descriptor.descriptor->IsRange()) { UniValue range(UniValue::VARR); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a58f09b953..c67a8f77da 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3265,6 +3265,30 @@ DescriptorScriptPubKeyMan* CWallet::GetDescriptorScriptPubKeyMan(const WalletDes return nullptr; } +std::optional CWallet::IsInternalScriptPubKeyMan(ScriptPubKeyMan* spk_man) const +{ + // Legacy script pubkey man can't be either external or internal + if (IsLegacy()) { + return std::nullopt; + } + + // only active ScriptPubKeyMan can be internal + if (!GetActiveScriptPubKeyMans().count(spk_man)) { + return std::nullopt; + } + + const auto desc_spk_man = dynamic_cast(spk_man); + if (!desc_spk_man) { + throw std::runtime_error(std::string(__func__) + ": unexpected ScriptPubKeyMan type."); + } + + LOCK(desc_spk_man->cs_desc_man); + const auto& type = desc_spk_man->GetWalletDescriptor().descriptor->GetOutputType(); + assert(type.has_value()); + + return GetScriptPubKeyMan(*type, /* internal= */ true) == desc_spk_man; +} + ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const FlatSigningProvider& signing_provider, const std::string& label, bool internal) { AssertLockHeld(cs_wallet); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ff6229b919..27cd3ab230 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -875,6 +875,11 @@ public: //! Return the DescriptorScriptPubKeyMan for a WalletDescriptor if it is already in the wallet DescriptorScriptPubKeyMan* GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const; + //! Returns whether the provided ScriptPubKeyMan is internal + //! @param[in] spk_man The ScriptPubKeyMan to test + //! @return contains value only for active DescriptorScriptPubKeyMan, otherwise undefined + std::optional IsInternalScriptPubKeyMan(ScriptPubKeyMan* spk_man) const; + //! Add a descriptor to the wallet, return a ScriptPubKeyMan & associated output type ScriptPubKeyMan* AddWalletDescriptor(WalletDescriptor& desc, const FlatSigningProvider& signing_provider, const std::string& label, bool internal) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); }; -- cgit v1.2.3 From c1b99c088c54eb101c0a28a67237965576ccf5ad Mon Sep 17 00:00:00 2001 From: S3RK <1466284+S3RK@users.noreply.github.com> Date: Wed, 8 Sep 2021 10:00:18 +0200 Subject: Return used destinations from ScriptPubKeyMan::MarkUnusedAddresses --- src/wallet/scriptpubkeyman.cpp | 36 +++++++++++++++++++++++++++++++----- src/wallet/scriptpubkeyman.h | 28 ++++++++++++++++++++++------ 2 files changed, 53 insertions(+), 11 deletions(-) (limited to 'src') diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index fdfb36bb0a..2894ff7c61 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -354,15 +354,22 @@ bool LegacyScriptPubKeyMan::TopUpInactiveHDChain(const CKeyID seed_id, int64_t i return true; } -void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script) +std::vector LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script) { LOCK(cs_KeyStore); + std::vector result; // extract addresses and check if they match with an unused keypool key for (const auto& keyid : GetAffectedKeys(script, *this)) { std::map::const_iterator mi = m_pool_key_to_index.find(keyid); if (mi != m_pool_key_to_index.end()) { WalletLogPrintf("%s: Detected a used keypool key, mark all keypool keys up to this key as used\n", __func__); - MarkReserveKeysAsUsed(mi->second); + for (const auto& keypool : MarkReserveKeysAsUsed(mi->second)) { + // derive all possible destinations as any of them could have been used + for (const auto& type : LEGACY_OUTPUT_TYPES) { + const auto& dest = GetDestinationForKey(keypool.vchPubKey, type); + result.push_back({dest, keypool.fInternal}); + } + } if (!TopUp()) { WalletLogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__); @@ -384,6 +391,8 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script) } } } + + return result; } void LegacyScriptPubKeyMan::UpgradeKeyMetadata() @@ -1427,7 +1436,7 @@ void LegacyScriptPubKeyMan::LearnAllRelatedScripts(const CPubKey& key) LearnRelatedScripts(key, OutputType::P2SH_SEGWIT); } -void LegacyScriptPubKeyMan::MarkReserveKeysAsUsed(int64_t keypool_id) +std::vector LegacyScriptPubKeyMan::MarkReserveKeysAsUsed(int64_t keypool_id) { AssertLockHeld(cs_KeyStore); bool internal = setInternalKeyPool.count(keypool_id); @@ -1435,6 +1444,7 @@ void LegacyScriptPubKeyMan::MarkReserveKeysAsUsed(int64_t keypool_id) std::set *setKeyPool = internal ? &setInternalKeyPool : (set_pre_split_keypool.empty() ? &setExternalKeyPool : &set_pre_split_keypool); auto it = setKeyPool->begin(); + std::vector result; WalletBatch batch(m_storage.GetDatabase()); while (it != std::end(*setKeyPool)) { const int64_t& index = *(it); @@ -1448,7 +1458,10 @@ void LegacyScriptPubKeyMan::MarkReserveKeysAsUsed(int64_t keypool_id) batch.ErasePool(index); WalletLogPrintf("keypool index %d removed\n", index); it = setKeyPool->erase(it); + result.push_back(std::move(keypool)); } + + return result; } std::vector GetAffectedKeys(const CScript& spk, const SigningProvider& provider) @@ -1820,19 +1833,32 @@ bool DescriptorScriptPubKeyMan::TopUp(unsigned int size) return true; } -void DescriptorScriptPubKeyMan::MarkUnusedAddresses(const CScript& script) +std::vector DescriptorScriptPubKeyMan::MarkUnusedAddresses(const CScript& script) { LOCK(cs_desc_man); + std::vector result; if (IsMine(script)) { int32_t index = m_map_script_pub_keys[script]; if (index >= m_wallet_descriptor.next_index) { WalletLogPrintf("%s: Detected a used keypool item at index %d, mark all keypool items up to this item as used\n", __func__, index); - m_wallet_descriptor.next_index = index + 1; + auto out_keys = std::make_unique(); + std::vector scripts_temp; + while (index >= m_wallet_descriptor.next_index) { + if (!m_wallet_descriptor.descriptor->ExpandFromCache(m_wallet_descriptor.next_index, m_wallet_descriptor.cache, scripts_temp, *out_keys)) { + throw std::runtime_error(std::string(__func__) + ": Unable to expand descriptor from cache"); + } + CTxDestination dest; + ExtractDestination(scripts_temp[0], dest); + result.push_back({dest, std::nullopt}); + m_wallet_descriptor.next_index++; + } } if (!TopUp()) { WalletLogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__); } } + + return result; } void DescriptorScriptPubKeyMan::AddDescriptorKey(const CKey& key, const CPubKey &pubkey) diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index ef74638751..10def20d79 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -148,6 +148,12 @@ public: } }; +struct WalletDestination +{ + CTxDestination dest; + std::optional internal; +}; + /* * A class implementing ScriptPubKeyMan manages some (or all) scriptPubKeys used in a wallet. * It contains the scripts and keys related to the scriptPubKeys it manages. @@ -180,8 +186,14 @@ public: */ virtual bool TopUp(unsigned int size = 0) { return false; } - //! Mark unused addresses as being used - virtual void MarkUnusedAddresses(const CScript& script) {} + /** Mark unused addresses as being used + * Affects all keys up to and including the one determined by provided script. + * + * @param script determines the last key to mark as used + * + * @return All of the addresses affected + */ + virtual std::vector MarkUnusedAddresses(const CScript& script) { return {}; } /** Sets up the key generation stuff, i.e. generates new HD seeds and sets them as active. * Returns false if already setup or setup fails, true if setup is successful @@ -356,7 +368,7 @@ public: bool TopUp(unsigned int size = 0) override; - void MarkUnusedAddresses(const CScript& script) override; + std::vector MarkUnusedAddresses(const CScript& script) override; //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo void UpgradeKeyMetadata(); @@ -481,9 +493,13 @@ public: void LearnAllRelatedScripts(const CPubKey& key); /** - * Marks all keys in the keypool up to and including reserve_key as used. + * Marks all keys in the keypool up to and including the provided key as used. + * + * @param keypool_id determines the last key to mark as used + * + * @return All affected keys */ - void MarkReserveKeysAsUsed(int64_t keypool_id) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore); + std::vector MarkReserveKeysAsUsed(int64_t keypool_id) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore); const std::map& GetAllReserveKeys() const { return m_pool_key_to_index; } std::set GetKeys() const override; @@ -563,7 +579,7 @@ public: // (with or without private keys), the "keypool" is a single xpub. bool TopUp(unsigned int size = 0) override; - void MarkUnusedAddresses(const CScript& script) override; + std::vector MarkUnusedAddresses(const CScript& script) override; bool IsHDEnabled() const override; -- cgit v1.2.3 From 9f3a622b1cea37e452560f2f82d8e82d3b48a73a Mon Sep 17 00:00:00 2001 From: S3RK <1466284+S3RK@users.noreply.github.com> Date: Thu, 9 Sep 2021 08:31:47 +0200 Subject: Automatically add labels to detected receiving addresses --- src/wallet/wallet.cpp | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c67a8f77da..0418cf4c9b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1062,8 +1062,23 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Co // loop though all outputs for (const CTxOut& txout: tx.vout) { - for (const auto& spk_man_pair : m_spk_managers) { - spk_man_pair.second->MarkUnusedAddresses(txout.scriptPubKey); + for (const auto& spk_man : GetScriptPubKeyMans(txout.scriptPubKey)) { + for (auto &dest : spk_man->MarkUnusedAddresses(txout.scriptPubKey)) { + // If internal flag is not defined try to infer it from the ScriptPubKeyMan + if (!dest.internal.has_value()) { + dest.internal = IsInternalScriptPubKeyMan(spk_man); + } + + // skip if can't determine whether it's a receiving address or not + if (!dest.internal.has_value()) continue; + + // If this is a receiving address and it's not in the address book yet + // (e.g. it wasn't generated on this node or we're restoring from backup) + // add it to the address book for proper transaction accounting + if (!*dest.internal && !FindAddressBookEntry(dest.dest, /* allow_change= */ false)) { + SetAddressBook(dest.dest, "", "receive"); + } + } } } -- cgit v1.2.3 From d04566415e16ae685af066384f346dff522c068f Mon Sep 17 00:00:00 2001 From: S3RK <1466284+S3RK@users.noreply.github.com> Date: Thu, 9 Sep 2021 08:32:13 +0200 Subject: Add to spends only transcations from me --- src/wallet/test/wallet_tests.cpp | 25 +++++++++++++++---------- src/wallet/wallet.cpp | 4 +++- 2 files changed, 18 insertions(+), 11 deletions(-) (limited to 'src') diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 5431a38bee..29d0ad7d4c 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -800,30 +800,35 @@ BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) context.args = &gArgs; context.chain = m_node.chain.get(); auto wallet = TestLoadWallet(context); - CKey key; - key.MakeNewKey(true); - AddKey(*wallet, key); + AddKey(*wallet, coinbaseKey); - std::string error; + // rescan to ensure coinbase transactions from test fixture are picked up by the wallet + { + WalletRescanReserver reserver(*wallet); + reserver.reserve(); + wallet->ScanForWalletTransactions(m_node.chain->getBlockHash(0), 0, /* max height= */ {}, reserver, /* update= */ true); + } + // create one more block to get the first block coinbase to maturity m_coinbase_txns.push_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); - auto block_tx = TestSimpleSpend(*m_coinbase_txns[0], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); - CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + // spend first coinbase tx + auto spend_tx = TestSimpleSpend(*m_coinbase_txns[0], 0, coinbaseKey, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + CreateAndProcessBlock({spend_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); SyncWithValidationInterfaceQueue(); { - auto block_hash = block_tx.GetHash(); + auto spend_tx_hash = spend_tx.GetHash(); auto prev_hash = m_coinbase_txns[0]->GetHash(); LOCK(wallet->cs_wallet); BOOST_CHECK(wallet->HasWalletSpend(prev_hash)); - BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 1u); + BOOST_CHECK_EQUAL(wallet->mapWallet.count(spend_tx_hash), 1u); - std::vector vHashIn{ block_hash }, vHashOut; + std::vector vHashIn{spend_tx_hash}, vHashOut; BOOST_CHECK_EQUAL(wallet->ZapSelectTx(vHashIn, vHashOut), DBErrors::LOAD_OK); BOOST_CHECK(!wallet->HasWalletSpend(prev_hash)); - BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 0u); + BOOST_CHECK_EQUAL(wallet->mapWallet.count(spend_tx_hash), 0u); } TestUnloadWallet(std::move(wallet)); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0418cf4c9b..339af292e7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -915,7 +915,9 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmatio wtx.nOrderPos = IncOrderPosNext(&batch); wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx)); wtx.nTimeSmart = ComputeTimeSmart(wtx, rescanning_old_block); - AddToSpends(hash, &batch); + if (IsFromMe(*tx.get())) { + AddToSpends(hash); + } } if (!fInsertedNew) -- cgit v1.2.3