index: batch db writes during initial sync #34489

Why keep ProcessBlock around? It should be simpler if we just have one function and it can be handed one or many blocks.

Because ProcessBlocks will be specialized in a follow-up commit (please see #26966) to enable additional speedups, such as processing blocks out-of-order for indexes that support it.

Also, I wouldn’t merge them because these two methods solve different problems. ProcessBlocks handles ordering, ProcessBlock handles a single unit of work. I think keeping them separate keeps the code clean and easy to follow while allowing us to introduce improvements in the future.

Or, if you just want to keep it for encapsulation, consider making it private at least?

Yeah, could do that.

just realized that ProcessBlocks is private already.

Has this been optimized with benchmarks or are you planning to do that? This might be something differs between the indexes so it might make sense to let each index define their own value.

Let’s handle it in a focused follow-up so we don’t lose momentum here tuning a parameter.

I’d rather land the structural improvements first, since they unblock other improvements and allow us to continue working on the parallelization goal (which introduces the major sync speedup).

Bitcoin blocks do not have a consistent size, so batching by block size will result in irregular batch sizes. I suggest considering other approaches, such as a fixed transaction count (e.g., adding blocks until the number of transactions within them falls within a target range). This could be particularly interesting for txindex, as most batches would have a similar byte size. This would make batches more uniform and lead to more predictable computation and performance.

However, this approach would make parallelisation more difficult, since generating the batch would require a precomputation step. This is a considerable trade-off, especially given that parallelisation of the batch computation (#26966) would likely be a more impactful enhancement.

so batching by block size will result in irregular batch sizes

I don’t think that matters. Whichever is simpler.

Let’s handle it in a focused follow-up so we don’t lose momentum here tuning a parameter.

I agree with @fjahr that tuning this based on measurements should be part of the PR. This seems like a random number, not based on actual behavior. What are the memory implications of this batch? What would be the speedup with bigger ones, and what’s the effect on LevelDB compaction and the number of files, etc.?

It’s expected that any batching implementation adds some memory overhead, but that shouldn’t be a problem

I personally find this hand-wavy approach concerning.

allow us to continue working on the parallelization goal

I also personally find this troubling. We should be concerned with correctness here, and not even mention parallelization before we’re sure batching is actually correct. And since I have concerns about the current implementation (reorgs, save + load, state + tip writing inconsistencies) and the lack of testing, I don’t think we should treat this as just a quick hack that helps us get to parallelization.

nit: inline constexpr is more idiomatic for headers:

0static inline constexpr int INDEX_BATCH_SIZE{500};

FWIW, I didn’t say that tweaking the value has to be part of this PR

Ok, but I did, we can’t just add a random value here, we have to back it by some measurements. I have measured a batch size of 1000 and it doesn’t seem to reduce the size further. The implementation still contains some bugs, so we probably have to remeasure after they’re fixed. I’m also curious about the peak memory usage here.

static inline constexpr int INDEX_BATCH_SIZE{500};

static inline constexpr int INDEX_BATCH_SIZE{1000};

 0BEFORE="4c40a923f003420193aa574745f70788bcf35265"; AFTER="1497086cdb0e64df861100e0715ffd975d4c41cf"; \
 1DATA_DIR="/mnt/my_storage/BitcoinData"; export DATA_DIR; \
 2RESULTS_FILE="${DATA_DIR}/index_benchmark_results.txt"; export RESULTS_FILE; \
 3wait_index() { \
 4  tail -F ${DATA_DIR}/debug.log 2>/dev/null | grep -q -m1 'index is enabled\|is enabled at height'; \
 5  sleep 2; \
 6  killall bitcoind 2>/dev/null || true; \
 7  wait; \
 8}; export -f wait_index; \
 9log_size() { \
10  local label="$1"; local index_dir="$2"; \
11  local size=$(du -sh "$index_dir" 2>/dev/null | cut -f1); \
12  local files=$(find "$index_dir" -type f -name '*.ldb' 2>/dev/null | wc -l); \
13  echo "$label: size=$size, ldb_files=$files" | tee -a "$RESULTS_FILE"; \
14}; export -f log_size; \
15git reset --hard >/dev/null 2>&1 && git clean -fxd >/dev/null 2>&1 && git fetch origin $BEFORE $AFTER >/dev/null 2>&1; \
16for c in $BEFORE:build-before $AFTER:build-after; do \
17  git checkout ${c%:*} >/dev/null 2>&1 && \
18  sed -i 's/INDEX_BATCH_SIZE = 500/INDEX_BATCH_SIZE = 1000/' src/index/base.h && \
19  cmake -B ${c#*:} -G Ninja -DCMAKE_BUILD_TYPE=Release >/dev/null 2>&1 && ninja -C ${c#*:} bitcoind >/dev/null 2>&1; \
20done; \
21echo "indexes | $(hostname) | $(uname -m) | $(lscpu | grep 'Model name' | head -1 | cut -d: -f2 | xargs) | $(nproc) cores | $(free -h | awk '/^Mem:/{print $2}') RAM | $(df -T $DATA_DIR | awk 'NR==2{print $2}') | $(lsblk -no ROTA $(df --output=source $DATA_DIR | tail -1) 2>/dev/null | grep -q 0 && echo SSD || echo HDD)" | tee -a "$RESULTS_FILE" && \
22for INDEX in txindex blockfilterindex coinstatsindex txospenderindex; do \
23  echo -e "\n--- $INDEX ---" | tee -a "$RESULTS_FILE"; \
24  if [ "$INDEX" = "blockfilterindex" ]; then \
25    INDEX_DIR="${DATA_DIR}/indexes/blockfilter/basic"; \
26  else \
27    INDEX_DIR="${DATA_DIR}/indexes/${INDEX}"; \
28  fi; \
29  export INDEX_DIR; \
30  for BUILD in before after; do \
31    if [ "$BUILD" = "before" ]; then COMMIT="${BEFORE:0:7}"; else COMMIT="${AFTER:0:7}"; fi; \
32    hyperfine --runs 1 --shell bash --sort command \
33      --prepare "rm -rf ${DATA_DIR}/indexes/* ${DATA_DIR}/debug.log" \
34      --cleanup "log_size '${INDEX} ${BUILD}' '${INDEX_DIR}'" \
35      -n "${BUILD} (${COMMIT})" \
36      "./build-${BUILD}/bin/bitcoind -datadir=${DATA_DIR} -${INDEX}=1 -connect=0 -printtoconsole=0 & wait_index" \
37      2>&1 | tee -a "$RESULTS_FILE"; \
38  done; \
39done;
40indexes | i7-hdd | x86_64 | Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz | 8 cores | 62Gi RAM | ext4 | HDD
41
42--- txindex ---
43Benchmark 1: before (4c40a92)
44  Time (abs ≡):        23766.792 s               [User: 13249.392 s, System: 1700.229 s]
45 
46Benchmark 1: after (1497086)
47  Time (abs ≡):        21667.695 s               [User: 12747.999 s, System: 1444.935 s]
48 
49
50--- blockfilterindex ---
51Benchmark 1: before (4c40a92)
52  Time (abs ≡):        14074.504 s               [User: 8335.840 s, System: 419.536 s]
53 
54Benchmark 1: after (1497086)
55  Time (abs ≡):        13987.488 s               [User: 8275.042 s, System: 416.125 s]
56 
57
58--- coinstatsindex ---
59Benchmark 1: before (4c40a92)
60  Time (abs ≡):        40104.717 s               [User: 34332.346 s, System: 408.091 s]
61 
62Benchmark 1: after (1497086)
63  Time (abs ≡):        39729.018 s               [User: 34321.404 s, System: 400.228 s]
64 
65
66--- txospenderindex ---
67Benchmark 1: before (4c40a92)
68  Time (abs ≡):        36491.202 s               [User: 23663.602 s, System: 2216.541 s]
69 
70Benchmark 1: after (1497086)
71  Time (abs ≡):        26654.635 s               [User: 22664.639 s, System: 1659.436 s]

I will measure INDEX_BATCH_SIZE = 100 next,

Finished INDEX_BATCH_SIZE = 100:

 0BEFORE="4c40a923f003420193aa574745f70788bcf35265"; AFTER="1497086cdb0e64df861100e0715ffd975d4c41cf"; DATA_DIR="/mnt/my_storage/BitcoinData"; export DATA_DIR; RESULTS_FILE="${DATA_DIR}/index_benchmark_results.txt"; export RESULTS_FILE; wait_index() {   tail -F ${DATA_DIR}/debug.log 2>/dev/null 
 1| grep -q -m1 'index is enabled\|is enabled at height';   sleep 2;   killall bitcoind 2>/dev/null || true;   wait; }; export -f wait_index; log_size() {   local label="$1"; local index_dir="$2";   local size=$(du -sh "$index_dir" 2>/dev/null | cut -f1);   local files=$(find "$index_dir" -type f -name '*.ldb' 2>/dev/null | wc -l);   echo "$label: size=$size, ldb_files=$files" | tee -a "$RESULTS_FILE"; }; export -f log_size; git reset --hard >/dev/null 2>&1 && git clean -fxd >/dev/null 2>&1 && git fetch origin $BEFORE $AFTER >/dev/null 2>&1; for c in $BEFORE:build-before $AFTER:build-after; do   git checkout ${c%:*} >/dev/null 2>&1 &&   sed -i 's/INDEX_BATCH_SIZE = 500/INDEX_BATCH_SIZE = 100/' src/index/base.h &&   cmake -B ${c#*:} -G Ninja -DCMAKE_BUILD_TYPE=Release >/dev/null 2>&1 && ninja -C ${c#*:} bitcoind >/dev/null 2>&1; done; echo "indexes | $(hostname) | $(uname -m) | $(lscpu | grep 'Model name' | head -1 | cut -d: -f2 | xargs) | $(nproc) cores | $(free -h | awk '/^Mem:/{print $2}') RAM | $(df -T $DATA_DIR | awk 'NR==2{print $2}') | $(lsblk -no ROTA $(df --output=source $DATA_DIR | tail -1) 2>/dev/null | grep -q 0 && echo SSD || echo HDD)" | tee -a "$RESULTS_FILE" && for INDEX in txindex blockfilterindex coinstatsindex txospenderindex; do   echo -e "\n--- $INDEX ---" | tee -a "$RESULTS_FILE";   if [ "$INDEX" = "blockfilterindex" ]; then     INDEX_DIR="${DATA_DIR}/indexes/blockfilter/basic";   else     INDEX_DIR="${DATA_DIR}/indexes/${INDEX}";   fi;   export INDEX_DIR;   for BUILD in before after; do     if [ "$BUILD" = "before" ]; then COMMIT="${BEFORE:0:7}"; else COMMIT="${AFTER:0:7}"; fi;     hyperfine --runs 1 --shell bash --sort command       --prepare "rm -rf ${DATA_DIR}/indexes/* ${DATA_DIR}/debug.log"       --cleanup "log_size '${INDEX} ${BUILD}' '${INDEX_DIR}'"       -n "${BUILD} (${COMMIT})"       "./build-${BUILD}/bin/bitcoind -datadir=${DATA_DIR} -${INDEX}=1 -connect=0 -printtoconsole=0 & wait_index"       2>&1 | tee -a "$RESULTS_FILE";   done; done;
 2indexes | i7-hdd | x86_64 | Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz | 8 cores | 62Gi RAM | ext4 | HDD
 3
 4--- txindex ---
 5Benchmark 1: before (4c40a92)
 6  Time (abs ≡):        23968.560 s               [User: 13214.417 s, System: 1732.668 s]
 7 
 8Benchmark 1: after (1497086)
 9  Time (abs ≡):        22914.283 s               [User: 13153.074 s, System: 1706.380 s]
10 
11
12--- blockfilterindex ---
13Benchmark 1: before (4c40a92)
14  Time (abs ≡):        14056.593 s               [User: 8332.068 s, System: 414.447 s]
15 
16Benchmark 1: after (1497086)
17  Time (abs ≡):        14055.667 s               [User: 8282.042 s, System: 413.627 s]
18 
19
20--- coinstatsindex ---
21Benchmark 1: before (4c40a92)
22  Time (abs ≡):        40062.595 s               [User: 34294.097 s, System: 401.118 s]
23 
24Benchmark 1: after (1497086)
25  Time (abs ≡):        39953.151 s               [User: 34338.162 s, System: 394.930 s]
26 
27
28--- txospenderindex ---
29Benchmark 1: before (4c40a92)
30  Time (abs ≡):        36106.469 s               [User: 23462.741 s, System: 2216.890 s]
31 
32Benchmark 1: after (1497086)
33  Time (abs ≡):        32262.354 s               [User: 22250.924 s, System: 1963.755 s]

Finished INDEX_BATCH_SIZE = 300:

 0--- txindex ---
 1Benchmark 1: before (4c40a92)
 2  Time (abs ≡):        23916.303 s               [User: 13219.568 s, System: 1732.442 s]
 3 
 4Benchmark 1: after (1497086)
 5  Time (abs ≡):        22366.743 s               [User: 12985.083 s, System: 1581.649 s]
 6 
 7
 8--- blockfilterindex ---
 9Benchmark 1: before (4c40a92)
10  Time (abs ≡):        14148.605 s               [User: 8334.721 s, System: 415.524 s]
11 
12Benchmark 1: after (1497086)
13  Time (abs ≡):        14078.675 s               [User: 8292.390 s, System: 414.992 s]
14 
15
16--- coinstatsindex ---
17Benchmark 1: before (4c40a92)
18  Time (abs ≡):        40208.707 s               [User: 34359.943 s, System: 401.578 s]
19 
20Benchmark 1: after (1497086)
21  Time (abs ≡):        40035.152 s               [User: 34408.491 s, System: 395.890 s]
22 
23
24--- txospenderindex ---
25Benchmark 1: before (4c40a92)
26  Time (abs ≡):        36154.602 s               [User: 23454.551 s, System: 2234.643 s]
27 
28Benchmark 1: after (1497086)
29  Time (abs ≡):        27110.711 s               [User: 21378.361 s, System: 1703.907 s]

I’d be interested in the extra memory consumption of these to help with choosing a good batch value.

A minor comment on the commit description: I feel an ambiguity or typo around “during”.

0Pass CDBBatch to subclasses during so writes can be accumulated and committed together instead of flushing per block.

unrelated nit:

0    // No need to handle errors in Commit. If it fails, the error will already be logged. The

The start and end are set here, according to the desired size. However, block_batch.start_index can change below.

It seems the code I had for this case got dropped in the previous bad squash. Good catch. See the missing line https://github.com/furszy/bitcoin-core/commit/54874bf43b2df6846061fbb844b7026f69e7bc74#diff-5251d93616c116c364d2d502ad2a59e901a3512194462fabacc9756f96fd8dddR274. Will re-add it. Thanks.

Wouldn’t it be better/cleaner to set it just before the ProcessBlocks call? (I mean to keep the logic up to the deleted pindex = pindex_next; line, and put the bactch setup there.)

That would not only require taking cs_main an extra time, but would also introduce race conditions. The extra locking comes from the chain lookup needed to determine the window’s end index. And, more importantly, if the tip changes between setting the start index and computing the end, we could end up on a fork at a lower height, resulting in an invalid window (start_index > end_index).

That would not only require taking cs_main an extra time, but would also introduce race conditions. The extra locking comes from the chain lookup needed to determine the window’s end index.

What I had in mind is setting both the start and end a bit later, after the pindex is finalized. I will try it out and provide a concrete code snippet a bit later.

What I had in mind is setting both the start and end a bit later, after the pindex is finalized. I will try it out and provide a concrete code snippet a bit later.

I see the same two problems there, with an extra requirement of a reorg check and another rewind call. Which makes it even worse.

I wouldn’t say this test truly mimics a restart (i.e., shutting down the node and immediately restarting it, without the blockchain growing). It is closer to a continuation after a clean shutdown of the whole node for a long period of time. This test is still interesting, but it does not cover a new bug window introduced by the batch approach (shutting down the node during a batch).

The test currently does the following:

• Pre-mine 100 blocks
• Mine 173 additional blocks
• Create the index
• Synchronize the index to the tip
• Mine another 173 blocks
• Recreate the index
• Synchronize again to the new tip

In a real-world scenario, all blocks would already exist on disk, and a restart would likely happen during a batch (for example at height 196, where the previous batch ended at 180).

I would like to see a test (either a modification of this one or a new one) that verifies the index is not corrupted and can correctly resume if the node is restarted during a sync, before reaching the tip.

A possible approach for such a test could be:

1. Pre-populate the blockchain (e.g., 300 blocks already on disk)
2. Start index sync with batch_size = 30
3. Interrupt the sync after several batches but before completion
4. Simulate a shutdown
5. Restart the index and verify (with no new blocks added):
   • It resumes from the last checkpoint (not from genesis or the last processed block)
   • No corruption occurs in re-processed blocks
   • The sync successfully completes

Sorry if I didn’t explain this clearly before, let me try again.

With this PR, some blocks are being indexed (on disk), but the index pointer is not updated because the Commit() function has not been called. The commit is executed every 30 seconds (which won’t happen during the test) or when the tip is processed (batch tip).

What I’m suggesting is to test the scenario where a batch is processed but the tip is not processed. In that case, the index should sync starting from the last committed block.

After debugging your test case, the second round started from the last element of the last batch, as it was the end of the chain at round 0. I would suggest checking the following cases (with a batch of 30 blocks and at least 101 blocks):

• If the sync process is shut down when block 100 is processed, after a restart, the first block to sync should be 91 (not 0, 91 or 120), verifying that if a batch is interrupted, after a restart, the index will reindex from the last committed batch.
• If the sync process is shut down when block 90 is processed, after a restart, the first block to sync should be 91, verify and an edge case of the previous scenario.

Update

I noticed that in the code, interruptions are handled by committing:

0if (m_interrupt) {
1    LogInfo("%s: m_interrupt set; exiting ThreadSync", GetName());
2
3    SetBestBlockIndex(pindex);
4    Commit();
5    return;
6}

Because of this, the scenarios described above may not be useful, since the last processed block might always be committed before shutdown.

I’m not sure whether this interruption only applies to graceful shutdowns or if it also covers crash scenarios.

With this PR, some blocks are being indexed (on disk), but the index pointer is not updated because the Commit() function has not been called. The commit is executed every 30 seconds (which won’t happen during the test) or when the tip is processed (batch tip).

Just to be accurate: batch writes and the index locator update are, on purpose, not related procedures. Commit() happens every 30 secs, during interruption or when the tip is reached prior to finishing initial sync.

What I’m suggesting is to test the scenario where a batch is processed but the tip is not processed. In that case, the index should sync starting from the last committed block.

Sure, we could return an error inside the test child index CustomAppend() that finishes the process early. It shouldn’t be hard to code. Feel encouraged to experiment with it. I can cherry-pick your changes if you have a test commit for it.

Just to be accurate: batch writes and the index locator update are, on purpose, not related procedures. Commit() happens every 30 secs, during interruption or when the tip is reached prior to finishing initial sync.

Yes, ofc :D

It shouldn’t be hard to code. Feel encouraged to experiment with it. 🫡

Having played around with the code, I found that the case i mentioned didnt worked correctly

• Round 0:

  • Max height: 273
  • After sync, tip is at 273

• Round 1:

  • Max height: 446 (273 + 173)
  • Sync fails at height 310

• Round 2:

  • The first batch starts at 273, however, it should be 304 (the previous round synced one batch)

  This happens because CustomAppend() returns false and the process is stpped before the following branch is reached

  0if (m_interrupt) {
  1    LogInfo("%s: m_interrupt set; exiting ThreadSync", GetName());
  2     SetBestBlockIndex(pindex);
  3    Commit();
  4    return;
  5}
  

Below is the code to reproduce the issue:

 0// Extend BlockFilterIndex to create a custom test function CustomAppend
 1class BlockFilterIndexTest : public BlockFilterIndex
 2{
 3public:
 4    BlockFilterIndexTest(std::unique_ptr<interfaces::Chain> chain, BlockFilterType filter_type, size_t n_cache_size, bool f_memory) :
 5        BlockFilterIndex(std::move(chain), filter_type, n_cache_size, f_memory) {}
 6
 7    // Override CustomAppend to simulate an error during append at a specific block height
 8    bool CustomAppend(CDBBatch& batch, const interfaces::BlockInfo& block) override
 9    {
10        if (block.height == 290) {
11            return false;
12        }
13        return BlockFilterIndex::CustomAppend(batch, block);
14    }
15};
16
17// Ensure the initial sync batch window behaves as expected.
18// Tests sync from the genesis block and from a higher block to mimic a restart.
19// Note: Test runs in /tmp by default, which is usually cached, so timings are not
20// a meaningful benchmark (use -testdatadir to run it elsewhere).
21BOOST_FIXTURE_TEST_CASE(initial_sync_batch_window, BuildChainTestingSetup)
22{
23    constexpr int MINE_BLOCKS = 173;
24    constexpr int BATCH_SIZE = 30;
25
26    int expected_tip = 100; // pre-mined blocks
27    for (int round = 0; round < 3; round++) { // two additional rounds to test sync from genesis and from a higher block
28        mineBlocks(MINE_BLOCKS); // Generate blocks
29        const int tip_height = WITH_LOCK(::cs_main, return m_node.chainman->ActiveChain().Height());
30        BOOST_REQUIRE(tip_height == MINE_BLOCKS + expected_tip);
31        expected_tip = tip_height;
32
33        BlockFilterIndexTest filter_index(interfaces::MakeChain(m_node), BlockFilterType::BASIC, 1 << 20, /*f_memory=*/false);
34        filter_index.SetProcessingBatchSize(BATCH_SIZE);
35        BOOST_REQUIRE(filter_index.Init());
36        BOOST_CHECK(!filter_index.BlockUntilSyncedToCurrentChain());
37
38        // Ensure we can sync up to the tip
39        const auto& summary_pre{filter_index.GetSummary()};
40        // breakpoint here for following gdb ouptut
41        filter_index.Sync();
42        if (round == 1) { continue; }
43        const auto& summary{filter_index.GetSummary()};
44        BOOST_CHECK(summary.synced);

GDB output before Sync() in round 2:

0gdb build/bin/test_bitcoin
1b blockfilter_index_tests.cpp:427
2b src/index/base.cpp:255
3run
4(gdb) p round
5$2 = 2
6(gdb) p expected_tip
7$3 = 619
8(gdb) p summary_pre.best_block_height
9$4 = 273

but the locator won’t update in your second round unless you advance the mock time.

yes i didnt want to simulate 30 sec, i wanted the error before the timmer commit (to fail into the error handling m_interrupt)

. Also, keep in mind that if any block within the batch fails, the entire process aborts

I saw that when CustomAppend returns false, it executes FatalErrorf, which simply stops the node (not gracefully) instead of raising m_interrupt, my bad

1. What do you think about modifying the error handling in the sync process to prevent losing stored data by raising the m_interrupt flag? In the worst case, we lose 29.59 seconds of execution, it might not be worth it and let everything as it currently is

2. I will try to modify the CustomAppend to raise the m_interrupt and create a test to show how it is correctly processed, i think it is more interesting this approach

What do you think about modifying the error handling in the sync process to prevent losing stored data by raising the m_interrupt flag? In the worst case, we lose 29.59 seconds of execution, it might not be worth it and let everything as it currently is

As you said, since we already save progress every 30 seconds, saving it during interruptions makes very little sense. The amount of work lost is minimal. Yet, this has been the case since the indexes were introduced, and I suspect it hasn’t received much attention, which is probably why no one removed it before.. It’s something we could consider.

Returning false here causes the interrupt to be handled incorrectly as it will be handled in the Sync function like this:

0if (!ProcessBlocks(db_batch, block_batch.start_index, block_batch.end_index)) {
1    return; // error logged internally
2}

This early return prevents execution from reaching line 226, where the correct interrupt-handling logic resides. As a result, the expected recovery path is skipped.

Specifically, this makes the following block effectively unreachable in this scenario:

0if (m_interrupt) {
1    LogInfo("%s: m_interrupt set; exiting ThreadSync", GetName());
2
3    SetBestBlockIndex(pindex);
4
5    Commit();
6    return;
7}

I didn’t find in the file a place where m_interrupt is handled in this way, might be interesting to try to make the code fall on the graceful path. A possible solution might be changing the return by continue, so it will go to the start of the loop and fall into the error handling logic

I debbuged and with this code, the restart point is ok:

0 if (!ProcessBlocks(db_batch, block_batch.start_index, block_batch.end_index)){
1      if (!m_interrupt) {
2           return;
3}
4                continue; 
5}

Nice catch. Pushed an update that fixes it and adds test coverage.

I’m considering alternative options for the structure, like removing the interruption update, but will let the ideas settle over the weekend. At least we have some nice test coverage for it now.

Some context on “removing the interruption update”: Since we save progress every 30 seconds, saving it during interruptions makes little sense, as the work lost is minimal. The code is probably there merely because it was introduced on the early index days, where things were way more chaotic, and no one really paid much attention to it afterwards.

Nice catch. Pushed an update that fixes it and adds test coverage.

Nice. I finally managed to explain the scenario I’ve been trying to describe from the beginning, sorry for not expressing it clearly before (I’m still new as a core reviewer), at least I found a potential bug.

I don’t want to take up more of your time on it, so feel free to resolve this thread. I think the test implemented for the scenario is enough.

Since we save progress every 30 seconds, saving it during interruptions makes little sense, as the work lost is minimal. The code is probably there merely because it was introduced in the early index days.

That’s a fair point. However, I’m not sure I see the benefit of removing it. I agree that this could be an argument for not introducing the code in the first place, since the potential work lost is minimal. But given that it’s already there, I’m not sure whether removing it (even if the performance impact is small) is an improvement. Do you have something specific in mind? Perhaps something related to the parallelisation of the indexing process?

In commit 965d4299a76badcf2257f390f5c02e6007ec6244

Nit: Why introduce the BlockBatch at all when we don’t pass it on here? I don’t think we gain much by de-referencing here.

Nit: Why introduce the BlockBatch at all when we don’t pass it on here? I don’t think we gain much by de-referencing here.

Mainly for the block range computation, it’s less error-prone to use a range structure instead of two independent variables. In the concurrent future (#26966), many block ranges will be computed at once and submitted to the pool (which will avoid further cs_main locking). This requires storing them in a container, and I wasn’t really a fan of using a less readable std::pair. But complain taken, have reworked it so NextBlockToSync returns the range, which gives this struct a more powerful use case.

Regarding ProcessBlocks usage, I thought about it a few times. In #26966, the structure takes on a more meaningful role, it becomes the work unit state (we call it Task there), and we expand it to store intermediate results there as well. Because of that, I didn’t think that making ProcessBlocks to use it was the best, the function should only receive what it will use, not an struct that contains other stuff.

On a side comment, I’m going to rebase #26966 on top of this PR now it is mature enough, so we use the same terms on both PRs and what I described above is super clear there as well.

Just a question: Why do we need to define all of these?

Not needed here, will remove them.

But, just to describe why these methods exist in the parent PR (and refresh my memory while doing it):

In #26966, this struct is extended to hold the block range processed data, it essentially becomes the work unit that flows through the entire pipeline:

1. It is computed by the index initial sync thread.
2. It is submitted to the thread pool, as a task there.
3. Once processed, it is pushed into a shared “ready” queue, which is then opportunistically processed.
4. Workers that finish their work units, post-processes “ready” units (this is where we flush data to disk in order, so the block filter index can build the headers chain correctly, since each header depends on the previous header’s hash, same will happen with the coinfilterindex in the future, etc).

The whole rationale behind this design is that work units are processed in parallel, no ordering required. And once a worker finishes a task, it opportunistically checks whether the next in-order unit is ready for post-processing. If it is, that worker performs the post-processing step and continues draining any subsequent ready units in order. This basically ensures we can process blocks in parallel while still preserving the ordered persistence requirement that some of the indexes require.

Im not sure if removing final from BlockFilterIndex only to allow IndexBlockSim to subclass it for testing is the better decision, i undertsand that it was used final for a reason, and i do not think modifying the code fo fit a test is the best option.

Restoring final makes the compilation to fail with error:

0  [ 82%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/checkqueue_tests.cpp.o
1/home/arejula27/workspaces/bitcoin/src/test/blockfilter_index_tests.cpp:440:7: error: cannot derive from ‘final’ base ‘BlockFilterIndex’ in derived type ‘blockfilter_index_tests::IndexBlockSim’
2  440 | class IndexBlockSim : public BlockFilterIndex
3      |       ^~~~~~~~~~~~~
4gmake[2]: *** [src/test/CMakeFiles/test_bitcoin.dir/build.make:348: src/test/CMakeFiles/test_bitcoin.dir/blockfilter_index_tests.cpp.o] Error 1
5gmake[2]: *** Waiting for unfinished jobs....
6gmake[1]: *** [CMakeFiles/Makefile2:2217: src/test/CMakeFiles/test_bitcoin.dir/all] Error 2
7gmake: *** [Makefile:146: all] Error 2

Maybe we should base BlockFilterIndex from BaseIndex like IndexReorgCrash

Yeah, I went back and forth on this decision for a while before pushing it. The idea was to actually cover BlockFilterIndex methods during reorgs, not just the BaseIndex behavior, even if they aren’t strictly required for this PR changes.

Ideally, we should have both testing scenarios: reorgs during initial sync against BaseIndex, and reorgs against BlockFilterIndex. But.. doing that would require extracting BuildChainTestingSetup into a separate file, and I didn’t want to expand the scope of this PR any further. This test, and even the existing index_reorg_crash (the one that uses IndexReorgCrash) do not belong to blockfilter_index_tests.cpp. They only test against the base class. But well, that is something that already exists in our codebase, and not something we are including here, so it is better to leave it for a different PR.

Also, the final declaration here (and in all other indexes) seems really unnecessary. I cannot think of anything we gain from it. It only restricts our unit testing capabilities. I would remove it from all indexes.

That being said, most of this is just me ranting about improvements we could make. So will take your suggestion and use the BaseIndex class here for now. Let’s go step by step. Thanks for the feedback.

Ideally, we should have both testing scenarios: reorgs during initial sync against BaseIndex, and reorgs against BlockFilterIndex.

Why? Isn’t that redundant?

They only test against the base class. But that is something that already exists in our codebase and is not included here, so it is better to leave it for a different PR.

Also, the final declaration here (and in all other indexes) seems really unnecessary. I cannot think of anything we gain from it. It only restricts our unit testing capabilities. I would remove it from all indexes.

Sounds reasonable to discuss in another PR. I like the idea of removing final if it is not needed, but as you said, it should be consistent across all indexes.

Why? Isn’t that redundant?

We can test at different levels. The BaseIndex test can verify that we always follow the best chain during initial sync, that the appropriate child methods are called in order (append - remove), and check the db chain locator updates. Then, BlockFilterIndex test, and others, should only focus on manually connecting and disconnecting blocks, verifying that its data is consistent in memory and disk.

Since WriteTxs was moved out of TxIndex::DB, it no longer has static or anonymous namespace scoping, which gives it external linkage. To avoid potential linker conflicts with other translation units, it might be worth wrapping it in an anonymous namespace as this function is not expected to be used outside this file

0 namespace {
1  void WriteTxs(CDBBatch& batch, const std::vector<std::pair<Txid,
2  CDiskTxPos>>& v_pos)
3  {
4      ...
5  }
6 } 

I compiled and passed all tests with this change

0e9f2aa index: prepare NextSyncBlock to return block ranges:

I still find it confusing, as mentioned, this is just an optimization, not sure how it fits into and a clearer special case for “already at tip”. It’s a reasonable change, but another one without any test coverage - please see the mentioned #32875 (review) and #32875 (review) for how this could be tested.

95fc91f index: sync, update iterator only after processing block:

We’re changing behavior in a commit that claims it’s not a behavior change, without adding context for why this change was needed or how to validate that it is correct. Given that it wasn’t obvious to the original authors, an explanation is needed. Also, No test was added to exercise this behavior, to explain exactly why it was necessary, and make sure it doesn’t happen again.

It also seems independent enough to be pushed in a separate PR, with a test documenting the current behavior in the first commit and, in the next commit, a fix and a test update that reflects the new behavior.

95fc91f index: sync, update iterator only after processing block:

We’re changing behavior in a commit that claims it’s not a behavior change, without adding context for why this change was needed or how to validate that it is correct. Given that it wasn’t obvious to the original authors, an explanation is needed. Also, No test was added to exercise this behavior, to explain exactly why it was necessary, and make sure it doesn’t happen again.

It also seems independent enough to be pushed in a separate PR, with a test documenting the current behavior in the first commit and, in the next commit, a fix and a test update that reflects the new behavior.

I’m puzzled here. You are claiming this is a behavior change without explaining how, and based on that, you write a strong affirmation for other reviewers rather than engaging with me about what you think this change introduces. That is not something I can act on, and it risks misleading other reviewers.

This is not a behavior change, it is a correctness change. The loop iterator that tracks the last successfully processed block was modified to be updated after processing the block, not before. The reordering of pindex = pindex_next to after the ProcessBlock call has zero effects on any execution path, because we immediately return if block processing fails, and at that point pindex is never read again. If you believe otherwise, please point to the specific code path you think is affected. This change was made to prepare the ground for the batch end block set, which is the actual focus of this PR. Splitting it out into a separate PR would add unnecessary overhead for a one-line reordering with no behavioral effect.

On the commit message: if the description was not clear enough, just say so. I would have updated it. Engaging directly about what is unclear is far more constructive than making assertions aimed at other reviewers.

On tone: “it wasn’t obvious to the original authors” is truly unhelpful. It does not engage with the code, it does not help improve anything, and it makes it genuinely harder to engage with your other comments in good faith, even the ones that may be perfectly valid. The purpose of review is to work with the author to improve the code, not to editorialize for other readers.

rather than engaging with me about what you think this change introduces

Not sure what you mean, I commented on your PR, meant it for you to read it.

I would have updated it.

I’m not sure about the past tense, I reviewed it because I’m expecting further changes, especially because of the bugs introduced here.

On tone: “it wasn’t obvious to the original authors” is truly unhelpful.

How so? I don’t know or care who wrote the original code, they had an intention here, if we’re changing it, it needs explanation and preferably tests.

I’m not sure about the past tense, I reviewed it because I’m expecting further changes, especially because of the bugs introduced here.

Please stick to the topic in question. I have not yet read your other comments on this PR. This is the first one, and it makes a strong claim without a proper explanation to back it up.

Can you please point to the specific code path where this commit introduces a behavior change? Thanks.

I have not yet read your other comments on this PR

That explains the confusion.

Can you please point to the specific code path where this commit introduces a behavior change

Please read the rest of my comments, that’s why I posted them…

This commit does not explain why the swap is needed. That only becomes clear later, when a non-adjacent commit relies on pindex meaning “last successfully processed block” for the new interrupt path, see #34489 (comment).

So the issue is not this line movement in isolation, but that it is a preparatory change whose purpose is only revealed several commits later, in a commit that changes behavior while claiming to be a refactor. I think those commits should be squashed and the behavior change either fixed or documented.

0cbe84d index: refactor, decouple interruption from sync loop:

The commit claims there’s no behavior change, but it doesn’t explain why the move is safe. Given all the internal returns and breaks it’s not self-evident.

It seems to me that previously a synced, and later interrupted, path could hit

0LogInfo("%s is enabled at height %d", GetName(), pindex->nHeight);

, but after the change it hits

0LogInfo("%s: m_interrupt set; exiting ThreadSync", GetName());

and returns.

I’m not sure there is any other change, but I would appreciate a commit message explanation, and maybe a test exercising this path, preferably a fuzz test to cover all these weird combinations.

0336c61 index: add method to process block ranges:

nit: for consistency we could use the same comment style as below:

0    // Collect all block indexes from end to start

0336c61 index: add method to process block ranges:

When can block be nullptr here? Wouldn’t it be a fatal error if we walk past genesis? And what if the caller mixes up begin and end? Or if start and end have valid heights but are on different forks.

Given how intertwined the code is here, a serious mess-up could cause an infinite loop and would be really hard to trace back to the source.

I would probably sleep better if we asserted that end->nHeight >= start->nHeight and used a bounded for loop here instead.

 0const int range_size{end->nHeight - start->nHeight + 1};
 1Assert(range_size > 0);
 2
 3// Collect all block indexes from end to start
 4std::vector<const CBlockIndex*> ordered_blocks;
 5{
 6    ordered_blocks.reserve(range_size);
 7    CBlockIndex* it = end;
 8    for (int i{0}; i < range_size; ++i) {
 9        ordered_blocks.emplace_back(Assert(it));
10        it = it->pprev;
11    }
12    Assert(it == start->pprev);
13}

0336c61 index: add method to process block ranges:

The method comment states:

Returns false on unrecoverable failure or during interruption

But if the interruption happens before we call the first ProcessBlock, we should be safe since no state was changed - i.e. nothing to recover from.

0336c61 index: add method to process block ranges:

How can we tell this failed “due to an interruption”? m_interrupt could have been set independently

0336c61 index: add method to process block ranges:

0    /// Processes blocks in the range [start, end], calling `ProcessBlock`.

0e9f2aa index: prepare NextSyncBlock to return block ranges:

0// Returns the next block to sync, or `nullptr` if fully synced

0e9f2aa index: prepare NextSyncBlock to return block ranges:

The diff would be a lot simpler if we kept the early return guard here instead:

0        return chain.Genesis();

which would reduce the indentation of the following code and the awkward naming of “first” for “next”

0e9f2aa index: prepare NextSyncBlock to return block ranges:

I find it a bit confusing that the return value of Next is called first, which is clearly the next of the prev.

0e9f2aa index: prepare NextSyncBlock to return block ranges:

The Assume(first) at the end is non-obvious. After the pindex_prev == chain.Tip() fast path returns nullptr, the remaining path reaches chain.Next(chain.FindFork(pindex_prev)). A reader needs to convince themselves this can’t be nullptr, i.e., that FindFork can never return the tip here. Could you add a comment explaining why? Or add it to the commit message to help with review.

Is it safe because the fork point can’t be the tip, because that would mean pindex_prev is in the active chain, which contradicts chain.Next(pindex_prev) having returned nullptr above?