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 (43a212cfdac6c64e82b601c664443d022f191520):

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 (486c71bc707f5765fc6698a746cf221bab1ac357):

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.

sedited

ACK 63ef83d72a979e9e557eec9fe87c2795a963dedb

src/bench/index_blockfilter.cpp

src/index/base.cpp

sedited

Re-ACK b19585e00a52c0b571e70ddb8f7c996d598c3435

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

nit: could be const function.

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

src/index/blockfilterindex.cpp

nit: const bool.

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

Concept ACK

I synced the block filter from scratch at c3e2915b14900c1379e4c3f1ac0262f60b362431 (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 5d5e22b979d00ca3d3c4b4013fea43ba58f571bc that might be more significant because there's more going on in cs_main.

Can you add some rationale to the b19585e00a52c0b571e70ddb8f7c996d598c3435 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 08d8608c51beb66dab4e12ac06559a70517fb054

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

sedited

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

            }

-            {
-                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

Closed

index: initial sync speedup, parallelize process #26966

Open

Add a "tx output spender" index #24539

Open

sedited approved these changes Nov 29, 2023

View reviewed changes

src/bench/index_blockfilter.cpp Outdated Show resolved Hide resolved

src/bench/index_blockfilter.cpp Outdated 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

sedited 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

18 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

Closed

24 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

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 sedited March 12, 2024 15:07

sedited 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

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

stickies-v mentioned this pull request May 23, 2024

[26.x] archive 26.1 release notes + backports #29899

Merged

bitcoin locked and limited conversation to collaborators Apr 16, 2025

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

Uh oh!

Conversation

furszy commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Sjors commented Nov 28, 2023

Uh oh!

furszy commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Dec 5, 2023

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

For Branch 1

For Branch 2

Uh oh!

andrewtoth left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewtoth left a comment

Choose a reason for hiding this comment

Uh oh!

andrewtoth Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

andrewtoth Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

Sjors commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Feb 24, 2024

Uh oh!

furszy commented Mar 12, 2024

Uh oh!

Sjors commented Mar 12, 2024

Uh oh!

Sjors commented Mar 12, 2024

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

andrewtoth left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Mar 19, 2024

Uh oh!

achow101 commented Mar 20, 2024

Uh oh!

furszy commented Nov 28, 2023 •

edited

Loading

DrahtBot commented Nov 28, 2023 •

edited

Loading

furszy commented Nov 28, 2023 •

edited

Loading

furszy left a comment •

edited

Loading

Sjors commented Feb 23, 2024 •

edited

Loading

furszy commented Feb 23, 2024 •

edited

Loading