Add scanblocks RPC call (attempt 2)

jamesob commented at 9:56 pm on November 18, 2021: member

Revives #20664. All feedback from the previous PR has either been responded to inline or incorporated here.

Major changes from Jonas’ PR:

consolidated arguments for scantxoutset/scanblocks
substantial cleanup of the functional test

Here’s the range-diff (git range-diff master jonasschnelli/2020/12/filterblocks_rpc jamesob/2021-11-scanblocks): https://gist.github.com/jamesob/aa4a975344209f0316444b8de2ec1d18

Original PR description

The scanblocks RPC call allows one to get relevant blockhashes from a set of descriptors by scanning all blockfilters in a given range.

Example:

scanblocks start '["addr(<bitcoin_address>)"]' 661000 (returns relevant blockhashes for <bitcoin_address> from blockrange 661000->tip)

Why is this useful?

Fast wallet rescans: get the relevant blocks and only rescan those via rescanblockchain getblockheader(<hash>)[height] getblockheader(<hash>)[height]). A future PR may add an option to allow to provide an array of blockhashes to rescanblockchain.

prune wallet rescans: (needs additional changes): together with a call to fetch blocks from the p2p network if they have been pruned, it would allow to rescan wallets back to the genesis block in pruned mode (relevant #15946).

SPV mode (needs additional changes): it would be possible to build the blockfilterindex from the p2p network (rather then deriving them from the blocks) and thus allow some sort of hybrid-SPV mode with moderate bandwidth consumption (related #9483)

jamesob force-pushed on Nov 18, 2021

jamesob commented at 10:14 pm on November 18, 2021: member

Previously involved:

@Sjors: Concept ACK, tested
@jonatack: Concept ACK, tested
@luke-jr: Rebased, tested
@fjahr: Concept ACK
@felipsoarez: Concept ACK
@chrisguida: Tested ACK
@prayank23: Concept ACK, tested
@kiminuo: Reviewed
@achow101: Reviewed
@theStack: Concept ACK
@darosior: Concept ACK

ghost commented at 10:18 pm on November 18, 2021: none

Thanks for reviving 20664. There are many interesting PRs by jonasschnelli which never got merged for different reasons.

Will test it this weekend.

DrahtBot commented at 11:02 pm on November 18, 2021: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26283 (p2p: Fill reconciliation sets and request reconciliation (Erlay) by naumenkogs)
#25366 (util, rpc: Add parameter to deriveaddresses to display address information by w0xlt)
#24162 (rpc: add require_checksum flag to deriveaddresses by kallewoof)
#23443 (p2p: Erlay support signaling by naumenkogs)

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

DrahtBot added the label RPC/REST/ZMQ on Nov 18, 2021

in src/rpc/blockchain.cpp:2374 in c1e71fbc39 outdated

2624+                LogPrint(BCLog::RPC, "Fetching blockfilters from height %d to height %d.\n", start_index->nHeight, block->nHeight);
2625+                if (index->LookupFilterRange(start_index->nHeight, block, filters)) {
2626+                    for (const BlockFilter& filter : filters) {
2627+                        // compare the elements-set with each filter
2628+                        if (filter.GetFilter().MatchAny(needle_set)) {
2629+                            blocks.push_back(filter.GetBlockHash().GetHex());

benthecarman commented at 9:11 pm on November 19, 2021:

I think all the scanning functionality makes more sense somewhere, not in the rpc folder

jamesob commented at 10:08 pm on November 19, 2021:

#20664 (review)

ghost commented at 9:43 pm on November 21, 2021: none

I tried the syntax based on example mentioned in PR description and it gives me an error:

0$ bitcoin-cli scanblocks start '["addr(mzrj4QmPhk98vc2yQw42uCsgwfBjVzPPLM)"]' 1000000
1error code: -1
2error message:
3Index is not enabled for filtertype basic

I checked the code for RPC help but don’t understand what exactly should be enabled and how are filters used in this RPC

fjahr commented at 11:21 pm on November 21, 2021: contributor

I tried the syntax based on example mentioned in PR description and it gives me an error:
0$ bitcoin-cli scanblocks start '["addr(mzrj4QmPhk98vc2yQw42uCsgwfBjVzPPLM)"]' 1000000
1error code: -1
2error message:
3Index is not enabled for filtertype basic
I checked the code for RPC help but don’t understand what exactly should be enabled and how are filters used in this RPC

You probably did not run bitcoind with -blockfilterindex=1?

I guess that issue could be caught earlier and use a better error message :)

ghost commented at 2:14 am on November 22, 2021: none

You probably did not run bitcoind with -blockfilterindex=1?

Thanks for the help @fjahr. Tried running bitcoind with -blockfilterindex=1:

02021-11-22T01:44:23Z basic block filter index is enabled at height 2104934
12021-11-22T01:44:23Z basic block filter index thread exit

0$ bitcoin-cli scanblocks start '["addr(mzrj4QmPhk98vc2yQw42uCsgwfBjVzPPLM)"]' 1000000

Got response after few minutes with lot of block hashes. I was expecting a quick response based on concept of this PR and time it took for -blockfilterindex=1 so not sure what exactly did we achieve but will check others things later. Will sleep now as its morning here already.

benthecarman commented at 7:00 pm on November 24, 2021: contributor

Concept ACK

Is there plans to add functionality similiar to rescanblockchain so it does it for your wallets, without having to enter in the descriptors manually?

in src/rpc/blockchain.cpp:2521 in d836ce3f2f outdated

2516+                {RPCResult::Type::ARR, "relevant_blocks", "", {{RPCResult::Type::STR_HEX, "blockhash", "A relevant blockhash"},}},
2517+                },
2518+            },
2519+        },
2520+        RPCExamples{
2521+            HelpExampleCli("scanblocks", "\"[\"addr(bcrt1q4u4nsgk6ug0sqz7r3rj9tykjxrsl0yy4d0wwte)\"]\" 300000") +

theStack commented at 3:43 pm on November 29, 2021:

The examples are missing the action argument, and the descriptors list should be surrounded by single quotes to work on the CLI (didn’t check what changes are needed for the RPC example below):

0            HelpExampleCli("scanblocks", "start \'[\"addr(bcrt1q4u4nsgk6ug0sqz7r3rj9tykjxrsl0yy4d0wwte)\"]\' 300000") +
1```-

theStack commented at 4:01 pm on November 29, 2021: contributor

Concept re-ACK. Thanks for picking this up! 👍

Did a first quick test on signet with a coinbase tx address from an arbitrary block (height 1337) that is spent about 1000 blocks later (https://explorer.bc-2.jp/address/tb1q6tnc9k7a3pdufryr9tpttjulh97g838vljyrgx):

0$ ./src/bitcoind -signet -blockfilterindex=1 &
1$ ./src/bitcoin-cli -signet scanblocks start '["addr(tb1q6tnc9k7a3pdufryr9tpttjulh97g838vljyrgx)"]'
2{
3  "from_height": 0,
4  "to_height": 66345,
5  "relevant_blocks": [
6    "00000109e0671e4b8bc44ae1f08a6a87d656968d08e264f20631ed32f69fbea8",
7    "00000030b26a4a94272999a68068adf0b617beef9caac51d6ff003dbf4bd8223"
8  ]
9}

On a quick glance over the second commit, I saw that the help examples are not working, see comment below. Will review more in-depth within the next days.

jamesob force-pushed on Dec 2, 2021

jamesob commented at 9:51 pm on December 2, 2021: member

On a quick glance over the second commit, I saw that the help examples are not working, see comment below. Will review more in-depth within the next days. @theStack fixed, thanks!

in src/rpc/blockchain.cpp:2321 in e1c89184cd outdated

2316+};
2317+
2318+static const auto scan_result_abort = RPCResult{"When action=='abort'", RPCResult::Type::BOOL, "", ""};
2319+static const auto scan_result_status_none = RPCResult{"When action=='status' and no scan is in progress", RPCResult::Type::NONE, "", ""};
2320+static const auto scan_result_status_some = RPCResult{
2321+    "When action=='status' and scan is in progress", RPCResult::Type::OBJ, "", "",

luke-jr commented at 2:45 am on December 4, 2021:

Other RPC docs use plain English "if verbose is not set or set to false"

Suggest adopting the language used in prior revisions of this feature

jamesob commented at 9:52 pm on September 7, 2022:

Not sure what you mean here.

luke-jr commented at 10:05 pm on September 7, 2022:

I mean we should stick to either plain English or pseudo-code for docs, but avoid mixing the two.

in src/rpc/blockchain.cpp:2252 in e1c89184cd outdated

2512+            scan_result_status_none,
2513+            scan_result_status_some,
2514+            RPCResult{"When action=='start'", RPCResult::Type::OBJ, "", "", {
2515+                {RPCResult::Type::NUM, "from_height", "The height we started the scan from"},
2516+                {RPCResult::Type::NUM, "to_height", "The height we ended the scan at"},
2517+                {RPCResult::Type::ARR, "relevant_blocks", "", {{RPCResult::Type::STR_HEX, "blockhash", "A relevant blockhash"},}},

luke-jr commented at 2:46 am on December 4, 2021:

0-  "relevant_blocks" : [    (json array) Blocks that may have matched a scanobject.
1-    "hex",                 (string) The blockhash
2+  "relevant_blocks" : [    (json array)
3+    "hex",                 (string) A relevant blockhash

I like the older doc better

Sjors commented at 9:36 am on July 19, 2022:

Same

in src/rpc/blockchain.cpp:2318 in e1c89184cd outdated

2313+            }},
2314+    },
2315+    "[scanobjects,...]"
2316+};
2317+
2318+static const auto scan_result_abort = RPCResult{"When action=='abort'", RPCResult::Type::BOOL, "", ""};

luke-jr commented at 2:49 am on December 4, 2021:

Prior revisions explained, "True if scan will be aborted (not necessarily before this RPC call returns), or false if there is no scan to abort."

in src/rpc/blockchain.cpp:2322 in e1c89184cd outdated

2317+
2318+static const auto scan_result_abort = RPCResult{"When action=='abort'", RPCResult::Type::BOOL, "", ""};
2319+static const auto scan_result_status_none = RPCResult{"When action=='status' and no scan is in progress", RPCResult::Type::NONE, "", ""};
2320+static const auto scan_result_status_some = RPCResult{
2321+    "When action=='status' and scan is in progress", RPCResult::Type::OBJ, "", "",
2322+    {{RPCResult::Type::NUM, "progress", "The scan progress"},}

luke-jr commented at 2:51 am on December 4, 2021:

Docs for "current_height" got lost somewhere?

0-  "current_height" : n     (numeric) Height of the block currently being scanned.

luke-jr commented at 2:53 am on December 4, 2021:

"Approximate percent complete." seemed better

in src/rpc/blockchain.cpp:2249 in e1c89184cd outdated

2509+        },
2510+        {
2511+            scan_result_abort,
2512+            scan_result_status_none,
2513+            scan_result_status_some,
2514+            RPCResult{"When action=='start'", RPCResult::Type::OBJ, "", "", {

luke-jr commented at 2:55 am on December 4, 2021:

The ordering is odd here. You start before checking status, and probably check status before aborting..

in src/rpc/blockchain.cpp:2249 in e1c89184cd outdated

2518+                },
2519+            },
2520+        },
2521+        RPCExamples{
2522+            HelpExampleCli("scanblocks", "start \"[\"addr(bcrt1q4u4nsgk6ug0sqz7r3rj9tykjxrsl0yy4d0wwte)\"]\" 300000") +
2523+            HelpExampleRpc("scanblocks", "start \"[\"addr(bcrt1q4u4nsgk6ug0sqz7r3rj9tykjxrsl0yy4d0wwte)\"]\" 300000") +

luke-jr commented at 3:07 am on December 4, 2021:

0> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "scanblocks", "params": [start "["addr(bcrt1q4u4nsgk6ug0sqz7r3rj9tykjxrsl0yy4d0wwte)"]" 300000]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/

“start” needs quotes. Params need comma separators. Array shouldn’t have quotes outside it.

(Other examples have some or all of the same issues)

in src/rpc/blockchain.cpp:2248 in e1c89184cd outdated

2517+                {RPCResult::Type::ARR, "relevant_blocks", "", {{RPCResult::Type::STR_HEX, "blockhash", "A relevant blockhash"},}},
2518+                },
2519+            },
2520+        },
2521+        RPCExamples{
2522+            HelpExampleCli("scanblocks", "start \"[\"addr(bcrt1q4u4nsgk6ug0sqz7r3rj9tykjxrsl0yy4d0wwte)\"]\" 300000") +

luke-jr commented at 3:11 am on December 4, 2021:

Additional escaping needed for double quotes within double quotes.

Sjors commented at 1:38 pm on July 19, 2022:

I tend to use ’ around arrays and dictionaries in the RPC to avoid the need for escape characters. The send help does that too.

unknown approved

unknown commented at 5:48 pm on December 4, 2021: none

.

unknown changes_requested

unknown commented at 5:50 pm on December 4, 2021: none

NACK

Based on my testing I was not able to do fast scan as mentioned in OP:

Why is this useful? Fast wallet rescans: get the relevant blocks and only rescan those via rescanblockchain getblockheader()[height] getblockheader()[height]). A future PR may add an option to allow to provide an array of blockhashes to rescanblockchain.

sipa commented at 5:54 pm on December 4, 2021: member

@prayank23 Without blockfilters it would take many times longer, and without this PR the functionality to scan blocks doesn’t exist at all.

ghost commented at 5:57 pm on December 4, 2021: none

@sipa is that considering the time it takes for -blockfilterindex=1 ? PR does not achieve any fast scans IMO. Maybe description should be changed or PR author can take some time to respond with comments when reviewers waste their nights to review the PR

sipa commented at 5:58 pm on December 4, 2021: member

@prayank23 You only need to build the filters once, and you need them too for -peerblockfilters (if you want to support BIP157 protocol).

luke-jr changes_requested

in test/functional/rpc_scanblocks.py:36 in e1c89184cd outdated

31+        height = node.getblockheader(blockhash)['height']
32+        self.wait_until(lambda: all(i["synced"] for i in node.getindexinfo().values()))
33+
34+        out = node.scanblocks("start", [f"addr({addr_1})"])
35+        assert(blockhash in out['relevant_blocks'])
36+        assert_equal(node.getblockheader(blockhash)['height'], out['to_height'])

luke-jr commented at 9:29 pm on March 23, 2022:

0        assert_equal(height, out['to_height'])

in src/rpc/blockchain.cpp:2232 in e1c89184cd outdated

2501+        "\nReturn relevant blockhashes for given descriptors.\n"
2502+        "This call may take several minutes. Make sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0)",
2503+        {
2504+            scan_action_arg_desc,
2505+            scan_objects_arg_desc,
2506+            RPCArg{"start_height", RPCArg::Type::NUM, RPCArg::DefaultHint{"0"}, "Height to start to scan from"},

luke-jr commented at 10:08 pm on March 23, 2022:

Why isn’t this just

0            RPCArg{"start_height", RPCArg::Type::NUM, RPCArg::Default{0}, "Height to start to scan from"},

Sjors commented at 1:35 pm on July 19, 2022:

@MarcoFalke?

maflcko commented at 2:22 pm on July 19, 2022:

Yes

in src/rpc/blockchain.cpp:2234 in e1c89184cd outdated

2503+        {
2504+            scan_action_arg_desc,
2505+            scan_objects_arg_desc,
2506+            RPCArg{"start_height", RPCArg::Type::NUM, RPCArg::DefaultHint{"0"}, "Height to start to scan from"},
2507+            RPCArg{"stop_height", RPCArg::Type::NUM, RPCArg::DefaultHint{"chain tip"}, "Height to stop to scan"},
2508+            RPCArg{"filtertype", RPCArg::Type::STR, RPCArg::DefaultHint{"basic"}, "The type name of the filter"}

luke-jr commented at 10:09 pm on March 23, 2022:

0            RPCArg{"filtertype", RPCArg::Type::STR, RPCArg::Default{"basic"}, "The type name of the filter"}

in src/rpc/blockchain.cpp:2585 in e1c89184cd outdated

2581+                if (!block) {
2582+                    throw JSONRPCError(RPC_MISC_ERROR, "Invalid start_height");
2583+                }
2584+            }
2585+            if (!request.params[3].isNull()) {
2586+                stop_block = active_chain[request.params[3].get_int()];

luke-jr commented at 10:18 pm on March 23, 2022:

It’s possible this block could be reorg’d out during the scan, and then it won’t stop until it reaches the new best block?

I guess if it was reorg’d out, it probably doesn’t matter, but might be nice to fix.

luke-jr commented at 0:27 am on March 24, 2022: member

Proposed changes (feel free to squash): https://github.com/jamesob/bitcoin/compare/2021-11-scanblocks...luke-jr:rpc_scanblocks

DrahtBot added the label Needs rebase on Apr 26, 2022

Sjors commented at 1:17 pm on June 8, 2022: member

Needs rebase:

0rpc/blockchain.cpp:2259:32: error: no viable conversion from 'std::atomic<int>' to 'const UniValue'

I just added .load().

0rpc/blockchain.cpp:2301:56: error: no member named 'get_int' in 'UniValue'
1                block = active_chain[request.params[2].get_int()];

This needs .getInt<int>() now.

I tested with Luke’s patches on top. It found all relevant blocks for the set of wpkh() descriptors I tried it on. Despite having to figure out the rebase stuff, and writing the below “script”, it was still faster than a regular wallet rescan :-)

In order to recover a wallet you can do this:

First get the list of descriptors:

0src/bitcoin-cli -rpcwallet=Test listdescriptors | jq '.descriptors[].desc'

Then call scanblocks, use getblockheader to get the corresponding block heights and call rescanblockchain with the same start and end height.

0src/bitcoin-cli -rpcclienttimeout=0 scanblocks start '["desc1", "desc2", etc]' | jq -r '[.relevant_blocks][][]'  | xargs -I {} sh -c "src/bitcoin-cli getblockheader {} | jq -r '.height'" | xargs -I {} sh -c  "src/bitcoin-cli -rpcwallet=Test rescanblockchain {} {}"

(the xargs incantation might be macOS specific, but it’s hideous anyway…)

Maybe someone can turn this into a less ugly Python contrib script. It would of course be easier if scanblockchain accepted an array of block hashes.

jamesob force-pushed on Jul 5, 2022

DrahtBot removed the label Needs rebase on Jul 5, 2022

jamesob force-pushed on Jul 18, 2022

Sjors approved

Sjors commented at 2:14 pm on July 19, 2022: member

tACK 779a4b30a75ec89693de9fb32a500209f4b2fde3 modulo string escape in documentation

in src/rpc/blockchain.cpp:2189 in bb553d4478 outdated

2185@@ -2168,13 +2186,206 @@ static RPCHelpMan scantxoutset()
2186         result.pushKV("unspents", unspents);
2187         result.pushKV("total_amount", ValueFromAmount(total_in));
2188     } else {
2189-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid command");
2190+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid action argument");

jamesob commented at 2:53 pm on July 19, 2022:

Oops, I think this is a silent merge conflict.

jamesob force-pushed on Sep 7, 2022

jamesob commented at 9:53 pm on September 7, 2022: member

I think I’ve addressed all of @luke-jr, @Sjors’ feedback - let me know if I’ve missed something.

Sjors commented at 9:24 am on September 13, 2022: member

tACK d19cf65daff4792951964e12ae3feb4be43574f8

in src/rpc/blockchain.cpp:2398 in ac018a0e03 outdated

2382+        ret.pushKV("to_height", g_scanfilter_progress_height.load());
2383+        ret.pushKV("relevant_blocks", blocks);
2384+    }
2385+    else {
2386+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid command");
2387+    }

furszy commented at 12:36 pm on September 13, 2022:

This doesn’t seems to be consistent with the RPC command args. Should be “Invalid action”.

But.. what if instead return “invalid command”, we return the help message?

jamesob commented at 1:33 pm on September 16, 2022:

I’m maintaining consistency with scantxoutset here.

furszy approved

furszy commented at 9:05 pm on September 15, 2022: member

Not sure if you will want to cherry-pick the commit here or should I push it in a follow-up PR but the scanblocks flow performance can be improved greatly by calculating the block filters range without traversing the chain block by block.

A bit of explanation:

The current flow walks-through every block in the active chain until hits the chain tip or processes 10k blocks, then calls the lookupFilterRange function to obtain all the filters from that particular range.

That isn’t the best as it traverses the whole chain only to obtain the heights range to lookup the block filters.

Speedup Improvement:

As scanblocks only lookup block filters in the active chain, we can directly calculate the lookup range heights, by using the chain tip, without requiring to traverse the chain block by block.

Have implemented this new flow, and other cleanups as well, here https://github.com/furszy/bitcoin/commit/279beb756467eb8cd79996be6cfe8e5b3c0236f3.

Side from that, code review ACK d19cf65d. Cool stuff ☕ .

theStack commented at 11:06 am on September 16, 2022: contributor

Running the test at the second-last commit (35daf38636a7dcb9a9cbfd25e319d23f775a1dc2) fails:

 0Traceback (most recent call last):
 1  File "/home/honey/bitcoin_prrev/test/functional/test_framework/test_framework.py", line 133, in main
 2    self.run_test()
 3  File "/home/honey/bitcoin_prrev/./test/functional/rpc_scanblocks.py", line 89, in run_test
 4    assert_raises_rpc_error(-8, "Invalid action argument", node.scanblocks, "foobar")
 5  File "/home/honey/bitcoin_prrev/test/functional/test_framework/util.py", line 125, in assert_raises_rpc_error
 6    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
 7  File "/home/honey/bitcoin_prrev/test/functional/test_framework/util.py", line 140, in try_rpc
 8    raise AssertionError(
 9AssertionError: Expected substring not found in error message:
10substring: 'Invalid action argument'
11error message: 'Invalid command'.

Didn’t try to fix it, but I assume that the assert_raises_rpc_error change (s/“Invalid action argument”/“Invalid command”/) shouldn’t be part of the last commit.

jamesob force-pushed on Sep 16, 2022

jamesob commented at 1:34 pm on September 16, 2022: member

@theStack thanks; that change accidentally made its way into the wrong commit. Fixed (no change in the resulting code at branch tip).

theStack approved

theStack commented at 10:48 pm on September 17, 2022: contributor

Tested ACK b770100b8fa2043dd8eaf8fff61eef9b09e1da37

Sjors commented at 12:56 pm on September 23, 2022: member

re-utACK b770100b8fa2043dd8eaf8fff61eef9b09e1da37

Not sure if you will want to cherry-pick the commit here or should I push it in a follow-up PR but the scanblocks flow performance can be improved greatly

Review-wise I’d prefer to keep that for a followup. It’s plenty fast already.

furszy approved

furszy commented at 1:21 pm on September 23, 2022: member

ACK b770100b

Not sure if you will want to cherry-pick the commit here or should I push it in a follow-up PR but the scanblocks flow performance can be improved greatly

Review-wise I’d prefer to keep that for a followup. It’s plenty fast already.

Sounds good.

DrahtBot added the label Needs rebase on Sep 30, 2022

rpc: move-only: consolidate blockchain scan args

For later reuse in `scanblocks`.

a4258f6e81

rpc: add scanblocks - scan for relevant blocks with descriptors

Co-authored-by: James O'Beirne <james.obeirne@gmail.com>

6ef2566b68

test: rpc: add scanblocks functional test

Co-authored-by: James O'Beirne <james.obeirne@gmail.com>

94fe5453c7

fuzz: add scanblocks as safe for fuzzing 626b7c8493

jamesob force-pushed on Oct 4, 2022

jamesob commented at 5:53 pm on October 4, 2022: member

Trivial rebase pushed.

Given the number of (tested) ACKs here it would be nice to get this merged sooner rather than later; it’s been open almost a year.

Sjors commented at 5:57 pm on October 4, 2022: member

On my list to reACK soon(tm)

in src/rpc/blockchain.cpp:2358 in 626b7c8493

2353+        g_scanfilter_progress_height = start_block->nHeight;
2354+
2355+        while (block) {
2356+            node.rpc_interruption_point(); // allow a clean shutdown
2357+            if (g_scanfilter_should_abort_scan) {
2358+                LogPrintf("scanblocks RPC aborted at height %d.\n", block->nHeight);

maflcko commented at 6:14 pm on October 4, 2022:

not sure about unconditional logging here. This will fill the disk and there is no way for a user to disable it, unless they turn off all logging.

Is there a reason to log as opposed to return something to the rpc user?

furszy commented at 2:53 pm on October 12, 2022:

yeah, wouldn’t be bad to return a “completed” flag inside the return object.

in src/rpc/blockchain.cpp:2375 in 626b7c8493

2370+                if (index->LookupFilterRange(start_index->nHeight, block, filters)) {
2371+                    for (const BlockFilter& filter : filters) {
2372+                        // compare the elements-set with each filter
2373+                        if (filter.GetFilter().MatchAny(needle_set)) {
2374+                            blocks.push_back(filter.GetBlockHash().GetHex());
2375+                            LogPrint(BCLog::RPC, "scanblocks: found match in %s\n", filter.GetBlockHash().GetHex());

maflcko commented at 6:16 pm on October 4, 2022:

nit: If this logging is for debugging, maybe put this in the debug category?

Though, I wonder what the use is, given that this will be returned to the RPC user anyway, so there would be no need to persist this in the debug log?

in src/rpc/blockchain.cpp:2369 in 626b7c8493

2364+                CChain& active_chain = chainman.ActiveChain();
2365+                next = active_chain.Next(block);
2366+                if (block == stop_block) next = nullptr;
2367+            }
2368+            if (start_index->nHeight + amount_per_chunk == block->nHeight || next == nullptr) {
2369+                LogPrint(BCLog::RPC, "Fetching blockfilters from height %d to height %d.\n", start_index->nHeight, block->nHeight);

maflcko commented at 6:16 pm on October 4, 2022:

same?

DrahtBot removed the label Needs rebase on Oct 4, 2022

maflcko commented at 6:18 pm on October 4, 2022: member

left some nits, feel free to ignore

jamesob commented at 5:05 pm on October 12, 2022: member

Unless there are any correctness problems I’m going to leave this PR as-is; 3 solid ACKs, so would be nice to get this merged.

furszy approved

furszy commented at 2:00 pm on October 13, 2022: member

diff re-ACK 626b7c8

I can tackle all the remaining nits on the follow-up PR that I’m planning to do (https://github.com/bitcoin/bitcoin/pull/23549#pullrequestreview-1105712566)

achow101 merged this on Oct 13, 2022

achow101 closed this on Oct 13, 2022

kouloumos commented at 2:59 pm on October 13, 2022: contributor

Does this also need release notes? edit: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/scanblocks-RPC-release-notes

maflcko referenced this in commit 3f1f5f6f1e on Oct 14, 2022

maflcko referenced this in commit 6d40484684 on Oct 21, 2022

sidhujag referenced this in commit 779df85258 on Oct 23, 2022

sidhujag referenced this in commit 019acaa084 on Oct 23, 2022

sidhujag referenced this in commit 882dc32022 on Oct 23, 2022

achow101 referenced this in commit 0eae93e65f on May 1, 2023

sidhujag referenced this in commit 51643513d2 on May 1, 2023

luke-jr referenced this in commit 0dd93fb812 on Jul 19, 2023

luke-jr referenced this in commit 2671e15add on Jul 19, 2023

luke-jr referenced this in commit 56765318a5 on Jul 19, 2023

luke-jr referenced this in commit 2881e412bb on Jul 19, 2023

bitcoin locked this on Oct 13, 2023

Add scanblocks RPC call (attempt 2) #23549

Original PR description

Why is this useful?

Conflicts

A bit of explanation:

Speedup Improvement: