diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 6f697264a6fd5..fc31dcc9864bf 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -134,7 +134,7 @@ class Wallet std::string* purpose) = 0; //! Get wallet address list. - virtual std::vector<WalletAddress> getAddresses() = 0; + virtual std::vector<WalletAddress> getAddresses() const = 0; //! Get receive requests. virtual std::vector<std::string> getAddressReceiveRequests() = 0; diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 92f826fedd3ba..9ecbc8dc3fa71 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -324,12 +324,6 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool read_only, b env = database.env.get(); pdb = database.m_db.get(); strFile = database.strFile; - if (!Exists(std::string("version"))) { - bool fTmp = fReadOnly; - fReadOnly = false; - Write(std::string("version"), CLIENT_VERSION); - fReadOnly = fTmp; - } } void BerkeleyDatabase::Open() diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 15e4f660224cb..4ef24818f0049 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -205,29 +205,27 @@ class WalletImpl : public Wallet std::string* purpose) override { LOCK(m_wallet->cs_wallet); - auto it = m_wallet->m_address_book.find(dest); - if (it == m_wallet->m_address_book.end() || it->second.IsChange()) { - return false; - } + const auto& entry = m_wallet->FindAddressBookEntry(dest, /*allow_change=*/false); + if (!entry) return false; // addr not found if (name) { - *name = it->second.GetLabel(); + *name = entry->GetLabel(); } if (is_mine) { *is_mine = m_wallet->IsMine(dest); } if (purpose) { - *purpose = it->second.purpose; + *purpose = entry->purpose; } return true; } - std::vector<WalletAddress> getAddresses() override + std::vector<WalletAddress> getAddresses() const override { LOCK(m_wallet->cs_wallet); std::vector<WalletAddress> result; - for (const auto& item : m_wallet->m_address_book) { - if (item.second.IsChange()) continue; - result.emplace_back(item.first, m_wallet->IsMine(item.first), item.second.GetLabel(), item.second.purpose); - } + m_wallet->ForEachAddrBookEntry([&](const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change) EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet) { + if (is_change) return; + result.emplace_back(dest, m_wallet->IsMine(dest), label, purpose); + }); return result; } std::vector<std::string> getAddressReceiveRequests() override { diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index cf2be39588a02..746a5258b368e 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -540,12 +540,12 @@ static RPCHelpMan listaddressbalances() static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { - std::set<CTxDestination> address_set; + std::vector<CTxDestination> address_vector; if (by_label) { // Get the set of addresses assigned to label std::string label = LabelFromValue(params[0]); - address_set = wallet.GetLabelAddresses(label); + address_vector = wallet.ListAddrBookAddresses(CWallet::AddrBookFilter{label}); } else { // Get the address CTxDestination dest = DecodeDestination(params[0].get_str()); @@ -556,7 +556,7 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b if (!wallet.IsMine(script_pub_key)) { throw JSONRPCError(RPC_WALLET_ERROR, "Address not found in wallet"); } - address_set.insert(dest); + address_vector.emplace_back(dest); } // Minimum confirmations @@ -576,7 +576,7 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b for (const CTxOut& txout : wtx.tx->vout) { CTxDestination address; - if (ExtractDestination(txout.scriptPubKey, address) && wallet.IsMine(address) && address_set.count(address)) { + if (ExtractDestination(txout.scriptPubKey, address) && wallet.IsMine(address) && std::find(address_vector.begin(), address_vector.end(), address) != address_vector.end()) { amount += txout.nValue; } } @@ -3776,17 +3776,6 @@ static UniValue DescribeWalletAddress(const CWallet& wallet, const CTxDestinatio return ret; } -/** Convert CAddressBookData to JSON record. */ -static UniValue AddressBookDataToJSON(const CAddressBookData& data, const bool verbose) -{ - UniValue ret(UniValue::VOBJ); - if (verbose) { - ret.pushKV("name", data.GetLabel()); - } - ret.pushKV("purpose", data.purpose); - return ret; -} - RPCHelpMan getaddressinfo() { return RPCHelpMan{"getaddressinfo", @@ -3957,10 +3946,10 @@ static RPCHelpMan getaddressesbylabel() // Find all addresses that have the given label UniValue ret(UniValue::VOBJ); std::set<std::string> addresses; - for (const std::pair<const CTxDestination, CAddressBookData>& item : pwallet->m_address_book) { - if (item.second.IsChange()) continue; - if (item.second.GetLabel() == label) { - std::string address = EncodeDestination(item.first); + pwallet->ForEachAddrBookEntry([&](const CTxDestination& _dest, const std::string& _label, const std::string& _purpose, bool _is_change) { + if (_is_change) return; + if (_label == label) { + std::string address = EncodeDestination(_dest); // CWallet::m_address_book is not expected to contain duplicate // address strings, but build a separate set as a precaution just in // case it does. @@ -3970,9 +3959,11 @@ static RPCHelpMan getaddressesbylabel() // and since duplicate addresses are unexpected (checked with // std::set in O(log(N))), UniValue::__pushKV is used instead, // which currently is O(1). - ret.__pushKV(address, AddressBookDataToJSON(item.second, false)); + UniValue value(UniValue::VOBJ); + value.pushKV("purpose", _purpose); + ret.__pushKV(address, value); } - } + }); if (ret.empty()) { throw JSONRPCError(RPC_WALLET_INVALID_LABEL_NAME, std::string("No addresses with label " + label)); @@ -4019,13 +4010,7 @@ static RPCHelpMan listlabels() } // Add to a set to sort by label name, then insert into Univalue array - std::set<std::string> label_set; - for (const std::pair<const CTxDestination, CAddressBookData>& entry : pwallet->m_address_book) { - if (entry.second.IsChange()) continue; - if (purpose.empty() || entry.second.purpose == purpose) { - label_set.insert(entry.second.GetLabel()); - } - } + std::set<std::string> label_set = pwallet->ListAddrBookLabels(purpose); UniValue ret(UniValue::VARR); for (const std::string& name : label_set) { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 017670e825e2b..693e85d672b4e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2900,21 +2900,44 @@ void CWallet::MarkDestinationsDirty(const std::set<CTxDestination>& destinations } } -std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) const +void CWallet::ForEachAddrBookEntry(const ListAddrBookFunc& func) const { - LOCK(cs_wallet); - std::set<CTxDestination> result; - for (const std::pair<const CTxDestination, CAddressBookData>& item : m_address_book) - { - if (item.second.IsChange()) continue; - const CTxDestination& address = item.first; - const std::string& strName = item.second.GetLabel(); - if (strName == label) - result.insert(address); + for (const std::pair<const CTxDestination, CAddressBookData>& item : m_address_book) { + const auto& entry = item.second; + func(item.first, entry.GetLabel(), entry.purpose, entry.IsChange()); } +} + +std::vector<CTxDestination> CWallet::ListAddrBookAddresses(const std::optional<AddrBookFilter>& _filter) const +{ + AssertLockHeld(cs_wallet); + std::vector<CTxDestination> result; + AddrBookFilter filter = _filter ? *_filter : AddrBookFilter(); + ForEachAddrBookEntry([&result, &filter](const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change) { + // Filter by change + if (filter.ignore_change && is_change) return; + // Filter by label + if (filter.m_op_label && *filter.m_op_label != label) return; + // All good + result.emplace_back(dest); + }); return result; } +std::set<std::string> CWallet::ListAddrBookLabels(const std::string& purpose) const +{ + AssertLockHeld(cs_wallet); + std::set<std::string> label_set; + ForEachAddrBookEntry([&](const CTxDestination& _dest, const std::string& _label, + const std::string& _purpose, bool _is_change) { + if (_is_change) return; + if (purpose.empty() || _purpose == purpose) { + label_set.insert(_label); + } + }); + return label_set; +} + bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool fInternalIn) { m_spk_man = pwallet->GetScriptPubKeyMan(fInternalIn); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index eec6b95d87bd3..0aef724b278b0 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -793,7 +793,30 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati std::set<std::set<CTxDestination>> GetAddressGroupings() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); std::map<CTxDestination, CAmount> GetAddressBalances() const; - std::set<CTxDestination> GetLabelAddresses(const std::string& label) const; + // Filter struct for 'ListAddrBookAddresses' + struct AddrBookFilter { + // Fetch addresses with the provided label + std::optional<std::string> m_op_label{std::nullopt}; + // Don't include change addresses by default + bool ignore_change{true}; + }; + + /** + * Filter and retrieve destinations stored in the addressbook + */ + std::vector<CTxDestination> ListAddrBookAddresses(const std::optional<AddrBookFilter>& filter) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + /** + * Retrieve all the known labels in the address book + */ + std::set<std::string> ListAddrBookLabels(const std::string& purpose) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + /** + * Walk-through the address book entries. + * Stops when the provided 'ListAddrBookFunc' returns false. + */ + using ListAddrBookFunc = std::function<void(const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change)>; + void ForEachAddrBookEntry(const ListAddrBookFunc& func) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Marks all outputs in each one of the destinations dirty, so their cache is diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index f9262d1579371..c895c6f483586 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -879,12 +879,10 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) if (result != DBErrors::LOAD_OK) return result; - // Last client version to open this wallet, was previously the file version number + // Last client version to open this wallet int last_client = CLIENT_VERSION; - m_batch->Read(DBKeys::VERSION, last_client); - - int wallet_version = pwallet->GetVersion(); - pwallet->WalletLogPrintf("Wallet File Version = %d\n", wallet_version > 0 ? wallet_version : last_client); + bool has_last_client = m_batch->Read(DBKeys::VERSION, last_client); + pwallet->WalletLogPrintf("Wallet file version = %d, last client version = %d\n", pwallet->GetVersion(), last_client); pwallet->WalletLogPrintf("Keys: %u plaintext, %u encrypted, %u total; Watch scripts: %u; HD PubKeys: %u; Metadata: %u; Unknown wallet records: %u\n", wss.nKeys, wss.nCKeys, wss.nKeys + wss.nCKeys, @@ -906,7 +904,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) if (wss.fIsEncrypted && (last_client == 40000 || last_client == 50000)) return DBErrors::NEED_REWRITE; - if (last_client < CLIENT_VERSION) // Update + if (!has_last_client || last_client != CLIENT_VERSION) // Update m_batch->Write(DBKeys::VERSION, CLIENT_VERSION); if (wss.fAnyUnordered) diff --git a/test/functional/feature_dbcrash.py b/test/functional/feature_dbcrash.py index 96e97c4da8307..030a8b112a0ee 100755 --- a/test/functional/feature_dbcrash.py +++ b/test/functional/feature_dbcrash.py @@ -62,8 +62,8 @@ def set_test_params(self): self.node2_args = ["-dbcrashratio=24", "-dbcache=16"] + self.base_args # Node3 is a normal node with default args, will mine full blocks - # and non-standard txs (e.g. txs with "dust" outputs) - self.node3_args = ["-acceptnonstdtxn"] + # and txs (with "dust" outputs + self.node3_args = ["-dustrelayfee=0"] self.extra_args = [self.node0_args, self.node1_args, self.node2_args, self.node3_args] def skip_test_if_missing_module(self): @@ -217,7 +217,11 @@ def run_test(self): # Start by creating a lot of utxos on node3 initial_height = self.nodes[3].getblockcount() - utxo_list = create_confirmed_utxos(self, self.nodes[3].getnetworkinfo()['relayfee'], self.nodes[3], 5000, sync_fun=self.no_op) + utxo_list = [] + for _ in range(5): + utxo_list.extend(create_confirmed_utxos(self, self.nodes[3].getnetworkinfo()['relayfee'], self.nodes[3], 1000, sync_fun=self.no_op)) + self.generate(self.nodes[3], 1, sync_fun=self.no_op) + assert_equal(len(self.nodes[3].getrawmempool()), 0) self.log.info(f"Prepped {len(utxo_list)} utxo entries") # Sync these blocks with the other nodes diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 8d393a6f08866..7d2007b354057 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -27,11 +27,11 @@ def set_test_params(self): self.num_nodes = 4 if self.options.descriptors: self.extra_args = [[ - "-acceptnonstdtxn=1", "-whitelist=noban@127.0.0.1" + "-dustrelayfee=0", "-whitelist=noban@127.0.0.1" ] for i in range(self.num_nodes)] else: self.extra_args = [[ - "-acceptnonstdtxn=1", "-whitelist=noban@127.0.0.1", + "-dustrelayfee=0", "-whitelist=noban@127.0.0.1", '-usehd={:d}'.format(i%2==0) ] for i in range(self.num_nodes)] self.setup_clean_chain = True diff --git a/test/functional/wallet_listreceivedby.py b/test/functional/wallet_listreceivedby.py index 33ddc104c352b..3d4f47b71a200 100755 --- a/test/functional/wallet_listreceivedby.py +++ b/test/functional/wallet_listreceivedby.py @@ -62,6 +62,11 @@ def run_test(self): {"address": empty_addr}, {"address": empty_addr, "label": "", "amount": 0, "confirmations": 0, "txids": []}) + # No returned addy should be a change addr + for node in self.nodes: + for addr_obj in node.listreceivedbyaddress(): + assert_equal(node.getaddressinfo(addr_obj["address"])["ischange"], False) + # Test Address filtering # Only on addr expected = {"address": addr, "label": "", "amount": Decimal("0.1"), "confirmations": 10, "txids": [txid, ]}