Partial IBD speed regression between 9c150222604478431669b66b2caa0d75e8324d4c and 1ea532e590cdc16b86436a2bc4f92d74082307f9 #35457

Is there an existing issue for this?

• I have searched the existing issues

Current behaviour

Benchcoin has observed an IBD speed regression between commits 9c150222604478431669b66b2caa0d75e8324d4c and 1ea532e590cdc16b86436a2bc4f92d74082307f9: https://bitcoin-dev-tools.github.io/benchcoin/

This includes the following changes:

<details><summary>Details</summary> <p>

src/core/bitcoin on  master [$?⇕] via △ v4.1.2 via 🐍 v3.13.13 via ❄️  impure (nix-shell-env)
❯ git range-diff 9c150222604478431669b66b2caa0d75e8324d4c...1ea532e590cdc16b86436a2bc4f92d74082307f9
 -:  ----------- >  1:  fedeff7f201 crypto: disable ASan instrumentation of SSE4 SHA256 for GCC
 -:  ----------- >  2:  0301c758ea0 wallet migration, fuzz: Migrate hd seed once
 -:  ----------- >  3:  cd912c4e108 wallet: Consolidate generation setup callers into one function
 -:  ----------- >  4:  f713fd1725f refactor: wallet: Don't reuse WALLET_BLANK flag for born-encrypted wallets.
 -:  ----------- >  5:  80b0c259921 wallet: Load everything into DescSPKM on construction
 -:  ----------- >  6:  8be5ee554bb test: wallet: Check that loading wallet with both unencrypted and encrypted keys fails.
 -:  ----------- >  7:  6538f691357 fuzz: Skip adding descriptor to wallet if it cannot be expanded
 -:  ----------- >  8:  aa4f7823aa1 wallet: include keys when constructing DescriptorSPKM during import
 -:  ----------- >  9:  e20aaff70f0 wallet: Construct ExternalSignerSPKM with the new descriptor
 -:  ----------- > 10:  32946e0291f wallet: Setup new autogenerated descriptors on construction
 -:  ----------- > 11:  451fdd26a4f test: wallet: Constructing a DSPKM that can't TopUp() throws.
 -:  ----------- > 12:  d5adb9d09b8 doc: fix doxygen links to threads in developer-notes.md
 -:  ----------- > 13:  a815e3e2629 rpc: Correct type for tx_sigops
 -:  ----------- > 14:  0774eaaf0c2 util: Require integers for SaturatingAdd() and AdditionOverflow()
 -:  ----------- > 15:  a3fe455a953 wallet: refactor to read -walletrbf only once instead of twice
 -:  ----------- > 16:  813b4a80d7f refactor: introduce SubmitBlock helper
 -:  ----------- > 17:  5b60f69e40a mining: add submitBlock IPC method to Mining interface
 -:  ----------- > 18:  3962138cc03 test: add IPC submitBlock functional test
 -:  ----------- > 19:  0b9e10ad404 guix: Update `python-signapple` and wrap with OpenSSL paths
 -:  ----------- > 20:  d846444d012 guix: Split manifest into build and codesign manifests
 -:  ----------- > 21:  fad4f417d15 test: Use operator<< for time_points instead of manual TickSinceEpoch
 -:  ----------- > 22:  c17cc76a187 test: speed up feature_dbcrash
 -:  ----------- > 23:  a2a2b1745f0 wallet, test: remove -walletrbf startup option from rpc_psbt.py
 -:  ----------- > 24:  5e833e068d7 wallet, test: -walletrbf startup option from wallet_bumpfee.py
 -:  ----------- > 25:  0ee94b2fef0 wallet, test: remove -deprecatedrpc=bip125 from wallet_listtransactions.py
 -:  ----------- > 26:  8cb6e405d88 wallet, test: remove -walletrbf startup option from wallet_listtransactions.py
 -:  ----------- > 27:  42330922dd8 wallet, test: remove -walletrbf startup option from wallet_backwards_compatibility.py
 -:  ----------- > 28:  fab5733f5d6 doc: Remove good_first_issue.yml
 -:  ----------- > 29:  4bdd46ace37 ci: switch runners from cirrus to warpbuild
 -:  ----------- > 30:  58cdb5c2e83 Squashed 'src/leveldb/' changes from ab6c84e6f3..a7f9bdc611
 -:  ----------- > 31:  4d58c3271c0 build: remove -Wno-conditional-uninitialized from leveldb build
 -:  ----------- > 32:  a9ac680af30 build: remove FALLTHROUGH_INTENDED from leveldb.cmake
 -:  ----------- > 33:  a52ea9bff90 wallet, test: remove -walletrbf startup option from wallet_backwards_compatibility.py
 -:  ----------- > 34:  3ec550d1688 wallet, test: remove -deprecatedrpc=bip125 from wallet_basic.py
 -:  ----------- > 35:  307134bd7e2 wallet, test: remove -deprecatedrpc=bip125 from wallet_migration.py
 -:  ----------- > 36:  2cbbcb5659b wallet, test: remove -deprecatedrpc=bip125 from wallet_send.py
 -:  ----------- > 37:  7bc39e3d084 wallet, test: add wallet_deprecated_rbf.py for walletrbf deprecated keys & options
 -:  ----------- > 38:  f701cd159af doc: fix typo in release notes of [#34917](/bitcoin-bitcoin/34917/)
 -:  ----------- > 39:  fa51f37f180 doc: Reword the Getting-Started section
 -:  ----------- > 40:  fd44d48b24b wallet: fix ancient wallets migration
 -:  ----------- > 41:  b86c1c443d8 test: add coverage for migrating ancient wallets

</p> </details>

Of these the one which stands out to me is

 -:  ----------- > 30:  58cdb5c2e83 Squashed 'src/leveldb/' changes from ab6c84e6f3..a7f9bdc611

This change included:

a7f9bdc611 Merge bitcoin-core/leveldb-subtree#52: Revert "Increase maximum read-only mmap()s used from 1000 to 4096 on 64-bit systems"

Benchcoin copies a fixed pruned datadir at height 840,000 and resumes IBD to 900,000. This means the benchmark opens an existing chainstate LevelDB layout created before the LevelDB subtree bump, rather than measuring a database created from scratch by the new LevelDB configuration.

The copied chainstate currently contains 6382 SSTable files:

[root@nixos:/data/pruned-840k]#   find chainstate -maxdepth 1 \
    \( -name '*.ldb' -o -name '*.sst' \) | wc -l
6382

The LevelDB subtree bump includes a revert of the 64-bit read-only mmap limit from 4096 back to 1000. If the database layout causes many SSTables to be consulted during the resumed sync, the new binary may use the non-mmap random access path for thousands more table files than before. With 6382 SSTable files, the old limit left roughly 4096 files eligible for mmap-backed access and ~2286 beyond the limit, while the new limit leaves only 1000 eligible and ~5382 beyond the limit.

This is only a hypothesis so far; I have not yet confirmed syscall counts, table-cache behavior, or whether the used table set actually crosses the mmap limit.

It also occurs to me that 1a166221cf Merge bitcoin-core/leveldb-subtree#61: Disable seek compaction may interact negatively here by preventing read-triggered compaction from improving an existing layout during the run.

Useful follow-up experiments would be:

• re-run the slow commit with only kDefaultMmapLimit restored to 4096;
• re-run with seek compaction restored;
• re-run with both restored;
• compare -debug=leveldb,coindb,bench logs or syscall profiles for mmap, pread, open/close, table-cache misses, and compaction activity.

I will test the first myself on benchcoin.

If this hypothesis is correct, the regression may affect nodes that upgrade while their chainstate has many SSTables, especially during continued IBD or other read-heavy phases. It may also be worth checking whether long-running tip nodes with large chainstate table counts see a smaller steady-state impact.

Expected behaviour

IBD performance not to degrade

Steps to reproduce

Benchcoin run: https://github.com/bitcoin-dev-tools/benchcoin/actions/runs/26674832655

Relevant log output

No response

How did you obtain Bitcoin Core

Compiled from source

What version of Bitcoin Core are you using?

1ea532e590cdc16b86436a2bc4f92d74082307f9

Operating system and version

Linux nixos 6.6.114

Machine specifications

OS: NixOS 25.05 (Warbler) x86_64 Kernel: Linux 6.6.114 Uptime: 177 days(!), 12 hours, 3 mins Packages: 519 (nix-system) Shell: bash 5.2.37 Terminal: /dev/pts/0 CPU: AMD Ryzen 7 7700 (16) @ 5.39 GHz GPU: AMD Raphael [Integrated] Memory: 2.57 GiB / 62.03 GiB (4%) Swap: Disabled Disk (/): 71.14 GiB / 937.34 GiB (8%) - ext4 Disk (/data): 28.07 GiB / 937.82 GiB (3%) - ext4 Local IP (eno1): 65.21.224.151/26 Locale: en_US.UTF-8

One thing that may be useful to separate here is whether this is mainly an upgrade-path / legacy-layout regression, or a regression for a chainstate produced with the current LevelDB settings. The kDefaultMmapLimit hypothesis makes sense for the existing benchcoin fixture because it has 6382 chainstate SSTables, so the 4096 -> 1000 mmap limit change could push many more table reads onto the non-mmap path. But the rationale in bitcoin-core/leveldb-subtree#52 was that after bitcoin/bitcoin#30039, the larger 32 MiB LevelDB table size should make 1000 mmapped files cover roughly 32 GiB of DB data. So I think it may be worth adding one more axis to the experiment matrix:

1. existing benchcoin 840k fixture
2. regenerated 840k fixture using current LevelDB max-file-size settings
3. possibly a fresh/reindex-chainstate path to the same height Then compare:

• 1ea532e
• 1ea532e with only kDefaultMmapLimit restored to 4096
• 1ea532e with seek compaction restored
• both restored Useful metrics would be per-level SSTable counts at start/end, total chainstate SSTable count, mmap vs pread/open counts, table-cache misses, and LevelDB compaction stats. That should help distinguish “nodes upgrading with a fragmented/small-SSTable chainstate can regress” from “normal fresh/current-layout IBD regresses”. If it only reproduces with the existing 6382-file fixture, the fix might be different from simply reverting the mmap-limit change globally.

I can help with the reproduction side rather than opening a speculative fix immediately.

To avoid duplicating the first experiment mentioned above, I can try to collect comparison data for the existing benchcoin fixture vs a regenerated/current-layout chainstate fixture, especially:

• total chainstate SSTable count before/after the run
• LevelDB table file count per level
• -debug=leveldb,coindb,bench logs
• mmap/pread/open counts using strace or perf
• runtime comparison between 9c15022, 1ea532e, and patched variants

This may help confirm whether the regression is mostly caused by the old 6382-SSTable fixture layout crossing the reduced mmap limit, or whether it also affects a fresh/current LevelDB chainstate layout.

So this seems to be confirmed as being an interaction between:

1. a pre- #30039 -style chainstate db (2MB files)
2. the seek compaction disablement
3. decreasing mmap limit

When these three elements combine there may be:

1. many SST files
2. which don't get compacted
3. and are are now not m-mapped

This can cause poor performance through consulting too many (on-disk) files per lookup. We thought our bloom filters would help us more here, but they do not seem to. LevelDB’s bloom filters are per table/filter block. They help once LevelDB is considering a specific table. They do not by themselves fix a layout where many tables remain candidates for the key range.

Previously the read compaction would gradually rewrite into the newer format and help us out a bit.

It's my understanding that this would only be a major issue for someone doing something exactly like benchcoin is (was) doing: syncing a part-synced node, from version <= 28.0, using a current master binary. This seems likely to be rare.

There may be some impact at the tip for a user who did full IBD using a <= 28.0 node, and still has many SST files which have not been read-compacted down, but I have not tried to measure this.

I re-ran benchmarks with each of #bitcoin-core/leveldb-subtree/pull/61 and #bitcoin-core/leveldb-subtree/pull/52 individually, and neither causes the same performance issue. This means backporting one, or the other, seems safe-enough. IIUC we have only backported the seek compaction commit, so should be good here.

I think a sensible move in any case could be to, on modern versions, detect the SST layout at startup and if it's using 2MB files, or there are many files, initiate a one-off compaction. This can already be done manually using the hidden startup flag -forcecompactdb, but we should be able to do this automatically via detection (and should probably do it if we change db structure again in the future).

Thanks, that makes sense. IIUC, this no longer looks like a simple revert of either leveldb-subtree#52 or #61, but more like an upgrade-path issue for legacy chainstate layouts with many old 2 MiB SST files. Would a reasonable next step be to prototype chainstate-only startup detection for this case, for example:

• count .ldb / .sst files in the chainstate directory before opening the DB;
• detect an old/small-file layout using file count and maybe average/median SST size;
• log a clear message when the layout looks legacy;
• optionally trigger the existing compaction path once, similar in spirit to -forcecompactdb. I can take a look at this direction if that sounds useful. I would probably start with detection + logging first, then keep the actual automatic compaction gated behind a conservative threshold so modern/current-layout chainstates are not affected.

@prashant2007-wq, we can also prompt LLMs ourselves, please don't spam here.