index: move disk read lookups to base class by furszy · Pull Request #32694 · bitcoin/bitcoin

furszy · 2025-06-06T20:38:40Z

Combining common refactors from #24230 and #26966, aiming to move both efforts forward while reducing their size and review burden.

Broadly, #24230 focuses on enabling indexes to run in a separate process, and #26966 aims to parallelize the indexes initial synchronization process. A shared prerequisite for both is ensuring that only the base index class interacts with the node’s chain internals - child index classes should instead operate solely through chain events.

This PR moves disk read lookups from child index classes to the base index class. It also includes a few documentation improvements and a test-only code cleanup.

The indexes test call StartBackgroundSync(), which spawns a thread to run Sync(), only for the test thread to wait for it to complete by calling IndexWaitSynced(). So, since the sync is performed synchronously, we can skip the extra thread creation entirely and call Sync() directly.

DrahtBot · 2025-06-06T20:38:42Z

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/32694.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, maflcko, davidgumberg, mzumsande, achow101
Stale ACK	ryanofsky

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:

#30469 (index: Fix coinstats overflow and introduce index versioning by fjahr)
#29307 (util: explicitly close all AutoFiles that have been written by vasild)
#26966 (index: initial sync speedup, parallelize process by furszy)

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.

ryanofsky

Code review ACK 3f390ef. Nice changes! This gets rid of blockstorage and CBlockIndex* accesses from index subclasses during sync to decouple the indexes from node internals and make new indexes more straightforward to write in the future. It should also simplify the parallelization PR and avoid the need for refactoring there.

ryanofsky · 2025-06-10T11:53:43Z

    // BlockUntilSyncedToCurrentChain should return false before index is started.
    BOOST_CHECK(!filter_index.BlockUntilSyncedToCurrentChain());

-    BOOST_REQUIRE(filter_index.StartBackgroundSync());


In commit "test: indexes, avoid creating threads when sync runs synchronously" (331a25c)

Motivation for this commit seems unclear. It seems to initialize indexes in unit tests differently than they are initialized in the node and reduce test coverage a little. Maybe commit message can be updated to mention reasons for wanting to do this?

In commit "test: indexes, avoid creating threads when sync runs synchronously" (331a25c)

Motivation for this commit seems unclear. It seems to initialize indexes in unit tests differently than they are initialized in the node and reduce test coverage a little. Maybe commit message can be updated to mention reasons for wanting to do this?

Having unit tests waiting for secondary thread creation seemed odd to me; especially since these tests are meant to only run and verify the initial index synchronization output once (once the initial sync is completed, they sync new blocks only via block connection signals). Besides that, the functionality should already be covered by the functional tests.

That being said, could drop it too if preferred. I think I just dislike having the thread inside the index class.

sedited

Approach ACK

This all felt very familiar :)

Can you double-check the commit message of 50696f0. I think there is a word too much in the first and second sentence.

…pendable This commit does not change behavior in any way.

… methods Move ReadBlock code from CoinStatsIndex::CustomRewind to BaseIndex::Rewind Move ReadUndo code from CoinStatsIndex::ReverseBlock to BaseIndex::Rewind This commit does change behavior slightly. Since the new CustomRemove methods only take a single block at a time instead of a range of disconnected blocks, when they call CopyHeightIndexToHashIndex they will now do an index seek for each removed block instead of only seeking once to the height of the earliest removed block. Seeking instead of scanning is a little worse for performance if there is a >1 block reorg, but probably not noticeable unless the reorg is very deep.

furszy · 2025-06-10T17:02:20Z

Thanks for the review ryanofsky and TheCharlatan!
Updated per feedback.

Can you double-check the commit message of 50696f0. I think there is a word too much in the first and second sentence.

Done.

sedited

ACK 4c5ec3a70a47798d8451da65c9c0059955cde937

mzumsande

Concept ACK

…e block The function previously handled a range of heights but now copies only a single entry. This change simplifies the code to match its current usage.

Moved CBlockUndo disk read lookups from child index classes to the base index class. The goal is for child index classes to synchronize only through events, without directly accessing the chain database. This change will enable future parallel synchronization mechanisms, reduce database access (when batched), and contribute toward the goal of running indexes in a separate process (with no chain database access). Besides that, this commit also documents how NextSyncBlock() behaves. It is not immediately clear this function could return the first block after the fork point during a reorg.

sedited

Re-ACK 029ba1a

maflcko · 2025-06-11T09:23:43Z

review ACK 029ba1a 👡

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 029ba1a21d570f7db6c4366ec9a30a381b56d6fb 👡
OQ13TwEEQORasoU+qCZ+jMGAPXKnsCJhTt8atkoWt5nRWhCn9ueXFilV6W5B9iknOUBhP6B2nDQJXduuKa0nAA==

ghost

ACK 029ba1a

Left some nits, and general observations that don't need to be addressed in this PR or at all.

ghost · 2025-06-11T18:23:17Z

-
-        UninterruptibleSleep(100ms);
-    }
-    assert(index.GetSummary().synced);


331a25c (test: indexes, avoid creating threads when sync runs synchronously)

nit, only if retouching: it would probably be nice to retain the m_synced check from here.

nit, only if retouching: it would probably be nice to retain the m_synced check from here.

the m_init check wasn't retained either, but I didn't mention it, as this is test-only code

Sure, will do it if it needs a re-touch.
I don't see it as that important because if the flag wouldn't being set, the test would be failing due to the index ignoring the BlockConnected signals anyway.

ghost · 2025-06-11T21:24:43Z

 {
-    DBHeightKey key(start_height);
+    DBHeightKey key(height);
    db_it.Seek(key);


Out of scope for this PR since it is incorrect on master, but db_it needs a Valid() check before GetKey(), since leveldb assert()'s on trying to read the key of an invalid iterator:

https://github.com/bitcoin-core/leveldb-subtree/tree/bitcoin-fork/db/db_iter.cc#L64-L67

Alternatively, this should probably do m_db.ReadKey(), but that would require this function to become a member of the index or to be passed a db handle, so maybe better suited for a refactor that also addresses @mzumsande's ask to de-deduplicate this (and maybe LookupOne?) in blockfilterindex and coinstatsindex

Out of scope for this PR since it is incorrect on master, but db_it needs a Valid() check before GetKey(), since leveldb assert()'s on trying to read the key of an invalid iterator:

I assume that the invalid iterator case could only happen when it cannot access db? Which is not common unless the user deletes/moves the index db directory after startup?

The iterator is invalid if no entry is found matching the key I believe.

Correction, the iterator is invalid if no entry is found which has a key greater than or equal to the "Seek"'ed key:

https://github.com/bitcoin-core/leveldb-subtree/blob/bitcoin-fork/db/db_test.cc#L861-L870

The GetKey() call and key.height check below protects against the situation where an entry with a height greater than the key we're seeking exists, but the height we're seeking doesn't exist. In the instance where the height we're seeking is greater than any entry which exists, which should never happen,db_it.GetKey() will assert and crash.

Just to be clear though, I think this is out-of-scope and the PR is ready to merge as-is.

ghost · 2025-06-11T22:23:44Z

+        }
    }

    // In the case of a reorg, ensure persisted block locator is not stale.


Just an observation:

While reviewing this PR's changes to CopyHeightIndexToHashIndex(), I wanted to understand LookUpOne from coinstatsindex.cpp and from blockfilterindex.cpp since this is where the lookups by block hashes are relevant:

bitcoin/src/index/coinstatsindex.cpp

Lines 307 to 325 in 0a24870

static bool LookUpOne(const CDBWrapper& db, const interfaces::BlockRef& block, DBVal& result)

{

// First check if the result is stored under the height index and the value

// there matches the block hash. This should be the case if the block is on

// the active chain.

std::pair<uint256, DBVal> read_out;

if (!db.Read(DBHeightKey(block.height), read_out)) {

return false;

}

if (read_out.first == block.hash) {

result = std::move(read_out.second);

return true;

}

// If value at the height index corresponds to an different block, the

// result will be stored in the hash index.

return db.Read(DBHashKey(block.hash), result);

}

I was a little confused about whether or not LookupOne() is correct, since it makes the assumption that if no entry exists for a given block height then there is no entry for that block hash, this is because CustomRewind()/CustomRemove() does not actually remove the rewound block from the DB, it just writes a hash-keyed copy of it, and then when the block is replaced by its sibling from a heavier chain, the height-keyed entry is overwritten.

But, I do struggle a little bit to understand why we even care about preserving the rewound block at all, since if we ever reconnect the block we don't even take advantage of already having written it to the DB, is it because users should be able to do gettxoutsetinfo and getblockfilter() on stale blocks that have already been indexed?

ghost · 2025-06-11T23:02:53Z

        virtual void chainStateFlushed(ChainstateRole role, const CBlockLocator& locator) {}
    };

+    //! Options specifying which chain notifications are required.


6f1392c (indexes, refactor: Remove remaining CBlockIndex* uses in index Rewind methods)

nit:

Suggested change

//! Options specifying which chain notifications are required.

//! Options specifying data required for chain notifications.

ghost · 2025-06-11T23:04:23Z

+    struct NotifyOptions
+    {
+        //! Include block data with block disconnected notifications.
+        bool disconnect_data = false;


6f1392c (indexes, refactor: Remove remaining CBlockIndex* uses in index Rewind methods)

feel-free-to-disregard-style-nit:

I feel this would be more consistent as:

Suggested change

bool disconnect_data = false;

bool disconnect_block_data = false;

ghost · 2025-06-12T18:21:09Z

Will try to not touch the code while we are all happy with the current state. Moving #26966 and #24230 forward after ~3 years sounds good enough to me :)

ghost

Code Review ACK 029ba1a

ghost · 2025-06-12T22:51:15Z

ACK 029ba1a

DrahtBot added the UTXO Db and Indexes label Jun 6, 2025

furszy mentioned this pull request Jun 6, 2025

index: initial sync speedup, parallelize process #26966

Draft

This was referenced Jun 7, 2025

ci, iwyu: Treat warnings as errors for specific directories #31308

Merged

index: Fix coinstats overflow #30469

Merged

util: explicitly close all AutoFiles that have been written #29307

Merged

ryanofsky approved these changes Jun 10, 2025

View reviewed changes

sedited reviewed Jun 10, 2025

View reviewed changes

Comment thread src/index/base.cpp

furszy force-pushed the 2025_indexes_remove_CBlockIndex_access branch from 3f390ef to 0ce3d9b Compare June 10, 2025 16:51

ryanofsky added 2 commits June 10, 2025 12:57

indexes, refactor: Stop requiring CBlockIndex type to call IsBIP30Uns…

0a24870

…pendable This commit does not change behavior in any way.

furszy force-pushed the 2025_indexes_remove_CBlockIndex_access branch from 0ce3d9b to 4c5ec3a Compare June 10, 2025 16:58

sedited approved these changes Jun 10, 2025

View reviewed changes

DrahtBot requested a review from ryanofsky June 10, 2025 18:43

mzumsande reviewed Jun 10, 2025

View reviewed changes

Comment thread src/index/blockfilterindex.cpp Outdated

furszy added 2 commits June 10, 2025 20:22

refactor: index, simplify CopyHeightIndexToHashIndex to process singl…

91b7ab6

…e block The function previously handled a range of heights but now copies only a single entry. This change simplifies the code to match its current usage.

furszy force-pushed the 2025_indexes_remove_CBlockIndex_access branch from 4c5ec3a to 029ba1a Compare June 11, 2025 00:22

sedited approved these changes Jun 11, 2025

View reviewed changes

DrahtBot requested a review from mzumsande June 11, 2025 08:12

davidgumberg approved these changes Jun 11, 2025

View reviewed changes

DrahtBot requested a review from davidgumberg June 11, 2025 23:07

mzumsande reviewed Jun 12, 2025

View reviewed changes

achow101 merged commit 19765dc into bitcoin:master Jun 12, 2025

furszy deleted the 2025_indexes_remove_CBlockIndex_access branch June 13, 2025 00:10

sedited mentioned this pull request Jun 15, 2025

Add a "tx output spender" index #24539

Merged

fjahr mentioned this pull request Jun 15, 2025

index: Check all necessary block data is available before starting to sync #29770

Merged

Sjors mentioned this pull request Jul 11, 2025

BIP352 index (for light wallets and consistency check) Sjors/bitcoin#86

Closed

1 task

furszy mentioned this pull request Oct 15, 2025

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

Draft

bitcoin locked and limited conversation to collaborators Jul 30, 2026

	static bool LookUpOne(const CDBWrapper& db, const interfaces::BlockRef& block, DBVal& result)
	{
	// First check if the result is stored under the height index and the value
	// there matches the block hash. This should be the case if the block is on
	// the active chain.
	std::pair<uint256, DBVal> read_out;
	if (!db.Read(DBHeightKey(block.height), read_out)) {
	return false;
	}
	if (read_out.first == block.hash) {
	result = std::move(read_out.second);
	return true;
	}

	// If value at the height index corresponds to an different block, the
	// result will be stored in the hash index.
	return db.Read(DBHashKey(block.hash), result);
	}

	//! Options specifying which chain notifications are required.
	//! Options specifying data required for chain notifications.

	bool disconnect_data = false;
	bool disconnect_block_data = false;

Conversation

furszy commented Jun 6, 2025

Uh oh!

DrahtBot commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

furszy Jun 10, 2025 • 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!

Uh oh!

Uh oh!

Uh oh!

sedited 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!

Uh oh!

furszy commented Jun 10, 2025

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 11, 2025

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

ghost Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

ghost Jun 12, 2025 • edited by furszy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

ghost Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

ghost Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

ghost Jun 12, 2025 • edited by davidgumberg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

ghost Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

ghost Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

ghost Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 6, 2025 •

edited

Loading

furszy Jun 10, 2025 •

edited

Loading

sedited left a comment •

edited

Loading

ghost left a comment •

edited by davidgumberg

Loading

ghost Jun 12, 2025 •

edited by furszy

Loading

ghost Jun 12, 2025 •

edited by davidgumberg

Loading