rpc: add `relevant_blocks` to `scanblocks status`

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

Currently, after starting a scan with scanblocks the user must wait until the scan is complete to see the associated/relevant block hashes. This updates the scanblocks status result object to provide the relevant_blocks found so far. This enables earlier subsequent lookup within matching blocks (e.g. to run getdescriptoractivity in PR #30708)

Below is an example tested on signet by starting and obtaining status of a scan for an address that has received many coinbase outputs (so there are matches over many blocks, showing the increase in relevant_blocks results in scanblocks status over the life of the scan). Also tested on mainnet (see #30713#pullrequestreview-2296520203).

0build/src/bitcoin-cli -signet scanblocks start '["addr(tb1qsygnet2jdqm8n2p7wmmklp9yel3k7agpnycv4f)"]'

 0build/src/bitcoin-cli -signet scanblocks status
 1{
 2  "progress": 14,
 3  "current_height": 30002,
 4  "relevant_blocks": [
 5    "0000012aee246273622a8fb3546454a1d6c11cb468dfb1cd536a3058b0590cba",
 6    "0000002e043f797a967fca25c18fd2b0f66ddce76b443e540327d2e19928d0aa",
 7...
 8    "000000f7626bb9f7917f48d0c63a9c30a4692232e5cd96366192a62c7d1a89a4",
 9    "000000db94d6817657bd9184ad5f6232981cfe60177b7395485260a339b32571"
10  ]
11}

or (to show snapshots over time)

0for i in `seq 1 20`; do sleep 1; src/bitcoin-cli -signet scanblocks status > test$i.txt; done

-deprecatedrpc seems like overkill for the addition of a key (not a data type change). Release notes have been added (affecting user interface).

If testing, be sure to enable the block filter index feature (e.g. adding blockfilterindex=1 to bitcoin.conf), and allow time for the index to build:

02024-09-11T12:17:49Z Config file arg: signet="1"
12024-09-11T12:17:49Z Config file arg: [signet] blockfilterindex="1"
22024-09-11T12:17:51Z Syncing basic block filter index with block chain from height 0
3...
42024-09-11T12:19:41Z basic block filter index is enabled at height 212758
52024-09-11T12:19:41Z basic block filter index thread exit

DrahtBot commented at 1:29 pm on August 25, 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/30713.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky
Concept ACK	jamesob
Stale ACK	pablomartin4btc

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

Conflicts

No conflicts as of last run.

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

tdb3 renamed this:
~~rpc: add revelant_blocks to scanblocks status~~
rpc: add `revelant_blocks` to `scanblocks status`
on Aug 25, 2024

tdb3 force-pushed on Aug 25, 2024

DrahtBot added the label CI failed on Aug 25, 2024

DrahtBot commented at 1:38 pm on August 25, 2024: contributor

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

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.

DrahtBot removed the label CI failed on Aug 25, 2024

jamesob commented at 10:59 pm on August 25, 2024: contributor

Concept ACK. This’d be useful, especially in conjunction with #30708.

in src/rpc/blockchain.cpp:2592 in 6bac76f25e outdated

2562@@ -2550,7 +2563,8 @@ static RPCHelpMan scanblocks()
2563 
2564         ret.pushKV("from_height", start_block_height);
2565         ret.pushKV("to_height", start_index->nHeight); // start_index is always the last scanned block here
2566-        ret.pushKV("relevant_blocks", std::move(blocks));
2567+        LOCK(cs_relevant_blocks);
2568+        ret.pushKV("relevant_blocks", g_relevant_blocks);

luke-jr commented at 0:52 am on August 27, 2024:

Since status stops working when the scan completes, it seems like we should clear g_relevant_blocks here?

tdb3 commented at 2:46 am on August 27, 2024:

Yes, thank you. Will update.

maflcko commented at 5:05 am on August 27, 2024:

That’d introduce a race, where the relevant blocks are cleared before the reserver is destructed (and thus the status call may return an empty list where previously it didn’t for the same call)

Not sure if it matters, or worthy to address.

tdb3 commented at 9:58 pm on September 9, 2024:

While leaving scanblocks() without resetting g_relevant_blocks isn’t ideal, resetting g_relevant_blocks just before BlockFiltersScanReserver in the start branch seemed to be a good way to prevent status from accidentally seeing an empty g_relevant_blocks (at least in common cases). Please let me know if I’m overlooking a likely concurrency case, and it can be adjusted.

luke-jr commented at 2:55 am on November 15, 2024:

Seems trivial to fix properly? https://github.com/luke-jr/bitcoin/commit/56981318da5dc3145e9f244ad0584ef05c815f6b

tdb3 commented at 5:59 pm on November 30, 2024:

included, thanks

luke-jr changes_requested

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

Concept ACK (coming from #30708).

DrahtBot added the label Needs rebase on Sep 3, 2024

tdb3 force-pushed on Sep 9, 2024

DrahtBot removed the label Needs rebase on Sep 9, 2024

tdb3 marked this as ready for review on Sep 9, 2024

pablomartin4btc commented at 11:23 am on September 11, 2024: member

tACK 39ee30c31f5b33a084f420ed49170e669a8f6440

Release notes has been added and g_relevant_blocks reset has been placed into the start conditional section since my last review.

 0/build/src/bitcoin-cli scanblocks start     '["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]' 850263
 1{
 2  "from_height": 850263,
 3  "to_height": 860861,
 4  "relevant_blocks": [
 5    "00000000000000000002b3c2a395905df9d12ce30d2237f064cdafacb81eb33d",
 6    "00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88",
 7    "00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb"
 8  ],
 9  "completed": true
10}
11
12./build/src/bitcoin-cli scanblocks status
13{
14  "progress": 0,
15  "current_height": 850263,
16  "relevant_blocks": [
17    "00000000000000000002b3c2a395905df9d12ce30d2237f064cdafacb81eb33d"
18  ]
19}

For those not used to scanblocks I’d add a little note somewhere in the description that in order to testing this change the block filter index feature should be enabled, by either passing -blockfilterindex as an argument on the node start up or putting it in the bitcoin.conf file (same for #30708), even it’s specified in the help or an error would lead to that, it’d make it easier and straightforward. Also that if the feature was not enabled before it could take up to a few hours to create the indexes.

for status subcommand, instead of returning no results when there are no scans in progress:

0./build/src/bitcoin-cli -datadir=${AU_DATADIR} scanblocks status
1{
2  "status": "No scan in progress"
3}

for start subcommand, when the abort has been triggered and was no completion:

0/build/src/bitcoin-cli -datadir=${AU_DATADIR} scanblocks start  '["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]' 750000
1{
2  "from_height": 750000,
3  "to_height": 780002,
4  "relevant_blocks": [
5 ],
6  "completed": false,
7  "status": "Abort call performed"
8}

DrahtBot requested review from jamesob on Sep 11, 2024

tdb3 commented at 12:33 pm on September 11, 2024: contributor

Thanks for reviewing.

Out of the scope of this PR, having played around start, status, abort subcommands, I’d add some more context in the results of the first 2. Perhaps as “good first issue”.
for status subcommand, instead of returning no results when there are no scans in progress:
0./build/src/bitcoin-cli -datadir=${AU_DATADIR} scanblocks status
1{
2  "status": "No scan in progress"
3}
for start subcommand, when the abort has been triggered and was no completion:
0/build/src/bitcoin-cli -datadir=${AU_DATADIR} scanblocks start  '["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]' 750000
1{
2  "from_height": 750000,
3  "to_height": 780002,
4  "relevant_blocks": [
5 ],
6  "completed": false,
7  "status": "Abort call performed"
8}

These are good ideas for follow-up PRs.

For those not used to scanblocks I’d add a little note somewhere in the description that in order to testing this change the block filter index feature should be enabled

Good idea. Description updated.

pablomartin4btc commented at 3:31 pm on September 11, 2024: member

Good idea. Description updated.

Thanks! It looks good.

Forgot to mention that it would be nice to add some test checking relevant_blocks is returned by scanblocks status in test/functional/rpc_scanblocks.py if possible.

tdb3 commented at 3:38 pm on September 11, 2024: contributor

Forgot to mention that it would be nice to add some test checking relevant_blocks is returned by scanblocks status in test/functional/rpc_scanblocks.py if possible.

It would be. I took a look at rpc_sccanblocks when building the PR. The existing test for status is pretty minimal, but that’s probably because it might be difficult to get the scan to run long enough to get status.

https://github.com/bitcoin/bitcoin/blob/0725a374941355349bb4bc8a79dad1affb27d3b9/test/functional/rpc_scanblocks.py#L128-L129

Any ideas on how to slow down scanblocks() enough to allow a status call to consistently return status of a scan in progress? When testing locally, I had inserted sleep_for() statements into the start path of scanblocks() to simulate a long scan. The only natural way I’ve thought of doing this is to build a purposefully complex/bloated chain, but even then, it wouldn’t necessarily be consistent on different hardware (faster machines might have the test fail, and slower machines might make the test run unnecessarily long), and it’s pretty bad practice.

pablomartin4btc commented at 11:51 am on September 12, 2024: member

The existing test for status is pretty minimal,

Exactly.

Any ideas on how to slow down scanblocks() enough to allow a status call to consistently return status of a scan in progress?

I’ve been playing a bit around the invalidateblock as I had present in my mind the recent issue from the assume-utxo testing but that wouldn’t make the trick either. I went thru the original PRs of scanblocks (#23549 and #20664) but it seems there were no questions around this ‘status’ call test either. Perhaps @furszy, that worked on a speedup of the rpc command, has an idea if it’s possible (and if worth it).

naiyoma commented at 12:52 pm on October 14, 2024: contributor

Tested changes on mainnet

Started scanning ./build/src/bitcoin-cli scanblocks start '["addr(bc1qgqrn8gratlc59gn5v02nrz0rg8anjjaxt3efh8)"]'

status check ./build/src/bitcoin-cli scanblocks status

0{
1  "progress": 98,
2  "current_height": 570056,
3  "relevant_blocks": [
4    "0000000000005ecf9f819ea03abd8f2a3f0b2cad5e5124bdabd0d7a770fd9067",
5    "000000000000000479f286761e91b2c983ad63e19b0d53cffc67e731a7558d85"
6  ]
7}

scan completed

 0./build/src/bitcoin-cli scanblocks start '["addr(bc1qgqrn8gratlc59gn5v02nrz0rg8anjjaxt3efh8)"]' 
 1{
 2  "from_height": 0,
 3  "to_height": 577394,
 4  "relevant_blocks": [
 5    "0000000000005ecf9f819ea03abd8f2a3f0b2cad5e5124bdabd0d7a770fd9067",
 6    "000000000000000479f286761e91b2c983ad63e19b0d53cffc67e731a7558d85"
 7  ],
 8  "completed": true
 9}
10
11`

furszy commented at 9:56 pm on October 15, 2024: member

Perhaps @furszy, that #26780 on a #23549#pullrequestreview-1105712566 of the rpc command, has an idea if it’s possible (and if worth it).

Not sure if it worth it but could craft an unit test that calls to the RPC command. Subclassing and setting the block manager class so the file opening method “stops” at a certain point. It would mimic the delay.

luke-jr referenced this in commit 17a01bce24 on Nov 15, 2024

luke-jr referenced this in commit 632c672b3b on Nov 15, 2024

luke-jr commented at 2:56 am on November 15, 2024: member

Any reason not to scope the new variables? https://github.com/luke-jr/bitcoin/commit/851d354fc4f94695aabd12d21e470dc90d267b93

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

Thanks. Planning to touch this up soon.

rpc: add relevant_blocks to scanblocks status

Provides relevant block hashes to status output.
Enables a user to start looking through matching
blocks without having to wait on scan completion.

Co-Authored-By: Luke Dashjr <luke-jr+git@utopios.org>

4c9dc4bbe6

doc: add release notes for 30713 5b2d0216d8

tdb3 force-pushed on Nov 30, 2024

tdb3 commented at 5:59 pm on November 30, 2024: contributor

Any reason not to scope the new variables? luke-jr@851d354

moved, thanks

tdb3 commented at 6:00 pm on November 30, 2024: contributor

Updated with suggested changes

jamesob commented at 3:54 pm on December 6, 2024: contributor

Want to reemphasize that I think this is a good change worth doing. I have some questions about using static vars rather than housing running state in a BlockFiltersScanReserver instance, but that’s a pretty small implementation detail. Will review/suggest changes more formally soon.

in src/rpc/blockchain.cpp:2456 in 4c9dc4bbe6 outdated

2451@@ -2441,15 +2452,20 @@ static RPCHelpMan scanblocks()
2452         },
2453         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
2454 {
2455+    static GlobalMutex cs_relevant_blocks;
2456+    static UniValue relevant_blocks GUARDED_BY(cs_relevant_blocks);

ryanofsky commented at 3:34 pm on January 6, 2025:

In commit “rpc: add relevant_blocks to scanblocks status” (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)

It would be good to declare these next to g_scanfilter_ variables so all global state is declared in one place, and so other functions besides scanblocks can see this information (other RPCs and maybe tests should be able to access it). Would suggest renaming relevant_blocks to g_scanfilter_relevant_blocks and cs_relevant_blocks to g_scanfilter_relevant_blocks_mutex. (The “critical section” naming is also a little archaic.)

tdb3 commented at 7:30 pm on January 11, 2025:

Thanks. Will include in next update.

in src/rpc/blockchain.cpp:2602 in 5b2d0216d8

2595@@ -2574,8 +2596,10 @@ static RPCHelpMan scanblocks()
2596 
2597         ret.pushKV("from_height", start_block_height);
2598         ret.pushKV("to_height", start_index->nHeight); // start_index is always the last scanned block here
2599-        ret.pushKV("relevant_blocks", std::move(blocks));
2600+        LOCK(cs_relevant_blocks);
2601+        ret.pushKV("relevant_blocks", std::move(relevant_blocks));
2602         ret.pushKV("completed", completed);
2603+        reserver.release(); // ensure this is before cs_relevant_blocks is released, so status doesn't try to use moved relevant_blocks

ryanofsky commented at 3:50 pm on January 6, 2025:

In commit “rpc: add relevant_blocks to scanblocks status” (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)

Think this is missing a word, suggest “ensure this is called before”

tdb3 commented at 7:30 pm on January 11, 2025:

Will include

in src/rpc/blockchain.cpp:2482 in 5b2d0216d8

2476@@ -2461,6 +2477,11 @@ static RPCHelpMan scanblocks()
2477         g_scanfilter_should_abort_scan = true;
2478         return true;
2479     } else if (request.params[0].get_str() == "start") {
2480+        {
2481+            LOCK(cs_relevant_blocks);
2482+            relevant_blocks = UniValue(UniValue::VARR);

ryanofsky commented at 4:00 pm on January 6, 2025:

In commit “rpc: add relevant_blocks to scanblocks status” (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)

Clearing relevant_blocks before checking g_scanfilter_in_progress seems not very nice, because it means if scanblocks start is called while another scan is in progress, instead of just returning an “already in progress” error, it will do that but also partially erase the state of the in-progress scan.

This behavior is not just a limitation of the new feature, but also a regression, because it messes up the “relevant_blocks” return value of the other RPC call when previously it would be accurate.

I think it should be straightforward to fix this by moving this new code below the reserve() call below.

tdb3 commented at 7:31 pm on January 11, 2025:

Good catch. Verified locally that truncation occurs. Fixed for next update.

in src/rpc/blockchain.cpp:2461 in 5b2d0216d8

2456+    static UniValue relevant_blocks GUARDED_BY(cs_relevant_blocks);
2457+
2458     UniValue ret(UniValue::VOBJ);
2459     if (request.params[0].get_str() == "status") {
2460         BlockFiltersScanReserver reserver;
2461+        LOCK(cs_relevant_blocks);

ryanofsky commented at 4:06 pm on January 6, 2025:

In commit “rpc: add relevant_blocks to scanblocks status” (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)

Lock order here seems a little subtle. Would be good to have a comment saying it is important to lock this before calling reserve so if a scan is currently happening but is about to finish, an accurate “relevant_blocks” value can be returned before the scanning thread clears it.

tdb3 commented at 7:32 pm on January 11, 2025:

Agreed. Comment will be added.

ryanofsky approved

ryanofsky commented at 4:30 pm on January 6, 2025: contributor

Code review ACK 5b2d0216d871bd481f733506a2e8f80b8a34eca0. Seems like a nice feature, and the implementation is pretty simple. I think there is a minor regression if scanblocks start is called from multiple threads, and suggested a fix below (https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904332839).

re: #30713 (comment)

Any ideas on how to slow down scanblocks() enough to allow a status call to consistently return status of a scan in progress?

Will suggest a very general solution that would probably be overkill here, but I think we could have a setmockcondition RPC that acts like the existing setmocktime RPC and lets tests toggle global state that could be useful for testing. In this case I imagine a test could call something like setmockcondition({"scanblocks_pause_at_height": 50}) to make the scanning pause at a certain height and setmockcondition({"scanblocks_pause_at_height": null}) to make it resume, and we could have some helpful functions that make this not too intrusive to implement.

DrahtBot requested review from pablomartin4btc on Jan 6, 2025

in doc/release-notes-30713.md:5 in 5b2d0216d8

0@@ -0,0 +1,5 @@
1+Updated RPCs
2+------------
3+
4+- The `status` action of the`scanblocks` RPC now returns an additional array `relevant_blocks` containing
5+  the matching block hashes found so far during a scan.

mzumsande commented at 9:46 pm on January 6, 2025:

Just a drive-by comment, but there is a typo in the PR title (revelant -> relevant)

tdb3 commented at 7:32 pm on January 11, 2025:

Thanks, fixed.

tdb3 renamed this:
~~rpc: add `revelant_blocks` to `scanblocks status`~~
rpc: add `relevant_blocks` to `scanblocks status`
on Jan 7, 2025

tdb3 commented at 7:36 pm on January 11, 2025: contributor

I think we could have a setmockcondition RPC that acts like the existing setmocktime RPC and lets tests toggle global state that could be useful for testing.

Great idea. Playing around with it locally. Perhaps it could eventually absorb setmocktime as well.

rpc: add relevant_blocks to scanblocks status #30713

Code Coverage & Benchmarks

Reviews

Conflicts

rpc: add `relevant_blocks` to `scanblocks status` #30713