rpc: simplify scan blocks

furszy commented at 12:38 pm on December 31, 2022: member

Coming from #23549#pullrequestreview-1105712566

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.

DrahtBot commented at 12:38 pm on December 31, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, achow101

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.

DrahtBot added the label RPC/REST/ZMQ on Dec 31, 2022

DrahtBot added the label Needs rebase on Jan 16, 2023

furszy force-pushed on Jan 24, 2023

DrahtBot removed the label Needs rebase on Jan 24, 2023

achow101 requested review from achow101 on Apr 25, 2023

achow101 requested review from TheCharlatan on Apr 25, 2023

achow101 requested review from aureleoules on Apr 25, 2023

in src/rpc/blockchain.cpp:2406 in 892335c5ac outdated

2406+                if (!stop_block || stop_block->nHeight < start_index->nHeight) {
2407                     throw JSONRPCError(RPC_MISC_ERROR, "Invalid stop_height");
2408                 }
2409+            } else {
2410+                // No stop block provided, stop at the chain tip.
2411+                stop_block = active_chain.Tip();

TheCharlatan commented at 9:50 am on April 30, 2023:

Isn’t this set already in line 2392?

furszy commented at 6:09 pm on April 30, 2023:

yeah, thanks.

in src/rpc/blockchain.cpp:2406 in 892335c5ac outdated

2410+                // No stop block provided, stop at the chain tip.
2411+                stop_block = active_chain.Tip();
2412             }
2413         }
2414-        CHECK_NONFATAL(block);
2415+        CHECK_NONFATAL(start_index);

TheCharlatan commented at 9:51 am on April 30, 2023:

To me the semantics of stop_block seem very similar. How about checking it as well?

furszy commented at 6:11 pm on April 30, 2023:

sounds good

in src/rpc/blockchain.cpp:2475 in a907887acc outdated

2473@@ -2474,8 +2474,9 @@ static RPCHelpMan scanblocks()
2474         } while (start_index != stop_block);
2475 
2476         ret.pushKV("from_height", start_block_height);
2477-        ret.pushKV("to_height", stop_block->nHeight);
2478+        ret.pushKV("to_height", start_index->nHeight); // start_index is always the last scanned block here

TheCharlatan commented at 10:47 am on April 30, 2023:

For reference,end_range->nHeight would be a bit easier to read, but is not guaranteed to have a value.

furszy commented at 6:21 pm on April 30, 2023:

yeah, the user could have aborted the process at the very beginning.

maybe could change it to:

0end_range ? end_range->nHeight : start_index->nHeight;

but I’m not so sure that this makes code easier to read.

TheCharlatan commented at 10:49 am on April 30, 2023: contributor

Concept ACK

Does this require a release note for the s/filtertype/filter_type/ change?

furszy commented at 6:04 pm on April 30, 2023: member

Thanks for the review TheCharlatan.

Does this require a release note for the s/filtertype/filter_type/ change?

Not necessarily. We branched-off v25 and haven’t released yet. It would just need to be back ported. But.. will just drop that commit. The change made more sense back in December just after merging #23549.

rpc: scanblocks, do not traverse the whole chain block by block

The current flow walks-through every block in the active chain until
hits the chain tip or processes 10k blocks, then calls
`lookupFilterRange()` to obtain all the 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.

ce50acc54f

rpc: scanblocks, add "completed" flag to the result obj

To tell the user whether the process was aborted or not.

Plus, as the process can be aborted prior to the end range,
have also changed the "to_height" result value to return the
last scanned block instead of the end range block.

b922f6b526

furszy force-pushed on Apr 30, 2023

TheCharlatan approved

TheCharlatan commented at 7:13 am on May 1, 2023: contributor

ACK b922f6b5262884f42d7483f1e9af35650bdb53a7

achow101 commented at 1:02 pm on May 1, 2023: member

ACK b922f6b5262884f42d7483f1e9af35650bdb53a7

DrahtBot removed review request from achow101 on May 1, 2023

achow101 merged this on May 1, 2023

achow101 closed this on May 1, 2023

furszy deleted the branch on May 1, 2023

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

bitcoin locked this on Apr 30, 2024

rpc: simplify scan blocks #26780

Reviews

Conflicts