diff options
author | MarcoFalke <falke.marco@gmail.com> | 2020-04-07 03:51:11 +0800 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2020-04-07 03:51:18 +0800 |
commit | c5966a87d1fdd7a98f2baee5b2deddd541fdfb5a (patch) | |
tree | 54b87c316546ff8808c537722effd00110ec1ab6 /src/wallet/rpcwallet.cpp | |
parent | c31bcaf203b5154117eee1cb2496dca2b7c3853d (diff) | |
parent | b5795a788639305bab86a8b3f6b75d6ce81be083 (diff) | |
download | bitcoin-c5966a87d1fdd7a98f2baee5b2deddd541fdfb5a.tar.xz |
Merge #18192: Bugfix: Wallet: Safely deal with change in the address book
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`.
ACKs for top commit:
ryanofsky:
Code review ACK b5795a788639305bab86a8b3f6b75d6ce81be083. Pretty clever and nicely implemented fix!
jonatack:
ACK b5795a788639305bab86a8b3f6b75d6ce81be083 nice improvements -- code review, built/ran tests rebased on current master ff53433fe4ed06893d7c4 and tested manually with rpc/cli
jnewbery:
Good fix. utACK b5795a788.
Tree-SHA512: 40525185a0bcc1723f602243c269499ec86ecb298fecb5ef24d626bbdd5e3efece86cdb1084ad7eebf7eeaf251db4a6e056bcd25bc8457b417fcbb53d032ebf0
Diffstat (limited to 'src/wallet/rpcwallet.cpp')
-rw-r--r-- | src/wallet/rpcwallet.cpp | 52 |
1 files changed, 29 insertions, 23 deletions
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index ea1d937849..336a6cff72 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -499,8 +499,9 @@ static UniValue listaddressgroupings(const JSONRPCRequest& request) addressInfo.push_back(EncodeDestination(address)); 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); @@ -1092,13 +1093,13 @@ static UniValue ListReceived(interfaces::Chain::Lock& locked_chain, const CWalle 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); } @@ -1106,6 +1107,7 @@ static UniValue ListReceived(interfaces::Chain::Lock& locked_chain, const CWalle 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; auto it = mapTally.find(address); @@ -1307,8 +1309,9 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle 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.at(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(-nFee)); @@ -1324,8 +1327,9 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle for (const COutputEntry& r : listReceived) { std::string label; - if (pwallet->mapAddressBook.count(r.destination)) { - label = pwallet->mapAddressBook.at(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; @@ -1349,7 +1353,7 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle 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); @@ -2957,9 +2961,9 @@ static UniValue listunspent(const JSONRPCRequest& request) if (fValidAddress) { entry.pushKV("address", EncodeDestination(address)); - 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); } std::unique_ptr<SigningProvider> provider = pwallet->GetSolvingProvider(scriptPubKey); @@ -3816,8 +3820,9 @@ UniValue getaddressinfo(const JSONRPCRequest& request) // DEPRECATED: 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->chain().rpcEnableDeprecated("label")) && (pwallet->mapAddressBook.count(dest))) { - ret.pushKV("label", pwallet->mapAddressBook.at(dest).name); + const auto* address_book_entry = pwallet->FindAddressBookEntry(dest); + if (pwallet->chain().rpcEnableDeprecated("label") && address_book_entry) { + ret.pushKV("label", address_book_entry->name); } ret.pushKV("ischange", pwallet->IsChange(scriptPubKey)); @@ -3840,14 +3845,13 @@ UniValue getaddressinfo(const JSONRPCRequest& request) // stable if we allow multiple labels to be associated with an address in // the future. UniValue labels(UniValue::VARR); - std::map<CTxDestination, CAddressBookData>::const_iterator mi = pwallet->mapAddressBook.find(dest); - if (mi != pwallet->mapAddressBook.end()) { + if (address_book_entry) { // DEPRECATED: The previous behavior of returning an array containing a // JSON object of `name` and `purpose` key/value pairs is deprecated. 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)); @@ -3891,10 +3895,11 @@ static UniValue getaddressesbylabel(const JSONRPCRequest& request) // 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->mapAddressBook) { + for (const std::pair<const CTxDestination, CAddressBookData>& item : pwallet->m_address_book) { + if (item.second.IsChange()) continue; if (item.second.name == label) { std::string address = EncodeDestination(item.first); - // 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; @@ -3955,7 +3960,8 @@ static UniValue listlabels(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) { + 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.name); } |