highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-03-20 04:30:31 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge #18192: Bugfix: Wallet: Safely deal with change in the address book

Browse Source 
b5795a7886 Wallet: Add warning comments and assert to CWallet::DelAddressBook (Luke Dashjr)
6d2905f57a Wallet: Avoid unnecessary/redundant m_address_book lookups (Luke Dashjr)
c751d886f4 Wallet: Avoid treating change-in-the-addressbook as non-change everywhere (Luke Dashjr)
8e64b8c84b Wallet: New FindAddressBookEntry method to filter out change entries (and skip ->second everywhere) (Luke Dashjr)
65b6bdc2b1 Wallet: Add CAddressBookData::IsChange which returns true iff label has never been set (Luke Dashjr)
144b2f85da Wallet: Require usage of new CAddressBookData::setLabel to change label (Luke Dashjr)
b86cd155f6 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 b5795a7886. Pretty clever and nicely implemented fix!
  jonatack:
    ACK b5795a7886 nice improvements -- code review, built/ran tests rebased on current master ff53433fe4 and tested manually with rpc/cli
  jnewbery:
    Good fix. utACK b5795a788.

Tree-SHA512: 40525185a0bcc1723f602243c269499ec86ecb298fecb5ef24d626bbdd5e3efece86cdb1084ad7eebf7eeaf251db4a6e056bcd25bc8457b417fcbb53d032ebf0
This commit is contained in:
MarcoFalke
2020-04-07 03:51:11 +08:00
parent c31bcaf203 b5795a7886
commit c5966a87d1
9 changed files with 93 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files

52
src/wallet/rpcwallet.cpp
View File

@@ -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);
}