Repository: bitcoin/bitcoin (language: cpp)

Issue:
RAM usage regression in 26.x and master on ARM 32-bit
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

The `bitcoind -dbcache=100 -par=4 -disablewallet -reindex-chainstate` command fails with OOM. First reported in https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1807197107.

### Expected behaviour

RAM usage does not exceed 0.9 GB.

### Steps to reproduce

100% reproducibility with Guix compiled binaries.

### Relevant log output

```
2023-11-17T18:43:31Z [loadblk] UpdateTip: new best=000000000000000016d35e6ca77dea6f4e2caa387f245a712b1df5bbab3c559c height=314541 version=0x00000002 log2_work=80.091273 tx=44112815 date='2014-08-08T11:42:41Z' progress=0.049410 cache=105.6MiB(13710725txo)
```

### How did you obtain Bitcoin Core

Compiled from source

### What version of Bitcoin Core are you using?

master@5aa0c82ccd6ceb4a141686fc8658f679de75a787

### Operating system and version

Armbian 23.8.3 jammy, 1 GB swap-file

### Machine specifications

armv7l, 8 cores, 2 GB RAM


Discussion / hints:
A regression is caused by https://github.com/bitcoin/bitcoin/pull/25325.

cc @martinus @sipa @achow101
Do we have a 32-bit CI task already?
> Do we have a 32-bit CI task already?

~x86, not ARM.~ we do
This is a shot in the dark, but could this be due to memory fragmentation? The `PoolResource` allocates blocks of 262144 bytes, which seems quite large for such a system. Could you try this patch, or is there a way for me to try this? It reduces the allocated block size to 16K (and fixes a few tests that rely on the hardcoded size).

```patch
diff --git a/src/support/allocators/pool.h b/src/support/allocators/pool.h
index c8e70ebacff..9f93511995e 100644
--- a/src/support/allocators/pool.h
+++ b/src/support/allocators/pool.h
@@ -184,7 +184,7 @@ public:
     /**
      * Construct a new Pool Resource object, defaults to 2^18=262144 chunk size.
      */
-    PoolResource() : PoolResource(262144) {}
+    PoolResource() : PoolResource(16384) {}
 
     /**
      * Disable copy & move semantics, these are not supported for the resource.
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 12dc4d1ccc4..5f1a0120aca 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -1088,18 +1088,18 @@ BOOST_AUTO_TEST_CASE(coins_resource_is_used)
 {
     CCoinsMapMemoryResource resource;
     PoolResourceTester::CheckAllDataAccountedFor(resource);
-
+    size_t num_elements = 16384 / 200;
     {
         CCoinsMap map{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource};
         BOOST_TEST(memusage::DynamicUsage(map) >= resource.ChunkSizeBytes());
 
-        map.reserve(1000);
+        map.reserve(num_elements);
 
         // The resource has preallocated a chunk, so we should have space for at several nodes without the need to allocate anything else.
         const auto usage_before = memusage::DynamicUsage(map);
 
         COutPoint out_point{};
-        for (size_t i = 0; i < 1000; ++i) {
+        for (size_t i = 0; i < num_elements; ++i) {
             out_point.n = i;
             map[out_point];
         }
diff --git a/src/test/validation_flush_tests.cpp b/src/test/validation_flush_tests.cpp
index 7398091215c..c8f2c9b32fa 100644
--- a/src/test/validation_flush_tests.cpp
+++ b/src/test/validation_flush_tests.cpp
@@ -37,7 +37,7 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate)
     };
 
     // PoolResource defaults to 256 KiB that will be allocated, so we'll take that and make it a bit larger.
-    constexpr size_t MAX_COINS_CACHE_BYTES = 262144 + 512;
+    constexpr size_t MAX_COINS_CACHE_BYTES = 16384 + 512;
 
     // Without any coins in the cache, we shouldn't need to flush.
     BOOST_TEST(
```
@martinus 

> This is a shot in the dark, but could this be due to memory fragmentation? The `PoolResource` allocates blocks of 262144 bytes, which seems quite large for such a system. Could you try this patch, or is there a way for me to try this? It reduces the allocated block size to 16K (and fixes a few tests that rely on the hardcoded size).

I've tested your [patch](https://github.com/hebasto/artefacts/blob/bisect-arm-OOM.1/bisect-arm-OOM/bitcoin-274a7b769afb-arm-linux-gnueabihf.tar.gz). No change in behavior, unfortunately. It still fails with OOM.
It could be that the memory usage calculation has changed, and calculation for memory used in the CCoinsMap is now more correct than before. Previously it overestimated memory usage, and now it's more correct but for the same cache size this also leads to a bit higher memory usage
> It could be that the memory usage calculation has changed, and calculation for memory used in the CCoinsMap is now more correct than before. Previously it overestimated memory usage, and now it's more correct but for the same cache size this also leads to a bit higher memory usage

It depends on definition of "a bit higher" :) It was less than 0.9 GB of RAM. Now 2 GB of RAM plus 1 GB swap is not enough.
"a bit higher" should be about 2% or so... I don't think there is a memory leak anywhere.. Is this issue only on ARM or also on x86 32bit?
I investigated a bit with [heaptrack](https://github.com/KDE/heaptrack) and it seems there are a bit higher spikes than I have thought. It's still not too bad, without the pool heaptrack says peack memory is ~600MB, and with the pool it's ~700 MB. That bigger peak seems to happen in `CCoinsViewCache::BatchWrite`.

Previously, the `mapCoins.erase(it)` freed a node, and a subsequent `cacheCoins[it->first]` could use that freed memory. With the pool that is currently not possible any more, because the `mapCoins.erase(it)` doesn't actually free the memory but puts it into the pool. And because `mapCoins` and `cacheCoins` use two different resources, that memory can't be reused.

I'll have a look if it is possible to use the same pool for both `mapCoins` and `cacheCoins`, this should reduce peak memory usage. If this would also fix the ARM issue, I can't say...
From the observing the RAM usage during runtime, I would say that RAM usage increases over time until OOM.
From [IRC](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-11-17#984227;):
> 19:42 \<sipa\> and it's only with guix builds?

I also can confirm the regression for binaries that were natively compiled with GCC 11.4.0.
@hebasto could you try another patch? https://github.com/martinus/bitcoin/commit/04fb0b35cf18d5805d58595c27f027fae4428e62 This removes the placement new that didn't reuse the returned pointer, which technically has always been illegal
Is this difference in the reported cache size expected:
- in master @ 3a93957a5dc97cb2fd0656d1e2451ebef57204df (pre-[PR25325](https://github.com/bitcoin/bitcoin/pull/25325)):
```
UpdateTip: new best=00000000000001bc255d63d3e82af7638566a5fbaadb6d5bdc41d0731a23d1ea height=209285 version=0x00000001 log2_work=69.067375 tx=9192843 date='2012-11-24T00:09:17Z' progress=0.010297 cache=376.0MiB(3276665txo)
Cache size (394377008) exceeds total space (394371840)
```
- in master @ 5aa0c82ccd6ceb4a141686fc8658f679de75a787 ([PR25325](https://github.com/bitcoin/bitcoin/pull/25325) merged):
```
UpdateTip: new best=00000000000001bc255d63d3e82af7638566a5fbaadb6d5bdc41d0731a23d1ea height=209285 version=0x00000001 log2_work=69.067375 tx=9192843 date='2012-11-24T00:09:17Z' progress=0.009937 cache=26.3MiB(3276665txo)
```
?
> @hebasto could you try another patch? [martinus@04fb0b3](https://github.com/martinus/bitcoin/commit/04fb0b35cf18d5805d58595c27f027fae4428e62) This removes the placement new that didn't reuse the returned pointer, which technically has always been illegal

Tested. No behavior change. Still OOM.
> Is this difference in the reported cache size expected

Yes, when the pool is used the flushes happen at different times because more transactions are cached, so the number of cached transaction at certain heights can be completely different

> Tested. No behavior change. Still OOM.

Thanks for testing! Hm, I'm out of ideas for now... I don't see anything in the code that could cause this. My only suspicion is that for some reason on ARM the freed memory from the pool is not reused, but I don't see why that would be the case
> > Is this difference in the reported cache size expected
> 
> 
> 
> Yes, when the pool is used the flushes happen at different times because more transactions are cached, so the number of cached transaction at certain heights can be completely different

The point is that the former log shows the first flush. In the latter one, there were no flushes by that point at all. 
> The point is that the former log shows the first flush. In the latter one, there were no flushes by that point at all.

That's ok, with pool it flushes later. But on second glance, the cache size is incorrect: `cache=26.3MiB(3276665txo)`
That's not possible, there's cache can only be 26.3MiB with 3276665tx in it. So this looks like the cache size estimation doesn't work for some reason. Can you post the full log? 
> Can you post the full log?

See https://github.com/hebasto/artefacts/blob/arm-OOM-logs/arm-OOM-logs/debug.log-04fb0b35cf18
Thanks a lot for the log! I am now pretty certain that the allocated blocks from the pool are not counted in `DynamicUsage`. I can reproduce practically exactly same cache sizes as in the log when I set `usage_chunks=0` here (and run 32bit bitcoind on x86): https://github.com/bitcoin/bitcoin/blob/master/src/memusage.h#L201 
So the `bucket_count` is calculated correctly which can be seen by the memory jumps.

So either `pool_resource->ChunkSizeBytes()` or `pool_resource->NumAllocatedChunks()` must return 0 for unknown reasons...
@martinus Is it possible that due to a mistake in the guessed node size, the utxo cache entries are allocated through the fallback to the normal allocator?
> @martinus Is it possible that due to a mistake in the guessed node size, the utxo cache entries are allocated through the fallback to the normal allocator?

Which part of code is responsible for that?
@hebasto The guess for node size is here: https://github.com/bitcoin/bitcoin/blob/v26.0rc2/src/coins.h#L148

Can you try increasing that (say, add 64 to it), and see if that fixes things?
> @martinus Is it possible that due to a mistake in the guessed node size, the utxo cache entries are allocated through the fallback to the normal allocator?

That sounds like a very likely explanation! Although I'm curious why the estimation would work in x86 32bit but not on ARM
Might it depend on `max_load_factor`?
@hebasto No, it shouldn't.

We're essentially trying to guess the `sizeof` of an internal data structure in `std::unordered_map`. If we guess too small, allocations do not go to the pool, but a fallback to the normal allocator is used.

@martinus Should we make sure that even allocations that use the fallback get accounted for in the resource?
@sipa I thought that too, it would definitely make sense. Although it would need to move the `size_t MallocUsage(size_t alloc);` into the pool too

If the size increase does not help, it could also be that ARM for some reason needs a bigger alignment for its nodes.
In any case it would be interesting to run this again with this commit: https://github.com/martinus/bitcoin/commit/eaa6aaa481c45866815d67dc80c509be15f22974 this will log each time the pool is called with an allocation size it can't handle, which should be very noisy.
@martinus 
> If the size increase does not help, it could also be that ARM for some reason needs a bigger alignment for its nodes. In any case it would be interesting to run this again with this commit: [martinus@eaa6aaa](https://github.com/martinus/bitcoin/commit/eaa6aaa481c45866815d67dc80c509be15f22974) this will log each time the pool is called with an allocation size it can't handle, which should be very noisy.

I've already tested my own similar commit. There were no log entries for it.
@hebasto Did the `+ 64` change change anything at all? 
> @hebasto Did the `+ 64` change change anything at all?

It is running... Will report soon.
> I've already tested my own similar commit. There were no log entries for it.

There should always be quite a few log lines for the map buckets, even when all works as expected
> @hebasto Did the `+ 64` change change anything at all?

Tested https://github.com/hebasto/bitcoin/commit/80c02a9fe6607fb0a4abfca66be7be42fd744905.

No behavior change. Still OOM.
> > I've already tested my own similar commit. There were no log entries for it.
> 
> There should always be quite a few log lines for the map buckets, even when all works as expected

Testing now...
> > I've already tested my own similar commit. There were no log entries for it.
> 
> There should always be quite a few log lines for the map buckets, even when all works as expected

The log is flooded with "Allocate: 104 align(8), MAX_BLOCK_SIZE_BYTES=112, ALIGN_BYTES=4"
> The log is flooded with "Allocate: 104 align(8), MAX_BLOCK_SIZE_BYTES=112, ALIGN_BYTES=4"

Ha! This is the issue right there. For whatever reason the nodes require alignment of 8 bytes, but the pool is configured to use the platform's 4 byte instead. Thanks a lot for all the testing! I'll try to prepare a fix

Fix patch:
diff --git a/src/bench/pool.cpp b/src/bench/pool.cpp
index b3e54d85a254f..b2a5f8debf9d0 100644
--- a/src/bench/pool.cpp
+++ b/src/bench/pool.cpp
@@ -37,8 +37,7 @@ static void PoolAllocator_StdUnorderedMapWithPoolResource(benchmark::Bench& benc
                                    std::hash<uint64_t>,
                                    std::equal_to<uint64_t>,
                                    PoolAllocator<std::pair<const uint64_t, uint64_t>,
-                                                 sizeof(std::pair<const uint64_t, uint64_t>) + 4 * sizeof(void*),
-                                                 alignof(void*)>>;
+                                                 sizeof(std::pair<const uint64_t, uint64_t>) + 4 * sizeof(void*)>>;
 
     // make sure the resource supports large enough pools to hold the node. We do this by adding the size of a few pointers to it.
     auto pool_resource = Map::allocator_type::ResourceType();
diff --git a/src/coins.h b/src/coins.h
index a6cbb0313393c..bbf9e3895facd 100644
--- a/src/coins.h
+++ b/src/coins.h
@@ -145,8 +145,7 @@ using CCoinsMap = std::unordered_map<COutPoint,
                                      SaltedOutpointHasher,
                                      std::equal_to<COutPoint>,
                                      PoolAllocator<std::pair<const COutPoint, CCoinsCacheEntry>,
-                                                   sizeof(std::pair<const COutPoint, CCoinsCacheEntry>) + sizeof(void*) * 4,
-                                                   alignof(void*)>>;
+                                                   sizeof(std::pair<const COutPoint, CCoinsCacheEntry>) + sizeof(void*) * 4>>;
 
 using CCoinsMapMemoryResource = CCoinsMap::allocator_type::ResourceType;
 
diff --git a/src/support/allocators/pool.h b/src/support/allocators/pool.h
index c8e70ebacff6c..873e260b65176 100644
--- a/src/support/allocators/pool.h
+++ b/src/support/allocators/pool.h
@@ -272,7 +272,7 @@ class PoolResource final
 /**
  * Forwards all allocations/deallocations to the PoolResource.
  */
-template <class T, std::size_t MAX_BLOCK_SIZE_BYTES, std::size_t ALIGN_BYTES>
+template <class T, std::size_t MAX_BLOCK_SIZE_BYTES, std::size_t ALIGN_BYTES = alignof(T)>
 class PoolAllocator
 {
     PoolResource<MAX_BLOCK_SIZE_BYTES, ALIGN_BYTES>* m_resource;


Test patch:
diff --git a/src/test/pool_tests.cpp b/src/test/pool_tests.cpp
index 8a07e09a442b5..5ad4afa3a108d 100644
--- a/src/test/pool_tests.cpp
+++ b/src/test/pool_tests.cpp
@@ -156,21 +156,20 @@ BOOST_AUTO_TEST_CASE(random_allocations)
 
 BOOST_AUTO_TEST_CASE(memusage_test)
 {
-    auto std_map = std::unordered_map<int, int>{};
-
-    using Map = std::unordered_map<int,
-                                   int,
-                                   std::hash<int>,
-                                   std::equal_to<int>,
-                                   PoolAllocator<std::pair<const int, int>,
-                                                 sizeof(std::pair<const int, int>) + sizeof(void*) * 4,
-                                                 alignof(void*)>>;
+    auto std_map = std::unordered_map<int64_t, int64_t>{};
+
+    using Map = std::unordered_map<int64_t,
+                                   int64_t,
+                                   std::hash<int64_t>,
+                                   std::equal_to<int64_t>,
+                                   PoolAllocator<std::pair<const int64_t, int64_t>,
+                                                 sizeof(std::pair<const int64_t, int64_t>) + sizeof(void*) * 4>>;
     auto resource = Map::allocator_type::ResourceType(1024);
 
     PoolResourceTester::CheckAllDataAccountedFor(resource);
 
     {
-        auto resource_map = Map{0, std::hash<int>{}, std::equal_to<int>{}, &resource};
+        auto resource_map = Map{0, std::hash<int64_t>{}, std::equal_to<int64_t>{}, &resource};
 
         // can't have the same resource usage
         BOOST_TEST(memusage::DynamicUsage(std_map) != memusage::DynamicUsage(resource_map));
@@ -182,6 +181,11 @@ BOOST_AUTO_TEST_CASE(memusage_test)
 
         // Eventually the resource_map should have a much lower memory usage because it has less malloc overhead
         BOOST_TEST(memusage::DynamicUsage(resource_map) <= memusage::DynamicUsage(std_map) * 90 / 100);
+
+        // Make sure the pool is actually used by the nodes
+        auto max_nodes_per_chunk = resource.ChunkSizeBytes() / sizeof(Map::value_type);
+        auto min_num_allocated_chunks = resource_map.size() / max_nodes_per_chunk + 1;
+        BOOST_TEST(resource.NumAllocatedChunks() >= min_num_allocated_chunks);
     }
 
     PoolResourceTester::CheckAllDataAccountedFor(resource);