rpc: add getdescriptoractivity

jamesob commented at 4:25 pm on August 24, 2024: contributor

The RPC command scanblocks provides a useful way to get a set of blockhashes that have activity relevant to a set of descriptors (relevant_blocks). However actually extracting the activity from those blocks is left as an exercise to the end user.

This process involves not only generating the (potentially ranged) set of scripts for the descriptor set on the client side (maybe via deriveaddresses), but then the user must retrieve each block’s contents one-by-one using getblock <hash>, which is transmitted over a network link. And that’s all before they perform the actual search over block content. There’s even more work required to incorporate unconfirmed transactions.

This PR introduces an RPC getdescriptoractivity that dovetails with scanblocks output, handling the process described above. Users specify the blockhashes (perhaps from relevant_blocks) and a set of descriptors; they are then given all spend/receive activity in that set of blocks.

This is a very useful tool when implementing lightweight wallets that want neither to require a third-party indexer like electrs, nor the overhead of creating and managing watch-only wallets in Core. This allows Core to be more easily used in a “stateless” manner by wallets, with potentially many nodes interchangeably acting as backends.

Example usage

 0% ./src/bitcoin-cli scanblocks start \
 1    '["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]' \
 2    857263
 3{
 4  "from_height": 857263,
 5  "to_height": 858263,
 6  "relevant_blocks": [
 7    "00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88",
 8    "00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb"
 9  ],
10  "completed": true
11}
12
13
14% ./src/bitcoin-cli getdescriptoractivity \
15    '["00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88", "00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb"]' \
16    '["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]'
17{
18  "activity": [
19    {
20      "type": "receive",
21      "amount": 0.00002900,
22      "blockhash": "00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88",
23      "height": 857907,
24      "txid": "c9d34f202c1f66d80cae76f305350f5fdde910b97cf6ae6bf79f5bcf2a337d06",
25      "vout": 254,
26      "output_spk": {
27        "asm": "1 7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
28        "desc": "rawtr(7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b)#yewcd80j",
29        "hex": "51207e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
30        "address": "bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t",
31        "type": "witness_v1_taproot"
32      }
33    },
34    {
35      "type": "spend",
36      "amount": 0.00002900,
37      "blockhash": "00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb",
38      "height": 858260,
39      "spend_txid": "7f61d1b248d4ee46376f9c6df272f63fbb0c17039381fb23ca5d90473b823c36",
40      "spend_vin": 0,
41      "prevout_txid": "c9d34f202c1f66d80cae76f305350f5fdde910b97cf6ae6bf79f5bcf2a337d06",
42      "prevout_vout": 254,
43      "prevout_spk": {
44        "asm": "1 7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
45        "desc": "rawtr(7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b)#yewcd80j",
46        "hex": "51207e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
47        "address": "bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t",
48        "type": "witness_v1_taproot"
49      }
50    }
51  ]
52}

DrahtBot commented at 4:26 pm on August 24, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30708.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3, instagibbs, achow101, rkrux
Concept ACK	danielabrozzoni, pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label RPC/REST/ZMQ on Aug 24, 2024

tdb3 commented at 6:26 pm on August 24, 2024: contributor

Concept ACK

Thank you. This seems to be a great way to simplify the chain of RPC commands needed to obtain targeted transaction history.

Thought about a potential alternative that updates/enhances scanblocks instead of adding a new RPC (with the advantage of not needing to provide block hashes). Adding getdescriptoractivity seems like a better solution, since it avoids breaking RPC compatibility. Carrying block hashes from scanblocks to getdescriptoractivity seems like a reasonably price to pay for compatibility.

DrahtBot added the label CI failed on Aug 24, 2024

DrahtBot commented at 6:47 pm on August 24, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29204025345

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

jamesob commented at 7:58 pm on August 24, 2024: contributor

Thought about a potential alternative that updates/enhances scanblocks instead of adding a new RPC (with the advantage of not needing to provide block hashes). Adding getdescriptoractivity seems like a better solution, since it avoids breaking RPC compatibility. Carrying block hashes from scanblocks to getdescriptoractivity seems like a reasonably price to pay for compatibility.

I thought about this initially and I think it’s probably worth doing at some point via an additional with_activity parameter or something. The code here could be factored out and reused. But ultimately I think it’s good to have a separate RPC command.

One possibility (that is probably more valuable) is to add an incremental relevant_blocks key to scanblocks status output, so that the client can begin calling getdescriptoractivity progressively as results roll in but before the entire scan is finished.

jamesob force-pushed on Aug 24, 2024

in test/functional/rpc_getdescriptoractivity.py:1 in c3cdf11a57 outdated

0@@ -0,0 +1,171 @@
1+#!/usr/bin/env python3

jonatack commented at 9:15 pm on August 24, 2024:

I think the new test file needs to be added to the runner.

0--- a/test/functional/test_runner.py
1+++ b/test/functional/test_runner.py
2@@ -396,6 +396,7 @@ BASE_SCRIPTS = [
3     'feature_config_args.py',
4     'feature_presegwit_node_upgrade.py',
5     'feature_settings.py',
6+    'rpc_getdescriptoractivity.py',
7     'rpc_getdescriptorinfo.py',
8     'rpc_mempool_info.py',
9     'rpc_help.py',

jamesob force-pushed on Aug 25, 2024

tdb3 commented at 1:30 pm on August 25, 2024: contributor

One possibility (that is probably more valuable) is to add an incremental relevant_blocks key to scanblocks status output, so that the client can begin calling getdescriptoractivity progressively as results roll in but before the entire scan is finished.

Great idea. Created an initial draft PR #30713

jamesob force-pushed on Aug 26, 2024

DrahtBot removed the label CI failed on Aug 26, 2024

danielabrozzoni commented at 3:01 pm on August 29, 2024: contributor

Concept ACK

This would greatly simplify the scanblock UX and allow for scanning activity in the mempool.

I think either a with_activity parameter in scanblocks or a new getdescriptoractivity RPC command would be a good solution. I prefer the former, as the user would only have to wait for one (potentially slow) RPC call instead of two. However, if we go with it, it would be useful to add either a include_mempool parameter to scanblocks, or add a scanmempool command.

jamesob commented at 4:56 pm on August 29, 2024: contributor

I think either a with_activity parameter in scanblocks or a new getdescriptoractivity RPC command would be a good solution. I prefer the former, as the user would only have to wait for one (potentially slow) RPC call instead of two.

This change in conjunction with #30713 will actually allow the most rapid report of descriptor activity, since this call over just a few blockhashes is actually very fast. Results can be scanned progressively as they appear; I don’t think we could get the same responsiveness using a scanblocks [with_activity] approach even though that would be simpler for the caller.

furszy commented at 7:43 pm on August 30, 2024: member

One downside of this approach compared to scanblocks [with_activity] is the re-parsing and re-derivation of the descriptor on each getdescriptoractivity call. E.g. for a ranged descriptor with default values, each command call would involve deriving 1,000 keys and generating their corresponding matching scripts, which are then added to the filter elements set to perform the membership test.

Orthogonal topic: Regardless of how this is implemented, it seems we could skip the repeated work by providing the encoded elements set directly. Curiously, I implemented a command to retrieve the wallet elements set two years ago and have gathered the commits here: https://github.com/furszy/bitcoin-core/tree/2024_wallet_retrieve_needle_set. One drawback of this idea is the inability to expand the descriptor range beyond the pre-established limits during the scanning procedure (it will not expand the limit once it finds a matching script), but this may or may not be an issue depending on the external wallet architecture. - Could move forward with it on another PR if there are some conceptual acks too -

jamesob commented at 1:41 am on September 1, 2024: contributor

One downside of this approach compared to scanblocks [with_activity] is the re-parsing and re-derivation of the descriptor on each getdescriptoractivity call. E.g. for a ranged descriptor with default values, each command call would involve deriving 1,000 keys and generating their corresponding matching scripts, which are then added to the filter elements set to perform the membership test.

How slow is this actually, though? Some kind of bounded cache would probably fix this pretty easily if it wound up taking non-negligible time relative to the other operations for these calls, which I would expect to swamp a non-IO computation like descriptor derivation.

tdb3 commented at 4:18 am on September 2, 2024: contributor

A quick data point. On mainnet, getting activity for 39C7fxSzEACPjM78Z7xdPxhf7mKxJwvfMJ over 200 block hashes was pretty fast. About 2.5s (on a Zen 2 CPU and a pedestrian DRAM-less SSD).

0time src/bitcoin-cli getdescriptoractivity '["00000000000000000000c58c91c455930e9ebd8d463529e1f0f833a16e132a51","000000000000000000014ffca17b9ec566e98e28b655cc835a0f933564b526ba",...]' '["addr(39C7fxSzEACPjM78Z7xdPxhf7mKxJwvfMJ)"]'
1...
2real    0m2.513s
3user    0m0.003s
4sys     0m0.006s

furszy commented at 1:49 pm on September 2, 2024: member

How slow is this actually, though? Some kind of bounded cache would probably fix this pretty easily if it wound up taking non-negligible time relative to the other operations for these calls, which I would expect to swamp a non-IO computation like descriptor derivation.

That was also my initial expectation but then faced stuff like #30632 (comment), which takes ~4 seconds to parse + derive. And it could get worse if the user adds some templating to it (#22838).

However, yeah.. I’m probably over-thinking this for a command that will mostly be used on standard descriptors where the parsing + derivation time is negligible.

Still, for the sake of playing with it; the ~4 seconds descriptor example with some templating:

0"wsh(andor(multi(2,[a0d3c79c/48'/1'/83'/2']tpubDFGoZLGBUQDBPzYHppiNcmX8hg2BkJvaanhUUyQHQCvkbjmqvb5akMW5AQKdYxSHbkaYPZR4JMMSMF7qSW3iERxPoVKSjdttnmEvwhpDAC7/0/*,[ea2484f9/48'/1'/83'/2']tpubDEzGdYvznBEvmWDgo8aJznu74ZRcQct2d2k6VEVtcgKJvCjCVitPVTtxgAfM2Hd5QVscv2jN8AjN6Ch69NhXYiceZ7eR8Sth2Sq6UND18So/<1;2;3;4>/*,[93f245d7/48'/1'/83'/2']tpubDFhRvp2M93SFsVPyp4bMbbgRMMtAs8iW7pMAFyLoZ1tcQF7RGfHUs8xmmC7EgXcE1K5TAQwZdYC8qRGCrp4xFGgv52LmHXPi1Axq3Tzx8vB/0/*),or_i(and_v(v:pkh([61cdf766/48'/1'/83'/2']tpubDECwF5HxsawRWjXiFK5M5aEXXa5suC4bKC3d3FH1N29FxZBTsfwFP6T5MEprZT3ztQMWKVqntYVsayo5EMRDY6o583aVXHeb15wz8goBBd9/0/*),after(1753574400)),thresh(2,pk([621f3bec/48'/1'/0'/2']tpubDEXD6B4sX85AyA3WJv1rB3NnADe8EvujnudmLTY8dmdgVKyUst3R65KQqAVyxY5q5USsMh9iqgGkDMtMzfV7zvRtUhV8timsH3H37P5C4Nt/0/*),s:pk([8275bddc/48'/1'/0'/2']tpubDEfYUTvUb7XpvzxGBXxTjQd7gq6yxHaoHj14igUiQtvf8GKmNLjwwLGib5Pojn2uaYzMQzFbJm9iEcW8QWgD6EfijJYssK6gEgnBZ3DZkVu/0/*),s:pk([d715111c/48'/1'/0'/2']tpubDEzThyvXPmkRAYSJryTPfVwHTEY2hjR8oH957Nk43wGtuvpTVLMKz3hYHe5rNyXXUCy3PSYHKfv3wRnupypT2YzkaCL9yPa4ELvTSKX1GuN/0/*),snl:after(1740182400))),and_v(v:thresh(2,pkh([621f3bec/48'/1'/0'/2']tpubDEXD6B4sX85AyA3WJv1rB3NnADe8EvujnudmLTY8dmdgVKyUst3R65KQqAVyxY5q5USsMh9iqgGkDMtMzfV7zvRtUhV8timsH3H37P5C4Nt/2/*),a:pkh([8275bddc/48'/1'/0'/2']tpubDEfYUTvUb7XpvzxGBXxTjQd7gq6yxHaoHj14igUiQtvf8GKmNLjwwLGib5Pojn2uaYzMQzFbJm9iEcW8QWgD6EfijJYssK6gEgnBZ3DZkVu/2/*),a:pkh([d715111c/48'/1'/0'/2']tpubDEzThyvXPmkRAYSJryTPfVwHTEY2hjR8oH957Nk43wGtuvpTVLMKz3hYHe5rNyXXUCy3PSYHKfv3wRnupypT2YzkaCL9yPa4ELvTSKX1GuN/2/*)),after(1757462400))))"

Informational update: this descriptor (without the templating part) comes from https://github.com/Blockstream/miniscript-templates/blob/main/mint-005.md.

pablomartin4btc commented at 5:00 pm on September 3, 2024: member

Concept ACK

Performed some light testing with the given examples from the top description.

For more complex cases/ non standard descriptors, as mentioned by @furszy, perhaps it’s worth it to analyze them later at some point.

jamesob commented at 2:25 pm on September 6, 2024: contributor

I think this is ready for “actual” review, in case anyone was wondering.

in test/functional/rpc_getdescriptoractivity.py:21 in c660db9867 outdated

16+        self.num_nodes = 1
17+
18+    def run_test(self):
19+        node = self.nodes[0]
20+        wallet = MiniWallet(node)
21+        self.generate(node, 101)

tdb3 commented at 2:31 pm on September 6, 2024:

Rather than generate 101 new blocks, maybe the test could take advantage of the pre-mined test framework chain here (since setup_clean_chain defaults to False, with 200 blocks generated)?

From test_framework/wallet.py:

0# When the pre-mined test framework chain is used, it contains coinbase
1# outputs to the MiniWallet's default address in blocks 76-100

Probably not an exciting speed increase, but every bit counts.

jamesob commented at 0:52 am on September 14, 2024:

Removed this generate() call.

in test/functional/rpc_getdescriptoractivity.py:100 in c660db9867 outdated

 95+
 96+        try:
 97+            node.getdescriptoractivity([invalid_blockhash], [f"addr({addr_1})"], True)
 98+            raise AssertionError("RPC call should have failed")
 99+        except Exception:
100+            pass

tdb3 commented at 2:32 pm on September 6, 2024:

Could use assert_raises_rpc_error here instead of the try/except. Something similar to:

0assert_raises_rpc_error(-5, "Block not found", node.getdescriptoractivity, [invalid_blockhash], [f"addr({addr_1})"], True)

in test/functional/rpc_getdescriptoractivity.py:121 in c660db9867 outdated

89+        self.generate(node, 20) # Generate to get more fees
90+
91+        _, spk_1, addr_1 = getnewdestination()
92+        wallet.send_to(from_node=node, scriptPubKey=spk_1, amount=1 * COIN)
93+
94+        invalid_blockhash = "0000000000000000000000000000000000000000000000000000000000000000"

tdb3 commented at 2:41 pm on September 6, 2024:

nit: 0000... is astronomically unlikely to hit, but not sure it is invalid. Maybe could use invalid_blockhash = "ffff..." instead, which seems appears to be above the powLimit for regtest.

https://github.com/bitcoin/bitcoin/blob/bbf95c0cc57147827b9f4577c641b12dd4170e78/src/kernel/chainparams.cpp#L541

I could be mistaken, and feel free to disregard

jamesob commented at 1:59 am on September 14, 2024:

This value is used as an invalid hash elsewhere (https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_listsinceblock.py#L95) so I’m going to leave this as is.

instagibbs commented at 4:12 pm on November 15, 2024:

testing that this is results in the call getting rejected even if it’s f.e. the second blockhash in a list is also a good idea

in test/functional/rpc_getdescriptoractivity.py:29 in c660db9867 outdated

24+        self.test_activity_in_block(node, wallet)
25+        self.test_no_mempool_inclusion(node, wallet)
26+        self.test_multiple_addresses(node, wallet)
27+        self.test_invalid_blockhash(node, wallet)
28+        self.test_confirmed_and_unconfirmed(node, wallet)
29+        # self.test_receive_then_spend(node, wallet)

tdb3 commented at 3:08 pm on September 6, 2024:

This would be a great test to include. Seeing test_framework.authproxy.JSONRPCException: mandatory-script-verify-flag-failed (Witness program hash mismatch) (-26) when uncommented.

in test/functional/rpc_getdescriptoractivity.py:160 in c660db9867 outdated

155+
156+        assert result['activity'][0]['type'] == 'receive'
157+        assert result['activity'][0]['txid'] == txid_1
158+        assert result['activity'][0]['blockhash'] == blockhash_1
159+        assert result['activity'][0]['address'] == addr_1
160+        assert result['activity'][0]['value'] == 1.0

tdb3 commented at 3:17 pm on September 6, 2024:

0- assert result['activity'][0]['value'] == 1.0
1+ assert result['activity'][0]['amount'] == 1.0

in test/functional/rpc_getdescriptoractivity.py:167 in c660db9867 outdated

162+        assert result['activity'][1]['type'] == 'spend'
163+        assert result['activity'][1]['spend_txid'] == txid_2
164+        assert result['activity'][1]['prevout_txid'] == txid_1
165+        assert result['activity'][1]['blockhash'] == blockhash_2
166+        assert result['activity'][0]['address'] == addr_1
167+        assert result['activity'][0]['value'] == 0.9999

tdb3 commented at 3:18 pm on September 6, 2024:

0- assert result['activity'][0]['value'] == 0.9999
1+ assert result['activity'][0]['amount'] == 0.9999

in test/functional/rpc_getdescriptoractivity.py:144 in c660db9867 outdated

139+
140+        inputs = [{'txid': txid_1, 'vout': vout_idx}]
141+        outputs = {addr_1: 0.9999}
142+        rawtx_2 = node.createrawtransaction(inputs, outputs)
143+        signed = CTransaction()
144+        signed.deserialize(BytesIO(bytes.fromhex(rawtx_2)))

tdb3 commented at 3:47 pm on September 6, 2024:

Could use test_framework/messages.py:tx_from_hex() here, e.g.:

0-        signed = CTransaction()
1-        signed.deserialize(BytesIO(bytes.fromhex(rawtx_2)))
2+        signed = tx_from_hex(rawtx_2)

tdb3 commented at 3:53 pm on September 6, 2024: contributor

Approach ACK

Thanks for the useful RPC. Love that for some use cases it enables the option to avoid ancillary software (e.g. electrum server).

Left some initial comments for now (mainly for tests). Planning to circle back.

DrahtBot added the label CI failed on Sep 7, 2024

in src/rpc/blockchain.cpp:2683 in c660db9867 outdated

2652+            const CTxIn& txin,
2653+            const CBlockIndex* index
2654+            ) {
2655+        UniValue event(UniValue::VOBJ);
2656+        event.pushKV("type", "spend");
2657+        event.pushKV("address", ScriptToAddress(spk).value_or(""));

tdb3 commented at 8:05 pm on September 8, 2024:

nit: instead of making this an empty string if there is no address, could make the “address” key optional and omit it if nullopt.

sipa commented at 3:41 pm on September 11, 2024:

I think emitting the address here is confusing. In all of Bitcoin Core’s RPC interface, addresses are seen as receive-only entities, through which a wallet receives coins, but the resulting balance then belongs to the wallet. This notion of a “from address” you’re using here only makes sense in an “address balance” situation, which doesn’t match the model of rest of the RPC interface. E.g. the wallet’s listtransactions does list an address for both send and receive, but for sending it’s the address being sent to, not the “spent from” address.

What would you think about having a hex-encoded scriptPubKey in both “spend” and “receive” entries here instead? That would be unambiguous, and also more general (doesn’t rely on the scriptPubKey having a well-defined address encoding).

jamesob commented at 0:51 am on September 14, 2024:

I’m in favor of leaving this in as a convenience to the end user, although I think the suggestion to include a hex-encoded sPK is a good one and I’ll add that. Many wallets will ultimately want to show which address is being spent from, e.g.

If we can inexpensively determine that here, I think it’s a nice option for the end user.

In terms of a blank string vs. omitted key, I think it’s better to have a consistent return schema than it is to avoid a blank string, so I’m going to leave that as-is. If there’s some way in our horribly confusing RPC machinery to make the value nullable, that would probably be my preference.

sipa commented at 2:32 pm on September 19, 2024:

Many wallets will ultimately want to show which address is being spent from, e.g.

The fact that people want to do dumb things is not a reason to support it. I blame blockchain explorers for instituting, and perpetuating, the misunderstanding that Bitcoin wallet balances can be assessed by observing their receive addresses’ balances.

Bitcoin Core has never followed the notion that addresses have their own individual balance or can be spent “from” (it’s not wrong or inconsistent to see things this way, but it contributes to the misunderstanding). I don’t think this PR should change that.

jamesob commented at 5:59 pm on October 11, 2024:

I think calling it a “dumb thing” that people want to do is a little bit of a simplification. Nearly every indexing API offers this information, and commercial end users make use of the address for various things. You can imagine a wallet that is programmed not to reuse addresses but still wants to display the address when a spend is detected. If we can easily generate this information (sPK -> address) and attach this information (as we can), it removes the need for the wallet to duplicate the implementation of that mapping.

instagibbs commented at 3:34 pm on November 15, 2024:

I think the lift for downstream wallets to encode spk to address is trivial, so I’d rather no expose “send from” type addresses, but I don’t feel super strongly about it.

If not, making the address field an optional return as @tdb3 says is my ask. Makes it a little more clear conceptually what’s being exposed.

regardless of the result here, there should be test coverage for an output with no address type and the delta from the other test case test_multiple_addresses kind of demonstrates why we shouldn’t be relying on addresses.

 0diff --git a/test/functional/rpc_getdescriptoractivity.py b/test/functional/rpc_getdescriptoractivity.py
 1index e1c4be84e0..3a309261e3 100755
 2--- a/test/functional/rpc_getdescriptoractivity.py
 3+++ b/test/functional/rpc_getdescriptoractivity.py
 4@@ -6,12 +6,11 @@
 5 from decimal import Decimal
 6 
 7 from test_framework.test_framework import BitcoinTestFramework
 8 from test_framework.util import assert_equal, assert_raises_rpc_error
 9 from test_framework.messages import COIN
10-from test_framework.wallet import MiniWallet, getnewdestination
11-
12+from test_framework.wallet import MiniWallet, MiniWalletMode, getnewdestination
13 
14 class GetBlocksActivityTest(BitcoinTestFramework):
15     def set_test_params(self):
16         self.num_nodes = 1
17         self.setup_clean_chain = True
18@@ -25,10 +24,11 @@ class GetBlocksActivityTest(BitcoinTestFramework):
19         self.test_no_activity(node)
20         self.test_activity_in_block(node, wallet)
21         self.test_no_mempool_inclusion(node, wallet)
22         self.test_multiple_addresses(node, wallet)
23         self.test_invalid_blockhash(node, wallet)
24+        self.test_no_address(node, wallet)
25         self.test_confirmed_and_unconfirmed(node, wallet)
26         self.test_receive_then_spend(node, wallet)
27 
28     def test_no_activity(self, node):
29         _, spk_1, addr_1 = getnewdestination()
30@@ -93,10 +93,37 @@ class GetBlocksActivityTest(BitcoinTestFramework):
31         assert a1['amount'] == 1.0
32 
33         assert a2['blockhash'] == blockhash
34         assert a2['amount'] == 2.0
35 
36+    def test_no_address(self, node, wallet):
37+        raw_wallet = MiniWallet(self.nodes[0], mode=MiniWalletMode.RAW_P2PK)
38+        raw_wallet.generate(100, invalid_call=False)
39+
40+        no_addr_tx = raw_wallet.send_self_transfer(from_node=node)
41+        raw_desc = raw_wallet.get_descriptor()
42+
43+        blockhash = self.generate(node, 1)[0]
44+
45+        result = node.getdescriptoractivity([blockhash], [raw_desc], False)
46+
47+        assert_equal(len(result['activity']), 2)
48+
49+        a1 = result['activity'][0]
50+        a2 = result['activity'][1]
51+
52+        assert a1['type'] == "spend"
53+        assert a1['blockhash'] == blockhash
54+        assert a1['address'] == ""
55+        assert a1['amount'] == no_addr_tx["fee"] + Decimal(no_addr_tx["tx"].vout[0].nValue) / COIN
56+
57+        assert a2['type'] == "receive"
58+        assert a2['blockhash'] == blockhash
59+        assert a2['address'] == ""
60+        assert a2['amount'] == Decimal(no_addr_tx["tx"].vout[0].nValue) / COIN
61+
62+
63     def test_invalid_blockhash(self, node, wallet):
64         self.generate(node, 20) # Generate to get more fees
65 
66         _, spk_1, addr_1 = getnewdestination()
67         wallet.send_to(from_node=node, scriptPubKey=spk_1, amount=1 * COIN)

jamesob commented at 8:34 pm on November 18, 2024:

As discussed on IRC, I’ve moved all sPK-specific info into a substructure that matches the one used in getrawtransaction 2. I’ve added @instagibbs’ test under a commit with him as co-author.

in src/rpc/blockchain.cpp:2658 in c660db9867 outdated

2622+            CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(bhash);
2623+            if (!pindex) {
2624+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
2625+            }
2626+            if (!chainman.ActiveChain().Contains(pindex)) {
2627+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Block is not in main chain");

tdb3 commented at 10:54 pm on September 8, 2024:

A test could be added for this error condition (or could be added in a follow-up PR). Here’s a rough idea (https://github.com/tdb3/bitcoin/commit/43301b2019e5b2ceb347641cdb8eb18aa7fa8879)

jamesob commented at 2:09 am on September 14, 2024:

I cherry-picked this, but it adds significant runtime to the test for what is a very basic check. I’d suggest filing a follow-up if you’re still interested in testing this.

instagibbs commented at 3:28 pm on November 15, 2024:

I think it deserves the coverage

tdb3 commented at 11:05 pm on September 8, 2024: contributor

Left a couple more comments.

DrahtBot removed the label CI failed on Sep 9, 2024

in src/rpc/blockchain.cpp:2669 in c660db9867 outdated

2638+        FlatSigningProvider provider;
2639+        std::vector<CScript> scripts = EvalDescriptorStringOrObject(scanobject, provider);
2640+
2641+        for (const CScript& script : scripts) {
2642+            scripts_to_watch.insert(script);
2643+            descriptors_watched.emplace(script, InferDescriptor(script, provider)->ToString());

sipa commented at 3:44 pm on September 11, 2024:

That’s a really roundabout way of generating specialized descriptors, but there doesn’t seem to be a better way unfortunately.

jamesob force-pushed on Sep 14, 2024

jamesob commented at 3:46 pm on September 14, 2024: contributor

I’ve pushed an update addressing feedback and adding release notes. Thanks for all review so far.

in src/rpc/blockchain.cpp:2583 in 1365ee8e9c outdated

2578+            RPCResult::Type::OBJ, "", "", {
2579+                {RPCResult::Type::ARR, "activity", "events", {
2580+                    {RPCResult::Type::OBJ, "", "", {
2581+                        {RPCResult::Type::STR, "type", "always 'spend'"},
2582+                        {RPCResult::Type::STR, "address", "The address being spent from"},
2583+                        {RPCResult::Type::STR, "scriptpubkey_hex", "A hex string of the scriptPubKey being spent from"},

tdb3 commented at 1:55 am on September 19, 2024:

nit: if another update occurs, may want to use STR_HEX for scriptpubkey_hex

jamesob commented at 12:03 pm on October 26, 2024:

Fixed, thanks.

in test/functional/rpc_getdescriptoractivity.py:155 in 1365ee8e9c outdated

125+        activity = result['activity']
126+        assert_equal(len(activity), 2)
127+
128+        [confirmed] = [a for a in activity if a['blockhash'] == blockhash]
129+        assert confirmed['txid'] == txid_1
130+        assert confirmed['height'] == node.getblockchaininfo()['blocks']

tdb3 commented at 2:19 am on September 19, 2024:

If this is touched again, could add asserts to check that the unconfirmed tx has blank blockhash and -1 height. Could be left for a follow-up PR.

jamesob commented at 12:03 pm on October 26, 2024:

Done.

tdb3 approved

tdb3 commented at 2:23 am on September 19, 2024: contributor

ACK 1365ee8e9c7a20aa63bcddb1a6d5843c05ff9330

This is a value-add that I hope gets more traction/review. Left a few more minor nits. Performed light local testing with regtest (sendtoaddress, unconfirmed and confirmed txs). As time allows, I may exercise additional descriptors.

Might also not be a bad idea to mark as experimental in help: EXPERIMENTAL warning: this call may be changed in future releases.

DrahtBot requested review from danielabrozzoni on Sep 19, 2024

DrahtBot requested review from pablomartin4btc on Sep 19, 2024

jamesob force-pushed on Oct 26, 2024

jamesob commented at 12:03 pm on October 26, 2024: contributor

Rebased.

tdb3 approved

tdb3 commented at 2:20 pm on October 26, 2024: contributor

re ACK f383db76ec3aaa9391509c1d9cca763d11b6fe00

Changes include updating scriptpubkey_hex result type to STR_HEX, cleaner Coin usage, and adding a test for unconfirmed tx.

 01:  1365ee8e9c7 ! 1:  f383db76ec3 rpc: add getdescriptoractivity
 1    @@ src/rpc/blockchain.cpp: static RPCHelpMan scanblocks()
 2     +                    {RPCResult::Type::OBJ, "", "", {
 3     +                        {RPCResult::Type::STR, "type", "always 'spend'"},
 4     +                        {RPCResult::Type::STR, "address", "The address being spent from"},
 5    -+                        {RPCResult::Type::STR, "scriptpubkey_hex", "A hex string of the scriptPubKey being spent from"},
 6    ++                        {RPCResult::Type::STR_HEX, "scriptpubkey_hex", "A hex string of the scriptPubKey being spent from"},
 7     +                        {RPCResult::Type::STR, "desc", "The inferred descriptor being spent from"},
 8     +                        {RPCResult::Type::STR_AMOUNT, "amount", "The total amount in " + CURRENCY_UNIT + " of the spent output"},
 9     +                        {RPCResult::Type::STR_HEX, "blockhash", "The blockhash this spend appears in. Empty if in mempool"},
10    @@ src/rpc/blockchain.cpp: static RPCHelpMan scanblocks()
11     +                    {RPCResult::Type::OBJ, "", "", {
12     +                        {RPCResult::Type::STR, "type", "always 'receive'"},
13     +                        {RPCResult::Type::STR, "address", "The address receiving value"},
14    -+                        {RPCResult::Type::STR, "scriptpubkey_hex", "A hex string of the scriptPubKey receiving value"},
15    ++                        {RPCResult::Type::STR_HEX, "scriptpubkey_hex", "A hex string of the scriptPubKey receiving value"},
16     +                        {RPCResult::Type::STR, "desc", "The inferred descriptor receiving value"},
17     +                        {RPCResult::Type::STR_AMOUNT, "amount", "The total amount in " + CURRENCY_UNIT + " of the new output"},
18     +                        {RPCResult::Type::STR_HEX, "blockhash", "The block that this receive is in"},
19    @@ src/rpc/blockchain.cpp: static RPCHelpMan scanblocks()
20     +
21     +            for (size_t vinIdx = 0; vinIdx < tx->vin.size(); ++vinIdx) {
22     +                const auto& txin = tx->vin.at(vinIdx);
23    -+                Coin coin;
24    ++                std::optional<Coin> coin = coins_view.GetCoin(txin.prevout);
25     +
26     +                // Check if the previous output is in the chain
27    -+                if (!coins_view.GetCoin(txin.prevout, coin)) {
28    ++                if (!coin) {
29     +                    // If not found in the chain, check the mempool. Likely, a child
30     +                    // transaction in the mempool has spent the coin.
31     +                    CTransactionRef prev_tx = CHECK_NONFATAL(mempool.get(txin.prevout.hash));
32    @@ src/rpc/blockchain.cpp: static RPCHelpMan scanblocks()
33     +                    value = out.nValue;
34     +                } else {
35     +                    // Coin found in the chain
36    -+                    const CTxOut& out = coin.out;
37    ++                    const CTxOut& out = coin->out;
38     +                    scriptPubKey = out.scriptPubKey;
39     +                    value = out.nValue;
40     +                }
41    @@ test/functional/rpc_getdescriptoractivity.py (new)
42     +        assert confirmed['txid'] == txid_1
43     +        assert confirmed['height'] == node.getblockchaininfo()['blocks']
44     +
45    ++        [unconfirmed] = [a for a in activity if not a['blockhash']]
46    ++        assert unconfirmed['blockhash'] == ""
47    ++        assert unconfirmed['height'] == -1
48    ++
49     +        assert any(a['txid'] == txid_2 for a in activity if a['blockhash'] == "")
50     +
51     +    def test_receive_then_spend(self, node, wallet):

  0[dev@dev01 bitcoin]$ build/src/bitcoin-cli scanblocks start '["addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)"]'
  1{
  2  "from_height": 0,
  3  "to_height": 219195,
  4  "relevant_blocks": [
  5    "000000ddbaa40d82c7ae3e3188347dd77858f37e57aae0e51d0045518c642c1b",
  6    "00000044e825e2222f52f863782dd673c9398eabc37b8cc7c189d38327f3ac7a",
  7    "0000009f95729225fbf95de5ab6e08d810e349d55730e37b17f372b5c3ed389f",
  8    "0000005df3c6ee21d95acab8132ffa712a489547b921d048b4c5bf059ebeae32",
  9    "0000006dca0c315c5d8a8c35900c81eb5b57e7a8be503ab1360eeb3de6935578",
 10    "00000050fe68a263c94c8c530c8218322fe1e3aec1f58c8285bb71d6ddf7acf2",
 11    "000000ab21e84253349e71d2105fb941c3e7809dcd32cedb33a6eb8c1c47c965",
 12    "0000003da13f4a3315722f6573958d858dc48cabc64a4753025bf4ba9b67f137",
 13    "000000d576a443feec7971548ab43b7274d4ab562a9d7e3d16e25cc9c56b0c47",
 14    "000000dee489ccfa856274907bb9b37d8b211d748b2b1bad81a9be4b4e1a1255",
 15    "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
 16    "000000806d2ecac41f893a7517b02cf64ac6313bb55e211738322abe4e150575",
 17    "000000b9b5dcecbbf8538795501491d29f41a05c4d7be3d792adcd21af072c6c",
 18    "00000132e7d605b22aa29ecb8255360dd724348fb49d8ae96fbc3163533723f5",
 19    "0000002725309643dc9ec3612ccf9308e80f9e4720756c400dbc7d458a556fdd"
 20  ],
 21  "completed": true
 22}
 23[dev@dev01 bitcoin]$ build/src/bitcoin-cli getdescriptoractivity '["0000005df3c6ee21d95acab8132ffa712a489547b921d048b4c5bf059ebeae32", "0000003da13f4a3
 24315722f6573958d858dc48cabc64a4753025bf4ba9b67f137", "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e"]' '["addr(tb1qr6u5sukpxq7pzh54gv
 25mtgk0cg47u7dxnp7hzdg)"]'
 26{
 27  "activity": [
 28    {
 29      "type": "receive",
 30      "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
 31      "scriptpubkey_hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
 32      "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
 33      "amount": 0.00001688,
 34      "blockhash": "0000005df3c6ee21d95acab8132ffa712a489547b921d048b4c5bf059ebeae32",
 35      "height": 219028,
 36      "txid": "c151b50870dedb98d73dd1b11d1bb2a9a88f30c67f2dc66472e2e4a30249f4fd",
 37      "vout": 0
 38    },
 39    {
 40      "type": "receive",
 41      "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
 42      "scriptpubkey_hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
 43      "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
 44      "amount": 0.00002532,
 45      "blockhash": "0000005df3c6ee21d95acab8132ffa712a489547b921d048b4c5bf059ebeae32",
 46      "height": 219028,
 47      "txid": "3e0e93d4034ab6d4785502b68c1217b255e4fe9eca64f71f4d7eb8d139c8e231",
 48      "vout": 0
 49    },
 50    {
 51      "type": "receive",
 52      "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
 53      "scriptpubkey_hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
 54      "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
 55      "amount": 0.00001376,
 56      "blockhash": "0000005df3c6ee21d95acab8132ffa712a489547b921d048b4c5bf059ebeae32",
 57      "height": 219028,
 58      "txid": "64e633f12206972a398cd0e22b1ddfad394a30cdd41ec89aa5f1a7cda0aa6c62",
 59      "vout": 0
 60    },
 61    {
 62      "type": "receive",
 63      "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
 64      "scriptpubkey_hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
 65      "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
 66      "amount": 0.00001220,
 67      "blockhash": "0000003da13f4a3315722f6573958d858dc48cabc64a4753025bf4ba9b67f137",
 68      "height": 219053,
 69      "txid": "5402894413a9e5a2a99f1b4b636b408f94f2f9e2c554880324e32906a963572e",
 70      "vout": 0
 71    },
 72    {
 73      "type": "spend",
 74      "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
 75      "scriptpubkey_hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
 76      "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
 77      "amount": 0.00001688,
 78      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
 79      "height": 219156,
 80      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
 81      "spend_vin": 0,
 82      "prevout_txid": "08d56bc381b037220ff2a5f626681d4dbd8feb535c19b1901c16cf25e8bbd38a",
 83      "prevout_vout": 0
 84    },
 85    {
 86      "type": "spend",
 87      "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
 88      "scriptpubkey_hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
 89      "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
 90      "amount": 0.00001786,
 91      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
 92      "height": 219156,
 93      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
 94      "spend_vin": 1,
 95      "prevout_txid": "425170c50211af2d39169c71954ca92d9954b5ef594d5c767642e9f183f871b7",
 96      "prevout_vout": 0
 97    },
 98    {
 99      "type": "spend",
100      "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
101      "scriptpubkey_hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
102      "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
103      "amount": 0.00009776,
104      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
105      "height": 219156,
106      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
107      "spend_vin": 2,
108      "prevout_txid": "450ce8bbb5859589ca71c2a4fb90b3e5be3ef2961ba9c7f57f5b2dcc83e258a8",
109      "prevout_vout": 0
110    },
111    {
112      "type": "spend",
113      "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
114      "scriptpubkey_hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
115      "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
116      "amount": 0.00023844,
117      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
118      "height": 219156,
119      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
120      "spend_vin": 3,
121      "prevout_txid": "4536d6d7af2a2ac194a6394891abb6ce67051b79e3cdcc9c1bac4d285130dfd0",
122      "prevout_vout": 0
123    },
124    {
125      "type": "spend",
126      "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
127      "scriptpubkey_hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
128      "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
129      "amount": 0.00001532,
130      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
131      "height": 219156,
132      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
133      "spend_vin": 4,
134      "prevout_txid": "47119e71a6ec9e212d677265213c19ef84c718099f1b3bad5b7a54fdc94965c1",
135      "prevout_vout": 0
136    },
137    {
138      "type": "spend",
139      "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
140      "scriptpubkey_hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
141      "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
142      "amount": 0.00001220,
143      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
144      "height": 219156,
145      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
146      "spend_vin": 5,
147      "prevout_txid": "5402894413a9e5a2a99f1b4b636b408f94f2f9e2c554880324e32906a963572e",
148      "prevout_vout": 0
149    },
150    {
151      "type": "spend",
152      "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
153      "scriptpubkey_hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
154      "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
155      "amount": 0.00001376,
156      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
157      "height": 219156,
158      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
159      "spend_vin": 6,
160      "prevout_txid": "74628abfe2d51ac73407bf93e024450b1401cea158a0ac990f0d6d607ecca3b5",
161      "prevout_vout": 0
162    },
163    {
164      "type": "spend",
165      "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
166      "scriptpubkey_hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
167      "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
168      "amount": 0.00001844,
169      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
170      "height": 219156,
171      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
172      "spend_vin": 7,
173      "prevout_txid": "764d136486c9752b39bdc0778d28de2e6f08846bc813f125d0daf6fd766aac59",
174      "prevout_vout": 0
175    },
176    {
177      "type": "spend",
178      "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
179      "scriptpubkey_hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
180      "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
181      "amount": 0.00031688,
182      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
183      "height": 219156,
184      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
185      "spend_vin": 8,
186      "prevout_txid": "dc37b9f15692ffb7d88ae1b2f6ee4ebc64d7d4144aa5e89b514f2ab2dc3b8cdd",
187      "prevout_vout": 0
188    }
189  ]
190}

in src/rpc/blockchain.cpp:2620 in f383db76ec outdated

2618+                        {RPCResult::Type::STR, "type", "always 'receive'"},
2619+                        {RPCResult::Type::STR, "address", "The address receiving value"},
2620+                        {RPCResult::Type::STR_HEX, "scriptpubkey_hex", "A hex string of the scriptPubKey receiving value"},
2621+                        {RPCResult::Type::STR, "desc", "The inferred descriptor receiving value"},
2622+                        {RPCResult::Type::STR_AMOUNT, "amount", "The total amount in " + CURRENCY_UNIT + " of the new output"},
2623+                        {RPCResult::Type::STR_HEX, "blockhash", "The block that this receive is in"},

rkrux commented at 12:14 pm on November 4, 2024:

0                        {RPCResult::Type::STR_HEX, "blockhash", "The blockhash that this receive is in. Empty if in mempool"},

A receive can also be in mempool as I checked.

 0➜  bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclitest getdescriptoractivity '[]' \
 1'["addr(tb1qf2xsp2fqyqfauj9elghz6jx7r3fuj07eu2xtgz)"]'
 2{
 3  "activity": [
 4    {
 5      "type": "receive",
 6      "address": "tb1qf2xsp2fqyqfauj9elghz6jx7r3fuj07eu2xtgz",
 7      "scriptpubkey_hex": "00144a8d00a9202013de48b9fa2e2d48de1c53c93fd9",
 8      "desc": "addr(tb1qf2xsp2fqyqfauj9elghz6jx7r3fuj07eu2xtgz)#wga5hye9",
 9      "amount": 0.00991959,
10      "blockhash": "",
11      "height": -1,
12      "txid": "a3e71a792f8bab57e21b8031c55397c468e916240730228c94ebc8bf0a4138a6",
13      "vout": 0
14    }
15  ]
16}

instagibbs commented at 4:52 pm on November 15, 2024:

Well you can have blockhashes and search mempool, so I’m -1 on this suggestion. “Can be empty for mempool-only results”?

rkrux commented at 12:13 pm on November 18, 2024:

I don’t quite understand this^.

From what I understood: If include_mempool is true, then the blockhash can be empty if the transaction is in mempool. If include_mempool is false, then the blockhash can’t be empty because there is no other way for the transaction to appear in the result here besides being in a block.

Am I missing something?

instagibbs commented at 2:31 pm on November 18, 2024:

My point was it doesn’t have to be empty if searching in mempool, as your suggestion sounds to me.

in src/rpc/blockchain.cpp:2626 in f383db76ec outdated

2621+                        {RPCResult::Type::STR, "desc", "The inferred descriptor receiving value"},
2622+                        {RPCResult::Type::STR_AMOUNT, "amount", "The total amount in " + CURRENCY_UNIT + " of the new output"},
2623+                        {RPCResult::Type::STR_HEX, "blockhash", "The block that this receive is in"},
2624+                        {RPCResult::Type::NUM, "height", "Height of the receive (-1 if unconfirmed)"},
2625+                        {RPCResult::Type::STR_HEX, "txid", "Txid of the receiving transaction"},
2626+                        {RPCResult::Type::NUM, "vout", "Vout of the receiving output"},

rkrux commented at 12:20 pm on November 4, 2024:

Nit: Would be easier on the eyes to have a similar wording as in the spend object above.

in src/rpc/blockchain.cpp:2660 in f383db76ec outdated

2655+            blockindexes.push_back(pindex);
2656+        }
2657+    }
2658+
2659+    std::set<CScript> scripts_to_watch;
2660+    std::map<CScript, std::string> descriptors_watched;

rkrux commented at 12:51 pm on November 4, 2024:

+1 on the different naming here. Took me a second to realise the need for the difference that is based on the usages later.

in doc/release-notes-30708.md:1 in f383db76ec outdated

0@@ -0,0 +1,6 @@
1+New RPCs

rkrux commented at 1:39 pm on November 4, 2024:

Should the “Support for Output Descriptors in Bitcoin Core” section in this doc also be updated?

https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md

jamesob commented at 4:39 pm on November 5, 2024:

Good catch. I’ll make that modification if I need to retouch otherwise, but I think it’d be fine as a followup.

rkrux approved

rkrux commented at 2:11 pm on November 4, 2024: none

tACK f383db76ec3aaa9391509c1d9cca763d11b6fe00

Successful make and functional tests. Left couple nits. I believe this is a good addition as explained in the PR description.

I tested this RPC in testnet4 and verified the response for the first 3 relevant blocks.

0➜  bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclitest scanblocks start \
1> '["addr(tb1p9nqshur7c07cnt96l7jlfcq92mvkg89yxguqkk4yx79twanzasus2kz7lg)"]'
2
3➜  bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclitest getdescriptoractivity \
4'["00000000003589f8627742f8cc7713dddb3dacc76218a49a1385014e94062a92","0000000000b216b0baaa06ac27bcc4f4260cd11527305df50124e019f30bb7eb","0000000000000006072f4b37e52f54cb5d2a07830d728a659c422f89d0bfa816"]' \
5'["addr(tb1p9nqshur7c07cnt96l7jlfcq92mvkg89yxguqkk4yx79twanzasus2kz7lg)"]'

For the mainnet, I run a pruned node and manually passed in the latest blockhashes I intended to search in because the blockfilterindex needed for scanblocks doesn’t work with a pruned node. Received a correct error when my node was not synced.

0➜  bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclimain getdescriptoractivity \
1> '["0000000000000000000123c142e45945484da3ea07e3729b4341b322dd0d94c5","00000000000000000001464570897b86bdf7ba2c234d06514b9fb73de93e46e4","00000000000000000002a2fda23944657a333083afc5980b29ac123df1cbf5a6"]' \
2> '["addr(bc1pdmgetk4ptdz3zaux7gxtfzgjqm3srxfhmk9fp0zreyfvxjk42rjqj5fat9)"]'
3
4error code: -8
5error message:
6Block is not in main chain

When I passed in a blockhash that was older than the current pruneheight, it correctly threw an error.

0➜  bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclimain getdescriptoractivity \
1'["00000000000000000002a2fda23944657a333083afc5980b29ac123df1cbf5a6"]' \
2'["addr(bc1pdmgetk4ptdz3zaux7gxtfzgjqm3srxfhmk9fp0zreyfvxjk42rjqj5fat9)"]'
3error code: -1
4error message:
5Block not available (pruned data)

Verified the response for a spend transaction.

 0➜  bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclimain getdescriptoractivity \
 1'["000000000000000000017da01cd2da3d567ac8939be7c260195517167a119dd5"]' \
 2'["addr(bc1qwfwhgkjjm8psacmldx97s389lt320vw2uktqlp)"]'
 3{
 4  "activity": [
 5    {
 6      "type": "spend",
 7      "address": "bc1qwfwhgkjjm8psacmldx97s389lt320vw2uktqlp",
 8      "scriptpubkey_hex": "0014725d745a52d9c30ee37f698be844e5fae2a7b1ca",
 9      "desc": "addr(bc1qwfwhgkjjm8psacmldx97s389lt320vw2uktqlp)#6m3j4j5q",
10      "amount": 0.01802567,
11      "blockhash": "000000000000000000017da01cd2da3d567ac8939be7c260195517167a119dd5",
12      "height": 863925,
13      "spend_txid": "36b4d999c3410524f6c5de229598b766fa90a025c437ab717e72671a51196df0",
14      "spend_vin": 4,
15      "prevout_txid": "363605a5936225e59b252bedc96150b92ee06dc5e4b4b6f648dc3e5bb01d4305",
16      "prevout_vout": 0
17    }
18  ]
19}

in test/functional/rpc_getdescriptoractivity.py:115 in f383db76ec outdated

93+        assert a1['amount'] == 1.0
94+
95+        assert a2['blockhash'] == blockhash
96+        assert a2['amount'] == 2.0
97+
98+    def test_invalid_blockhash(self, node, wallet):

instagibbs commented at 3:24 pm on November 15, 2024:

should also test invalid descriptors

jamesob commented at 8:06 pm on November 18, 2024:

Added.

in test/functional/rpc_getdescriptoractivity.py:163 in f383db76ec outdated

133+        assert unconfirmed['blockhash'] == ""
134+        assert unconfirmed['height'] == -1
135+
136+        assert any(a['txid'] == txid_2 for a in activity if a['blockhash'] == "")
137+
138+    def test_receive_then_spend(self, node, wallet):

instagibbs commented at 4:07 pm on November 15, 2024:

note: this is also a key test of multiple blockhashes

jamesob commented at 8:20 pm on November 18, 2024:

Added a comment to this effect.

in test/functional/rpc_getdescriptoractivity.py:175 in f383db76ec outdated

144+
145+        sent2 = wallet.send_self_transfer(from_node=node, utxo_to_spend=utxo)
146+        blockhash_2 = self.generate(node, 1)[0]
147+
148+        result = node.getdescriptoractivity(
149+            [blockhash_1, blockhash_2], [wallet.get_descriptor()], True)

instagibbs commented at 4:10 pm on November 15, 2024:

block order matters, run this case twice, once with blocks in reverse history order?

instagibbs commented at 4:11 pm on November 15, 2024:

should also test that repeated blockhashes is acceptable(AFAICT it just repeats the event)

jamesob commented at 8:20 pm on November 18, 2024:

Added tests including duplicate and reversal.

in test/functional/rpc_getdescriptoractivity.py:91 in f383db76ec outdated

80+        wallet.send_to(from_node=node, scriptPubKey=spk_1, amount=1 * COIN)
81+        wallet.send_to(from_node=node, scriptPubKey=spk_2, amount=2 * COIN)
82+
83+        blockhash = self.generate(node, 1)[0]
84+
85+        result = node.getdescriptoractivity([blockhash], [f"addr({addr_1})", f"addr({addr_2})"], True)

instagibbs commented at 4:15 pm on November 15, 2024:

would be great if there was a case showing flipping descriptor ordering doesn’t change result ordering

instagibbs commented at 4:18 pm on November 15, 2024:

another nice case would be demonstrating the order of activity results intra-block (order in block seems to be respected)

can either set fees differentially, or explicitly construct a block via node.rpc.generateblock to be quick about it

jamesob commented at 8:04 pm on November 18, 2024:

Added testcases flipping both descriptor order and blockhash order to ensure results don’t change.

instagibbs commented at 4:23 pm on November 15, 2024: member

feature seems to make sense, think the modular approach is best, mostly reviewed tests to get myself familiar with the interface

in test/functional/rpc_getdescriptoractivity.py:167 in f383db76ec outdated

136+        assert any(a['txid'] == txid_2 for a in activity if a['blockhash'] == "")
137+
138+    def test_receive_then_spend(self, node, wallet):
139+        self.generate(node, 20) # Generate to get more fees
140+
141+        sent1 = wallet.send_self_transfer(from_node=node)

instagibbs commented at 4:43 pm on November 15, 2024:

micro-nit: less important but a case that would be nice is showing multi-utxo spending txs show up as multiple spends as well as multi-outputs in one tx

 0    def test_multiple_inputs_outputs(self, node, wallet):
 1        self.generate(node, 1)
 2        assert_equal(node.getrawmempool(), [])
 3
 4        # Two inputs, two outputs, one tx
 5        utxos = [wallet.get_utxo(), wallet.get_utxo()]
 6        wallet.send_self_transfer_multi(from_node=node, num_outputs=2, utxos_to_spend=utxos)
 7
 8        blockhash = self.generate(node, 1)[0]
 9
10        result = node.getdescriptoractivity([blockhash], [wallet.get_descriptor()], True)
11
12        assert_equal(len(result['activity']), 4)
13
14        # Assertions go here
15
16``

instagibbs commented at 4:46 pm on November 15, 2024:

last test nit: for test cleanliness it’d be nice if we knew that the mempool was empty each sub-case

jamesob commented at 8:42 pm on November 18, 2024:

Maybe could come as a follow-up?

instagibbs commented at 2:03 pm on November 19, 2024:

yep feel free to not do this one for now (I just ran into it writing my own test case)

in src/rpc/blockchain.cpp:2606 in f383db76ec outdated

2601+        RPCResult{
2602+            RPCResult::Type::OBJ, "", "", {
2603+                {RPCResult::Type::ARR, "activity", "events", {
2604+                    {RPCResult::Type::OBJ, "", "", {
2605+                        {RPCResult::Type::STR, "type", "always 'spend'"},
2606+                        {RPCResult::Type::STR, "address", "The address being spent from"},

instagibbs commented at 4:49 pm on November 15, 2024:

0                        {RPCResult::Type::STR, "address", "The address being spent from, empty string if none"},

rpc: move-only: move ScriptPubKeyDoc to utils 25fe087de5

jamesob force-pushed on Nov 18, 2024

jamesob commented at 8:42 pm on November 18, 2024: contributor

I’ve pushed changes addressing all outstanding feedback. I’ve slightly changed the return schema as discussed on IRC, moving all scriptPubKey related information into output_spk/prevout_spk, which matches the schema used in getrawtransaction 2.

jamesob commented at 0:07 am on November 19, 2024: contributor

Can someone rerun the windows job? Now that I’m not a member of the org, I can’t kick CI Jobs.

fanquake commented at 11:21 am on November 19, 2024: member

Can someone rerun the windows job?

Kicked it now that the fix is in.

bitcoin deleted a comment on Nov 19, 2024

instagibbs commented at 2:18 pm on November 19, 2024: member

light review Ack 878b6c85466366c4ae5f454ec49b5a5f561e0ed2

Reviewed tests and interface mostly

jamesob commented at 2:41 am on November 21, 2024: contributor

Ready for a re-review when you’re ready, @tdb3 @rkrux.

in test/functional/rpc_getdescriptoractivity.py:11 in 878b6c8546 outdated

 6+from decimal import Decimal
 7+
 8+from test_framework.test_framework import BitcoinTestFramework
 9+from test_framework.util import assert_equal, assert_raises_rpc_error, assert_is_hex_string
10+from test_framework.messages import COIN
11+from test_framework.wallet import MiniWallet, MiniWalletMode, getnewdestination

tdb3 commented at 3:47 am on November 21, 2024:

non-blocking pico nit

If needing to touch this file again:

If more than one name from a module is needed, use lexicographically sorted multi-line imports in order to reduce the possibility of potential merge conflicts

https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines

in test/functional/rpc_getdescriptoractivity.py:41 in 878b6c8546 outdated

36+        _, spk_1, addr_1 = getnewdestination()
37+        result = node.getdescriptoractivity([], [f"addr({addr_1})"], True)
38+        assert_equal(len(result['activity']), 0)
39+
40+    def test_activity_in_block(self, node, wallet):
41+        _, spk_1, addr_1 = getnewdestination()

tdb3 commented at 4:17 am on November 21, 2024:

For a follow-up (if any, or if touching this file again before merge):

Could explicitly use getnewdestination(address_type='bech32m') instead of relying on the default.

Later in the function, the output_spk of the returned activity assumes p2tr. This currently works because bech32m is the default address_type argument of getnewdestination() (currently creates a p2tr addr). Being explicit prevents future changes from causing the test to fail.

jamesob commented at 1:36 pm on November 22, 2024:

Done.

tdb3 approved

tdb3 commented at 4:35 am on November 21, 2024: contributor

Code review and test ACK 878b6c85466366c4ae5f454ec49b5a5f561e0ed2

Really liking how this turned out. Left a couple of minor things. None blocking.

  0$ build/src/bitcoin-cli scanblocks start '["addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)"]'                    
  1{
  2  "from_height": 0,
  3  "to_height": 222935,
  4  "relevant_blocks": [
  5    "000000ddbaa40d82c7ae3e3188347dd77858f37e57aae0e51d0045518c642c1b",
  6    "00000044e825e2222f52f863782dd673c9398eabc37b8cc7c189d38327f3ac7a",
  7    "0000009f95729225fbf95de5ab6e08d810e349d55730e37b17f372b5c3ed389f",
  8    "0000005df3c6ee21d95acab8132ffa712a489547b921d048b4c5bf059ebeae32",
  9    "0000006dca0c315c5d8a8c35900c81eb5b57e7a8be503ab1360eeb3de6935578",
 10    "00000050fe68a263c94c8c530c8218322fe1e3aec1f58c8285bb71d6ddf7acf2",
 11    "000000ab21e84253349e71d2105fb941c3e7809dcd32cedb33a6eb8c1c47c965",
 12    "0000003da13f4a3315722f6573958d858dc48cabc64a4753025bf4ba9b67f137",
 13    "000000d576a443feec7971548ab43b7274d4ab562a9d7e3d16e25cc9c56b0c47",
 14    "000000dee489ccfa856274907bb9b37d8b211d748b2b1bad81a9be4b4e1a1255",
 15    "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
 16    "000000806d2ecac41f893a7517b02cf64ac6313bb55e211738322abe4e150575",
 17    "000000b9b5dcecbbf8538795501491d29f41a05c4d7be3d792adcd21af072c6c",
 18    "00000132e7d605b22aa29ecb8255360dd724348fb49d8ae96fbc3163533723f5",
 19    "0000002725309643dc9ec3612ccf9308e80f9e4720756c400dbc7d458a556fdd",
 20    "0000003a70069a50819c79db5aa6ccf1c9816d010acf0023f2fef215a74b2cae",
 21    "000001495e69fc1c9b89bda912304b117520be2ceca0e816ca0413e3c9f38689",
 22    "000000fd27727531370f04b149d720efaed65a4fdc9edab808d564fcd4b08b60",
 23    "000000e3cbf3670babfe5a9cf5a4a030866a3bdd55b94ee882992e9b6cc1341e",
 24    "00000079e9453d130d83811aec42e64c2208c144e05f23a06dc5348f69a4d683",
 25    "000001410a993190c1bf6c781116ad5375f17501db637804ab5287dae2e4ae04",
 26    "000000d5a0211c72bf9273178bb5a6c91959633a4452ef06fa7f29e4a9c306ac",
 27    "0000006adff987a338db4ac8db74badb9b1b62ecfd244087374d722997919302",
 28    "000000b1f17918e39fa98dc724ff072661985bc81e9f38be8dce72a8538d7639",
 29    "00000119abd3bec20bce8059c3ed1e4feefe948f0650a24b055fb54e83ba55cb",
 30    "00000093c560375959a1264a07b4887001a9d3e022be3fbe0398798fcb067df7",
 31    "000000430e1d404b43777290bec34e0ad1a5ee861a2b6861ebcc02d278915e04",
 32    "000000d85ca9ea66eeba3315d3d41cccca947de1090f5e558ea3a6c371584adb",
 33    "000000ffa900fa98f455711b71e1111766843356d975121f339e0e3861ae0379",
 34    "0000001b3eac9da4a952d575a7f6c5b52b6673f8d2a155ae2865d0577f3408c1",
 35    "0000010cef852109fc17d29a6c0e5a89d2f5393cf4695af93a4794b187163d31",
 36    "0000002a172f4d781bca964dde69363a17848df4fa54541123e03382ea4d3777",
 37    "00000088546b935b77c014ef3271d430e624e8b5bd506a7e455ced2684ddbb6b"
 38  ],
 39  "completed": true
 40}
 41$ build/src/bitcoin-cli getdescriptoractivity '["0000005df3c6ee21d95acab8132ffa712a489547b921d048b4c5bf059ebeae32", "0000003da13f4a3315722f6573958d858dc48cabc64a4753025bf4ba9b67f137", "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e"]' '["addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)"]'
 42{
 43  "activity": [
 44    {
 45      "type": "receive",
 46      "amount": 0.00001688,
 47      "blockhash": "0000005df3c6ee21d95acab8132ffa712a489547b921d048b4c5bf059ebeae32",
 48      "height": 219028,
 49      "txid": "c151b50870dedb98d73dd1b11d1bb2a9a88f30c67f2dc66472e2e4a30249f4fd",
 50      "vout": 0,
 51      "output_spk": {
 52        "asm": "0 1eb94872c1303c115e954336b459f8457dcf34d3",
 53        "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
 54        "hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
 55        "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
 56        "type": "witness_v0_keyhash"
 57      }
 58    },
 59    {
 60      "type": "receive",
 61      "amount": 0.00002532,
 62      "blockhash": "0000005df3c6ee21d95acab8132ffa712a489547b921d048b4c5bf059ebeae32",
 63      "height": 219028,
 64      "txid": "3e0e93d4034ab6d4785502b68c1217b255e4fe9eca64f71f4d7eb8d139c8e231",
 65      "vout": 0,
 66      "output_spk": {
 67        "asm": "0 1eb94872c1303c115e954336b459f8457dcf34d3",
 68        "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
 69        "hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
 70        "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
 71        "type": "witness_v0_keyhash"
 72      }
 73    },
 74    {
 75      "type": "receive",
 76      "amount": 0.00001376,
 77      "blockhash": "0000005df3c6ee21d95acab8132ffa712a489547b921d048b4c5bf059ebeae32",
 78      "height": 219028,
 79      "txid": "64e633f12206972a398cd0e22b1ddfad394a30cdd41ec89aa5f1a7cda0aa6c62",
 80      "vout": 0,
 81      "output_spk": {
 82        "asm": "0 1eb94872c1303c115e954336b459f8457dcf34d3",
 83        "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
 84        "hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
 85        "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
 86        "type": "witness_v0_keyhash"
 87      }
 88    },
 89    {
 90      "type": "receive",
 91      "amount": 0.00001220,
 92      "blockhash": "0000003da13f4a3315722f6573958d858dc48cabc64a4753025bf4ba9b67f137",
 93      "height": 219053,
 94      "txid": "5402894413a9e5a2a99f1b4b636b408f94f2f9e2c554880324e32906a963572e",
 95      "vout": 0,
 96      "output_spk": {
 97        "asm": "0 1eb94872c1303c115e954336b459f8457dcf34d3",
 98        "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
 99        "hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
100        "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
101        "type": "witness_v0_keyhash"
102      }
103    },
104    {
105      "type": "spend",
106      "amount": 0.00001688,
107      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
108      "height": 219156,
109      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
110      "spend_vin": 0,
111      "prevout_txid": "08d56bc381b037220ff2a5f626681d4dbd8feb535c19b1901c16cf25e8bbd38a",
112      "prevout_vout": 0,
113      "prevout_spk": {
114        "asm": "0 1eb94872c1303c115e954336b459f8457dcf34d3",
115        "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
116        "hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
117        "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
118        "type": "witness_v0_keyhash"
119      }
120    },
121    {
122      "type": "spend",
123      "amount": 0.00001786,
124      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
125      "height": 219156,
126      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
127      "spend_vin": 1,
128      "prevout_txid": "425170c50211af2d39169c71954ca92d9954b5ef594d5c767642e9f183f871b7",
129      "prevout_vout": 0,
130      "prevout_spk": {
131        "asm": "0 1eb94872c1303c115e954336b459f8457dcf34d3",
132        "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
133        "hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
134        "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
135        "type": "witness_v0_keyhash"
136      }
137    },
138    {
139      "type": "spend",
140      "amount": 0.00009776,
141      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
142      "height": 219156,
143      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
144      "spend_vin": 2,
145      "prevout_txid": "450ce8bbb5859589ca71c2a4fb90b3e5be3ef2961ba9c7f57f5b2dcc83e258a8",
146      "prevout_vout": 0,
147      "prevout_spk": {
148        "asm": "0 1eb94872c1303c115e954336b459f8457dcf34d3",
149        "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
150        "hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
151        "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
152        "type": "witness_v0_keyhash"
153      }
154    },
155    {
156      "type": "spend",
157      "amount": 0.00023844,
158      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
159      "height": 219156,
160      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
161      "spend_vin": 3,
162      "prevout_txid": "4536d6d7af2a2ac194a6394891abb6ce67051b79e3cdcc9c1bac4d285130dfd0",
163      "prevout_vout": 0,
164      "prevout_spk": {
165        "asm": "0 1eb94872c1303c115e954336b459f8457dcf34d3",
166        "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
167        "hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
168        "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
169        "type": "witness_v0_keyhash"
170      }
171    },
172    {
173      "type": "spend",
174      "amount": 0.00001532,
175      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
176      "height": 219156,
177      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
178      "spend_vin": 4,
179      "prevout_txid": "47119e71a6ec9e212d677265213c19ef84c718099f1b3bad5b7a54fdc94965c1",
180      "prevout_vout": 0,
181      "prevout_spk": {
182        "asm": "0 1eb94872c1303c115e954336b459f8457dcf34d3",
183        "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
184        "hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
185        "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
186        "type": "witness_v0_keyhash"
187      }
188    },
189    {
190      "type": "spend",
191      "amount": 0.00001220,
192      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
193      "height": 219156,
194      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
195      "spend_vin": 5,
196      "prevout_txid": "5402894413a9e5a2a99f1b4b636b408f94f2f9e2c554880324e32906a963572e",
197      "prevout_vout": 0,
198      "prevout_spk": {
199        "asm": "0 1eb94872c1303c115e954336b459f8457dcf34d3",
200        "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
201        "hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
202        "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
203        "type": "witness_v0_keyhash"
204      }
205    },
206    {
207      "type": "spend",
208      "amount": 0.00001376,
209      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
210      "height": 219156,
211      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
212      "spend_vin": 6,
213      "prevout_txid": "74628abfe2d51ac73407bf93e024450b1401cea158a0ac990f0d6d607ecca3b5",
214      "prevout_vout": 0,
215      "prevout_spk": {
216        "asm": "0 1eb94872c1303c115e954336b459f8457dcf34d3",
217        "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
218        "hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
219        "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
220        "type": "witness_v0_keyhash"
221      }
222    },
223    {
224      "type": "spend",
225      "amount": 0.00001844,
226      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
227      "height": 219156,
228      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
229      "spend_vin": 7,
230      "prevout_txid": "764d136486c9752b39bdc0778d28de2e6f08846bc813f125d0daf6fd766aac59",
231      "prevout_vout": 0,
232      "prevout_spk": {
233        "asm": "0 1eb94872c1303c115e954336b459f8457dcf34d3",
234        "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
235        "hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
236        "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
237        "type": "witness_v0_keyhash"
238      }
239    },
240    {
241      "type": "spend",
242      "amount": 0.00031688,
243      "blockhash": "000000ce427e4e24c0866b474f318dc20cb429af214ad3c8cf5f748a7f36c43e",
244      "height": 219156,
245      "spend_txid": "5ac1df04b23250d40bd7fa01cea1ee6afd4354df25c3b97a0c0cf84e83a16db2",
246      "spend_vin": 8,
247      "prevout_txid": "dc37b9f15692ffb7d88ae1b2f6ee4ebc64d7d4144aa5e89b514f2ab2dc3b8cdd",
248      "prevout_vout": 0,
249      "prevout_spk": {
250        "asm": "0 1eb94872c1303c115e954336b459f8457dcf34d3",
251        "desc": "addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)#fwegejqh",
252        "hex": "00141eb94872c1303c115e954336b459f8457dcf34d3",
253        "address": "tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg",
254        "type": "witness_v0_keyhash"
255      }
256    }
257  ]
258}

DrahtBot requested review from rkrux on Nov 21, 2024

in test/functional/rpc_getdescriptoractivity.py:91 in 878b6c8546 outdated

86+        wallet.send_to(from_node=node, scriptPubKey=spk_1, amount=1 * COIN)
87+        wallet.send_to(from_node=node, scriptPubKey=spk_2, amount=2 * COIN)
88+
89+        blockhash = self.generate(node, 1)[0]
90+
91+        result = node.getdescriptoractivity([blockhash], [f"addr({addr_1})", f"addr({addr_2})"], True)

rkrux commented at 10:19 am on November 21, 2024:

Can also test here for duplicated addresses at the end of test_multiple_addresses.

0        # Duplicated descriptors do not return duplicated results
1        result = node.getdescriptoractivity([blockhash], [f"addr({addr_1})", f"addr({addr_1})"], True)
2        assert_equal(len(result['activity']), 1)

jamesob commented at 1:43 pm on November 22, 2024:

Added.

in test/functional/rpc_getdescriptoractivity.py:63 in 878b6c8546 outdated

58+            assert_equal(activity[k], v)
59+
60+        outspk = activity['output_spk']
61+
62+        assert_equal(outspk['asm'][:2], '1 ')
63+        assert_equal(outspk['desc'].split('(')[0], 'rawtr')

rkrux commented at 10:25 am on November 21, 2024:

Nesting outspk worked out well, I was not a fan of the special check for desc in the loop above earlier but at that moment seemed unavoidable.

in test/functional/rpc_getdescriptoractivity.py:53 in 878b6c8546 outdated

48+        [activity] = result['activity']
49+
50+        for k, v in {
51+                'amount': Decimal('1.00000000'),
52+                'blockhash': blockhash,
53+                'height': 201,

rkrux commented at 10:29 am on November 21, 2024:

Nit: Although only one usage right now, but making 201 dependent on 200 above in the setup seems more forward facing: https://github.com/bitcoin/bitcoin/pull/30708/files#diff-dfe8598636b25e8bef111dc785f4cfc330e936191d9adb95b66add813a926839R23.

0GENERATED_BLOCKS = 200
1...
2...
3'height': GENERATED_BLOCKS + 1,

in src/rpc/blockchain.cpp:2733 in 878b6c8546 outdated

2728+
2729+    for (const CBlockIndex* blockindex : blockindexes_sorted) {
2730+        const std::vector<uint8_t> block_data{GetRawBlockChecked(chainman.m_blockman, *blockindex)};
2731+        DataStream block_stream{block_data};
2732+        CBlock block{};
2733+        block_stream >> TX_WITH_WITNESS(block);

rkrux commented at 11:26 am on November 21, 2024:

Thinking out loud, not necessary to be a change atm: The crux of these 4 lines is to return a CBlock given chainman.m_blockman, *blockindex . block_data and block_stream are not used beyond these 4 lines and seem internal to this intent, a sign these can be encapsulated in a function if we anticipate this pattern to be replicated.

achow101 commented at 11:41 pm on November 21, 2024:

In dd19d076c528a075d02fa66aaf906f96fa314450 “rpc: add getdescriptoractivity”

Why not GetBlockChecked?

Additionally, both functions will throw if the block was not found on disk, i.e. pruned. However, since multiple blocks may be passed in, perhaps it should continue with scanning the blocks it can find? Although in that case, it should have a way to report to the user which blocks were skipped.

rkrux commented at 6:51 am on November 22, 2024:

Interesting edge case, agree it would be nice to show results for the blocks that can be found. Right now, passing in 1 available block and 1 pruned block in a pruned node results in an error.

Edit: From a practical standpoint, the usage of this RPC is dependent on the output of the scannblocks output, so hitting this case might be relatively rare but it would still be nice to make this RPC more “response-friendly”.

 0➜  bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclimain getblockchaininfo
 1{
 2  "chain": "main",
 3  "blocks": 871458,
 4  "headers": 871458,
 5  "bestblockhash": "00000000000000000000f8ee686deb34ddfd6d888279a7f0b42f623f938745ea",
 6  "difficulty": 102289407543323.8,
 7  ...
 8  "pruned": true,
 9  "pruneheight": 870990,
10  "automatic_pruning": true,
11  "prune_target_size": 1073741824,
12   ...
13}

0➜  bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclimain getdescriptoractivity \
1'["00000000000000000001dca75f6da4a2ed1c24547e85e5a89fe75701726fb268","00000000000000000000e7b7e55b630178d7e001e0ed5aace4cb18f26ef5a2d9"]' \
2'["addr(bc1qryhgpmfv03qjhhp2dj8nw8g4ewg08jzmgy3cyx)"]'
3error code: -1
4error message:
5Block not available (pruned data)

rkrux commented at 7:01 am on November 22, 2024:

If this behaviour is incorporated, then a test testing with a present block and a pruned block would be nice as well.

And on similar lines, this test test_invalid_blockhash can also be updated wherein an invalid block hash and a valid one are passed but result is still returned for the valid one instead of the RPC throwing an error like it does atm. Below the first block is invalid, the second valid.

0➜  bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclimain getdescriptoractivity \
1'["00000000000000000001dca75f6da4a2ed1c24547e85e5a89fe75701726fb26a","00000000000000000000e7b7e55b630178d7e001e0ed5aace4cb18f26ef5a2d9"]' \
2'["addr(bc1qryhgpmfv03qjhhp2dj8nw8g4ewg08jzmgy3cyx)"]'
3error code: -5
4error message:
5Block not found

jamesob commented at 1:29 pm on November 22, 2024:

I’m going to leave this as-is.

I think partial results would be weird. I like the current behavior that it will succeed if possible but fail if results would be missing; I think any kind of requirement about running pruned or not would impair cases for which the descriptors the user cares about are on the “right side” of the prune cliff.

rkrux approved

rkrux commented at 12:18 pm on November 21, 2024: none

tACK 878b6c85466366c4ae5f454ec49b5a5f561e0ed2

Successful make, unit and functional tests. Definitely a value-add, I like how the problem a user faces has been described in the PR description, and then how this feature solves it.

I reviewed the range diff along with reviewing again the PR as whole.

0git range-diff f383db7...878b6c8

I tested it on mainnet on a quite active address bc1qryhgpmfv03qjhhp2dj8nw8g4ewg08jzmgy3cyx for blocks 870930 and 870931. Sharing an observation below that I found to be interesting.

On a pruned node that was not fully synced, the RPC took 1min and 19sec and I assume it didn’t return unconfirmed transactions. Whereas, when the node was fully synced, it took just 0.5sec & returned unconfirmed transactions as well.

jobeirne-nydig commented at 4:23 pm on November 21, 2024: none

Thanks for the review, all. 3 ACKs so far - any notes from maintainers?

in src/rpc/blockchain.cpp:2664 in dd19d076c5 outdated

2659+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Block is not in main chain");
2660+            }
2661+            if (blockindexes_sorted.insert(pindex).second) {
2662+                if (!heights_seen.insert(pindex->nHeight).second) {
2663+                    throw JSONRPCError(RPC_INVALID_PARAMETER, "Duplicate heights found; blockhashes must be along a single chain");
2664+                }

achow101 commented at 11:33 pm on November 21, 2024:

In dd19d076c528a075d02fa66aaf906f96fa314450 “rpc: add getdescriptoractivity”

This check seems superfluous as it isn’t possible to have two different block hashes at the same height if both are in the main chain.

jamesob commented at 1:30 pm on November 22, 2024:

Good point, removed. I forgot that I had a main-chain check in there. I debated about whether this call would be useful for checking balances on orphaned tips, but probably better to just restrict usage to the main chain to avoid confusion.

in src/rpc/blockchain.cpp:2610 in dd19d076c5 outdated

2605+                {RPCResult::Type::ARR, "activity", "events", {
2606+                    {RPCResult::Type::OBJ, "", "", {
2607+                        {RPCResult::Type::STR, "type", "always 'spend'"},
2608+                        {RPCResult::Type::STR_AMOUNT, "amount", "The total amount in " + CURRENCY_UNIT + " of the spent output"},
2609+                        {RPCResult::Type::STR_HEX, "blockhash", "The blockhash this spend appears in. Empty if in mempool"},
2610+                        {RPCResult::Type::NUM, "height", "Height of the spend (-1 if unconfirmed)"},

achow101 commented at 11:39 pm on November 21, 2024:

In dd19d076c528a075d02fa66aaf906f96fa314450 “rpc: add getdescriptoractivity”

I would prefer to omit these fields entirely rather than placing dummy values for unconfirmed transactions. This is what the wallet does in gettransaction.

Same below for receive.

jamesob commented at 1:30 pm on November 22, 2024:

Done.

achow101 commented at 6:04 pm on November 26, 2024:

blockhash should be omitted too.

jamesob commented at 1:09 am on November 27, 2024:

Fixed.

in src/rpc/blockchain.cpp:2740 in dd19d076c5 outdated

2735+        const CBlockUndo block_undo{GetUndoChecked(*blockman, *blockindex)};
2736+
2737+        for (size_t i = 0; i < block.vtx.size(); ++i) {
2738+            const auto& tx = block.vtx.at(i);
2739+
2740+            if (i > 0) {

achow101 commented at 11:48 pm on November 21, 2024:

In dd19d076c528a075d02fa66aaf906f96fa314450 “rpc: add getdescriptoractivity”

nit: tx->IsCoinbase() is more readable.

0            if (!tx->IsCoinbase()) {

jamesob commented at 1:31 pm on November 22, 2024:

Done.

in src/rpc/blockchain.cpp:2744 in dd19d076c5 outdated

2739+
2740+            if (i > 0) {
2741+                // skip coinbase; spends can't happen there.
2742+                const auto& txundo = block_undo.vtxundo.at(i - 1);
2743+
2744+                for (size_t vinIdx = 0; vinIdx < tx->vin.size(); ++vinIdx) {

achow101 commented at 11:50 pm on November 21, 2024:

In dd19d076c528a075d02fa66aaf906f96fa314450 “rpc: add getdescriptoractivity”

nit: snake_case

0                for (size_t vin_idx = 0; vin_idx < tx->vin.size(); ++vin_idx) {

jamesob commented at 1:27 pm on November 22, 2024:

Fixed.

in src/rpc/blockchain.cpp:2747 in dd19d076c5 outdated

2742+                const auto& txundo = block_undo.vtxundo.at(i - 1);
2743+
2744+                for (size_t vinIdx = 0; vinIdx < tx->vin.size(); ++vinIdx) {
2745+                    const auto& coin = txundo.vprevout.at(vinIdx);
2746+                    const auto& txin = tx->vin.at(vinIdx);
2747+                    if (scripts_to_watch.find(coin.out.scriptPubKey) != scripts_to_watch.end()) {

achow101 commented at 11:52 pm on November 21, 2024:

In dd19d076c528a075d02fa66aaf906f96fa314450 “rpc: add getdescriptoractivity”

nit: .contains() is more readable

Same below for vouts

0                    if (scripts_to_watch.contains(coin.out.scriptPubKey)) {

jamesob commented at 1:27 pm on November 22, 2024:

Done.

in src/rpc/blockchain.cpp:2785 in dd19d076c5 outdated

2780+                std::optional<Coin> coin = coins_view.GetCoin(txin.prevout);
2781+
2782+                // Check if the previous output is in the chain
2783+                if (!coin) {
2784+                    // If not found in the chain, check the mempool. Likely, a child
2785+                    // transaction in the mempool has spent the coin.

achow101 commented at 11:58 pm on November 21, 2024:

In dd19d076c528a075d02fa66aaf906f96fa314450 “rpc: add getdescriptoractivity”

This comment doesn’t make sense to me. It suggests that a different child transaction has spent the coin. But what actually happens in this case is that this transaction (which is already in the mempool) is the child of a transaction that is also in the mempool.

rkrux commented at 5:47 am on November 22, 2024:

Agree, I had understood the essence of this comment within the context it lies, now that I re-read the comment it seems confusing in the first glance and can be improved.

jamesob commented at 1:35 pm on November 22, 2024:

Fixed.

in src/rpc/blockchain.cpp:2776 in dd19d076c5 outdated

2771+        for (const CTxMemPoolEntry& e : mempool.entryAll()) {
2772+            const auto& tx = e.GetSharedTx();
2773+
2774+            const CCoinsViewCache& coins_view = &active_chainstate.CoinsTip();
2775+            CScript scriptPubKey;
2776+            CAmount value;

achow101 commented at 11:58 pm on November 21, 2024:

In dd19d076c528a075d02fa66aaf906f96fa314450 “rpc: add getdescriptoractivity”

These should be scoped in the loop iterating the inputs.

jamesob commented at 1:34 pm on November 22, 2024:

Done.

in src/core_io.h:50 in dd19d076c5 outdated

46@@ -46,5 +47,6 @@ std::string EncodeHexTx(const CTransaction& tx);
47 std::string SighashToStr(unsigned char sighash_type);
48 void ScriptToUniv(const CScript& script, UniValue& out, bool include_hex = true, bool include_address = false, const SigningProvider* provider = nullptr);
49 void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry, bool include_hex = true, const CTxUndo* txundo = nullptr, TxVerbosity verbosity = TxVerbosity::SHOW_DETAILS);
50+std::optional<std::string> ScriptToAddress(const CScript& script);

achow101 commented at 0:00 am on November 22, 2024:

In dd19d076c528a075d02fa66aaf906f96fa314450 “rpc: add getdescriptoractivity”

This function appears to be unused.

rkrux commented at 6:29 am on November 22, 2024:

Probably a remnant of the previous commits now that ScriptToUniv is used that essentially contains this functionality but this ScriptToAddress function still seems generic enough to be added but in a different commit (or PR because this PR doesn’t use it). Though, I doubt adding a function without an usage would be accepted.

jamesob commented at 1:29 pm on November 22, 2024:

Removed.

in test/functional/rpc_getdescriptoractivity.py:35 in dd19d076c5 outdated

30+        self.test_invalid_descriptor(node, wallet)
31+        self.test_confirmed_and_unconfirmed(node, wallet)
32+        self.test_receive_then_spend(node, wallet)
33+
34+    def test_no_activity(self, node):
35+        _, spk_1, addr_1 = getnewdestination()

achow101 commented at 0:02 am on November 22, 2024:

In dd19d076c528a075d02fa66aaf906f96fa314450 “rpc: add getdescriptoractivity”

spk_1 is unused.

jamesob commented at 1:36 pm on November 22, 2024:

Fixed.

in test/functional/rpc_getdescriptoractivity.py:63 in dd19d076c5 outdated

58+
59+        outspk = activity['output_spk']
60+
61+        assert_equal(outspk['asm'][:2], '1 ')
62+        assert_equal(outspk['desc'].split('(')[0], 'rawtr')
63+        assert_is_hex_string(outspk['hex'])

achow101 commented at 0:07 am on November 22, 2024:

In dd19d076c528a075d02fa66aaf906f96fa314450 “rpc: add getdescriptoractivity”

The scriptPubKey bytes are known, so this can directly check they are the same:

0        assert_equal(outspk['hex'], spk_1.hex())

jamesob commented at 1:42 pm on November 22, 2024:

Fixed.

in test/functional/rpc_getdescriptoractivity.py:63 in dd19d076c5 outdated

57+            assert_equal(activity[k], v)
58+
59+        outspk = activity['output_spk']
60+
61+        assert_equal(outspk['asm'][:2], '1 ')
62+        assert_equal(outspk['desc'].split('(')[0], 'rawtr')

achow101 commented at 0:08 am on November 22, 2024:

In dd19d076c528a075d02fa66aaf906f96fa314450 “rpc: add getdescriptoractivity”

I feel like this check is not meaningfully helpful as type is already checked, and the address (and spk too, if my suggestion is tagken) is already directly checked. Furthermore, as the address type is not explicitly specified, I’m not sure that it even makes sense to validate address type when the more important thing is that the correct scriptPubKey and address were found.

jamesob commented at 1:40 pm on November 22, 2024:

IMO it’s fine to sort of overconstrain here; if these results change, we should know about it. I’ve specified the address type as @tdb3 suggested above.

jamesob force-pushed on Nov 22, 2024

DrahtBot commented at 1:51 pm on November 22, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33383517836

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Nov 22, 2024

jamesob force-pushed on Nov 22, 2024

jamesob commented at 2:09 pm on November 22, 2024: contributor

Feedback addressed. Thanks for all review.

DrahtBot removed the label CI failed on Nov 22, 2024

jamesob commented at 5:46 pm on November 22, 2024: contributor

Tests passing, ready for re-review. I think we’re close on this one.

tdb3 approved

tdb3 commented at 6:26 pm on November 23, 2024: contributor

code review and test re ACK 1ca5f7b998c14c44b6afd5e362fc4c007d8eebd3

Repeated test in #30708#pullrequestreview-2450158177

DrahtBot requested review from rkrux on Nov 23, 2024

rkrux approved

rkrux commented at 9:18 am on November 25, 2024: none

re-ACK 1ca5f7b998c14c44b6afd5e362fc4c007d8eebd3

Successful make and tests. I reviewed the range diff.

0git range-diff 878b6c8...1ca5f7b

jamesob requested review from achow101 on Nov 25, 2024

jamesob requested review from jonatack on Nov 25, 2024

instagibbs approved

instagibbs commented at 4:02 pm on November 25, 2024: member

light re-ACK 1ca5f7b998c14c44b6afd5e362fc4c007d8eebd3 ~~878b6c85466366c4ae5f454ec49b5a5f561e0ed2~~ oops

Height returned is now optional, some doc cleanups, removal of duplicate heights check, use of GetBlockChecked

DrahtBot requested review from instagibbs on Nov 25, 2024

jamesob commented at 5:01 pm on November 26, 2024: contributor

@achow101 ready for merge?

jamesob force-pushed on Nov 27, 2024

jamesob commented at 1:09 am on November 27, 2024: contributor

Pushed a small fix addressing blockhash nullability.

rpc: add getdescriptoractivity 811f76f3a5

test: rpc: add no address case for getdescriptoractivity

Co-authored-by: Greg Sanders <gsanders87@gmail.com>

ee3ce6a4f4

doc: update descriptors.md for getdescriptoractivity 37a5c5d836

jamesob force-pushed on Nov 27, 2024

tdb3 approved

tdb3 commented at 1:58 am on November 27, 2024: contributor

Code review and light retest ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21

The changes are very scoped to address the comment. Nice use of get().

Repeated test in #30708#pullrequestreview-2450158177 for sanity.

DrahtBot requested review from rkrux on Nov 27, 2024

instagibbs commented at 2:28 pm on November 27, 2024: member

reACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21

blockhash is now an optional return

achow101 commented at 5:04 pm on November 27, 2024: member

ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21

rkrux approved

rkrux commented at 5:21 pm on November 27, 2024: none

re-ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21

achow101 merged this on Nov 27, 2024

achow101 closed this on Nov 27, 2024

rpc: add getdescriptoractivity #30708

Example usage

Code Coverage & Benchmarks

Reviews