index: block filters sync, reduce disk read operations by caching last header #28955

furszy · 2023-11-28T12:42:40Z

Work decoupled from #26966 per request.

The aim is to remove an unnecessary disk read operation that currently takes place with every new arriving block (or scanned block during background sync). Instead of reading the last filter header from disk merely to access its hash for constructing the next filter, this work caches it, occupying just 32 more bytes in memory.

Also, reduces cs_main lock contention during the index initial sync process. And, simplifies the indexes initialization and shutdown procedure.

Testing Note:
To compare the changes, added a pretty basic benchmark in the second commit. Alternatively, could also test the changes by timing the block filter sync from scratch on any network; start the node with -blockfilterindex and monitor the logs until the syncing process finish.

Local Benchmark Results:

*Master (c252a0f):

ns/op	op/s	err%	total	benchmark
132,042,516.60	7.57	0.3%	7.79	`BlockFilterIndexSync`

*PR (43a212c):

ns/op	op/s	err%	total	benchmark
126,915,841.60	7.88	0.6%	7.51	`BlockFilterIndexSync`

DrahtBot · 2023-11-28T12:42:43Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, TheCharlatan, andrewtoth, achow101
Concept ACK	brunoerg

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:

#29432 (Stratum v2 Template Provider (take 3) by Sjors)
#24539 (Add a "tx output spender" index by sstone)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

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 · 2023-11-28T14:09:17Z

Ran bench on MacBook Pro 2019 (2,3 GHz 8-Core Intel Core i9, SSD):

src/bench/bench_bitcoin -filter=BlockFilterIndexSync

Before (486c71b):

ns/op	op/s	err%	total	benchmark
336,034,835.00	2.98	0.6%	19.86	`BlockFilterIndexSync`

After:

ns/op	op/s	err%	total	benchmark
339,403,771.80	2.95	0.6%	19.98	`BlockFilterIndexSync`

Not seeing any improvement in the bench. Haven't tried running the full indexer though.

furszy · 2023-11-28T14:26:24Z

Ran bench on MacBook Pro 2019 (2,3 GHz 8-Core Intel Core i9, SSD):
src/bench/bench_bitcoin -filter=BlockFilterIndexSync
Before (486c71b):

ns/op op/s err% total benchmark
336,034,835.00 2.98 0.6% 19.86 BlockFilterIndexSync
After:

ns/op op/s err% total benchmark
339,403,771.80 2.95 0.6% 19.98 BlockFilterIndexSync
Not seeing any improvement in the bench. Haven't tried running the full indexer though.

Probably, it is because the benchmarks, same as the unit tests, create temp directories. Which may be faster to access than regular directories. See #26684 (review).
I should verify if the bench framework accepts custom paths so we can test this differently. Most likely, it doesn't and we should add the feature.

Also, you could increase the CHAIN_SIZE variable inside the benchmark. Go from 600, to 3000 or more. The time should grow up linearly. I didn't do it merely because the bench would take longer to setup but the difference should be evident with a longer chain.

TheCharlatan

ACK 63ef83d

src/bench/index_blockfilter.cpp

src/index/base.cpp

TheCharlatan

Re-ACK b19585e

luke-jr · 2023-12-05T02:37:58Z

Probably, it is because the benchmarks, same as the unit tests, create temp directories. Which may be faster to access than regular directories.

Is it, though? I would think the OS should have this cached regardless?

furszy

Probably, it is because the benchmarks, same as the unit tests, create temp directories. Which may be faster to access than regular directories.

Is it, though? I would think the OS should have this cached regardless?

We will know it soon. I'm working on a way to test it inside #26564 (review).

But, it seems to be an orthogonal topic. Regardless of the result, which could vary depending on the OS, the changes in this PR should be good to go as is.

Conceptually, the block filter index synchronization process makes a db call on every new block (forever) just to get the tip's header hash when it could just cache those 32 bytes.

furszy

Finished testing this on a custom directory. Results are favorable @Sjors and @luke-jr.
I have observed that access to the OS temp directory is faster than accessing regular directories locally. Benchmark outputs are provided at the end.

The testing methodology is as following:

Running ./bench_bitcoin -filter="BlockFilterIndexSync" -testdatadir=<custom_test_datadir_path> on any of the following branches:

Branch 1 -> Which includes: this PR changes + #26564 + a commit that introduces the custom datadir feature for the benchmark framework.

Branch 2 -> Which includes: the block filter index benchmark + #26564 + a commit that introduces the custom datadir feature for the benchmark framework.

The results were:

For Branch 1

ns/op	op/s	err%	total	benchmark
117,648,576.33	8.50	0.5%	6.96	`BlockFilterIndexSync`

For Branch 2

ns/op	op/s	err%	total	benchmark
121,370,493.17	8.24	0.5%	7.16	`BlockFilterIndexSync`

andrewtoth

This patch seems to be doing more than what is in the description. From my understanding, it is also reducing cs_main lock contention in the renamed Sync method, which is also now made public. The former seems like a simple win, but it's unclear to me why we want the latter?
Also, a refactor to indexes in init.cpp that also seems fine but unrelated?
Maybe the PR description can be updated to describe the motivation behind these other changes?

furszy

Updated per feedback. Thanks @andrewtoth!

This patch seems to be doing more than what is in the description. From my understanding, it is also reducing cs_main lock contention in the renamed Sync method, which is also now made public. The former seems like a simple win, but it's unclear to me why we want the latter?

The benchmark, introduced in the second commit, makes use of it. If BaseIndex::Sync() is not public, the benchmark would need to call BaseIndex::StartBackgroundSync(), who wraps BaseIndex::Sync() on a separate thread, which goes against the objective of the benchmark (we want to bench the process, not the time it takes the OS to create, wait-for and destroy a thread).

Also, it facilitates the future decoupling of the index inner thread in #26966. Replacing it for a thread-pool class provided by the caller.

Also, a refactor to indexes in init.cpp that also seems fine but unrelated?
Maybe the PR description can be updated to describe the motivation behind these other changes?

Sure. This PR improves the index sources. And, while the changes simplify the code, to me, pushing a PR just to do such small code refactoring does not seem worth the efforts (from reviewers and from myself).
Will update the description. Thanks!

andrewtoth

Concept ACK

I've ran the benchmark locally on this PR and the branches you mention in #28955 (review), and it is sometimes better on one branch then the other for both tests. The results you show are very small improvements which I think could just be attributed to noise.

Regardless of the result, which could vary depending on the OS, the changes in this PR should be good to go as is.

I agree, except for the first 2 commits. The benchmark as I mentioned doesn't seem particularly useful, and the Sync method being made public should then only be done in a patch that will take advantage of it's new accessibility.

andrewtoth · 2024-02-23T14:57:52Z

src/index/blockfilterindex.cpp

@@ -215,10 +224,25 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter&
    return data_size;
 }

+std::optional<uint256> BlockFilterIndex::ReadHeader(int height, const uint256& expected_block_hash)


nit: could be const function.

andrewtoth · 2024-02-23T14:58:14Z

src/index/blockfilterindex.cpp

    }

    BlockFilter filter(m_filter_type, *Assert(block.data), block_undo);

+    const uint256& header = filter.ComputeHeader(last_header);
+    bool res = Write(filter, block.height, header);


nit: const bool.

Sjors · 2024-02-23T15:14:49Z

Concept ACK

I synced the block filter from scratch at c3e2915 (rebased on master) and then on the last commit. AMD Ryzen 7950x with SSD drive running Ubuntu 23.10:

before: 5 hours 35 minutes
after: 5 hours 46 minutes

So not much difference, which seems fine with the goal of #26966 in mind - which does make a dramatic difference.

I haven't measured the performance impact on syncing the index during IBD; in light of 5d5e22b that might be more significant because there's more going on in cs_main.

Can you add some rationale to the b19585e commit message as to what was preventing this simplification before?

@andrewtoth wrote:

The benchmark as I mentioned doesn't seem particularly useful,

Benchmarks are also useful to prevent future regressions.

src/index/blockfilterindex.h

src/index/blockfilterindex.cpp

src/index/blockfilterindex.h

furszy · 2024-02-23T19:49:02Z

Updated per feedback. Thanks both.

Can you add some rationale to the b19585e commit message as to what was preventing this simplification before?

Nothing was preventing it.

The benchmark as I mentioned doesn't seem particularly useful

Have you tried it on a spinning disk? The diff should be noticeable there. We need to avoid disk writes/reads where is possible. See #28037 discussion as a good example of it.

the Sync method being made public should then only be done in a patch that will take advantage of its new accessibility.

Have merged the first two commits.

Sjors · 2024-02-24T09:28:45Z

tACK 08d8608

Introduce benchmark for the block filter index sync. And makes synchronous 'Sync()' mechanism accessible.

furszy · 2024-03-12T12:45:45Z

Rebased due a one-line conflict with #29236.

Sjors · 2024-03-12T14:50:20Z

Tidy complains about

index/blockfilterindex.cpp:135:22: error: Unterminated format string used with LogPrintf [bitcoin-unterminated-logprintf,-warnings-as-errors]
  135 |             LogError("Cannot read last block filter header; index may be corrupted");
      |                      ^                                                            
      |                                                                                   \n

Avoid disk read operations on every new processed block.

Only NextSyncBlock requires cs_main lock. The other function calls like Commit or Rewind will lock or not cs_main internally when they need it. Avoiding keeping cs_main locked when Commit() or Rewind() write data to disk.

Sjors · 2024-03-12T15:07:54Z

re-ACK 99afb9d

TheCharlatan

Re-ACK 99afb9d

andrewtoth

ACK 99afb9d

brunoerg · 2024-03-19T16:28:03Z

Concept ACK

achow101 · 2024-03-20T16:19:23Z

ACK 99afb9d

maflcko · 2024-04-15T08:24:30Z

Follow-up in #29867 (comment)

ryanofsky · 2024-04-16T14:01:16Z

src/index/base.cpp

@@ -159,23 +159,20 @@ void BaseIndex::Sync()
                return;
            }

-            {
-                LOCK(cs_main);


In commit "index: decrease ThreadSync cs_main contention" (0faafb5)

Note: This commit introduces a race condition, because it is no longer locking cs_main while calling NextSyncBlock and setting m_synced = true. As a result, a new block could be connected by another thread after NextSyncBlock returns null in this thread, but before m_synced is set to true, so the block will never be indexed because BlockConnected notifications are ignored while m_synced is false. This should be fixed in #29867

DrahtBot added the UTXO Db and Indexes label Nov 28, 2023

This was referenced Nov 28, 2023

Silent payment index (for light wallets and consistency check) #28241

Draft

index: blockfilter initial sync speedup, parallelize process #26966

Draft

Add a "tx output spender" index #24539

Open

TheCharlatan approved these changes Nov 29, 2023

View reviewed changes

src/bench/index_blockfilter.cpp Show resolved Hide resolved

src/bench/index_blockfilter.cpp Show resolved Hide resolved

src/index/base.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2023_index_blockfilter_cache_header branch from 63ef83d to b19585e Compare November 30, 2023 02:04

TheCharlatan approved these changes Nov 30, 2023

View reviewed changes

DrahtBot mentioned this pull request Dec 1, 2023

Stratum v2 Template Provider (take 2) #28983

Closed

30 tasks

furszy commented Dec 5, 2023

View reviewed changes

This was referenced Dec 8, 2023

test: test_bitcoin: allow -testdatadir=<datadir> #26564

Merged

Use of a wallet shouldn't be blocked in prune mode ("wallet loading failed... beyond pruned data") #27188

Closed

furszy commented Dec 29, 2023

View reviewed changes

furszy mentioned this pull request Jan 4, 2024

Prune Node Rescan Project Tracking #29183

Open

17 tasks

DrahtBot mentioned this pull request Jan 11, 2024

log: Nuke error(...) #29236

Merged

DrahtBot added the CI failed label Jan 16, 2024

DrahtBot mentioned this pull request Feb 14, 2024

Stratum v2 Template Provider (take 3) #29432

Draft

25 tasks

andrewtoth reviewed Feb 22, 2024

View reviewed changes

furszy commented Feb 23, 2024

View reviewed changes

andrewtoth reviewed Feb 23, 2024

View reviewed changes

Sjors reviewed Feb 23, 2024

View reviewed changes

src/index/blockfilterindex.h Outdated Show resolved Hide resolved

src/index/blockfilterindex.cpp Outdated Show resolved Hide resolved

src/index/blockfilterindex.h Outdated Show resolved Hide resolved

furszy force-pushed the 2023_index_blockfilter_cache_header branch 2 times, most recently from e3d3763 to 08d8608 Compare February 23, 2024 19:47

DrahtBot removed the CI failed label Feb 23, 2024

furszy added 3 commits March 12, 2024 09:30

bench: basic block filter index initial sync

bcbd7eb

Introduce benchmark for the block filter index sync. And makes synchronous 'Sync()' mechanism accessible.

index: blockfilter, decouple Write into its own function

331f044

index: blockfilter, decouple header lookup into its own function

a6756ec

furszy force-pushed the 2023_index_blockfilter_cache_header branch from 08d8608 to 1cf73a3 Compare March 12, 2024 12:36

DrahtBot removed the Needs rebase label Mar 12, 2024

furszy added 3 commits March 12, 2024 11:55

index: cache last block filter header

f1469eb

Avoid disk read operations on every new processed block.

index: decrease ThreadSync cs_main contention

0faafb5

Only NextSyncBlock requires cs_main lock. The other function calls like Commit or Rewind will lock or not cs_main internally when they need it. Avoiding keeping cs_main locked when Commit() or Rewind() write data to disk.

refactor: init, simplify index shutdown code

99afb9d

furszy force-pushed the 2023_index_blockfilter_cache_header branch from 1cf73a3 to 99afb9d Compare March 12, 2024 14:55

DrahtBot added the CI failed label Mar 12, 2024

DrahtBot requested a review from TheCharlatan March 12, 2024 15:07

TheCharlatan approved these changes Mar 12, 2024

View reviewed changes

DrahtBot removed the CI failed label Mar 13, 2024

DrahtBot mentioned this pull request Mar 14, 2024

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

Draft

andrewtoth approved these changes Mar 16, 2024

View reviewed changes

DrahtBot requested a review from brunoerg March 20, 2024 16:19

achow101 self-assigned this Mar 20, 2024

achow101 merged commit 0b96a19 into bitcoin:master Mar 20, 2024
16 checks passed

furszy deleted the 2023_index_blockfilter_cache_header branch March 20, 2024 19:38

hebasto mentioned this pull request Mar 23, 2024

cmake: Regular rebasing of the cmake-staging branch hebasto/bitcoin#127

Closed

ryanofsky reviewed Apr 16, 2024

View reviewed changes

ryanofsky mentioned this pull request Apr 16, 2024

index: race fix, lock cs_main while 'm_synced' is subject to change #29867

Merged

luke-jr mentioned this pull request Apr 21, 2024

ThreadSanitizer: Fix #29767 #29776

Merged

stickies-v mentioned this pull request May 9, 2024

[27.x] Backports #29888

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

index: block filters sync, reduce disk read operations by caching last header #28955

index: block filters sync, reduce disk read operations by caching last header #28955

furszy commented Nov 28, 2023 •

edited

DrahtBot commented Nov 28, 2023 •

edited

Sjors commented Nov 28, 2023

furszy commented Nov 28, 2023 •

edited

TheCharlatan left a comment

TheCharlatan left a comment

luke-jr commented Dec 5, 2023

furszy left a comment

furszy left a comment

andrewtoth left a comment

furszy left a comment •

edited

andrewtoth left a comment

andrewtoth Feb 23, 2024

andrewtoth Feb 23, 2024

Sjors commented Feb 23, 2024 •

edited

furszy commented Feb 23, 2024 •

edited

Sjors commented Feb 24, 2024

furszy commented Mar 12, 2024

Sjors commented Mar 12, 2024

Sjors commented Mar 12, 2024

TheCharlatan left a comment

andrewtoth left a comment

brunoerg commented Mar 19, 2024

achow101 commented Mar 20, 2024

maflcko commented Apr 15, 2024

ryanofsky Apr 16, 2024

index: block filters sync, reduce disk read operations by caching last header #28955

index: block filters sync, reduce disk read operations by caching last header #28955

Conversation

furszy commented Nov 28, 2023 • edited

DrahtBot commented Nov 28, 2023 • edited

Code Coverage

Reviews

Conflicts

Sjors commented Nov 28, 2023

furszy commented Nov 28, 2023 • edited

TheCharlatan left a comment

Choose a reason for hiding this comment

TheCharlatan left a comment

Choose a reason for hiding this comment

luke-jr commented Dec 5, 2023

furszy left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

For Branch 1

For Branch 2

andrewtoth left a comment

Choose a reason for hiding this comment

furszy left a comment • edited

Choose a reason for hiding this comment

andrewtoth left a comment

Choose a reason for hiding this comment

andrewtoth Feb 23, 2024

Choose a reason for hiding this comment

andrewtoth Feb 23, 2024

Choose a reason for hiding this comment

Sjors commented Feb 23, 2024 • edited

furszy commented Feb 23, 2024 • edited

Sjors commented Feb 24, 2024

furszy commented Mar 12, 2024

Sjors commented Mar 12, 2024

Sjors commented Mar 12, 2024

TheCharlatan left a comment

Choose a reason for hiding this comment

andrewtoth left a comment

Choose a reason for hiding this comment

brunoerg commented Mar 19, 2024

achow101 commented Mar 20, 2024

maflcko commented Apr 15, 2024

ryanofsky Apr 16, 2024

Choose a reason for hiding this comment

furszy commented Nov 28, 2023 •

edited

DrahtBot commented Nov 28, 2023 •

edited

furszy commented Nov 28, 2023 •

edited

furszy left a comment •

edited

Sjors commented Feb 23, 2024 •

edited

furszy commented Feb 23, 2024 •

edited