Add scanblocks RPC call

jonasschnelli commented at 7:40 pm on December 15, 2020: contributor

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)

jonasschnelli added the label UTXO Db and Indexes on Dec 15, 2020

jonasschnelli added the label RPC/REST/ZMQ on Dec 15, 2020

DrahtBot commented at 6:06 am on December 16, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23549 (Add scanblocks RPC call (attempt 2) by jamesob)

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.

in test/functional/rpc_scanblockfilters.py:5 in b245ecc148 outdated

0@@ -0,0 +1,29 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2020 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test the scantxoutset rpc call."""

Sjors commented at 4:14 pm on December 30, 2020:

scanblockfilters

in test/functional/rpc_scanblockfilters.py:12 in b245ecc148 outdated

 7+from test_framework.util import assert_equal
 8+
 9+class ScanblockfiltersTest(BitcoinTestFramework):
10+    def set_test_params(self):
11+        self.num_nodes = 1
12+        self.extra_args = [["-blockfilterindex=1"]]

Sjors commented at 4:15 pm on December 30, 2020:

Maybe add a node without this index and test that the RPC call fails.

jonatack commented at 7:31 pm on December 30, 2020:

Perhaps also assert_raises_rpc_error on

“Unknown filtertype”
“Index is not enabled for filtertype " + filtertype_name
“Invalid stopheight”

in test/functional/rpc_scanblockfilters.py:25 in b245ecc148 outdated

20+        assert_equal(self.nodes[0].scanblockfilters(["addr("+addr_1+")"]), [])
21+        blockhash = self.nodes[0].generate(1)[0]
22+        assert(blockhash in self.nodes[0].scanblockfilters(["addr("+addr_1+")"]))
23+        blockhash_new = self.nodes[0].generate(1)[0]
24+        self.log.info("Test scanblockfilters startheight...")
25+        assert(blockhash not in self.nodes[0].scanblockfilters(["addr("+addr_1+")"], self.nodes[0].getblockheader(blockhash_new)['height']))

Sjors commented at 4:17 pm on December 30, 2020:

This is a bit hard to read, maybe start with start_height = self.nodes[0].getblockheader(blockhash_new)['height']

in src/rpc/blockchain.cpp:2314 in 94722b9fd0 outdated

2309+                    RPCResult::Type::ARR, "", "",
2310+                    {
2311+                        {RPCResult::Type::STR_HEX, "", "The blockhash"},
2312+                    }},
2313+                RPCExamples{
2314+                    HelpExampleCli("scanblockfilters", "[\"addr(bcrt1q4u4nsgk6ug0sqz7r3rj9tykjxrsl0yy4d0wwte)\"] 300000") +

Sjors commented at 4:19 pm on December 30, 2020:

Would be good to add an object descriptor too.

jonatack commented at 7:06 pm on December 30, 2020:

Looks like the example is missing quotes

0                    HelpExampleCli("scanblockfilters", "'[\"addr(bcrt1q4u4nsgk6ug0sqz7r3rj9tykjxrsl0yy4d0wwte)\"]' 300000") +

0$ bitcoin-cli -signet scanblockfilters ["addr(tb1qdydhyacp7tj2asc0u4mhtf6rn65dfjxclrdd7a)"] 0
1error: Error parsing JSON: [addr(tb1qdydhyacp7tj2asc0u4mhtf6rn65dfjxclrdd7a)]

0$ bitcoin-cli -signet scanblockfilters '["addr(tb1qdydhyacp7tj2asc0u4mhtf6rn65dfjxclrdd7a)"]' 0
1[
2  "0000013b318daf6e8d4c531b8293ebf800871f4220fc4b7b25cc6798ec5cb155",
3  "000001003f625cbecd53924713929b5a91b12edf4eb844f33781ecb6bb78e69b",
4  "000000840904ea8ff963d4f776c0175a622228738cdf9ebd481e3ef483ccdff0"
5]

jonasschnelli commented at 3:49 pm on January 6, 2021:

Fixed

Sjors commented at 4:21 pm on December 30, 2020: member

Concept ACK

When I call without a height param it doesn’t actually scan (documentation suggests otherwise).

I was able to find some blocks used by addresses in my own wallet, both receiving on and sending from. If anyone wants to ACK #19136 I’ll test with a ranged descriptor too.

Sadly the RPC call timed out, so it didn’t return the result. You may need to add an abstraction like for rescanwallet where one can query rescans in progress, but also obtain the result later. That might be a lot of extra code though.

#20295 should also help with prune wallet rescans

Don’t forget to drop Conflicts from the second commit message.

Can you also add a test for passing a descriptor object (with range)?

Other RPC methods that involve descriptors require a checksum; we should probably do that here as well.

For a followup it would be nice if descriptors are automatically expanded, so we don’t need to specify a range. This comes with some headaches, see #15845

The documentation should remind the user about false positive block matches.

in src/rpc/blockchain.cpp:2367 in 94722b9fd0 outdated

2365+        {
2366+            LOCK(cs_main);
2367+            next = ChainActive().Next(block);
2368+        }
2369+        if (start_index->nHeight+amount_per_chunk == block->nHeight || next == nullptr) {
2370+            LogPrint(BCLog::RPC, "Fetching blockfilters from height %d to height %d.\n", start_index->nHeight, block->nHeight);

Sjors commented at 4:25 pm on December 30, 2020:

I think this is worth logging in general rather than RPC only, just like rescanwallet. It took a few seconds between issuing the RPC command and seeing these log entries, any idea why?

in src/rpc/blockchain.cpp:2305 in b245ecc148 outdated

2300+                                    {"range", RPCArg::Type::RANGE, /* default */ "1000", "The range of HD chain indexes to explore (either end or [begin,end])"},
2301+                                },
2302+                            },
2303+                        },
2304+                        "[scanobjects,...]"},
2305+                    {"startheight", RPCArg::Type::NUM, /*default*/ "0", "height to start to filter from"},

jonatack commented at 7:13 pm on December 30, 2020:

The actual default start height currently seems to be the tip, not 0.

0block = ::ChainActive().Tip();

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

2316+                },
2317+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
2318+{
2319+    std::string filtertype_name = "basic";
2320+    if (!request.params[2].isNull()) {
2321+        filtertype_name = request.params[1].get_str();

jonatack commented at 7:19 pm on December 30, 2020:

Looks like this should be params[2]

0        filtertype_name = request.params[2].get_str();

In the functional test, suggest asserting that a call with “basic” and another with no arg return the same result:

 0bitcoin-cli -signet scanblockfilters \
 1  '["addr(tb1qdydhyacp7tj2asc0u4mhtf6rn65dfjxclrdd7a)"]' 0 basic
 2[
 3  "0000013b318daf6e8d4c531b8293ebf800871f4220fc4b7b25cc6798ec5cb155",
 4  "000001003f625cbecd53924713929b5a91b12edf4eb844f33781ecb6bb78e69b",
 5  "000000840904ea8ff963d4f776c0175a622228738cdf9ebd481e3ef483ccdff0"
 6]
 7
 8bitcoin-cli -signet scanblockfilters \
 9  '["addr(tb1qdydhyacp7tj2asc0u4mhtf6rn65dfjxclrdd7a)"]' 0 
10[
11  "0000013b318daf6e8d4c531b8293ebf800871f4220fc4b7b25cc6798ec5cb155",
12  "000001003f625cbecd53924713929b5a91b12edf4eb844f33781ecb6bb78e69b",
13  "000000840904ea8ff963d4f776c0175a622228738cdf9ebd481e3ef483ccdff0"
14]

jonatack commented at 7:21 pm on December 30, 2020:

Can be const

0- std::string filtertype_name = "basic";
1- if (!request.params[2].isNull()) {
2-     filtertype_name = request.params[1].get_str();
3- }
4+ const std::string filtertype_name{request.params[2].isNull() ? "basic" : request.params[2].get_str()};

jonasschnelli commented at 8:57 am on January 6, 2021:

Nice. Will adapt this.

jonatack commented at 7:36 pm on December 30, 2020: member

Tested concept ACK

Naming-wise, scanobjects, startheight and filtertype should probably be snakecase.

luke-jr commented at 9:20 pm on January 3, 2021: member

Seems like block filters are just a potential implementation detail/optimisation here, not part of the fundamental concept…

Maybe rename it to scanblocks (and consider supporting a slow scan when the filters are disabled?)

fjahr commented at 0:11 am on January 6, 2021: member

Concept ACK

jonasschnelli commented at 8:51 am on January 6, 2021: contributor

Maybe rename it to scanblocks (and consider supporting a slow scan when the filters are disabled?)

I can do that. But the call doesn’t scan blocks (hence your proposed name scanblocks). It looks for relevant blockhashes based on descriptors. scanblocks doesn’t sound after what this call is doing.

flack commented at 10:17 am on January 6, 2021: contributor

maybe call it findblockhashes then? Or findblocks even?

Sjors commented at 12:03 pm on January 6, 2021: member

I think scanblocks is fine. Without filters it “scans blocks” and returns block hashes. With filter it doesn’t scan raw block data, but it scans filters block by block.

jonasschnelli commented at 3:02 pm on January 6, 2021: contributor

Without filters it “scans blocks” and returns block hashes. With filter it doesn’t scan raw block data, but it scans filters block by block.

Wouldn’t it be inconsistent to have scanblocks (general) in relation to rescanblockchain (wallet)? But yes… using scanblock perhaps is acceptable.

jonasschnelli force-pushed on Jan 6, 2021

jonasschnelli renamed this:
~~Add scanblockfilters RPC call~~
Add scanblocks RPC call
on Jan 6, 2021

jonasschnelli force-pushed on Jan 6, 2021

felipsoarez commented at 4:11 pm on January 6, 2021: none

Concept ACK, not tested

jonasschnelli force-pushed on Jan 7, 2021

jonasschnelli force-pushed on Jan 8, 2021

jonasschnelli commented at 10:56 am on January 8, 2021: contributor

Overhauled this PR. Fixed all the reported issues.

Added the same interface then scantxoutset (with action “start”, “abort”, “status”). Allows to get progress reports via a different RPC call (or abort the current scan). No concurrent scans possible.

Also added stop_height.

Some performance reports on an i7-8700 CPU @ 3.20GHz with NVme disk:

Scanning an pkh xpub (m/0/* & m/1/*, range 0-220 == 440 sPUk) on mainnet from genesis to tip: 2m19.176s
Same xpub, scan only the last three years: 1m0.729s
Same xpub, “catch up”-scan the last 5 weeks (scan the 5040 latest blocks): 0m2.465s

Single sPUk:

Scanning an single address on mainnet from genesis to tip: 1m20.258s
Same address, scan only the last three years: 0m41.519s
Same address, “catch up”-scan the last 5 weeks: 0m1.744s

Sjors commented at 4:11 pm on January 8, 2021: member

Thanks for the update! Unfortunately unlike rescanblockchain we actually need the result of this call. Just having status is not enough to obtain the result if RPC times out. I think we need to store the result somewhere too, perhaps in a way that you can only read once. However if that’s too difficult we could instead recommend that the user pays attention to the start_height and stop_height and calls the RPC in batches for very old wallets.

jonasschnelli commented at 4:59 pm on January 8, 2021: contributor

Thanks for the update! Unfortunately unlike rescanblockchain we actually need the result of this call. Just having status is not enough to obtain the result if RPC times out.

I think we can handle this the same way as scantxoutset (both call are in the same group of how much time they consume). You just need to set the rpctimeout correctly (as one has to do for the three waitfor* calls).

I thought about adding dispatched threads. start could return a UUID where status and abort would take this as an argument. But it seems overengineering this.

Let’s just keep it identical to scantxoutset (they are pretty much identical, one scan the blocks, the other the utxo set).

Sjors commented at 5:11 pm on January 8, 2021: member

Having long running tasks that you can manage via a UUID does sound useful, but I agree it would be overkill in this PR.

luke-jr referenced this in commit 0a488740b3 on Jan 28, 2021

DrahtBot added the label Needs rebase on Jan 28, 2021

jonasschnelli force-pushed on Jan 29, 2021

jonasschnelli commented at 9:00 am on January 29, 2021: contributor

Rebased

DrahtBot removed the label Needs rebase on Jan 29, 2021

in src/rpc/blockchain.cpp:2358 in 7bb2c05e57 outdated

2355+                },
2356+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
2357+{
2358+    UniValue ret(UniValue::VOBJ);
2359+    if (request.params[0].get_str() == "status") {
2360+        BlockFiltersScanReserver reserver;

fjahr commented at 3:06 pm on February 7, 2021:

in 7bb2c05e576f56f9ccfc44f7b95c3a8ebfb9419c:

You can move this line outside the if..elseif, it’s repeated each time.

jonasschnelli commented at 8:57 am on February 18, 2021:

Could.. yes. But since there is no else (unknown action), I tend to keep it at that level.

in src/rpc/blockchain.cpp:2330 in 7bb2c05e57 outdated

2325+                "This call may take serval minutes. Make sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0)",
2326+                {
2327+                    {"action", RPCArg::Type::STR, RPCArg::Optional::NO, "The action to execute\n"
2328+            "                                      \"start\" for starting a scan\n"
2329+            "                                      \"abort\" for aborting the current scan (returns true when abort was successful)\n"
2330+            "                                      \"status\" for progress report (in %) of the current scan"},

fjahr commented at 3:08 pm on February 7, 2021:

in 7bb2c05e576f56f9ccfc44f7b95c3a8ebfb9419c:

nit: add something like “(returns Null if there is no ongoing scan)”

jonasschnelli commented at 9:09 am on February 18, 2021:

Good point. Added.

in test/functional/rpc_scanblockfilters.py:27 in ab315e5294 outdated

23+        addr_2 = "mkS4HXoTYWRTescLGaUTGbtTTYX5EjJyEE" # childkey 5 of tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B
24+        self.nodes[0].sendtoaddress(addr_2, 1.0)
25+
26+        # mine a block and assure that the mined blockhash is in the filterresult
27+        blockhash = self.nodes[0].generate(1)[0]
28+        out = self.nodes[0].scanblocks("start", ["addr("+addr_1+")"])

fjahr commented at 3:48 pm on February 7, 2021:

in ab315e5294bdd01c839463b0736a67a326ba38b5:

nit: Could put ["addr("+addr_1+")"] in a variable since it’s reused many times.

in test/functional/rpc_scanblockfilters.py:23 in ab315e5294 outdated

18+        # send 1.0, mempool only
19+        addr_1 = self.nodes[0].getnewaddress()
20+        self.nodes[0].sendtoaddress(addr_1, 1.0)
21+
22+        # send 1.0, mempool only
23+        addr_2 = "mkS4HXoTYWRTescLGaUTGbtTTYX5EjJyEE" # childkey 5 of tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B

fjahr commented at 3:52 pm on February 7, 2021:

in ab315e5294bdd01c839463b0736a67a326ba38b5:

Where you going to use this addr in an assert explicitly? I don’t think it is at the moment.

in test/functional/rpc_scanblockfilters.py:2 in ab315e5294 outdated

0@@ -0,0 +1,74 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2020 The Bitcoin Core developers

fjahr commented at 3:55 pm on February 7, 2021:

in ab315e5294bdd01c839463b0736a67a326ba38b5:

nit: 2020 - 2021

in src/rpc/blockchain.cpp:2399 in 7bb2c05e57 outdated

2396+        const CBlockIndex* block = nullptr;
2397+        const CBlockIndex* stop_block = nullptr;
2398+        {
2399+            LOCK(cs_main);
2400+            block = ::ChainActive().Genesis();
2401+            stop_block = ::ChainActive().Tip();

fjahr commented at 4:06 pm on February 7, 2021:

in 7bb2c05e576f56f9ccfc44f7b95c3a8ebfb9419c:

Could put these into else blocks of the respective options so these calls only run when needed.

jonasschnelli commented at 9:02 am on February 18, 2021:

Yes. That would be possible. But IMO it makes the code hard(er) to read and I guess performance wise is it almost no impact.

in src/rpc/blockchain.cpp:2417 in 7bb2c05e57 outdated

2412+                }
2413+            }
2414+        }
2415+        CHECK_NONFATAL(block);
2416+
2417+        // loop through the scan objects, add scripts to the element_set

fjahr commented at 4:07 pm on February 7, 2021:

in 7bb2c05e576f56f9ccfc44f7b95c3a8ebfb9419c:

Is this meant to say needle_set?

jonasschnelli commented at 9:02 am on February 18, 2021:

Good catch. Fixed.

in src/rpc/blockchain.cpp:2472 in 7bb2c05e57 outdated

2469+            }
2470+            last_scanned_block = block;
2471+            block = next;
2472+        }
2473+        ret.pushKV("from_height", start_block->nHeight);
2474+        ret.pushKV("to_height", last_scanned_block->nHeight);

fjahr commented at 4:25 pm on February 7, 2021:

in 7bb2c05e576f56f9ccfc44f7b95c3a8ebfb9419c:

I think you could use g_scanfilter_progress_height here and remove last_scanned_block completely.

fjahr commented at 4:32 pm on February 7, 2021:

Actually, I think it would have to be used because otherwise reported results could be inconsistent in case of the abort because it’s not really the last scanned block but the last block that the while(block) loop has gone through.

jonasschnelli commented at 9:07 am on February 18, 2021:

Not sure about this. The interruption point is at the beginning of the while loop. Processing blocks (scanning filters) can’t be done without setting last_scanned_block = block;. Shouldn’t therefore last_scanned_block always point to stopindex of LookupFilterRange() (a.k.a. the last scanned block)? Or do I miss something?

fjahr commented at 0:14 am on March 5, 2021:

Sorry, that I didn’t get around to answer earlier. I am trying to simplify so please ignore if I get some minor details wrong: Let’s say we are scanning blocks 10.000 - 30.000. The while loop runs through every block and with each block last_scanned_block is updated. But no scanning is actually happening. At 20.000 it does the actual scanning for the first time, blocks 10k-20k. So at height 20k last_scanned_block is actually in sync with that, up to 20k was scanned and the last_scanned_block is 20k. From the next block on last_scanned_block increments with every loop again and is out of sync with what was actually synced. So if the interruption point is hit at, for example, 25k, then to_height would say 25k but actually, the scan was only done up to 20k. One very simple fix would be to move last_scanned_block = block; just 2 lines higher so it is only updated after an actual scan. But then it is just reports the same as g_scanfilter_progress_height so I think it can be dropped altogether. I don’t see last_scanned_block being used anywhere aside for the to_height return value usage in this line here, so I am not sure what you mean by “it can’t be done without it”. And just fundamentally I don’t see why this would not be the same: The progress height and the last scanned block are the same value to me.

I just realized I am writing a whole paragraph for a 3 line change, code is probably much easier to reason about :) https://github.com/fjahr/bitcoin/commit/16c7cb5b67e95ba1c8050eacfd4099fad108acf3 Tests pass.

achow101 commented at 7:26 pm on May 10, 2021:

In 6a69dd267e4b96e6657de7b77521311994b2a902 “Add scanblocks RPC call - scan for relevant blocks with descriptors”

I agree with @fjahr here. last_scanned_block isn’t actually the last block we have scanned, and if we abort, then to_height does not accurately reflect where we have actually scanned up to. It would be better to move it up into the if the guards the actual scanning. But in that case, it’s the same as g_scanfilter_progress_height, so it can just be removed.

jamesob commented at 9:57 pm on November 18, 2021:

Fixed in #23549

in src/rpc/blockchain.cpp:2447 in 7bb2c05e57 outdated

2442+            {
2443+                LOCK(cs_main);
2444+                next = ChainActive().Next(block);
2445+                if (block == stop_block) next = nullptr;
2446+            }
2447+            if (start_index->nHeight+amount_per_chunk == block->nHeight || next == nullptr) {

fjahr commented at 4:41 pm on February 7, 2021:

in 7bb2c05e576f56f9ccfc44f7b95c3a8ebfb9419c:

formatting nit: spaces aroung the +

jonasschnelli commented at 9:08 am on February 18, 2021:

Fixed.

fjahr commented at 5:03 pm on February 7, 2021: member

Concept ACK

Will do some manual testing soon.

jonasschnelli force-pushed on Feb 18, 2021

jonasschnelli commented at 9:09 am on February 18, 2021: contributor

Thanks @fjahr for the review. Fixed the reported points (see also answer #20664 (review)).

Sjors commented at 4:44 pm on February 18, 2021: member

CI is unhappy. The “prune wallet rescans” comment in the description needs an update. Maybe add test for a pruned chain?

~When I call scanblocks status the progress fields updates every now and then, but the current_height field is stuck at the initial height.~

When I called abort it returned true, but then when I called status again it was still there. I guess that’s because the abort command is asynchronous.

in src/rpc/blockchain.cpp:2323 in e0ea89f1ab outdated

2318+
2319+static RPCHelpMan scanblocks()
2320+{
2321+    return RPCHelpMan{"scanblocks",
2322+                "\nReturn relevant blockhashes for given descriptors.\n"
2323+                "This call may take serval minutes. Make sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0)",

Sjors commented at 5:10 pm on February 18, 2021:

Maybe add a hint here that -debug=rpc will show progress in the log (this behavior is different from rescanblockchain which always prints to the log)

in src/rpc/blockchain.cpp:2476 in e0ea89f1ab outdated

2471+        ret.pushKV("from_height", start_block->nHeight);
2472+        ret.pushKV("to_height", last_scanned_block->nHeight);
2473+        ret.pushKV("relevant_blocks", blocks);
2474+    }
2475+    else {
2476+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid command");

Sjors commented at 5:13 pm on February 18, 2021:

nit: “Invalid action argument”

in src/rpc/blockchain.cpp:2452 in e0ea89f1ab outdated

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

Sjors commented at 5:30 pm on February 18, 2021:

Maybe add (height)

jonasschnelli commented at 9:55 am on March 4, 2021:

Would require to fetch the height from the hash (just for the sake of logging).

Sjors commented at 5:38 pm on February 18, 2021: member

73077f1b795d1b3325896b01018de63fcf700ddd looks mostly good to me. I was able to test as well with some descriptors.

Add scanblocks RPC call - scan for relevant blocks with descriptors 6a69dd267e

Unify "invalid action argument" error (fix scantxoutset) 4c8ac1c6c5

jonasschnelli force-pushed on Mar 4, 2021

Add scanblock functional test 71b7cdb460

jonasschnelli force-pushed on Mar 4, 2021

jonasschnelli commented at 4:19 pm on March 4, 2021: contributor

Fixed the RPC test failure and implemented some of @Sjors suggestions.

jonasschnelli added this to the "Blockers" column in a project

chrisguida commented at 2:03 am on March 12, 2021: none

ACK 71b7cdb460e2d0179aa87bdd0d30a82a821f6d05

I reviewed the code, ran the functional tests, and did some manual testing. This rpc call works great; I was able to scan 1000 addresses from genesis to tip on an M1 Macbook Air in 3 min 27 sec.

The only nit I have is that the bitcoin-cli help scanblocks examples don’t work, but the functions that create the examples (HelpExampleCli and HelpExampleRpc) don’t seem to accept an action argument, and I’m not sure what adding one would break. The other problem is that the examples use double quotes around the json list of scan objects, when the rpc api is expecting single quotes, as in the example in the comment at the top of this PR (that one works!).

I’m super excited for this feature to get merged, as it will enable wallet trackers such as EPS and BWT to be practical with pruned nodes. Is it possible for this to be released in 0.22?

Thanks @jonasschnelli for making this happen!

in src/rpc/blockchain.cpp:2415 in 6a69dd267e outdated

2410+                }
2411+            }
2412+        }
2413+        CHECK_NONFATAL(block);
2414+
2415+        // loop through the scan objects, add scripts to the needle_set

Sjors commented at 8:48 am on March 12, 2021:

Can we move the actual scanning code away from RPC land?

chrisguida commented at 4:03 pm on March 12, 2021:

Was thinking this, I’m guessing we’ll want it abstracted out if we want to be able to perform internal rescans, if, for example, the user decides to add an old wallet.

Sjors commented at 6:10 pm on March 12, 2021:

It would also help with GUI support. I imagine soon (tm) to have a wizard where you can import a BIP39 seed and it recovers coins.

jamesob commented at 9:02 pm on November 18, 2021:

I’d like to handle moving this code out of RPC land in a future change. It isn’t a trivial move (given the global progress state), people are probably going to want unittests if we do the move, and IMO the important short-term objective here is getting fast rescans working ASAP. So if it’s okay with you guys I’d like to save this move for a later PR once.

in test/functional/rpc_scanblockfilters.py:28 in 71b7cdb460

23+        self.nodes[0].sendtoaddress("mkS4HXoTYWRTescLGaUTGbtTTYX5EjJyEE", 1.0) # childkey 5 of tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B
24+
25+        # mine a block and assure that the mined blockhash is in the filterresult
26+        blockhash = self.nodes[0].generate(1)[0]
27+        out = self.nodes[0].scanblocks("start", ["addr("+addr_1+")"])
28+        assert(blockhash in out['relevant_blocks'])

Sjors commented at 9:11 am on March 12, 2021:

This assertion fails frequently on my machine.

in test/functional/rpc_scanblockfilters.py:38 in 71b7cdb460

33+        blockhash_new = self.nodes[0].generate(1)[0]
34+
35+        # make sure the blockhash is not in the filter result if we set the start_height to the just mined block (unlikely to hit a false positive)
36+        assert(blockhash not in self.nodes[0].scanblocks("start", ["addr("+addr_1+")"], self.nodes[0].getblockheader(blockhash_new)['height'])['relevant_blocks'])
37+
38+        # make sure the blockhash is present when using the first mined block as start_height

Sjors commented at 9:12 am on March 12, 2021:

When the above assertion doesn’t fail, then this one tends to fail.

Sjors commented at 9:16 am on March 12, 2021: member

71b7cdb460e2d0179aa87bdd0d30a82a821f6d05 still looks good, but I’m getting frequent test failures on my machine. Frequent, but not always, sometimes the test does pass.

First I thought it was due to me using descriptor wallets (./configure --without-bdb does that). But I added a sanity check for that: https://github.com/Sjors/bitcoin/commit/44c6b38b2878c1c559dd9b1128b978d9e66afe7e

Also, fjahr@16c7cb5’s suggestion makes sense to me.

ghost commented at 2:19 am on April 12, 2021: none

Concept ACK. Will test today.

Two questions:

Can we include -rpcclienttimeout=0 by default if not mentioned by the user because the call can take time? ~~2. Do we have any RPC which scans mempool instead of blocks and return the transactions associated with it?~~ getrawmempool works for mempool

ghost commented at 8:50 pm on April 12, 2021: none

Compiled successfully on Ubuntu. Two functional tests failed:

0feature_config_args.py                             | ✖ Failed  | 34 s
1feature_pruning.py                                 | ✖ Failed  | 480 s

MarcoFalke commented at 4:29 am on April 13, 2021: member

What is the combined log of the test failure? ./test/functional/combine_logs.py

ghost commented at 1:24 pm on April 13, 2021: none

Logs:

feature_block.py : https://ghostbin.com/paste/Q6fRX
feature_pruning.py : https://ghostbin.com/paste/GQP81

in src/rpc/blockchain.cpp:2460 in 71b7cdb460

2456+                }
2457+                start_index = block;
2458+
2459+                // update progress
2460+                int blocks_processed = block->nHeight - start_block->nHeight;
2461+                int total_blocks_to_process = stop_block->nHeight - start_block->nHeight;

kiminuo commented at 2:47 pm on April 23, 2021:

nit: It seems to me that this line can be put out of while cycle.

jamesob commented at 9:57 pm on November 18, 2021:

Fixed in #23549

in src/rpc/blockchain.cpp:2437 in 71b7cdb460

2433+        g_scanfilter_progress = 0;
2434+        g_scanfilter_progress_height = start_block->nHeight;
2435+        while (block) {
2436+            node.rpc_interruption_point(); // allow a clean shutdown
2437+            if (g_scanfilter_should_abort_scan) {
2438+                break;

kiminuo commented at 2:50 pm on April 23, 2021:

Would it be helpful to log that the scanning was aborted?

jamesob commented at 9:57 pm on November 18, 2021:

Fixed in #23549

in src/rpc/blockchain.cpp:2341 in 71b7cdb460

2337+                                    {"range", RPCArg::Type::RANGE, /* default */ "1000", "The range of HD chain indexes to explore (either end or [begin,end])"},
2338+                                },
2339+                            },
2340+                        },
2341+                        "[scanobjects,...]"},
2342+                    {"start_height", RPCArg::Type::NUM, /*default*/ "0", "height to start to filter from"},

kiminuo commented at 2:59 pm on April 23, 2021:

0                    {"start_height", RPCArg::Type::NUM, /*default*/ "0", "Height to start to filter from"},

?

Is filter correct verb here? scan?

jamesob commented at 9:57 pm on November 18, 2021:

Fixed in #23549

in src/rpc/blockchain.cpp:2342 in 71b7cdb460

2338+                                },
2339+                            },
2340+                        },
2341+                        "[scanobjects,...]"},
2342+                    {"start_height", RPCArg::Type::NUM, /*default*/ "0", "height to start to filter from"},
2343+                    {"stop_height", RPCArg::Type::NUM, /*default*/ "<tip>", "height to stop to scan"},

kiminuo commented at 2:59 pm on April 23, 2021:

Similar to above.

in src/rpc/blockchain.cpp:2323 in 71b7cdb460

2319+
2320+static RPCHelpMan scanblocks()
2321+{
2322+    return RPCHelpMan{"scanblocks",
2323+                "\nReturn relevant blockhashes for given descriptors.\n"
2324+                "This call may take serval minutes. Make sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0)",

kiminuo commented at 3:00 pm on April 23, 2021:

0                "This call may take several minutes. Make sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0)",

jamesob commented at 9:57 pm on November 18, 2021:

Fixed in https://github.com/bitcoin/bitcoin/pull/23549

in src/rpc/blockchain.cpp:2417 in 71b7cdb460

2413+        }
2414+        CHECK_NONFATAL(block);
2415+
2416+        // loop through the scan objects, add scripts to the needle_set
2417+        GCSFilter::ElementSet needle_set;
2418+        for (const UniValue& scanobject : request.params[1].get_array().getValues()) {

kiminuo commented at 3:12 pm on April 23, 2021:

For my information: This array seems to unbounded. The bound is probably some maximum size of RPC requests in general. Anyway, I wonder whether it is an issue or not. I guess not because nobody mentioned it here. Could anybody elaborate on this a bit, please?

jamesob commented at 9:03 pm on November 18, 2021:

This is only subject to authenticated RPC user input and isn’t handled in previous RPC calls (e.g. scantxoutset) so if someone wants to DoS their own node with a massive input array that seems okay to me.

in src/rpc/blockchain.cpp:2431 in 71b7cdb460

2427+        const int amount_per_chunk = 10000;
2428+        const CBlockIndex* start_index = block; // for remembering the start of a blockfilter range
2429+        std::vector<BlockFilter> filters;
2430+        const CBlockIndex* start_block = block; // for progress reporting
2431+        const CBlockIndex* last_scanned_block = block;
2432+        g_scanfilter_should_abort_scan = false;

kiminuo commented at 3:27 pm on April 23, 2021:

For my information: Would it make sense to move this line to L2414 and to make the for-loop on L2417 abortable too? Or is it an overkill? I’m not sure how long the for loop can take for “many scanobjects”.

If the for-loop can take a long time then this g_scanfilter_should_abort_scan = false; assignment may mean that a user abort request might be ignored.

jamesob commented at 9:05 pm on November 18, 2021:

This is overkill IMO; even for a long list, EvalDescriptorStringOrObject should be quite fast since it doesn’t rely on any disk IO, unlike the calls below.

in test/functional/rpc_scanblockfilters.py:5 in 71b7cdb460

0@@ -0,0 +1,73 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2021 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test the scanblocks rpc call."""

kiminuo commented at 3:32 pm on April 23, 2021:

nit: What about:

0"""Test the scanblocks RPC call."""

?

It looks like it is mostly written in this way in tests.

jamesob commented at 9:57 pm on November 18, 2021:

Fixed in #23549

in test/functional/rpc_scanblockfilters.py:9 in 71b7cdb460

0@@ -0,0 +1,73 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2021 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test the scanblocks rpc call."""
6+from test_framework.test_framework import BitcoinTestFramework
7+from test_framework.util import assert_equal, assert_raises_rpc_error
8+
9+class scanblocksTest(BitcoinTestFramework):

kiminuo commented at 3:34 pm on April 23, 2021:

Nit:

0class ScanblocksTest(BitcoinTestFramework):

jamesob commented at 9:57 pm on November 18, 2021:

Fixed in #23549

in src/rpc/blockchain.cpp:2345 in 6a69dd267e outdated

2340+                        "[scanobjects,...]"},
2341+                    {"start_height", RPCArg::Type::NUM, /*default*/ "0", "height to start to filter from"},
2342+                    {"stop_height", RPCArg::Type::NUM, /*default*/ "<tip>", "height to stop to scan"},
2343+                    {"filtertype", RPCArg::Type::STR, /*default*/ "basic", "The type name of the filter"}
2344+                },
2345+                RPCResult{

achow101 commented at 7:20 pm on May 10, 2021:

In 6a69dd267e4b96e6657de7b77521311994b2a902 “Add scanblocks RPC call - scan for relevant blocks with descriptors”

This RPCResult does not reflect the actual results.

jamesob commented at 9:57 pm on November 18, 2021:

Fixed in #23549

in test/functional/rpc_scanblockfilters.py:26 in 71b7cdb460

21+
22+        # send 1.0, mempool only
23+        self.nodes[0].sendtoaddress("mkS4HXoTYWRTescLGaUTGbtTTYX5EjJyEE", 1.0) # childkey 5 of tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B
24+
25+        # mine a block and assure that the mined blockhash is in the filterresult
26+        blockhash = self.nodes[0].generate(1)[0]

achow101 commented at 7:35 pm on May 10, 2021:

In 71b7cdb460e2d0179aa87bdd0d30a82a821f6d05 “Add scanblock functional test”

Since the indexes are built asynchronously, it is possible that after generating the block, the block filter index has not updated to include the new block, so scanblocks might not find anything. Instead, we need to wait for it to become synced:

0        blockhash = self.nodes[0].generate(1)[0]
1        self.wait_until(lambda: all(i["synced"] for i in self.nodes[0].getindexinfo().values()))

jamesob commented at 9:58 pm on November 18, 2021:

Fixed in #23549

theStack commented at 2:11 pm on July 30, 2021: member

Concept ACK

There have been various code reviews but no changes since months. @jonasschnelli: are you still working on this PR? (Just checking out before I start to code-review).

MarcoFalke removed this from the "Blockers" column in a project

MarcoFalke added the label Waiting for author on Jul 31, 2021

MarcoFalke commented at 7:59 am on July 31, 2021: member

Removed from high-prio for now, but can be added back any time.

Kirandevraj commented at 7:17 am on August 24, 2021: none

Compiled and tested Successfully 6a69dd267e4b96e6657de7b77521311994b2a902 at Ubuntu 18.04. Started Bitcoin Core in regtest with keypool=5. Created and mined blocks to addresses in descriptor wallet after creating addresses more than the keypool size using getnewaddress. Scanned blocks for all the addresses using scanblocks RPC and were able to get the right blocks that contained the transactions in it for all the addresses.

darosior commented at 12:53 pm on October 4, 2021: member

Concept ACK

jamesob commented at 1:54 am on October 5, 2021: member

Concept ACK. Would love to see this, or any other mechanism that allows performing wallet rescans in ~2min on good hardware, make it in.

luke-jr commented at 9:57 pm on October 5, 2021: member

Rebased here: https://github.com/bitcoin/bitcoin/compare/master...luke-jr:rpc_filterblocks

MarcoFalke commented at 11:03 am on October 6, 2021: member

Should this be marked “up for grabs” @jonasschnelli ?

in test/functional/rpc_scanblockfilters.py:44 in 71b7cdb460

39+        assert(blockhash in self.nodes[0].scanblocks("start", ["addr("+addr_1+")"], self.nodes[0].getblockheader(blockhash)['height'])['relevant_blocks'])
40+
41+        # also test the stop height
42+        assert(blockhash in self.nodes[0].scanblocks("start", ["addr("+addr_1+")"], self.nodes[0].getblockheader(blockhash)['height'], self.nodes[0].getblockheader(blockhash)['height'])['relevant_blocks'])
43+
44+        # use the stop_height to exclude the relevent block

luke-jr commented at 8:55 pm on October 10, 2021:

relevent ==> relevant

jamesob commented at 9:58 pm on November 18, 2021:

Fixed in #23549

jamesob commented at 9:19 pm on October 10, 2021: member

I haven’t started review yet, but given @MarcoFalke has done some previous work on this, we should ensure that this change isn’t subject to the bugs he points out here: #15845 (comment)

MarcoFalke removed the label Waiting for author on Oct 11, 2021

MarcoFalke added the label Up for grabs on Oct 11, 2021

jamesob commented at 8:16 pm on November 18, 2021: member

For what it’s worth, I’m working on resurrecting this PR. Hoping to have a branch pushed by end of day.

MarcoFalke removed the label Up for grabs on Nov 19, 2021

meshcollider commented at 7:33 pm on December 3, 2021: contributor

Please see #23549

meshcollider closed this on Dec 3, 2021

achow101 referenced this in commit bc2b1f0fe2 on Oct 13, 2022

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

DrahtBot locked this on Dec 3, 2022

Add scanblocks RPC call #20664

Why is this useful?

Conflicts