Port Bitcoin Core PR#25325: Pool-based memory resource for CCoinsMap

[Naviabheeman]

Adds a PoolAllocator backed by PoolResource for the UTXO cache map (CCoinsMap). Pre-allocates memory in 256KB chunks and serves node allocations from freelists, eliminating per-entry malloc/free overhead during IBD.

Changes:

• src/support/allocators/pool.h: new PoolResource + PoolAllocator
• src/coins.h: replace CCoinsMap typedef with PoolAllocator variant; add CCoinsMapMemoryResource member; declare ReallocateCache()
• src/coins.cpp: wire resource into CCoinsMap constructor; add ReallocateCache() to return chunk memory after each Flush()
• src/memusage.h: add DynamicUsage overload for PoolAllocator maps

Tapyrus adaptation notes:

• No m_deterministic flag; SaltedOutpointHasher{} used directly
• COutPoint uses hashMalFix (same sizeof as Bitcoin's hash field)
• No validation.cpp change needed (Tapyrus has no ResizeCoinsCaches)

Bitcoin Core reference: bitcoin/bitcoin@5aa0c82

[azuchi]

I have a question about the Flush() implementation.

In both the current Tapyrus and Bitcoin Core, cacheCoins.clear() is always executed.
However, in this PR, it is only executed when fOk is true:

if (fOk) {                                                                                                                                                                                                                   
    cacheCoins.clear();                                                                                                                                                                                                      
    ReallocateCache();                                                                                                                                                                                                       
}                                                                                                                                                                                                                            
cachedCoinsUsage = 0;  // <- This is always executed                                                                                                                                                                         

Is this change intentional? If so, could you explain the reasoning?

Also, when fOk is false, the cache is preserved but cachedCoinsUsage is reset to 0,
which seems inconsistent. Could you clarify this behavior as well?

[Naviabheeman]

if (fOk) was added under the assumption that on a failed flush the cache still had entries and needed to be preserved. That assumption was wrong because BatchWrite erases entries unconditionally regardless of outcome.

[azuchi]

Thanks for the analysis — that matches what I see when I trace through CCoinsViewDB::BatchWrite (txdb.cpp:108) and CCoinsViewCache::BatchWrite (coins.cpp:165): both erase entries from mapCoins inside the iteration loop, so cacheCoins is empty by the time BatchWrite returns regardless of fOk.

Given that, the cacheCoins.clear() here is a no-op. Bitcoin Core's PR #25325 handles the same situation by replacing the redundant clear with an invariant assertion:

if (fOk) {
    if (!cacheCoins.empty()) {
        throw std::logic_error("Not all cached coins were erased");
    }
    ReallocateCache();
}

This surfaces a programming error if BatchWrite ever stops erasing entries (e.g., a future refactor that breaks the in-loop erase). I'd suggest mirroring that here — it costs nothing at runtime and turns a silent regression into a loud one.

If you'd rather keep the current clear(), please add a short comment explaining that BatchWrite has already emptied the map and the call is defensive only.

[azuchi]

Additional review notes — the port itself looks faithful and the open thread on Flush() has a separate reply. Comments below cover testing and documentation gaps that would be worth addressing before merge.

[azuchi]

M1 — pool_tests.cpp is not ported (medium)

Bitcoin Core PR #25325 includes ~500 lines of unit tests for PoolResource covering allocation/deallocation, freelist behavior, chunk reuse across destruct/reconstruct cycles, alignment edge cases, and oversized allocations falling back to operator new. None of those tests are in this PR.

This is a 350-line bespoke allocator landing in production with no direct tests. The existing coins_tests.cpp exercises it indirectly through CCoinsMap, but that doesn't cover the freelist edge cases (e.g., chunk exhaustion mid-allocation, alignment > ELEM_ALIGN_BYTES falling through to ::operator new, Deallocate of pool-allocated vs new-allocated blocks).

Suggested fix: port src/test/pool_tests.cpp from Bitcoin Core. The PoolResourceTester friend class declared at line 116 is already in place expecting it.

[azuchi]

M2 — Placement-new pattern needs an explanatory comment (minor)

Destroy-and-reconstruct-in-place on member objects is unusual and easy to misread as undefined behavior. A reader unfamiliar with Bitcoin Core PR #25325 has to reverse-engineer the intent.

Suggested doc comment above the function:

// After Flush(), the pool resource still holds all chunks that were used
// during this flush cycle. We want to return that memory to the OS so the
// next flush cycle starts clean. We can't simply reset the resource because
// cacheCoins's allocator holds a pointer to it — so we tear down both objects
// and reconstruct them in place. The member addresses are preserved, so any
// references held elsewhere (there should be none, but defensively) remain
// valid.
//
// Order matters: cacheCoins must be destroyed before the resource it
// allocates from, and reconstructed after the new resource is in place.

No code change required.

[azuchi]

M3 — Make the declaration-order dependency explicit (minor)

m_cache_coins_memory_resource must be declared before cacheCoins because the latter's constructor takes &m_cache_coins_memory_resource. C++ initializes members in declaration order, so swapping these two lines would pass a pointer to an uninitialized object → UB.

A short comment makes this hard to break:

// Note: declaration order is load-bearing. m_cache_coins_memory_resource
// must precede cacheCoins because cacheCoins's allocator stores a pointer
// to it, which is dereferenced from cacheCoins's destructor (and from
// every allocate/deallocate). Reordering these is undefined behavior.
mutable CCoinsMapMemoryResource m_cache_coins_memory_resource{};
mutable CCoinsMap cacheCoins;

[azuchi]

M4 — Document the thread-safety contract (minor)

pool.h line 35 explicitly states "PoolResource is not thread-safe." In Tapyrus, cacheCoins (and therefore the resource) is protected by cs_main for all consensus paths, but that contract isn't stated at the declaration site.

Suggested:

// cacheCoins (and the underlying memory resource) must only be accessed
// while cs_main is held. PoolResource is not thread-safe — concurrent
// access from multiple threads is undefined behavior.
mutable CCoinsMap cacheCoins;

This is purely documentation, no behavior change.