rpc: add revelant_blocks to scanblocks status #30713

pull tdb3 wants to merge 2 commits into bitcoin:master from tdb3:relevant_blocks_in_scanblocks_status changing 2 files +23 −3
  1. tdb3 commented at 1:29 pm on August 25, 2024: contributor

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

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

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

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

    or (to show snapshots over time)

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

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

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

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

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30713.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK pablomartin4btc
    Concept ACK jamesob

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

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label RPC/REST/ZMQ on Aug 25, 2024
  4. tdb3 renamed this:
    rpc: add revelant_blocks to scanblocks status
    rpc: add `revelant_blocks` to `scanblocks status`
    on Aug 25, 2024
  5. tdb3 force-pushed on Aug 25, 2024
  6. DrahtBot added the label CI failed on Aug 25, 2024
  7. DrahtBot commented at 1:38 pm on August 25, 2024: contributor

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

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  8. DrahtBot removed the label CI failed on Aug 25, 2024
  9. jamesob commented at 10:59 pm on August 25, 2024: contributor
    Concept ACK. This’d be useful, especially in conjunction with #30708.
  10. in src/rpc/blockchain.cpp:2592 in 6bac76f25e outdated 
    2562@@ -2550,7 +2563,8 @@ static RPCHelpMan scanblocks()
2563 
2564         ret.pushKV("from_height", start_block_height);
2565         ret.pushKV("to_height", start_index->nHeight); // start_index is always the last scanned block here
2566-        ret.pushKV("relevant_blocks", std::move(blocks));
2567+        LOCK(cs_relevant_blocks);
2568+        ret.pushKV("relevant_blocks", g_relevant_blocks);
    luke-jr commented at 0:52 am on August 27, 2024:
    Since status stops working when the scan completes, it seems like we should clear g_relevant_blocks here?
    tdb3 commented at 2:46 am on August 27, 2024:
    Yes, thank you. Will update.
    maflcko commented at 5:05 am on August 27, 2024:

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

    Not sure if it matters, or worthy to address.

    tdb3 commented at 9:58 pm on September 9, 2024:
    While leaving scanblocks() without resetting g_relevant_blocks isn’t ideal, resetting g_relevant_blocks just before BlockFiltersScanReserver in the start branch seemed to be a good way to prevent status from accidentally seeing an empty g_relevant_blocks (at least in common cases). Please let me know if I’m overlooking a likely concurrency case, and it can be adjusted.
    luke-jr commented at 2:55 am on November 15, 2024:
    Seems trivial to fix properly? https://github.com/luke-jr/bitcoin/commit/56981318da5dc3145e9f244ad0584ef05c815f6b
  11. luke-jr changes_requested
  12. pablomartin4btc commented at 5:01 pm on September 3, 2024: member
    Concept ACK (coming from #30708).
  13. DrahtBot added the label Needs rebase on Sep 3, 2024
  14. rpc: add relevant_blocks to scanblocks status 
    Provides relevant block hashes to status output.
Enables a user to start looking through matching
blocks without having to wait on scan completion.
    bdda0c14d9
  15. tdb3 force-pushed on Sep 9, 2024
  16. DrahtBot removed the label Needs rebase on Sep 9, 2024
  17. tdb3 marked this as ready for review on Sep 9, 2024
  18. doc: add release notes for 30713 39ee30c31f
  19. pablomartin4btc commented at 11:23 am on September 11, 2024: member

    tACK 39ee30c31f5b33a084f420ed49170e669a8f6440

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

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

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

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

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

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

      0/build/src/bitcoin-cli -datadir=${AU_DATADIR} scanblocks start  '["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]' 750000
1{
2  "from_height": 750000,
3  "to_height": 780002,
4  "relevant_blocks": [
5 ],
6  "completed": false,
7  "status": "Abort call performed"
8}
  20. DrahtBot requested review from jamesob on Sep 11, 2024
  21. tdb3 commented at 12:33 pm on September 11, 2024: contributor

    Thanks for reviewing.

    Out of the scope of this PR, having played around start, status, abort subcommands, I’d add some more context in the results of the first 2. Perhaps as “good first issue”.

    • for status subcommand, instead of returning no results when there are no scans in progress: 
      0./build/src/bitcoin-cli -datadir=${AU_DATADIR} scanblocks status
1{
2  "status": "No scan in progress"
3}
    • for start subcommand, when the abort has been triggered and was no completion: 
      0/build/src/bitcoin-cli -datadir=${AU_DATADIR} scanblocks start  '["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]' 750000
1{
2  "from_height": 750000,
3  "to_height": 780002,
4  "relevant_blocks": [
5 ],
6  "completed": false,
7  "status": "Abort call performed"
8}

    These are good ideas for follow-up PRs.

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

    Good idea. Description updated.

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

    Good idea. Description updated.

    Thanks! It looks good.

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

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

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

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

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

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

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

    The existing test for status is pretty minimal,

    Exactly.

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

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

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

    Tested changes on mainnet

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

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

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

    scan completed

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

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

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

  27. luke-jr referenced this in commit 17a01bce24 on Nov 15, 2024
  28. luke-jr referenced this in commit 632c672b3b on Nov 15, 2024
  29. luke-jr commented at 2:56 am on November 15, 2024: member
    Any reason not to scope the new variables? https://github.com/luke-jr/bitcoin/commit/851d354fc4f94695aabd12d21e470dc90d267b93
  30. tdb3 commented at 4:41 am on November 21, 2024: contributor
    Thanks. Planning to touch this up soon.


tdb3 DrahtBot jamesob luke-jr maflcko pablomartin4btc naiyoma furszy


jamesob

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me