Add scanblocks RPC call #20664

jonasschnelli · 2020-12-15T19:40:57Z

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)

DrahtBot · 2020-12-16T06:06:11Z

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.

Sjors

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.

test/functional/rpc_scanblockfilters.py

src/rpc/blockchain.cpp

jonatack

Tested concept ACK

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

src/rpc/blockchain.cpp

test/functional/rpc_scanblockfilters.py

luke-jr · 2021-01-03T21:20:10Z

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 · 2021-01-06T00:11:02Z

Concept ACK

jonasschnelli · 2021-01-06T08:51:14Z

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 · 2021-01-06T10:17:33Z

maybe call it findblockhashes then? Or findblocks even?

Sjors · 2021-01-06T12:03:01Z

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 · 2021-01-06T15:02:20Z

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.

felipsoarez

Concept ACK, not tested

jonasschnelli · 2021-01-08T10:56:50Z

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 · 2021-01-08T16:11:39Z

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 · 2021-01-08T16:59:33Z

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 · 2021-01-08T17:11:57Z

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

Github-Pull: bitcoin#20664 Rebased-From: f22a048

jonasschnelli · 2021-01-29T09:00:51Z

Rebased

kiminuo · 2021-04-23T14:59:39Z

src/rpc/blockchain.cpp

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


Similar to above.

kiminuo · 2021-04-23T15:00:31Z

src/rpc/blockchain.cpp

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


Suggested change

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

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

Fixed in #23549

kiminuo · 2021-04-23T15:12:39Z

src/rpc/blockchain.cpp

+
+        // loop through the scan objects, add scripts to the needle_set
+        GCSFilter::ElementSet needle_set;
+        for (const UniValue& scanobject : request.params[1].get_array().getValues()) {


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?

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.

kiminuo · 2021-04-23T15:27:46Z

src/rpc/blockchain.cpp

+        std::vector<BlockFilter> filters;
+        const CBlockIndex* start_block = block; // for progress reporting
+        const CBlockIndex* last_scanned_block = block;
+        g_scanfilter_should_abort_scan = false;


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.

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.

kiminuo · 2021-04-23T15:32:37Z

test/functional/rpc_scanblockfilters.py

+# Copyright (c) 2021 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test the scanblocks rpc call."""


nit: What about:

Suggested change

"""Test the scanblocks rpc call."""

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

?

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

Fixed in #23549

kiminuo · 2021-04-23T15:34:14Z

test/functional/rpc_scanblockfilters.py

+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal, assert_raises_rpc_error
+
+class scanblocksTest(BitcoinTestFramework):


Nit:

Suggested change

class scanblocksTest(BitcoinTestFramework):

class ScanblocksTest(BitcoinTestFramework):

Fixed in #23549

achow101 · 2021-05-10T19:20:45Z

src/rpc/blockchain.cpp

+                    {"stop_height", RPCArg::Type::NUM, /*default*/ "<tip>", "height to stop to scan"},
+                    {"filtertype", RPCArg::Type::STR, /*default*/ "basic", "The type name of the filter"}
+                },
+                RPCResult{


In 6a69dd2 "Add scanblocks RPC call - scan for relevant blocks with descriptors"

This RPCResult does not reflect the actual results.

Fixed in #23549

achow101 · 2021-05-10T19:26:25Z

src/rpc/blockchain.cpp

+            block = next;
+        }
+        ret.pushKV("from_height", start_block->nHeight);
+        ret.pushKV("to_height", last_scanned_block->nHeight);


In 6a69dd2 "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.

achow101 · 2021-05-10T19:35:14Z

test/functional/rpc_scanblockfilters.py

+        self.nodes[0].sendtoaddress("mkS4HXoTYWRTescLGaUTGbtTTYX5EjJyEE", 1.0) # childkey 5 of tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B
+
+        # mine a block and assure that the mined blockhash is in the filterresult
+        blockhash = self.nodes[0].generate(1)[0]


In 71b7cdb "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:

Suggested change

blockhash = self.nodes[0].generate(1)[0]

blockhash = self.nodes[0].generate(1)[0]

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

Fixed in #23549

theStack · 2021-07-30T14:11:14Z

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

maflcko · 2021-07-31T07:59:59Z

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

Kirandevraj · 2021-08-24T07:17:09Z

Compiled and tested Successfully 6a69dd2 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

Concept ACK

jamesob · 2021-10-05T01:54:23Z

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 · 2021-10-05T21:57:41Z

Rebased here: master...luke-jr:rpc_filterblocks

maflcko · 2021-10-06T11:03:45Z

Should this be marked "up for grabs" @jonasschnelli ?

luke-jr · 2021-10-10T20:55:07Z

test/functional/rpc_scanblockfilters.py

+        # also test the stop height
+        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'])
+
+        # use the stop_height to exclude the relevent block


relevent ==> relevant

Fixed in #23549

jamesob · 2021-10-10T21:19:06Z

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)

jamesob · 2021-11-18T20:16:17Z

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

meshcollider · 2021-12-03T19:33:10Z

Please see #23549

626b7c8 fuzz: add scanblocks as safe for fuzzing (James O'Beirne) 94fe545 test: rpc: add scanblocks functional test (Jonas Schnelli) 6ef2566 rpc: add scanblocks - scan for relevant blocks with descriptors (Jonas Schnelli) a4258f6 rpc: move-only: consolidate blockchain scan args (James O'Beirne) Pull request description: 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) ACKs for top commit: furszy: diff re-ACK 626b7c8 Tree-SHA512: f84e4dcb851b122b39e9700c58fbc31e899cdcf9b587df9505eaf1f45578cc4253e89ce2a45d1ff21bd213e31ddeedbbcad2c80810f46755b30acc17b07e2873

626b7c8 fuzz: add scanblocks as safe for fuzzing (James O'Beirne) 94fe545 test: rpc: add scanblocks functional test (Jonas Schnelli) 6ef2566 rpc: add scanblocks - scan for relevant blocks with descriptors (Jonas Schnelli) a4258f6 rpc: move-only: consolidate blockchain scan args (James O'Beirne) Pull request description: Revives bitcoin#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 bitcoin#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 bitcoin#9483) ACKs for top commit: furszy: diff re-ACK 626b7c8 Tree-SHA512: f84e4dcb851b122b39e9700c58fbc31e899cdcf9b587df9505eaf1f45578cc4253e89ce2a45d1ff21bd213e31ddeedbbcad2c80810f46755b30acc17b07e2873

jonasschnelli added UTXO Db and Indexes RPC/REST/ZMQ labels Dec 15, 2020

This was referenced Dec 16, 2020

rpc: Remove duplicate name and argNames from CRPCCommand #20012

Merged

Allow maintaining the blockfilterindex when using prune #15946

Merged

jonasschnelli mentioned this pull request Dec 17, 2020

Add address-based index (attempt 4?) #14053

Closed

romanz mentioned this pull request Dec 19, 2020

rpc: Add getblocklocations call #20702

Closed

Sjors reviewed Dec 30, 2020

View reviewed changes

test/functional/rpc_scanblockfilters.py Outdated Show resolved Hide resolved

test/functional/rpc_scanblockfilters.py Outdated Show resolved Hide resolved

test/functional/rpc_scanblockfilters.py Outdated Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

Sjors reviewed Dec 30, 2020

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

jonatack reviewed Dec 30, 2020

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

test/functional/rpc_scanblockfilters.py Outdated Show resolved Hide resolved

jonasschnelli force-pushed the 2020/12/filterblocks_rpc branch from b245ecc to 4e87eb7 Compare January 6, 2021 15:45

jonasschnelli changed the title ~~Add scanblockfilters RPC call~~ Add scanblocks RPC call Jan 6, 2021

jonasschnelli force-pushed the 2020/12/filterblocks_rpc branch from 4e87eb7 to 9d7ba40 Compare January 6, 2021 15:49

felipsoarez reviewed Jan 6, 2021

View reviewed changes

jonasschnelli force-pushed the 2020/12/filterblocks_rpc branch 2 times, most recently from c6f0c5d to f22a048 Compare January 8, 2021 10:42

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jan 28, 2021

Add scanblock functional test

0a48874

Github-Pull: bitcoin#20664 Rebased-From: f22a048

DrahtBot added the Needs rebase label Jan 28, 2021

jonasschnelli force-pushed the 2020/12/filterblocks_rpc branch from f22a048 to ab315e5 Compare January 29, 2021 08:56

kiminuo reviewed Apr 23, 2021

View reviewed changes

DrahtBot mentioned this pull request May 7, 2021

rpc: allow dumptxoutset to dump human-readable data #18689

Closed

achow101 reviewed May 10, 2021

View reviewed changes

maflcko removed this from Blockers in High-priority for review Jul 31, 2021

maflcko added the Waiting for author label Jul 31, 2021

darosior reviewed Oct 4, 2021

View reviewed changes

luke-jr reviewed Oct 10, 2021

View reviewed changes

maflcko added Up for grabs and removed Waiting for author labels Oct 11, 2021

jamesob mentioned this pull request Nov 18, 2021

Add scanblocks RPC call (attempt 2) #23549

Merged

maflcko removed the Up for grabs label Nov 19, 2021

meshcollider closed this Dec 3, 2021

jamesob mentioned this pull request Dec 9, 2021

Make rescans faster #23727

Closed

4 tasks

bitcoin locked and limited conversation to collaborators Dec 3, 2022

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

	"""Test the scanblocks rpc call."""
	"""Test the scanblocks RPC call."""

	class scanblocksTest(BitcoinTestFramework):
	class ScanblocksTest(BitcoinTestFramework):

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

Add scanblocks RPC call #20664

Add scanblocks RPC call #20664

Conversation

jonasschnelli commented Dec 15, 2020 • edited

Why is this useful?

DrahtBot commented Dec 16, 2020 • edited

Conflicts

Sjors left a comment • edited

Choose a reason for hiding this comment

jonatack left a comment

Choose a reason for hiding this comment

luke-jr commented Jan 3, 2021

fjahr commented Jan 6, 2021

jonasschnelli commented Jan 6, 2021

flack commented Jan 6, 2021

Sjors commented Jan 6, 2021

jonasschnelli commented Jan 6, 2021

felipsoarez left a comment

Choose a reason for hiding this comment

jonasschnelli commented Jan 8, 2021

Sjors commented Jan 8, 2021

jonasschnelli commented Jan 8, 2021

Sjors commented Jan 8, 2021

jonasschnelli commented Jan 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiminuo Apr 23, 2021 • edited

Choose a reason for hiding this comment

jamesob Nov 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theStack commented Jul 30, 2021

maflcko commented Jul 31, 2021

Kirandevraj commented Aug 24, 2021

darosior left a comment

Choose a reason for hiding this comment

jamesob commented Oct 5, 2021

luke-jr commented Oct 5, 2021

maflcko commented Oct 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesob commented Oct 10, 2021

jamesob commented Nov 18, 2021

meshcollider commented Dec 3, 2021

jonasschnelli commented Dec 15, 2020 •

edited

DrahtBot commented Dec 16, 2020 •

edited

Sjors left a comment •

edited

kiminuo Apr 23, 2021 •

edited

jamesob Nov 18, 2021 •

edited