Skip to content

Commit

Permalink
Merge #18192: Bugfix: Wallet: Safely deal with change in the address …
Browse files Browse the repository at this point in the history 
…book

Summary:
b5795a788639305bab86a8b3f6b75d6ce81be083 Wallet: Add warning comments and assert to CWallet::DelAddressBook (Luke Dashjr)
6d2905f57aaeb3ec3b63d31043f7673ca10003f2 Wallet: Avoid unnecessary/redundant m_address_book lookups (Luke Dashjr)
c751d886f499257627b308b11ffaa51c22db6cc0 Wallet: Avoid treating change-in-the-addressbook as non-change everywhere (Luke Dashjr)
8e64b8c84bcbd63caea06f3af087af1f0609eaf5 Wallet: New FindAddressBookEntry method to filter out change entries (and skip ->second everywhere) (Luke Dashjr)
65b6bdc2b164343ec3cc3d32a0297daff9e24fec Wallet: Add CAddressBookData::IsChange which returns true iff label has never been set (Luke Dashjr)
144b2f85da4d51bf7d72b987888ddcaf5b429eed Wallet: Require usage of new CAddressBookData::setLabel to change label (Luke Dashjr)
b86cd155f6f661052042048aa7cfc2a397afe4f7 scripted-diff: Wallet: Rename mapAddressBook to m_address_book (Luke Dashjr)

Pull request description:
  In many places, our code assumes that presence in the address book indicates a non-change key, and absence of an entry in mapAddressBook indicates change.

  This no longer holds true after #13756 (first released in 0.19) since it added a "used" DestData populated even for change addresses. Only avoid-reuse wallets should be affected by this issue.

  Thankfully, populating DestData does not write a label to the database, so we can retroactively fix this (so long as the user didn't see the change address and manually assign it a real label).

  Fixing it is accomplished by:

  * Adding a new bool to CAddressBookData to track if the label has ever been assigned, either by loading one from the database, or by assigning one at runtime.
  * `CAddressBookData::IsChange` and `CWallet::FindAddressBookEntry` are new methods to assist in excluding change from code that doesn't expect to see them.
  * For safety in merging, `CAddressBookData::name` has been made read-only (the actual data is stored in `m_label`, a new private member, and can be changed only with `setLabel` which updates the `m_change` flag), and `mapAddressBook` has been renamed to `m_address_book` (to force old code to be rebased to compile).

  A final commit also does some minor optimisation, avoiding redundant lookups in `m_address_book` when we already have a pointer to the `CAddressBookData`.

Backport of Core [[bitcoin/bitcoin#18192 | PR18192]]

Tests for this change to follow in [[bitcoin/bitcoin#18546 | PR18546]]

Test Plan: `ninja check check-functional` for sanity

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7810
  • Loading branch information
@jasonbcox
MarcoFalke authored and jasonbcox committed Oct 8, 2020
1 parent 2179ddf commit 082ae84
Show file tree
Hide file tree
Showing 10 changed files with 134 additions and 74 deletions.
9 changes: 6 additions & 3 deletions src/interfaces/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,8 @@ namespace {
bool getAddress(const CTxDestination &dest, std::string *name,
isminetype *is_mine, std::string *purpose) override {
LOCK(m_wallet->cs_wallet);
auto it = m_wallet->mapAddressBook.find(dest);
if (it == m_wallet->mapAddressBook.end()) {
auto it = m_wallet->m_address_book.find(dest);
if (it == m_wallet->m_address_book.end() || it->second.IsChange()) {
return false;
}
if (name) {
Expand All @@ -184,7 +184,10 @@ namespace {
std::vector<WalletAddress> getAddresses() override {
LOCK(m_wallet->cs_wallet);
std::vector<WalletAddress> result;
for (const auto &item : m_wallet->mapAddressBook) {
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.name, item.second.purpose);
}
Expand Down
4 changes: 2 additions & 2 deletions src/qt/addresstablemodel.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ struct AddressTableEntryLessThan {
static AddressTableEntry::Type
translateTransactionType(const QString &strPurpose, bool isMine) {
AddressTableEntry::Type addressType = AddressTableEntry::Hidden;
// "refund" addresses aren't shown, and change addresses aren't in
// mapAddressBook at all.
// "refund" addresses aren't shown, and change addresses aren't returned by
// getAddresses at all.
if (strPurpose == "send") {
addressType = AddressTableEntry::Sending;
} else if (strPurpose == "receive") {
Expand Down
2 changes: 1 addition & 1 deletion src/qt/test/addressbooktests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ void TestAddAddressesToSendBook(interfaces::Node &node) {

auto check_addbook_size = [&wallet](int expected_size) {
LOCK(wallet->cs_wallet);
QCOMPARE(static_cast<int>(wallet->mapAddressBook.size()),
QCOMPARE(static_cast<int>(wallet->m_address_book.size()),
expected_size);
};

Expand Down
7 changes: 4 additions & 3 deletions src/wallet/rpcdump.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,13 @@ GetWalletAddressesForKey(const Config &config, LegacyScriptPubKeyMan *spk_man,
CKey key;
spk_man->GetKey(keyid, key);
for (const auto &dest : GetAllDestinationsForKey(key.GetPubKey())) {
if (pwallet->mapAddressBook.count(dest)) {
const auto *address_book_entry = pwallet->FindAddressBookEntry(dest);
if (address_book_entry) {
if (!strAddr.empty()) {
strAddr += ",";
}
strAddr += EncodeDestination(dest, config);
strLabel = EncodeDumpString(pwallet->mapAddressBook[dest].name);
strLabel = EncodeDumpString(address_book_entry->name);
fLabelFound = true;
}
}
Expand Down Expand Up @@ -194,7 +195,7 @@ UniValue importprivkey(const Config &config, const JSONRPCRequest &request) {
// label was passed.
for (const auto &dest : GetAllDestinationsForKey(pubkey)) {
if (!request.params[1].isNull() ||
pwallet->mapAddressBook.count(dest) == 0) {
!pwallet->FindAddressBookEntry(dest)) {
pwallet->SetAddressBook(dest, strLabel, "receive");
}
}
Expand Down
66 changes: 39 additions & 27 deletions src/wallet/rpcwallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -551,10 +551,10 @@ static UniValue listaddressgroupings(const Config &config,
addressInfo.push_back(EncodeDestination(address, config));
addressInfo.push_back(ValueFromAmount(balances[address]));

if (pwallet->mapAddressBook.find(address) !=
pwallet->mapAddressBook.end()) {
addressInfo.push_back(
pwallet->mapAddressBook.find(address)->second.name);
const auto *address_book_entry =
pwallet->FindAddressBookEntry(address);
if (address_book_entry) {
addressInfo.push_back(address_book_entry->name);
}
jsonGrouping.push_back(addressInfo);
}
Expand Down Expand Up @@ -1256,19 +1256,22 @@ ListReceived(const Config &config, interfaces::Chain::Lock &locked_chain,
UniValue ret(UniValue::VARR);
std::map<std::string, tallyitem> label_tally;

// Create mapAddressBook iterator
// Create m_address_book iterator
// If we aren't filtering, go from begin() to end()
auto start = pwallet->mapAddressBook.begin();
auto end = pwallet->mapAddressBook.end();
auto start = pwallet->m_address_book.begin();
auto end = pwallet->m_address_book.end();
// If we are filtering, find() the applicable entry
if (has_filtered_address) {
start = pwallet->mapAddressBook.find(filtered_address);
start = pwallet->m_address_book.find(filtered_address);
if (start != end) {
end = std::next(start);
}
}

for (auto item_it = start; item_it != end; ++item_it) {
if (item_it->second.IsChange()) {
continue;
}
const CTxDestination &address = item_it->first;
const std::string &label = item_it->second.name;
std::map<CTxDestination, tallyitem>::iterator it =
Expand Down Expand Up @@ -1494,9 +1497,10 @@ static void ListTransactions(interfaces::Chain::Lock &locked_chain,
MaybePushAddress(entry, s.destination);
entry.pushKV("category", "send");
entry.pushKV("amount", ValueFromAmount(-s.amount));
if (pwallet->mapAddressBook.count(s.destination)) {
entry.pushKV("label",
pwallet->mapAddressBook[s.destination].name);
const auto *address_book_entry =
pwallet->FindAddressBookEntry(s.destination);
if (address_book_entry) {
entry.pushKV("label", address_book_entry->name);
}
entry.pushKV("vout", s.vout);
entry.pushKV("fee", ValueFromAmount(-1 * nFee));
Expand All @@ -1512,8 +1516,10 @@ static void ListTransactions(interfaces::Chain::Lock &locked_chain,
if (listReceived.size() > 0 && wtx.GetDepthInMainChain() >= nMinDepth) {
for (const COutputEntry &r : listReceived) {
std::string label;
if (pwallet->mapAddressBook.count(r.destination)) {
label = pwallet->mapAddressBook[r.destination].name;
const auto *address_book_entry =
pwallet->FindAddressBookEntry(r.destination);
if (address_book_entry) {
label = address_book_entry->name;
}
if (filter_label && label != *filter_label) {
continue;
Expand All @@ -1536,7 +1542,7 @@ static void ListTransactions(interfaces::Chain::Lock &locked_chain,
entry.pushKV("category", "receive");
}
entry.pushKV("amount", ValueFromAmount(r.amount));
if (pwallet->mapAddressBook.count(r.destination)) {
if (address_book_entry) {
entry.pushKV("label", label);
}
entry.pushKV("vout", r.vout);
Expand Down Expand Up @@ -3527,9 +3533,10 @@ static UniValue listunspent(const Config &config,
if (fValidAddress) {
entry.pushKV("address", EncodeDestination(address, config));

auto i = pwallet->mapAddressBook.find(address);
if (i != pwallet->mapAddressBook.end()) {
entry.pushKV("label", i->second.name);
const auto *address_book_entry =
pwallet->FindAddressBookEntry(address);
if (address_book_entry) {
entry.pushKV("label", address_book_entry->name);
}

const SigningProvider *provider =
Expand Down Expand Up @@ -4287,8 +4294,9 @@ UniValue getaddressinfo(const Config &config, const JSONRPCRequest &request) {
// Return label field if existing. Currently only one label can be
// associated with an address, so the label should be equivalent to the
// value of the name key/value pair in the labels array below.
if (pwallet->mapAddressBook.count(dest)) {
ret.pushKV("label", pwallet->mapAddressBook[dest].name);
const auto *address_book_entry = pwallet->FindAddressBookEntry(dest);
if (address_book_entry) {
ret.pushKV("label", address_book_entry->name);
}

ret.pushKV("ischange", pwallet->IsChange(scriptPubKey));
Expand Down Expand Up @@ -4316,13 +4324,11 @@ UniValue getaddressinfo(const Config &config, const JSONRPCRequest &request) {
// DEPRECATED: The previous behavior of returning an array containing a JSON
// object of `name` and `purpose` key/value pairs has been deprecated.
UniValue labels(UniValue::VARR);
std::map<CTxDestination, CAddressBookData>::iterator mi =
pwallet->mapAddressBook.find(dest);
if (mi != pwallet->mapAddressBook.end()) {
if (address_book_entry) {
if (pwallet->chain().rpcEnableDeprecated("labelspurpose")) {
labels.push_back(AddressBookDataToJSON(mi->second, true));
labels.push_back(AddressBookDataToJSON(*address_book_entry, true));
} else {
labels.push_back(mi->second.name);
labels.push_back(address_book_entry->name);
}
}
ret.pushKV("labels", std::move(labels));
Expand Down Expand Up @@ -4365,10 +4371,13 @@ UniValue getaddressesbylabel(const Config &config,
UniValue ret(UniValue::VOBJ);
std::set<std::string> addresses;
for (const std::pair<const CTxDestination, CAddressBookData> &item :
pwallet->mapAddressBook) {
pwallet->m_address_book) {
if (item.second.IsChange()) {
continue;
}
if (item.second.name == label) {
std::string address = EncodeDestination(item.first, config);
// CWallet::mapAddressBook is not expected to contain duplicate
// 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.
bool unique = addresses.emplace(address).second;
Expand Down Expand Up @@ -4429,7 +4438,10 @@ UniValue listlabels(const Config &config, const JSONRPCRequest &request) {
// 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->mapAddressBook) {
pwallet->m_address_book) {
if (entry.second.IsChange()) {
continue;
}
if (purpose.empty() || entry.second.purpose == purpose) {
label_set.insert(entry.second.name);
}
Expand Down
20 changes: 10 additions & 10 deletions src/wallet/test/walletdb_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,18 @@ BOOST_AUTO_TEST_CASE(write_erase_name) {
{
auto w = LoadWallet(batch);
LOCK(w->cs_wallet);
BOOST_CHECK_EQUAL(1, w->mapAddressBook.count(dst1));
BOOST_CHECK_EQUAL("name1", w->mapAddressBook[dst1].name);
BOOST_CHECK_EQUAL("name2", w->mapAddressBook[dst2].name);
BOOST_CHECK_EQUAL(1, w->m_address_book.count(dst1));
BOOST_CHECK_EQUAL("name1", w->m_address_book[dst1].name);
BOOST_CHECK_EQUAL("name2", w->m_address_book[dst2].name);
}

batch.EraseName(dst1);

{
auto w = LoadWallet(batch);
LOCK(w->cs_wallet);
BOOST_CHECK_EQUAL(0, w->mapAddressBook.count(dst1));
BOOST_CHECK_EQUAL(1, w->mapAddressBook.count(dst2));
BOOST_CHECK_EQUAL(0, w->m_address_book.count(dst1));
BOOST_CHECK_EQUAL(1, w->m_address_book.count(dst2));
}
}

Expand All @@ -67,18 +67,18 @@ BOOST_AUTO_TEST_CASE(write_erase_purpose) {
{
auto w = LoadWallet(batch);
LOCK(w->cs_wallet);
BOOST_CHECK_EQUAL(1, w->mapAddressBook.count(dst1));
BOOST_CHECK_EQUAL("purpose1", w->mapAddressBook[dst1].purpose);
BOOST_CHECK_EQUAL("purpose2", w->mapAddressBook[dst2].purpose);
BOOST_CHECK_EQUAL(1, w->m_address_book.count(dst1));
BOOST_CHECK_EQUAL("purpose1", w->m_address_book[dst1].purpose);
BOOST_CHECK_EQUAL("purpose2", w->m_address_book[dst2].purpose);
}

batch.ErasePurpose(dst1);

{
auto w = LoadWallet(batch);
LOCK(w->cs_wallet);
BOOST_CHECK_EQUAL(0, w->mapAddressBook.count(dst1));
BOOST_CHECK_EQUAL(1, w->mapAddressBook.count(dst2));
BOOST_CHECK_EQUAL(0, w->m_address_book.count(dst1));
BOOST_CHECK_EQUAL(1, w->m_address_book.count(dst2));
}
}

Expand Down
Loading

0 comments on commit 082ae84

Please sign in to comment.