diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 086f6d9de8..6cb6bd50d5 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -228,9 +228,22 @@ public: return m_wallet->GetAddressReceiveRequests(); } bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) override { + // Note: The setAddressReceiveRequest interface used by the GUI to store + // receive requests is a little awkward and could be improved in the + // future: + // + // - The same method is used to save requests and erase them, but + // having separate methods could be clearer and prevent bugs. + // + // - Request ids are passed as strings even though they are generated as + // integers. + // + // - Multiple requests can be stored for the same address, but it might + // be better to only allow one request or only keep the current one. LOCK(m_wallet->cs_wallet); WalletBatch batch{m_wallet->GetDatabase()}; - return m_wallet->SetAddressReceiveRequest(batch, dest, id, value); + return value.empty() ? m_wallet->EraseAddressReceiveRequest(batch, dest, id) + : m_wallet->SetAddressReceiveRequest(batch, dest, id, value); } bool displayAddress(const CTxDestination& dest) override { diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 772feed240..da4b553394 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -427,19 +427,63 @@ BOOST_AUTO_TEST_CASE(ComputeTimeSmart) BOOST_CHECK_EQUAL(AddTx(*m_node.chainman, m_wallet, 5, 50, 600), 300); } -BOOST_AUTO_TEST_CASE(LoadReceiveRequests) -{ - CTxDestination dest = PKHash(); - LOCK(m_wallet.cs_wallet); - WalletBatch batch{m_wallet.GetDatabase()}; - m_wallet.SetAddressUsed(batch, dest, true); - m_wallet.SetAddressReceiveRequest(batch, dest, "0", "val_rr0"); - m_wallet.SetAddressReceiveRequest(batch, dest, "1", "val_rr1"); +static const DatabaseFormat DATABASE_FORMATS[] = { +#ifdef USE_SQLITE + DatabaseFormat::SQLITE, +#endif +#ifdef USE_BDB + DatabaseFormat::BERKELEY, +#endif +}; - auto values = m_wallet.GetAddressReceiveRequests(); - BOOST_CHECK_EQUAL(values.size(), 2U); - BOOST_CHECK_EQUAL(values[0], "val_rr0"); - BOOST_CHECK_EQUAL(values[1], "val_rr1"); +void TestLoadWallet(const std::string& name, DatabaseFormat format, std::function)> f) +{ + node::NodeContext node; + auto chain{interfaces::MakeChain(node)}; + DatabaseOptions options; + options.require_format = format; + DatabaseStatus status; + bilingual_str error; + std::vector warnings; + auto database{MakeWalletDatabase(name, options, status, error)}; + auto wallet{std::make_shared(chain.get(), "", std::move(database))}; + BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::LOAD_OK); + WITH_LOCK(wallet->cs_wallet, f(wallet)); +} + +BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup) +{ + for (DatabaseFormat format : DATABASE_FORMATS) { + const std::string name{strprintf("receive-requests-%i", format)}; + TestLoadWallet(name, format, [](std::shared_ptr wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) { + BOOST_CHECK(!wallet->IsAddressPreviouslySpent(PKHash())); + WalletBatch batch{wallet->GetDatabase()}; + BOOST_CHECK(batch.WriteAddressPreviouslySpent(PKHash(), true)); + BOOST_CHECK(batch.WriteAddressPreviouslySpent(ScriptHash(), true)); + BOOST_CHECK(wallet->SetAddressReceiveRequest(batch, PKHash(), "0", "val_rr00")); + BOOST_CHECK(wallet->EraseAddressReceiveRequest(batch, PKHash(), "0")); + BOOST_CHECK(wallet->SetAddressReceiveRequest(batch, PKHash(), "1", "val_rr10")); + BOOST_CHECK(wallet->SetAddressReceiveRequest(batch, PKHash(), "1", "val_rr11")); + BOOST_CHECK(wallet->SetAddressReceiveRequest(batch, ScriptHash(), "2", "val_rr20")); + }); + 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)); + WalletBatch batch{wallet->GetDatabase()}; + BOOST_CHECK(batch.WriteAddressPreviouslySpent(PKHash(), false)); + BOOST_CHECK(batch.EraseAddressData(ScriptHash())); + }); + 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)); + }); + } } // Test some watch-only LegacyScriptPubKeyMan methods by the procedure of loading (LoadWatchOnly), diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6df0d12df6..76ebf115ad 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -974,11 +974,11 @@ void CWallet::SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned CTxDestination dst; if (ExtractDestination(srctx->tx->vout[n].scriptPubKey, dst)) { if (IsMine(dst)) { - if (used != IsAddressUsed(dst)) { + if (used != IsAddressPreviouslySpent(dst)) { if (used) { tx_destinations.insert(dst); } - SetAddressUsed(batch, dst, used); + SetAddressPreviouslySpent(batch, dst, used); } } } @@ -991,7 +991,7 @@ bool CWallet::IsSpentKey(const CScript& scriptPubKey) const if (!ExtractDestination(scriptPubKey, dest)) { return false; } - if (IsAddressUsed(dest)) { + if (IsAddressPreviouslySpent(dest)) { return true; } if (IsLegacy()) { @@ -999,15 +999,15 @@ bool CWallet::IsSpentKey(const CScript& scriptPubKey) const assert(spk_man != nullptr); for (const auto& keyid : GetAffectedKeys(scriptPubKey, *spk_man)) { WitnessV0KeyHash wpkh_dest(keyid); - if (IsAddressUsed(wpkh_dest)) { + if (IsAddressPreviouslySpent(wpkh_dest)) { return true; } ScriptHash sh_wpkh_dest(GetScriptForDestination(wpkh_dest)); - if (IsAddressUsed(sh_wpkh_dest)) { + if (IsAddressPreviouslySpent(sh_wpkh_dest)) { return true; } PKHash pkh_dest(keyid); - if (IsAddressUsed(pkh_dest)) { + if (IsAddressPreviouslySpent(pkh_dest)) { return true; } } @@ -2437,19 +2437,15 @@ bool CWallet::DelAddressBook(const CTxDestination& address) WalletBatch batch(GetDatabase()); { LOCK(cs_wallet); - // If we want to delete receiving addresses, we need to take care that DestData "used" (and possibly newer DestData) gets preserved (and the "deleted" address transformed into a change entry instead of actually being deleted) - // NOTE: This isn't a problem for sending addresses because they never have any DestData yet! - // When adding new DestData, it should be considered here whether to retain or delete it (or move it?). + // If we want to delete receiving addresses, we should avoid calling EraseAddressData because it will delete the previously_spent value. Could instead just erase the label so it becomes a change address, and keep the data. + // NOTE: This isn't a problem for sending addresses because they don't have any data that needs to be kept. + // When adding new address data, it should be considered here whether to retain or delete it. if (IsMine(address)) { WalletLogPrintf("%s called with IsMine address, NOT SUPPORTED. Please report this bug! %s\n", __func__, PACKAGE_BUGREPORT); return false; } - // Delete destdata tuples associated with address - std::string strAddress = EncodeDestination(address); - for (const std::pair &item : m_address_book[address].destdata) - { - batch.EraseDestData(strAddress, item.first); - } + // Delete data rows associated with this address + batch.EraseAddressData(address); m_address_book.erase(address); } @@ -2824,51 +2820,42 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old return nTimeSmart; } -bool CWallet::SetAddressUsed(WalletBatch& batch, const CTxDestination& dest, bool used) +bool CWallet::SetAddressPreviouslySpent(WalletBatch& batch, const CTxDestination& dest, bool used) { - const std::string key{"used"}; if (std::get_if(&dest)) return false; if (!used) { - if (auto* data = util::FindKey(m_address_book, dest)) data->destdata.erase(key); - return batch.EraseDestData(EncodeDestination(dest), key); + if (auto* data{util::FindKey(m_address_book, dest)}) data->previously_spent = false; + return batch.WriteAddressPreviouslySpent(dest, false); } - const std::string value{"1"}; - m_address_book[dest].destdata.insert(std::make_pair(key, value)); - return batch.WriteDestData(EncodeDestination(dest), key, value); + LoadAddressPreviouslySpent(dest); + return batch.WriteAddressPreviouslySpent(dest, true); } -void CWallet::LoadDestData(const CTxDestination &dest, const std::string &key, const std::string &value) +void CWallet::LoadAddressPreviouslySpent(const CTxDestination& dest) { - m_address_book[dest].destdata.insert(std::make_pair(key, value)); + m_address_book[dest].previously_spent = true; } -bool CWallet::IsAddressUsed(const CTxDestination& dest) const +void CWallet::LoadAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& request) { - const std::string key{"used"}; - std::map::const_iterator i = m_address_book.find(dest); - if(i != m_address_book.end()) - { - CAddressBookData::StringMap::const_iterator j = i->second.destdata.find(key); - if(j != i->second.destdata.end()) - { - return true; - } - } + m_address_book[dest].receive_requests[id] = request; +} + +bool CWallet::IsAddressPreviouslySpent(const CTxDestination& dest) const +{ + if (auto* data{util::FindKey(m_address_book, dest)}) return data->previously_spent; return false; } std::vector CWallet::GetAddressReceiveRequests() const { - const std::string prefix{"rr"}; std::vector values; - for (const auto& address : m_address_book) { - for (const auto& data : address.second.destdata) { - if (!data.first.compare(0, prefix.size(), prefix)) { - values.emplace_back(data.second); - } + for (const auto& [dest, entry] : m_address_book) { + for (const auto& [id, request] : entry.receive_requests) { + values.emplace_back(request); } } return values; @@ -2876,15 +2863,15 @@ std::vector CWallet::GetAddressReceiveRequests() const bool CWallet::SetAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id, const std::string& value) { - const std::string key{"rr" + id}; // "rr" prefix = "receive request" in destdata - CAddressBookData& data = m_address_book.at(dest); - if (value.empty()) { - if (!batch.EraseDestData(EncodeDestination(dest), key)) return false; - data.destdata.erase(key); - } else { - if (!batch.WriteDestData(EncodeDestination(dest), key, value)) return false; - data.destdata[key] = value; - } + if (!batch.WriteAddressReceiveRequest(dest, id, value)) return false; + m_address_book[dest].receive_requests[id] = value; + return true; +} + +bool CWallet::EraseAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id) +{ + if (!batch.EraseAddressReceiveRequest(dest, id)) return false; + m_address_book[dest].receive_requests.erase(id); return true; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 586f215499..bba7bf6d45 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -223,17 +223,22 @@ struct CAddressBookData std::optional purpose; /** - * Additional address metadata map that can currently hold two types of keys: - * - * "used" keys with values always set to "1" or "p" if present. This is set on - * IsMine addresses that have already been spent from if the - * avoid_reuse option is enabled - * - * "rr##" keys where ## is a decimal number, with serialized - * RecentRequestEntry objects as values + * Whether coins with this address have previously been spent. Set when the + * the wallet avoid_reuse option is enabled and this is an IsMine address + * that has already received funds and spent them. This is used during coin + * selection to increase privacy by not creating different transactions + * that spend from the same addresses. */ - typedef std::map StringMap; - StringMap destdata; + bool previously_spent{false}; + + /** + * Map containing data about previously generated receive requests + * requesting funds to be sent to this address. Only present for IsMine + * addresses. Map keys are decimal numbers uniquely identifying each + * request, and map values are serialized RecentRequestEntry objects + * containing BIP21 URI information including message and amount. + */ + std::map receive_requests{}; /** Accessor methods. */ bool IsChange() const { return !label.has_value(); } @@ -513,8 +518,10 @@ public: bool LoadMinVersion(int nVersion) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); nWalletVersion = nVersion; return true; } - //! Adds a destination data tuple to the store, without saving it to disk - void LoadDestData(const CTxDestination& dest, const std::string& key, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + //! Marks destination as previously spent. + void LoadAddressPreviouslySpent(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + //! Appends payment request to destination. + void LoadAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& request) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Holds a timestamp at which point the wallet is scheduled (externally) to be relocked. Caller must arrange for actual relocking to occur via Lock(). int64_t nRelockTime GUARDED_BY(cs_wallet){0}; @@ -741,11 +748,12 @@ public: bool DelAddressBook(const CTxDestination& address); - bool IsAddressUsed(const CTxDestination& dest) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool SetAddressUsed(WalletBatch& batch, const CTxDestination& dest, bool used) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + 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); 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); unsigned int GetKeyPoolSize() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index c8e8ce4614..005592d720 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -615,7 +615,20 @@ ReadKeyValue(CWallet* pwallet, DataStream& ssKey, CDataStream& ssValue, ssKey >> strAddress; ssKey >> strKey; ssValue >> strValue; - pwallet->LoadDestData(DecodeDestination(strAddress), strKey, strValue); + const CTxDestination& dest{DecodeDestination(strAddress)}; + if (strKey.compare("used") == 0) { + // Load "used" key indicating if an IsMine address has + // previously been spent from with avoid_reuse option enabled. + // The strValue is not used for anything currently, but could + // hold more information in the future. Current values are just + // "1" or "p" for present (which was written prior to + // f5ba424cd44619d9b9be88b8593d69a7ba96db26). + pwallet->LoadAddressPreviouslySpent(dest); + } else if (strKey.compare(0, 2, "rr") == 0) { + // Load "rr##" keys where ## is a decimal number, and strValue + // is a serialized RecentRequestEntry object. + pwallet->LoadAddressReceiveRequest(dest, strKey.substr(2), strValue); + } } else if (strType == DBKeys::HDCHAIN) { CHDChain chain; ssValue >> chain; @@ -1088,16 +1101,28 @@ void MaybeCompactWalletDB(WalletContext& context) fOneThread = false; } -bool WalletBatch::WriteDestData(const std::string &address, const std::string &key, const std::string &value) +bool WalletBatch::WriteAddressPreviouslySpent(const CTxDestination& dest, bool previously_spent) { - return WriteIC(std::make_pair(DBKeys::DESTDATA, std::make_pair(address, key)), value); + auto key{std::make_pair(DBKeys::DESTDATA, std::make_pair(EncodeDestination(dest), std::string("used")))}; + return previously_spent ? WriteIC(key, std::string("1")) : EraseIC(key); } -bool WalletBatch::EraseDestData(const std::string &address, const std::string &key) +bool WalletBatch::WriteAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& receive_request) { - return EraseIC(std::make_pair(DBKeys::DESTDATA, std::make_pair(address, key))); + return WriteIC(std::make_pair(DBKeys::DESTDATA, std::make_pair(EncodeDestination(dest), "rr" + id)), receive_request); } +bool WalletBatch::EraseAddressReceiveRequest(const CTxDestination& dest, const std::string& id) +{ + return EraseIC(std::make_pair(DBKeys::DESTDATA, std::make_pair(EncodeDestination(dest), "rr" + id))); +} + +bool WalletBatch::EraseAddressData(const CTxDestination& dest) +{ + DataStream prefix; + prefix << DBKeys::DESTDATA << EncodeDestination(dest); + return m_batch->ErasePrefix(prefix); +} bool WalletBatch::WriteHDChain(const CHDChain& chain) { diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index c97356a71f..72086e950a 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -7,6 +7,7 @@ #define BITCOIN_WALLET_WALLETDB_H #include