RPC: allow to track coins by parent descriptors

darosior commented at 5:26 pm on June 29, 2022: member

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.

DrahtBot added the label RPC/REST/ZMQ on Jun 29, 2022

DrahtBot added the label Wallet on Jun 29, 2022

in src/wallet/wallet.cpp:3320 in a3f89dfe58 outdated

3248@@ -3249,6 +3249,17 @@ std::unique_ptr<SigningProvider> CWallet::GetSolvingProvider(const CScript& scri
3249     return nullptr;
3250 }
3251 
3252+std::vector<WalletDescriptor> CWallet::GetWalletDescriptors(const CScript& script) const
3253+{
3254+    std::vector<WalletDescriptor> descs;
3255+    for (const auto spk_man: GetScriptPubKeyMans(script)) {
3256+        if (const auto desc_spk_man = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_man)) {
3257+            descs.push_back(desc_spk_man->GetWalletDescriptor());

jonatack commented at 6:12 pm on June 29, 2022:

0wallet/wallet.cpp:3286:43: error: calling function 'GetWalletDescriptor' requires holding mutex 'desc_spk_man->cs_desc_man' exclusively [-Werror,-Wthread-safety-analysis]
1            descs.push_back(desc_spk_man->GetWalletDescriptor());
2                                          ^
31 error generated.

w0xlt commented at 1:47 am on June 30, 2022:

Suggestion to avoid the error mentioned in https://github.com/bitcoin/bitcoin/pull/25504/files#r910264911.

0            LOCK(desc_spk_man->cs_desc_man);
1            descs.push_back(desc_spk_man->GetWalletDescriptor());

darosior commented at 8:45 am on June 30, 2022:

Thanks

in src/wallet/rpc/transactions.cpp:326 in a3f89dfe58 outdated

321@@ -322,6 +322,16 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest)
322     }
323 }
324 
325+//! Fetch wallet descriptors matching this scriptPubKey.
326+static void MaybePushWalletDescriptors(UniValue& entry, const CWallet& wallet, const CScript& script_pubkey)

jonatack commented at 6:17 pm on June 29, 2022:

Suggest ordering the params by in, then out (developer notes: “When ordering function parameters, place input parameters first, then any in-out parameters, followed by any output parameters.”)

in src/wallet/wallet.cpp:3318 in a3f89dfe58 outdated

3248@@ -3249,6 +3249,17 @@ std::unique_ptr<SigningProvider> CWallet::GetSolvingProvider(const CScript& scri
3249     return nullptr;
3250 }
3251 
3252+std::vector<WalletDescriptor> CWallet::GetWalletDescriptors(const CScript& script) const
3253+{
3254+    std::vector<WalletDescriptor> descs;
3255+    for (const auto spk_man: GetScriptPubKeyMans(script)) {
3256+        if (const auto desc_spk_man = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_man)) {

jonatack commented at 6:18 pm on June 29, 2022:

Can this cast be avoided, or a static cast used instead?

darosior commented at 8:48 am on June 30, 2022:

The point here is to filter only descriptor spkmans, which can only be done dynamically.

in src/wallet/receive.cpp:268 in a3f89dfe58 outdated

264@@ -265,7 +265,7 @@ void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,
265             address = CNoDestination();
266         }
267 
268-        COutputEntry output = {address, txout.nValue, (int)i};
269+        COutputEntry output = {address, std::move(txout.scriptPubKey), txout.nValue, (int)i};

jonatack commented at 6:27 pm on June 29, 2022:

Unsure here, is txout e.g. for txout.nValue in a valid state after this move?

darosior commented at 9:01 am on June 30, 2022:

Oh, yeah, i wanted to revisit this but forgot and pushed.. It’s invalid since the caller will use the scriptPubKey (for instance in WalletTxToJSON).

w0xlt approved

w0xlt commented at 1:47 am on June 30, 2022: contributor

Approach ACK

DrahtBot commented at 4:34 am on June 30, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25158 (rpc, wallet: add abandoned field for all categories of transaction in ListTransaction by brunoerg)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

darosior force-pushed on Jun 30, 2022

kristapsk commented at 11:02 am on June 30, 2022: contributor

Concept ACK. JoinMarket currently imports each address into watchonly manually and assigns same label (JM wallet id) to them. So this is needed for JoinMarket to support ranged descriptors in future instead.

achow101 commented at 4:07 pm on June 30, 2022: member

To be clear, this affects more than just imported descriptors. It outputs the parent descriptor for each scriptPubKey, and so it will as well for the automatically generated descriptors.

in src/wallet/rpc/transactions.cpp:326 in 8976cd8905 outdated

321@@ -322,6 +322,16 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest)
322     }
323 }
324 
325+//! Fetch wallet descriptors matching this scriptPubKey.
326+static void MaybePushWalletDescriptors(const CWallet& wallet, const CScript& script_pubkey, UniValue& entry)

achow101 commented at 4:14 pm on June 30, 2022:

In 8976cd89051d262bfed2498de5aa9ed190e3b331 “rpc: output wallet descriptors for received entries in listsinceblock”

nit: There’s no maybe here, it will always push wallet_descs.

in src/wallet/rpc/transactions.cpp:451 in 8976cd8905 outdated

446@@ -436,7 +447,11 @@ static const std::vector<RPCResult> TransactionDescriptionString()
447            {RPCResult::Type::NUM_TIME, "timereceived", "The time received expressed in " + UNIX_EPOCH_TIME + "."},
448            {RPCResult::Type::STR, "comment", /*optional=*/true, "If a comment is associated with the transaction, only present if not empty."},
449            {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no|unknown\") Whether this transaction could be replaced due to BIP125 (replace-by-fee);\n"
450-               "may be unknown for unconfirmed transactions not in the mempool."}};
451+               "may be unknown for unconfirmed transactions not in the mempool."},
452+           {RPCResult::Type::ARR, "wallet_descs", /*optional=*/true, "Only if 'category' is 'received'. List of imported descriptors matching the scriptPubKey of this coin.", {

achow101 commented at 4:56 pm on June 30, 2022:

In 8976cd89051d262bfed2498de5aa9ed190e3b331 “rpc: output wallet descriptors for received entries in listsinceblock”

Not just import descriptors

in src/wallet/rpc/transactions.cpp:746 in 8976cd8905 outdated

742@@ -728,6 +743,9 @@ RPCHelpMan gettransaction()
743                                     "'send' category of transactions."},
744                                 {RPCResult::Type::BOOL, "abandoned", /*optional=*/true, "'true' if the transaction has been abandoned (inputs are respendable). Only available for the \n"
745                                      "'send' category of transactions."},
746+                                {RPCResult::Type::ARR, "wallet_descs", /*optional=*/true, "Only if 'category' is 'received'. List of imported descriptors matching the scriptPubKey of this coin.", {

achow101 commented at 4:56 pm on June 30, 2022:

In 8976cd89051d262bfed2498de5aa9ed190e3b331 “rpc: output wallet descriptors for received entries in listsinceblock”

Not just imported descriptors.

jb55 commented at 7:11 pm on June 30, 2022: contributor

Concept ACK

darosior commented at 1:26 pm on July 5, 2022: member

Good to see others have an interest in this. :) I’ve addressed the review comments and added the same field to the listunspent entries.

Note however that there is another barrier to tracking coins by descriptor on the RPC interface, if you want to track multiple descriptors on the same wallet. listsinceblock will not add entries for outputs it considers being change, so if you have two descriptors A and B imported on your wallet and you send a coin from A to B it will not be shown as a deposit for B. I think the only reasonable workaround would be to have yet another argument to listsinceblock instructing to not treat change outputs differently (like listunspent does). Ideas welcome, but i’d like to tackle this other issue in another PR.

darosior force-pushed on Jul 5, 2022

furszy commented at 3:32 pm on July 5, 2022: member

As ListTransactions receives the wtx, instead of adding a copy of the scriptPubKey inside COutputEntry to later use it to call PushWalletDescriptors(wallet, r.script_pubkey, entry), what if you just get the output directly?

In other words, drop 9bfd9a96 and change d8b1e37a (line 398) in this way:

0PushWalletDescriptors(wallet, wtx.tx->vout.at(r.vout).scriptPubKey, entry);

darosior force-pushed on Jul 6, 2022

darosior commented at 9:30 am on July 6, 2022: member

Heh, indeed it’s much cleaner. Thanks, done.

(Force pushed again to re-kick CI which was blocked on “Agent is not responding!”)

darosior force-pushed on Jul 6, 2022

in src/wallet/rpc/util.cpp:132 in 83b8bdd30d outdated

127+{
128+    UniValue wallet_descs(UniValue::VARR);
129+    for (const auto& desc: wallet.GetWalletDescriptors(script_pubkey)) {
130+        wallet_descs.push_back(desc.descriptor->ToString());
131+    }
132+    entry.pushKV("wallet_descs", wallet_descs);

Sjors commented at 3:50 pm on July 14, 2022:

Maybe call this parent_descs? The term wallet_ is redundant for a wallet RPC.

The listunspent RPC already has a field called desc. This has the limitation that it’s not actually looking at the wallet descriptor.

In a followup PR we may want to add descs to get descriptor(s) for an individual address, rather than the full range. See #24003.

darosior commented at 9:16 am on July 15, 2022:

Indeed parent_descs is a better name, thanks. Done.

darosior force-pushed on Jul 15, 2022

wallet: allow to fetch the wallet descriptors for a given Script

We currently expose a method to get the signing providers, which allows
to infer a descriptor from the scriptPubKey. But in order to identify
"on" what descriptor a coin was received, we need access to the
descriptors that were imported to the wallet.

55a82eaf91

darosior force-pushed on Jul 15, 2022

darosior commented at 11:07 am on July 15, 2022: member

I added a new commit to make listsinceblock optionally list change outputs as well, and added release notes for this PR. Also rebased due to conflict between the new commit and master.

darosior renamed this:
~~RPC: allow to track coins by imported descriptors~~
RPC: allow to track coins by parent descriptors
on Jul 15, 2022

darosior force-pushed on Jul 15, 2022

in src/wallet/rpc/transactions.cpp:307 in 2a7f812df2 outdated

302@@ -303,6 +303,16 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest)
303     }
304 }
305 
306+//! Fetch parent descriptors of this scriptPubKey.
307+static void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey, UniValue& entry)

achow101 commented at 8:29 pm on July 18, 2022:

In 2a7f812df2ac5c809b4690f9d956564108f705f8 “rpc: output wallet descriptors for received entries in listsinceblock”

To make it easier to read the diffs, perhaps move this to it’s final location when you introduce here rather than moving it again in the next commit.

darosior commented at 10:54 am on July 19, 2022:

Done.

in src/wallet/receive.cpp:227 in 812c45af8e outdated

222@@ -223,7 +223,8 @@ CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx,
223 
224 void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,
225                   std::list<COutputEntry>& listReceived,
226-                  std::list<COutputEntry>& listSent, CAmount& nFee, const isminefilter& filter)
227+                  std::list<COutputEntry>& listSent, CAmount& nFee, const isminefilter& filter,
228+                  bool report_change)

achow101 commented at 8:30 pm on July 18, 2022:

In 812c45af8ed06c985cb5349b11043952e013003d “rpc: add a report_change parameter to listsinceblock”

nit: s/report_change/include_change for consistency with the rpc parameter.

darosior commented at 10:54 am on July 19, 2022:

Done.

rpc: output wallet descriptors for received entries in listsinceblock

The descriptor wallets allow an application to track coins of multiple
descriptors in a single wallet. However, such an application would not
previously be able to (easily) tell what received coin "belongs" to what
descriptor.

This commit tackles this issues by adding a "wallet_desc" entry to the
entries for received coins in 'listsinceblock'.

b724476158

rpc: output parent wallet descriptors for coins in listunspent 55f98d087e

darosior force-pushed on Jul 19, 2022

darosior commented at 5:09 pm on August 10, 2022: member

Friendly ping to reviewers, as i’d love to have this in the next release. :)

MarcoFalke added this to the milestone 24.0 on Aug 10, 2022

achow101 commented at 7:41 pm on August 15, 2022: member

ACK 9e78836cc383357a02412f30f39609d2ea1b3546

fanquake requested review from instagibbs on Aug 15, 2022

in test/functional/wallet_listsinceblock.py:436 in e072f3cf94 outdated

431+        addr = self.nodes[2].getrawchangeaddress()
432+        self.nodes[2].sendtoaddress(addr, 1)
433+        self.generate(self.nodes[2], 1)
434+        coins = self.nodes[2].listsinceblock()["transactions"]
435+        assert_equal(len([c for c in coins if c["address"] == addr]), 0)
436+        coins = self.nodes[2].listsinceblock(include_change=True)["transactions"]

instagibbs commented at 1:27 pm on August 16, 2022:

would this have two entries since you have the send(to change), and the normal change output as well?

I think the test should be explicit about it if so

darosior commented at 1:49 pm on August 16, 2022:

Yes. What would you suggest? Just asserting there are two entries?

instagibbs commented at 1:52 pm on August 16, 2022:

Extract both outputs, assert they are change addresses, I suspect is the best.

darosior commented at 4:41 pm on August 16, 2022:

Done. This needed a couple re-arrangements since listsinceblock lists also the “sent” coins, and we only want coins since the last block.

I couldn’t resist a few drive-by cosmetic changes to the tests, including removing the unneeded block generation.

in doc/release-notes-25504.md:6 in 9e78836cc3 outdated

0@@ -0,0 +1,7 @@
1+Updated RPCs
2+------------
3+
4+- The `listsinceblock`, `listtransactions` and `gettransaction` output now contain a new
5+  `parent_descs` field for every "receive" entry.
6+- A new optional parameter was added to the `listsinceblock` command, allowing to also list outputs

instagibbs commented at 1:28 pm on August 16, 2022:

Just name the argument here?

darosior commented at 4:41 pm on August 16, 2022:

Done.

rpc: add an include_change parameter to listsinceblock

It's useful for an external application tracking coins to not be limited
by our change detection. For instance, for a watchonly wallet with two
descriptors a transaction from one to the other would be considered a
change output and not be included in the result (if the address was not
generated by this wallet).

0fd2d14454

doc: add releases notes for 25504 (listsinceblock updates) a6b0c1fcc0

darosior force-pushed on Aug 16, 2022

instagibbs commented at 4:43 pm on August 16, 2022: member

ACK https://github.com/bitcoin/bitcoin/pull/25504/commits/a6b0c1fcc06485ecd320727fa7534a51a20608c1

achow101 commented at 4:54 pm on August 16, 2022: member

re-ACK a6b0c1fcc06485ecd320727fa7534a51a20608c1

achow101 merged this on Aug 16, 2022

achow101 closed this on Aug 16, 2022

darosior deleted the branch on Aug 16, 2022

sidhujag referenced this in commit 75d2335ad9 on Aug 16, 2022

in src/wallet/rpc/transactions.cpp:635 in a6b0c1fcc0

631@@ -623,6 +632,7 @@ RPCHelpMan listsinceblock()
632     }
633 
634     bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());
635+    bool include_change = (!request.params[4].isNull() && request.params[4].get_bool());

MarcoFalke commented at 8:58 am on August 19, 2022:

I find it ugly to encode default argument values in the code syntax, as opposed to simply name them with a literal. If the default were to change, one would have to change the code syntax as opposed to simply changing a literal.

0const bool include_change{p.isNull()?false:p.get_bool()}; // should be clearer now and in the future if the default was changed or refactored out

S3RK commented at 7:21 am on August 23, 2022: contributor

listsinceblock will not add entries for outputs it considers being change, so if you have two descriptors A and B imported on your wallet and you send a coin from A to B it will not be shown as a deposit for B. @darosior I thought I fixed that in #22929. The test you added also specifically generates change address and not receiving address. Could you elaborate on the use-case here since it differs from the test? listchange parameter still could be useful, but I’d like to understand when and if deposits are not listed correctly.

darosior commented at 7:57 am on August 23, 2022: member

Sure, the use case is using Bitcoin Core to track coins for multiple descriptors, and any descriptor. There exists a number of applications doing that (and in my opinion, there should be more :p).

#22929 was a good fix for users of the wallet as such, but it was limited to a single active descriptor per output type. It’s not a viable solutions for applications that want “just an index of transactions per descriptors”. Such an application should not care about the wallet internals: whether the descriptor is active or not, the spkman internal or not, the address in the book or not. No, they just want to know the transactions related to a set of descriptor in a given block range. We can provide this information, and that’s what this PR is permitting.

Regarding the test, i generated a change address to simulate an address that was externally generated (not in the address book), and not necessarily from a descriptor that was imported as “active” in the watchonly wallet (therefore may be considered internal, ie change).

bitcoin locked this on Aug 23, 2023

RPC: allow to track coins by parent descriptors #25504

Conflicts