Skip to content

Instantly share code, notes, and snippets.

@LarryRuane

LarryRuane/claude-conversation.txt

Created July 26, 2026 17:52
Show Gist options

  • Select an option

    Learn more about clone URLs
  • Save LarryRuane/e7389a3448e25caa7765f37a0765f37b to your computer and use it in GitHub Desktop.

Select an option

Learn more about clone URLs
Save LarryRuane/e7389a3448e25caa7765f37a0765f37b to your computer and use it in GitHub Desktop.
Download ZIP
claude - opus conversation July 2026
Raw
claude-conversation.txt
╭─── Claude Code v2.1.218 ─────────────────────────────────────────────────────╮
│ │ Tips for getting │
│ Welcome back Larry Michael! │ started │
│ │ Run /init to create a … │
│ ▐▛███▜▌ │ ─────────────────────── │
│ ▝▜█████▛▘ │ What's new │
│ ▘▘ ▝▝ │ Changed `/code-review`… │
│ Fable 5 · Claude Max · larryruane@gmail.com's │ Added screen-reader an… │
│ Organization │ Fixed Windows paths wi… │
│ /sd/g/w/bitcoin-spentdb │ /release-notes for more │
╰──────────────────────────────────────────────────────────────────────────────╯
▎ Fable 5 is now a standard part of your Max plan
▎ You can use up to 50% of your weekly usage limit on Fable 5. If you hit your limit, you can continue on Fable 5 with usage credits. Fable 5 draws down usage faster than Opus 4.8. Run /model and select Fable to use it. Learn more: https://support.claude.com/en/articles/15424964-claude-fable-5-promotional-access
❯ i'm trying to make a PR here (the unstaged files) to improve initial block
download (IBD) by making the dbcache (memory cache of UTXOs, unspent coins)
more efficient. this cache actually contains many "spent" coins, which are
needed to cause the coin to be deleted when we flush the cache to disk
(leveldb). the coin is spent in this cache, but still exists in leveldb, and
we need to delete it there too, when we flush the cache. currently (without
this patch), these spent coins use the same amount of memory as an unspent
coin. I'm attempting to store spent coins in a compressed or compact fashion,
because all we really need for spent coins is the coin's identity (its key,
i.e., its COutPoint). so I'm storing them in a vector, so there's very little
memory overhead, actually zero extra allocation overhead, the memory usage
is just 36 bytes (32+4), the minimum. But now when we look for a coin, and we
don't find it in the main map (cacheCoins), we have to look for it in this
vector, and if not found there, then we go down to leveldb to try to find it
there. this extra search in the vector (m_compact_spents) requires, for any
reasonble performance, the vector to be in sorted order, then we just use
binary search. the code as it is now is somewhat faster according to my own
benchmarks, but I think this binary search could be implemented in a faster
way, I think fewer CPU cache misses. So can I ask you to do 2 things, first,
check if there are any bugs in what i have now, and also, help me figure out
a more efficient way to search this sorted vector.
● I'll start by looking at the actual changes, then review for bugs and think
about the search optimization.
Read 1 file, ran 2 shell commands (ctrl+o to expand)
❯ oh and also, i haven't got the fuzzing to compile yet
Searched for 2 patterns (ctrl+o to expand)
❯ but this does run, the functional and unit tests both pass, and i've synched
the mainnet chain successfully ... I just would like to make it faster
Searched for 2 patterns, read 4 files, listed 2 directories (ctrl+o to expand)
● Update(src/coins.cpp)
⎿  User rejected update to src/coins.cpp
● Update(src/coins.cpp)
⎿  User rejected update to src/coins.cpp
✻ Cooked for 54m 37s
❯ oh right, i commented out the calls to IsCompactSpent in the two places it's
called because i wanted to benchmark how much overhead there is in doing the
binary search, and it's significant. so of course those lines will have to be
commented back in. do you see any other bugs?
Searched for 2 patterns, read 1 file, ran 1 shell command (ctrl+o to expand)
● Yes — several. I went through the whole diff carefully (the linked-list
handling, BatchWrite merge logic, and flush ordering actually hold up well —
Gemini's Merge() is correct, and the erase-spents-before-writing-cursor
ordering in CCoinsViewDB::BatchWrite is right). Here's everything else I
found, roughly by severity.
Real bugs
1. The new constructor parameter silently breaks 5 existing call sites. You
added initial_compact_spents as the second parameter, but these callers pass
deterministic positionally, and bool converts silently to size_t:
- src/test/fuzz/coins_view.cpp:357, 370, 381 — CCoinsViewCache{&x,
/*deterministic=*/true}
- src/test/fuzz/coinscache_sim.cpp:245, 376 — new CCoinsViewCache(&bottom,
/*deterministic=*/true)
In all five, true now binds to initial_compact_spents (=1) and deterministic
becomes false. So the fuzzers silently lose their deterministic salted hasher
(breaking fuzz reproducibility) and get compaction enabled with limit 1
unintentionally. This compiles without a peep — it's the nastiest kind of API
change. Recommendation: don't make it a constructor parameter at all. Your own
LMR comment in validation.cpp:InitCache already points the way — InitCache
doesn't know the cache size, so add a setter like EnableCompactSpents(size_t)
called after construction. That eliminates this entire hazard class and the
call-site churn.
2. Fuzz compile errors (this is why fuzzing doesn't build), all in
src/test/fuzz/coins_view.cpp:
- Lines 215–216: CCoinsCacheEntry::SetDirty(*it, sentinel) / SetFresh(*it,
sentinel) — sentinel is now a CoinsSentinel, but these take CoinsCachePair&.
Needs sentinel.unspent.
- Line 224: coins_view_cache.BatchWrite(cursor, spents, best_block) — spents
is never declared in that lambda. Needs a CompactSpentsList spents; (and
ideally the fuzzer should sometimes populate it for coverage — the receiving
BatchWrite doesn't require it sorted, so arbitrary outpoints are fine).
3. m_spent_count undercount can crash CompactSpents (your known ccoins_spend
issue, but worse than an Assume). When InsertCoinsMapEntry
(coins_tests.cpp:646) inserts a SPENT+DIRTY entry raw, m_spent_count stays 0.
Beyond the failed Assume(TrySub(...)), note the knock-on: CompactSpents does
new_spents.reserve(m_spent_count) and then assert(new_spents.size() <
new_spents.capacity()) — an undercounted m_spent_count makes that a hard
assert failure (and Bitcoin Core always compiles with asserts on). My
suggested fix, three parts:
- InsertCoinsMapEntry takes CoinsSentinel& and links spent+dirty entries onto
sentinel.spent instead of unspent.
- SingleEntryCacheTest bumps a new spent_count() accessor (mirroring the
existing dirty()/usage() pattern) for spent+dirty cache coins.
- Change the production Assume(TrySub(m_spent_count,
it->second.coin.IsSpent())) sites to IsSpent() && IsDirty(). In production
every spent map entry is dirty, so it's equivalent — but it makes the counter
immune to the test-only SPENT_CLEAN/SPENT_FRESH states that never go through
SetSpent.
4. The single-entry matrix tests and compaction don't mix. CCoinsViewCacheTest
hardcodes initial_compact_spents=1, so in SingleEntryCacheTest flows a
SpendCoin can immediately compact the entry out of the map — then
GetCoinsMapEntry sees MISSING where the test expects SPENT_DIRTY. Relatedly,
SelfTest still does BOOST_CHECK_EQUAL(GetCacheSize(), count) where count only
counts map entries, but GetCacheSize() now includes m_compact_spents.size() —
that comparison fails any time the vector is non-empty at check time. I'd give
CCoinsViewCacheTest a constructor parameter: matrix tests
(SingleEntryCacheTest) use 0, the big simulation tests use 1. Worth actually
running coins_tests in a Debug (not RelWithDebInfo) build once the calls are
uncommented — Assume only aborts there, and I suspect some of the "passing"
you're seeing is Assume failing silently in RelWithDebInfo.
Correct, but fragile — document or harden
5. AddCoin on a compact-spent outpoint marks the new entry FRESH (try_emplace
inserts → fresh = !dirty = true), even though the coin still exists in
leveldb. This is correct, but only because of two unstated invariants: (a)
vector entries are never removed except at flush, so the pending erase
survives even if the FRESH entry is later spent and dropped from the map; (b)
every BatchWrite implementation must apply spents erases before cursor writes.
Both hold today (you got this right in both CCoinsViewDB::BatchWrite and
CCoinsViewCache::BatchWrite), but nothing enforces (b) for future
implementations — the CCoinsView::BatchWrite doc comment in coins.h:361 should
state it as a hard requirement: "map/cursor entries always supersede spents
entries; implementations must process spents first."
6. Duplicate outpoints can enter m_compact_spents. Spend X → compact → re-add
X with possible_overwrite=true (pre-BIP34-style) → spend again → compact
again: std::merge keeps both copies. It's benign (binary_search works on
dupes, double batch.Erase is idempotent) but memory is double-counted and it's
a latent surprise. Cheap fix: use std::ranges::set_union instead of
std::merge in CompactSpents — it drops cross-range duplicates and self-heals.
Performance nits in your new code
- txdb.cpp try_write_batch takes std::string batch_kind by value — that
constructs a std::string per coin in the flush hot loop. Make it const char*
or std::string_view.
- LogDebug(BCLog::COINDB, "begin writing coins"); is missing \n (and "Lookups
%u" too).
Debug leftovers to strip before the PR
lmr_debug asserts; m_lookups (also an unsynchronized mutable increment from
const methods); COutPoint::ToString 64-char change (transaction.cpp);
time.sleep(10) in feature_block.py; NUM_SIMULATION_ITERATIONS 40k→400k; the
lmrcount hack in CheckWriteCoins; the CLOSE/NearFull() experiment in
validation.{h,cpp} (dead code); //FreeSpents();; the various .diff/debug files
in the repo root.
Making the lookup faster
Since your benchmark says the binary search overhead is significant, here's
how I'd attack it. The problem with std::binary_search here: ~23 probes over a
385 MB array of 36-byte keys ≈ 10–20 DRAM misses per lookup (the top few tree
levels stay cached, the rest don't). And the dominant caller is a negative
lookup — an outpoint that is not in the vector, on its way to leveldb.
Best fix: a blocked Bloom filter in front (your own bloom instinct is right —
this is the concrete design). The key insight that makes it clean:
m_compact_spents is immutable between compactions — it only changes inside
CompactSpents() and ReallocCompactSpents(). So:
- Rebuild the filter inside CompactSpents (one O(n) pass over the merged
vector) and clear it in ReallocCompactSpents. No incremental maintenance, no
deletions.
- Blocked layout: one 64-byte cache line per query — pick the block from some
txid bits, set/test ~8 bits within it from other txid bits. A negative lookup
becomes one memory access instead of 10–20.
- No hash function needed in principle (txids are uniform SHA256), but since
attackers can grind txids (same reason SaltedOutpointHasher exists), mix in
the existing salt — worst case a ground txid just causes a false positive →
you fall back to binary search, so it degrades gracefully, no DoS.
- ~2 bytes/key ⇒ ~21 MB for your observed 10.7M max (vs. 385 MB for the vector
itself), FP rate well under 1%. Count it in DynamicMemoryUsage().
- Correctness constraint: false positives are fine (you just do the binary
search and get the right answer); false negatives would be a consensus bug (an
existing leveldb coin reported spent — no wait, the dangerous direction is a
compacted spent reported unspent). Bloom filters guarantee no false negatives,
which is exactly the guarantee you need.
Secondary option (compatible with the above) if you want the search itself
faster: build a parallel std::vector<uint64_t> of each key's first 8 bytes
during the merge, and binary-search that. It's 86 MB instead of 385 MB, 8 keys
per cache line, so far more of the search tree stays cached; you only touch
the full vector to confirm the final match (64-bit prefix collisions at 10M
keys are essentially nonexistent, but handle equal-prefix runs correctly). If
you want to go further, lay that prefix array out in Eytzinger (BFS) order
with __builtin_prefetch a few levels ahead — the classic Khuong & Morin
result, ~2–3× over std::binary_search. I'd skip fancy S+-tree layouts: with
36-byte keys you get <2 keys per cache line, so the prefix array captures most
of the available win with much less code.
My recommendation: bloom filter only. It targets the actual hot case
(negatives), it's ~50 lines, and its rebuild cost hides inside the sort/merge
you're already paying for.
Want me to apply the fuzz fixes and the test-accounting fix, and/or implement
the blocked bloom filter so you can benchmark it?
✻ Cogitated for 4m 26s
※ recap: Reviewing your compact-spents dbcache patch for bugs and lookup speed: found the constructor-arg misbinding in 5 fuzz call sites, fuzz compile errors, and m_spent_count test issues. Next: say the word and I'll apply the fuzz fixes and bloom filter. (disable recaps in /config)
❯ for problem 4, please change initial_compact_spents to be 0, we'll figure
that out later
● Update(src/test/coins_tests.cpp)
⎿  Added 1 line, removed 1 line
84 class CCoinsViewCacheTest : public CCoinsViewCache
85 {
86 public:
87 - explicit CCoinsViewCacheTest(CCoinsView* _base) : CCoinsVie
-wCache(_base, /*initial_compact_spents=*/1) {}
87 + explicit CCoinsViewCacheTest(CCoinsView* _base) : CCoinsVie
+wCache(_base, /*initial_compact_spents=*/0) {}
88
89 void SelfTest(bool sanity_check = true) const
90 {
● User declined to answer questions
⎿  · Which of the remaining items should this plan cover? (The
initial_compact_spents=0 test change is already done.) (Fuzz compile fixes
/ Re-enable IsCompactSpent / m_spent_count test fix / Blocked bloom filter)
· How should compaction be enabled to avoid the silent bool→size_t
constructor hazard? (Setter method (Recommended) / Keep constructor param)
● What would you like to clarify about those questions? For example — whether
you'd rather split this into separate steps, something about the bloom filter
approach, or details of the setter-vs-constructor tradeoff. Let me know what's
on your mind and I'll reshape the plan (and the questions) accordingly.
✻ Cogitated for 32s
❯ i don't want to make a major change just yet, nothing regarding bloom filter.
for now, please fix the 1, 2, 3 only
● Updated plan
⎿  /plan to preview
● User approved Claude's plan
⎿  Plan saved to: ~/.claude/plans/noble-roaming-eich.md · /plan to edit
Fix compact-spents branch: fuzz compilation, re-enable checks,
m_spent_count test accounting
Context
The working branch (2026-07-compact-spents) stores spent coins compactly in
a sorted
vector (m_compact_spents) instead of full cacheCoins map entries, to make
the dbcache
more memory-efficient during IBD. A review found three sets of problems to
fix now
(bloom filter / API refactor deliberately deferred):
1. Fuzz targets don't compile, and 5 call sites silently misbind the new
constructor parameter (bool true → size_t initial_compact_spents=1, losing
deterministic=true).
2. The two IsCompactSpent() calls are commented out (left over from
benchmarking). Without them, a compact-spent coin falls through to leveldb
and
comes back as unspent — double-spend rejection breaks (feature_block.py and
coins_tests fail).
3. m_spent_count accounting breaks in unit tests: InsertCoinsMapEntry
inserts
SPENT+DIRTY entries raw without bumping m_spent_count, so later
Assume(TrySub(...))
guards fail, and an undercount can trip
assert(new_spents.size() < new_spents.capacity()) in CompactSpents().
Already done: CCoinsViewCacheTest now uses initial_compact_spents=0
(coins_tests.cpp:87).
Changes
1. Fuzz compile fixes
src/test/fuzz/coins_view.cpp
- Lines 215–216: CCoinsCacheEntry::SetDirty(*it, sentinel) / SetFresh(*it,
sentinel)
→ pass sentinel.unspent (they take CoinsCachePair&; sentinel is now
CoinsSentinel).
- Near line 219 (in the same lambda): declare CompactSpentsList spents; —
it is used
at line 224 (coins_view_cache.BatchWrite(cursor, spents, best_block)) but
never declared.
- Lines 357, 370, 381: {&x, /*deterministic=*/true} →
{&x, /*initial_compact_spents=*/0, /*deterministic=*/true} (restores
determinism;
0 preserves pre-branch behavior).
src/test/fuzz/coinscache_sim.cpp
- Lines 245, 376: same fix — new CCoinsViewCache(&base,
/*initial_compact_spents=*/0, /*deterministic=*/true).
2. Re-enable the IsCompactSpent checks
src/coins.cpp
- PeekCoin (~line 46): uncomment the if (IsCompactSpent(outpoint)) return
std::nullopt; block.
- FetchCoin (~line 83): uncomment the
if (IsCompactSpent(outpoint)) { cacheCoins.erase(ret); return
cacheCoins.end(); } block.
- Keep the surrounding LMR commentary untouched; only remove the /* */.
3. m_spent_count test accounting fix
src/test/coins_tests.cpp
- InsertCoinsMapEntry (line 646): change parameter from CoinsCachePair&
sentinel to
CoinsSentinel& sentinel. Link the entry onto sentinel.spent when the coin
is
spent AND dirty (cache_coin.value == SPENT && cache_coin.IsDirty()), else
sentinel.unspent (test-only states SPENT_CLEAN / SPENT_FRESH stay on the
unspent
list and are deliberately not counted).
- CCoinsViewCacheTest::sentinel() accessor (line ~106): return
CoinsSentinel&
(whole struct) instead of m_sentinel.unspent; update its call sites in this
file.
- Add accessor size_t& spent_count() const { return m_spent_count; } to
CCoinsViewCacheTest, mirroring the existing dirty() / usage() accessors.
- SingleEntryCacheTest ctor (line ~692): after inserting into cache.map(),
add
cache.spent_count() += cache_coin->IsDirty() && cache_coin->value == SPENT;
- WriteCoinsViewEntry (line 676): pass the whole sentinel instead of
sentinel.unspent.
src/coins.cpp — make the counter guards immune to test-only states
(equivalent in
production, where every spent map entry is dirty). Change
Assume(TrySub(m_spent_count, <entry>.coin.IsSpent())) →
Assume(TrySub(m_spent_count, <entry>.coin.IsSpent() && <entry>.IsDirty()))
at all
sites: AddCoin, SpendCoin, and the two places in
CCoinsViewCache::BatchWrite.
(Note: order matters — evaluate before the entry is cleared/overwritten, as
the code
already does.)
Verification
1. Rebuild and run the unit tests:
cmake --build build -j$(nproc) -t test_bitcoin then
./build/bin/test_bitcoin --run_test=coins_tests (also
coinscachepair_tests).
Expect ccoins_spend and the simulation tests to pass with the checks
re-enabled.
2. Fuzz compilation (the original blocker): configure a separate fuzz
build,
cmake -B build_fuzz -DBUILD_FOR_FUZZING=ON (clang), then build the fuzz
target;
run the coins_view and coinscache_sim targets briefly with a few seconds of
random input to confirm no immediate assertion failures.
3. Functional test that exercises double-spend rejection through the
compact-spents
path: build/test/functional/feature_block.py.
(Note: it currently contains a leftover time.sleep(10) — not removed by
this plan
unless requested.)
● Starting implementation. First, re-enabling the two IsCompactSpent checks in
coins.cpp:
● Update(src/coins.cpp)
⎿  Removed 2 lines
43 //
44 // LMR I actually did comment out this call (and the one be
low) and the functional test feature_block.py
45 // fails, and the coins_tests unit test; I believe those ar
e the only mutants.
46 - /*
46 if (IsCompactSpent(outpoint)) {
47 return std::nullopt;
48 }
50 - */
49 return base->PeekCoin(outpoint);
50 }
51
● Update(src/coins.cpp)
⎿  Removed 2 lines
78 if (inserted) {
79 // LMR here is the second of 2 places we must check to
see if there's a spent coin in the
80 // vector (this is a binary search).
81 - /*
81 if (IsCompactSpent(outpoint)) {
82 cacheCoins.erase(ret);
83 return cacheCoins.end();
84 }
86 - */
85 if (auto coin{FetchCoinFromBase(outpoint)}) {
86 ret->second.coin = std::move(*coin);
87 cachedCoinsUsage += ret->second.coin.DynamicMemoryU
sage();
● Now the fuzz fixes:
● Update(src/test/fuzz/coins_view.cpp)
⎿  Added 2 lines, removed 2 lines
212 bool fresh{!coins_view_cache.PeekCoin(rand
om_out_point) && fuzzed_data_provider.ConsumeBool()};
213 bool dirty{fresh || fuzzed_data_provider.C
onsumeBool()};
214 auto it{coins_map.emplace(random_out_point
, std::move(coins_cache_entry)).first};
215 - if (dirty) CCoinsCacheEntry::SetDirty(*it,
- sentinel);
216 - if (fresh) CCoinsCacheEntry::SetFresh(*it,
- sentinel);
215 + if (dirty) CCoinsCacheEntry::SetDirty(*it,
+ sentinel.unspent);
216 + if (fresh) CCoinsCacheEntry::SetFresh(*it,
+ sentinel.unspent);
217 dirty_count += dirty;
218 }
219 auto cursor{CoinsViewCacheCursor(dirty_count,
sentinel, coins_map, /*will_erase=*/true)};
● Update(src/test/fuzz/coins_view.cpp)
⎿  Added 1 line
217 dirty_count += dirty;
218 }
219 auto cursor{CoinsViewCacheCursor(dirty_count,
sentinel, coins_map, /*will_erase=*/true)};
220 + CompactSpentsList spents;
221 uint256 best_block{coins_view_cache.GetBestBlo
ck()};
222 if (fuzzed_data_provider.ConsumeBool()) best_b
lock = ConsumeUInt256(fuzzed_data_provider);
223 // Set best block hash to non-null to satisfy
the assertion in CCoinsViewDB::BatchWrite().
Ran 2 shell commands (ctrl+o to expand)
● Now coinscache_sim.cpp:
● Update(src/test/fuzz/coinscache_sim.cpp)
⎿  Added 1 line, removed 1 line
242 ++current_height;
243 // Make sure there is always at least one CCoinsViewCa
che.
244 if (caches.empty()) {
245 - caches.emplace_back(new CCoinsViewCache(&bottom, /
-*deterministic=*/true));
245 + caches.emplace_back(new CCoinsViewCache(&bottom, /
+*initial_compact_spents=*/0, /*deterministic=*/true));
246 sim_caches[caches.size()].Wipe();
247 }
248
● Update(src/test/fuzz/coinscache_sim.cpp)
⎿  Added 1 line, removed 1 line
373 if (caches.size() != MAX_CACHES) {
374 // Apply to real caches.
375 if (provider.ConsumeBool()) {
376 - caches.emplace_back(new CCoinsViewCach
-e(&*caches.back(), /*deterministic=*/true));
376 + caches.emplace_back(new CCoinsViewCach
+e(&*caches.back(), /*initial_compact_spents=*/0, /*determinist
+ic=*/true));
377 } else {
378 caches.emplace_back(new CoinsViewOverl
ay(&*caches.back(), /*deterministic=*/true));
379 }
Searched for 1 pattern (ctrl+o to expand)
● Update(src/test/coins_tests.cpp)
⎿  Added 2 lines, removed 1 line
103 }
104
105 CCoinsMap& map() const { return cacheCoins; }
106 - CoinsCachePair& sentinel() const { return m_sentinel.unspe
-nt; }
106 + CoinsSentinel& sentinel() const { return m_sentinel; }
107 + size_t& spent_count() const { return m_spent_count; }
108 size_t& usage() const { return cachedCoinsUsage; }
109 size_t& dirty() const { return m_dirty_count; }
110 };
● Update(src/test/coins_tests.cpp)
⎿  Added 9 lines, removed 7 lines
644 }
645 }
646
647 -static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsCachePa
-ir& sentinel, const CoinEntry& cache_coin)
647 +static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsSentine
+l& sentinel, const CoinEntry& cache_coin)
648 {
649 CCoinsCacheEntry entry;
650 SetCoinsValue(cache_coin.value, entry.coin);
651 - // LMR ccoins_spend fails because here we can add a SPENT
-coin to the map
652 - // directly (instead of calling AddCoin), without bumping
-m_spent_count. So
653 - // later when we spend this spent coin, we try to decremen
-t m_spent_count but
654 - // it's still zero (so we assert). Not sure what's the bes
-t way to fix this.
651 auto [iter, inserted] = map.emplace(OUTPOINT, std::move(en
try));
652 assert(inserted);
657 - if (cache_coin.IsDirty()) CCoinsCacheEntry::SetDirty(*iter
-, sentinel);
658 - if (cache_coin.IsFresh()) CCoinsCacheEntry::SetFresh(*iter
-, sentinel);
653 + // Spent coins that are dirty belong on the spent list, ma
+tching the
654 + // production invariant maintained by CCoinsViewCache::Set
+Spent(). The
655 + // test-only states SPENT_CLEAN and SPENT_FRESH cannot occ
+ur in production
656 + // and are deliberately kept off the spent list (and not c
+ounted).
657 + const bool spent{cache_coin.value == SPENT && cache_coin.I
+sDirty()};
658 + CoinsCachePair& list{spent ? sentinel.spent : sentinel.uns
+pent};
659 + if (cache_coin.IsDirty()) CCoinsCacheEntry::SetDirty(*iter
+, list);
660 + if (cache_coin.IsFresh()) CCoinsCacheEntry::SetFresh(*iter
+, list);
661 return iter->second.coin.DynamicMemoryUsage();
662 }
663
● Update(src/test/coins_tests.cpp)
⎿  Added 1 line, removed 1 line
676 CoinsSentinel sentinel{};
677 CCoinsMapMemoryResource resource;
678 CCoinsMap map{0, CCoinsMap::hasher{}, CCoinsMap::key_equal
{}, &resource};
679 - if (cache_coin) InsertCoinsMapEntry(map, sentinel.unspent,
- *cache_coin);
679 + if (cache_coin) InsertCoinsMapEntry(map, sentinel, *cache_
+coin);
680 size_t dirty_count{cache_coin && cache_coin->IsDirty()};
681 auto cursor{CoinsViewCacheCursor(dirty_count, sentinel, ma
p, /*will_erase=*/true)};
682 CompactSpentsList spents;
● Update(src/test/coins_tests.cpp)
⎿  Added 1 line
694 if (cache_coin) {
695 cache.usage() += InsertCoinsMapEntry(cache.map(),
cache.sentinel(), *cache_coin);
696 cache.dirty() += cache_coin->IsDirty();
697 + cache.spent_count() += cache_coin->value == SPENT
+&& cache_coin->IsDirty();
698 }
699 }
700
Searched for 1 pattern, ran 2 shell commands (ctrl+o to expand)
● While that builds, let me configure the fuzz build (the actual fuzz
compilation will wait for the main build to finish so they don't fight over
cores):
Ran 1 shell command (ctrl+o to expand)
● No clang available — I'll configure with GCC and -DBUILD_FUZZ_BINARY=ON, which
builds the fuzz targets (compile verification + corpus runs, just no
libFuzzer instrumentation):
Read 1 file, ran 5 shell commands (ctrl+o to expand)
● Both fuzz translation units now compile cleanly. Checking on the main build:
Read 1 file (ctrl+o to expand)
● The main build output is piped through tail, so it only prints at completion —
I'll get notified. Meanwhile, starting the full fuzz build in the background
(fresh build dir, so it takes a while):
Ran 1 shell command (ctrl+o to expand)
● Background command "Build test_bitcoin" completed (exit code 0)
● Main build succeeded. Running the unit tests:
Ran 1 shell command (ctrl+o to expand)
● ccoins_add has 15 failures — an entry ends up DIRTY where the test expects
DIRTY|FRESH. Let me look at the failing check:
Read 1 file (ctrl+o to expand)
● Update(src/coins.cpp)
⎿  Added 4 lines, removed 2 lines
134 it->second.coin = std::move(coin);
135 // LMR since spent and unspent coins are on different list
s (different sentinels), we have to
136 // remove this coin from whatever list it might be on (I t
hink in practice it's never on any list)
137 - // and make sure it gets added to the unspent list.
137 + // and make sure it gets added to the unspent list. SetCle
+an() also clears the FRESH flag, which
138 + // AddCoin() has always preserved on overwrite, so remembe
+r it and re-apply it below.
139 + const bool was_fresh{it->second.IsFresh()};
140 it->second.SetClean();
141 CCoinsCacheEntry::SetDirty(*it, m_sentinel.unspent);
142 ++m_dirty_count;
141 - if (fresh) CCoinsCacheEntry::SetFresh(*it, m_sentinel.unsp
-ent);
143 + if (fresh || was_fresh) CCoinsCacheEntry::SetFresh(*it, m_
+sentinel.unspent);
144 cachedCoinsUsage += it->second.coin.DynamicMemoryUsage();
145 TRACEPOINT(utxocache, add,
146 outpoint.hash.data(),
Ran 1 shell command (ctrl+o to expand)
● Background command "Build fuzz binary in background" completed (exit code 0)
● Unit tests all pass now, and the fuzz binary built successfully. Running the
full unit suite quickly plus the two fuzz targets on random inputs:
Ran 7 shell commands (ctrl+o to expand)
● Background command "Rebuild fuzz binary with BUILD_FOR_FUZZING=ON" completed
(exit code 0)
❯ just to let you know a convention i use, i always use gcc for optimized
builds (the build directory), and clang for debug builds (the debug
directory) because clang works much better with vscode (code) debugging. but
my installed clang compilers include the version number, 22, so it's clang-22
and clang++-22, so please use those for the debug builds
Searched for 1 pattern, ran 2 shell commands, wrote 2 memories (ctrl+o to expand)
● Update(src/coins.cpp)
⎿  Added 9 lines, removed 5 lines
151 }
152
153 void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& ou
tpoint, Coin&& coin) {
154 - // LMR we put this on the unspent list below, so let's mak
-e sure it's unspent.
155 - // I'm almost positive this can't be a spent coin, that wo
-uld make no sense.
156 - assert(!coin.IsSpent());
154 + // LMR spent coins can be emplaced here (coinsviewoverlay_
+tests does it to
155 + // simulate a parent cache containing spent entries), so t
+hey must go on the
156 + // spent list to maintain the spent-list/m_spent_count inv
+ariant.
157 const auto mem_usage{coin.DynamicMemoryUsage()};
158 auto [it, inserted] = cacheCoins.try_emplace(std::move(out
point), std::move(coin));
159 if (inserted) {
160 - CCoinsCacheEntry::SetDirty(*it, m_sentinel.unspent);
161 - ++m_dirty_count;
160 + if (it->second.coin.IsSpent()) {
161 + SetSpent(*it);
162 + } else {
163 + CCoinsCacheEntry::SetDirty(*it, m_sentinel.unspent
+);
164 + ++m_dirty_count;
165 + }
166 cachedCoinsUsage += mem_usage;
167 }
168 }
Searched for 4 patterns, ran 9 shell commands (ctrl+o to expand)
● Background command "Rebuild fuzz binary with latest coins.cpp" completed (exit
code 0)
● Update(src/test/fuzz/coinscache_sim.cpp)
⎿  Added 1 line, removed 1 line
375 if (provider.ConsumeBool()) {
376 caches.emplace_back(new CCoinsViewCach
e(&*caches.back(), /*initial_compact_spents=*/0, /*determinist
ic=*/true));
377 } else {
378 - caches.emplace_back(new CoinsViewOverl
-ay(&*caches.back(), /*deterministic=*/true));
378 + caches.emplace_back(new CoinsViewOverl
+ay(&*caches.back(), /*initial_compact_spents=*/0, /*determinis
+tic=*/true));
379 }
380 // Apply to simulation data.
381 sim_caches[caches.size()].Wipe();
Searched for 1 pattern, ran 2 shell commands (ctrl+o to expand)
● All done — everything in the plan is fixed and verified, plus two additional
real bugs that the newly-passing tests exposed.
What was fixed
1. Fuzz compilation (the original blocker):
- coins_view.cpp: SetDirty/SetFresh now take sentinel.unspent; added the
missing CompactSpentsList spents; declaration.
- The positional-argument hazard turned out to be at seven call sites, not
five — gdb caught a CoinsViewOverlay at coinscache_sim.cpp:378 that my grep
missed because it doesn't contain the string "CCoinsViewCache". All sites in
coins_view.cpp (4) and coinscache_sim.cpp (3) now pass
/*initial_compact_spents=*/0, /*deterministic=*/true explicitly. That last one
was silently constructing caches with deterministic=false, which is what
tripped the fuzzer's "global random state" detector.
2. Both IsCompactSpent() calls re-enabled in PeekCoin and FetchCoin.
3. m_spent_count accounting: InsertCoinsMapEntry now takes the whole
CoinsSentinel and links spent+dirty entries onto the spent list;
SingleEntryCacheTest bumps a new spent_count() accessor; the four
Assume(TrySub(m_spent_count, ...)) guards now use IsSpent() && IsDirty() so
test-only states can't corrupt the counter.
Two additional bugs found by the tests
- AddCoin was dropping the FRESH flag on overwrite. The new SetClean() (needed
to move entries between lists) clears FRESH, which upstream AddCoin always
preserved — all 15 ccoins_add failures were exactly the FRESH-preserving rows.
Fixed by capturing the flag before SetClean and re-applying it, same pattern
as your BatchWrite "normal modification" path.
- EmplaceCoinInternalDANGER's assert(!coin.IsSpent()) was wrong —
coinsviewoverlay_tests legitimately emplaces spent coins. But your instinct
was half right: a spent coin there would land on the unspent list and evade
m_spent_count. It now routes spent coins through SetSpent().
Verification results
- coins_tests, coinsviewoverlay_tests, coinscachepair_tests: all pass.
- Full unit suite: passes except validation_flush_tests (3625 failures) —
that's your pre-existing CLOSE/NearFull experiment in validation.{h,cpp}
returning CLOSE where the test expects OK; I left it untouched.
- feature_block.py: passes against the fully rebuilt bitcoind.
- All four coins fuzz targets (coins_view, coins_view_db, coins_view_overlay,
coinscache_sim) compile and run clean over 200 random inputs each.
One note on your compiler convention (saved to memory): the build_fuzz/
directory I created uses gcc with -DBUILD_FOR_FUZZING=ON, which executes
corpus inputs but has no libFuzzer instrumentation. For actual fuzzing you'll
want it reconfigured with -DCMAKE_C_COMPILER=clang-22
-DCMAKE_CXX_COMPILER=clang++-22 -DSANITIZERS=fuzzer — happy to set that up
when you want it.
✻ Sautéed for 30m 59s
※ recap: Working on your compact-spents dbcache branch: fixed fuzz compilation, re-enabled the IsCompactSpent checks, and fixed spent-count test accounting; all tests and fuzz targets now pass. Next: set up a clang-22 libFuzzer build if you want real fuzzing. (disable recaps in /config)
❯ i just did a git add on all modified files, just so i can see more easily the
edits you will do. so let's go forward now with the bloom filter
● Staged — good, that makes the new edits easy to see with git diff.
Implementing the blocked bloom filter now. Design recap: one 64-byte
cache-line probe answers the dominant negative lookups; false positives fall
back to the binary search (so worst case is exactly today's behavior); the
filter is rebuilt inside CompactSpents() — the only place the vector changes —
and cleared with it.
First the filter class in coins.h:
Read 1 file (ctrl+o to expand)
● Update(src/coins.h)
⎿  Added 1 line
9 #include <attributes.h>
10 #include <compressor.h>
11 #include <core_memusage.h>
12 +#include <crypto/common.h>
13 #include <memusage.h>
14 #include <primitives/transaction.h>
15 #include <serialize.h>
● Update(src/coins.h)
⎿  Added 108 lines
230 // A sorted list of spent coins
231 using CompactSpentsList = std::vector<COutPoint>;
232
233 +/**
234 + * A blocked bloom filter over the compact spents vector (m_co
+mpact_spents).
235 + *
236 + * Most IsCompactSpent() lookups are negative: the outpoint be
+ing fetched is not
237 + * a compacted spent coin, and the lookup proceeds to the base
+ view. A binary
238 + * search over the (large) sorted vector costs O(log n) scatte
+red memory reads,
239 + * most of which are cache misses. This filter answers negativ
+e lookups with a
240 + * single 64-byte cache-line probe. Positive answers (rare, an
+d including false
241 + * positives) fall back to the binary search, so a false posit
+ive only costs an
242 + * unnecessary search; false negatives cannot occur, which is
+the property that
243 + * correctness depends on.
244 + *
245 + * The filter must always describe the current contents of m_c
+ompact_spents.
246 + * That vector only changes wholesale (rebuilt in CompactSpent
+s(), cleared in
247 + * ReallocCompactSpents()), so the filter is rebuilt or cleare
+d at those same
248 + * two places, and needs no incremental deletion support.
249 + *
250 + * Each entry sets HASHES bits within a single 64-byte block;
+~16 bits per entry
251 + * of total filter space keeps the false positive rate well un
+der 1%. Sizing to
252 + * ~2 bytes per entry is small next to the 36 bytes per entry
+of the vector.
253 + */
254 +class CompactSpentsFilter
255 +{
256 + //! 64 bytes, i.e. one cache line, per block.
257 + static constexpr size_t WORDS_PER_BLOCK{8};
258 + //! Bits set/tested per entry. 7 bit positions of 9 bits e
+ach (a block holds
259 + //! 512 bits) consume 63 of the 64 bits of one hash word.
260 + static constexpr int HASHES{7};
261 + //! Target filter bits per entry, before rounding the bloc
+k count up to a
262 + //! power of two.
263 + static constexpr size_t BITS_PER_ENTRY{16};
264 +
265 + //! Salts, random unless deterministic (see SaltedOutpoint
+Hasher). The txid
266 + //! part of an outpoint is already uniformly distributed,
+but is chosen by
267 + //! peers; the salt keeps an attacker from grinding txids
+that concentrate
268 + //! in one block. Even a successful grinding attack only r
+aises the false
269 + //! positive rate, degrading to the pre-filter behavior (b
+inary search).
270 + const uint64_t m_k0, m_k1;
271 + std::vector<uint64_t> m_words;
272 + uint64_t m_block_mask{0};
273 +
274 + static uint64_t Mix(uint64_t x) noexcept
275 + {
276 + // splitmix64 finalizer
277 + x ^= x >> 30; x *= 0xbf58476d1ce4e5b9ULL;
278 + x ^= x >> 27; x *= 0x94d049bb133111ebULL;
279 + x ^= x >> 31;
280 + return x;
281 + }
282 +
283 + //! Two independent 64-bit hashes: one selects the block,
+one supplies the
284 + //! bit positions within it.
285 + std::pair<uint64_t, uint64_t> Hash(const COutPoint& outpoi
+nt) const noexcept
286 + {
287 + const uint64_t h1{Mix((ReadLE64(outpoint.hash.data())
++ outpoint.n) ^ m_k0)};
288 + const uint64_t h2{Mix((ReadLE64(outpoint.hash.data() +
+ 8) - outpoint.n) ^ m_k1)};
289 + return {h1, h2};
290 + }
291 +
292 +public:
293 + explicit CompactSpentsFilter(bool deterministic);
294 +
295 + //! Clear the filter and size it for the given number of e
+ntries (0 frees all
296 + //! memory). Insert() may then be called for each entry.
297 + void Reset(size_t count)
298 + {
299 + m_words.clear();
300 + m_words.shrink_to_fit();
301 + m_block_mask = 0;
302 + if (count == 0) return;
303 + const size_t min_blocks{(count * BITS_PER_ENTRY + 511)
+ / 512};
304 + size_t blocks{1};
305 + while (blocks < min_blocks) blocks <<= 1;
306 + m_words.assign(blocks * WORDS_PER_BLOCK, 0);
307 + m_block_mask = blocks - 1;
308 + }
309 +
310 + void Insert(const COutPoint& outpoint) noexcept
311 + {
312 + const auto [h1, h2] = Hash(outpoint);
313 + uint64_t* block{&m_words[(h1 & m_block_mask) * WORDS_P
+ER_BLOCK]};
314 + uint64_t bits{h2};
315 + for (int i{0}; i < HASHES; ++i) {
316 + const unsigned pos(bits & 511);
317 + block[pos >> 6] |= uint64_t{1} << (pos & 63);
318 + bits >>= 9;
319 + }
320 + }
321 +
322 + //! False means definitely not inserted; true means probab
+ly inserted.
323 + //! An empty filter (empty set) returns false for everythi
+ng.
324 + bool MayContain(const COutPoint& outpoint) const noexcept
325 + {
326 + if (m_words.empty()) return false;
327 + const auto [h1, h2] = Hash(outpoint);
328 + const uint64_t* block{&m_words[(h1 & m_block_mask) * W
+ORDS_PER_BLOCK]};
329 + uint64_t bits{h2};
330 + for (int i{0}; i < HASHES; ++i) {
331 + const unsigned pos(bits & 511);
332 + if ((block[pos >> 6] & uint64_t{1} << (pos & 63))
+== 0) return false;
333 + bits >>= 9;
334 + }
335 + return true;
336 + }
337 +
338 + size_t DynamicMemoryUsage() const { return memusage::Dynam
+icUsage(m_words); }
339 +};
340 +
341 using CCoinsMapMemoryResource = CCoinsMap::allocator_type::Res
ourceType;
342
343 /** Cursor for iterating over CoinsView state */
● Now wire it into CCoinsViewCache — member declaration and the new
IsCompactSpent:
● Update(src/coins.h)
⎿  Added 5 lines
559 // (Increases over time.)
560 size_t m_spents_limit;
561
562 + // Bloom filter answering most (negative) m_compact_spents
+ lookups with a
563 + // single cache-line probe; must always describe m_compact
+_spents exactly
564 + // (rebuilt in CompactSpents(), cleared in ReallocCompactS
+pents()).
565 + mutable CompactSpentsFilter m_spents_filter;
566 +
567 /* Cached dynamic memory usage for the inner Coin objects.
*/
568 mutable size_t cachedCoinsUsage{0};
569 /* Running count of dirty Coin cache entries. */
● Update(src/coins.h)
⎿  Added 4 lines, removed 5 lines
684 //! See: https://stackoverflow.com/questions/42114044/how-
to-release-unordered-map-memory
685 void ReallocateCache();
686
687 - // LMR could try putting a bloom filter in front of this l
-ookup:
688 - // https://share.google/aimode/IzqmZE2rXMb4TbPJf
687 bool IsCompactSpent(const COutPoint& outpoint) const {
690 - // LMR guarding with sorted_index > 0 is just for perf
-ormance, unmeasured
691 - return m_compact_spents.size() > 0 && std::binary_sear
-ch(
692 - m_compact_spents.begin(), m_compact_spents.end(),
-outpoint);
688 + // The filter cheaply resolves the common case (not a
+compacted spent,
689 + // including the empty-vector case); the binary search
+ confirms the rest.
690 + if (!m_spents_filter.MayContain(outpoint)) return fals
+e;
691 + return std::binary_search(m_compact_spents.begin(), m_
+compact_spents.end(), outpoint);
692 }
693
694 // LMR Compact (verb) the existing spent map coins
● Update(src/coins.cpp)
⎿  Added 6 lines
23 return instance;
24 }
25
26 +CompactSpentsFilter::CompactSpentsFilter(bool deterministic) :
27 + m_k0{deterministic ? uint64_t{0x76b0a5e15e2c1bd9} : FastRan
+domContext().rand64()},
28 + m_k1{deterministic ? uint64_t{0xd79f38f1bd264268} : FastRan
+domContext().rand64()}
29 +{
30 +}
31 +
32 // Like GetCoin() except doesn't populate the local cache.
33 std::optional<Coin> CCoinsViewCache::PeekCoin(const COutPoint&
outpoint) const
34 {
● Update(src/coins.cpp)
⎿  Added 2 lines, removed 1 line
61 CCoinsViewCache::CCoinsViewCache(CCoinsView* in_base, size_t in
itial_compact_spents, bool deterministic) :
62 CCoinsViewBacked(in_base), m_deterministic(deterministic),
63 cacheCoins(0, SaltedOutpointHasher(/*deterministic=*/determ
inistic), CCoinsMap::key_equal{}, &m_cache_coins_memory_resourc
e),
64 - m_initial_compact_spents(initial_compact_spents), m_spents_
-limit(initial_compact_spents)
64 + m_initial_compact_spents(initial_compact_spents), m_spents_
+limit(initial_compact_spents),
65 + m_spents_filter(deterministic)
66 {
67 // LMR instead of initializing two sentinels (spent and uns
pent) here, makes more sense to do this in
68 // the CoinsSentinel constructor. Might even make even more
sense to do it in the CoinsCachePair constructor,
● Update(src/coins.cpp)
⎿  Added 1 line, removed 1 line
72 size_t CCoinsViewCache::DynamicMemoryUsage() const {
73 // LMR this new vector must count as cache space used (like
cachedCoinsUsage, it's allocated using
74 // the system allocator), not the pool resource.
75 - return memusage::DynamicUsage(cacheCoins) + cachedCoinsUsag
-e + memusage::DynamicUsage(m_compact_spents);
75 + return memusage::DynamicUsage(cacheCoins) + cachedCoinsUsag
+e + memusage::DynamicUsage(m_compact_spents) + m_spents_filter.
+DynamicMemoryUsage();
76 }
77
78 std::optional<Coin> CCoinsViewCache::FetchCoinFromBase(const CO
utPoint& outpoint) const
● Update(src/coins.cpp)
⎿  Added 1 line
433 m_compact_spents.shrink_to_fit();
434 m_compact_spents.reserve(m_initial_compact_spents);
435 m_spents_limit = m_initial_compact_spents;
436 + m_spents_filter.Reset(0);
437 }
438 }
439 void CCoinsViewCache::Flush(bool reallocate_cache)
● Update(src/coins.cpp)
⎿  Added 7 lines
626 new_spents.begin(), new_spents.end(),
627 std::back_inserter(spents));
628 m_compact_spents = std::move(spents);
629 + // Rebuild the bloom filter to match the new vector conten
+ts. This is the
630 + // only place the vector gains entries, so an insertion pa
+ss here keeps the
631 + // filter's no-false-negative guarantee without any increm
+ental maintenance.
632 + m_spents_filter.Reset(m_compact_spents.size());
633 + for (const COutPoint& outpoint : m_compact_spents) {
634 + m_spents_filter.Insert(outpoint);
635 + }
636 // 20% growth each flush cycle
637 // LMR this 20% is just an intuitive guess, of course
638 m_spents_limit = m_compact_spents.size() * 20 / 100;
● Update(src/coins.cpp)
⎿  Added 4 lines
697 // LMR wanted to say m_compact_spends[i] >= m_compact_
spends[i-1] but there's no >= operator on COutPoint
698 assert(!(m_compact_spents[i] < m_compact_spents[i-1]))
;
699 }
700 + // the bloom filter must have no false negatives for the v
+ector's contents
701 + for (const COutPoint& outpoint : m_compact_spents) {
702 + assert(m_spents_filter.MayContain(outpoint));
703 + }
704 }
705
706 static const uint64_t MIN_TRANSACTION_OUTPUT_WEIGHT{WITNESS_SC
ALE_FACTOR * ::GetSerializeSize(CTxOut())};
● Now the test-side updates: SelfTest memory accounting and a dedicated filter
unit test. Checking the suite fixture first:
Searched for 1 pattern (ctrl+o to expand)
● Update(src/test/coins_tests.cpp)
⎿  Added 1 line, removed 1 line
89 void SelfTest(bool sanity_check = true) const
90 {
91 // Manually recompute the dynamic usage of the whole da
ta, and compare it.
92 - size_t ret = memusage::DynamicUsage(cacheCoins) + memus
-age::DynamicUsage(m_compact_spents);
92 + size_t ret = memusage::DynamicUsage(cacheCoins) + memus
+age::DynamicUsage(m_compact_spents) + m_spents_filter.DynamicMe
+moryUsage();
93 size_t count = 0;
94 for (const auto& entry : cacheCoins) {
95 ret += entry.second.coin.DynamicMemoryUsage();
● Update(src/test/coins_tests.cpp) ⎿  Added 35 lines
1217 BOOST_CHECK_EQUAL(cache.GetDirtyCount(), 0U);
1218 }
1219
1220 +BOOST_AUTO_TEST_CASE(compact_spents_filter_basic)
1221 +{
1222 + CompactSpentsFilter filter{/*deterministic=*/false};
1223 +
1224 + // An empty filter contains nothing.
1225 + BOOST_CHECK(!filter.MayContain(COutPoint(Txid::FromUint25
+6(m_rng.rand256()), 0)));
1226 +
1227 + constexpr size_t NUM_ENTRIES{10'000};
1228 + std::vector<COutPoint> entries;
1229 + entries.reserve(NUM_ENTRIES);
1230 + for (size_t i{0}; i < NUM_ENTRIES; ++i) {
1231 + entries.emplace_back(Txid::FromUint256(m_rng.rand256(
+)), m_rng.randbits(2));
1232 + }
1233 + filter.Reset(entries.size());
1234 + for (const COutPoint& outpoint : entries) filter.Insert(o
+utpoint);
1235 +
1236 + // No false negatives: correctness of IsCompactSpent() de
+pends on this.
1237 + for (const COutPoint& outpoint : entries) {
1238 + BOOST_CHECK(filter.MayContain(outpoint));
1239 + }
1240 +
1241 + // The false positive rate for outpoints never inserted s
+hould be far below
1242 + // this generous 1% bound (expected is well under 0.1%).
1243 + size_t false_positives{0};
1244 + constexpr size_t NUM_PROBES{100'000};
1245 + for (size_t i{0}; i < NUM_PROBES; ++i) {
1246 + false_positives += filter.MayContain(COutPoint(Txid::
+FromUint256(m_rng.rand256()), 0));
1247 + }
1248 + BOOST_CHECK_LT(false_positives, NUM_PROBES / 100);
1249 +
1250 + // Reset(0) empties the filter again.
1251 + filter.Reset(0);
1252 + BOOST_CHECK(!filter.MayContain(entries[0]));
1253 +}
1254 +
1255 BOOST_AUTO_TEST_CASE(ccoins_peekcoin)
1256 {
1257 CCoinsViewTest base{m_rng};
● Update(src/test/coins_tests.cpp)
⎿  Added 32 lines
1252 BOOST_CHECK(!filter.MayContain(entries[0]));
1253 }
1254
1255 +// End-to-end: spent coins compacted into m_compact_spents mu
+st be reported as
1256 +// nonexistent by every lookup path, even though the base sti
+ll has them; this
1257 +// exercises CompactSpents() (merge + filter rebuild) and IsC
+ompactSpent().
1258 +BOOST_AUTO_TEST_CASE(compact_spents_lookup)
1259 +{
1260 + CCoinsViewCacheTest base{&CoinsViewEmpty::Get()};
1261 + std::vector<COutPoint> outpoints;
1262 + for (int i{0}; i < 100; ++i) {
1263 + COutPoint outpoint{Txid::FromUint256(m_rng.rand256())
+, 0};
1264 + base.AddCoin(outpoint, Coin{CTxOut{i + 1, CScript{}},
+ 1, false}, false);
1265 + outpoints.push_back(outpoint);
1266 + }
1267 + // Compact aggressively: every spend immediately moves to
+ the vector, so
1268 + // each iteration below performs a merge and a filter reb
+uild.
1269 + CCoinsViewCache cache{&base, /*initial_compact_spents=*/1
+};
1270 + for (const COutPoint& outpoint : outpoints) {
1271 + BOOST_CHECK(cache.SpendCoin(outpoint));
1272 + }
1273 + cache.SanityCheck();
1274 + for (const COutPoint& outpoint : outpoints) {
1275 + BOOST_CHECK(!cache.HaveCoin(outpoint));
1276 + BOOST_CHECK(!cache.GetCoin(outpoint));
1277 + BOOST_CHECK(!cache.PeekCoin(outpoint));
1278 + BOOST_CHECK(base.PeekCoin(outpoint)); // base unaware
+ until Flush
1279 + }
1280 + cache.Flush();
1281 + for (const COutPoint& outpoint : outpoints) {
1282 + BOOST_CHECK(!base.PeekCoin(outpoint));
1283 + BOOST_CHECK(!cache.HaveCoin(outpoint));
1284 + }
1285 +}
1286 +
1287 BOOST_AUTO_TEST_CASE(ccoins_peekcoin)
1288 {
1289 CCoinsViewTest base{m_rng};
Ran 2 shell commands (ctrl+o to expand)
● All coins suites pass including the two new tests. Now the fuzz build and
functional test:
Ran 4 shell commands (ctrl+o to expand)
● Background command "Rebuild fuzz binary with bloom filter" completed (exit
code 0)
● The bloom filter is implemented and fully verified. Since you staged
beforehand, git diff now shows exactly these changes.
What was added
src/coins.h — new CompactSpentsFilter class (a blocked bloom filter), a
m_spents_filter member on CCoinsViewCache, and IsCompactSpent() now probes the
filter first, only falling back to the binary search on a (rare) positive:
- One 64-byte cache line per query: the block is selected by one salted hash,
and 7 bit positions within it come from a second. A negative lookup — the
overwhelmingly common case — costs a single memory access instead of the
~log₂(n) scattered reads of the binary search over the 385 MB vector.
- No false negatives (the property correctness depends on); false positives
just mean an unnecessary binary search, i.e. exactly the pre-filter behavior.
At ~16+ bits per entry with 7 probes, the FP rate is well under 0.1%.
- Sizing: ~2–4 bytes per entry (power-of-two block count), so ~21–42 MB at
your observed 10.7M-entry peak, versus 385 MB for the vector itself. It's
counted in DynamicMemoryUsage().
- Salted like SaltedOutpointHasher (fixed salts in deterministic mode, so the
fuzzers stay reproducible): txids are attacker-chosen, and the salt prevents
grinding outpoints into one block. Even a successful attack only raises the FP
rate — degrading to today's behavior, not breaking anything.
src/coins.cpp — the filter is rebuilt at the end of CompactSpents() (the only
place the vector gains entries, and the rebuild pass hides behind the
sort/merge you already pay for there) and cleared in ReallocCompactSpents().
SanityCheck() now also asserts the no-false-negative invariant over the whole
vector.
src/test/coins_tests.cpp — two new tests:
- compact_spents_filter_basic: direct filter test — empty-filter behavior,
zero false negatives over 10k entries, FP rate below 1% over 100k probes, and
Reset(0).
- compact_spents_lookup: end-to-end — a cache with initial_compact_spents=1
spends 100 coins (forcing a merge + filter rebuild per spend), then verifies
HaveCoin/GetCoin/PeekCoin all report the compacted coins as gone while the
base still holds them, and that Flush() propagates the erasures. This matters
because the regular test caches now run with compaction disabled.
Verification
- All coins-related unit suites pass; full suite shows only the pre-existing
validation_flush_tests failures (your CLOSE experiment).
- feature_block.py passes against the rebuilt bitcoind.
- All four coins fuzz targets run clean over the 200-input corpus.
build/bin/bitcoind is already rebuilt with the filter, so you can benchmark
your reindex directly. One thing to watch in your benchmark: the filter helps
precisely the case your earlier experiment measured (cache-miss lookups that
aren't compacted spents), so the fair comparison is this build vs. the staged
version with the binary-search-only IsCompactSpent — not vs. the
checks-commented-out build, which is the unreachable lower bound.
✻ Worked for 15m 11s
※ recap: Working on your compact-spents dbcache branch: fixed the fuzz/test bugs, then added the blocked bloom filter in front of the spent-vector binary search; all tests and fuzz targets pass. Next: run your reindex benchmark to measure the speedup. (disable recaps in /config)
❯ reindex-chainstate on mainnet to height 938343, ssd datadir, master (using
the bash builtin time command): 219m52.003s (real time), this branch (with
the bloom filter): 207m33.056s
● That's a solid result — 12m19s saved, about a 5.6% speedup on a 3.5-hour
reindex. Worth recording, and a few thoughts on what it does and doesn't tell
us:
What it shows: the branch wins end-to-end on real hardware at full mainnet
scale. Given that a reindex-chainstate is dominated by connect-block work and
leveldb I/O, a 5.6% wall-clock improvement from a dbcache-efficiency change is
meaningful.
What it doesn't isolate yet — two effects are bundled in that number:
1. The compact storage itself: more unspent coins fit in the same dbcache, so
fewer leveldb misses and fewer/later flushes. This is the core value of the
patch.
2. The bloom filter: cheapens the per-miss IsCompactSpent probe, which you'd
measured as significant with the plain binary search.
For the eventual PR discussion, two comparisons would make the story airtight:
- This branch with vs. without the filter (your earlier binary-search-only
staged state) — isolates the filter's contribution and justifies its ~50
lines.
- Flush counts and leveldb lookup totals between master and branch — your own
instrumentation already logs both (Committed ... to coin database lines and
the Lookups %u counter). If the branch shows materially fewer flushes and
fewer db lookups at the same -dbcache, that's the mechanism demonstrated
directly, not just the symptom.
One caveat worth a sentence in any writeup: single runs each. Reindex is
fairly reproducible on the same machine, but if the ~5% number becomes a
headline claim, a second run of each (or at least confirming the gap exceeds
your run-to-run variance) will preempt the obvious review question.
Let me save the result so we have it in future sessions:
Wrote 2 memories (ctrl+o to expand)
● Saved. Whenever you're ready, the natural next steps are the
filter-vs-no-filter run to isolate its contribution (just git stash the
unstaged filter changes and rebuild — your staged state is exactly the
binary-search-only version, which makes that comparison easy), or comparing
the flush/lookup counts from the debug logs of the two runs you already have.
✻ Worked for 47s
❯ i thought it's a larger speedup than 5.6%: try this shell command line: echo
'4k 1 207 60*33+ 232 60*53+ / - p'|dc
Ran 1 shell command (ctrl+o to expand)
● Your dc line gives 10.88% — but it uses 232m53s as the master baseline, while
your previous message said master was 219m52.003s (which is where my 5.6% came
from: 1 - 12453/13192 = .0561, second line above).
So the speedup is 10.9% if master's real time was 232m53s, or 5.6% if it was
219m52s. Which one is the actual master -reindex-chainstate real time?
(Perhaps the 219m52s was a different run — different height, dbcache, or the
branch-without-filter?) I'll update the saved benchmark note once you confirm.
✻ Cogitated for 25s
❯ oh sorry, i did give you the wrong time for master, it's 192m36.540s
● Hmm, that makes the picture inconsistent rather than clearer — with master =
192m36.540s and the branch = 207m33.056s (from your first message), the branch
would be ~7.8% slower, not faster. And your dc line used yet another value,
232m53s, as the baseline.
So far four times have appeared: 219m52.003s, 207m33.056s, 232m53s, and
192m36.540s. Depending on pairing:
- master 232m53 vs branch 207m33 → 10.9% faster (what your dc computed)
- master 219m52 vs branch 207m33 → 5.6% faster
- master 207m33 vs branch 192m36 → 7.2% faster
- master 192m36 vs branch 207m33 → 7.8% slower
Could you check your notes and give me the definitive pair — master real time
and branch(+bloom) real time? I'll update the saved benchmark record with
whichever it is.
✻ Worked for 16s
❯ typo again, sorry! master is 232m53.381s (same height, ssd, everything the
same as the run i just did with your new bloom code)
● No problem — so the definitive pair is master 232m53.381s vs. branch+bloom
207m33.056s: 25m20s saved, a 10.9% speedup. That matches your dc computation
exactly. Updating the record:
Wrote 1 memory (ctrl+o to expand)
● That's a genuinely strong result — 10.9% off a full mainnet reindex is the
kind of headline number that gets a performance PR taken seriously. The one
measurement still worth having before you write it up is the branch without
the filter (your staged binary-search-only state) at the same settings, so you
can attribute the win between the compact storage itself and the filter — and
the flush-count/Lookups comparison from the two debug logs you already have
would show the mechanism directly.
✻ Churned for 13s
❯ yes, that Lookups count, the last one printed is 609203665 ... i just copied
the debug.log file to debuglog-07-25-ssd-bloom in the current directory (you
can see that number there near the end of the file). The debug.log file for
the corresponding run on the master branch is at
/sd/g/w/bitcoin-master/debug-938343-jul14 (in case you want to look at it),
but one small detail is different in the Lookup value printed: in that master
branch, I reset Lookups to zero on each flush (whereas in this new code i
don't), so i have to add up all the Lookup values, i did that this way: grep
Lookup /sd/g/w/bitcoin-master/debug-938343-jul14 |awk '{i+=$4}END{print i}'
....... and the result is 714594427 ..... so the new code is definitely doing
fewer leveldb lookups, as we would expect (more coins are found in the
memory cache since the cache is effectively larger)
● Nice — that's the mechanism confirmed. Let me verify both numbers from the
logs and pull the flush counts too, since that's the other half of the story:
Ran 2 shell commands (ctrl+o to expand)
● Verified, and here's the full mechanism picture from the two logs:
┌───────────────────────┬───────────────┬────────────────────────┬────────┐
│ Metric │ master │ branch (+bloom) │ change │
├───────────────────────┼───────────────┼────────────────────────┼────────┤
│ leveldb lookups │ 714,594,427 │ 609,203,665 │ −14.7% │
├───────────────────────┼───────────────┼────────────────────────┼────────┤
│ cache flushes │ 184 │ 112 │ −39% │
├───────────────────────┼───────────────┼────────────────────────┼────────┤
│ leveldb batch ops │ │ 800,146,351 + │ │
│ (writes+erases) │ 1,525,936,270 │ 515,057,019 = │ −13.8% │
│ │ │ 1,315,203,370 │ │
└───────────────────────┴───────────────┴────────────────────────┴────────┘
(Both your numbers check out exactly against the logs. For the write
comparison I had to add the branch's vector-driven erases — the "Committed"
line only counts cursor entries, and the 515M compact-spent erases are logged
separately as "txdb BatchWrite spents N".)
All three metrics point the same way and tell a coherent story for the PR:
spent coins taking 36 bytes instead of 128 means the same -dbcache holds more
state, so the cache flushes 39% less often, and with longer flush cycles more
coins are both created and spent entirely in memory — never touching leveldb
in either direction. That shows up as ~105M fewer lookups and ~210M fewer
batch operations, which is where the 10.9% wall-clock win comes from.
One nuance worth including in the PR description: the −39% flush count
overstates the steady-state benefit slightly (flush frequency scales with
effective cache size, but each branch flush cycle also accumulates more work),
which is why the op-count and lookup reductions land near 14% rather than 39%
— those two are the honest per-work numbers, and they're the ones that should
scale to users' nodes.
✻ Churned for 52s
❯ oh one question i meant to ask ... this bloom filter you've added, does it
scale somehow with different dbcache configuration settings? here we're using
1000 (mib, i.e. about 1gb), but on a raspberry pi, dbache might be smaller,
or the user may configure it larger, is that okay with respect to the bloom
filter? or does it need some kind of dynamic sizing?
● Good question — the filter itself is fine: it's sized dynamically from the
content, not from any configuration. Every CompactSpents() rebuild calls
Reset(m_compact_spents.size()), which sizes the bit array at ~16 bits (2
bytes) per actual compacted spent, rounded up to a power-of-two number of
cache-line blocks. So it self-scales in both directions:
- Small dbcache (Pi): the cache fills and flushes sooner, so the spents vector
stays small, and the filter shrinks proportionally — down to as little as one
64-byte block. The false-positive rate is a property of bits-per-entry, so it
stays equally good at any size.
- Large dbcache: the vector grows to tens of millions of entries and the
filter grows with it, but always at ~2–4 bytes per entry versus the vector's
36 — roughly 6–11% overhead on top of the thing it's indexing. And since it's
included in DynamicMemoryUsage(), it participates in the flush-threshold
accounting rather than being invisible memory.
The worst case from the power-of-two rounding is a filter ~2× the
16-bits-per-entry target, which just means an even lower FP rate for a few
extra MB.
What does need dbcache-aware sizing is the thing we deferred:
initial_compact_spents, hardcoded to 1,000,000 in InitCache. Two issues on a
small-dbcache node:
1. Compaction may never trigger. 1M spent map entries is ~128 MB — larger than
a Pi-sized dbcache entirely, so m_spent_count never reaches the threshold
before a flush wipes everything, and the whole feature silently no-ops.
2. The standing reserve. ReallocCompactSpents() reserves
initial_compact_spents capacity after every flush — that's a permanent 36 MB
(1M × 36 bytes) regardless of dbcache, which is a big bite out of a 100 MiB
cache.
Both point at what your own LMR comment in InitCache already suggested: the
threshold should be derived from the cache size (something like a fixed
fraction of -dbcache, e.g. 1M per GB works out to roughly 3.5% of the cache
budget), set via a method after construction since InitCache knows the size at
that point. That's the "figure it out later" item — the bloom filter will
follow along automatically whatever you choose there.
✻ Worked for 23s
❯ oh that's great, i didn't realize the bloom would scale so nicely ... so i
think you already suggested it, maybe i should start a benchmark run now with
the bloom lookup commented out to see what its overhead is ... also i'm
wondering if maybe we can squeeze a few more percent improvement with a more
efficient binary search, without making the code too complicated. i would be
curious to see the bloom FP rate, maybe we could add some counters for that,
and print them out when we reset the bloom at flush time?
● Good plan all around — and the counters will actually answer your
binary-search question for us, so let me add those first and explain why.
Key insight: with the bloom in front, the binary search almost never runs.
During IBD on a valid chain, essentially every lookup that reaches
IsCompactSpent is for an outpoint not in the vector (nobody legitimately
references an already-spent coin, and BIP30-style HaveCoin checks probe
brand-new outpoints). So the search executes only on filter false positives
(~0.1% of probes) plus true hits (~zero). If the counters confirm that, a
fancier binary search (Eytzinger, prefix arrays) would optimize code that runs
1-in-1000 lookups — not worth the complexity. It only becomes interesting if
your no-bloom benchmark somehow wins.
Adding the counters plus a one-line toggle so your no-bloom benchmark doesn't
need any stash gymnastics:
● Update(src/coins.h)
⎿  Added 6 lines
564 // (rebuilt in CompactSpents(), cleared in ReallocCompactS
pents()).
565 mutable CompactSpentsFilter m_spents_filter;
566
567 + // LMR filter effectiveness counters (temporary instrument
+ation), logged and
568 + // reset each flush cycle in ReallocCompactSpents().
569 + mutable size_t m_filter_probes{0};
570 + mutable size_t m_filter_positives{0};
571 + mutable size_t m_filter_true_positives{0};
572 +
573 /* Cached dynamic memory usage for the inner Coin objects.
*/
574 mutable size_t cachedCoinsUsage{0};
575 /* Running count of dirty Coin cache entries. */
● Update(src/coins.h)
⎿  Added 13 lines, removed 4 lines
691 void ReallocateCache();
692
693 bool IsCompactSpent(const COutPoint& outpoint) const {
694 - // The filter cheaply resolves the common case (not a
-compacted spent,
695 - // including the empty-vector case); the binary search
- confirms the rest.
696 - if (!m_spents_filter.MayContain(outpoint)) return fals
-e;
697 - return std::binary_search(m_compact_spents.begin(), m_
-compact_spents.end(), outpoint);
694 + // LMR benchmarking toggle: set to false to bypass the
+ bloom filter and
695 + // measure the pure-binary-search configuration.
696 + constexpr bool use_filter{true};
697 + if (use_filter) {
698 + // The filter cheaply resolves the common case (no
+t a compacted spent,
699 + // including the empty-vector case); the binary se
+arch confirms the rest.
700 + ++m_filter_probes;
701 + if (!m_spents_filter.MayContain(outpoint)) return
+false;
702 + ++m_filter_positives;
703 + }
704 + const bool spent{std::binary_search(m_compact_spents.b
+egin(), m_compact_spents.end(), outpoint)};
705 + m_filter_true_positives += spent;
706 + return spent;
707 }
708
709 // LMR Compact (verb) the existing spent map coins
● Update(src/coins.cpp)
⎿  Added 9 lines
425
426 void CCoinsViewCache::ReallocCompactSpents()
427 {
428 + // LMR temporary instrumentation: report how the bloom fil
+ter did this flush
429 + // cycle. False positives are the (harmless) cases where t
+he filter said
430 + // "maybe" but the binary search said no.
431 + if (m_filter_probes > 0) {
432 + LogDebug(BCLog::COINDB, "CompactSpents filter: probes=
+%u positives=%u true=%u false-positive rate=%.5f%%\n",
433 + m_filter_probes, m_filter_positives, m_filter
+_true_positives,
434 + 100.0 * (m_filter_positives - m_filter_true_p
+ositives) / m_filter_probes);
435 + }
436 + m_filter_probes = m_filter_positives = m_filter_true_posit
+ives = 0;
437 m_spent_count = 0;
438 // LMR m_initial_compact_spents being nonzero enables this
whole thing. Yes, it
439 // probably could be a std::optional but I don't mind this
(maybe I'm too old school).
Ran 1 shell command (ctrl+o to expand)
❯ this is ok but you do realize that when we reindex mainnet, we will never get
a true positive -- none of the historically accepted blocks will contain a
double-spend attempt!
● Exactly — that's the point I led with: on a reindex, every lookup reaching the
vector is for an outpoint that isn't there, so the binary search post-filter
runs only on false positives. The true-positive counter is still worth having,
for three small reasons:
1. It proves the claim with data instead of assertion — the log should read
true=0, which is precisely the evidence that a faster binary search would
optimize dead code (as long as the filter stays).
2. One tiny exception exists: the pre-BIP34 duplicate-coinbase era. The BIP30
HaveCoin checks probe outpoints of previously existing, fully spent
transactions, and for the known historical txid duplicates those outpoints can
genuinely sit in the spents vector — so you might see a handful of true
positives across the whole chain, and it'd be fun to catch them.
3. At the tip in live operation it's no longer zero: mempool validation of
double-spend attempts from peers does look up already-spent outpoints, so if
you ever watch these counters on a synced node, true > 0 is expected and the
distinction keeps the FP-rate math honest.
Everything is built and tests pass. Current state of the instrumentation:
- IsCompactSpent (coins.h) counts probes / filter-positives / confirmed-hits,
and has a constexpr bool use_filter{true} right at the top — flip that to
false and rebuild (cmake --build build -t bitcoind) for your no-bloom
benchmark run; the compiler removes the filter probe entirely, so it's a clean
A/B against the identical binary-search-only code path.
- Each flush cycle logs one line: CompactSpents filter: probes=N positives=N
true=N false-positive rate=X%, then resets — during reindex, positives ≈ false
positives, and I'd expect the rate well under 0.1% given the ~26 bits/entry
the power-of-two rounding typically lands on.
My prediction for the two runs: the no-bloom time lands close to your earlier
pre-bloom benchmarks (the binary search was your measured overhead), and the
counters on the bloom run show the search executing a few hundred thousand
times instead of ~600M — at which point I'd argue the "more efficient binary
search" idea is settled: not worth the complexity. If the data surprises us,
the prefix-array approach is the one I'd reach for first.
✻ Worked for 6m 57s
※ recap: Improving IBD dbcache via compact spent-coin storage; bloom filter added gave 10.9% reindex speedup, FP counters and a use_filter toggle are built and tested. Next: run the no-bloom benchmark by flipping use_filter to false and rebuilding.
❯ ok all good points! ok i flipped use_filter to false, recompiled, and it's
running
● Sounds good. While it runs, a quick reference for what we'll compare when it
finishes (all at the same settings):
- master: 232m53.381s
- branch + bloom: 207m33.056s (10.9% faster than master)
- branch, binary-search only: this run — my expectation is it lands somewhere
between the two, and the gap between it and 207m33s is the bloom filter's
isolated contribution.
One note on interpreting this run's log: with use_filter=false the probes
counter stays zero, so the CompactSpents filter: lines won't appear — that's
expected. The Lookups and Committed/BatchWrite spents totals should come out
essentially identical to the bloom run (the filter changes per-lookup cost,
not cache behavior), which is itself a useful sanity check that the two runs
did the same work.
When it's done, drop the log somewhere like you did with
debuglog-07-25-ssd-bloom (or just tell me the time output) and I'll pull the
comparison together.
✻ Churned for 9s
❯ will do ... so I also have a raspi 4, purchased in 2021, it's a myNode (so
has a bunch of various apps on it), and i've been able to install the
toolchain and run benchmarks there (but not with any of these recent
versions). it has both a HDD and SSD datadir. so i could try all this there,
that would be interesting ... I think this patch would be most helpful on a
small system like that, do you agree?
● Yes, I agree — with one important caveat that affects how you should test it
there.
Why the Pi is the best case for this patch: the benefit scales with how
constrained the cache is relative to the workload, and with how expensive each
avoided leveldb operation is. A Pi has both amplifiers: a small dbcache
(myNode typically configures something modest out of its ~4GB) means flushes
happen constantly, and slow storage — the HDD especially — makes every avoided
lookup and every avoided flush write worth an order of magnitude more than on
your desktop SSD. Your 14% fewer lookups and 39% fewer flushes would
translate into much larger relative wall-clock savings when each random read
costs ~10ms instead of ~0.1ms. The HDD datadir run could be genuinely
dramatic.
The caveat: as the code stands, the feature may never activate on a Pi.
initial_compact_spents is hardcoded to 1,000,000 in InitCache, and compaction
only triggers once a million spent entries (~128 MB of map space) accumulate
between flushes. With a small dbcache, the cache fills and flushes long before
that, so CompactSpents never runs and you'd benchmark a no-op — worse, the
ReallocCompactSpents reserve permanently pins 36 MB of vector capacity, so a
small-dbcache run could plausibly come out slightly worse than master. That's
the deferred "scale with dbcache" item, and it graduates from cleanup to
prerequisite before the Pi runs mean anything.
Two ways to handle it:
1. Quick and manual: before building for the Pi, just edit the constant to
something proportional — your desktop data point is 1M per 1000 MiB of
dbcache, so ~3.5% of the cache budget; e.g. 100k for dbcache=100. Fine for a
first experiment.
2. Do the real fix now: replace the constructor constant with the deferred
setter (e.g. EnableCompactSpents(size_t)), and have the caller derive the
threshold from the actual dbcache size — Chainstate knows
m_coinstip_cache_size_bytes, so it can be set after InitCache or on cache
resize. That also handles myNode's habit of users adjusting dbcache in the UI,
and it's work the PR needs regardless.
Two smaller Pi-specific notes: the transient memory spike during CompactSpents
(old vector + merge target live simultaneously) deserves a glance at 4 GB
with other myNode apps running, though at small thresholds the absolute sizes
are small too; and a full reindex to 938k on a Pi 4 is a multi-day run — you
may want to benchmark to a lower height (say 500–600k) per configuration,
which still exercises the mechanism heavily.
Want me to implement the setter and dbcache-proportional sizing now, so the
branch is Pi-ready as-is? It's the same change the PR needs eventually anyway.
✻ Worked for 35s
❯ oh that's right, you already mentioned that, scaling initial_compact_spents,
that shouldn't be difficult at all, maybe i can ask you to do it?
● Happy to — one operational note first: since your benchmark is running on this
machine, I'll make all the source edits now but hold off on compiling and
testing until your run finishes, so a parallel build doesn't steal CPU from
the timing measurement.
While I'm at it, this is the natural moment to do the API properly: removing
the constructor parameter in favor of a setter also reverts all those
fuzz/test call sites to their upstream form and eliminates the positional-bool
hazard for good. Let me look at the validation.cpp side first:
Searched for 1 pattern, read 2 files (ctrl+o to expand)
● Update(src/coins.h)
⎿  Added 6 lines, removed 7 lines
550 // Store "compact" spent coins (outpoints only) until Flus
h or Sync; this list must
551 // be sorted.
552 mutable CompactSpentsList m_compact_spents;
553 - // How many spent coins there must be to start compacting
-them. Zero means never
554 - // compact spent coins. (Does not change.)
555 - // LMR don't think I need a {0} here or on the next one be
-cause these get initialized
556 - // in the constructor.
557 - size_t m_initial_compact_spents;
553 + // How many spent coins there must be to start compacting
+them. Zero (the
554 + // default) means never compact spent coins. Set via SetCo
+mpactSpentsThreshold().
555 + size_t m_initial_compact_spents{0};
556 // When the number of cacheCoins spents reaches this value
, then compact them.
559 - // (Increases over time.)
560 - size_t m_spents_limit;
557 + // (Increases over time within a flush cycle; reset to m_i
+nitial_compact_spents
558 + // at each flush.)
559 + size_t m_spents_limit{0};
560
561 // Bloom filter answering most (negative) m_compact_spents
lookups with a
562 // single cache-line probe; must always describe m_compact
_spents exactly
● Update(src/coins.h)
⎿  Added 9 lines, removed 1 line
588 virtual std::optional<Coin> FetchCoinFromBase(const COutPo
int& outpoint) const;
589
590 public:
591 - CCoinsViewCache(CCoinsView* in_base, size_t initial_compac
-t_spents = 0, bool deterministic = false);
591 + CCoinsViewCache(CCoinsView* in_base, bool deterministic =
+false);
592
593 + //! Enable (threshold > 0) or disable (0, the default) com
+paction of spent
594 + //! coins. The threshold is the number of spent map entrie
+s that triggers the
595 + //! first compaction of a flush cycle; it should be derive
+d from the cache
596 + //! size (see Chainstate::InitCoinsCache()). If compaction
+ has already run in
597 + //! the current flush cycle, a new threshold takes full ef
+fect after the next
598 + //! flush.
599 + void SetCompactSpentsThreshold(size_t threshold);
600 +
601 /**
602 * By deleting the copy constructor, we prevent accidental
ly using it when one intends to create a cache on top of a bas
e cache.
603 */
● Update(src/coins.cpp)
⎿  Added 1 line, removed 5 lines
55 return base->PeekCoin(outpoint);
56 }
57
58 -// LMR the extra argument, initial_compact_spents, determines h
-ow many spent coins (in the cacheCoins map)
59 -// are required before we compact all existing spent coins, so
-it should be somehow related to the size
60 -// of the cache. A value of 1m seems good for a cache size of 1
-000 (mib, usually the default I think)
61 -CCoinsViewCache::CCoinsViewCache(CCoinsView* in_base, size_t in
-itial_compact_spents, bool deterministic) :
58 +CCoinsViewCache::CCoinsViewCache(CCoinsView* in_base, bool dete
+rministic) :
59 CCoinsViewBacked(in_base), m_deterministic(deterministic),
60 cacheCoins(0, SaltedOutpointHasher(/*deterministic=*/determ
inistic), CCoinsMap::key_equal{}, &m_cache_coins_memory_resourc
e),
64 - m_initial_compact_spents(initial_compact_spents), m_spents_
-limit(initial_compact_spents),
61 m_spents_filter(deterministic)
62 {
63 // LMR instead of initializing two sentinels (spent and uns
pent) here, makes more sense to do this in
● Update(src/coins.cpp)
⎿  Added 18 lines, removed 8 lines
431 }
432 m_filter_probes = m_filter_positives = m_filter_true_posit
ives = 0;
433 m_spent_count = 0;
434 - // LMR m_initial_compact_spents being nonzero enables this
- whole thing. Yes, it
435 - // probably could be a std::optional but I don't mind this
- (maybe I'm too old school).
436 - if (m_initial_compact_spents > 0) {
437 - m_compact_spents.clear();
438 - m_compact_spents.shrink_to_fit();
439 - m_compact_spents.reserve(m_initial_compact_spents);
440 - m_spents_limit = m_initial_compact_spents;
441 - m_spents_filter.Reset(0);
434 + // Note: no reserve() on m_compact_spents here; CompactSpe
+nts() builds each
435 + // merged vector with an exact reserve and move-assigns it
+, so any capacity
436 + // reserved here would be held (unused) until the first co
+mpaction and then
437 + // discarded.
438 + m_compact_spents.clear();
439 + m_compact_spents.shrink_to_fit();
440 + m_spents_filter.Reset(0);
441 + m_spents_limit = m_initial_compact_spents;
442 +}
443 +
444 +void CCoinsViewCache::SetCompactSpentsThreshold(size_t thresho
+ld)
445 +{
446 + m_initial_compact_spents = threshold;
447 + if (m_compact_spents.empty()) {
448 + // No compaction has happened yet in this flush cycle,
+ so the new
449 + // threshold can take effect immediately. Otherwise m_
+spents_limit is
450 + // tracking the vector's growth and gets reset at the
+next flush.
451 + m_spents_limit = threshold;
452 }
453 }
454 void CCoinsViewCache::Flush(bool reallocate_cache)
● Update(src/validation.cpp)
⎿  Added 12 lines, removed 7 lines
1862 void CoinsViews::InitCache()
1863 {
1864 AssertLockHeld(::cs_main);
1865 - // LMR this is the only place the compact spents logic is
- enabled (it's instantiating the
1866 - // large cache); the default is zero (disabled).
1867 - // Experiments show that with dbcache=1000, we get about
-1.3m spents minimum (max 10.7m)
1868 - // this parameter can probably simply scale with the size
- of the cache. This code here doesn't
1869 - // know the size of the cache, so maybe we'll need to not
- make this a constructor argument,
1870 - // but instead have it settable via a new object method.
1871 - m_cacheview = std::make_unique<CCoinsViewCache>(&m_catche
-rview, /*initial_compact_spents=*/2 /*1'000'000*/);
1865 + m_cacheview = std::make_unique<CCoinsViewCache>(&m_catche
+rview);
1866 m_connect_block_view = std::make_unique<CoinsViewOverlay>
(&*m_cacheview);
1867 }
1868
1869 +//! Derive the spent-coin compaction threshold from the coins
+tip cache size:
1870 +//! begin compacting once spent map entries (~128 bytes each
+in the pool
1871 +//! allocator) would otherwise occupy roughly 1/8th of the ca
+che. For
1872 +//! dbcache=1000 (MiB) this is ~1M entries, which benchmarked
+ well; it scales
1873 +//! down proportionally for memory-constrained nodes so that
+compaction still
1874 +//! activates well before the cache fills.
1875 +static size_t CompactSpentsThreshold(size_t coinstip_cache_si
+ze_bytes)
1876 +{
1877 + return coinstip_cache_size_bytes / 8 / 128;
1878 +}
1879 +
1880 Chainstate::Chainstate(
1881 CTxMemPool* mempool,
1882 BlockManager& blockman,
● Update(src/validation.cpp)
⎿  Added 1 line
1951 assert(m_coins_views != nullptr);
1952 m_coinstip_cache_size_bytes = cache_size_bytes;
1953 m_coins_views->InitCache();
1954 + CoinsTip().SetCompactSpentsThreshold(CompactSpentsThresho
+ld(cache_size_bytes));
1955 }
1956
1957 // Lock-free: depends on `m_cached_is_ibd`, which is latched
by `UpdateIBDStatus()`.
● Update(src/validation.cpp)
⎿  Added 1 line
5498 size_t old_coinstip_size = m_coinstip_cache_size_bytes;
5499 m_coinstip_cache_size_bytes = coinstip_size;
5500 m_coinsdb_cache_size_bytes = coinsdb_size;
5501 + CoinsTip().SetCompactSpentsThreshold(CompactSpentsThresho
+ld(coinstip_size));
5502 CoinsDB().ResizeCache(coinsdb_size);
5503
5504 LogInfo("[%s] resized coinsdb cache to %.1f MiB",
● Now revert all the call sites to the upstream two-argument form, and switch
the tests to the setter:
Ran 1 shell command (ctrl+o to expand)
● Update(src/test/coins_tests.cpp)
⎿  Added 1 line, removed 1 line
84 class CCoinsViewCacheTest : public CCoinsViewCache
85 {
86 public:
87 - explicit CCoinsViewCacheTest(CCoinsView* _base) : CCoinsVie
-wCache(_base, /*initial_compact_spents=*/0) {}
87 + explicit CCoinsViewCacheTest(CCoinsView* _base) : CCoinsVie
+wCache(_base) {}
88
89 void SelfTest(bool sanity_check = true) const
90 {
● Update(src/test/coins_tests.cpp)
⎿  Added 2 lines, removed 1 line
1266 }
1267 // Compact aggressively: every spend immediately moves to
the vector, so
1268 // each iteration below performs a merge and a filter reb
uild.
1269 - CCoinsViewCache cache{&base, /*initial_compact_spents=*/1
-};
1269 + CCoinsViewCache cache{&base};
1270 + cache.SetCompactSpentsThreshold(1);
1271 for (const COutPoint& outpoint : outpoints) {
1272 BOOST_CHECK(cache.SpendCoin(outpoint));
1273 }
Ran 2 shell commands (ctrl+o to expand)
● Done — all edits are in and syntax-checked (no full builds until your
benchmark finishes). Summary:
API change: CCoinsViewCache's constructor is back to its upstream signature
(CCoinsView*, bool deterministic) — the positional-bool hazard is gone for
good, and all 7 fuzz call sites reverted to their original upstream form (nice
diff reduction). Compaction is now enabled via
SetCompactSpentsThreshold(size_t); 0 disables, and a mid-cycle change defers
gracefully if compaction already ran.
dbcache scaling: Chainstate::InitCoinsCache and ResizeCoinsCaches now both
call the setter with cache_size_bytes / 8 / 128 — i.e. compact once spent map
entries (~128 bytes each) would occupy ~1/8th of the cache. That reproduces
your benchmarked 1M at dbcache=1000, gives ~460k at the 450 MiB default, and
~102k on a Pi-ish dbcache=100 — so the feature activates properly on small
systems, and it tracks runtime dbcache resizes too.
Bonus cleanup: I removed the standing reserve(initial) in ReallocCompactSpents
— nothing ever appends to m_compact_spents directly (CompactSpents builds
each merged vector with an exact reserve and move-assigns it), so that
reserved buffer sat unused until the first compaction and was then discarded.
On a Pi that was 36 MB of pure waste; now it's gone everywhere.
Your test with SetCompactSpentsThreshold(1) replaces the old {&base, 1}
construction, so the aggressive-compaction coverage is preserved.
And repeating the flag from before since it affects the run in progress:
InitCache had /*initial_compact_spents=*/2 /*1'000'000*/ before my edit. If
your currently-running no-bloom binary was compiled with 2, it's compacting
after every 2 spends — tiny merges constantly — and won't be comparable to the
bloom run's 207m33s (nor will its absolute time mean much). Worth confirming
which value it built with; if it's 2, I'd kill it and rebuild from the current
source once you flip use_filter to whichever configuration you want to
measure first.
✻ Churned for 3m 58s
❯ oh yes, i forgot about setting that to 2 ... i did that just so i could step
through your bloom changes in the debugger and hit the new code right away
... and i forgot to restore it... so i'll restart the benchmark run ... i'm
glad you noticed that memory spike, i noticed it too ... it does momentarily
violate our dbcache memory budget, but (well, i may have said all this in my
LMR comments), this already happens with the cacheCoins unordered map, when
it grows its bucket array. also, i do think it's actually ok for there to be
a memory spike, because even if that pushes some stuff out to swap, for
example, it only happens on the order of several minutes, so it's in a
different category than using that larger (spike) size *continuously*
● That was a good debugging trick — just unlucky that it lingered. And yes, your
restarted run will now use the scaled threshold: dbcache=1000 works out to
1,024,000 instead of the 1,000,000 the bloom run used. That difference is
noise (after the first compaction, the limit sequence is driven by the
20%-growth rule off the vector size anyway), but worth a line in your notes
for rigor.
On the memory spike, I think your categorization is right, and it's worth
writing down precisely for the PR, because a reviewer will ask. The honest
quantification:
- Shape of the spike: during the merge, the old vector, new_spents, and the
merged target coexist, so the peak is roughly double the final vector size,
for the duration of one memcpy-speed pass. At your observed 10.7M-entry worst
case that's ~385 MB extra on top of a 1000 MiB budget for well under a second
— not "several minutes"; the spike itself is brief even if the allocator holds
the pages a bit longer.
- The precedent is real but smaller: the cacheCoins bucket-array rehash also
violates the budget transiently (old + doubled-new bucket arrays coexist), but
at 8 bytes per bucket that's an order of magnitude less relative overshoot.
So the precedent supports the principle (transient ≠ steady-state), while this
patch does stretch its magnitude — better to own that in the PR description
than have a reviewer discover it.
- Two things now bound it nicely: first, the dbcache scaling we just added
makes the spike proportional to the budget everywhere — on a Pi at dbcache=100
the whole vector is ~4 MB, so the spike is trivial exactly where memory is
scarcest. Second, DynamicMemoryUsage() counting the vector and filter means
the steady-state accounting is honest; only the merge instant escapes it.
And your continuous-vs-momentary argument has a nice flip side worth stating:
the patch's entire benefit is that it reduces continuous memory per spent coin
by 3.5×, which is what buys the 39% fewer flushes. Trading a brief 2× spike
on a shrinking component for a large permanent reduction is a good deal by any
accounting.
If a reviewer still objects, there's a known escape hatch to mention (not
implement now): an in-place backward merge — resize the existing vector to the
combined size and merge from the ends — which shrinks the duration of the
double-buffer condition to just the realloc copy, though its peak is similar.
I'd keep the current simple std::merge and defend it with the numbers above.
Good luck with the restarted run — when it's done, send the time and drop the
log like last time, and I'll put together the three-way comparison (master /
binary-search-only / bloom).
✻ Cooked for 51s
❯ really good catch on your part about that initial unnecessary reserve. one
thing i think you did correctly is, we definitely don't want the small
per-block instance of the cache object (the one that batch writes to the main
cache after each block) to use any of this new stuff, no reason for it at
all since the size is so small ... i think you preserved that (i.e. it's only
used for the big main cache)
● Yes, that's preserved — and verified: the only calls to
SetCompactSpentsThreshold with a nonzero value are on CoinsTip() (the big main
cache) in InitCoinsCache/ResizeCoinsCaches. The per-block CoinsViewOverlay
(m_connect_block_view), mempool validation views, and every other ephemeral
cache construct with the default of 0, so they never compact, and their
IsCompactSpent short-circuits on the empty filter (m_words.empty() → false)
without ever touching a binary search.
But your question just made me trace the per-block overlay's path through the
new code, and I spotted one thing worth fixing before your next bloom-enabled
run: the overlay does increment the probe counter on every cache miss
(harmless, one integer add), and its per-block Sync() runs
ReallocCompactSpents — which would emit the CompactSpents filter: log line
once per block with -debug=coindb on. Over a reindex that's ~938k junk log
lines from a cache that doesn't even use the feature. Guarding the log on
compaction being enabled:
● Update(src/coins.cpp)
⎿  Added 4 lines, removed 2 lines
423 {
424 // LMR temporary instrumentation: report how the bloom fil
ter did this flush
425 // cycle. False positives are the (harmless) cases where t
he filter said
426 - // "maybe" but the binary search said no.
427 - if (m_filter_probes > 0) {
426 + // "maybe" but the binary search said no. Only log for cac
+hes with compaction
427 + // enabled; the per-block overlay and other ephemeral cach
+es also count
428 + // probes (all trivially negative) and would otherwise log
+ constantly.
429 + if (m_initial_compact_spents > 0 && m_filter_probes > 0) {
430 LogDebug(BCLog::COINDB, "CompactSpents filter: probes=
%u positives=%u true=%u false-positive rate=%.5f%%\n",
431 m_filter_probes, m_filter_positives, m_filter
_true_positives,
432 100.0 * (m_filter_positives - m_filter_true_p
ositives) / m_filter_probes);
Ran 1 shell command (ctrl+o to expand)
● Fixed and syntax-checked (your current run is unaffected either way — with
use_filter=false the probe counter never increments, so the log line never
fired).
So to restate the full answer to your question: the separation is clean by
construction now. Compaction is opt-in via the setter, only Chainstate ever
opts in, and it does so only for CoinsTip(). The overlay pays exactly two
costs from the new code: an empty-vector check per IsCompactSpent call (one
branch) and the probe-counter increment (which disappears anyway when you
strip the temporary instrumentation before the PR). Everything heavier —
vector, filter memory, merges, rebuilds, logging — is confined to the main
cache.
✻ Sautéed for 1m 2s
❯ good catch, i like that change you just made. super small thing, is it worth
guarding the call to std::binary search with a check if
m_compact_spends.size() > 0 like i had it before? it's probably trivial now
because of the bloom filter (hopefully we hardly ever do a binary search),
but just wondering how much overhead there is in started a binary search on
an empty vector (i never measured that)
● Not worth it — the guard would buy literally nothing, for two reasons:
1. With the filter on, that code path is unreachable when the vector is empty.
The filter is rebuilt/cleared in lockstep with the vector, so empty vector ⇒
empty filter ⇒ MayContain returns false at its m_words.empty() check ⇒ the
binary search never runs. A size guard there would be dead code.
2. An empty-range std::binary_search is the size check. binary_search calls
lower_bound, whose loop condition is count > 0 — with an empty range it falls
straight through and returns, and then the it != last check fails. At -O2 the
whole thing inlines to a compare of the vector's begin/end pointers, which is
the exact same work as size() > 0 (that's also just end - begin != 0, reading
the same two pointers from the same cache line). You'd be adding a check to
skip an operation that costs the same as the check.
The one configuration where empty-vector searches actually occur is
use_filter=false on compaction-disabled caches (the overlay does one per cache
miss) — and there it's those same few inlined instructions, single-digit
nanoseconds with no memory traffic. Your original guard made sense as
insurance when the search was unconditional and unmeasured, but now it can
stay gone.
✻ Crunched for 19s
❯ ok perfect ... ok i just ran cmake again, and started the benchmark (with
use_filter set to false), i'll report back later
● Sounds good. While it runs, let me update the project memory so the state is
captured:
Wrote 1 memory (ctrl+o to expand)
● Noted and saved. When you report back we'll have the clean three-way
comparison — master 232m53, no-bloom (this run), bloom 207m33 — plus the
Lookups/flush-count cross-check between the two branch runs, which should come
out nearly identical since the filter only changes per-lookup cost.
One reminder for after the run: the setter refactor has only been
syntax-checked so far — before you build anything for the Pi, give me a shout
and I'll run the full unit suite and the four fuzz targets against it (a few
minutes of machine time once the benchmark is done).
✻ Sautéed for 21s
※ recap: Optimizing your compact-spents dbcache patch: bloom filter added (10.9% faster reindex vs master), API now scales with dbcache. Your no-bloom benchmark is running; when it finishes, report the time and I'll run full tests on the setter refactor.
❯ side question, i know SSDs have a limited lifetime in terms of writes
(compared with HDD), so is doing all these reindex-chainstate benchmarks
wearing out my SSD prematurely? or is that so minor it's not worth worrying
about?
● Short answer: it's real but minor — worth a quick measurement, not worth
worrying about.
Rough numbers: a -reindex-chainstate run doesn't touch the block files (no
re-download, no block writes); the writes are the chainstate leveldb — your
master log showed ~1.5B coin entries written across all flushes, which at ~50
bytes serialized is ~75 GB of logical batch data, and leveldb's compaction
write-amplification (typically several ×, sometimes 10×+ for random-key churn
like txids) plausibly turns that into a few hundred GB of actual device writes
per full run.
Against SSD endurance ratings: a typical modern 1–2 TB consumer TLC drive is
rated for 600–1200 TBW (terabytes written). So one full reindex is on the
order of 0.05% of the drive's rated life. You could run these benchmarks
weekly for years before it added up to a few percent. The rating itself is
also conservative — drives usually far outlast it.
You can replace my estimate with the exact number in ten seconds:
sudo smartctl -a /dev/nvme0 # or /dev/sda for SATA
Look at "Data Units Written" (NVMe; multiply by 512,000 bytes) or
Total_LBAs_Written (SATA). Note it before and after one benchmark run and
you'll know your true per-run cost — I'd genuinely be curious what the write
amplification comes out to.
Two small related points:
- Also check "Percentage Used" (NVMe) or the wear-leveling attribute — that's
the drive's own opinion of consumed endurance, which settles the question
regardless of arithmetic.
- Pleasant irony: your patch is itself an SSD-longevity feature — 13.8% fewer
leveldb operations and 39% fewer flush cycles means every node running it
writes meaningfully less. That's a legitimate line for the PR description,
since plenty of node operators run on exactly the kind of small SSDs where
this matters.
The Pi's HDD datadir has no comparable wear mechanism, so benchmark there
freely (patience permitting).
✻ Brewed for 29s
❯ ok i just ran it, and we just started the benchmark a few minutes ago so
close enough ... Data Units Written: 93,406,356 [47.8 TB] Percentage Used: 3%
..... oh wow, that's a great observation about this patch helping there!
i'll be sure to include that (i'm sure i'll ask for your help with the PR)
● Your drive is in great shape — 47.8 TB written and only 3% consumed implies
the controller expects on the order of 1.5 PB of endurance, so at your current
pace this SSD will outlive several computers. Benchmark guilt-free.
And you've accidentally set up the perfect experiment: since you ran it
minutes after starting the benchmark, that 93,406,356 reading is an
almost-exact baseline for this run. Run smartctl again right when the reindex
finishes and the delta gives us the true device-level bytes-per-reindex —
including leveldb's write amplification, which the logical batch totals can't
show. Do the same around the next bloom-enabled run and we can even quote the
patch's write savings in real device terms rather than my estimate. Let me
record the baseline:
Wrote 1 memory (ctrl+o to expand)
● Saved. And yes, happy to help with the PR when the time comes — between the
three-way benchmark, the lookup/flush/write-op counts, and now potentially
real SMART deltas, you'll have an unusually well-evidenced performance PR.
Talk to you when the run finishes.
✻ Sautéed for 22s
❯ there's one other big thing to do however ... rebase! this just dropped, and
i haven't rebased on it yet ... i didn't yet because i was curious on how
this would work without it, but of course we'll have to rebase before the PR
goes up, and re-benchmark before the PR. intuitively, i don't think it will
make too much of a difference in terms of how much of an improvement this
patch gives, but who knows?
● Let me see what actually landed upstream so we can reason about the
interaction — checking the remotes:
Ran 1 shell command (ctrl+o to expand)
❯ oh i didn't give you the link: https://github.com/bitcoin/bitcoin/pull/35295
Ran 2 shell commands (ctrl+o to expand)
● Now I have the full picture. Your base is master as of June 27 (7a74f65293),
110 merge-commits behind — so #35295 (merged July 9) is indeed not in it.
Notably, the CoinsViewOverlay/PeekCoin infrastructure your branch builds on
came from that effort's preparatory PRs, which you do have — #35295 adds the
worker-thread prefetching on top of that same overlay. That's good news for
rebase mechanics: you're building on the same foundation, not a divergent one.
On your intuition about the performance interaction — I'd temper it a bit.
Three separate effects:
1. Partial overlap on reads (your number likely shrinks on SSD). Both changes
attack the same cost — leveldb read latency on cache misses — by different
means: #35295 hides it (parallel prefetch), your patch removes it (fewer
misses via bigger effective cache). Optimizations aimed at the same stall
don't compose fully. Once misses are prefetched in parallel ahead of
validation, each miss your patch avoids saves less wall-clock than it did
serially. So expect the read-side portion of your 10.9% to shrink on a fast
SSD.
2. The write side is fully additive. #35295 does nothing about writes: your
39% fewer flushes, ~14% fewer batch operations, and the SSD-wear reduction
survive the rebase untouched. That part of the story is rebase-proof.
3. The hardware story inverts nicely. Parallel prefetch helps most where the
device has spare IOPS (NVMe) and least on a single HDD spindle, where 8
parallel seeks are still seeks. Your patch is the mirror image — it shines
exactly where #35295 can't: HDD, Pi, small dbcache. "Complementary to #35295
across hardware classes" is a genuinely good framing for the PR, and your
planned Pi/HDD benchmarks become more important, not less.
One technical flag for the rebase, and it's the big one: threading. #35295's
fetcher threads read through the coins hierarchy concurrently. Your
IsCompactSpent is read-only between flushes (vector and filter are stable),
but two things need auditing against however #35295 synchronizes cache access:
(a) FetchCoin's temp-entry insert/erase and TryCompactSpents's map mutations
must not run concurrently with fetcher reads of the same cache, and (b) the
instrumentation counters (m_filter_probes etc., and your m_lookups in txdb)
become data races if probes happen on worker threads — they're slated for
removal anyway, but they must go before the rebased benchmark, not after. I'll
audit how the fetchers interact with the main cache when we do the rebase.
Logistics: your work is currently staged+unstaged but uncommitted — the rebase
needs it committed on the branch first. My suggestion for sequencing: let the
current no-bloom benchmark finish (it's still a valid data point for the
pre-rebase comparison and the filter-contribution question), then I'll commit
the work sensibly, run the full test suite, and do the rebase — expect
conflicts concentrated in coins.{h,cpp}, validation.cpp, and the fuzz files,
all in code we now know well. Then re-benchmark against the new base.
✻ Worked for 1m 48s
※ recap: Optimizing your compact-spents dbcache branch; the no-bloom benchmark is running now to isolate the bloom filter's contribution. When it finishes, report the time and I'll run tests on the setter refactor, then rebase onto master with PR 35295.
❯ benchmark result: 215m14.842s
● That completes the three-way comparison — and it cleanly answers the
binary-search question:
┌────────────────────────────┬─────────────┬───────────┐
│ Configuration │ real time │ vs master │
├────────────────────────────┼─────────────┼───────────┤
│ master │ 232m53.381s │ — │
├────────────────────────────┼─────────────┼───────────┤
│ branch, binary search only │ 215m14.842s │ −7.6% │
├────────────────────────────┼─────────────┼───────────┤
│ branch + bloom filter │ 207m33.056s │ −10.9% │
└────────────────────────────┴─────────────┴───────────┘
So the split is: compact storage alone buys 7.6%, and the bloom filter adds
another 3.3 points (7m42s saved relative to the no-bloom run, a 3.6% speedup
on top of it). That's a very healthy return on ~50 lines of filter code, and
it confirms your original instinct that the binary-search overhead was
significant — roughly a third of the patch's total benefit was being given
back to those searches.
It also settles the "fancier binary search" question definitively: the
theoretical ceiling for a better search is the same 461 seconds the filter
already recovers, and the filter gets essentially all of it (one cache-line
probe is within noise of not searching at all, per your earlier
commented-out-entirely experiment). Eytzinger/prefix-array would be chasing
the sliver the filter's <0.1% false positives leave behind — not worth any
complexity.
Two follow-ups while it's fresh:
1. Grab the SMART reading now (sudo smartctl -a → Data Units Written) before
other work muddies it — baseline at run start was 93,406,356 units (47.8 TB),
so the delta is your true device-writes-per-reindex.
2. If you drop this run's debug.log next to the other one, I'll cross-check
Lookups and flush counts against the bloom run — they should match closely
(the filter doesn't change cache behavior), which doubles as validation that
the two runs did identical work.
Updating the record:
Wrote 1 memory (ctrl+o to expand)
● Saved. Whenever you're ready for the next phase — committing the work and
doing the #35295 rebase — say the word. The machine's free now, so I can also
run the full verification pass on the setter refactor first (unit suite + fuzz
targets), which I'd want green before we start moving commits around anyway.
✻ Sautéed for 42s
※ recap: We're preparing your compact-spents dbcache patch for a PR; benchmarks are done (10.9% faster total, bloom filter contributing 3.3 points). Next: run the full test suite on the setter refactor, then commit and rebase onto current master.
❯ smartctl rerun: Data Units Written: 94,693,469 [48.4 TB] Percentage Used: 3%
● The delta is 1,287,113 data units × 512,000 bytes = ~659 GB of device writes
for one reindex (matches the 47.8 → 48.4 TB display).
What that tells us:
- Endurance: at ~1.5 PB implied endurance, one reindex costs ~0.04% of drive
life — you could run this benchmark roughly two thousand more times before it
mattered. Definitively not worth worrying about.
- Write amplification: the run's logical batch data was on the order of 60–70
GB (1.3B batch ops at ~50 bytes each), so the device saw roughly 10×
amplification — leveldb compaction churn plus filesystem overhead. That's
within the normal range for random-key leveldb workloads, and it's worth
knowing because it means every batch operation your patch avoids saves ~10×
its logical size in real flash writes.
- For the PR: if you take one more smartctl pair around a master run someday,
you'd have the device-level comparison directly. Extrapolating from the −13.8%
batch ops, the patch plausibly saves on the order of ~100 GB of flash writes
per full IBD/reindex — a concrete, user-facing number for the SSD-longevity
argument.
Wrote 1 memory (ctrl+o to expand)
● Recorded. That's a tidy little side-study — and it means your benchmark habit
costs the drive about as much as a week of normal node operation. Ready to
move on to committing and the #35295 rebase whenever you are.
✻ Churned for 25s
❯ i forgot to mention to you the CPU time measurements... i've been measuring
with the builtin bash time command, and the master run used 271m18.026s
22m59.7 for user and system time respectively. the new code run yesterday
with bloom enabled: 234m48.210s 19m53.349s
● Those CPU numbers add a genuinely important dimension. Totals:
┌────────────────┬─────────┬───────────┬──────────┬───────────┐
│ │ real │ user │ sys │ total CPU │
├────────────────┼─────────┼───────────┼──────────┼───────────┤
│ master │ 232m53s │ 271m18.0s │ 22m59.7s │ 294m17.7s │
├────────────────┼─────────┼───────────┼──────────┼───────────┤
│ branch + bloom │ 207m33s │ 234m48.2s │ 19m53.3s │ 254m41.6s │
├────────────────┼─────────┼───────────┼──────────┼───────────┤
│ change │ −10.9% │ −13.4% │ −13.5% │ −13.5% │
└────────────────┴─────────┴───────────┴──────────┴───────────┘
Two observations worth keeping:
1. The CPU savings (13.5%) exceed the wall-clock savings (10.9%) — and track
your −13.8% batch operations and −14.7% lookups almost exactly. That's strong
evidence the patch removes work rather than merely overlapping waits: less
serialization and CRC in batch-building, less leveldb memtable/compaction
churn, fewer read syscalls (the sys-time drop), and fewer memory-stall cycles
(stalls bill as user time, which is where the bloom's cache-line probe vs.
binary-search-miss difference shows up).
2. This reframes the #35295 rebase question in your favor. Parallel prefetch
hides latency but doesn't remove work — if anything it adds CPU (worker
threads doing the same reads). Your patch's CPU reduction should therefore
largely survive the rebase even where the wall-clock delta shrinks, and on
CPU-constrained hardware — the Pi again — removing work beats hiding it.
"−13.5% total CPU per reindex" is arguably a more durable headline than the
wall-clock number, and it compounds with the SSD-wear and energy angles for
the PR.
One small ask: if the terminal with today's no-bloom run is still open, grab
its user/sys too — that would split the 39m36s of saved CPU between the
storage-work reduction and the bloom's stall elimination, completing the
attribution story.
Wrote 1 memory (ctrl+o to expand)
● Recorded. The evidence file for this PR keeps getting better — wall-clock,
CPU, lookups, flushes, batch ops, and flash wear all pointing the same
direction with clean attribution between the two mechanisms.
✻ Cogitated for 33s
❯ for completeness (not sure it's important), the run i just did with bloom
disabled: 240m43.933s 19m28.891s
● It is important — it completes the attribution cleanly. Full picture (total
CPU = user + sys):
┌──────────────────┬──────────────────┬──────────────────┐
│ │ real │ total CPU │
├──────────────────┼──────────────────┼──────────────────┤
│ master │ 232m53s (13973s) │ 294m18s (17658s) │
├──────────────────┼──────────────────┼──────────────────┤
│ branch, no bloom │ 215m15s (12915s) │ 260m13s (15613s) │
├──────────────────┼──────────────────┼──────────────────┤
│ branch + bloom │ 207m33s (12453s) │ 254m42s (15282s) │
└──────────────────┴──────────────────┴──────────────────┘
Attribution of savings vs. master:
- Compact storage: 1059s real, 2045s CPU — the storage change removes about
twice as much CPU as it saves wall-clock, the signature of work removed from a
partially-overlapped pipeline (leveldb batch/compaction work happens partly
off the critical path).
- Bloom filter: a further 462s real, 331s CPU (almost all user time — exactly
what you'd expect from eliminating memory-stall cycles in the binary search,
which bill as user CPU). The ~130s gap between its real and CPU savings is
within single-run noise (~1%).
So the story is fully coherent: storage compaction removes leveldb work (11.6%
of master's CPU), the filter removes search stalls (another 1.9 points),
totalling the 13.5%. Nothing unexplained.
Wrote 1 memory (ctrl+o to expand)
● Saved. The benchmark story is now complete and internally consistent across
every axis we've measured. Next milestone when you're ready: verification pass
on the setter refactor, commit the work, then the #35295 rebase.
✻ Worked for 26s
❯ what do you mean let's do verificatoin pass? i was thinking we should enable
bloom and run again, to see the bloom FP rate statistics
● By "verification pass" I just meant: the setter refactor (constructor→setter,
dbcache scaling, reserve removal, log guard) has only been syntax-checked so
far — I deliberately didn't compile or run any tests while your benchmarks
owned the machine. Before we trust it (and before any commit/rebase), it needs
one full build + unit suite + fuzz run. A few minutes, and it has to happen
anyway before your next benchmark since the same code is involved.
Your plan composes perfectly with it: I'll flip use_filter back to true, build
everything, run the tests, and hand you a ready-to-launch binary. Two notes
on the FP-stats run:
- You don't need a full 3.5-hour run for FP statistics — a CompactSpents
filter: line prints at every flush, so you'll have real numbers within the
first hour. But a full run is still worth it as a bonus: it doubles as a
repeat bloom timing to gauge run-to-run variance (with the tiny caveat that
this build differs slightly — counter increments per probe and the 1.024M
threshold — all noise-level).
Doing it now:
● Update(src/coins.h)
⎿  Added 1 line, removed 1 line
700 bool IsCompactSpent(const COutPoint& outpoint) const {
701 // LMR benchmarking toggle: set to false to bypass the
bloom filter and
702 // measure the pure-binary-search configuration.
703 - constexpr bool use_filter{false};
703 + constexpr bool use_filter{true};
704 if (use_filter) {
705 // The filter cheaply resolves the common case (no
t a compacted spent,
706 // including the empty-vector case); the binary se
arch confirms the rest.
Ran 2 shell commands (ctrl+o to expand)
❯ in case you're interested, i moved the debug.log file from the most recent
benchmark run (with the bloom filter disabled) to local file
debuglog-07-25-ssd-bloom-disabled
● Noted — I'll cross-check that log now. Unit suite is green except the known
validation_flush_tests (CLOSE experiment). Continuing verification and the log
comparison in parallel:
Ran 4 shell commands (ctrl+o to expand)
● Background command "Rebuild fuzz binary with setter refactor" completed (exit
code 0)
● Verification pass complete — everything is green:
- Build: bitcoind and test_bitcoin compile cleanly with the setter refactor
and use_filter{true}.
- Unit suite: passes except the known pre-existing validation_flush_tests
(your CLOSE experiment).
- feature_block.py: passes.
- All four coins fuzz targets: pass over the corpus.
So the setter refactor is now fully verified, and build/bin/bitcoind is ready
for your bloom-enabled FP-stats run — launch whenever you like. What to watch:
each flush will emit one CompactSpents filter: probes=… positives=… true=…
false-positive rate=… line under -debug=coindb. My predictions, so we can
check calibration: FP rate well under 0.1% (power-of-two rounding typically
lands ~26 bits/entry), true=0 throughout — with the possible fun exception of
a tiny nonzero blip in the flush cycles covering the pre-BIP34
duplicate-coinbase blocks (~heights 91k), if those BIP30 checks catch a
compacted spent.
And to restate the log cross-check from above since it's the last piece of the
attribution: your two branch runs did statistically identical cache work —
Lookups 610.3M vs 609.2M (0.17% apart), flushes 112 vs 112, total batch ops
within 1% — so the 7m42s wall / 5m31s CPU difference between them is cleanly
attributable to the filter alone. The benchmark methodology holds up all the
way down.
✻ Sautéed for 8m 31s
❯ one thing you might notice in that debug log file i just copied to the
current directory is that the time to run CompactSpents is surprisingly (to
me, anyway) small. To compact almost 1m coins, with almost 5m existing (see
timestamp 2026-07-26T09:56:18.192619Z) takes well below one second real time.
the 20% growth is just a number i pulled out of thin air, seems reasonable,
but there could be a better choice?
● Let me look at the actual timing in the log around that spot first:
Ran 1 shell command (ctrl+o to expand)
❯ (i mentioned the 20% growth there because if it was 10%, we'd call
CompactSpents more often, but we'd compact more spent coins)
Searched for 1 pattern (ctrl+o to expand)
● I pulled the actual brackets from the log — your observation is confirmed and
quantifiable:
- 940481 + 4702408 at 09:56:18.19: next log line lands 0.74s later, and that
gap includes connecting a whole block, so the compaction itself (sort 940k +
merge 5.6M + rebuild filter) took maybe 0.3–0.5s.
- The biggest one in the run, 1625151 + 8125759 (9.75M total), fits inside a
1.4s bracket including a block connect.
That's ~70–130ns per element, which makes sense once you see what compaction
actually is: pure sequential streaming — sort a small batch, then a linear
merge writing 36-byte elements at near-memcpy speed, plus one cache-line touch
per filter insert. It's the polar opposite of the pointer-chasing workload
the rest of the cache does. Streaming 350MB of vectors is just... fast.
On the 20% question — one correction to the parenthetical first: with 10%
you'd compact more often in smaller batches, and the total work per flush
cycle goes up, not down. The math: with growth factor g, each element gets
copied about (1+g)/g times before the flush — g=0.2 → 6 copies, g=0.1 → 11,
g=0.5 → 3. Your thin-air choice sits in a nicely flat region, and more
importantly the family is right: a fixed batch size (say "always compact at
1M") would make total work quadratic in the vector size; proportional growth
keeps it linear. Good instinct.
But the log shows where a genuinely better choice hides, and it's not g
itself. Right after that 8.1M compaction: Cache size (1291798400) exceeds
total space. Late in a cycle, limit = 0.2 × 8.1M tolerates 1.6M uncompacted
spents sitting in the map at 128 bytes each — ~208MB of exactly the bloat this
patch exists to eliminate, and it's counted in DynamicMemoryUsage, so it
drags flushes earlier. Meanwhile your cache-scaled initial threshold (1M ≈
128MB) already encodes "how much map-spent bloat we tolerate." So the natural
refinement is a one-liner:
m_spents_limit = std::min(m_compact_spents.size() * 20 / 100,
m_initial_compact_spents);
Early cycle: unchanged geometric behavior. Late cycle: the tolerance stops
growing past the level you already decided was acceptable, reclaiming
~100–200MB of effective cache exactly when the cache is fullest — at the cost
of a few extra sub-second merges per cycle (roughly doubling compaction CPU in
the largest cycles, still single-digit seconds against the minutes each flush
cycle takes). Given that the patch's entire 7.6% storage win came from
effective cache size, this plausibly buys another measurable slice, and it's
trivially benchmarkable.
I'd hold off applying it until your FP-stats run finishes so we don't fork the
configuration mid-experiment — want me to make the change after that, so it
rides along in the rebase and you can A/B it later?
✻ Churned for 2m 5s
❯ ok i love that, but yes, let's wait. we do want to eliminate those "exceeds
total space" events (also you can see critical=1 in the log), reviewers will
not like that! in the master run, there were zero of those events. i think
maybe here or there could be acceptable, but the fewer the better, and i
think your suggested change will help
● Let me quantify the problem across the three runs so we know the target:
Ran 1 shell command (ctrl+o to expand)
● The counts sharpen the picture: 8 critical flushes in the bloom run, 12 in the
no-bloom run, 0 on master — so roughly 1 in 10–14 of the branch's flushes
tripped CRITICAL. Reviewers will indeed zero in on that, and the log pair you
spotted shows the anatomy: even right after that 1.6M-entry compaction freed
~200MB of map space, the cache still read 1232MiB against a 1176MiB budget —
meaning the late-cycle spent bloat had already carried usage well past the
LARGE threshold's headroom before the flush machinery caught up.
Master never hits CRITICAL because its per-block growth is smooth and the
90%→100% band leaves plenty of blocks for the flush to land. The branch's
late-cycle behavior is lumpier — up to 0.2 × S map-resident spents
accumulating between compactions — which is exactly what the cap eliminates.
Two-step plan when the FP run finishes:
1. The min(20% × size, m_initial_compact_spents) cap — bounds the lumpiness to
the same 128MB the initial threshold already budgets, which alone may get the
count to zero or near it.
2. If any CRITICALs survive, the principled second lever is the one you
already prototyped: your CLOSE/NearFull() experiment, reborn as "force a
compaction when the cache state reaches LARGE" — it's sub-second, frees a
couple hundred MB on demand, and gives the flush logic its headroom back. If
the cap alone suffices, we delete the CLOSE machinery entirely instead — which
also un-breaks validation_flush_tests for free. Either way that experimental
code stops being dead weight: it becomes either the fix or deliberately
removed.
Both are tiny diffs and easy to A/B with the flush-count/critical-count greps
we've already got. I'll queue them for after your run, along with the commit +
rebase sequencing. Enjoy watching the FP lines roll in — curious whether my
<0.1% prediction holds.
✻ Worked for 1m 11s
❯ i think it could happen in master because a bucket array realloc can happen
at any time. but the bucket elements are only 8 bytes each, so it's not as
much space as when our spent vector resizes. i'm not sure you're correct
about the 90%-100% gap, because with non-trivially-small dbcache, it's
actually the total available cache space minus 10 MiB (or 10 MB). I think the
code author was thinking about how, while validating a single block, you're
not likely to have more than 10 MB of growth in the coins cache (the space is
checked after each block validation). but I think the author likely forgot
about the bucket array. so it can still happen on master, but much less
likely. i think the gap should not be just a fixed 10 MB like it is now (for
significant dbcache size), but should be more like 98% of the available
cache, something like that, to avoid getting these exceeds events completely.
maybe we can make that change too
● Let me ground this in the actual code first:
Searched for 1 pattern, ran 1 shell command (ctrl+o to expand)
● You're right, and I was wrong about the 90% band: LargeCoinsCacheThreshold =
max(90% × total, total − 10MiB), so for any dbcache above ~100MiB the LARGE
warning fires just 10MiB below the ceiling. That 10MiB was calibrated exactly
as you say — to the organic coins growth of validating one block — and it
silently assumes memory usage moves smoothly. Your bucket-array observation
explains master's residual risk perfectly: a late-cycle rehash steps the
accounting by far more than 10MiB in one block, but doubling events are
exponentially spaced, so one landing inside that tiny window is rare enough
that your master run saw zero.
The branch breaks that assumption routinely, which is why 8–12 flushes went
CRITICAL: every compaction is a step of 36B × batch in a single block — that
1.6M-entry merge was a +58MiB jump into a 10MiB-headroom zone. No timing luck
can save you from that; it's structural.
The nice part is that the two planned changes compose into a self-consistent
pair of constants:
- With the cap in place, the worst-case compaction step becomes 36B ×
(cache/1024 entries) = 3.52% of the cache, at every dbcache size.
- So the headroom should be a percentage that covers that step plus one
block's organic growth — which says ~95%, not 98%: at dbcache=1000, 2%
headroom is 23.5MiB, still smaller than the ~35MiB capped step; 5% (59MiB)
covers it with margin. And because both the step and the headroom scale with
the cache, it works identically on the Pi. Concretely: LARGE = total −
max(10MiB, total × 5/100) — the 10MiB floor preserves current behavior for
tiny caches, and the comment can show the 3.52% derivation so the constant
isn't magic.
That framing also writes the PR justification for touching upstream's formula:
"the 10MiB fixed headroom assumes smooth per-block growth; this PR introduces
deliberate stepwise accounting changes (bounded at 3.5% of the cache), so the
headroom must scale accordingly." A reviewer can check that reasoning in one
sitting.
Remaining tail risk — a map bucket-array doubling near the ceiling — stays
theoretically possible on both master and branch (a late doubling can step
~7%), but your patch actually shrinks it: fewer map entries means the last
doubling happens earlier in the cycle, further from the ceiling.
So the post-FP-run queue is: (1) the limit cap, (2) the 95% threshold, (3)
re-run and gate on exceeds total space == 0, (4) delete the CLOSE/NearFull
experiment if — as I now expect — it's no longer needed, healing
validation_flush_tests too. Then commit everything and rebase onto #35295.
✻ Brewed for 1m 27s
※ recap: Optimizing Bitcoin Core's dbcache via compact spent-coin storage plus bloom filter; benchmarks show ~11% faster reindex. Your bloom FP-stats run is in progress; when it finishes I'll apply the spents-limit cap and 95% flush-threshold change.
❯ the running benchmark data directory is /home/larry/dotbitcoin, and it looks
like the FP rate is just over 2%, as expected, you can check it now while
it's running
Searched for 1 pattern (ctrl+o to expand)
● Good news — you misread it by two orders of magnitude: that's 0.02%, not 2%.
The log lines read e.g. false-positive rate=0.02283% — two hundredths of a
percent. The consistent range across all 13 flush cycles so far is
0.020%–0.027%, which lands right where the blocked-bloom math says it should
for ~16–26 bits/entry (the power-of-two rounding makes it wobble depending on
where the vector size falls), and comfortably inside my <0.1% prediction.
The other predictions are holding too:
- true=0 on every line so far — zero genuine hits, confirming the binary
search post-filter is executing only ~1,000 times per ~4.7M probes (0.02%).
The "faster binary search" question is now empirically dead: the code it would
speed up runs once per five thousand lookups.
- Probes are ~4–5M per cycle with positives ~1,000 — so the filter is
eliminating 99.98% of the binary searches the no-bloom run performed. That's
the 7m42s / 331s CPU you measured, now with its microscopic mechanism visible.
One structural note from the cadence: those filter lines are arriving every ~2
minutes, which means ReallocCompactSpents is running at the periodic
incremental Sync(), not just at full flushes — so each line covers one sync
interval. Fine for statistics (arguably nicer — more samples), just worth
knowing that "13 lines" ≠ "13 full flush cycles" when you read the log later.
Keep an eye out for one fun detail as the run passes height ~91,800: if a
true=1 ever appears, it'll be the BIP30 checks meeting the historical
duplicate-coinbase outpoints — the only legitimate true positives a valid
chain can produce. If the whole run ends true=0 everywhere, that's fine too
(those heights may fall before the first compaction of their cycle).
✻ Churned for 1m 23s
❯ you can tell if it's a flush versus sync in the log by seeing large=1 (flush)
or large=0 (which is a sync), so far i see only 2 sync out of 15. i don't
really understand how it decides whether to do a flush versus sync
● Let me read the actual decision logic so I can explain it precisely:
Searched for 1 pattern, read 1 file (ctrl+o to expand)
● Now I can give you the precise answer — it's one line, validation.cpp:2836:
empty_cache ? CoinsTip().Flush() : CoinsTip().Sync();
where empty_cache = FORCE_FLUSH || fCacheLarge || fCacheCritical. So the rule
is:
- Flush (large=1) — the write was triggered by memory pressure (cache ≥ LARGE,
i.e. within 10MiB of budget, or CRITICAL, or an explicit FORCE_FLUSH like
shutdown/resize). Since the whole point is reclaiming memory, it writes
everything and wipes the cache.
- Sync (large=0) — the write was triggered for any other reason while memory
is fine: overwhelmingly the periodic timer (m_next_write, randomized 50–70
min, line 2851 — randomized so the network's nodes don't all hit disk in
sync), plus prune-driven writes and FORCE_SYNC. Its purpose is bounding
crash-recovery replay time, and since memory isn't the problem, it writes the
dirty entries but keeps the cache contents warm — throwing away hot coins
would be pure waste.
Your 13-flush/2-sync ratio then falls right out: every write (either kind)
re-arms the 50–70 min timer, so a sync only happens when a flush cycle takes
longer than the timer. At this run's pace the cache is filling to LARGE
roughly every 2 minutes, so memory pressure nearly always wins the race; the
two syncs mark stretches where a cycle happened to span the timer (or
startup).
Two details worth knowing since they touch our work:
1. There's a mode asymmetry: fCacheCritical is only checked at IF_NEEDED
(called during block activation) while fCacheLarge only at PERIODIC (called
between activations). That's why CRITICAL exists at all as a distinct state —
it's the "can't wait for the next periodic check" escape hatch. Our goal of
zero CRITICALs amounts to: LARGE + the headroom must always catch it first.
2. Line 2845 right below is your commented-out CLOSE → NearFull() hook —
confirming it's positioned exactly where a "compact proactively before we get
squeezed" trigger would go, if the cap + 95% threshold don't already zero the
CRITICALs on their own.
Also note what Sync means for our patch: it drains and clears m_compact_spents
(the erases get batch-written) but keeps the map's unspent coins — so after a
sync the vector restarts empty while the cache stays hot. That's handled
correctly by ReallocCompactSpents being called from both paths.
✻ Churned for 1m 2s
❯ one thing i've wondered for a while (before starting on this project) is if
it might be possible for Flush to retain the cache entries (but they would be
non-dirty) like Sync does. i would think that would make the cache much more
efficient, since in many cases it wouldn't have to fault unspent coins in
from leveldb (they would be cache hits). but yes, there would still be memory
pressure, but what might be possible (or maybe not, i was just thinking
might be possible) is, when we need a cache entry, just grab one of the
existing non-dirty unspent entries and use (replace) it with the new coin, no
need to write it to disk since it's not dirty. there's probably a reason
this isn't possible or is a bad idea, but i don't understand why it wouldn't
work.
● Your instinct is correct — there's no fundamental reason it can't work. Clean
(non-dirty) entries are, by definition, exact copies of what's in leveldb, so
retaining them after a flush is always safe, and evicting one at any moment is
always safe (no write needed, exactly as you say, and no FRESH-flag or
crash-consistency hazard — those only constrain dirty entries). What you're
describing is turning the coins cache from a "write buffer that gets wiped"
into a true replacement cache. The reasons it isn't done are engineering
economics, not correctness:
1. Finding a victim is the missing machinery. Dirty and fresh entries are on
the linked lists; clean entries are on no list at all. "Grab a non-dirty
entry" means either adding a third linked list (16 bytes/entry overhead on
everything, plus list maintenance on the hottest paths — note your
spent/unspent sentinel split is literally the pattern you'd extend) or a
roaming bucket-cursor that skips dirty entries (cheap, effectively random
eviction — and random is respectable here since the salted hasher makes
iteration order adversary-proof).
2. The pool allocator changes what "freeing" means. Erasing one entry returns
its node to the pool freelist, not to the OS. For steady-state replacement
(your proposal) that's perfectly fine — evict one, insert one, RSS constant.
Where it bites is bulk shrinking: nTotalSpace is dynamic (the coins cache
borrows unused mempool budget, so it shrinks as the mempool fills), and today
the only way to actually give memory back is the full wipe + ReallocateCache.
A retaining cache needs an answer for "the budget just dropped 200MB" beyond
node-by-node erase that reclaims nothing.
3. The IBD payoff is real but bounded. Right after a flush the cache refills
mostly with new dirty coins; retention only converts misses on older coins
into hits. Spends are recency-biased, so with 1GB holding ~6% of the UTXO set,
a decent slice of your 609M leveldb lookups would become hits — but nothing
like all of them. And the value has been shrinking: #35295's parallel prefetch
now hides much of the refault latency (though retention would still save the
work — the CPU/leveldb-op argument, same one your patch benefits from).
4. Upstream has been circling this for years. The special case already merged:
prune-driven flushes no longer wipe the cache (that was andrewtoth's #28280)
— which is exactly your idea applied where it was free, since prune flushes
aren't memory-pressure flushes. Sync() itself is another half-step: retain
everything, when memory permits. The full version — retain + evict-on-demand —
has been experimented with periodically and always stalled on the same trade:
policy machinery on the hottest code path in the node, versus gains that
benchmarks found modest and workload-dependent. Old experiments also predate
both the pool allocator and prefetching, so the calculus keeps shifting.
The connection to your patch is worth noticing: compact-spents attacks the
same waste from the opposite end. Retention says "don't throw away clean
entries"; your patch says "make entries cheap enough that we flush 39% less
often" — every avoided flush is an avoided wipe, so you're already recovering
a chunk of what retention would recover, with no eviction policy needed.
They'd compose, but definitely as a separate future PR — this one has exactly
the right scope as it stands.
If you want to gauge the ceiling empirically someday: count, at each flush,
how many of the wiped clean unspent entries get re-faulted from leveldb within
the following cycle. That number (vs. 609M total lookups) is retention's
maximum possible win on this workload — measurable with a few log lines,
before writing any eviction code.
✻ Cooked for 1m 26s
❯ but maybe the clean coins can be on a new linked list? so all coins would be
on a list, unspent dirty, spent (that separation is what this patch does), or
unspent clean. then we could find a clean coin to replace in O(1) time. am i
missing something?
● No, you're not missing anything fundamental — and in fact the idea is cheaper
than I implied a moment ago, for a reason sitting right in front of us: every
CCoinsCacheEntry already carries the m_prev/m_next pointers. Clean entries
just leave them null today. The 16 bytes/entry I called overhead is already
paid for on every entry in the map; a clean list would simply use dead space.
Your CoinsSentinel struct extends naturally: unspent, spent, clean.
The hot-path cost analysis also comes out better than "list maintenance on the
hottest paths" suggests:
- A pure cache hit (AccessCoin/GetCoin on an existing entry) changes no flags
— zero list operations. The hottest operation is untouched.
- List links change only at state transitions (fault-in, spend, add,
flush-clean) — and every one of those already does something far more
expensive (a map insert/erase or a disk write). The relink is noise.
- Head-of-list eviction gives you FIFO-by-fault-time victims in O(1). True LRU
would require relinking on every read — that's the one variant that does
touch the hot path, and you likely don't need it; FIFO or CLOCK-style second
chance is a perfectly respectable policy for this workload.
So the mechanism is sound. What remains — the things that make it a real
project rather than an afternoon — are these three:
1. Budget shrinkage. Steady-state replacement (evict one clean, admit one new)
keeps memory flat via the pool freelist. But when nTotalSpace drops — the
mempool reclaiming its borrowed budget — per-entry eviction returns nothing to
the OS; the pool arena only shrinks on full teardown. The workable hybrid:
retain-and-replace normally, keep the full wipe + ReallocateCache as the
rare-path response to a genuine budget reduction (and shutdown). You need that
code anyway — it already exists.
2. Flush semantics shift. Flush becomes "write dirty entries, move them to the
clean list, keep everything" — i.e. Sync — and the only thing that ever
empties the map is the rare-path wipe. Memory pressure then gets absorbed by
eviction instead of flushing, which means the flush trigger itself wants
rethinking (you'd flush when the dirty portion alone threatens the budget,
since clean entries are reclaimable on demand). That's a deeper behavioral
change than it first appears — it touches the same GetCoinsCacheSizeState
logic we're already planning to adjust.
3. Proving it's worth it. The review bar for adding an eviction policy to
consensus-critical caching is high, and #35295 has already harvested some of
the same win by hiding refault latency. This is why I'd still run the cheap
measurement first: log, at each flush, how many of the about-to-be-wiped clean
unspent entries get re-faulted from leveldb in the next cycle. That single
number is the retention win's ceiling. If it's 5% of lookups, the idea waits;
if it's 40%, you have the opening line of the next PR's description.
One more pleasant synergy worth noting: eviction gives your NearFull() hook a
gentler cousin — when approaching the budget, evicting clean coins is even
cheaper than compacting spents, and completely incremental. A retain-cache
with both levers might rarely need memory-pressure flushes at all.
But all of that is the PR after this one. The current patch stands alone, the
sentinel refactor you've already done is the natural foundation for the clean
list, and the measurement hook can ride along whenever. For now: FP run, cap,
95% threshold, zero CRITICALs, commit, rebase.
✻ Brewed for 1m 5s
❯ oh yes, absoutely for sure, this would be way after the current one, if we
pursue it at all (but it seems like it may be worth pursuing)
● Agreed — it goes in the "promising, measurable, later" bucket. Let me add it
to the project notes so it doesn't get lost:
Wrote 1 memory (ctrl+o to expand)
● Saved. Meanwhile the queue for the current PR stands: FP run finishes → apply
the limit cap and the 95% LARGE threshold → re-run and gate on zero exceeds
total space events → strip the debug instrumentation → commit → rebase onto
#35295 → re-benchmark against the new base. I'll be ready whenever the run
completes.
✻ Worked for 15s
❯ one thing interesting in the logs that actually shows the compaction is
working is that, for a while after each CompactSpents event, the cache= log
at the end of each log line for each block doesn't change. that's expected,
because the coins being created are using memory that was freed by compacting
spents. but the DynamicMemory measurement of the cache is based on the pool
resource, so that memory is still allocated by the system (you were saying
this too just above). so it's interesting, but could be slightly confusing to
someone watching the logs who doesn't understand this. they would be
wondering, why isn't the cache going up?
● Your explanation is exactly right, and it's worth being precise about the
mechanism because it's actually the patch's win made visible:
memusage::DynamicUsage on a pool-allocated map counts allocated chunks in the
arena, not live nodes. Compaction returns ~a million nodes to the pool
freelist in one instant (chunk count unchanged, so cache= doesn't drop), and
then every new coin for the next stretch of blocks consumes a freelist node
instead of forcing a new chunk (so cache= doesn't rise either). The plateau
ends exactly when the freelist drains. Each block connected during a plateau
is a block of free cache occupancy — that flat line is precisely the "3.5×
cheaper spent coins" claim, drawn in the log.
Two things worth noting about the confusion concern:
1. The phenomenon isn't new, just amplified. Master already exhibits a milder
version: every FRESH coin spent within a cycle erases its map entry, feeding
the freelist, so master's cache= also grows sub-linearly during high-churn
stretches. Your patch turns a gentle sub-linearity into conspicuous flat
plateaus following visible CompactSpents events. And the accounting itself is
correct — arena-based measurement is what tracks actual RSS, which is what the
dbcache budget exists to bound. You wouldn't want it to report freelist space
as free.
2. The cheap fix is to make the log self-explanatory. Since the CompactSpents
debug line immediately precedes each plateau, extending it to say what
happened to memory closes the loop for any log reader, e.g.:
2. CompactSpents compacting: 940481 existing: 4702408 (freed ~120 MB of map
nodes for reuse; cache usage will plateau until consumed)
2. — or a shorter version, plus a sentence in the eventual code comment / PR
description pointing out that flat cache= after compaction is expected and is
the mechanism working. Reviewers reading their own test logs will hit exactly
the confusion you predicted, so preempting it in the PR text is probably worth
more than the log wording itself.
I'd fold the log-line tweak in with the cap change after the FP run finishes —
same file, same function, one commit.
✻ Crunched for 41s
❯ i don't quite like making that log line have that many words, but yes, maybe
something shorter, or just document elsewhere. that reminds me, we should
mention in the PR that one way to think about why this patch works is that
once a coin becomes spent, it stays spent until the reset event (flush or
sync), which maps perfectly to both a vector and bloom filter, because you
can't remove entries from those. leaving it as a map entry gives it
flexibility (being erased) that is unneeded, and there's a natural price to
pay for that. there is a VERY unusual scenario where a spent coin can be
deleted, actually replaced by an unspent, which is a chain reorg, but we
cover that too by giving priority to the map, which BatchWrite does by
processing the compact spents first (you said this earlier, that we need to
very clearly document this requirement). another thing: some may wonder, as i
initially did, whether these new data structures should use the pool
resource. but the answer is definitely no, because first of all, the pool
resource only works with fairly small allocations, although that can be
increased. but mainly because, its real advantage is when items are allocated
and freed very often, when there is a lot of churn, which there is in the
dbcache. but the spents don't have churn, so there's no advantage to having
the vector and bloom filter be part of the resource. might be worth
explaining that in the PR description
● All three points are exactly the kind of material that makes a big PR
reviewable, and rather than let them scatter across our conversation, let me
start a working draft file that accumulates them — we keep generating
PR-description content (benchmarks, mechanisms, design rationale) and it
should live somewhere durable:
● Write(pr-notes.md)
⎿  Wrote 116 lines to pr-notes.md
1 # PR description working notes — compact spents (draft, not for
2
3 Scratch material for the eventual PR description / commit messag
4
6
7 Once a coin becomes spent, it stays spent until the next reset e
8 Sync). The set of spent coins is therefore *append-only within a
n epoch*, and
tion — a sorted
a bloom filter
… +169 lines (ctrl+o to expand)
● Created pr-notes.md in the repo root (marked as a scratch draft, not for
commit) with everything so far — the monotonicity framing, the BatchWrite
ordering requirement, the pool-resource rationale, all benchmark numbers, and
the pending cap/threshold items.
On your specific points:
The monotonicity framing is the best one-paragraph explanation of the patch we
have, and I'd lead the PR design section with it. One sharpening worth
adopting: even the reorg case isn't a deletion from the vector — the re-added
coin lives in the map, the map shadows the vector on every lookup, and the
epoch-boundary ordering (spents erased before cursor writes) makes the newer
state win. So the vector's append-only property is literally never violated;
"deletion" is expressed as shadowing plus ordering. That's a stronger claim
than "we handle the exception" and it's what makes the design easy to trust.
Pool resource: agreed on both your reasons, and I added a third that reviewers
will appreciate — keeping the structures out of the pool keeps their
accounting and lifecycle independent of the arena (they're counted explicitly
and freed at reset without touching the pool's chunk lifecycle). Your churn
argument is the core one though: the pool amortizes high-frequency node-sized
alloc/free, and the spents structures do ~a dozen large long-lived allocations
per cycle. Opposite workload shape in both size and frequency.
Log line: agreed on brevity. Shortest useful form would be appending just the
freed byte count, e.g. CompactSpents compacting: 940481 existing: 4702408
freed=115MiB — one token, and a curious reader greps from there. The full "why
the plateau" explanation belongs in a comment above CompactSpents and one
line in the PR description, which is where I've put it in the notes. We can
finalize wording when the cap change goes in.
✻ Cogitated for 1m 15s
❯ love all that. could you make the change now to that log line? On the
monotonicity aspect, one thing i had to think about long and hard is, i was
worried about the following sequence: coin gets spent and turned into a spent
coin (it was not FRESH), then coin gets re-created as unspent, then it gets
deleted somehow. then that shadowing would no longer exist. There is an
Uncache method, and I'm not sure what it's used for, but i convinced myself
that this couldn't be a problem.. i think because it would always be DIRTY,
and if you look at that method, it won't touch a dirty coin.
● Both done below — the log line change, and your Uncache reasoning deserves to
be captured because it's exactly right and it generalizes into a clean
invariant. First the code:
● Update(src/coins.cpp)
⎿  Added 5 lines, removed 1 line
622 assert(m_initial_compact_spents > 0);
623 assert(m_spent_count >= m_spents_limit);
624 assert(m_spent_count > 0);
625 - LogDebug(BCLog::COINDB, "CompactSpents compacting: %i exis
-ting: %i\n", m_spent_count, m_compact_spents.size());
625 + // The freed value is approximate (map pool nodes are ~128
+ bytes). The freed
626 + // nodes return to the pool's freelist, not to the OS, so
+the reported cache
627 + // size plateaus after this event until new coins consume
+the freelist.
628 + LogDebug(BCLog::COINDB, "CompactSpents compacting: %i exis
+ting: %i freed~%iMiB\n",
629 + m_spent_count, m_compact_spents.size(), m_spent_c
+ount * 128 / (1 << 20));
630 //FreeSpents();
631 CompactSpentsList new_spents;
632 new_spents.reserve(m_spent_count);
Ran 1 shell command (ctrl+o to expand)
● Update(pr-notes.md)
⎿  Added 26 lines
25 > spents first makes the newer state win. Both `CCoinsViewDB::B
atchWrite` and
26 > `CCoinsViewCache::BatchWrite` do this; the base-class doc com
ment states it.
27
28 +### The shadow invariant (why shadowing can never be lost mid-e
+poch)
29 +
30 +The scary scenario: coin spent (non-FRESH) → compacted into the
+ vector → coin
31 +re-created unspent (a DIRTY map entry now *shadows* the stale v
+ector entry) →
32 +the map entry somehow disappears → the vector would wrongly rep
+ort the coin
33 +spent. This cannot happen; enumerate every path that removes or
+ cleans a map
34 +entry:
35 +
36 +1. **`Uncache()`** — the mempool's anti-pollution eviction — re
+fuses DIRTY
37 + entries, and a shadowing entry is always DIRTY (`AddCoin` ma
+rks DIRTY
38 + unconditionally). Safe.
39 +2. **`SpendCoin` of a FRESH re-added coin** erases the map entr
+y — but that is
40 + itself a semantic spend: the coin is now spent again, so the
+ surviving
41 + vector entry states the truth once more, and its pending lev
+eldb erase is
42 + exactly right. Safe.
43 +3. **`SetClean` transitions** occur only while `BatchWrite` con
+sumes the cursor
44 + — i.e. during Flush/Sync, the epoch reset that drains and cl
+ears the vector
45 + at the same moment (`ReallocCompactSpents`). Safe.
46 +4. **`Reset()`** clears map and vector together. Safe.
47 +
48 +Invariant: *a map entry shadowing a vector entry is always DIRT
+Y, and every
49 +path that removes or cleans a map entry is either itself a sema
+ntic state
50 +change that restores the vector entry's truth, or an epoch rese
+t that empties
51 +the vector simultaneously.* Worth stating in a code comment nea
+r
52 +IsCompactSpent.
53 +
54 ## Why the new structures do NOT use the pool resource
55
56 A question reviewers (and we ourselves, initially) will ask: sh
ould
● Done, syntax-checked (no rebuild while your run is going — the new freed~%iMiB
field will show up starting with the next build, alongside the cap change).
On what Uncache is actually for, since you mentioned not being sure: it's the
mempool's anti-pollution valve. When a transaction arrives for mempool
acceptance, validating it faults its input coins into the dbcache. If the tx
is rejected, those coins would linger as useless clean entries — and a peer
could exploit that by spamming invalid transactions to bloat your cache and
evict genuinely useful entries. So MemPoolAccept tracks which coins it pulled
in (coins_to_uncache) and calls Uncache on them when acceptance fails. The
reason it refuses dirty entries is fundamental, not incidental: a dirty entry
is unflushed state — removing it would silently lose a write. Which is
precisely why your reasoning holds: the refusal that protects normal dirty
state automatically protects the shadow entries too.
And your instinct to chase that scenario "long and hard" was the right
paranoia — I've written it up in the notes as the general shadow invariant
with all four removal/clean paths enumerated: Uncache (refuses dirty),
FRESH-shadow re-spend (the erase is itself a semantic spend, so the surviving
vector entry becomes true again), SetClean (only happens inside the epoch
reset that drains the vector anyway), and Reset (clears both together). Each
path is either semantics-preserving or simultaneous-with-vector-drain, so
shadowing can't be lost mid-epoch. That enumeration is PR-description gold —
it's the difference between "we think reorgs are handled" and "here is the
closed list of ways a shadow can disappear, and why each is safe" — and it
deserves a compact version in a comment near IsCompactSpent when we do the
cleanup pass.
✻ Churned for 1m 46s
❯ just a quick check, in the LogDebug lines i've been adding, and i see you did
this too just now, i'm using the format character %i (integer), is that the
best to use for size_t values? i think it's intended to be the most flexible,
so should be ok for both signed and unsigned, and various sizes of integers,
but just thought i'd ask you to make sure
● Short answer: with Bitcoin Core's logging it's safe, because
LogDebug/strprintf are tinyformat, not C printf — and tinyformat is type-safe.
The actual argument type (deduced by template) determines how the value is
rendered; the conversion character mostly just selects formatting (base,
width, etc.). So %i with a size_t prints the full 64-bit value correctly — no
truncation, no undefined behavior. The same holds for %d and %u; you'd have
needed %zu only in real printf, which is exactly the trap tinyformat exists to
remove.
Two nuances worth knowing:
1. Convention: the codebase overwhelmingly uses %d (and %u) for integral types
including size_t — %i is functionally identical but rare, so a reviewer might
nit it purely for consistency. When we do the cleanup pass I'd normalize the
new lines to %d.
2. What can still go wrong: the count of format specifiers must match the
argument count — that's now checked at compile time (ConstevalFormatString),
which is also why a stray literal % in a log message fails to build. And the
one genuinely dangerous place is any string that might ever reach a real C
printf-family function (e.g. leveldb internals or third-party code) — there
the C rules apply again.
So your instinct was right in effect, though the reason is "tinyformat ignores
your lie about the type" rather than "%i is flexible" — in C printf, %i is
strictly int-sized and would have been wrong for size_t.
✻ Baked for 22s
❯ okay i agree, let's change those to %d to head off review complaints, i'm
fine with that! if you want to do that now, go for it
Searched for 1 pattern (ctrl+o to expand)
● Update(src/txdb.cpp)
⎿  Added 1 line, removed 1 line
170
171 // It's important to erase these spent coin first, in case
a coin in this spent
172 // list is also in the cursor; the cursor coin must be wri
tten (not erased).
173 - LogDebug(BCLog::COINDB, "txdb BatchWrite spents %i\n", spe
-nts.size());
173 + LogDebug(BCLog::COINDB, "txdb BatchWrite spents %d\n", spe
+nts.size());
174 for (const COutPoint& outpoint : spents) {
175 CoinEntry entry(&outpoint);
176 batch.Erase(entry);
● Update(src/txdb.cpp)
⎿  Added 1 line, removed 1 line
201
202 LogDebug(BCLog::COINDB, "Writing final batch of %.2f MiB\n
", batch.ApproximateSize() / double(1_MiB));
203 m_db->WriteBatch(batch);
204 - LogDebug(BCLog::COINDB, "Committed %u (%i spent) changed t
-ransaction outputs (out of %u) to coin database...", (unsigned
- int)dirty_count, spent_count, (unsigned int)count);
204 + LogDebug(BCLog::COINDB, "Committed %u (%d spent) changed t
+ransaction outputs (out of %u) to coin database...", (unsigned
+ int)dirty_count, spent_count, (unsigned int)count);
205 LogDebug(BCLog::COINDB, "Lookups %u", m_lookups);
206 }
207
● Update(src/coins.cpp)
⎿  Added 1 line, removed 1 line
625 // The freed value is approximate (map pool nodes are ~128
bytes). The freed
626 // nodes return to the pool's freelist, not to the OS, so
the reported cache
627 // size plateaus after this event until new coins consume
the freelist.
628 - LogDebug(BCLog::COINDB, "CompactSpents compacting: %i exis
-ting: %i freed~%iMiB\n",
628 + LogDebug(BCLog::COINDB, "CompactSpents compacting: %d exis
+ting: %d freed~%dMiB\n",
629 m_spent_count, m_compact_spents.size(), m_spent_c
ount * 128 / (1 << 20));
630 //FreeSpents();
631 CompactSpentsList new_spents;
Ran 1 shell command (ctrl+o to expand)
● Done — all three branch-added %i uses are now %d (txdb.cpp's two lines and the
CompactSpents line), syntax-checked clean. The upstream %is in validation.cpp
are untouched, and I left the %us alone (including the filter-stats line)
since %u is also idiomatic in the codebase — though all of these
instrumentation lines are on the strip-before-PR list anyway, so their format
specifiers are mostly living on borrowed time.
Running tally of changes queued but not yet built (waiting on your FP run):
the %d normalizations and the freed~MiB log field. Still pending after that:
the limit cap, the 95% LARGE threshold, then the full rebuild + test pass.
✻ Cooked for 40s
❯ in a C project i worked on a long time ago, we had a test mode in which our
equivalent of LogDebug would generate the string to print but not actually
print it anywhere (just go reduce screen clutter or to keep the log file from
being too big). This was very helpful because without this, there could be
bugs at the LogDebug call sites where there could be a crash but only if a
specific log category was enabled. so for example, LogPrint("%d", a/b) ....
if b is zero, but the LogPrint never evaluates its arguments, this would sail
right through, no problem. But then a customer enables that log, and boom,
crash. but i think bitcoin core prevents this kind of problem too, doesn't
it?
● Let me check the exact mechanics in this tree so the answer is precise:
Searched for 1 pattern, ran 1 shell command (ctrl+o to expand)
● Confirmed both halves. The macro is the classic conditional form — if
(shouldlog(category)) { format-and-print(__VA_ARGS__); } — so arguments are
not evaluated when the category is disabled, exactly the hazard class from
your C project. Your a/b example would sail through silently until a user
enables that category. Bitcoin Core doesn't have a "format but discard" mode;
instead it defuses the problem from three other directions:
1. The test framework runs every functional-test node with -debug (all
categories) — that's test_node.py:144, with only leveldb and rand excluded for
noise. So CI is your old test mode in effect: every LogDebug site reached by
a functional test gets its arguments fully evaluated and formatted, and a
divide-by-zero there fails the test rather than a customer's node. The
residual exposure is exactly what you'd expect: sites in the two excluded
categories, and sites (or crashing input values) no test reaches.
2. Format strings are checked at compile time — the count of specifiers must
match the argument count, verified via consteval machinery regardless of
whether the category is ever enabled. So the "enabled a category and the
format string itself was broken" variant can't survive a build.
3. tinyformat's type safety removes the UB class — a wrong conversion
character can't corrupt memory the way mismatched printf specifiers can in C;
worst case is a formatting exception, not a wild pointer read.
What none of that covers, and the developer notes warn about explicitly, is
the flip side of conditional evaluation: side effects in log arguments —
LogDebug(cat, "%d", counter++) increments only for users with the category on,
which is a behavior fork rather than a crash and arguably nastier to debug.
For our branch this all lands comfortably: the new lines are in coindb, which
functional tests enable, so feature_block.py has been evaluating and
formatting every one of them on each run — and their arguments are all plain
member reads with no side effects and no arithmetic that can trap (the
m_spent_count * 128 / (1 << 20) divides by a constant).
✻ Sautéed for 1m 14s
❯ cool, thanks for checking all that, i thought so but wasn't positive. One
other thing we should mention in the PR is that feature_block.py does test
the filter/binary search logic, because if you disable those lookups, that
test fails. i believe that's the only functional test that fails, but great
that there is one! Also, the test_bitcoin coins_test fails if that lookup is
disabled ... Hey here's a thought! what if we add code that says, if the
bloom filter returns not found, we still do the binary search but within an
Assume? won't be any performance penalty on optimized build, but will check
in debug build (but still not slow things down too much) ... reviewers might
like to have that assurance
● Let me double-check the Assume semantics in this tree before answering,
because the answer hinges on it:
Searched for 1 pattern, ran 1 shell command (ctrl+o to expand)
● Update(src/coins.h)
⎿  Added 9 lines, removed 1 line
705 // The filter cheaply resolves the common case (no
t a compacted spent,
706 // including the empty-vector case); the binary se
arch confirms the rest.
707 ++m_filter_probes;
708 - if (!m_spents_filter.MayContain(outpoint)) return
-false;
708 + if (!m_spents_filter.MayContain(outpoint)) {
709 + if constexpr (G_ABORT_ON_FAILED_ASSUME) {
710 + // Debug builds only (compiles to nothing
+in release, where
711 + // it would negate the filter's benefit):
+verify the filter's
712 + // no-false-negative guarantee, on which c
+orrectness depends.
713 + Assume(!std::binary_search(m_compact_spent
+s.begin(), m_compact_spents.end(), outpoint));
714 + }
715 + return false;
716 + }
717 ++m_filter_positives;
718 }
719 const bool spent{std::binary_search(m_compact_spents.b
egin(), m_compact_spents.end(), outpoint)};
Ran 1 shell command (ctrl+o to expand)
● Syntax-clean. Also adding your mutation-coverage observation to the PR notes,
alongside the full verification story:
● Update(pr-notes.md)
⎿  Added 19 lines
51 the vector simultaneously.* Worth stating in a code comment nea
r
52 IsCompactSpent.
53
54 +## Test coverage / verification of the lookup correctness
55 +
56 +- **Mutation coverage**: disabling the `IsCompactSpent()` looku
+ps (the mutant
57 + that would silently accept double-spends) fails both `feature
+_block.py`
58 + (functional) and `coins_tests` (unit). Believed to be the onl
+y tests that
59 + catch this mutant — good that they exist; worth saying so in
+the PR.
60 +- **No-false-negative guarantee, verified two ways**:
61 + 1. `SanityCheck()` asserts `MayContain(v)` for every element
+of the vector —
62 + by contrapositive this proves any filter-negative outpoint
+ is not in the
63 + vector, exhaustively for the current state. Runs throughou
+t unit/fuzz tests.
64 + 2. Debug builds additionally verify per lookup: on every filt
+er-negative
65 + answer, an `if constexpr (G_ABORT_ON_FAILED_ASSUME)` block
+ runs the binary
66 + search inside `Assume`. Compiles to nothing in release. (N
+B: a bare
67 + `Assume` would NOT work — Assume always evaluates its expr
+ession in all
68 + build types; only the abort is debug-gated.)
69 +- `compact_spents_filter_basic` unit test: no false negatives o
+ver 10k inserts,
70 + FP rate bound, empty/Reset behavior. `compact_spents_lookup`
+unit test:
71 + end-to-end spend→compact→lookup-shadowing→flush against a bas
+e cache.
72 +
73 ## Why the new structures do NOT use the pool resource
74
75 A question reviewers (and we ourselves, initially) will ask: sh
ould
● Done and noted. The verification story now has a nice two-pronged shape for
reviewers:
1. Exhaustive, state-level: the SanityCheck loop asserts MayContain(v) for
every vector element — whose contrapositive is the no-false-negative guarantee
for the entire current state, checked constantly throughout unit and fuzz
tests.
2. Continuous, lookup-level (your idea, debug builds only): every
filter-negative answer re-verifies against the binary search, via if constexpr
(G_ABORT_ON_FAILED_ASSUME) so release builds compile it out entirely — the
assurance you wanted reviewers to have, at literally zero release cost.
And your mutation-coverage point is in the notes too — "commenting out the
lookup fails feature_block.py and coins_tests, and here's why those are the
only two" is exactly the kind of sentence that saves a reviewer an afternoon
of wondering whether the test suite would catch the scariest possible
regression.
One small heads-up for your Pi plans: since you build debug with clang-22,
that debug build will run the per-lookup verification — so don't benchmark
with it (which you wouldn't anyway; benchmarks come from the gcc build/ dir
where it compiles away).
Current queue unchanged: FP run finishes → limit cap + 95% LARGE threshold →
rebuild, full test pass, re-check CRITICAL count → strip instrumentation →
commit → rebase onto #35295.
✻ Cogitated for 1m 38s
❯ wow, that's great i had no idea G_ABORT_ON_FAILED_ASSUME exists! that's very
cool. yes, i realized right after i typed that message to you, the Assume
does run in optimized builds, it just doesn't care about the result (doesn't
crash), so it does need to be guarded by G_ABORT_ON_FAILED_ASSUME, excellent
... glad you mentioned that about clang, makes sense ... I think this guard
may be too much clutter for the final version, but reviewers will have an
opinion on that, definitely good to have in the first review version, love
that.
● Agreed on all counts — ship it in the first review version and let reviewers
vote. If it survives review but they want it tidier, it collapses nicely into
a one-liner helper (VerifyFilterMiss(outpoint)) so the hot path reads clean,
but that's a bikeshed for later. There's also a decent chance a reviewer
strengthens it instead — "debug-only verification of the scary invariant"
tends to be popular with the people who review consensus-adjacent code.
Status stands: waiting on the FP run, then cap + threshold + rebuild +
CRITICAL recount, strip instrumentation, commit, rebase. The draft PR material
in pr-notes.md now covers the framing, the shadow invariant, the
pool-resource rationale, the verification story, and all benchmark evidence —
it'll be a well-armed PR.
✻ Churned for 12s
❯ i don't quite understand how it could be strengthened?
● Fair question — "strengthen" was doing a lot of work in that sentence.
Concretely, the kinds of things a reviewer might ask for beyond the per-lookup
debug check:
1. A dedicated fuzz target for the filter itself. There's direct precedent:
src/test/fuzz/bloom_filter.cpp and rolling_bloom_filter.cpp exist for the
other bloom filters in the codebase. The pattern is differential: feed random
insert/query sequences, mirror them in a std::set, and assert the
no-false-negative property on every query — so the guarantee gets checked
against adversarial input shapes (empty resets, size-1 filters, duplicate
inserts, boundary counts around the power-of-two rounding) rather than the
friendly random data in our unit test. New probabilistic data structure in
consensus-adjacent code + existing precedent = very likely request.
2. Enabling compaction in coinscache_sim. This is the big one, and it's
already on your "figure out later" list. That fuzzer compares the whole cache
hierarchy against a naive ground-truth model — running it with
SetCompactSpentsThreshold(1) would exercise compaction, the filter, shadowing,
and the BatchWrite ordering against arbitrary operation sequences including
reorg-like patterns. It's the strongest verification the design can get, much
stronger than the per-lookup check (which only validates the filter against
the vector, not the vector against the truth).
3. Cheap always-on asserts at the rebuild sites. The per-lookup check is
debug-only by necessity (it's expensive). But O(1) consistency checks are free
enough for release: e.g. after CompactSpents, assert the filter is non-empty
iff the vector is — catching a desync at its source rather than waiting for a
lookup to trip over it.
4. static_asserts on the filter's constants — HASHES * 9 <= 64, block size is
a power of two, etc. — so a future "tune the parameters" patch can't silently
break the bit-packing.
The per-lookup positive direction, for completeness, needs no check at all:
when the filter says "maybe," the binary search is the answer — it's
self-verifying by construction.
Items 3 and 4 are five-minute additions we could just include; items 1 and 2
are real work that could either go in the first version (preempting the
request) or wait to see if reviewers ask. Given that #2 doubles as the answer
to your deferred "should tests enable compaction" question, it's probably
worth doing before the PR goes up.
✻ Worked for 26s
❯ oh i see ... another thought is, this definitely would be just just for
initial review, is to call SanityCheck from a few strategic places (but i've
seen before that there are places you can't call it where you would think you
can, because of the weird coins_tests -- it does things that aren't possible
in production) guarded by G_ABORT_ON_FAILED_ASSUME ... too bad there's not a
clang equivalent for that!
● Quick check on how that flag actually gets set, because there's good news
hiding in your lament:
Searched for 1 pattern (ctrl+o to expand)
● Good news on the lament: there's nothing to wish for —
G_ABORT_ON_FAILED_ASSUME is build-type-driven, not compiler-driven.
ABORT_ON_FAILED_ASSUME is a compile definition CMake sets for Debug
configurations, so your clang-22 debug/ build gets it automatically, identical
to gcc. And notice the other branch of that constexpr: fuzzing builds force
it true unconditionally — meaning the per-lookup verification we just added
has been silently promoted: it runs during every coins fuzz execution too,
where the inputs are adversarial. That's a nicer property than we ordered.
On strategic SanityCheck calls — yes, with one placement trick that
neutralizes exactly the coins_tests weirdness you've been burned by before.
The problem: the matrix tests raw-insert states that can't exist in production
(SPENT_CLEAN, SPENT_FRESH...) and deliberately skip SanityCheck for them; if
we embed calls inside coins.cpp functions those tests invoke (AddCoin,
BatchWrite...), Debug CI trips over intentionally-impossible states. The
dodge: put the calls one layer up, in validation.cpp — e.g. right after
CoinsTip().Flush() / Sync() in FlushStateToDisk, and perhaps after
CompactSpents-heavy stretches via the periodic flush path. Unit tests never
execute FlushStateToDisk, so only production-shaped state ever reaches those
checks. Guarded with if constexpr (G_ABORT_ON_FAILED_ASSUME), it costs nothing
in release, and in Debug it's an O(map + vector) pass at flush cadence —
trivial next to a Debug-build IBD.
One more pattern worth knowing exists, in case reviewers want verification
available in release builds too: the mempool precedent. CTxMemPool::check() is
a full internal-consistency audit that runs in any build, gated by the
-checkmempool=<ratio> runtime flag rather than build type. A -checkcoins-style
knob doing periodic SanityCheck would follow that exact precedent — probably
overkill for this PR, but it's the established answer if someone asks "how
would a release user verify this."
So the initial-review hardening menu, cheapest first: the two static_asserts,
the O(1) rebuild-site asserts, validation-level SanityCheck calls under the
debug guard, and — the substantial one — compaction enabled in coinscache_sim.
All are queued behind the cap/threshold work; none block the benchmark
analysis.
✻ Cogitated for 53s
※ recap: Improving IBD via the compact-spents dbcache patch; currently waiting for your bloom-enabled FP-stats reindex to finish. Next: apply the compaction-limit cap and 95% flush threshold, then rebuild and confirm zero critical flushes before commit and rebase.
❯ ok so i've lost track a little .. are there code changes you can make now? if
so, go ahead please
● Yes — several queued items are now unblocked. Let me first check whether your
FP benchmark is still running (that gates whether I can build):
Ran 1 shell command (ctrl+o to expand)
● Update(src/coins.cpp)
⎿  Added 8 lines, removed 6 lines
654 for (const COutPoint& outpoint : m_compact_spents) {
655 m_spents_filter.Insert(outpoint);
656 }
657 - // 20% growth each flush cycle
658 - // LMR this 20% is just an intuitive guess, of course
659 - m_spents_limit = m_compact_spents.size() * 20 / 100;
660 - // LMR needed because in some tests, we start this mechani
-sm very small,
661 - // initial_compact_spents = 1 (anything non-zero enables i
-t).
662 - // But we have to make progress
657 + // Geometric growth (20% of the vector size) keeps the tot
+al merge work per
658 + // flush cycle linear in the final vector size (a fixed ba
+tch size would be
659 + // quadratic). The cap bounds how much map-resident spent
+bloat (~128 bytes
660 + // per entry) can accumulate late in a cycle to the same l
+evel the initial
661 + // threshold already deems acceptable (~1/8 of the cache),
+ at the cost of a
662 + // few extra sub-second merges.
663 + m_spents_limit = std::min(m_compact_spents.size() * 20 / 1
+00, m_initial_compact_spents);
664 + // Tests can enable compaction with a threshold as small a
+s 1; always make progress.
665 if (m_spents_limit == 0) m_spents_limit = 1;
666 }
667
● Update(src/validation.h)
⎿  Added 11 lines, removed 2 lines
520 {
521 // No periodic flush needed if at least this much space is
free
522 constexpr int64_t MAX_BLOCK_COINSDB_USAGE_BYTES{int64_t(10
_MiB)};
523 - return std::max((total_space * 9) / 10,
524 - total_space - MAX_BLOCK_COINSDB_USAGE_BYTE
-S);
523 + // The fixed 10 MiB headroom assumes cache usage grows smo
+othly with each
524 + // block. Compacting spent coins steps the measured usage
+by up to 36 bytes
525 + // per compacted entry in a single block — bounded by the
+compaction
526 + // threshold (~1/1024th of the cache in entries, so ~3.5%
+of the cache in
527 + // bytes; see CompactSpentsThreshold()). Cap the trigger a
+t 95% so the
528 + // headroom scales with the cache and always covers that s
+tep plus one
529 + // block's organic growth. (For caches below ~200 MiB the
+pre-existing
530 + // terms already provide at least this much headroom.)
531 + return std::min((total_space * 95) / 100,
532 + std::max((total_space * 9) / 10,
533 + total_space - MAX_BLOCK_COINSDB_U
+SAGE_BYTES));
534 }
535
536 //! Chainstate assumeutxo validity.
● Update(src/coins.h)
⎿  Added 4 lines
262 //! power of two.
263 static constexpr size_t BITS_PER_ENTRY{16};
264
265 + static_assert(WORDS_PER_BLOCK * 64 == 512, "a block must b
+e exactly one 512-bit cache line (bit positions are 9 bits)");
266 + static_assert(HASHES * 9 <= 64, "bit positions are drawn 9
+ bits at a time from a single 64-bit hash");
267 + static_assert(BITS_PER_ENTRY > 0);
268 +
269 //! Salts, random unless deterministic (see SaltedOutpoint
Hasher). The txid
270 //! part of an outpoint is already uniformly distributed,
but is chosen by
271 //! peers; the salt keeps an attacker from grinding txids
that concentrate
● Update(src/coins.h)
⎿  Added 3 lines
339 return true;
340 }
341
342 + //! True when the filter describes the empty set.
343 + bool Empty() const noexcept { return m_words.empty(); }
344 +
345 size_t DynamicMemoryUsage() const { return memusage::Dynam
icUsage(m_words); }
346 };
347
● Update(src/coins.cpp)
⎿  Added 4 lines
654 for (const COutPoint& outpoint : m_compact_spents) {
655 m_spents_filter.Insert(outpoint);
656 }
657 + // Cheap always-on check: the filter and vector must be in
+ lockstep (a
658 + // desync in the other direction, a false negative, is cau
+ght per-lookup in
659 + // debug builds and exhaustively by SanityCheck()).
660 + Assume(m_compact_spents.empty() == m_spents_filter.Empty()
+);
661 // Geometric growth (20% of the vector size) keeps the tot
al merge work per
662 // flush cycle linear in the final vector size (a fixed ba
tch size would be
663 // quadratic). The cap bounds how much map-resident spent
bloat (~128 bytes
● Update(src/validation.cpp)
⎿  Added 7 lines
2834 }
2835 // Flush the chainstate (which may refer to b
lock index entries).
2836 empty_cache ? CoinsTip().Flush() : CoinsTip()
.Sync();
2837 + if constexpr (G_ABORT_ON_FAILED_ASSUME) {
2838 + // Debug builds: full consistency audit o
+f the coins cache at
2839 + // flush boundaries. Placed here (not in
+coins.cpp) so unit
2840 + // tests that deliberately construct impo
+ssible cache states
2841 + // never reach it.
2842 + CoinsTip().SanityCheck();
2843 + }
2844 full_flush_completed = true;
2845 TRACEPOINT(utxocache, flush,
2846 int64_t{Ticks<std::chrono::microseconds>(
NodeClock::now() - nNow)},
Ran 1 shell command (ctrl+o to expand)
● Update(src/test/fuzz/coinscache_sim.cpp)
⎿  Added 4 lines
243 // Make sure there is always at least one CCoinsViewCa
che.
244 if (caches.empty()) {
245 caches.emplace_back(new CCoinsViewCache(&bottom, /
*deterministic=*/true));
246 + // Compact spent coins as aggressively as possible
+, so this fuzzer
247 + // exercises compaction, the bloom filter, and sha
+dowing against the
248 + // simulated ground truth.
249 + caches.back()->SetCompactSpentsThreshold(1);
250 sim_caches[caches.size()].Wipe();
251 }
252
● Update(src/test/fuzz/coinscache_sim.cpp)
⎿  Added 1 line
381 } else {
382 caches.emplace_back(new CoinsViewOverl
ay(&*caches.back(), /*deterministic=*/true));
383 }
384 + caches.back()->SetCompactSpentsThreshold(1
+);
385 // Apply to simulation data.
386 sim_caches[caches.size()].Wipe();
387 }
● Update(src/test/fuzz/coins_view.cpp) ⎿  Added 1 line
356 {
357 FuzzedDataProvider fuzzed_data_provider{buffer.data(), buf
fer.size()};
358 CCoinsViewCache coins_view_cache{&CoinsViewEmpty::Get(), /
*deterministic=*/true};
359 + coins_view_cache.SetCompactSpentsThreshold(1);
360 TestCoinsView(fuzzed_data_provider, coins_view_cache, &Coi
nsViewEmpty::Get());
361 }
362
● Update(src/test/fuzz/coins_view.cpp)
⎿  Added 1 line
370 };
371 CCoinsViewDB backend_coins_view{std::move(db_params), Coin
sViewOptions{}};
372 CCoinsViewCache coins_view_cache{&backend_coins_view, /*de
terministic=*/true};
373 + coins_view_cache.SetCompactSpentsThreshold(1);
374 TestCoinsView(fuzzed_data_provider, coins_view_cache, &bac
kend_coins_view);
375 }
376
● Update(src/test/fuzz/coins_view.cpp)
⎿  Added 4 lines
382 {
383 FuzzedDataProvider fuzzed_data_provider{buffer.data(), buf
fer.size()};
384 MutationGuardCoinsViewCache backend_cache{&CoinsViewEmpty:
:Get(), /*deterministic=*/true};
385 + // Compaction on the backend happens entirely inside Batch
+Write, which the
386 + // mutation guard permits; the overlay compacts during nor
+mal operation.
387 + backend_cache.SetCompactSpentsThreshold(1);
388 CoinsViewOverlay coins_view_cache{&backend_cache, /*determ
inistic=*/true};
389 + coins_view_cache.SetCompactSpentsThreshold(1);
390 TestCoinsView(fuzzed_data_provider, coins_view_cache, &bac
kend_cache);
391 }
● All edits in. Now the niced syntax checks across every touched TU:
Ran 1 shell command (ctrl+o to expand)
● Update(pr-notes.md)
⎿  Added 13 lines, removed 10 lines
139 silent bool→size_t misbinding at call sites).
140 - Bloom filter self-sizes from vector content (~2–4 bytes/entr
y, power-of-two
141 blocks); no configuration.
142 -- Growth rule: compact again when map spents reach 20% of the
-vector size
143 - (geometric growth keeps total merge work linear; fixed batch
-es would be
144 - quadratic). PENDING: cap the limit at the initial threshold
-—
145 - `min(size*20/100, m_initial_compact_spents)` — to bound late
--cycle map bloat
146 - (~200 MB at 8M vector) which currently causes CRITICAL flush
-es (8–12 per
147 - reindex vs 0 on master).
148 -- PENDING: LARGE flush threshold headroom should scale: `total
- − max(10MiB,
149 - 5%·total)`. The fixed 10 MiB assumes smooth per-block growth
-; compaction
150 - introduces deliberate stepwise accounting changes bounded (a
-fter the cap) at
151 - 36B × threshold ≈ 3.52% of the cache.
142 +- Growth rule: compact again when map spents reach 20% of the
+vector size,
143 + capped at the initial threshold — `min(size*20/100, m_initia
+l_compact_spents)`.
144 + Geometric growth keeps total merge work linear (fixed batche
+s would be
145 + quadratic); the cap bounds late-cycle map bloat (~200 MB at
+8M vector
146 + uncapped), which caused CRITICAL flushes (8–12 per reindex v
+s 0 on master).
147 + DONE 2026-07-26; verify CRITICAL count == 0 in the next benc
+hmark run.
148 +- LARGE flush threshold clamped at 95% of total space (was: fi
+xed 10 MiB
149 + headroom, which assumes smooth per-block growth; compaction
+steps accounting
150 + by up to 36B × threshold ≈ 3.52% of the cache in one block).
+ DONE 2026-07-26.
151 +- Fuzz coverage: all coins fuzz targets now run with compactio
+n threshold 1
152 + (coinscache_sim = differential vs ground-truth model). Debug
+ builds also
153 + run SanityCheck at every flush boundary and re-verify every
+filter-negative
154 + lookup. DONE 2026-07-26.
155
156 ## Memory spike during CompactSpents (be upfront in PR)
157
● Everything that can move without the machine has moved. When your FP run
finishes, the sequence is:
1. Full rebuild + verification — unit suite, feature_block.py, all four fuzz
targets (these now exercise compaction for the first time, so this run has
real teeth — I half-expect the sim fuzzer to find something interesting, and
if it does, that's it earning its keep).
2. A benchmark run with the cap + 95% threshold, gating on grep -c "exceeds
total space" == 0 — plus it'll produce the first freed~MiB log lines and
another timing data point.
3. If the CRITICALs are gone: delete the CLOSE/NearFull experiment (healing
validation_flush_tests), strip the instrumentation, commit, and start the
#35295 rebase.
Send word when the run's done — and I'm still curious whether a stray true=1
shows up in the filter stats around the duplicate-coinbase heights.
✻ Crunched for 3m 30s
❯ by the way I'd forgotten i added NearFull, it was for an earlier approach
where we wouldn't compact any coins until we were almost full. but i like
this current idea much better (compact as we go, exponentially increasing),
and i forgot to remove NearFull. but we probaly will want to bring that back
if and when we try implementing the other idea (keeping the cache hot after
Flush) ... i think that concept will be needed then .... okay, benchmark is
still running, i'll let you know when it's done
● Good context — that explains its shape perfectly (a trigger sitting exactly at
the "almost full" boundary). And you're right that it's conceptually
load-bearing for the retain-cache idea: in that design, "nearly full" stops
meaning "flush soon" and starts meaning "evict or compact something," so a
NearFull-style hook becomes the natural valve. Let me capture the provenance
so future-us doesn't reinvent it:
Wrote 1 memory (ctrl+o to expand)
● Noted. So the deletion queue after the benchmark is guilt-free: CLOSE, the
NearFull() comment, and the enum renumbering all go, validation_flush_tests
heals, and the concept lives on in the notes for the retain-cache project
where it actually belongs. Talk to you when the run's done.
✻ Sautéed for 16s
❯ you could remove that NearFull and CoinsCacheSizeState::CLOSE, we have a note
of it for later, definitely don't want reviewers to see that stuff
Searched for 1 pattern (ctrl+o to expand)
● Update(src/validation.h)
⎿  Added 2 lines, removed 3 lines
509 enum class CoinsCacheSizeState
510 {
511 //! The coins cache is in immediate need of a flush.
512 - CRITICAL = 3,
512 + CRITICAL = 2,
513 //! The cache is at >= 90% capacity.
514 - LARGE = 2,
515 - CLOSE = 1,
514 + LARGE = 1,
515 OK = 0
516 };
517
● Update(src/validation.cpp)
⎿  Added 1 line, removed 6 lines
2710 if (cacheSize > nTotalSpace) {
2711 LogInfo("Cache size (%s) exceeds total space (%s)\n",
cacheSize, nTotalSpace);
2712 return CoinsCacheSizeState::CRITICAL;
2713 - }
2714 - int64_t large_coins_cache_threshhold{LargeCoinsCacheThres
-hold(nTotalSpace)};
2715 - if (cacheSize > large_coins_cache_threshhold) {
2713 + } else if (cacheSize > LargeCoinsCacheThreshold(nTotalSpa
+ce)) {
2714 return CoinsCacheSizeState::LARGE;
2715 }
2718 - if (cacheSize > large_coins_cache_threshhold * 98 / 100)
-{
2719 - return CoinsCacheSizeState::CLOSE;
2720 - }
2716 return CoinsCacheSizeState::OK;
2717 }
2718
Ran 1 shell command (ctrl+o to expand)
● Update(src/validation.cpp)
⎿  Removed 2 lines
2844 (uint64_t)coins_mem_usage,
2845 (bool)fFlushForPrune);
2846 }
2847 - } else if (cache_state == CoinsCacheSizeState::CLOSE)
- {
2848 - //CoinsTip().NearFull();
2847 }
2848
2849 if (should_write || m_next_write == NodeClock::time_p
oint::max()) {
Ran 1 shell command (ctrl+o to expand)
● Gone — no references left, enum values restored to upstream (CRITICAL = 2,
LARGE = 1), and GetCoinsCacheSizeState is back to its exact upstream shape
(the only intentional remaining change in that area is the 95% clamp inside
LargeCoinsCacheThreshold, which for the small cache sizes
validation_flush_tests uses is a no-op — the 90% term dominates below ~200 MiB
— so that suite should go fully green on the next build).
That clears the last piece of dead experimental code from the diff. Current
uncommitted state is now, in its entirety: the compact-spents core, the bloom
filter + verification hardening, the cap + scaled threshold, the fuzz/test
coverage enabling, and the (deliberately temporary) instrumentation. Once your
benchmark finishes: full rebuild, complete test pass (expecting zero failures
anywhere for the first time), then the cap-verification benchmark whenever
you want to run it.
✻ Churned for 1m 4s
❯ side question about claude / opus : these diffs you're showing me as you do
them are very useful, but they're not in the transscript. is it possible,
after i exit from this session, to see those again, to recreate or reproduce
everything i'm seeing now?
● Good question — let me pull in the Claude Code documentation agent to make
sure I give you exact, current answers about transcript storage and resume
behavior:
● claude-code-guide(Check transcript/diff persistence)
⎿  Done (12 tool uses · 28.9k tokens · 1m 29s)
(ctrl+o to expand)
● Yes — everything is recoverable, at several levels of convenience:
1. The session transcript is fully preserved on disk. Every session lives at
~/.claude/projects/<project-slug>/<session-id>.jsonl (this one:
~/.claude/projects/-sd-g-w-bitcoin-spentdb/2fdc30fe-....jsonl). Every
Edit/Write call is stored with its complete old_string/new_string, so all the
diffs you've watched can be reconstructed exactly. Caveats: the JSONL format
is internal and can change between releases, and sessions are cleaned up after
30 days by default — if you want this one kept indefinitely, set
cleanupPeriodDays higher in ~/.claude/settings.json, or export it (next point)
while it's fresh.
2. /export gives you a durable, human-readable copy. Run /export <filename> in
this session (or after resuming it) and you get the rendered conversation as
a plain-text file — the most practical "keep this forever" option.
3. Resuming re-enters the session itself. claude --continue reopens the most
recent session in this directory; claude --resume gives a picker (Space
previews a session). History, context, and the conversation state come back.
Whether old diffs re-render with their original terminal formatting on resume
is (per the docs agent I just consulted) not guaranteed — the reliable
rendering path is /export.
4. But the strongest answer for this project is one you already built: git.
The diffs I show you are increments; their sum is sitting in your working tree
right now. You staged the pre-bloom state deliberately "to see the edits you
will do" — so git diff is, at this moment, the complete, canonical, replayable
record of everything from the bloom filter onward, and git diff --staged
covers the earlier phase. The moment we commit (soon, pre-rebase), that
history becomes permanent regardless of transcript retention. If you want
finer granularity than two big diffs, that's an argument for committing the
work as a series of logical commits rather than one blob — which we should do
anyway for reviewability, and which I'd propose structuring as roughly: core
compact-spents, bloom filter, threshold scaling, test/fuzz coverage,
instrumentation (to be dropped).
Practical recommendation: run /export at the end of today's session for the
discussion record, and let git carry the code record. Between those two plus
pr-notes.md and my memory files, nothing we've done here is at risk of being
unreproducible.
✻ Cooked for 2m 11s
❯ yes, actually, i thought that already, before review, this should definitely
be broken into logical commits ... i like to not worry about that too early,
because it's tedious (at least when i do this manually) to make changes to
earlier commits ... so i like to just do everything without any commits,
maybe staging once in a while, before organizing the commits. but we will
have to do that, i'm sure you can help with that
● That workflow makes total sense — and the tedious part (interactive rebase
surgery when an early commit needs a tweak) is exactly the part I'm good at
absorbing, since I can rebuild the series from scratch cheaply: with the full
diff in the working tree, I can stage hunks selectively (git add -p-style) and
construct the logical series in one pass, then verify each commit builds and
passes tests individually (reviewers and git bisect both appreciate that). If
a change to an early commit is needed later, regenerating the series is often
easier than editing it.
One sequencing note: the commit organization has to happen before the #35295
rebase (rebase moves commits, so they need to exist), so the natural order
stands as: benchmark → verification → strip instrumentation → build the commit
series → rebase. Rough series shape, refined when we get there:
1. Sentinel split (spent/unspent lists) + Merge() — mechanical foundation, no
behavior change
2. Core compact-spents: vector, CompactSpents, BatchWrite signature +
ordering, IsCompactSpent
3. Bloom filter + debug verification
4. Threshold scaling (SetCompactSpentsThreshold, CompactSpentsThreshold, cap,
95% clamp)
5. Test/fuzz coverage (unit tests, fuzz enablement, SanityCheck extensions)
Let me record the workflow preference:
Wrote 2 memories (ctrl+o to expand)
● Saved (and fixed a stale "5.6%" in the index while I was there). So when the
time comes, you review the series — commit boundaries, messages, what goes
where — and I do the assembly and any later surgery. Until then, keep working
commitless exactly as you prefer. Awaiting the benchmark.
✻ Cooked for 32s
❯ your help actually can change my workflow, it seems ... since you're good at
amending earlier commits, we can probably move to separate commits earlier
than what i've done in the past, that's great
● That's a nice second-order effect — the cost of maintaining structure just
dropped, so structure earlier becomes rational. The practical middle ground
that works well: once the code stabilizes (say, after this benchmark cycle),
we cut the series, and from then on new work goes into whichever commit it
logically belongs to via fixups I fold in — you get the "just keep working"
feel with a review-ready series at all times, plus per-commit history if you
ever want to bisect your own branch. Let me update the note so it reflects the
evolving preference rather than freezing the old one:
Wrote 1 memory (ctrl+o to expand)
● Updated. And this branch is actually the perfect first candidate: once your
benchmark confirms the cap kills the CRITICALs, the code is stable enough to
cut the series — then the remaining work (instrumentation strip, rebase
fallout, review feedback) lands as targeted amendments to the right commits
instead of one growing blob. We'll be practicing the new workflow on the PR
that motivated it.
✻ Cooked for 19s
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment