Add scanblocks RPC call (attempt 2) #23549

jamesob · 2021-11-18T21:56:52Z

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 · 2021-11-18T22:14:31Z

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 · 2021-11-18T22:18:47Z

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

Will test it this weekend.

DrahtBot · 2021-11-18T23:02:34Z

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.

src/rpc/blockchain.cpp

ghost · 2021-11-21T21:43:29Z

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

$ bitcoin-cli scanblocks start '["addr(mzrj4QmPhk98vc2yQw42uCsgwfBjVzPPLM)"]' 1000000
error code: -1
error message:
Index 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 · 2021-11-21T23:21:14Z

I tried the syntax based on example mentioned in PR description and it gives me an error:
$ bitcoin-cli scanblocks start '["addr(mzrj4QmPhk98vc2yQw42uCsgwfBjVzPPLM)"]' 1000000
error code: -1
error message:
Index 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 · 2021-11-22T02:14:59Z

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

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

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

$ 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 · 2021-11-24T19:00:44Z

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?

theStack

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

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

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.

src/rpc/blockchain.cpp

jamesob · 2021-12-02T21:51:58Z

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!

ghost

.

ghost

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 · 2021-12-04T17:54:12Z

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

ghost · 2021-12-04T17:57:53Z

@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 · 2021-12-04T17:58:30Z

@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 · 2021-12-04T02:45:35Z

src/rpc/blockchain.cpp

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


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

Not sure what you mean here.

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

src/rpc/blockchain.cpp

test/functional/rpc_scanblocks.py

src/rpc/blockchain.cpp

luke-jr · 2022-03-23T22:18:19Z

src/rpc/blockchain.cpp

+                }
+            }
+            if (!request.params[3].isNull()) {
+                stop_block = active_chain[request.params[3].get_int()];


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 · 2022-03-24T00:27:44Z

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

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

jamesob · 2022-10-04T17:53:47Z

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 · 2022-10-04T17:57:21Z

On my list to reACK soon(tm)

maflcko

left some nits, feel free to ignore

maflcko · 2022-10-04T18:14:37Z

src/rpc/blockchain.cpp

+        while (block) {
+            node.rpc_interruption_point(); // allow a clean shutdown
+            if (g_scanfilter_should_abort_scan) {
+                LogPrintf("scanblocks RPC aborted at height %d.\n", block->nHeight);


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?

yeah, wouldn't be bad to return a "completed" flag inside the return object.

maflcko · 2022-10-04T18:16:27Z

src/rpc/blockchain.cpp

+                        // compare the elements-set with each filter
+                        if (filter.GetFilter().MatchAny(needle_set)) {
+                            blocks.push_back(filter.GetBlockHash().GetHex());
+                            LogPrint(BCLog::RPC, "scanblocks: found match in %s\n", filter.GetBlockHash().GetHex());


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?

maflcko · 2022-10-04T18:16:36Z

src/rpc/blockchain.cpp

+                if (block == stop_block) next = nullptr;
+            }
+            if (start_index->nHeight + amount_per_chunk == block->nHeight || next == nullptr) {
+                LogPrint(BCLog::RPC, "Fetching blockfilters from height %d to height %d.\n", start_index->nHeight, block->nHeight);


jamesob · 2022-10-12T17:05:11Z

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

diff re-ACK 626b7c8

I can tackle all the remaining nits on the follow-up PR that I'm planning to do (#23549 (review))

kouloumos · 2022-10-13T14:59:54Z

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

ae3626e test: use MiniWallet for rpc_scanblocks.py (Sebastian Falbesoner) Pull request description: This is a small follow-up for bitcoin#23549 which introduced `scanblocks`. Since that RPC doesn't need the wallet, we can switch the functional test to use MiniWallet. ACKs for top commit: MarcoFalke: review ACK ae3626e Tree-SHA512: e0b0088103e059b29719299c58fd5173b1cff58cb73025a9a33ad493cd0ac50ba25a5f790e471c00a09b4dca80f3c011174ca4e0c8f34746c39831f5823dc8ba

ff138f9 doc: add `scanblocks` to list of descriptor RPCs (Sebastian Falbesoner) Pull request description: This is a tiny documentation follow-up to #23549. ACKs for top commit: aureleoules: ACK ff138f9 shaavan: ACK ff138f9 Tree-SHA512: cc45b78c13ec4aa5bac688648f8e83f04a9ae54376e67371b280428f0253e2585cf0575fa51e3835d4c13c940559bfcdd88d7504bf97a81b2a73bb34a0db7577

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

ae3626e test: use MiniWallet for rpc_scanblocks.py (Sebastian Falbesoner) Pull request description: This is a small follow-up for bitcoin#23549 which introduced `scanblocks`. Since that RPC doesn't need the wallet, we can switch the functional test to use MiniWallet. ACKs for top commit: MarcoFalke: review ACK ae3626e Tree-SHA512: e0b0088103e059b29719299c58fd5173b1cff58cb73025a9a33ad493cd0ac50ba25a5f790e471c00a09b4dca80f3c011174ca4e0c8f34746c39831f5823dc8ba

ff138f9 doc: add `scanblocks` to list of descriptor RPCs (Sebastian Falbesoner) Pull request description: This is a tiny documentation follow-up to bitcoin#23549. ACKs for top commit: aureleoules: ACK ff138f9 shaavan: ACK ff138f9 Tree-SHA512: cc45b78c13ec4aa5bac688648f8e83f04a9ae54376e67371b280428f0253e2585cf0575fa51e3835d4c13c940559bfcdd88d7504bf97a81b2a73bb34a0db7577

b922f6b rpc: scanblocks, add "completed" flag to the result obj (furszy) ce50acc rpc: scanblocks, do not traverse the whole chain block by block (furszy) Pull request description: Coming from bitcoin/bitcoin#23549 (review) The current `scanblocks` flow walks-through every block in the active chain until hits the chain tip or processes 10k blocks, then calls `lookupFilterRange` function to obtain all filters from that particular range. This is only done to obtain the heights range to look up the block filters. Which is unneeded. 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. ACKs for top commit: achow101: ACK b922f6b TheCharlatan: ACK b922f6b Tree-SHA512: 0587e6d9cf87a59184adb2dbc26a4e2bce3a16233594c6c330f69feb49bf7dc63fdacf44fc20308e93441159ebc604c63eb7de19204d3e745a2ff16004892b45

b922f6b rpc: scanblocks, add "completed" flag to the result obj (furszy) ce50acc rpc: scanblocks, do not traverse the whole chain block by block (furszy) Pull request description: Coming from bitcoin#23549 (review) The current `scanblocks` flow walks-through every block in the active chain until hits the chain tip or processes 10k blocks, then calls `lookupFilterRange` function to obtain all filters from that particular range. This is only done to obtain the heights range to look up the block filters. Which is unneeded. 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. ACKs for top commit: achow101: ACK b922f6b TheCharlatan: ACK b922f6b Tree-SHA512: 0587e6d9cf87a59184adb2dbc26a4e2bce3a16233594c6c330f69feb49bf7dc63fdacf44fc20308e93441159ebc604c63eb7de19204d3e745a2ff16004892b45

For later reuse in `scanblocks`. Github-Pull: bitcoin#23549 Rebased-From: a4258f6

Co-authored-by: James O'Beirne <james.obeirne@gmail.com> Github-Pull: bitcoin#23549 Rebased-From: 6ef2566

Co-authored-by: James O'Beirne <james.obeirne@gmail.com> Github-Pull: bitcoin#23549 Rebased-From: 94fe545

Github-Pull: bitcoin#23549 Rebased-From: 626b7c8

jamesob mentioned this pull request Nov 18, 2021

Add scanblocks RPC call #20664

Closed

jamesob force-pushed the 2021-11-scanblocks branch from c0c2115 to 1e5ec5f Compare November 18, 2021 22:01

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

benthecarman reviewed Nov 19, 2021

View reviewed changes

src/rpc/blockchain.cpp Show resolved Hide resolved

theStack reviewed Nov 29, 2021

View reviewed changes

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

jamesob force-pushed the 2021-11-scanblocks branch from c1e71fb to e1c8918 Compare December 2, 2021 21:51

ghost approved these changes Dec 4, 2021

View reviewed changes

ghost suggested changes Dec 4, 2021

View reviewed changes

luke-jr suggested changes Dec 5, 2021

View reviewed changes

DrahtBot mentioned this pull request Jan 26, 2022

rpc: add require_checksum flag to deriveaddresses #24162

Closed

DrahtBot mentioned this pull request Feb 5, 2022

p2p: Erlay support signaling #23443

Merged

luke-jr reviewed Mar 23, 2022

View reviewed changes

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

luke-jr reviewed Mar 23, 2022

View reviewed changes

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

luke-jr reviewed Mar 23, 2022

View reviewed changes

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

luke-jr reviewed Mar 23, 2022

View reviewed changes

This was referenced Apr 23, 2022

[Draft / POC] Silent Payments #24897

Closed

Remove not needed clang-format off comments #24959

Merged

DrahtBot added the Needs rebase label Apr 26, 2022

jonasschnelli and others added 2 commits October 4, 2022 13:51

test: rpc: add scanblocks functional test

94fe545

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

fuzz: add scanblocks as safe for fuzzing

626b7c8

jamesob force-pushed the 2021-11-scanblocks branch from b770100 to 626b7c8 Compare October 4, 2022 17:51

DrahtBot removed the Needs rebase label Oct 4, 2022

maflcko reviewed Oct 4, 2022

View reviewed changes

DrahtBot mentioned this pull request Oct 8, 2022

p2p: Fill reconciliation sets and request reconciliation (Erlay) #26283

Closed

furszy approved these changes Oct 13, 2022

View reviewed changes

achow101 merged commit bc2b1f0 into bitcoin:master Oct 13, 2022

theStack mentioned this pull request Oct 13, 2022

test: use MiniWallet for rpc_scanblocks.py #26307

Merged

ryanofsky mentioned this pull request Oct 13, 2022

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

theStack mentioned this pull request Oct 20, 2022

doc: add scanblocks to list of descriptor RPCs #26352

Merged

furszy mentioned this pull request Dec 31, 2022

rpc: simplify scan blocks #26780

Merged

This was referenced May 24, 2023

25.0 Final Changes #27686

Merged

25.0 RC Testing Guide Feedback #27736

Closed

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jul 19, 2023

rpc: move-only: consolidate blockchain scan args

0dd93fb

For later reuse in `scanblocks`. Github-Pull: bitcoin#23549 Rebased-From: a4258f6

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jul 19, 2023

rpc: add scanblocks - scan for relevant blocks with descriptors

2671e15

Co-authored-by: James O'Beirne <james.obeirne@gmail.com> Github-Pull: bitcoin#23549 Rebased-From: 6ef2566

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jul 19, 2023

test: rpc: add scanblocks functional test

5676531

Co-authored-by: James O'Beirne <james.obeirne@gmail.com> Github-Pull: bitcoin#23549 Rebased-From: 94fe545

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jul 19, 2023

fuzz: add scanblocks as safe for fuzzing

2881e41

Github-Pull: bitcoin#23549 Rebased-From: 626b7c8

bitcoin locked and limited conversation to collaborators Oct 13, 2023

Add scanblocks RPC call (attempt 2) #23549

Add scanblocks RPC call (attempt 2) #23549

Uh oh!

Conversation

jamesob commented Nov 18, 2021

Original PR description

Why is this useful?

Uh oh!

jamesob commented Nov 18, 2021

Uh oh!

ghost commented Nov 18, 2021

Uh oh!

DrahtBot commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

ghost commented Nov 21, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjahr commented Nov 21, 2021

Uh oh!

ghost commented Nov 22, 2021

Uh oh!

benthecarman commented Nov 24, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob commented Dec 2, 2021

Uh oh!

ghost left a comment • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

sipa commented Dec 4, 2021

Uh oh!

ghost commented Dec 4, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Dec 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr Dec 4, 2021

Choose a reason for hiding this comment

Uh oh!

jamesob Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

luke-jr Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

luke-jr Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Mar 24, 2022

Uh oh!

jamesob commented Oct 4, 2022

Uh oh!

Sjors commented Oct 4, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 18, 2021 •

edited

Loading

ghost commented Nov 21, 2021 •

edited by ghost

Loading

ghost left a comment •

edited by ghost

Loading

ghost commented Dec 4, 2021 •

edited by ghost

Loading

sipa commented Dec 4, 2021 •

edited

Loading

maflcko Oct 4, 2022 •

edited

Loading

kouloumos commented Oct 13, 2022 •

edited

Loading