rpc: Return accurate results for scanblocks #26325

aureleoules commented at 2:55 PM on October 17, 2022: member

Implements #26322. Adds a filter_false_positives mode to scanblocks to accurately verify results from blockfilters.

If the option is enabled, pre-results given by blockfilters will be filtered out again by checking vouts and vins of all transactions of the relevant blocks against the given descriptors.

Master

./src/bitcoin-cli -testnet -named scanblocks action=start scanobjects='["addr(tb1qcxf2gv93c26s6mqz7y6etpqdf70zmn67dualgr)"]'
{
  "from_height": 0,
  "to_height": 2376055,
  "relevant_blocks": [
    "000000000001bc35077dec4104e0ab1f667ae27059bd907f9a8fac55c802ae36",
    "00000000000120a9c50542d73248fb7c37640c252850f0cf273134ad9febaf61",
    "0000000000000082f7af3835da8b6146b0bfb243b8842f09c495fa1e74d454ed",
    "0000000000000094c32651728193bfbe91f6789683b8d6ac6ae2d22ebd3cb5d3"
  ]
}

PR (without `filter_false_positives` mode)

Same as master

./src/bitcoin-cli -testnet -named scanblocks action=start scanobjects='["addr(tb1qcxf2gv93c26s6mqz7y6etpqdf70zmn67dualgr)"]' filter_false_positives=false
{
  "from_height": 0,
  "to_height": 2376055,
  "relevant_blocks": [
    "000000000001bc35077dec4104e0ab1f667ae27059bd907f9a8fac55c802ae36",
    "00000000000120a9c50542d73248fb7c37640c252850f0cf273134ad9febaf61",
    "0000000000000082f7af3835da8b6146b0bfb243b8842f09c495fa1e74d454ed",
    "0000000000000094c32651728193bfbe91f6789683b8d6ac6ae2d22ebd3cb5d3"
  ]
}

PR (with `filter_false_positives` mode)

./src/bitcoin-cli -testnet -named scanblocks action=start scanobjects='["addr(tb1qcxf2gv93c26s6mqz7y6etpqdf70zmn67dualgr)"]' filter_false_positives=true
{
  "from_height": 0,
  "to_height": 2376058,
  "relevant_blocks": [
    "0000000000000082f7af3835da8b6146b0bfb243b8842f09c495fa1e74d454ed",
    "0000000000000094c32651728193bfbe91f6789683b8d6ac6ae2d22ebd3cb5d3"
  ]
}

Also adds a test to check that the blockhash of a transaction will be included in the relevant_blocks whether the filter_false_positives mode is enabled or not.

aureleoules commented at 2:57 PM on October 17, 2022: member

I'm wondering whether the accurate mode should be enabled by default. If an application or a user uses the scanblocks RPC, they'll need to filter out false positives anyway, so maybe Bitcoin Core should do it?

Also, is there a way to reproduce false positives? It would be nice to add a test but I haven't figured out how?

aureleoules force-pushed on Oct 17, 2022

DrahtBot added the label RPC/REST/ZMQ on Oct 17, 2022

in src/rpc/blockchain.cpp:2280 in ca139abad1 outdated

2275 | @@ -2244,7 +2276,8 @@ static RPCHelpMan scanblocks()
2276 |              scan_objects_arg_desc,
2277 |              RPCArg{"start_height", RPCArg::Type::NUM, RPCArg::Default{0}, "Height to start to scan from"},
2278 |              RPCArg{"stop_height", RPCArg::Type::NUM, RPCArg::DefaultHint{"chain tip"}, "Height to stop to scan"},
2279 | -            RPCArg{"filtertype", RPCArg::Type::STR, RPCArg::Default{BlockFilterTypeName(BlockFilterType::BASIC)}, "The type name of the filter"}
2280 | +            RPCArg{"filtertype", RPCArg::Type::STR, RPCArg::Default{BlockFilterTypeName(BlockFilterType::BASIC)}, "The type name of the filter"},
2281 | +            RPCArg{"accurate", RPCArg::Type::BOOL, RPCArg::Default{false}, "Enable accurate mode (requires -txindex). Inaccurate mode may result in false positives but is faster."},

w0xlt commented at 2:56 AM on October 18, 2022:

Shouldn't it be optional ?

aureleoules commented at 7:58 AM on October 18, 2022:

It is optional.

w0xlt commented at 2:59 AM on October 18, 2022: contributor

Approach ACK.

w0xlt approved

w0xlt commented at 1:04 PM on October 18, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/26325/commits/ca139abad173b9142189f52e3dd1178684f81c0c

in src/rpc/blockchain.cpp:22 in ca139abad1 outdated

  18 | @@ -19,12 +19,14 @@
  19 |  #include <hash.h>
  20 |  #include <index/blockfilterindex.h>
  21 |  #include <index/coinstatsindex.h>
  22 | +#include <index/txindex.h>

maflcko commented at 1:24 PM on October 18, 2022:

Can't you read the scriptpubkey from the undo data?

theStack commented at 1:55 PM on October 18, 2022:

Isn't undo data only kept for recent blocks in the case of reorgs? Didn't check but I can hardly imagine that we store every scriptPubKey of a spent output twice forever.

aureleoules commented at 2:05 PM on October 18, 2022:

I didn't know about undo data, I pushed a version using it instead of txindex. Can revert if undo data is not reliable :/ EDIT: I looked it up and undo data doesn't seem to ever be pruned so I think this is safe?

theStack commented at 2:27 PM on October 18, 2022:

Seems my suspicion was wrong and we indeed keep undo data forever on a non-pruned node. Sorry for confusing!

andrewtoth commented at 7:54 PM on October 19, 2022:

EDIT: I looked it up and undo data doesn't seem to ever be pruned so I think this is safe?

It does indeed get pruned at the same time as block files https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L557-L565.

aureleoules commented at 10:14 AM on October 26, 2022:

It does indeed get pruned at the same time as block files master/src/node/blockstorage.cpp#L557-L565.

I meant that they are not pruned on non-pruned nodes but you are correct. I've added a check that throws if both pruning and filter_false_positives modes are enabled.

andrewtoth commented at 12:03 PM on October 26, 2022:

But you already added the GetBlockChecked and GetUndoChecked functions which check and throw if pruned. I don't think it's necessary to disable completely if pruned, since the blocks that are found might not have been pruned yet.

aureleoules commented at 1:19 PM on October 26, 2022:

Alright, reverted to 134caa7c25fee8ed16d0c390cf729f3033d06dc4.

aureleoules force-pushed on Oct 18, 2022

in src/rpc/blockchain.cpp:2246 in f2174f7c59 outdated

2241 | +    if (!ReadBlockFromDisk(block, pindex, chainparams.GetConsensus())) {
2242 | +        return false;
2243 | +    }
2244 | +
2245 | +    CBlockUndo block_undo;
2246 | +    if(!UndoReadFromDisk(block_undo, pindex)) {

theStack commented at 3:20 PM on October 18, 2022:

nit: spacing

    if (!UndoReadFromDisk(block_undo, pindex)) {

theStack commented at 3:36 PM on October 18, 2022: contributor

Concept ACK

Also, is there a way to reproduce false positives? It would be nice to add a test but I haven't figured out how?

Discussed the same question this with @furszy just a few days ago talking about #26322, and came to the conclusion that the easiest way to find false-positives is to only use a single block with some random outputs, but increase the needle set (e.g. a ranged descriptor with a large range like 0-10000), until the RPC call returns a block. The found result can then be used in the test. Will tinker a bit around and see if I can come up with a commit that you can include (or in a follow-up).

aureleoules commented at 4:54 PM on October 18, 2022: member

Thanks for the tips @theStack. I tried creating a test as well. I managed to generate false positives using the following test, but it only happens once every few runs. I'll continue to work on it later, but here's my snippet in case you have ideas.

diff --git a/test/functional/rpc_scanblocks.py b/test/functional/rpc_scanblocks.py
index 0ba5f12bd..e7d26a5ef 100755
--- a/test/functional/rpc_scanblocks.py
+++ b/test/functional/rpc_scanblocks.py
@@ -102,6 +102,42 @@ class ScanblocksTest(BitcoinTestFramework):
         # test invalid command
         assert_raises_rpc_error(-8, "Invalid command", node.scanblocks, "foobar")
 
+        # generate block with a lot of random outputs
+        print("Generate block with random transactions")
+        for i in range(500): # should be deterministic and give false positives
+            _, spk, _ = getnewdestination()
+            wallet.send_to(from_node=self.nodes[1], scriptPubKey=spk, amount=400)
+
+        blockhash = self.generate(self.nodes[1], 1)[0]
+        print(blockhash)
+
+        # scan the block with the ranged descriptor
+        height = node.getblockheader(blockhash)['height']
+        desc = f"pkh({parent_key}/*)"
+
+        # begin debug
+        blocks = node.scanblocks(
+            action="start",
+            scanobjects=[{"desc": desc, "range": [0, 200000]}],
+            start_height=height,
+            accurate=False)['relevant_blocks']
+
+        print(blocks)
+        # end debug
+
+        for v in [False, True]:
+            blocks = node.scanblocks(
+                action="start",
+                scanobjects=[{"desc": desc, "range": [0, 200000]}],
+                start_height=height,
+                accurate=v)['relevant_blocks']
+
+            if v:
+                assert_equal(len(blocks), 0)
+            else:
+                assert_equal(len(blocks), 1)
+                assert_equal(blocks[0], blockhash)
+
 
 if __name__ == '__main__':
     ScanblocksTest().main()

</details>

The only part that isn't deterministic is:

for i in range(500): # should be deterministic and give false positives
	_, spk, _ = getnewdestination()
	wallet.send_to(from_node=self.nodes[1], scriptPubKey=spk, amount=400)

Maybe we could use i to generate deterministic scriptPubKeys and hope a certain arrangement always gives false positives? Not sure how reliable this is.

aureleoules commented at 11:24 AM on October 19, 2022: member

I'm able to generate false positives consistently enough with this test by increasing nblocks but the test is very slow and still likely to cause intermittent failures so not worth adding it to the PR.

<details> <summary>Details</summary>

diff --git a/test/functional/rpc_scanblocks.py b/test/functional/rpc_scanblocks.py
index 0ba5f12bd..05779973e 100755
--- a/test/functional/rpc_scanblocks.py
+++ b/test/functional/rpc_scanblocks.py
@@ -102,6 +102,34 @@ class ScanblocksTest(BitcoinTestFramework):
         # test invalid command
         assert_raises_rpc_error(-8, "Invalid command", node.scanblocks, "foobar")
 
+        # generate block with a lot of random outputs
+        print("Generate block with random transactions")
+        height = node.getblockcount() + 1
+        nblocks = 10
+        for i in range(nblocks):
+            for _ in range(550):
+                _, spk, _ = getnewdestination()
+                wallet.send_to(from_node=self.nodes[1], scriptPubKey=spk, amount=400)
+
+            print(f"Generate block {i}/{nblocks}")
+            self.generate(self.nodes[1], 1)[0]
+
+        # scan the block with the ranged descriptor
+        desc = f"pkh({parent_key}/*)"
+        print(desc)
+
+        for v in [False, True]:
+            blocks = node.scanblocks(
+                action="start",
+                scanobjects=[{"desc": desc, "range": [0, 200000]}],
+                start_height=height,
+                accurate=v)['relevant_blocks']
+
+            if v:
+                assert_equal(len(blocks), 0)
+            else:
+                assert len(blocks) > 1
+
 
 if __name__ == '__main__':
     ScanblocksTest().main()

</details>

andrewtoth commented at 6:05 PM on October 19, 2022: contributor

I'm not sure accurate is very descriptive about what this feature does. Would filter_false_positives be a better name for this option?

I'm wondering whether the accurate mode should be enabled by default. If an application or a user uses the scanblocks RPC, they'll need to filter out false positives anyway, so maybe Bitcoin Core should do it?

With this feature enabled Bitcoin Core will be reading from disk and scanning both the block and undo files for each block, which would then be read again when fetched and then scanned by the user. I don't think it would be worth this extra effort unless the user's network latency or scanning code is very slow so that a single false positive block getting to them would be slower than having Bitcoin Core do it for every block. So IMO it should be disabled by default.

in src/rpc/blockchain.cpp:2242 in f2174f7c59 outdated

2234 | @@ -2234,6 +2235,39 @@ class BlockFiltersScanReserver
2235 |      }
2236 |  };
2237 |  
2238 | +static bool CheckBlockFilterMatches(const CBlockIndex* pindex, const GCSFilter::ElementSet& needles, const CChainParams& chainparams)
2239 | +{
2240 | +    CBlock block;
2241 | +    if (!ReadBlockFromDisk(block, pindex, chainparams.GetConsensus())) {
2242 | +        return false;

andrewtoth commented at 7:00 PM on October 19, 2022:

Here and below when reading the undo data, if the read fails then this returns false and the block is recorded as a false positive and won't be returned by scanblocks. These should instead throw a JSONRPCError because the results will not be correct.

andrewtoth commented at 7:03 PM on October 19, 2022: contributor

I believe scanblocks is compatible with a pruned node as long as the block filters were generated before the blocks were pruned (https://github.com/bitcoin/bitcoin/pull/15946). If so, then this mode will fail silently for the pruned blocks and just not return the hashes, even if the blocks can be retrieved by another node. But also more generally if a block read fails for any reason it should throw and not return false.

Also see #26316.

theStack commented at 7:53 PM on October 19, 2022: contributor

I'm able to generate false positives consistently enough with this test by increasing nblocks but the test is very slow and still likely to cause intermittent failures so not worth adding it to the PR.

I digged a bit deeper into the rabbit-hole and ended up opening #26341, adding a testing for a fixed false-positive for the genesis block and also verifying it with the ranged hashes described in BIP158. Feel free to base you PR on that (though I think the PR is already in a pretty good state and it's also fine if the test is add in a follow-up). With the newly introduced helpers, it would be possible to calculate false-positives for newly created blocks at runtime, but at least in my impression that takes too much time for a functional test -- about ~800k scriptPubKeys have to be tried out on average until one of them corresponds to a scriptPubKey already on the block.

aureleoules force-pushed on Oct 20, 2022

aureleoules commented at 10:25 AM on October 20, 2022: member

Thank you @andrewtoth for the review.

Rebased this PR on top of #26341 as suggested by @theStack.
Added a test case to filter false positives
Added a check to not fetch block undo data for the genesis block
Throw if ReadBlockFromDisk or UndoReadFromDisk fails instead of returning false
Renamed accurate mode to filter_false_positives

in src/rpc/blockchain.cpp:2279 in 33793d67bf outdated

2244 | +
2245 | +    CBlockUndo block_undo;
2246 | +    // Genesis block has no undo data
2247 | +    if (pindex->nHeight > 0 && !UndoReadFromDisk(block_undo, pindex)) {
2248 | +        throw JSONRPCError(RPC_DATABASE_ERROR, "Cannot read block undo data from disk");
2249 | +    }

theStack commented at 11:33 PM on October 22, 2022:

Would be nice to use the helper functions GetBlockChecked and GetUndoChecked here to have consistent error messages and RPC return codes and avoid repetition. The only drawback right now is that GetUndoChecked doesn't include the exception for the genesis blocks yet, so the test would fail. Fixed by #19888 or #26369.

aureleoules commented at 9:41 AM on October 24, 2022:

Pushed suggestion https://github.com/bitcoin/bitcoin/compare/33793d67bff6519b6ebdd0b1229d03166c505672..f0b27d2d8bd76e329ef3b90473b14fe57e079c4e

aureleoules force-pushed on Oct 24, 2022

in test/functional/rpc_scanblocks.py:109 in f0b27d2d8b outdated

 104 | +        assert(genesis_blockhash not in node.scanblocks(
 105 | +            "start", [{"desc": f"raw({false_positive_spk.hex()})"}], 0, 0, "basic", True)['relevant_blocks'])
 106 | +
 107 | +
 108 |          # TODO: after an "accurate" mode for scanblocks is implemented (e.g. PR #26325)
 109 |          # check here that it filters out the false-positive

theStack commented at 12:09 PM on October 24, 2022:

These comment lines from the previous commit can be removed now.

aureleoules commented at 1:19 PM on October 24, 2022:

Removed thanks

theStack approved

theStack commented at 12:10 PM on October 24, 2022: contributor

Code-review ACK f0b27d2d8bd76e329ef3b90473b14fe57e079c4e

aureleoules force-pushed on Oct 24, 2022

theStack approved

theStack commented at 1:33 PM on October 24, 2022: contributor

re-ACK 5fb8f8dc49d00b9e2eb0d6a968783a61449ac683 📔

in src/rpc/blockchain.cpp:2284 in 5fb8f8dc49 outdated

2279 | @@ -2244,7 +2280,8 @@ static RPCHelpMan scanblocks()
2280 |              scan_objects_arg_desc,
2281 |              RPCArg{"start_height", RPCArg::Type::NUM, RPCArg::Default{0}, "Height to start to scan from"},
2282 |              RPCArg{"stop_height", RPCArg::Type::NUM, RPCArg::DefaultHint{"chain tip"}, "Height to stop to scan"},
2283 | -            RPCArg{"filtertype", RPCArg::Type::STR, RPCArg::Default{BlockFilterTypeName(BlockFilterType::BASIC)}, "The type name of the filter"}
2284 | +            RPCArg{"filtertype", RPCArg::Type::STR, RPCArg::Default{BlockFilterTypeName(BlockFilterType::BASIC)}, "The type name of the filter"},
2285 | +            RPCArg{"filter_false_positives", RPCArg::Type::BOOL, RPCArg::Default{false}, "Filter false positives. Otherwise they may occur at a rate of 1/M"},

andrewtoth commented at 1:40 PM on October 24, 2022:

Should we document that this RPC requires blockfilterindex=1 and that it is scanning using the block filters? It doesn't seem like this is noted in the help text above. Then we can say here that block filter scanning has a rate of false positives of 1/M, and that using this option will require Bitcoin Core to read all the blocks from disk and scan them, so it will be more expensive setting this option. Also that this option can fail on a pruned node.

theStack commented at 1:58 PM on October 24, 2022:

I agree that the scanblocks RPC documentation right now should be improved, at least the need for blockfilters to use it should be mentioned to avoid confusion. Probably that's a good candidate for an own PR with a focus solely on polishing documentation? (I see there is also no release note yet, this one could also be included there). Happy to review those changes either here in this PR or in a follow-up.

luke-jr commented at 3:37 PM on October 25, 2022:

Shouldn't be a positional argument

aureleoules commented at 4:35 PM on October 25, 2022:

Pushed, see diff https://github.com/bitcoin/bitcoin/compare/5fb8f8dc49d00b9e2eb0d6a968783a61449ac683..134caa7c25fee8ed16d0c390cf729f3033d06dc4.

aureleoules commented at 4:37 PM on October 25, 2022:

I've made the argument RPCArg::Optional::OMITTED_NAMED_ARG, is this what you asked? I'm not familiar with RPC args.

luke-jr commented at 11:00 PM on November 3, 2022:

No. Listing it in order here makes it a positional argument. Look at RPCs with an "options" argument to see how it should be done.

aureleoules commented at 10:40 AM on November 4, 2022:

Ah thanks, I pushed it https://github.com/bitcoin/bitcoin/compare/c39234b070a0d2102af835fda77f3a78d4a631d7..e94d80dd45c0610c3c40b8a6589c5f7f1d329117

in test/functional/rpc_scanblocks.py:103 in 5fb8f8dc49 outdated

  96 | +        assert(genesis_blockhash in node.scanblocks(
  97 | +            "start", [{"desc": f"raw({genesis_coinbase_spk.hex()})"}], 0, 0)['relevant_blocks'])
  98 | +        assert(genesis_blockhash in node.scanblocks(
  99 | +            "start", [{"desc": f"raw({false_positive_spk.hex()})"}], 0, 0)['relevant_blocks'])
 100 | +
 101 | +        # check that the filter_false_positives option works

andrewtoth commented at 2:29 PM on October 25, 2022:

Should we also include a test that breaks if the block containing the false positive gets pruned? Could be done in a follow-up.

aureleoules commented at 4:36 PM on October 25, 2022:

Considering that false positives can only be tested in the genesis block right now and that the genesis block cannot be pruned, I'm not sure how to test this. I will leave this test as a follow-up for a brave soul.

furszy commented at 6:48 PM on October 31, 2022:

could hand-craft the first block on the python test suite side.

aureleoules commented at 2:04 PM on November 1, 2022:

My assumptions were wrong, I thought the genesis block could not get pruned. Added a test case for this https://github.com/bitcoin/bitcoin/compare/fd0ed48ecd9f0d65acc3199a50b3ca6479aa5a3e..c39234b070a0d2102af835fda77f3a78d4a631d7

aureleoules force-pushed on Oct 25, 2022

aureleoules force-pushed on Oct 26, 2022

aureleoules commented at 10:16 AM on October 26, 2022: member

Added release notes
Improved RPC documentation
Made RPC arg filter_false_positives "OMITTED_NAMED_ARG"
~~Added a check that throws Filtering false positives is not supported when running with pruning enabled when pruning and filter_false_positives modes are enabled because undo data may be pruned.~~

aureleoules force-pushed on Oct 26, 2022

achow101 referenced this in commit e25de33e7b on Oct 26, 2022

aureleoules force-pushed on Oct 26, 2022

aureleoules commented at 4:19 PM on October 26, 2022: member

Rebased for #26341. CI error unrelated.

DrahtBot commented at 11:40 PM on October 26, 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	theStack, achow101, furszy
Stale ACK	w0xlt

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26780 (rpc: simplify scan blocks by furszy)
#26700 (test: Wallet interactions with rescanning wallet by aureleoules)

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.

sidhujag referenced this in commit 7faede1033 on Oct 27, 2022

w0xlt approved

w0xlt commented at 5:20 PM on October 31, 2022: contributor

reACK https://github.com/bitcoin/bitcoin/pull/26325/commits/fd0ed48ecd9f0d65acc3199a50b3ca6479aa5a3e

aureleoules force-pushed on Nov 1, 2022

aureleoules commented at 2:04 PM on November 1, 2022: member

I've added a test case in feature_pruning.py that checks the filter_false_positives mode fails on pruned nodes when the relevant block is pruned, as suggested by @andrewtoth. https://github.com/bitcoin/bitcoin/compare/fd0ed48ecd9f0d65acc3199a50b3ca6479aa5a3e..c39234b070a0d2102af835fda77f3a78d4a631d7

aureleoules force-pushed on Nov 4, 2022

aureleoules commented at 10:42 AM on November 4, 2022: member

Moved filter_false_positives RPC arg inside options to make it optional as suggested by @luke-jr: diff
Rebased to fix CI error

in src/rpc/blockchain.cpp:2283 in d20b019698 outdated

2250 | +    {
2251 | +        LOCK(cs_main);
2252 | +        block = GetBlockChecked(blockman, pindex);
2253 | +        if (pindex->nHeight > 0) {
2254 | +            block_undo = GetUndoChecked(blockman, pindex);
2255 | +        }

theStack commented at 2:37 PM on December 6, 2022:

After rebasing on master, excluding the genesis block is not needed anymore, as GetUndoChecked takes care of this now and simply returns an empty CBlockUndo instance (since #19888, commit 2ca5a496c2f3cbcc63ea15fa05c1658e7f527bbc has been merged):

        block_undo = GetUndoChecked(blockman, pindex);

aureleoules commented at 3:09 PM on December 6, 2022:

Thanks, done. Dropped fd0ed48ecd9f0d65acc3199a50b3ca6479aa5a3e and added your suggestion.

theStack commented at 2:40 PM on December 6, 2022: contributor

Considering that there is no release out yet that includes the scanblocks RPC, this particular release note is not needed I'd say? (We should not forget to add a general one that mentions the introduction of scanblocks RPC, but this is unrelated to this PR.)

aureleoules force-pushed on Dec 6, 2022

in src/rpc/blockchain.cpp:2276 in 8da8bee074 outdated

2271 | +
2272 | +    CBlock block;
2273 | +    CBlockUndo block_undo;
2274 | +
2275 | +    {
2276 | +        LOCK(cs_main);

maflcko commented at 4:19 PM on December 6, 2022:

Any reason for this lock, given that there are other places without the lock?

andrewtoth commented at 4:23 PM on December 6, 2022:

GetBlockChecked and GetUndoChecked both require a lock. #26308 removes the lock requirement for these functions. Calling ReadBlockFromDisk or UndoReadFromDisk directly doesn't require the lock.

maflcko commented at 4:26 PM on December 6, 2022:

Ugh, thanks. Didn't fully grasp the function name

maflcko commented at 9:54 AM on December 8, 2022:

#26308 is merged

theStack approved

theStack commented at 2:18 AM on December 7, 2022: contributor

ACK 8da8bee0746a58dae77f790d2695a23f07639830

w0xlt approved

w0xlt commented at 2:24 AM on December 7, 2022: contributor

reACK https://github.com/bitcoin/bitcoin/pull/26325/commits/8da8bee0746a58dae77f790d2695a23f07639830

maflcko added this to the milestone 25.0 on Dec 7, 2022

aureleoules force-pushed on Dec 8, 2022

aureleoules commented at 10:25 AM on December 8, 2022: member

Rebased to remove the LOCK on cs_main in CheckBlockFilterMatches.

in src/rpc/blockchain.cpp:2272 in 1a0a2ede28 outdated

2268 | @@ -2268,17 +2269,49 @@ class BlockFiltersScanReserver
2269 |      }
2270 |  };
2271 |  
2272 | +static bool CheckBlockFilterMatches(BlockManager& blockman, const CBlockIndex* pindex, const GCSFilter::ElementSet& needles)

maflcko commented at 10:30 AM on December 8, 2022:

static bool CheckBlockFilterMatches(BlockManager& blockman, const CBlockIndex& block, const GCSFilter::ElementSet& needles)

Shouldn't this be &? (Passing in nullptr would be UB)

aureleoules commented at 11:10 AM on December 8, 2022:

Thanks, done.

aureleoules force-pushed on Dec 8, 2022

w0xlt approved

w0xlt commented at 9:07 PM on December 9, 2022: contributor

reACK https://github.com/bitcoin/bitcoin/pull/26325/commits/c8d5ee3f3f60d50968c7d48f8fdfd678ea2a1fbc

theStack approved

theStack commented at 1:37 AM on December 11, 2022: contributor

re-ACK c8d5ee3f3f60d50968c7d48f8fdfd678ea2a1fbc

in src/rpc/blockchain.cpp:2292 in c8d5ee3f3f outdated

2289 | +        for (const auto& txundo : block_undo.vtxundo) {
2290 | +            if (std::any_of(txundo.vprevout.cbegin(), txundo.vprevout.cend(), [&](const auto& coin) { return coin.out.scriptPubKey == script; })) {
2291 | +                return true;
2292 | +            }
2293 | +        }
2294 | +    }

achow101 commented at 8:07 PM on December 14, 2022:

ISTM this outer loop isn't necessary and could potentially be quite slow. GCSFilter::ElementSet is a std::unordered_set which has constant time lookup. But with this loop, the lookup becomes linear. It should be much faster to see if each scriptPubKey is contained in needles.

// Check if any of the outputs match the scriptPubKey
    for (const auto& tx : block.vtx) {
        if (std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& txout) { return needles.count(std::vector<unsigned char>(txout.scriptPubKey.begin(), txout.scriptPubKey.end())) != 0; })) {
            return true;
        }
    }

    // Check if any of the inputs match the scriptPubKey
    for (const auto& txundo : block_undo.vtxundo) {
        if (std::any_of(txundo.vprevout.cbegin(), txundo.vprevout.cend(), [&](const auto& coin) { return needles.count(std::vector<unsigned char>(coin.out.scriptPubKey.begin(), coin.out.scriptPubKey.end())) != 0; })) {
            return true;
        }
    }

aureleoules force-pushed on Dec 15, 2022

aureleoules commented at 9:02 AM on December 15, 2022: member

Thanks @achow101 I added your suggestion.

theStack approved

theStack commented at 6:44 PM on December 18, 2022: contributor

re-ACK 0b2f0f394248c67a36303c09f7519a9a2d5d29df

I agree that this loop order (as suggested by achow101 in #26325 (review)) is the superior approach.

DrahtBot added the label Needs rebase on Jan 3, 2023

aureleoules force-pushed on Jan 6, 2023

rpc: Return accurate results for scanblocks

This makes use of undo data to accurately verify results
from blockfilters.

5ca7a7be76

aureleoules force-pushed on Jan 6, 2023

aureleoules commented at 11:01 AM on January 6, 2023: member

Rebased.

DrahtBot removed the label Needs rebase on Jan 6, 2023

in src/rpc/blockchain.cpp:2447 in 5ca7a7be76

2441 | @@ -2408,6 +2442,15 @@ static RPCHelpMan scanblocks()
2442 |                      for (const BlockFilter& filter : filters) {
2443 |                          // compare the elements-set with each filter
2444 |                          if (filter.GetFilter().MatchAny(needle_set)) {
2445 | +                            if (filter_false_positives) {
2446 | +                                // Double check the filter matches by scanning the block
2447 | +                                const CBlockIndex& blockindex = *CHECK_NONFATAL(WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(filter.GetBlockHash())));

furszy commented at 1:31 PM on January 6, 2023:

nit

                                const CBlockIndex& blockindex = *CHECK_NONFATAL(WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(filter.GetBlockHash())));

theStack approved

theStack commented at 7:14 PM on January 8, 2023: contributor

re-ACK 5ca7a7be76f2521dca895daa70949fd11df0844c

achow101 commented at 10:17 PM on January 12, 2023: member

ACK 5ca7a7be76f2521dca895daa70949fd11df0844c

furszy approved

furszy commented at 10:31 PM on January 13, 2023: member

Code review ACK 5ca7a7be

achow101 merged this on Jan 16, 2023

achow101 closed this on Jan 16, 2023

sidhujag referenced this in commit 021761aeac on Jan 17, 2023

bitcoin locked this on Jan 16, 2024

rpc: Return accurate results for scanblocks #26325

Master

PR (without filter_false_positives mode)

PR (with filter_false_positives mode)

Reviews

Conflicts

PR (without `filter_false_positives` mode)

PR (with `filter_false_positives` mode)