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

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34654 ([RFC] BlockMap and CChain Concurrency Improvement by alexanderwiederin)
• #32875 (index: handle case where pindex_prev equals chain tip in NextSyncBlock() by HowHsu)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

• // attached while m_synced is still false, and it would not be -> // attached while m_synced is still false, and it would not be indexed. [fragmentary comment missing its conclusion; adding "indexed." completes the sentence and makes the intent clear]

• // No need to handle errors in Commit. If it fails, the error will be already be -> // No need to handle errors in Commit. If it fails, the error will already be -> duplicate word "be" makes the sentence ungrammatical; remove the extra "be" to restore correct syntax.

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

• WaitForHeight(&index, blocking_height + 2, 5s) in src/test/blockfilter_index_tests.cpp

^{2026-03-08 18:11:08}

🚧 At least one of the CI tasks failed.
_{Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/21616547248/job/62296358632
LLM reason (✨ experimental): IWYU (include-what-you-use) reported a failure in the test script, causing the CI to fail.}

Concept ACK, I prefer this over #33306 - and will investigate if this solves that problem once my benchmarking servers free up.

because @l0rinc did not find that this was leading to a speed up.

I experimented with something similar in https://github.com/l0rinc/bitcoin/pull/37/changes, will compare it against this change. I wrote:

indexes: batch index writes: 34188s, 211M, 8 files
It solved the fragmentation, but didn't speed up anything.
I still think this is a better direction than adding manual compactions.

Most likely since writing isn't the bottleneck but MuHash calculations were.

This relates to what you wrote here right? It would be helpful that you check if it solves the LevelDB file issue in coinstatsindex and I am also curious about your benchmark results because @l0rinc did not find that this was leading to a speed up.

@fjahr the comment was merely an excuse to decouple the DB writes batching code out of #26966 rather than me having a formed opinion in favor or against #33306. That's why I didn't mention it in the PR description. Maybe the changes complement each other.

To be crystal clear, just updated the PR description with further details on why this change worth alone, independently on #33306.

To summarize it here, the goal of the changes are:

• Batch DB writes instead of flushing per block, which will improve sync time on HDD due to the reduced number of IO operations.
• Reduce cs_main lock contention, which orthogonally improves the overall node responsiveness (and primarily IBD) while the indexes threads are syncing.
• Lays the groundwork for the real speedups, since part of index: initial sync speedup, parallelize process #26966 parallelization pre-work is introduced here as well.

Updated per feedback from @hebasto (thanks!).
Changed <cinttypes> include for <cstdint> to make IWYU happy.

Updated per feedback from @hebasto (thanks!). Changed <cinttypes> include for <cstdint> to make IWYU happy.

I guess, this is a bug in IWYU caused by include-what-you-use/include-what-you-use@44480a2.

UPDATE: Fixed in #34498.

Can you run all unit and functional tests on all commits? Or do they time out?

Can you run all unit and functional tests on all commits? Or do they time out?

Bad squash, my bad. Thanks for the heads up. Fixed.
Also rebased the branch to pick up the CI changes.

Builds clean on ARM64 (Pi 5, Debian Bookworm). All index-related functional tests and blockfilter_index unit tests pass.
:)

I am confused as to how this batching relates to the batching logic of the orginal code.
Looking at the pre-change BaseIndex::Sync(), it seems that Commit/WriteBatch occured only every 30 seconds (or at the chain tip), not after each block. This change does a WriteBatch after every 500 blocks, which in fact may be even more frequent. The original commit-after-every-30-secs is still there, so I don't see how this change makes less writes. Maybe I misunderstand the change.

            if (!ProcessBlock(pindex)) return; // error logged internally
            ...
            if (current_time - last_locator_write_time >= SYNC_LOCATOR_WRITE_INTERVAL) {
                SetBestBlockIndex(pindex);
                last_locator_write_time = current_time;
                // No need to handle errors in Commit. See rationale above.
                Commit();
            }

I am confused as to how this batching relates to the batching logic of the orginal code. Looking at the pre-change BaseIndex::Sync(), it seems that Commit/WriteBatch occured only every 30 seconds (or at the chain tip), not after each block. This change does a WriteBatch after every 500 blocks, which in fact may be even more frequent. The original commit-after-every-30-secs is still there, so I don't see how this change makes less writes. Maybe I misunderstand the change.

            if (!ProcessBlock(pindex)) return; // error logged internally
            ...
            if (current_time - last_locator_write_time >= SYNC_LOCATOR_WRITE_INTERVAL) {
                SetBestBlockIndex(pindex);
                last_locator_write_time = current_time;
                // No need to handle errors in Commit. See rationale above.
                Commit();
            }

Commit() only persists the locator (i.e. "how far the index has synced") so we can resume from that point after a restart. It does not flush or batch the per-block digested data.

The block data is written inside CustomAppend() which is called from ProcessBlock() per block, and it writes to the db immediately.

We're not replacing locator update time nor merging it with something else here, we're only batching something that previously wasn't batched at all.

Also, using a block window instead of reusing the existing time-based window is intentional. Time-based flushing makes memory usage hard to predict, and difficult to test (see #32878), because the amount of buffered data depends on runtime speed, which does not necessarily correlate with available memory (batched data stays in memory until it's flushed..). A fixed block window provides predictable memory usage and deterministic behavior, which is overall better and also integrates nicely with multiple workers sync (#26966). I would argue in favor of ditching the time window in the future, but.. that's not something related to this PR.

Updated per feedback, plus added test coverage for the introduced changes. Thanks @optout21.

Thanks for the explanations, and apologies for asking trivial/irrelevant questions.

Commit() only persists the locator (i.e. "how far the index has synced") so we can resume from that point after a restart.
The block data is written inside CustomAppend() which is called from ProcessBlock() per block, and it writes to the db immediately.
These are two important points, I can see now.

I was puzzled by the fact that there is a loop in Sync(), and another loop is introduced in ProcessBlocks(). But the two complement each other.

Another approach would be to keep the outer loop in Sync() block-by-block, store a CDBBatch variable, process one block, count the blocks in the batch, and flush the batch if batch size is reached (outside of ProcessBlock). This would be slightly less change (no secondary loop), but -- if I'm correct -- the cs_main lock would be still accessed at every block.

... I would argue in favor of ditching the time window in the future, but.. that's not something related to this PR.
Maybe Commit() could be called with the same granularity as batch writes, but that's out of scope here.

Another approach would be to keep the outer loop in Sync() block-by-block, store a CDBBatch variable, process one block, count the blocks in the batch, and flush the batch if batch size is reached (outside of ProcessBlock). This would be slightly less change (no secondary loop), but -- if I'm correct -- the cs_main lock would be still accessed at every block.

Yes. That would still lock cs_main on every block, which harms the overall node responsiveness, and it is also not very helpful for the parallelization goal, which requires certain isolation between batches so worker threads can process them concurrently.

CI failure is unrelated.

Why would it be unrealated, when it consistently happens on this pull only?

This PR only touches indexes code and nothing else. CI was failing in interface_ipc.py this morning, and now after the restart fails on p2p_orphan_handling.py. None of these tests have any index enabled.

None of these tests have any index enabled.

Yeah, I guess this is a good point. It is just odd that the p2p_orphan_handling.py happened twice here, but nowhere else so far. The IPC one is tracked in #34711

Edit: Let's see if the CI fails in #34726 ...

Let's see if the CI fails in #34726

Could add some debug logging to see where it's getting stuck. At first glance, it looks like it stalls during block creation.

Also, just to be clear to other reviewers. Since the issue happens there as well, we know for sure it is not related to this PR.

looks like CI passed now

looks like CI passed now

yes, review can continue.

Two small comments, I feel the one about removing final is relevant, the other one not really but I saw the same pattern on other files of the project so if it is not an inconvenient you can add it, if not just resolve it.

Waiting for the HDD benchmark of @l0rinc. I can do it if he is busy

Waiting for the HDD benchmark of l0rinc. I can do it if he is busy

Having more benchmarks does not hurt :). Feel more than welcome adding your results as well!

Updated per feedback. Thanks @arejula27.
Also rebased on master to pull the CI fixes.

Did another index benchmark for the latest push on an HDD:

BEFORE="4c40a923f003420193aa574745f70788bcf35265"; AFTER="5be65dbcdceaafb6f5a0e34c69efc2b3dfcfe27a"; \
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 | 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 && 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;

--- txindex ---

Benchmark 1: before (4c40a92)
Time (abs ≡): 23757.916 s [User: 13234.906 s, System: 1697.284 s]

Benchmark 1: after (5be65db)
Time (abs ≡): 21134.433 s [User: 12627.245 s, System: 1483.096 s]

--- blockfilterindex ---

Benchmark 1: before (4c40a92)
Time (abs ≡): 14205.566 s [User: 8328.684 s, System: 418.981 s]

Benchmark 1: after (5be65db)
Time (abs ≡): 14046.610 s [User: 8286.885 s, System: 415.464 s]

--- coinstatsindex ---

Benchmark 1: before (4c40a92)
Time (abs ≡): 40075.612 s [User: 34320.885 s, System: 403.669 s]

Benchmark 1: after (5be65db)
Time (abs ≡): 39869.235 s [User: 34307.695 s, System: 398.322 s]

--- txospenderindex ---
Benchmark 1: before (4c40a92)
Time (abs ≡): 36324.897 s [User: 23490.919 s, System: 2227.044 s]

Benchmark 1: after (5be65db)
Time (abs ≡): 26375.223 s [User: 21783.716 s, System: 1690.540 s]

Edit: added txospenderindex

Did another index benchmark for the latest push on an HDD:

---

indexes | i7-hdd | x86_64 | Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz | 8 cores | 62Gi RAM | ext4 | HDD

BEFORE="4c40a923f003420193aa574745f70788bcf35265"; AFTER="5be65dbcdceaafb6f5a0e34c69efc2b3dfcfe27a"; \
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 | 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 && 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; 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;

--- txindex ---

Benchmark 1: before (4c40a92)
Time (abs ≡): 23757.916 s [User: 13234.906 s, System: 1697.284 s]

Benchmark 1: after (5be65db)
Time (abs ≡): 21134.433 s [User: 12627.245 s, System: 1483.096 s]

--- blockfilterindex ---

Benchmark 1: before (4c40a92)
Time (abs ≡): 14205.566 s [User: 8328.684 s, System: 418.981 s]

Benchmark 1: after (5be65db)
Time (abs ≡): 14046.610 s [User: 8286.885 s, System: 415.464 s]

--- coinstatsindex ---

Benchmark 1: before (4c40a92)
Time (abs ≡): 40075.612 s [User: 34320.885 s, System: 403.669 s]

Benchmark 1: after (5be65db)
Time (abs ≡): 39869.235 s [User: 34307.695 s, System: 398.322 s]

It looks like the improvement is not very noticeable for two of the indexes. It might be interesting to try increasing the batch size. Do you have any suggestions on what could be causing this?

It looks like the improvement is not very noticeable for two of the indexes. It might be interesting to try increasing the batch size. Do you have any suggestions on what could be causing this?

I'm not sure this is really surprising given the difference in the amount of data written to the latter two. I would expect a jump more similar to the txindex for the txospenderindex.

What is the expectation on the increase of memory usage here? I tried profiling it with massif, but usage when running with this patch seemed a bit erratic, which I guess is still an indication that it is likely to be higher. Can we really batch operations in this way without putting more memory pressure on the OS?

Can we really batch operations in this way without putting more memory pressure on the OS?

The batch size is intentionally small. I was very conservative with the chosen value to avoid having to worry about it here. The goal of this PR is to land the structural improvements, with a net positive: less cs_main locking, merging prerequisites for the parallelization goal, and the last discovery of reducing the number of created LBD files.

It's expected that any batching implementation adds some memory overhead, but that shouldn't be a problem, it pays off in many other areas, not only with a faster sync promise. And, if we ever wanted to, we could add a config flag to disable batching by setting the batch size to 1. But realistically speaking, I don't think anyone running indexes would want them to take hours to sync while locking cs_main and slowing down the whole node. People who run indexes want them to sync-up quickly so they can start using them.

Also, an important point. All these changes only matter during the index initial sync period. Once the index is synced, they are no longer relevant.

We can benchmark this on small devices like the Pi 4 to see the benefits more clearly, but I honestly wouldn't spend too much time on it. If we agree this PR lands structural, scalable improvements with many benefits and no noticeable downside, and also lets us move forward with the major parallelization speedup path, it seems reasonable to just move forward. This also has a good number of tests we currently lack from.