From ea1f304a9a5588d59e12176dbf2c5ad0becd5a37 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 14 Apr 2023 15:05:04 -0400 Subject: [PATCH 1/8] wallet: implement IsKeyActive() in scriptpubkeyman This new method returns true if the given CScript key is derived from the SPKM. For Legacy that means checking the hd_seed_id in the key's metadata. Also patches PKDescriptor to return associated public keys in MakeScripts() --- src/script/descriptor.cpp | 5 ++++- src/wallet/scriptpubkeyman.cpp | 17 +++++++++++++++++ src/wallet/scriptpubkeyman.h | 7 +++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index 0951504ee02..04315f73270 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -730,8 +730,11 @@ class PKDescriptor final : public DescriptorImpl private: const bool m_xonly; protected: - std::vector MakeScripts(const std::vector& keys, Span, FlatSigningProvider&) const override + std::vector MakeScripts(const std::vector& keys, Span, FlatSigningProvider& out) const override { + CKeyID id = keys[0].GetID(); + out.pubkeys.emplace(id, keys[0]); + if (m_xonly) { CScript script = CScript() << ToByteVector(XOnlyPubKey(keys[0])) << OP_CHECKSIG; return Vector(std::move(script)); diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 62bd0c06cd0..b8b304c9e63 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -392,6 +392,23 @@ std::vector LegacyScriptPubKeyMan::MarkUnusedAddresses(const return result; } +bool LegacyScriptPubKeyMan::IsKeyActive(const CScript& script) const +{ + LOCK(cs_KeyStore); + + if (!IsMine(script)) return false; // Not in the keystore at all + + for (const auto& key_id : GetAffectedKeys(script, *this)) { + const auto it = mapKeyMetadata.find(key_id); + if (it == mapKeyMetadata.end()) return false; // This key must be really old + + if (!it->second.hd_seed_id.IsNull() && it->second.hd_seed_id == m_hd_chain.seed_id) return true; + } + + // Imported or dumped for a new keypool + return false; +} + void LegacyScriptPubKeyMan::UpgradeKeyMetadata() { LOCK(cs_KeyStore); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 22b67c88e95..dd0465a68b3 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -198,6 +198,9 @@ class ScriptPubKeyMan */ virtual std::vector MarkUnusedAddresses(const CScript& script) { return {}; } + /* Determines if address is derived from active key manager */ + [[nodiscard]] virtual bool IsKeyActive(const CScript& script) const { return false; }; + /** 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 * Set force=true to make it re-setup if already setup, used for upgrades @@ -382,6 +385,8 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv std::vector MarkUnusedAddresses(const CScript& script) override; + [[nodiscard]] bool IsKeyActive(const CScript& script) const override; + //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo void UpgradeKeyMetadata(); @@ -607,6 +612,8 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan std::vector MarkUnusedAddresses(const CScript& script) override; + [[nodiscard]] bool IsKeyActive(const CScript& script) const override { return IsMine(script); } + bool IsHDEnabled() const override; //! Setup descriptors based on the given CExtkey From abad1e0613f33a6c213767628c56dc0e15115381 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 14 Apr 2023 15:06:43 -0400 Subject: [PATCH 2/8] wallet: implement IsDestinationActive() and add to rpc getaddressinfo This connects SPKM.IsKeyActive() up to the wallet level. --- src/wallet/rpc/addresses.cpp | 2 ++ src/wallet/wallet.cpp | 7 +++++++ src/wallet/wallet.h | 5 +++++ 3 files changed, 14 insertions(+) diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index a5be0739a9b..15035395609 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -502,6 +502,7 @@ RPCHelpMan getaddressinfo() {RPCResult::Type::STR, "address", "The bitcoin address validated."}, {RPCResult::Type::STR_HEX, "scriptPubKey", "The hex-encoded scriptPubKey generated by the address."}, {RPCResult::Type::BOOL, "ismine", "If the address is yours."}, + {RPCResult::Type::BOOL, "isactive", "If the key is in the active keypool (always equal to \"ismine\" in descriptor wallets)."}, {RPCResult::Type::BOOL, "iswatchonly", "If the address is watchonly."}, {RPCResult::Type::BOOL, "solvable", "If we know how to spend coins sent to this address, ignoring the possible lack of private keys."}, {RPCResult::Type::STR, "desc", /*optional=*/true, "A descriptor for spending coins sent to this address (only when solvable)."}, @@ -572,6 +573,7 @@ RPCHelpMan getaddressinfo() isminetype mine = pwallet->IsMine(dest); ret.pushKV("ismine", bool(mine & ISMINE_SPENDABLE)); + ret.pushKV("isactive", pwallet->IsDestinationActive(dest)); if (provider) { auto inferred = InferDescriptor(scriptPubKey, *provider); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index caf95a3f03f..da20c8d1219 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2523,6 +2523,13 @@ void CWallet::ForEachAddrBookEntry(const ListAddrBookFunc& func) const } } +bool CWallet::IsDestinationActive(const CTxDestination& dest) const +{ + const CScript& script{GetScriptForDestination(dest)}; + const std::set& spkms{GetActiveScriptPubKeyMans()}; + return std::any_of(spkms.cbegin(), spkms.cend(), [&script](const auto& spkm) { return spkm->IsKeyActive(script); }); +} + std::vector CWallet::ListAddrBookAddresses(const std::optional& _filter) const { AssertLockHeld(cs_wallet); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 5252b46cdc8..0b87d824f9e 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -715,6 +715,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati using ListAddrBookFunc = std::function purpose)>; void ForEachAddrBookEntry(const ListAddrBookFunc& func) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** + * Determines if a destination is in the active spkm (not imported and not dumped for a new keypool) + */ + [[nodiscard]] bool IsDestinationActive(const CTxDestination& dest) const; + /** * Marks all outputs in each one of the destinations dirty, so their cache is * reset and does not return outdated information. From c6d256562e4cc843220aa023028f9f3553ddf56b Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 14 Apr 2023 15:05:51 -0400 Subject: [PATCH 3/8] test: cover ScriptPubKeyMan::IsKeyActive() --- src/wallet/test/scriptpubkeyman_tests.cpp | 154 ++++++++++++++++++++++ 1 file changed, 154 insertions(+) diff --git a/src/wallet/test/scriptpubkeyman_tests.cpp b/src/wallet/test/scriptpubkeyman_tests.cpp index 90042f52526..f8c8dc2be57 100644 --- a/src/wallet/test/scriptpubkeyman_tests.cpp +++ b/src/wallet/test/scriptpubkeyman_tests.cpp @@ -39,5 +39,159 @@ BOOST_AUTO_TEST_CASE(CanProvide) BOOST_CHECK(keyman.CanProvide(p2sh_script, data)); } +BOOST_AUTO_TEST_CASE(Legacy_IsKeyActive) +{ + CWallet wallet(m_node.chain.get(), "", CreateMockWalletDatabase()); + { + LOCK(wallet.cs_wallet); + wallet.SetMinVersion(FEATURE_LATEST); + wallet.m_keypool_size = 10; + } + LegacyScriptPubKeyMan& spkm = *wallet.GetOrCreateLegacyScriptPubKeyMan(); + + // Start off empty + BOOST_CHECK(spkm.GetScriptPubKeys().empty()); + + // Generate 20 keypool keys (10 internal, 10 external) + { + LOCK(wallet.cs_wallet); + spkm.SetupGeneration(); + } + + // 4 scripts per keypool key (P2PK, P2PKH, P2WPKH, P2SH-P2WPKH) + // Plus 4 scripts for the seed key + auto scripts1 = spkm.GetScriptPubKeys(); + BOOST_CHECK_EQUAL(scripts1.size(), 84); + + // All keys are active + for (const CScript& script : scripts1) { + BOOST_CHECK(spkm.IsKeyActive(script)); + } + + // Requesting single from spkm should not deactivate key + CTxDestination dest1; + { + LOCK(wallet.cs_wallet); + auto result = spkm.GetNewDestination(OutputType::BECH32); + dest1 = result.value(); + } + CScript script = GetScriptForDestination(dest1); + BOOST_CHECK(spkm.IsKeyActive(script)); + + // Key pool size did not change + auto scripts2 = spkm.GetScriptPubKeys(); + BOOST_CHECK_EQUAL(scripts2.size(), 84); + + // Use key that is not the next key + // (i.e. address gap in wallet recovery) + { + LOCK(wallet.cs_wallet); + LOCK(spkm.cs_KeyStore); + auto keys = spkm.MarkReserveKeysAsUsed(5); + BOOST_CHECK_EQUAL(keys.size(), 4); // Because we already used one with GetNewDestination + } + + // Key pool size did not change + auto scripts3 = spkm.GetScriptPubKeys(); + BOOST_CHECK_EQUAL(scripts3.size(), 84); + + // All keys are still active + for (const CScript& script : scripts3) { + BOOST_CHECK(spkm.IsKeyActive(script)); + } + + // When user encrypts wallet for the first time, + // all existing keys are removed from active keypool + { + LOCK(wallet.cs_wallet); + // called by EncryptWallet() + spkm.SetupGeneration(true); + } + + // 20 new keys were added + auto scripts4 = spkm.GetScriptPubKeys(); + BOOST_CHECK_EQUAL(scripts4.size(), 84 * 2); + + // All 10 original keys are now inactive + for (const CScript& script : scripts3) { + BOOST_CHECK(!spkm.IsKeyActive(script)); + } +} + +BOOST_AUTO_TEST_CASE(Descriptor_IsKeyActive) +{ + CWallet wallet(m_node.chain.get(), "", CreateMockWalletDatabase()); + { + LOCK(wallet.cs_wallet); + wallet.LoadMinVersion(FEATURE_LATEST); + wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); + wallet.m_keypool_size = 10; + wallet.SetupDescriptorScriptPubKeyMans(); + } + DescriptorScriptPubKeyMan* spkm = dynamic_cast(wallet.GetScriptPubKeyMan(OutputType::BECH32, /*internal=*/false)); + + // Start off with 10 pre-generated keys, 1 script each + auto scripts1 = spkm->GetScriptPubKeys(); + BOOST_CHECK_EQUAL(scripts1.size(), 10); + + // All keys are active + for (const CScript& script : scripts1) { + BOOST_CHECK(spkm->IsKeyActive(script)); + } + + // Requesting single key from spkm should not deactivate key + auto dest1 = spkm->GetNewDestination(OutputType::BECH32); + CScript script = GetScriptForDestination(dest1.value()); + BOOST_CHECK(spkm->IsKeyActive(script)); + + // Key pool size did not change + auto scripts2 = spkm->GetScriptPubKeys(); + BOOST_CHECK_EQUAL(scripts2.size(), 10); + + // Use key that is not the next key + // (i.e. address gap in wallet recovery) + { + LOCK(spkm->cs_desc_man); + WalletDescriptor descriptor = spkm->GetWalletDescriptor(); + FlatSigningProvider provider; + std::vector scripts3; + descriptor.descriptor->ExpandFromCache(/*pos=*/5, descriptor.cache, scripts3, provider); + + BOOST_CHECK_EQUAL(scripts3.size(), 1); + spkm->MarkUnusedAddresses(scripts3.front()); + } + + // Key pool size increased to replace used keys + auto scripts4 = spkm->GetScriptPubKeys(); + BOOST_CHECK_EQUAL(scripts4.size(), 16); + + // All keys are still active + for (const CScript& script : scripts4) { + BOOST_CHECK(spkm->IsKeyActive(script)); + } + + // When user encrypts wallet for the first time, + // all existing keys are removed from active keypool + { + LOCK(wallet.cs_wallet); + // called by EncryptWallet() + wallet.SetupDescriptorScriptPubKeyMans(); + } + + // This SPKM is not affected + for (const CScript& script : scripts4) { + BOOST_CHECK(spkm->IsKeyActive(script)); + } + + // ...but at the wallet level all the keys from that SPKM are deactivated + int num_script_keys_not_found = 0; + for (const CScript& script : scripts4) { + if (!wallet.IsDestinationActive(WitnessV0ScriptHash(script))) { + ++num_script_keys_not_found; + } + } + BOOST_CHECK_EQUAL(num_script_keys_not_found, 16); +} + BOOST_AUTO_TEST_SUITE_END() } // namespace wallet From 1b2e020295c5cc796cbb6e1aec3d9aa13fd799f0 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 14 Apr 2023 15:08:03 -0400 Subject: [PATCH 4/8] test: cover "isactive" field in rpc getaddressinfo --- test/functional/wallet_keypool.py | 121 ++++++++++++++++++++++++------ 1 file changed, 97 insertions(+), 24 deletions(-) diff --git a/test/functional/wallet_keypool.py b/test/functional/wallet_keypool.py index bd978511536..33812affbcd 100755 --- a/test/functional/wallet_keypool.py +++ b/test/functional/wallet_keypool.py @@ -4,11 +4,17 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test the wallet keypool and interaction with wallet encryption/locking.""" +import os +import re import time from decimal import Decimal from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal, assert_raises_rpc_error +from test_framework.descriptors import descsum_create + +TEST_KEYPOOL_SIZE = 10 +TEST_NEW_KEYPOOL_SIZE = TEST_KEYPOOL_SIZE + 2 class KeyPoolTest(BitcoinTestFramework): def add_options(self, parser): @@ -16,20 +22,89 @@ def add_options(self, parser): def set_test_params(self): self.num_nodes = 1 + self.extra_args = [[f"-keypool={TEST_KEYPOOL_SIZE}"]] def skip_test_if_missing_module(self): self.skip_if_no_wallet() def run_test(self): nodes = self.nodes + + # Derive addresses from the wallet without removing them from keypool + addrs = [] + if not self.options.descriptors: + path = os.path.join(self.nodes[0].datadir, 'wallet.dump') + nodes[0].dumpwallet(path) + file = open(path, "r", encoding="utf8") + m = re.search(r"masterkey: (\w+)", file.read()) + file.close() + xpriv = m.group(1) + desc = descsum_create(f"wpkh({xpriv}/0h/0h/*h)") + addrs = nodes[0].deriveaddresses(descriptor=desc, range=[0, 9]) + else: + list_descriptors = nodes[0].listdescriptors() + for desc in list_descriptors["descriptors"]: + if desc['active'] and not desc["internal"] and desc["desc"][:4] == "wpkh": + addrs = nodes[0].deriveaddresses(descriptor=desc["desc"], range=[0, 9]) + + addr0 = addrs[0] + addr9 = addrs[9] # arbitrary future address index + + # Address is active before it is removed from keypool by getnewaddress + addr0_before_getting_data = nodes[0].getaddressinfo(addr0) + assert addr0_before_getting_data['isactive'] + addr_before_encrypting = nodes[0].getnewaddress() addr_before_encrypting_data = nodes[0].getaddressinfo(addr_before_encrypting) + assert addr0 == addr_before_encrypting + # Address is still active even after being removed from keypool + assert addr_before_encrypting_data['isactive'] + wallet_info_old = nodes[0].getwalletinfo() if not self.options.descriptors: assert addr_before_encrypting_data['hdseedid'] == wallet_info_old['hdseedid'] + # Address is active before wallet is encrypted (resetting keypool) + addr9_before_encrypting_data = nodes[0].getaddressinfo(addr9) + assert addr9_before_encrypting_data['isactive'] + + # Imported things are never considered active, no need to rescan + if self.options.descriptors: + nodes[0].importdescriptors([{ + "desc": "addr(bcrt1q95gp4zeaah3qcerh35yhw02qeptlzasdtst55v)", + "timestamp": "now" + }]) + else: + nodes[0].importaddress("bcrt1q95gp4zeaah3qcerh35yhw02qeptlzasdtst55v", "label", rescan=False) + import_addr_data = nodes[0].getaddressinfo("bcrt1q95gp4zeaah3qcerh35yhw02qeptlzasdtst55v") + assert not import_addr_data["ismine"] + assert import_addr_data["iswatchonly"] is not self.options.descriptors + assert not import_addr_data["isactive"] + + if self.options.descriptors: + nodes[0].importdescriptors([{ + "desc": "pk(02f893ca95b0d55b4ce4e72ae94982eb679158cb2ebc120ff62c17fedfd1f0700e)", + "timestamp": "now" + }]) + else: + nodes[0].importpubkey("02f893ca95b0d55b4ce4e72ae94982eb679158cb2ebc120ff62c17fedfd1f0700e", "label", rescan=False) + import_pub_data = nodes[0].getaddressinfo("bcrt1q4v7a8wn5vqd6fk4026s5gzzxyu7cfzz23n576h") + assert not import_pub_data["ismine"] + assert import_pub_data["iswatchonly"] is not self.options.descriptors + assert not import_pub_data["isactive"] + + nodes[0].importprivkey("cPMX7v5CNV1zCphFSq2hnR5rCjzAhA1GsBfD1qrJGdj4QEfu38Qx", "label", rescan=False) + import_priv_data = nodes[0].getaddressinfo("bcrt1qa985v5d53qqtrfujmzq2zrw3r40j6zz4ns02kj") + assert import_priv_data["ismine"] + assert not import_priv_data["iswatchonly"] + assert not import_priv_data["isactive"] + # Encrypt wallet and wait to terminate nodes[0].encryptwallet('test') + addr9_after_encrypting_data = nodes[0].getaddressinfo(addr9) + # Key is from unencrypted seed, no longer considered active + assert not addr9_after_encrypting_data['isactive'] + if self.options.descriptors: # Import hardened derivation only descriptors nodes[0].walletpassphrase('test', 10) @@ -75,7 +150,9 @@ def run_test(self): } ]) nodes[0].walletlock() - # Keep creating keys + # Keep creating keys until we run out + for _ in range(TEST_KEYPOOL_SIZE - 1): + nodes[0].getnewaddress() addr = nodes[0].getnewaddress() addr_data = nodes[0].getaddressinfo(addr) wallet_info = nodes[0].getwalletinfo() @@ -84,50 +161,46 @@ def run_test(self): assert addr_data['hdseedid'] == wallet_info['hdseedid'] assert_raises_rpc_error(-12, "Error: Keypool ran out, please call keypoolrefill first", nodes[0].getnewaddress) - # put six (plus 2) new keys in the keypool (100% external-, +100% internal-keys, 1 in min) + # put two new keys in the keypool nodes[0].walletpassphrase('test', 12000) - nodes[0].keypoolrefill(6) + nodes[0].keypoolrefill(TEST_NEW_KEYPOOL_SIZE) nodes[0].walletlock() wi = nodes[0].getwalletinfo() if self.options.descriptors: - assert_equal(wi['keypoolsize_hd_internal'], 24) - assert_equal(wi['keypoolsize'], 24) + # Descriptors wallet: keypool size applies to both internal and external + # chains and there are four of each (legacy, nested, segwit, and taproot) + assert_equal(wi['keypoolsize_hd_internal'], TEST_NEW_KEYPOOL_SIZE * 4) + assert_equal(wi['keypoolsize'], TEST_NEW_KEYPOOL_SIZE * 4) else: - assert_equal(wi['keypoolsize_hd_internal'], 6) - assert_equal(wi['keypoolsize'], 6) + # Legacy wallet: keypool size applies to both internal and external HD chains + assert_equal(wi['keypoolsize_hd_internal'], TEST_NEW_KEYPOOL_SIZE) + assert_equal(wi['keypoolsize'], TEST_NEW_KEYPOOL_SIZE) # drain the internal keys - nodes[0].getrawchangeaddress() - nodes[0].getrawchangeaddress() - nodes[0].getrawchangeaddress() - nodes[0].getrawchangeaddress() - nodes[0].getrawchangeaddress() - nodes[0].getrawchangeaddress() - addr = set() + for _ in range(TEST_NEW_KEYPOOL_SIZE): + nodes[0].getrawchangeaddress() # the next one should fail assert_raises_rpc_error(-12, "Keypool ran out", nodes[0].getrawchangeaddress) # drain the external keys - addr.add(nodes[0].getnewaddress(address_type="bech32")) - addr.add(nodes[0].getnewaddress(address_type="bech32")) - addr.add(nodes[0].getnewaddress(address_type="bech32")) - addr.add(nodes[0].getnewaddress(address_type="bech32")) - addr.add(nodes[0].getnewaddress(address_type="bech32")) - addr.add(nodes[0].getnewaddress(address_type="bech32")) - assert len(addr) == 6 + addr = set() + for _ in range(TEST_NEW_KEYPOOL_SIZE): + addr.add(nodes[0].getnewaddress(address_type="bech32")) # the next one should fail assert_raises_rpc_error(-12, "Error: Keypool ran out, please call keypoolrefill first", nodes[0].getnewaddress) - # refill keypool with three new addresses + # refill keypool nodes[0].walletpassphrase('test', 1) - nodes[0].keypoolrefill(3) + # At this point the keypool has >45 keys in it + # calling keypoolrefill with anything smaller than that is a noop + nodes[0].keypoolrefill(50) # test walletpassphrase timeout time.sleep(1.1) assert_equal(nodes[0].getwalletinfo()["unlocked_until"], 0) # drain the keypool - for _ in range(3): + for _ in range(50): nodes[0].getnewaddress() assert_raises_rpc_error(-12, "Keypool ran out", nodes[0].getnewaddress) From 712e366b6198eb77b713989188ff7f1610ffe627 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 14 Apr 2023 15:11:37 -0400 Subject: [PATCH 5/8] doc: release notes for #27216 --- doc/release-notes/release-notes-27216.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 doc/release-notes/release-notes-27216.md diff --git a/doc/release-notes/release-notes-27216.md b/doc/release-notes/release-notes-27216.md new file mode 100644 index 00000000000..9250cf1711d --- /dev/null +++ b/doc/release-notes/release-notes-27216.md @@ -0,0 +1,8 @@ +Updated RPCs +------------ + +- RPC `getaddressinfo` now returns a new boolean field `"isactive"`, which is +only `true` if the address is derived from an active descriptor or HD seed. +This information can inform a user to avoid reusing certain addresses that may +have been imported from external sources or generated by their wallet before it +was encrypted. (#27216) From 979f8c2294e636a68c9147acc8981fb2bfdfe215 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 24 Mar 2023 10:08:52 -0400 Subject: [PATCH 6/8] qt: add warnings to receive request table and dialog Tool tip and dialog field can be expanded in the future. For now the first warning we show is if the address was derived from a non-active seed or descriptor. See https://github.com/bitcoin/bitcoin/issues/3314 --- src/interfaces/wallet.h | 16 +++++++-- src/qt/forms/receiverequestdialog.ui | 34 +++++++++++++++++- src/qt/receivecoinsdialog.cpp | 13 ++++--- src/qt/receiverequestdialog.cpp | 12 +++++-- src/qt/receiverequestdialog.h | 4 +-- src/qt/recentrequeststablemodel.cpp | 54 ++++++++++++++++++++++------ src/qt/recentrequeststablemodel.h | 24 +++++++++---- src/qt/test/wallettests.cpp | 8 +++-- src/qt/walletmodel.cpp | 2 +- src/wallet/interfaces.cpp | 3 +- src/wallet/test/wallet_tests.cpp | 20 +++++++---- src/wallet/wallet.cpp | 7 ++-- src/wallet/wallet.h | 2 +- 13 files changed, 155 insertions(+), 44 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 8c31112fc94..93040d0b8e6 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -46,6 +46,7 @@ namespace interfaces { class Handler; struct WalletAddress; +struct ReceiveRequest; struct WalletBalances; struct WalletTx; struct WalletTxOut; @@ -118,8 +119,8 @@ class Wallet //! Get wallet address list. virtual std::vector getAddresses() const = 0; - //! Get receive requests. - virtual std::vector getAddressReceiveRequests() = 0; + //! Get receive requests. Bool indicating key is active, string containing serialized destination data + virtual std::vector getAddressReceiveRequests() = 0; //! Save or remove receive request. virtual bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0; @@ -362,6 +363,17 @@ struct WalletAddress } }; +//! Information about one receive request. +struct ReceiveRequest { + std::string m_data; + bool m_is_active; + + ReceiveRequest(std::string data, bool is_active) + : m_data(std::move(data)), m_is_active(is_active) {} + + bool operator==(const ReceiveRequest& a) const { return (m_data == a.m_data) && (m_is_active == a.m_is_active); }; +}; + //! Collection of wallet balances. struct WalletBalances { diff --git a/src/qt/forms/receiverequestdialog.ui b/src/qt/forms/receiverequestdialog.ui index 70a7cf71de9..f1dbb4401b6 100644 --- a/src/qt/forms/receiverequestdialog.ui +++ b/src/qt/forms/receiverequestdialog.ui @@ -232,7 +232,39 @@ - + + + + + 75 + true + + + + Warnings: + + + Qt::NoTextInteraction + + + + + + + warnings + + + Qt::PlainText + + + true + + + Qt::TextSelectableByMouse + + + + diff --git a/src/qt/receivecoinsdialog.cpp b/src/qt/receivecoinsdialog.cpp index be7741e8a8c..e135748119c 100644 --- a/src/qt/receivecoinsdialog.cpp +++ b/src/qt/receivecoinsdialog.cpp @@ -160,14 +160,19 @@ void ReceiveCoinsDialog::on_receiveButton_clicked() // Success SendCoinsRecipient info(address, label, ui->reqAmount->value(), ui->reqMessage->text()); + + /* Store request for later reference */ + std::optional entry = model->getRecentRequestsTableModel()->addNewRequest(info); + + // Wallet failed to set address receive request + if (!entry.has_value()) break; + ReceiveRequestDialog *dialog = new ReceiveRequestDialog(this); dialog->setAttribute(Qt::WA_DeleteOnClose); dialog->setModel(model); - dialog->setInfo(info); + dialog->setInfo(entry.value()); dialog->show(); - /* Store request for later reference */ - model->getRecentRequestsTableModel()->addNewRequest(info); break; } case AddressTableModel::EditStatus::WALLET_UNLOCK_FAILURE: @@ -194,7 +199,7 @@ void ReceiveCoinsDialog::on_recentRequestsView_doubleClicked(const QModelIndex & const RecentRequestsTableModel *submodel = model->getRecentRequestsTableModel(); ReceiveRequestDialog *dialog = new ReceiveRequestDialog(this); dialog->setModel(model); - dialog->setInfo(submodel->entry(index.row()).recipient); + dialog->setInfo(submodel->entry(index.row())); dialog->setAttribute(Qt::WA_DeleteOnClose); dialog->show(); } diff --git a/src/qt/receiverequestdialog.cpp b/src/qt/receiverequestdialog.cpp index 3453857f985..dfe06237bd5 100644 --- a/src/qt/receiverequestdialog.cpp +++ b/src/qt/receiverequestdialog.cpp @@ -42,9 +42,9 @@ void ReceiveRequestDialog::setModel(WalletModel *_model) update(); } -void ReceiveRequestDialog::setInfo(const SendCoinsRecipient &_info) +void ReceiveRequestDialog::setInfo(const RecentRequestEntry& entry) { - this->info = _info; + this->info = entry.recipient; setWindowTitle(tr("Request payment to %1").arg(info.label.isEmpty() ? info.address : info.label)); QString uri = GUIUtil::formatBitcoinURI(info); @@ -89,6 +89,14 @@ void ReceiveRequestDialog::setInfo(const SendCoinsRecipient &_info) ui->wallet_content->hide(); } + QString warnings{entry.GetAddressWarnings()}; + if (warnings.isEmpty()) { + ui->warnings_tag->hide(); + ui->warnings_content->hide(); + } else { + ui->warnings_content->setText(warnings); + } + ui->btnVerify->setVisible(model->wallet().hasExternalSigner()); connect(ui->btnVerify, &QPushButton::clicked, [this] { diff --git a/src/qt/receiverequestdialog.h b/src/qt/receiverequestdialog.h index d64fee36636..6a64209e70f 100644 --- a/src/qt/receiverequestdialog.h +++ b/src/qt/receiverequestdialog.h @@ -5,7 +5,7 @@ #ifndef BITCOIN_QT_RECEIVEREQUESTDIALOG_H #define BITCOIN_QT_RECEIVEREQUESTDIALOG_H -#include +#include #include @@ -24,7 +24,7 @@ class ReceiveRequestDialog : public QDialog ~ReceiveRequestDialog(); void setModel(WalletModel *model); - void setInfo(const SendCoinsRecipient &info); + void setInfo(const RecentRequestEntry& entry); private Q_SLOTS: void on_btnCopyURI_clicked(); diff --git a/src/qt/recentrequeststablemodel.cpp b/src/qt/recentrequeststablemodel.cpp index 52d4e45d49f..6ba53cb8668 100644 --- a/src/qt/recentrequeststablemodel.cpp +++ b/src/qt/recentrequeststablemodel.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -17,19 +18,29 @@ #include +#include #include #include -RecentRequestsTableModel::RecentRequestsTableModel(WalletModel *parent) : - QAbstractTableModel(parent), walletModel(parent) +[[nodiscard]] QString RecentRequestEntry::GetAddressWarnings() const +{ + QString warnings; + if (!m_is_active) { + warnings += QObject::tr("This address should not be used. It was derived from an inactive seed, was imported, or may have been stored unencrypted.\n"); + } + + return warnings; +} + +RecentRequestsTableModel::RecentRequestsTableModel(const PlatformStyle* platformStyle, WalletModel* parent) : QAbstractTableModel(parent), walletModel(parent) { // Load entries from wallet - for (const std::string& request : parent->wallet().getAddressReceiveRequests()) { + for (const interfaces::ReceiveRequest& request : parent->wallet().getAddressReceiveRequests()) { addNewRequest(request); } /* These columns must match the indices in the ColumnIndex enumeration */ - columns << tr("Date") << tr("Label") << tr("Message") << getAmountTitle(); + columns << tr("Date") << tr("Warnings") << tr("Label") << tr("Message") << getAmountTitle(); connect(walletModel->getOptionsModel(), &OptionsModel::displayUnitChanged, this, &RecentRequestsTableModel::updateDisplayUnit); } @@ -57,11 +68,13 @@ QVariant RecentRequestsTableModel::data(const QModelIndex &index, int role) cons if(!index.isValid() || index.row() >= list.length()) return QVariant(); + const RecentRequestEntry* rec = &list[index.row()]; if(role == Qt::DisplayRole || role == Qt::EditRole) { - const RecentRequestEntry *rec = &list[index.row()]; switch(index.column()) { + case Warnings: + return {}; case Date: return GUIUtil::dateTimeStr(rec->date); case Label: @@ -95,6 +108,16 @@ QVariant RecentRequestsTableModel::data(const QModelIndex &index, int role) cons { if (index.column() == Amount) return (int)(Qt::AlignRight|Qt::AlignVCenter); + } else if (role == Qt::DecorationRole) { + if (index.column() == Warnings) { + if (rec->GetAddressWarnings().isEmpty()) { + return {}; + } else { + return platformStyle->TextColorIcon(QIcon(":/icons/warning")); + } + } + } else if (role == Qt::ToolTipRole) { + if (index.column() == Warnings) return rec->GetAddressWarnings(); } return QVariant(); } @@ -168,7 +191,7 @@ Qt::ItemFlags RecentRequestsTableModel::flags(const QModelIndex &index) const } // called when adding a request from the GUI -void RecentRequestsTableModel::addNewRequest(const SendCoinsRecipient &recipient) +std::optional RecentRequestsTableModel::addNewRequest(const SendCoinsRecipient& recipient) { RecentRequestEntry newEntry; newEntry.id = ++nReceiveRequestsMaxId; @@ -179,35 +202,44 @@ void RecentRequestsTableModel::addNewRequest(const SendCoinsRecipient &recipient ss << newEntry; if (!walletModel->wallet().setAddressReceiveRequest(DecodeDestination(recipient.address.toStdString()), ToString(newEntry.id), ss.str())) - return; + return std::nullopt; + + // If we are using the GUI to get a new receive address, + // the key must be active (derived from active seed or descriptor) + newEntry.m_is_active = true; addNewRequest(newEntry); + return newEntry; } // called from ctor when loading from wallet -void RecentRequestsTableModel::addNewRequest(const std::string &recipient) +std::optional RecentRequestsTableModel::addNewRequest(const interfaces::ReceiveRequest& recipient) { - std::vector data(recipient.begin(), recipient.end()); + std::vector data(recipient.m_data.begin(), recipient.m_data.end()); DataStream ss{data}; RecentRequestEntry entry; ss >> entry; if (entry.id == 0) // should not happen - return; + return std::nullopt; if (entry.id > nReceiveRequestsMaxId) nReceiveRequestsMaxId = entry.id; + entry.m_is_active = recipient.m_is_active; + addNewRequest(entry); + return entry; } // actually add to table in GUI -void RecentRequestsTableModel::addNewRequest(RecentRequestEntry &recipient) +std::optional RecentRequestsTableModel::addNewRequest(RecentRequestEntry& recipient) { beginInsertRows(QModelIndex(), 0, 0); list.prepend(recipient); endInsertRows(); + return recipient; } void RecentRequestsTableModel::sort(int column, Qt::SortOrder order) diff --git a/src/qt/recentrequeststablemodel.h b/src/qt/recentrequeststablemodel.h index 151f8322a8a..f0f57bf6aaa 100644 --- a/src/qt/recentrequeststablemodel.h +++ b/src/qt/recentrequeststablemodel.h @@ -5,8 +5,12 @@ #ifndef BITCOIN_QT_RECENTREQUESTSTABLEMODEL_H #define BITCOIN_QT_RECENTREQUESTSTABLEMODEL_H +#include + +#include #include +#include #include #include @@ -26,12 +30,16 @@ class RecentRequestEntry QDateTime date; SendCoinsRecipient recipient; + bool m_is_active{false}; // memory only + SERIALIZE_METHODS(RecentRequestEntry, obj) { unsigned int date_timet; SER_WRITE(obj, date_timet = obj.date.toSecsSinceEpoch()); READWRITE(obj.nVersion, obj.id, date_timet, obj.recipient); SER_READ(obj, obj.date = QDateTime::fromSecsSinceEpoch(date_timet)); } + + [[nodiscard]] QString GetAddressWarnings() const; }; class RecentRequestEntryLessThan @@ -54,14 +62,15 @@ class RecentRequestsTableModel: public QAbstractTableModel Q_OBJECT public: - explicit RecentRequestsTableModel(WalletModel *parent); + explicit RecentRequestsTableModel(const PlatformStyle* platformStyle, WalletModel* parent); ~RecentRequestsTableModel(); enum ColumnIndex { Date = 0, - Label = 1, - Message = 2, - Amount = 3, + Warnings, + Label, + Message, + Amount, NUMBER_OF_COLUMNS }; @@ -79,9 +88,9 @@ class RecentRequestsTableModel: public QAbstractTableModel /*@}*/ const RecentRequestEntry &entry(int row) const { return list[row]; } - void addNewRequest(const SendCoinsRecipient &recipient); - void addNewRequest(const std::string &recipient); - void addNewRequest(RecentRequestEntry &recipient); + std::optional addNewRequest(const SendCoinsRecipient& recipient); + std::optional addNewRequest(const interfaces::ReceiveRequest& recipient); + std::optional addNewRequest(RecentRequestEntry& recipient); public Q_SLOTS: void updateDisplayUnit(); @@ -91,6 +100,7 @@ public Q_SLOTS: QStringList columns; QList list; int64_t nReceiveRequestsMaxId{0}; + const PlatformStyle* platformStyle; /** Updates the column title to "Amount (DisplayUnit)" and emits headerDataChanged() signal for table headers to react. */ void updateAmountColumnTitle(); diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 072718fe151..5a718391d30 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -5,9 +5,9 @@ #include #include -#include #include #include +#include #include #include #include @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -366,10 +367,11 @@ void TestGUI(interfaces::Node& node, const std::shared_ptr& wallet) QCOMPARE(currentRowCount, initialRowCount+1); // Check addition to wallet - std::vector requests = walletModel.wallet().getAddressReceiveRequests(); + std::vector requests = walletModel.wallet().getAddressReceiveRequests(); QCOMPARE(requests.size(), size_t{1}); + QVERIFY(requests[0].m_is_active); // Key was derived from active seed or descriptor RecentRequestEntry entry; - DataStream{MakeUCharSpan(requests[0])} >> entry; + DataStream{MakeUCharSpan(requests[0].m_data)} >> entry; QCOMPARE(entry.nVersion, int{1}); QCOMPARE(entry.id, int64_t{1}); QVERIFY(entry.date.isValid()); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index ee3327530c4..baeda4d7402 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -51,7 +51,7 @@ WalletModel::WalletModel(std::unique_ptr wallet, ClientModel fHaveWatchOnly = m_wallet->haveWatchOnly(); addressTableModel = new AddressTableModel(this); transactionTableModel = new TransactionTableModel(platformStyle, this); - recentRequestsTableModel = new RecentRequestsTableModel(this); + recentRequestsTableModel = new RecentRequestsTableModel(platformStyle, this); subscribeToCoreSignals(); } diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index cd438cfe2f3..4d385e81a2b 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -223,7 +223,8 @@ class WalletImpl : public Wallet }); return result; } - std::vector getAddressReceiveRequests() override { + std::vector getAddressReceiveRequests() override + { LOCK(m_wallet->cs_wallet); return m_wallet->GetAddressReceiveRequests(); } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index da4b553394c..dc0c146da25 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -451,6 +451,13 @@ void TestLoadWallet(const std::string& name, DatabaseFormat format, std::functio WITH_LOCK(wallet->cs_wallet, f(wallet)); } +void CompareRecReqVectors(std::vector a, std::vector b) +{ + BOOST_REQUIRE(a.size() == b.size()); + for (size_t i = 0; i < a.size(); ++i) + BOOST_CHECK(a[i] == b[i]); +} + BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup) { for (DatabaseFormat format : DATABASE_FORMATS) { @@ -469,9 +476,10 @@ BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup) TestLoadWallet(name, format, [](std::shared_ptr wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) { BOOST_CHECK(wallet->IsAddressPreviouslySpent(PKHash())); BOOST_CHECK(wallet->IsAddressPreviouslySpent(ScriptHash())); - auto requests = wallet->GetAddressReceiveRequests(); - auto erequests = {"val_rr11", "val_rr20"}; - BOOST_CHECK_EQUAL_COLLECTIONS(requests.begin(), requests.end(), std::begin(erequests), std::end(erequests)); + std::vector requests = wallet->GetAddressReceiveRequests(); + // Key should be marked as not active (m_is_active = false) since it was effectively imported, not derived from active seed + std::vector erequests = {{"val_rr11", false}, {"val_rr20", false}}; + CompareRecReqVectors(requests, erequests); WalletBatch batch{wallet->GetDatabase()}; BOOST_CHECK(batch.WriteAddressPreviouslySpent(PKHash(), false)); BOOST_CHECK(batch.EraseAddressData(ScriptHash())); @@ -479,9 +487,9 @@ BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup) TestLoadWallet(name, format, [](std::shared_ptr wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) { BOOST_CHECK(!wallet->IsAddressPreviouslySpent(PKHash())); BOOST_CHECK(!wallet->IsAddressPreviouslySpent(ScriptHash())); - auto requests = wallet->GetAddressReceiveRequests(); - auto erequests = {"val_rr11"}; - BOOST_CHECK_EQUAL_COLLECTIONS(requests.begin(), requests.end(), std::begin(erequests), std::end(erequests)); + std::vector requests = wallet->GetAddressReceiveRequests(); + std::vector erequests = {{"val_rr11", false}}; + CompareRecReqVectors(requests, erequests); }); } } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index da20c8d1219..ac51fd983fb 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2823,12 +2823,13 @@ bool CWallet::IsAddressPreviouslySpent(const CTxDestination& dest) const return false; } -std::vector CWallet::GetAddressReceiveRequests() const +std::vector CWallet::GetAddressReceiveRequests() const { - std::vector values; + std::vector values; for (const auto& [dest, entry] : m_address_book) { for (const auto& [id, request] : entry.receive_requests) { - values.emplace_back(request); + bool active{IsDestinationActive(dest)}; + values.emplace_back(interfaces::ReceiveRequest(request, active)); } } return values; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 0b87d824f9e..c9897adf103 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -754,7 +754,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati bool IsAddressPreviouslySpent(const CTxDestination& dest) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool SetAddressPreviouslySpent(WalletBatch& batch, const CTxDestination& dest, bool used) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::vector GetAddressReceiveRequests() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + std::vector GetAddressReceiveRequests() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool SetAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool EraseAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From fc6cc2aa075749a1f9f2f5c6493007994ae48b9c Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Fri, 24 Mar 2023 14:32:09 -0400 Subject: [PATCH 7/8] qt: add warning labels in address book receive tab Tool tip can be expanded in the future. For now the first warning we show is if the address was derived from a non-active seed or descriptor. See https://github.com/bitcoin/bitcoin/issues/3314 --- src/interfaces/wallet.h | 5 ++-- src/qt/addresstablemodel.cpp | 40 ++++++++++++++++++++++++------ src/qt/addresstablemodel.h | 9 ++++--- src/qt/signverifymessagedialog.cpp | 2 +- src/qt/walletmodel.cpp | 6 ++--- src/qt/walletmodel.h | 2 +- src/wallet/interfaces.cpp | 2 +- 7 files changed, 47 insertions(+), 19 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 93040d0b8e6..3935052d36a 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -356,9 +356,10 @@ struct WalletAddress wallet::isminetype is_mine; wallet::AddressPurpose purpose; std::string name; + bool is_active{false}; - WalletAddress(CTxDestination dest, wallet::isminetype is_mine, wallet::AddressPurpose purpose, std::string name) - : dest(std::move(dest)), is_mine(is_mine), purpose(std::move(purpose)), name(std::move(name)) + WalletAddress(CTxDestination dest, wallet::isminetype is_mine, wallet::AddressPurpose purpose, std::string name, bool is_active) + : dest(std::move(dest)), is_mine(is_mine), purpose(std::move(purpose)), name(std::move(name)), is_active(is_active) { } }; diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index 0d0f1a4d154..612a657648b 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -30,10 +30,12 @@ struct AddressTableEntry Type type; QString label; QString address; + bool is_active; AddressTableEntry() = default; - AddressTableEntry(Type _type, const QString &_label, const QString &_address): - type(_type), label(_label), address(_address) {} + AddressTableEntry(Type _type, const QString& _label, const QString& _address, bool is_active) : type(_type), label(_label), address(_address), is_active(is_active) {} + + [[nodiscard]] QString GetAddressWarnings() const; }; struct AddressTableEntryLessThan @@ -65,6 +67,15 @@ static AddressTableEntry::Type translateTransactionType(wallet::AddressPurpose p assert(false); } +[[nodiscard]] QString AddressTableEntry::GetAddressWarnings() const +{ + QString warnings; + if (!is_active) + warnings += QObject::tr("This address should not be used. It was derived from an inactive seed, was imported, or may have been stored unencrypted.\n"); + + return warnings; +} + // Private implementation class AddressTablePriv { @@ -87,8 +98,9 @@ class AddressTablePriv AddressTableEntry::Type addressType = translateTransactionType( address.purpose, address.is_mine); cachedAddressTable.append(AddressTableEntry(addressType, - QString::fromStdString(address.name), - QString::fromStdString(EncodeDestination(address.dest)))); + QString::fromStdString(address.name), + QString::fromStdString(EncodeDestination(address.dest)), + address.is_active)); } } // std::lower_bound() and std::upper_bound() require our cachedAddressTable list to be sorted in asc order @@ -118,7 +130,7 @@ class AddressTablePriv break; } parent->beginInsertRows(QModelIndex(), lowerIndex, lowerIndex); - cachedAddressTable.insert(lowerIndex, AddressTableEntry(newEntryType, label, address)); + cachedAddressTable.insert(lowerIndex, AddressTableEntry(newEntryType, label, address, /*TODO:*/ false)); parent->endInsertRows(); break; case CT_UPDATED: @@ -162,10 +174,9 @@ class AddressTablePriv } }; -AddressTableModel::AddressTableModel(WalletModel *parent, bool pk_hash_only) : - QAbstractTableModel(parent), walletModel(parent) +AddressTableModel::AddressTableModel(const PlatformStyle* platformStyle, WalletModel* parent, bool pk_hash_only) : QAbstractTableModel(parent), walletModel(parent), platformStyle(platformStyle) { - columns << tr("Label") << tr("Address"); + columns << tr("Warnings") << tr("Label") << tr("Address"); priv = new AddressTablePriv(this); priv->refreshAddressTable(parent->wallet(), pk_hash_only); } @@ -201,6 +212,8 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const const auto column = static_cast(index.column()); if (role == Qt::DisplayRole || role == Qt::EditRole) { switch (column) { + case Warnings: + return {}; case Label: if (rec->label.isEmpty() && role == Qt::DisplayRole) { return tr("(no label)"); @@ -213,6 +226,7 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const assert(false); } else if (role == Qt::FontRole) { switch (column) { + case Warnings: case Label: return QFont(); case Address: @@ -230,6 +244,16 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const return {}; } // no default case, so the compiler can warn about missing cases assert(false); + } else if (role == Qt::DecorationRole) { + if (index.column() == Warnings) { + if (rec->GetAddressWarnings().isEmpty()) { + return {}; + } else { + return platformStyle->TextColorIcon(QIcon(":/icons/warning")); + } + } + } else if (role == Qt::ToolTipRole) { + if (index.column() == Warnings) return rec->GetAddressWarnings(); } return QVariant(); } diff --git a/src/qt/addresstablemodel.h b/src/qt/addresstablemodel.h index 599aa89cadd..7018af91e43 100644 --- a/src/qt/addresstablemodel.h +++ b/src/qt/addresstablemodel.h @@ -6,6 +6,7 @@ #define BITCOIN_QT_ADDRESSTABLEMODEL_H #include +#include #include #include @@ -30,12 +31,13 @@ class AddressTableModel : public QAbstractTableModel Q_OBJECT public: - explicit AddressTableModel(WalletModel *parent = nullptr, bool pk_hash_only = false); + explicit AddressTableModel(const PlatformStyle* platformStyle, WalletModel* parent = nullptr, bool pk_hash_only = false); ~AddressTableModel(); enum ColumnIndex { - Label = 0, /**< User specified label */ - Address = 1 /**< Bitcoin address */ + Warnings = 0, /**< Warn user about using this address with tooltip explanation */ + Label, /**< User specified label */ + Address /**< Bitcoin address */ }; enum RoleIndex { @@ -92,6 +94,7 @@ class AddressTableModel : public QAbstractTableModel AddressTablePriv *priv = nullptr; QStringList columns; EditStatus editStatus = OK; + const PlatformStyle* platformStyle; /** Look up address book data given an address string. */ bool getAddressData(const QString &address, std::string* name, wallet::AddressPurpose* purpose) const; diff --git a/src/qt/signverifymessagedialog.cpp b/src/qt/signverifymessagedialog.cpp index 0e725acb33e..9c1d3e1a3f0 100644 --- a/src/qt/signverifymessagedialog.cpp +++ b/src/qt/signverifymessagedialog.cpp @@ -90,7 +90,7 @@ void SignVerifyMessageDialog::on_addressBookButton_SM_clicked() { if (model && model->getAddressTableModel()) { - model->refresh(/*pk_hash_only=*/true); + model->refresh(platformStyle, /*pk_hash_only=*/true); AddressBookPage dlg(platformStyle, AddressBookPage::ForSelection, AddressBookPage::ReceivingTab, this); dlg.setModel(model->getAddressTableModel()); if (dlg.exec()) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index baeda4d7402..d0b78f0a085 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -49,7 +49,7 @@ WalletModel::WalletModel(std::unique_ptr wallet, ClientModel timer(new QTimer(this)) { fHaveWatchOnly = m_wallet->haveWatchOnly(); - addressTableModel = new AddressTableModel(this); + addressTableModel = new AddressTableModel(platformStyle, this); transactionTableModel = new TransactionTableModel(platformStyle, this); recentRequestsTableModel = new RecentRequestsTableModel(platformStyle, this); @@ -600,9 +600,9 @@ bool WalletModel::isMultiwallet() const return m_node.walletLoader().getWallets().size() > 1; } -void WalletModel::refresh(bool pk_hash_only) +void WalletModel::refresh(const PlatformStyle* platformStyle, bool pk_hash_only) { - addressTableModel = new AddressTableModel(this, pk_hash_only); + addressTableModel = new AddressTableModel(platformStyle, this, pk_hash_only); } uint256 WalletModel::getLastBlockProcessed() const diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 4f75d41404a..59467fd5a16 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -149,7 +149,7 @@ class WalletModel : public QObject bool isMultiwallet() const; - void refresh(bool pk_hash_only = false); + void refresh(const PlatformStyle* platformStyle, bool pk_hash_only = false); uint256 getLastBlockProcessed() const; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 4d385e81a2b..18892b0652a 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -219,7 +219,7 @@ class WalletImpl : public Wallet if (is_change) return; isminetype is_mine = m_wallet->IsMine(dest); // In very old wallets, address purpose may not be recorded so we derive it from IsMine - result.emplace_back(dest, is_mine, purpose.value_or(is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND), label); + result.emplace_back(dest, is_mine, purpose.value_or(is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND), label, m_wallet->IsDestinationActive(dest)); }); return result; } From d6519a80eaa79371616053906723e74c6f06cc1a Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Tue, 4 Apr 2023 15:08:26 -0400 Subject: [PATCH 8/8] qt: update address warnings when wallet is encrypted --- src/interfaces/wallet.h | 10 ++++++---- src/qt/addresstablemodel.cpp | 9 ++++----- src/qt/addresstablemodel.h | 2 +- src/qt/bitcoingui.cpp | 3 +++ src/qt/signverifymessagedialog.cpp | 2 +- src/qt/walletmodel.cpp | 31 ++++++++++++++++-------------- src/qt/walletmodel.h | 5 +++-- src/qt/walletview.cpp | 8 ++++++++ src/qt/walletview.h | 1 + src/wallet/interfaces.cpp | 2 +- src/wallet/wallet.cpp | 5 +++-- src/wallet/wallet.h | 2 +- 12 files changed, 49 insertions(+), 31 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 3935052d36a..2fc2a1448a1 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -293,10 +293,12 @@ class Wallet //! Register handler for address book changed messages. using AddressBookChangedFn = std::function; + const std::string& label, + bool is_mine, + wallet::AddressPurpose purpose, + ChangeType status, + bool is_active)>; + virtual std::unique_ptr handleAddressBookChanged(AddressBookChangedFn fn) = 0; //! Register handler for transaction changed messages. diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index 612a657648b..a5d838685ca 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -109,7 +109,7 @@ class AddressTablePriv std::sort(cachedAddressTable.begin(), cachedAddressTable.end(), AddressTableEntryLessThan()); } - void updateEntry(const QString &address, const QString &label, bool isMine, wallet::AddressPurpose purpose, int status) + void updateEntry(const QString& address, const QString& label, bool isMine, wallet::AddressPurpose purpose, int status, bool isActive) { // Find address / label in model QList::iterator lower = std::lower_bound( @@ -130,7 +130,7 @@ class AddressTablePriv break; } parent->beginInsertRows(QModelIndex(), lowerIndex, lowerIndex); - cachedAddressTable.insert(lowerIndex, AddressTableEntry(newEntryType, label, address, /*TODO:*/ false)); + cachedAddressTable.insert(lowerIndex, AddressTableEntry(newEntryType, label, address, isActive)); parent->endInsertRows(); break; case CT_UPDATED: @@ -357,11 +357,10 @@ QModelIndex AddressTableModel::index(int row, int column, const QModelIndex &par } } -void AddressTableModel::updateEntry(const QString &address, - const QString &label, bool isMine, wallet::AddressPurpose purpose, int status) +void AddressTableModel::updateEntry(const QString& address, const QString& label, bool isMine, wallet::AddressPurpose purpose, int status, bool isActive) { // Update address book model from Bitcoin core - priv->updateEntry(address, label, isMine, purpose, status); + priv->updateEntry(address, label, isMine, purpose, status, isActive); } QString AddressTableModel::addRow(const QString &type, const QString &label, const QString &address, const OutputType address_type) diff --git a/src/qt/addresstablemodel.h b/src/qt/addresstablemodel.h index 7018af91e43..88ada425385 100644 --- a/src/qt/addresstablemodel.h +++ b/src/qt/addresstablemodel.h @@ -105,7 +105,7 @@ class AddressTableModel : public QAbstractTableModel public Q_SLOTS: /* Update address list from core. */ - void updateEntry(const QString &address, const QString &label, bool isMine, wallet::AddressPurpose purpose, int status); + void updateEntry(const QString& address, const QString& label, bool isMine, wallet::AddressPurpose purpose, int status, bool isActive); friend class AddressTablePriv; }; diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index d26ef52eb43..15c77612b30 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -1418,6 +1418,9 @@ void BitcoinGUI::updateWalletStatus() WalletModel * const walletModel = walletView->getWalletModel(); setEncryptionStatus(walletModel->getEncryptionStatus()); setHDStatus(walletModel->wallet().privateKeysDisabled(), walletModel->wallet().hdEnabled()); + + // Refresh requests table and address book to show warnings on old unencrypted keys + walletView->refreshAddressTables(); } #endif // ENABLE_WALLET diff --git a/src/qt/signverifymessagedialog.cpp b/src/qt/signverifymessagedialog.cpp index 9c1d3e1a3f0..6a4bc0cb5fa 100644 --- a/src/qt/signverifymessagedialog.cpp +++ b/src/qt/signverifymessagedialog.cpp @@ -90,7 +90,7 @@ void SignVerifyMessageDialog::on_addressBookButton_SM_clicked() { if (model && model->getAddressTableModel()) { - model->refresh(platformStyle, /*pk_hash_only=*/true); + model->RefreshAddressTableModel(platformStyle, /*pk_hash_only=*/true); AddressBookPage dlg(platformStyle, AddressBookPage::ForSelection, AddressBookPage::ReceivingTab, this); dlg.setModel(model->getAddressTableModel()); if (dlg.exec()) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index d0b78f0a085..76008af53ff 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -137,11 +137,10 @@ void WalletModel::updateTransaction() fForceCheckBalanceChanged = true; } -void WalletModel::updateAddressBook(const QString &address, const QString &label, - bool isMine, wallet::AddressPurpose purpose, int status) +void WalletModel::updateAddressBook(const QString& address, const QString& label, bool isMine, wallet::AddressPurpose purpose, int status, bool isActive) { if(addressTableModel) - addressTableModel->updateEntry(address, label, isMine, purpose, status); + addressTableModel->updateEntry(address, label, isMine, purpose, status, isActive); } void WalletModel::updateWatchOnlyFlag(bool fHaveWatchonly) @@ -375,20 +374,19 @@ static void NotifyKeyStoreStatusChanged(WalletModel *walletmodel) assert(invoked); } -static void NotifyAddressBookChanged(WalletModel *walletmodel, - const CTxDestination &address, const std::string &label, bool isMine, - wallet::AddressPurpose purpose, ChangeType status) +static void NotifyAddressBookChanged(WalletModel* walletmodel, const CTxDestination& address, const std::string& label, bool isMine, wallet::AddressPurpose purpose, ChangeType status, bool isActive) { QString strAddress = QString::fromStdString(EncodeDestination(address)); QString strLabel = QString::fromStdString(label); - qDebug() << "NotifyAddressBookChanged: " + strAddress + " " + strLabel + " isMine=" + QString::number(isMine) + " purpose=" + QString::number(static_cast(purpose)) + " status=" + QString::number(status); + qDebug() << "NotifyAddressBookChanged: " + strAddress + " " + strLabel + " isMine=" + QString::number(isMine) + " purpose=" + QString::number(static_cast(purpose)) + " status=" + QString::number(status) + " isActive=" + QString::number(isActive); bool invoked = QMetaObject::invokeMethod(walletmodel, "updateAddressBook", - Q_ARG(QString, strAddress), - Q_ARG(QString, strLabel), - Q_ARG(bool, isMine), - Q_ARG(wallet::AddressPurpose, purpose), - Q_ARG(int, status)); + Q_ARG(QString, strAddress), + Q_ARG(QString, strLabel), + Q_ARG(bool, isMine), + Q_ARG(wallet::AddressPurpose, purpose), + Q_ARG(int, status), + Q_ARG(bool, isActive)); assert(invoked); } @@ -427,7 +425,7 @@ void WalletModel::subscribeToCoreSignals() // Connect signals to wallet m_handler_unload = m_wallet->handleUnload(std::bind(&NotifyUnload, this)); m_handler_status_changed = m_wallet->handleStatusChanged(std::bind(&NotifyKeyStoreStatusChanged, this)); - m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5)); + m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5, std::placeholders::_6)); m_handler_transaction_changed = m_wallet->handleTransactionChanged(std::bind(NotifyTransactionChanged, this, std::placeholders::_1, std::placeholders::_2)); m_handler_show_progress = m_wallet->handleShowProgress(std::bind(ShowProgress, this, std::placeholders::_1, std::placeholders::_2)); m_handler_watch_only_changed = m_wallet->handleWatchOnlyChanged(std::bind(NotifyWatchonlyChanged, this, std::placeholders::_1)); @@ -600,11 +598,16 @@ bool WalletModel::isMultiwallet() const return m_node.walletLoader().getWallets().size() > 1; } -void WalletModel::refresh(const PlatformStyle* platformStyle, bool pk_hash_only) +void WalletModel::RefreshAddressTableModel(const PlatformStyle* platformStyle, bool pk_hash_only) { addressTableModel = new AddressTableModel(platformStyle, this, pk_hash_only); } +void WalletModel::RefreshRecentRequestsTableModel(const PlatformStyle* platformStyle) +{ + recentRequestsTableModel = new RecentRequestsTableModel(platformStyle, this); +} + uint256 WalletModel::getLastBlockProcessed() const { return m_client_model ? m_client_model->getBestBlockHash() : uint256{}; diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 59467fd5a16..379c9f7cc25 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -149,7 +149,8 @@ class WalletModel : public QObject bool isMultiwallet() const; - void refresh(const PlatformStyle* platformStyle, bool pk_hash_only = false); + void RefreshAddressTableModel(const PlatformStyle* platformStyle, bool pk_hash_only = false); + void RefreshRecentRequestsTableModel(const PlatformStyle* platformStyle); uint256 getLastBlockProcessed() const; @@ -236,7 +237,7 @@ public Q_SLOTS: /* New transaction, or transaction changed status */ void updateTransaction(); /* New, updated or removed address book entry */ - void updateAddressBook(const QString &address, const QString &label, bool isMine, wallet::AddressPurpose purpose, int status); + void updateAddressBook(const QString& address, const QString& label, bool isMine, wallet::AddressPurpose purpose, int status, bool isActive); /* Watch-only added */ void updateWatchOnlyFlag(bool fHaveWatchonly); /* Current, immature or unconfirmed balance might have changed - emit 'balanceChanged' if so */ diff --git a/src/qt/walletview.cpp b/src/qt/walletview.cpp index 09e07715340..f4115acbf79 100644 --- a/src/qt/walletview.cpp +++ b/src/qt/walletview.cpp @@ -200,6 +200,14 @@ void WalletView::showOutOfSyncWarning(bool fShow) overviewPage->showOutOfSyncWarning(fShow); } +void WalletView::refreshAddressTables() +{ + walletModel->RefreshRecentRequestsTableModel(platformStyle); + receiveCoinsPage->setModel(walletModel); + walletModel->RefreshAddressTableModel(platformStyle); + usedReceivingAddressesPage->setModel(walletModel->getAddressTableModel()); +} + void WalletView::encryptWallet() { auto dlg = new AskPassphraseDialog(AskPassphraseDialog::Encrypt, this); diff --git a/src/qt/walletview.h b/src/qt/walletview.h index 475085044db..c571a290a6a 100644 --- a/src/qt/walletview.h +++ b/src/qt/walletview.h @@ -48,6 +48,7 @@ class WalletView : public QStackedWidget bool handlePaymentRequest(const SendCoinsRecipient& recipient); void showOutOfSyncWarning(bool fShow); + void refreshAddressTables(); private: ClientModel* clientModel{nullptr}; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 18892b0652a..6a762640f39 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -540,7 +540,7 @@ class WalletImpl : public Wallet { return MakeSignalHandler(m_wallet->NotifyAddressBookChanged.connect( [fn](const CTxDestination& address, const std::string& label, bool is_mine, - AddressPurpose purpose, ChangeType status) { fn(address, label, is_mine, purpose, status); })); + AddressPurpose purpose, ChangeType status, bool is_active) { fn(address, label, is_mine, purpose, status, is_active); })); } std::unique_ptr handleTransactionChanged(TransactionChangedFn fn) override { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ac51fd983fb..09b21fa2418 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2371,6 +2371,7 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add bool fUpdated = false; bool is_mine; std::optional purpose; + bool is_active = IsDestinationActive(address); { LOCK(cs_wallet); std::map::iterator mi = m_address_book.find(address); @@ -2386,7 +2387,7 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add // In very old wallets, address purpose may not be recorded so we derive it from IsMine NotifyAddressBookChanged(address, strName, is_mine, purpose.value_or(is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND), - (fUpdated ? CT_UPDATED : CT_NEW)); + (fUpdated ? CT_UPDATED : CT_NEW), is_active); if (new_purpose && !batch.WritePurpose(EncodeDestination(address), PurposeToString(*new_purpose))) return false; return batch.WriteName(EncodeDestination(address), strName); @@ -2415,7 +2416,7 @@ bool CWallet::DelAddressBook(const CTxDestination& address) m_address_book.erase(address); } - NotifyAddressBookChanged(address, "", /*is_mine=*/false, AddressPurpose::SEND, CT_DELETED); + NotifyAddressBookChanged(address, "", /*is_mine=*/false, AddressPurpose::SEND, CT_DELETED, false); batch.ErasePurpose(EncodeDestination(address)); return batch.EraseName(EncodeDestination(address)); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c9897adf103..934fc39dd26 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -787,7 +787,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati */ boost::signals2::signal + AddressPurpose purpose, ChangeType status, bool isActive)> NotifyAddressBookChanged; /**