Harden binary fuse filter

lib/binaryfusefilter.h

@@ -22,6 +22,16 @@
 
 typedef std::array<uint8_t, crypto_shorthash_KEYBYTES> binary_fuse_seed_t;
 
+#ifdef BUILD_TESTS
+
+#include "test/CovMark.h"
+
+// Test-only flags: force specific rare paths in populate() so tests can
+// exercise them without needing pathological hash distributions.
+inline bool gBinaryFuseForcePopulateError = false;
+inline bool gBinaryFuseForcePeelingFailure = false;
+#endif
+
 #ifndef XOR_MAX_ITERATIONS
 #define XOR_MAX_ITERATIONS \
     1000000 // probability of success should always be > 0.5, so with this many
@@ -256,14 +266,14 @@ template <typename T> class binary_fuse_t
         return h;
     }
 
-    // Construct the filter, returns true on success, false on failure.
-    // The algorithm fails when there is insufficient memory.
+    // Construct the filter. Throws std::runtime_error if population fails
+    // after max iterations (practically impossible).
     // For best performance, the caller should ensure that there are not
     // too many duplicated keys.
-    // While highly improbable, it is possible that the population fails, at
-    // which point the seed must be rotated. keys will be sorted and duplicates
-    // removed if any duplicate keys exist
-    [[nodiscard]] bool
+    // While highly improbable, it is possible that an iteration fails, at
+    // which point the seed is rotated and retried. keys will be sorted and
+    // duplicates removed if any duplicate keys exist
+    void
     populate(std::vector<uint64_t>& keys, binary_fuse_seed_t rngSeed)
     {
         ZoneScoped;
@@ -295,6 +305,9 @@ template <typename T> class binary_fuse_t
         std::vector<uint32_t> startPos(block);
         std::vector<uint32_t> h012(5);
 
+        // Non-zero sentinel past the last valid slot. The placement loop
+        // treats 0 as empty; this prevents startPos from advancing past
+        // the end of the array.
         reverseOrder.at(size) = 1;
         for (int loop = 0; true; ++loop)
         {
@@ -303,7 +316,8 @@ template <typename T> class binary_fuse_t
                 // The probability of this happening is lower than the
                 // the cosmic-ray probability (i.e., a cosmic ray corrupts your
                 // system).
-                return false;
+                throw std::runtime_error(
+                    "BinaryFuseFilter failed to populate after max iterations");
             }
 
             for (uint32_t i = 0; i < block; i++)
@@ -363,9 +377,17 @@ template <typename T> class binary_fuse_t
                 error = (t2count.at(h1) < 4) ? 1 : error;
                 error = (t2count.at(h2) < 4) ? 1 : error;
             }
+#ifdef BUILD_TESTS
+            if (!error && gBinaryFuseForcePopulateError && loop == 0)
+            {
+                error = 1;
+                gBinaryFuseForcePopulateError = false;
+            }
+#endif
             if (error)
             {
-                std::fill(reverseOrder.begin(), reverseOrder.end(), 0);
+                COVMARK_HIT(BINARY_FUSE_POPULATE_ERROR_RETRY);
+                std::fill_n(reverseOrder.begin(), size, 0);
                 std::fill(t2count.begin(), t2count.end(), 0);
                 std::fill(t2hash.begin(), t2hash.end(), 0);
 
@@ -419,6 +441,14 @@ template <typename T> class binary_fuse_t
                     t2hash.at(other_index2) ^= hash;
                 }
             }
+#ifdef BUILD_TESTS
+            if (gBinaryFuseForcePeelingFailure && loop == 0)
+            {
+                stacksize = 0;
+                duplicates = 0;
+                gBinaryFuseForcePeelingFailure = false;
+            }
+#endif
             if (stacksize + duplicates == size)
             {
                 // success
@@ -427,12 +457,18 @@ template <typename T> class binary_fuse_t
             }
             else if (duplicates > 0)
             {
+                COVMARK_HIT(BINARY_FUSE_DUPLICATE_REMOVAL);
                 // Sort keys and remove duplicates
                 std::sort(keys.begin(), keys.end());
                 keys.erase(std::unique(keys.begin(), keys.end()), keys.end());
                 size = keys.size();
+
+                // Size may hav decreased, so make sure we write a new sentinel
+                // value.
+                reverseOrder.at(size) = 1;
             }
 
+            COVMARK_HIT(BINARY_FUSE_POPULATE_PEELING_FAILURE_RETRY);
             // Reset everything except for the last entry in reverseOrder
             std::fill_n(reverseOrder.begin(), size, 0);
             std::fill(t2count.begin(), t2count.end(), 0);
@@ -457,8 +493,6 @@ template <typename T> class binary_fuse_t
             Fingerprints.at(h012.at(found)) = xor2 ^ Fingerprints.at(h012.at(found + 1)) ^
                                         Fingerprints.at(h012.at(found + 2));
         }
-
-        return true;
     }
 
   public:
@@ -497,10 +531,7 @@ template <typename T> class binary_fuse_t
         SegmentCountLength = SegmentCount * SegmentLength;
         Fingerprints.resize(ArrayLength);
 
-        if (!populate(keys, rngSeed))
-        {
-            throw std::runtime_error("BinaryFuseFilter failed to populate");
-        }
+        populate(keys, rngSeed);
     }
 
     explicit binary_fuse_t(stellar::SerializedBinaryFuseFilter const& xdrFilter)

src/bucket/DiskIndex.cpp

@@ -255,38 +255,7 @@ DiskIndex<BucketT>::DiskIndex(BucketManager& bm,
     // Binary Fuse filter requires at least 2 elements
     if (keyHashes.size() > 1)
     {
-        // There is currently an access error that occurs very rarely
-        // for some random seed values. If this occurs, simply rotate
-        // the seed and try again.
-        for (int i = 0; i < 10; ++i)
-        {
-            try
-            {
-                mData.filter =
-                    std::make_unique<BinaryFuseFilter16>(keyHashes, seed);
-                break;
-            }
-            catch (std::out_of_range& e)
-            {
-                auto seedToStr = [](auto seed) {
-                    std::string result;
-                    for (auto b : seed)
-                    {
-                        fmt::format_to(std::back_inserter(result), "{:02x}", b);
-                    }
-                    return result;
-                };
-
-                CLOG_ERROR(Bucket,
-                           "Bad memory access in BinaryFuseFilter with "
-                           "seed {}, retrying",
-                           seedToStr(seed));
-                seed[0]++;
-            }
-        }
-
-        // Population failure is probabilistic is very, very unlikely.
-        releaseAssertOrThrow(mData.filter);
+        mData.filter = std::make_unique<BinaryFuseFilter16>(keyHashes, seed);
     }
 
     CLOG_DEBUG(Bucket, "Indexed {} positions in {}", mData.keysToOffset.size(),

src/test/CovMark.cpp

@@ -0,0 +1,12 @@
+// Copyright 2026 Stellar Development Foundation and contributors. Licensed
+// under the Apache License, Version 2.0. See the COPYING file at the root
+// of this distribution or at http://www.apache.org/licenses/LICENSE-2.0
+
+#include "test/CovMark.h"
+
+#ifdef BUILD_TESTS
+namespace stellar
+{
+CovMarks gCovMarks;
+}
+#endif

src/test/CovMark.h

@@ -0,0 +1,121 @@
+// Copyright 2026 Stellar Development Foundation and contributors. Licensed
+// under the Apache License, Version 2.0. See the COPYING file at the root
+// of this distribution or at http://www.apache.org/licenses/LICENSE-2.0
+
+#pragma once
+
+// Coverage marks: a lightweight mechanism for tests to assert that specific
+// internal code paths were actually executed. Inspired by the "coverage marks"
+// pattern from Ferrous Systems.
+//
+// Usage in production code:
+//     COVMARK_HIT(SOME_RARE_PATH);
+//
+// Usage in test code:
+//     COVMARK_CHECK_HIT_IN_CURR_SCOPE(SOME_RARE_PATH);
+//     // ... code that should trigger SOME_RARE_PATH ...
+//
+// The check-hit guard records the counter on construction and verifies it
+// increased by scope exit. If the marked path was not hit, the test fails.
+
+#include <array>
+#include <atomic>
+#include <cstdint>
+#include <exception>
+#include <stdexcept>
+
+#ifdef BUILD_TESTS
+#include <fmt/format.h>
+#endif
+
+namespace stellar
+{
+
+// Each coverage mark is an enum value. Add new marks before COVMARK_COUNT.
+enum CovMark : std::size_t
+{
+    BINARY_FUSE_POPULATE_ERROR_RETRY = 0,
+    BINARY_FUSE_POPULATE_PEELING_FAILURE_RETRY,
+    BINARY_FUSE_DUPLICATE_REMOVAL,
+    COVMARK_COUNT // must be last
+};
+
+#ifdef BUILD_TESTS
+
+class CovMarks
+{
+    std::array<std::atomic<std::uint64_t>, CovMark::COVMARK_COUNT> mCounters{};
+
+  public:
+    void
+    hit(CovMark mark)
+    {
+        mCounters[mark].fetch_add(1, std::memory_order_relaxed);
+    }
+
+    std::uint64_t
+    get(CovMark mark) const
+    {
+        return mCounters[mark].load(std::memory_order_relaxed);
+    }
+
+    void
+    reset()
+    {
+        for (auto& c : mCounters)
+        {
+            c.store(0, std::memory_order_relaxed);
+        }
+    }
+};
+
+extern CovMarks gCovMarks;
+
+class CovMarkGuard
+{
+    CovMark mMark;
+    std::uint64_t mValueOnEntry;
+    char const* mFile;
+    int mLine;
+    char const* mName;
+
+  public:
+    CovMarkGuard(CovMark mark, char const* file, int line, char const* name)
+        : mMark(mark)
+        , mValueOnEntry(gCovMarks.get(mark))
+        , mFile(file)
+        , mLine(line)
+        , mName(name)
+    {
+    }
+
+    ~CovMarkGuard() noexcept(false)
+    {
+        if (std::uncaught_exceptions() == 0)
+        {
+            auto valueOnExit = gCovMarks.get(mMark);
+            if (valueOnExit <= mValueOnEntry)
+            {
+                throw std::runtime_error(
+                    fmt::format("{}:{}: coverage mark '{}' was not hit during "
+                                "this scope",
+                                mFile, mLine, mName));
+            }
+        }
+    }
+};
+
+#define COVMARK_HIT(covmark) \
+    ::stellar::gCovMarks.hit(::stellar::CovMark::covmark)
+
+#define COVMARK_CHECK_HIT_IN_CURR_SCOPE(covmark) \
+    ::stellar::CovMarkGuard _covMarkGuard_##covmark( \
+        ::stellar::CovMark::covmark, __FILE__, __LINE__, #covmark)
+
+#else // !BUILD_TESTS
+
+#define COVMARK_HIT(covmark) ((void)0)
+#define COVMARK_CHECK_HIT_IN_CURR_SCOPE(covmark) ((void)0)
+
+#endif // BUILD_TESTS
+}