aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMarcoFalke <falke.marco@gmail.com>2020-04-07 18:47:28 +0800
committerMarcoFalke <falke.marco@gmail.com>2020-04-07 18:47:38 +0800
commit63dad673487d3623f9076721d2a1924263272eb9 (patch)
tree5840b35c0d1f2ca5755f34e1485057a7bb0ee570
parentd12568e2f8863e76d82de3c6bd933e9f5c20daaf (diff)
parent7a2ecf16df938dd95d3130a46082def7a02338eb (diff)
downloadbitcoin-63dad673487d3623f9076721d2a1924263272eb9.tar.xz
Merge #18546: Bugfix: Wallet: Safely deal with change in the address book [part 2]
7a2ecf16df938dd95d3130a46082def7a02338eb Wallet: Change IsMine check in CWallet::DelAddressBook from assert to failure (Luke Dashjr) 2952c46b923042f2de801f319e03ed5c4c4eb735 Wallet: Replace CAddressBookData.name with GetLabel() method (Luke Dashjr) d7092c392e10889cd7a080b3d22ed6446a59b87a QA: Test that change doesn't turn into non-change when spent in an avoid-reuse wallet (Luke Dashjr) Pull request description: Follow-up to #18192, not strictly necessary for 0.20 ACKs for top commit: MarcoFalke: re-ACK 7a2ecf16df, only change is adding an assert_equal in the test 🔰 jnewbery: utACK 7a2ecf16df938dd95d3130a46082def7a02338eb Tree-SHA512: e0933ee40f705b751697dc27249e1868ed4874254b174ebdd0a7150125d8c818402e66df2371718c7eeb90e67ee2317215fb260aa9b9d7b9b45ee436de2988ff
-rw-r--r--src/interfaces/wallet.cpp4
-rw-r--r--src/wallet/rpcdump.cpp2
-rw-r--r--src/wallet/rpcwallet.cpp20
-rw-r--r--src/wallet/wallet.cpp7
-rw-r--r--src/wallet/wallet.h4
-rwxr-xr-xtest/functional/wallet_avoidreuse.py25
6 files changed, 45 insertions, 17 deletions
diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp
index d55b92a5ff..abce09ca4a 100644
--- a/src/interfaces/wallet.cpp
+++ b/src/interfaces/wallet.cpp
@@ -156,7 +156,7 @@ public:
return false;
}
if (name) {
- *name = it->second.name;
+ *name = it->second.GetLabel();
}
if (is_mine) {
*is_mine = m_wallet->IsMine(dest);
@@ -172,7 +172,7 @@ public:
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.name, item.second.purpose);
+ result.emplace_back(item.first, m_wallet->IsMine(item.first), item.second.GetLabel(), item.second.purpose);
}
return result;
}
diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
index e1d8f51c4a..ea54027c48 100644
--- a/src/wallet/rpcdump.cpp
+++ b/src/wallet/rpcdump.cpp
@@ -66,7 +66,7 @@ static bool GetWalletAddressesForKey(LegacyScriptPubKeyMan* spk_man, const CWall
strAddr += ",";
}
strAddr += EncodeDestination(dest);
- strLabel = EncodeDumpString(address_book_entry->name);
+ strLabel = EncodeDumpString(address_book_entry->GetLabel());
fLabelFound = true;
}
}
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 336a6cff72..769a343365 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -501,7 +501,7 @@ static UniValue listaddressgroupings(const JSONRPCRequest& request)
{
const auto* address_book_entry = pwallet->FindAddressBookEntry(address);
if (address_book_entry) {
- addressInfo.push_back(address_book_entry->name);
+ addressInfo.push_back(address_book_entry->GetLabel());
}
}
jsonGrouping.push_back(addressInfo);
@@ -1109,7 +1109,7 @@ static UniValue ListReceived(interfaces::Chain::Lock& locked_chain, const CWalle
{
if (item_it->second.IsChange()) continue;
const CTxDestination& address = item_it->first;
- const std::string& label = item_it->second.name;
+ const std::string& label = item_it->second.GetLabel();
auto it = mapTally.find(address);
if (it == mapTally.end() && !fIncludeEmpty)
continue;
@@ -1311,7 +1311,7 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle
entry.pushKV("amount", ValueFromAmount(-s.amount));
const auto* address_book_entry = pwallet->FindAddressBookEntry(s.destination);
if (address_book_entry) {
- entry.pushKV("label", address_book_entry->name);
+ entry.pushKV("label", address_book_entry->GetLabel());
}
entry.pushKV("vout", s.vout);
entry.pushKV("fee", ValueFromAmount(-nFee));
@@ -1329,7 +1329,7 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle
std::string label;
const auto* address_book_entry = pwallet->FindAddressBookEntry(r.destination);
if (address_book_entry) {
- label = address_book_entry->name;
+ label = address_book_entry->GetLabel();
}
if (filter_label && label != *filter_label) {
continue;
@@ -2963,7 +2963,7 @@ static UniValue listunspent(const JSONRPCRequest& request)
const auto* address_book_entry = pwallet->FindAddressBookEntry(address);
if (address_book_entry) {
- entry.pushKV("label", address_book_entry->name);
+ entry.pushKV("label", address_book_entry->GetLabel());
}
std::unique_ptr<SigningProvider> provider = pwallet->GetSolvingProvider(scriptPubKey);
@@ -3710,7 +3710,7 @@ static UniValue AddressBookDataToJSON(const CAddressBookData& data, const bool v
{
UniValue ret(UniValue::VOBJ);
if (verbose) {
- ret.pushKV("name", data.name);
+ ret.pushKV("name", data.GetLabel());
}
ret.pushKV("purpose", data.purpose);
return ret;
@@ -3822,7 +3822,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
// value of the name key/value pair in the labels array below.
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("label", address_book_entry->GetLabel());
}
ret.pushKV("ischange", pwallet->IsChange(scriptPubKey));
@@ -3851,7 +3851,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
if (pwallet->chain().rpcEnableDeprecated("labelspurpose")) {
labels.push_back(AddressBookDataToJSON(*address_book_entry, true));
} else {
- labels.push_back(address_book_entry->name);
+ labels.push_back(address_book_entry->GetLabel());
}
}
ret.pushKV("labels", std::move(labels));
@@ -3897,7 +3897,7 @@ static UniValue getaddressesbylabel(const JSONRPCRequest& request)
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.name == label) {
+ if (item.second.GetLabel() == label) {
std::string address = EncodeDestination(item.first);
// CWallet::m_address_book is not expected to contain duplicate
// address strings, but build a separate set as a precaution just in
@@ -3963,7 +3963,7 @@ static UniValue listlabels(const JSONRPCRequest& request)
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);
+ label_set.insert(entry.second.GetLabel());
}
}
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 9ee3bbd038..45f5542cad 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3221,7 +3221,10 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
// 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?).
- assert(!IsMine(address));
+ if (IsMine(address)) {
+ WalletLogPrintf("%s called with IsMine address, NOT SUPPORTED. Please report this bug! %s\n", __func__, PACKAGE_BUGREPORT);
+ return false;
+ }
{
LOCK(cs_wallet);
@@ -3472,7 +3475,7 @@ std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) co
{
if (item.second.IsChange()) continue;
const CTxDestination& address = item.first;
- const std::string& strName = item.second.name;
+ const std::string& strName = item.second.GetLabel();
if (strName == label)
result.insert(address);
}
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 7e770a40f2..6c54c72e76 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -185,15 +185,15 @@ private:
bool m_change{true};
std::string m_label;
public:
- const std::string& name;
std::string purpose;
- CAddressBookData() : name(m_label), purpose("unknown") {}
+ CAddressBookData() : purpose("unknown") {}
typedef std::map<std::string, std::string> StringMap;
StringMap destdata;
bool IsChange() const { return m_change; }
+ const std::string& GetLabel() const { return m_label; }
void SetLabel(const std::string& label) {
m_change = false;
m_label = label;
diff --git a/test/functional/wallet_avoidreuse.py b/test/functional/wallet_avoidreuse.py
index 8e2dc03ac2..2ce8d459c6 100755
--- a/test/functional/wallet_avoidreuse.py
+++ b/test/functional/wallet_avoidreuse.py
@@ -83,6 +83,7 @@ class AvoidReuseTest(BitcoinTestFramework):
self.nodes[0].generate(110)
self.sync_all()
+ self.test_change_remains_change(self.nodes[1])
reset_balance(self.nodes[1], self.nodes[0].getnewaddress())
self.test_fund_send_fund_senddirty()
reset_balance(self.nodes[1], self.nodes[0].getnewaddress())
@@ -137,6 +138,30 @@ class AvoidReuseTest(BitcoinTestFramework):
# Unload temp wallet
self.nodes[1].unloadwallet(tempwallet)
+ def test_change_remains_change(self, node):
+ self.log.info("Test that change doesn't turn into non-change when spent")
+
+ reset_balance(node, node.getnewaddress())
+ addr = node.getnewaddress()
+ txid = node.sendtoaddress(addr, 1)
+ out = node.listunspent(minconf=0, query_options={'minimumAmount': 2})
+ assert_equal(len(out), 1)
+ assert_equal(out[0]['txid'], txid)
+ changeaddr = out[0]['address']
+
+ # Make sure it's starting out as change as expected
+ assert node.getaddressinfo(changeaddr)['ischange']
+ for logical_tx in node.listtransactions():
+ assert logical_tx.get('address') != changeaddr
+
+ # Spend it
+ reset_balance(node, node.getnewaddress())
+
+ # It should still be change
+ assert node.getaddressinfo(changeaddr)['ischange']
+ for logical_tx in node.listtransactions():
+ assert logical_tx.get('address') != changeaddr
+
def test_fund_send_fund_senddirty(self):
'''
Test the same as test_fund_send_fund_send, except send the 10 BTC with