Merge bitcoin/bitcoin#25504: RPC: allow to track coins by parent descriptors

achow101 · achow101 · commit c336f813b3cd · 2022-08-16T13:08:05.000-04:00
a6b0c1f doc: add releases notes for 25504 (listsinceblock updates) (Antoine Poinsot) 0fd2d14 rpc: add an include_change parameter to listsinceblock (Antoine Poinsot) 55f98d0 rpc: output parent wallet descriptors for coins in listunspent (Antoine Poinsot) b724476 rpc: output wallet descriptors for received entries in listsinceblock (Antoine Poinsot) 55a82ea wallet: allow to fetch the wallet descriptors for a given Script (Antoine Poinsot) Pull request description: Wallet descriptors are useful for applications using the Bitcoin Core wallet as a backend for tracking coins, as they allow to track coins for multiple descriptors in a single wallet. However there is no information currently given for such applications to link a coin with an imported descriptor, severely limiting the possibilities for such applications of using multiple descriptors in a single wallet. This PR outputs the matching imported descriptor(s) for a given received coin in `listsinceblock` (and friends). It comes from a need for an application i'm working on, but i think it's something any software using `bitcoind` to track multiple descriptors in a single wallet would have eventually. For instance i'm thinking about the BDK project. Currently, the way to achieve this is to import raw addresses with labels and to have your application be responsible for wallet things like the gap limit. I'll add this to the output of `listunspent` too if this gets a few Concept ACKs. ACKs for top commit: instagibbs: ACK bitcoin/bitcoin@a6b0c1f achow101: re-ACK a6b0c1f Tree-SHA512: 7a5850e8de98b439ddede2cb72de0208944f8cda67272e8b8037678738d55b7a5272375be808b0f7d15def4904430e089dafdcc037436858ff3292c5f8b75e37
diff --git a/doc/release-notes-25504.md b/doc/release-notes-25504.md
@@ -0,0 +1,6 @@
+Updated RPCs
+------------
+
+- The `listsinceblock`, `listtransactions` and `gettransaction` output now contain a new
+  `parent_descs` field for every "receive" entry.
+- A new optional `include_change` parameter was added to the `listsinceblock` command.
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
@@ -74,6 +74,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "listsinceblock", 1, "target_confirmations" },
     { "listsinceblock", 2, "include_watchonly" },
     { "listsinceblock", 3, "include_removed" },
+    { "listsinceblock", 4, "include_change" },
     { "sendmany", 1, "amounts" },
     { "sendmany", 2, "minconf" },
     { "sendmany", 4, "subtractfeefrom" },
diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp
@@ -193,7 +193,8 @@ CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx,
 
 void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,
                   std::list<COutputEntry>& listReceived,
-                  std::list<COutputEntry>& listSent, CAmount& nFee, const isminefilter& filter)
+                  std::list<COutputEntry>& listSent, CAmount& nFee, const isminefilter& filter,
+                  bool include_change)
 {
     nFee = 0;
     listReceived.clear();
@@ -218,8 +219,7 @@ void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,
         //   2) the output is to us (received)
         if (nDebit > 0)
         {
-            // Don't report 'change' txouts
-            if (OutputIsChange(wallet, txout))
+            if (!include_change && OutputIsChange(wallet, txout))
                 continue;
         }
         else if (!(fIsMine & filter))
diff --git a/src/wallet/receive.h b/src/wallet/receive.h
@@ -42,7 +42,8 @@ struct COutputEntry
 void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,
                         std::list<COutputEntry>& listReceived,
                         std::list<COutputEntry>& listSent,
-                        CAmount& nFee, const isminefilter& filter);
+                        CAmount& nFee, const isminefilter& filter,
+                        bool include_change);
 bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter);
 bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set<uint256>& trusted_parents) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
 bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx);
diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
@@ -543,6 +543,9 @@ RPCHelpMan listunspent()
                             {RPCResult::Type::BOOL, "solvable", "Whether we know how to spend this output, ignoring the lack of keys"},
                             {RPCResult::Type::BOOL, "reused", /*optional=*/true, "(only present if avoid_reuse is set) Whether this output is reused/dirty (sent to an address that was previously spent from)"},
                             {RPCResult::Type::STR, "desc", /*optional=*/true, "(only when solvable) A descriptor for spending this output"},
+                            {RPCResult::Type::ARR, "parent_descs", /*optional=*/false, "List of parent descriptors for the scriptPubKey of this coin.", {
+                                {RPCResult::Type::STR, "desc", "The descriptor string."},
+                            }},
                             {RPCResult::Type::BOOL, "safe", "Whether this output is considered safe to spend. Unconfirmed transactions\n"
                                                             "from outside keys and unconfirmed replacement transactions are considered unsafe\n"
                                                             "and are not eligible for spending by fundrawtransaction and sendtoaddress."},
@@ -722,6 +725,7 @@ RPCHelpMan listunspent()
                 entry.pushKV("desc", descriptor->ToString());
             }
         }
+        PushParentDescriptors(*pwallet, scriptPubKey, entry);
         if (avoid_reuse) entry.pushKV("reused", reused);
         entry.pushKV("safe", out.safe);
         results.push_back(entry);
diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp
@@ -315,13 +315,16 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest)
  * @param  filter_label   Optional label string to filter incoming transactions.
  */
 template <class Vec>
-static void ListTransactions(const CWallet& wallet, const CWalletTx& wtx, int nMinDepth, bool fLong, Vec& ret, const isminefilter& filter_ismine, const std::string* filter_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
+static void ListTransactions(const CWallet& wallet, const CWalletTx& wtx, int nMinDepth, bool fLong,
+                             Vec& ret, const isminefilter& filter_ismine, const std::string* filter_label,
+                             bool include_change = false)
+    EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
 {
     CAmount nFee;
     std::list<COutputEntry> listReceived;
     std::list<COutputEntry> listSent;
 
-    CachedTxGetAmounts(wallet, wtx, listReceived, listSent, nFee, filter_ismine);
+    CachedTxGetAmounts(wallet, wtx, listReceived, listSent, nFee, filter_ismine, include_change);
 
     bool involvesWatchonly = CachedTxIsFromMe(wallet, wtx, ISMINE_WATCH_ONLY);
 
@@ -367,6 +370,7 @@ static void ListTransactions(const CWallet& wallet, const CWalletTx& wtx, int nM
                 entry.pushKV("involvesWatchonly", true);
             }
             MaybePushAddress(entry, r.destination);
+            PushParentDescriptors(wallet, wtx.tx->vout.at(r.vout).scriptPubKey, entry);
             if (wtx.IsCoinBase())
             {
                 if (wallet.GetTxDepthInMainChain(wtx) < 1)
@@ -418,7 +422,11 @@ static const std::vector<RPCResult> TransactionDescriptionString()
            {RPCResult::Type::NUM_TIME, "timereceived", "The time received expressed in " + UNIX_EPOCH_TIME + "."},
            {RPCResult::Type::STR, "comment", /*optional=*/true, "If a comment is associated with the transaction, only present if not empty."},
            {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no|unknown\") Whether this transaction could be replaced due to BIP125 (replace-by-fee);\n"
-               "may be unknown for unconfirmed transactions not in the mempool."}};
+               "may be unknown for unconfirmed transactions not in the mempool."},
+           {RPCResult::Type::ARR, "parent_descs", /*optional=*/true, "Only if 'category' is 'received'. List of parent descriptors for the scriptPubKey of this coin.", {
+               {RPCResult::Type::STR, "desc", "The descriptor string."},
+           }},
+           };
 }
 
 RPCHelpMan listtransactions()
@@ -543,6 +551,7 @@ RPCHelpMan listsinceblock()
                     {"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Include transactions to watch-only addresses (see 'importaddress')"},
                     {"include_removed", RPCArg::Type::BOOL, RPCArg::Default{true}, "Show transactions that were removed due to a reorg in the \"removed\" array\n"
                                                                        "(not guaranteed to work on pruned nodes)"},
+                    {"include_change", RPCArg::Type::BOOL, RPCArg::Default{false}, "Also add entries for change outputs.\n"},
                 },
                 RPCResult{
                     RPCResult::Type::OBJ, "", "",
@@ -623,6 +632,7 @@ RPCHelpMan listsinceblock()
     }
 
     bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());
+    bool include_change = (!request.params[4].isNull() && request.params[4].get_bool());
 
     int depth = height ? wallet.GetLastBlockHeight() + 1 - *height : -1;
 
@@ -632,7 +642,7 @@ RPCHelpMan listsinceblock()
         const CWalletTx& tx = pairWtx.second;
 
         if (depth == -1 || abs(wallet.GetTxDepthInMainChain(tx)) < depth) {
-            ListTransactions(wallet, tx, 0, true, transactions, filter, nullptr /* filter_label */);
+            ListTransactions(wallet, tx, 0, true, transactions, filter, nullptr /* filter_label */, /*include_change=*/include_change);
         }
     }
 
@@ -649,7 +659,7 @@ RPCHelpMan listsinceblock()
             if (it != wallet.mapWallet.end()) {
                 // We want all transactions regardless of confirmation count to appear here,
                 // even negative confirmation ones, hence the big negative.
-                ListTransactions(wallet, it->second, -100000000, true, removed, filter, nullptr /* filter_label */);
+                ListTransactions(wallet, it->second, -100000000, true, removed, filter, nullptr /* filter_label */, /*include_change=*/include_change);
             }
         }
         blockId = block.hashPrevBlock;
@@ -709,6 +719,9 @@ RPCHelpMan gettransaction()
                                     "'send' category of transactions."},
                                 {RPCResult::Type::BOOL, "abandoned", /*optional=*/true, "'true' if the transaction has been abandoned (inputs are respendable). Only available for the \n"
                                      "'send' category of transactions."},
+                                {RPCResult::Type::ARR, "parent_descs", /*optional=*/true, "Only if 'category' is 'received'. List of parent descriptors for the scriptPubKey of this coin.", {
+                                    {RPCResult::Type::STR, "desc", "The descriptor string."},
+                                }},
                             }},
                         }},
                         {RPCResult::Type::STR_HEX, "hex", "Raw data for transaction"},
diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp
@@ -123,6 +123,15 @@ std::string LabelFromValue(const UniValue& value)
     return label;
 }
 
+void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey, UniValue& entry)
+{
+    UniValue parent_descs(UniValue::VARR);
+    for (const auto& desc: wallet.GetWalletDescriptors(script_pubkey)) {
+        parent_descs.push_back(desc.descriptor->ToString());
+    }
+    entry.pushKV("parent_descs", parent_descs);
+}
+
 void HandleWalletError(const std::shared_ptr<CWallet> wallet, DatabaseStatus& status, bilingual_str& error)
 {
     if (!wallet) {
diff --git a/src/wallet/rpc/util.h b/src/wallet/rpc/util.h
@@ -5,6 +5,8 @@
 #ifndef BITCOIN_WALLET_RPC_UTIL_H
 #define BITCOIN_WALLET_RPC_UTIL_H
 
+#include <script/script.h>
+
 #include <any>
 #include <memory>
 #include <string>
@@ -39,6 +41,8 @@ const LegacyScriptPubKeyMan& EnsureConstLegacyScriptPubKeyMan(const CWallet& wal
 bool GetAvoidReuseFlag(const CWallet& wallet, const UniValue& param);
 bool ParseIncludeWatchonly(const UniValue& include_watchonly, const CWallet& wallet);
 std::string LabelFromValue(const UniValue& value);
+//! Fetch parent descriptors of this scriptPubKey.
+void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey, UniValue& entry);
 
 void HandleWalletError(const std::shared_ptr<CWallet> wallet, DatabaseStatus& status, bilingual_str& error);
 } //  namespace wallet
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -3336,6 +3336,18 @@ std::unique_ptr<SigningProvider> CWallet::GetSolvingProvider(const CScript& scri
     return nullptr;
 }
 
+std::vector<WalletDescriptor> CWallet::GetWalletDescriptors(const CScript& script) const
+{
+    std::vector<WalletDescriptor> descs;
+    for (const auto spk_man: GetScriptPubKeyMans(script)) {
+        if (const auto desc_spk_man = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_man)) {
+            LOCK(desc_spk_man->cs_desc_man);
+            descs.push_back(desc_spk_man->GetWalletDescriptor());
+        }
+    }
+    return descs;
+}
+
 LegacyScriptPubKeyMan* CWallet::GetLegacyScriptPubKeyMan() const
 {
     if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
@@ -845,6 +845,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     std::unique_ptr<SigningProvider> GetSolvingProvider(const CScript& script) const;
     std::unique_ptr<SigningProvider> GetSolvingProvider(const CScript& script, SignatureData& sigdata) const;
 
+    //! Get the wallet descriptors for a script.
+    std::vector<WalletDescriptor> GetWalletDescriptors(const CScript& script) const;
+
     //! Get the LegacyScriptPubKeyMan which is used for all types, internal, and external.
     LegacyScriptPubKeyMan* GetLegacyScriptPubKeyMan() const;
     LegacyScriptPubKeyMan* GetOrCreateLegacyScriptPubKeyMan();
diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py
@@ -7,6 +7,7 @@
 from itertools import product
 
 from test_framework.blocktools import COINBASE_MATURITY
+from test_framework.descriptors import descsum_create
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_array_result,
@@ -700,6 +701,38 @@ def run_test(self):
         txid_feeReason_four = self.nodes[2].sendmany(dummy='', amounts={address: 5}, verbose=False)
         assert_equal(self.nodes[2].gettransaction(txid_feeReason_four)['txid'], txid_feeReason_four)
 
+        self.log.info("Testing 'listunspent' outputs the parent descriptor(s) of coins")
+        # Create two multisig descriptors, and send a UTxO each.
+        multi_a = descsum_create("wsh(multi(1,tpubD6NzVbkrYhZ4YBNjUo96Jxd1u4XKWgnoc7LsA1jz3Yc2NiDbhtfBhaBtemB73n9V5vtJHwU6FVXwggTbeoJWQ1rzdz8ysDuQkpnaHyvnvzR/*,tpubD6NzVbkrYhZ4YHdDGMAYGaWxMSC1B6tPRTHuU5t3BcfcS3nrF523iFm5waFd1pP3ZvJt4Jr8XmCmsTBNx5suhcSgtzpGjGMASR3tau1hJz4/*))")
+        multi_b = descsum_create("wsh(multi(1,tpubD6NzVbkrYhZ4YHdDGMAYGaWxMSC1B6tPRTHuU5t3BcfcS3nrF523iFm5waFd1pP3ZvJt4Jr8XmCmsTBNx5suhcSgtzpGjGMASR3tau1hJz4/*,tpubD6NzVbkrYhZ4Y2RLiuEzNQkntjmsLpPYDm3LTRBYynUQtDtpzeUKAcb9sYthSFL3YR74cdFgF5mW8yKxv2W2CWuZDFR2dUpE5PF9kbrVXNZ/*))")
+        addr_a = self.nodes[0].deriveaddresses(multi_a, 0)[0]
+        addr_b = self.nodes[0].deriveaddresses(multi_b, 0)[0]
+        txid_a = self.nodes[0].sendtoaddress(addr_a, 0.01)
+        txid_b = self.nodes[0].sendtoaddress(addr_b, 0.01)
+        self.generate(self.nodes[0], 1, sync_fun=self.no_op)
+        # Now import the descriptors, make sure we can identify on which descriptor each coin was received.
+        self.nodes[0].createwallet(wallet_name="wo", descriptors=True, disable_private_keys=True)
+        wo_wallet = self.nodes[0].get_wallet_rpc("wo")
+        wo_wallet.importdescriptors([
+            {
+                "desc": multi_a,
+                "active": False,
+                "timestamp": "now",
+            },
+            {
+                "desc": multi_b,
+                "active": False,
+                "timestamp": "now",
+            },
+        ])
+        coins = wo_wallet.listunspent(minconf=0)
+        assert_equal(len(coins), 2)
+        coin_a = next(c for c in coins if c["txid"] == txid_a)
+        assert_equal(coin_a["parent_descs"][0], multi_a)
+        coin_b = next(c for c in coins if c["txid"] == txid_b)
+        assert_equal(coin_b["parent_descs"][0], multi_b)
+        self.nodes[0].unloadwallet("wo")
+
 
 if __name__ == '__main__':
     WalletTest().main()
diff --git a/test/functional/wallet_listsinceblock.py b/test/functional/wallet_listsinceblock.py
@@ -6,6 +6,7 @@
 
 from test_framework.address import key_to_p2wpkh
 from test_framework.blocktools import COINBASE_MATURITY
+from test_framework.descriptors import descsum_create
 from test_framework.key import ECKey
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.messages import MAX_BIP125_RBF_SEQUENCE
@@ -39,6 +40,8 @@ def run_test(self):
         self.test_double_send()
         self.double_spends_filtered()
         self.test_targetconfirmations()
+        self.test_desc()
+        self.test_send_to_self()
 
     def test_no_blockhash(self):
         self.log.info("Test no blockhash")
@@ -383,5 +386,65 @@ def double_spends_filtered(self):
         assert_equal(original_found, False)
         assert_equal(double_found, False)
 
+    def test_desc(self):
+        """Make sure we can track coins by descriptor."""
+        self.log.info("Test descriptor lookup by scriptPubKey.")
+
+        # Create a watchonly wallet tracking two multisig descriptors.
+        multi_a = descsum_create("wsh(multi(1,tpubD6NzVbkrYhZ4YBNjUo96Jxd1u4XKWgnoc7LsA1jz3Yc2NiDbhtfBhaBtemB73n9V5vtJHwU6FVXwggTbeoJWQ1rzdz8ysDuQkpnaHyvnvzR/*,tpubD6NzVbkrYhZ4YHdDGMAYGaWxMSC1B6tPRTHuU5t3BcfcS3nrF523iFm5waFd1pP3ZvJt4Jr8XmCmsTBNx5suhcSgtzpGjGMASR3tau1hJz4/*))")
+        multi_b = descsum_create("wsh(multi(1,tpubD6NzVbkrYhZ4YHdDGMAYGaWxMSC1B6tPRTHuU5t3BcfcS3nrF523iFm5waFd1pP3ZvJt4Jr8XmCmsTBNx5suhcSgtzpGjGMASR3tau1hJz4/*,tpubD6NzVbkrYhZ4Y2RLiuEzNQkntjmsLpPYDm3LTRBYynUQtDtpzeUKAcb9sYthSFL3YR74cdFgF5mW8yKxv2W2CWuZDFR2dUpE5PF9kbrVXNZ/*))")
+        self.nodes[0].createwallet(wallet_name="wo", descriptors=True, disable_private_keys=True)
+        wo_wallet = self.nodes[0].get_wallet_rpc("wo")
+        wo_wallet.importdescriptors([
+            {
+                "desc": multi_a,
+                "active": False,
+                "timestamp": "now",
+            },
+            {
+                "desc": multi_b,
+                "active": False,
+                "timestamp": "now",
+            },
+        ])
+
+        # Send a coin to each descriptor.
+        assert_equal(len(wo_wallet.listsinceblock()["transactions"]), 0)
+        addr_a = self.nodes[0].deriveaddresses(multi_a, 0)[0]
+        addr_b = self.nodes[0].deriveaddresses(multi_b, 0)[0]
+        self.nodes[2].sendtoaddress(addr_a, 1)
+        self.nodes[2].sendtoaddress(addr_b, 2)
+        self.generate(self.nodes[2], 1)
+
+        # We can identify on which descriptor each coin was received.
+        coins = wo_wallet.listsinceblock()["transactions"]
+        assert_equal(len(coins), 2)
+        coin_a = next(c for c in coins if c["amount"] == 1)
+        assert_equal(coin_a["parent_descs"][0], multi_a)
+        coin_b = next(c for c in coins if c["amount"] == 2)
+        assert_equal(coin_b["parent_descs"][0], multi_b)
+
+    def test_send_to_self(self):
+        """We can make listsinceblock output our change outputs."""
+        self.log.info("Test the inclusion of change outputs in the output.")
+
+        # Create a UTxO paying to one of our change addresses.
+        block_hash = self.nodes[2].getbestblockhash()
+        addr = self.nodes[2].getrawchangeaddress()
+        self.nodes[2].sendtoaddress(addr, 1)
+
+        # If we don't list change, we won't have an entry for it.
+        coins = self.nodes[2].listsinceblock(blockhash=block_hash)["transactions"]
+        assert not any(c["address"] == addr for c in coins)
+
+        # Now if we list change, we'll get both the send (to a change address) and
+        # the actual change.
+        res = self.nodes[2].listsinceblock(blockhash=block_hash, include_change=True)
+        coins = [entry for entry in res["transactions"] if entry["category"] == "receive"]
+        assert_equal(len(coins), 2)
+        assert any(c["address"] == addr for c in coins)
+        assert all(self.nodes[2].getaddressinfo(c["address"])["ischange"] for c in coins)
+
+
 if __name__ == '__main__':
     ListSinceBlockTest().main()

Original file line number	Diff line number	Diff line change
`@@ -193,7 +193,8 @@ CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx,`
`193`	`193`
`194`	`194`	`void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,`
`195`	`195`	`std::list<COutputEntry>& listReceived,`
`196`		`- std::list<COutputEntry>& listSent, CAmount& nFee, const isminefilter& filter)`
	`196`	`+ std::list<COutputEntry>& listSent, CAmount& nFee, const isminefilter& filter,`
	`197`	`+ bool include_change)`
`197`	`198`	`{`
`198`	`199`	`nFee = 0;`
`199`	`200`	`listReceived.clear();`
`@@ -218,8 +219,7 @@ void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,`
`218`	`219`	`// 2) the output is to us (received)`
`219`	`220`	`if (nDebit > 0)`
`220`	`221`	`{`
`221`		`- // Don't report 'change' txouts`
`222`		`- if (OutputIsChange(wallet, txout))`
	`222`	`+ if (!include_change && OutputIsChange(wallet, txout))`
`223`	`223`	`continue;`
`224`	`224`	`}`
`225`	`225`	`else if (!(fIsMine & filter))`