RPC: allow to track coins by parent descriptors #25504

darosior · 2022-06-29T17:26:10Z

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.

src/wallet/wallet.cpp

src/wallet/rpc/transactions.cpp

jonatack · 2022-06-29T18:18:32Z

src/wallet/wallet.cpp

+{
+    std::vector<WalletDescriptor> descs;
+    for (const auto spk_man: GetScriptPubKeyMans(script)) {
+        if (const auto desc_spk_man = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_man)) {


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

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

src/wallet/receive.cpp

w0xlt

Approach ACK

src/wallet/wallet.cpp

DrahtBot · 2022-06-30T04:34:37Z

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.

kristapsk · 2022-06-30T11:02:50Z

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 · 2022-06-30T16:07:06Z

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.

src/wallet/rpc/transactions.cpp

jb55 · 2022-06-30T19:11:07Z

Concept ACK

darosior · 2022-07-05T13:26:45Z

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.

furszy

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 9bfd9a9 and change d8b1e37 (line 398) in this way:

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

darosior · 2022-07-06T09:30:11Z

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

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

src/wallet/rpc/util.cpp

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.

darosior · 2022-07-15T11:07:25Z

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.

src/wallet/rpc/transactions.cpp

src/wallet/receive.cpp

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'.

darosior · 2022-08-10T17:09:57Z

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

achow101 · 2022-08-15T19:41:36Z

ACK 9e78836

instagibbs · 2022-08-16T13:27:37Z

test/functional/wallet_listsinceblock.py

+        self.generate(self.nodes[2], 1)
+        coins = self.nodes[2].listsinceblock()["transactions"]
+        assert_equal(len([c for c in coins if c["address"] == addr]), 0)
+        coins = self.nodes[2].listsinceblock(include_change=True)["transactions"]


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

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

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

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.

doc/release-notes-25504.md

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).

instagibbs · 2022-08-16T16:43:22Z

ACK a6b0c1f

achow101 · 2022-08-16T16:54:51Z

re-ACK a6b0c1f

maflcko · 2022-08-19T08:58:31Z

src/wallet/rpc/transactions.cpp

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


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.

const 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 · 2022-08-23T07:21:43Z

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 · 2022-08-23T07:57:51Z

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).

DrahtBot added RPC/REST/ZMQ Wallet labels Jun 29, 2022

jonatack reviewed Jun 29, 2022

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

jonatack reviewed Jun 29, 2022

View reviewed changes

src/wallet/rpc/transactions.cpp Outdated Show resolved Hide resolved

jonatack reviewed Jun 29, 2022

View reviewed changes

src/wallet/receive.cpp Outdated Show resolved Hide resolved

w0xlt approved these changes Jun 30, 2022

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Jun 30, 2022

rpc, wallet: add abandoned field for all categories of transaction in ListTransaction #25158

Merged

darosior force-pushed the rpc_track_coins_by_descriptor branch from a3f89df to 8976cd8 Compare June 30, 2022 09:03

darosior mentioned this pull request Jun 30, 2022

No label field in listunspent entries in ranged descriptors #23749

Closed

achow101 reviewed Jun 30, 2022

View reviewed changes

src/wallet/rpc/transactions.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/transactions.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/transactions.cpp Outdated Show resolved Hide resolved

darosior force-pushed the rpc_track_coins_by_descriptor branch from 8976cd8 to 053f19a Compare July 5, 2022 13:26

furszy reviewed Jul 5, 2022

View reviewed changes

DrahtBot mentioned this pull request Jul 6, 2022

tests: Run both descriptor and legacy tests within a single test invocation #20892

Closed

darosior force-pushed the rpc_track_coins_by_descriptor branch from 053f19a to f7a0398 Compare July 6, 2022 09:21

darosior force-pushed the rpc_track_coins_by_descriptor branch from f7a0398 to 83b8bdd Compare July 6, 2022 14:52

darosior mentioned this pull request Jul 7, 2022

bitcoind: Use Miniscript descriptors for the watchonly wallet revault/revaultd#419

Draft

Sjors reviewed Jul 14, 2022

View reviewed changes

src/wallet/rpc/util.cpp Outdated Show resolved Hide resolved

darosior force-pushed the rpc_track_coins_by_descriptor branch from 83b8bdd to 3447b80 Compare July 15, 2022 09:16

darosior force-pushed the rpc_track_coins_by_descriptor branch from 3447b80 to dc148f3 Compare July 15, 2022 11:06

darosior changed the title ~~RPC: allow to track coins by imported descriptors~~ RPC: allow to track coins by parent descriptors Jul 15, 2022

darosior force-pushed the rpc_track_coins_by_descriptor branch from dc148f3 to 02cc302 Compare July 15, 2022 15:52

achow101 reviewed Jul 18, 2022

View reviewed changes

src/wallet/rpc/transactions.cpp Outdated Show resolved Hide resolved

src/wallet/receive.cpp Outdated Show resolved Hide resolved

darosior added 2 commits July 19, 2022 12:46

rpc: output parent wallet descriptors for coins in listunspent

55f98d0

darosior force-pushed the rpc_track_coins_by_descriptor branch from 02cc302 to 9e78836 Compare July 19, 2022 10:54

This was referenced Jul 26, 2022

refactor: Add util::Result failure values, multiple error and warning messages #25665

Open

refactor: Use util::Result class for wallet loading #25722

Draft

maflcko added this to the 24.0 milestone Aug 10, 2022

fanquake requested a review from instagibbs August 15, 2022 20:40

instagibbs reviewed Aug 16, 2022

View reviewed changes

darosior added 2 commits August 16, 2022 18:33

doc: add releases notes for 25504 (listsinceblock updates)

a6b0c1f

darosior force-pushed the rpc_track_coins_by_descriptor branch from 9e78836 to a6b0c1f Compare August 16, 2022 16:41

achow101 merged commit c336f81 into bitcoin:master Aug 16, 2022

darosior deleted the rpc_track_coins_by_descriptor branch August 16, 2022 18:04

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 16, 2022

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

75d2335

maflcko reviewed Aug 19, 2022

View reviewed changes

kouloumos mentioned this pull request Sep 7, 2022

test: BDB-only build fails wallet_basic.py #26029

Closed

darosior mentioned this pull request Sep 22, 2022

listtransactions fails to list self-send transactions (for imported descriptor wallet) #26096

Open

craigraw mentioned this pull request Nov 24, 2022

Add Taproot support bwt-dev/bwt#91

Open

bitcoin locked and limited conversation to collaborators Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC: allow to track coins by parent descriptors #25504

RPC: allow to track coins by parent descriptors #25504

darosior commented Jun 29, 2022

jonatack Jun 29, 2022

darosior Jun 30, 2022

w0xlt left a comment

DrahtBot commented Jun 30, 2022 •

edited

kristapsk commented Jun 30, 2022

achow101 commented Jun 30, 2022

jb55 commented Jun 30, 2022

darosior commented Jul 5, 2022

furszy left a comment •

edited

darosior commented Jul 6, 2022 •

edited

darosior commented Jul 15, 2022

darosior commented Aug 10, 2022

achow101 commented Aug 15, 2022

instagibbs Aug 16, 2022

darosior Aug 16, 2022

instagibbs Aug 16, 2022

darosior Aug 16, 2022

instagibbs commented Aug 16, 2022

achow101 commented Aug 16, 2022

maflcko Aug 19, 2022

S3RK commented Aug 23, 2022

darosior commented Aug 23, 2022

RPC: allow to track coins by parent descriptors #25504

RPC: allow to track coins by parent descriptors #25504

Conversation

darosior commented Jun 29, 2022

jonatack Jun 29, 2022

Choose a reason for hiding this comment

darosior Jun 30, 2022

Choose a reason for hiding this comment

w0xlt left a comment

Choose a reason for hiding this comment

DrahtBot commented Jun 30, 2022 • edited

Conflicts

kristapsk commented Jun 30, 2022

achow101 commented Jun 30, 2022

jb55 commented Jun 30, 2022

darosior commented Jul 5, 2022

furszy left a comment • edited

Choose a reason for hiding this comment

darosior commented Jul 6, 2022 • edited

darosior commented Jul 15, 2022

darosior commented Aug 10, 2022

achow101 commented Aug 15, 2022

instagibbs Aug 16, 2022

Choose a reason for hiding this comment

darosior Aug 16, 2022

Choose a reason for hiding this comment

instagibbs Aug 16, 2022

Choose a reason for hiding this comment

darosior Aug 16, 2022

Choose a reason for hiding this comment

instagibbs commented Aug 16, 2022

achow101 commented Aug 16, 2022

maflcko Aug 19, 2022

Choose a reason for hiding this comment

S3RK commented Aug 23, 2022

darosior commented Aug 23, 2022

DrahtBot commented Jun 30, 2022 •

edited

furszy left a comment •

edited

darosior commented Jul 6, 2022 •

edited