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

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:

• #34440 (Refactor CChain methods to use references, tests by optout21)
• #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.

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.

I was thinking to buy and external disk (HDD) and try to benchmark the memory impact, would it be interesting or having the blockchain and the index on different disks make the test not useful?

having the blockchain and the index on different disks make the test not useful?

for memory profiling it shouldn't matter - just know that these measurements are notoriously slow

@furszy Do you think #34897 has any implications on this PR and #26966? If indexes can only commit up to the chainstate's last flushed block, the batching and parallelisation speedups might be limited by the flush interval (~50-70 min) rather than by index processing itself. Would it make sense to keep the current per-block behaviour during IBD (where the index is catching up and flush lag matters most) and use batching only for fully synced nodes processing new blocks using new methods?? Another option could be each index sets its own batch size. For example, blockfilterindex (required during IBD) could use a batch size of 1 to keep commit granularity tight, while optional indexes like coinstatsindex or txindex could use larger batches since their commit timing is less critical.

PS: I got my chain synced, will try to do the memory benchmark this week

🐙 This pull request conflicts with the target branch and needs rebase.