Merge bitcoin/bitcoin#28913: coins: make sure PoolAllocator uses the correct alignment

d5b4c0b69e543de51bb37d602d488ee0949ba185 pool: change memusage_test to use int64_t, add allocation check (Martin Leitner-Ankerl)
ce881bf9fcb7c30bb1fafd6ce38844f4f829452a pool: make sure PoolAllocator uses the correct alignment (Martin Leitner-Ankerl)

Pull request description:

  The class `CTxOut` has a member `CAmount` which is an int64_t, and on ARM 32bit int64_t are 8 byte aligned, which is larger than the pointer alignment of 4 bytes.

  So for `CCoinsMap` to be able to use the pool, we need to use the alignment of the member instead of just `alignof(void*)`.

  This fixes #28906 (first noted in https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1807197107) and #28440.

ACKs for top commit:
  pinheadmz:
    ACK d5b4c0b69e543de51bb37d602d488ee0949ba185
  hebasto:
    re-ACK d5b4c0b69e543de51bb37d602d488ee0949ba185, the only change since my recent [review](https://github.com/bitcoin/bitcoin/pull/28913#pullrequestreview-1739334189) is an updated test.
  theStack:
    Tested ACK d5b4c0b69e543de51bb37d602d488ee0949ba185

Tree-SHA512: 4446793fad6d56f0fe22e09ac9ade051e86de11ac039cd61c0f6b7f79874242878a6a46a2c76ac3b8f1d53464872620d39139f54b1471daccad26d6bb1ae8ca1

4 files changed, 17 insertions, 15 deletions

diff --git a/src/bench/pool.cpp b/src/bench/pool.cpp
index b3e54d85a2..b2a5f8debf 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 a6cbb03133..bbf9e3895f 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 c8e70ebacf..873e260b65 100644
--- a/src/support/allocators/pool.h
+++ b/src/support/allocators/pool.h
@@ -272,7 +272,7 @@ public:
 /**
  * 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;
diff --git a/src/test/pool_tests.cpp b/src/test/pool_tests.cpp
index 8a07e09a44..5ad4afa3a1 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);