From 007f05493a24059431ef16b3d68ae86cf06dc38d Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 10 Feb 2022 19:26:04 -0700 Subject: [PATCH] Add change field to z_listreceivedbyaddress for transparent addrs. This updates the `IsChange` method to check the HD keypath associated with the key for whether the address was generated as internal or external. --- qa/rpc-tests/wallet_listreceived.py | 2 +- src/wallet/rpcwallet.cpp | 3 +- src/wallet/wallet.cpp | 48 +++++++++++++++++++---------- src/zcash/address/zip32.cpp | 10 ++++++ src/zcash/address/zip32.h | 5 +++ 5 files changed, 49 insertions(+), 19 deletions(-) diff --git a/qa/rpc-tests/wallet_listreceived.py b/qa/rpc-tests/wallet_listreceived.py index abc5e988f..aec604357 100755 --- a/qa/rpc-tests/wallet_listreceived.py +++ b/qa/rpc-tests/wallet_listreceived.py @@ -477,7 +477,7 @@ class ListReceivedTest (BitcoinTestFramework): 'txid': txid_taddr, 'amount': Decimal('0.2'), 'amountZat': 20000000, - 'change': None, + 'change': False, 'memo': None, 'confirmations': 0, } in outputs) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index c36370bed..50274f61a 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3443,7 +3443,7 @@ UniValue z_listreceivedbyaddress(const UniValue& params, bool fHelp) " \"jsindex\" (sprout) : n, (numeric) the joinsplit index\n" " \"jsoutindex\" (sprout) : n, (numeric) the output index of the joinsplit\n" " \"outindex\" (transparent, sapling, orchard) : n, (numeric) the output index for transparent and Sapling outputs, or the action index for Orchard\n" - " \"change\": true|false, (boolean) true if the address that received the note is also one of the sending addresses\n" + " \"change\": true|false, (boolean) true if the output was received to a change address\n" "}\n" "\nExamples:\n" + HelpExampleCli("z_listreceivedbyaddress", "\"ztfaW34Gj9FrnGUEf833ywDVL62NWXBM81u6EQnM6VR45eYnXhwztecW1SjxA7JrmAXKJhxhj3vDNEpVCQoSvVoSpmbhtjf\"") @@ -3498,6 +3498,7 @@ UniValue z_listreceivedbyaddress(const UniValue& params, bool fHelp) obj.pushKV("amountZat", txout.nValue); obj.pushKV("outindex", int(i)); obj.pushKV("confirmations", nDepth); + obj.pushKV("change", pwalletMain->IsChange(txout)); result.push_back(obj); } } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6fda85be3..39e6d0ffd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1,5 +1,6 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto // Copyright (c) 2009-2014 The Bitcoin Core developers +// Copyright (c) 2016-2022 The Zcash developers // Distributed under the MIT software license, see the accompanying // file COPYING or https://www.opensource.org/licenses/mit-license.php . @@ -3008,24 +3009,37 @@ CAmount CWallet::GetCredit(const CTxOut& txout, const isminefilter& filter) cons bool CWallet::IsChange(const CTxOut& txout) const { - // TODO: fix handling of 'change' outputs. The assumption is that any - // payment to a script that is ours, but is not in the address book - // is change. That assumption is likely to break when we implement multisignature - // wallets that return change back into a multi-signature-protected address; - // a better way of identifying which outputs are 'the send' and which are - // 'the change' will need to be implemented (maybe extend CWalletTx to remember - // which output, if any, was change). - if (::IsMine(*this, txout.scriptPubKey)) - { - CTxDestination address; - if (!ExtractDestination(txout.scriptPubKey, address)) - return true; + // Addresses must be ours to be change + if (!::IsMine(*this, txout.scriptPubKey)) + return false; - LOCK(cs_wallet); - if (!mapAddressBook.count(address)) - return true; - } - return false; + // Only p2pkh addresses are used for change + CTxDestination address; + if (!ExtractDestination(txout.scriptPubKey, address)) + return true; + + LOCK(cs_wallet); + // Any payment to a script that is in the address book is not change. + if (mapAddressBook.count(address)) + return false; + + // We look to key metadata to determine whether the address was generated + // using an internal key path. This could fail to identify some legacy + // change addresses as change outputs. + return std::visit(match { + [&](const CKeyID& key) { + auto keyMetaIt = mapKeyMetadata.find(key); + return + // If we don't have key metadata, it's a legacy address that is not + // in the address book, so we judge it to be change. + keyMetaIt == mapKeyMetadata.end() || + keyMetaIt->second.hdKeypath == "" || + // If we do have non-null key metadata, we inspect the metadata to + // make our determination + IsInternalKeyPath(44, BIP44CoinType(), keyMetaIt->second.hdKeypath); + }, + [&](const auto& other) { return false; } + }, address); } CAmount CWallet::GetChange(const CTxOut& txout) const diff --git a/src/zcash/address/zip32.cpp b/src/zcash/address/zip32.cpp index 700057d03..a2557d185 100644 --- a/src/zcash/address/zip32.cpp +++ b/src/zcash/address/zip32.cpp @@ -285,4 +285,14 @@ std::optional ParseHDKeypathAccount(uint32_t purpose, uint32_t co } } +bool IsInternalKeyPath(uint32_t purpose, uint32_t coinType, const std::string& keyPath) { + std::regex pattern("m/" + std::to_string(purpose) + "'/" + std::to_string(coinType) + "'/[0-9]+'/([01])/.*"); + std::smatch matches; + if (std::regex_match(keyPath, matches, pattern)) { + return stoul(matches[1]) == 1; + } else { + return false; + } +} + }; diff --git a/src/zcash/address/zip32.h b/src/zcash/address/zip32.h index f31a8ffb6..7bdc5b890 100644 --- a/src/zcash/address/zip32.h +++ b/src/zcash/address/zip32.h @@ -287,6 +287,11 @@ std::optional ParseHDKeypathAccount( uint32_t coinType, const std::string& keyPath); +bool IsInternalKeyPath( + uint32_t purpose, + uint32_t coinType, + const std::string& keyPath); + } //namespace libzcash #endif // ZCASH_ZCASH_ADDRESS_ZIP32_H