Fix cache size calculation for supporting TXs (#7755)

Co-authored-by: Amaury Chamayou <amchamay@microsoft.com>
Co-authored-by: Amaury Chamayou <amaury@xargs.fr>
Co-authored-by: Eddy Ashton <edashton@microsoft.com>

Author: 3 people

CHANGELOG.md

@@ -16,7 +16,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ### Fixed
 
-- Fixed the Turin SEV-SNP CPUID mapping used for product detection. (#7748)
+- Fixed the Turin SEV-SNP CPUID mapping used for product detection (#7748).
+- Fixed cache size calculations for historical queries, resolving a bug where signature transactions could become orphaned and fill the cache's useful space, resulting in incoming user-requested stores being immediately evicted (#7755).
 
 ## [7.0.0-dev12]
 

CMakeLists.txt

@@ -1265,6 +1265,11 @@ if(BUILD_TESTS)
       NAME e2e_curl PYTHON_SCRIPT ${CMAKE_SOURCE_DIR}/tests/e2e_curl.py
     )
 
+    add_e2e_test(
+      NAME historical_query_cache_test
+      PYTHON_SCRIPT ${CMAKE_SOURCE_DIR}/tests/historical_query_cache.py
+    )
+
     add_e2e_test(
       NAME consistency_trace_validation
       PYTHON_SCRIPT ${CMAKE_SOURCE_DIR}/tests/consistency_trace_validation.py

doc/schemas/node_openapi.json

@@ -234,6 +234,17 @@
         ],
         "type": "object"
       },
+      "GetHistoricalCacheInfo__Out": {
+        "properties": {
+          "estimated_size": {
+            "$ref": "#/components/schemas/uint64"
+          }
+        },
+        "required": [
+          "estimated_size"
+        ],
+        "type": "object"
+      },
       "GetNetworkInfo__Out": {
         "properties": {
           "current_service_create_txid": {
@@ -918,7 +929,7 @@
   "info": {
     "description": "This API provides public, uncredentialed access to service and node state.",
     "title": "CCF Public Node API",
-    "version": "5.0.3"
+    "version": "5.0.4"
   },
   "openapi": "3.0.0",
   "paths": {
@@ -1097,6 +1108,29 @@
         }
       }
     },
+    "/node/historical_cache": {
+      "get": {
+        "operationId": "GetNodeHistoricalCache",
+        "responses": {
+          "200": {
+            "content": {
+              "application/json": {
+                "schema": {
+                  "$ref": "#/components/schemas/GetHistoricalCacheInfo__Out"
+                }
+              }
+            },
+            "description": "Default response description"
+          },
+          "default": {
+            "$ref": "#/components/responses/default"
+          }
+        },
+        "x-ccf-forwarding": {
+          "$ref": "#/components/x-ccf-forwarding/sometimes"
+        }
+      }
+    },
     "/node/index/strategies": {
       "get": {
         "operationId": "GetNodeIndexStrategies",

include/ccf/historical_queries_interface.h

@@ -196,5 +196,9 @@ namespace ccf::historical
      * more aggressively than waiting for the states to expire.
      */
     virtual bool drop_cached_states(RequestHandle handle) = 0;
+
+    /** Get the estimated size in bytes of all cached stores.
+     */
+    virtual size_t get_estimated_store_cache_size() = 0;
   };
 }

src/node/historical_queries.h

@@ -21,6 +21,7 @@
 
 #ifdef ENABLE_HISTORICAL_VERBOSE_LOGGING
 #  define HISTORICAL_LOG(...) LOG_INFO_FMT(__VA_ARGS__)
+#  include <ranges>
 #else
 #  define HISTORICAL_LOG(...)
 #endif
@@ -63,7 +64,6 @@ FMT_END_NAMESPACE
 namespace ccf::historical
 {
   static constexpr auto slow_fetch_threshold = std::chrono::milliseconds(1000);
-  static constexpr size_t soft_to_raw_ratio{5};
 
   static std::optional<ccf::PrimarySignature> get_signature(
     const ccf::kv::StorePtr& sig_store)
@@ -269,9 +269,17 @@ namespace ccf::historical
             else if (prev_it != my_stores.end() && *new_it > prev_it->first)
             {
               // No longer looking for a seqno which was previously requested.
-              // Remove it from my_stores
-              removed.push_back(prev_it->first);
-              prev_it = my_stores.erase(prev_it);
+              if (
+                supporting_signatures.find(prev_it->first) ==
+                supporting_signatures.end())
+              {
+                removed.push_back(prev_it->first);
+                prev_it = my_stores.erase(prev_it);
+              }
+              else
+              {
+                ++prev_it;
+              }
             }
             else
             {
@@ -307,15 +315,31 @@ namespace ccf::historical
           if (prev_it != my_stores.end())
           {
             // If we have a suffix of seqnos previously requested, now
-            // unrequested, purge them
-            for (auto it = prev_it; it != my_stores.end(); ++it)
+            // unrequested, purge them - but keep supporting signature entries
+            auto it = prev_it;
+            while (it != my_stores.end())
             {
-              removed.push_back(it->first);
+              if (
+                supporting_signatures.find(it->first) ==
+                supporting_signatures.end())
+              {
+                removed.push_back(it->first);
+                it = my_stores.erase(it);
+              }
+              else
+              {
+                ++it;
+              }
             }
-            my_stores.erase(prev_it, my_stores.end());
           }
         }
 
+        HISTORICAL_LOG(
+          "Added seqnos: {}, removed seqnos: {}, supporting signatures: {}",
+          fmt::join(added, ","),
+          fmt::join(removed, ","),
+          fmt::join(std::views::keys(supporting_signatures), ","));
+
         const bool any_diff = !removed.empty() || !added.empty();
 
         if (!any_diff && (should_include_receipts == include_receipts))
@@ -338,13 +362,36 @@ namespace ccf::historical
 
           for (auto seqno : new_seqnos)
           {
-            populate_receipts(seqno);
+            auto more_to_add = populate_receipts(seqno);
+            std::sort(added.begin(), added.end());
+            std::sort(more_to_add.begin(), more_to_add.end());
+
+            std::vector<SeqNo> together;
+            std::merge(
+              added.begin(),
+              added.end(),
+              more_to_add.begin(),
+              more_to_add.end(),
+              std::back_inserter(together));
+
+            if (more_to_add.size() + added.size() != together.size())
+            {
+              LOG_FAIL_FMT(
+                "Invariant violation in adjust_ranges: more_to_add({}) + "
+                "added({}) != together({})",
+                more_to_add.size(),
+                added.size(),
+                together.size());
+              assert(false);
+            }
+
+            std::swap(added, together);
           }
         }
         return {removed, added};
       }
 
-      void populate_receipts(ccf::SeqNo new_seqno)
+      std::vector<ccf::SeqNo> populate_receipts(ccf::SeqNo new_seqno)
       {
         HISTORICAL_LOG(
           "Looking at {}, and populating receipts from it", new_seqno);
@@ -372,6 +419,16 @@ namespace ccf::historical
             HISTORICAL_LOG("{} is not a signature", new_seqno);
             supporting_signatures.erase(new_seqno);
 
+            if (new_details->receipt != nullptr)
+            {
+              HISTORICAL_LOG(
+                "Already have a receipt for {}, so no need to populate more",
+                new_seqno);
+              return {};
+            }
+
+            std::vector<SeqNo> added;
+
             auto next_seqno = new_seqno + 1;
             while (true)
             {
@@ -383,6 +440,18 @@ namespace ccf::historical
                 HISTORICAL_LOG(
                   "Looking for new supporting signature at {}", next_seqno);
                 details = std::make_shared<StoreDetails>();
+                auto my_it = my_stores.find(next_seqno);
+                if (my_it == my_stores.end())
+                {
+                  LOG_TRACE_FMT(
+                    "Tracking potential supporting signature for new seqno {} "
+                    "at {}",
+                    new_seqno,
+                    next_seqno);
+                  added.push_back(next_seqno);
+                  my_stores.insert_or_assign(my_it, next_seqno, details);
+                }
+
                 all_stores.insert_or_assign(all_it, next_seqno, details);
               }
 
@@ -396,7 +465,7 @@ namespace ccf::historical
                   next_seqno,
                   new_seqno);
                 supporting_signatures[next_seqno] = details;
-                return;
+                return added;
               }
 
               if (details->is_signature)
@@ -415,15 +484,18 @@ namespace ccf::historical
                     new_seqno));
                 }
 
-                return;
+                return added;
               }
 
               // This is a normal transaction, and its already fetched.
               // Nothing to do, consider the next.
               ++next_seqno;
             }
+
+            return added;
           }
         }
+        return {};
       }
 
     private:
@@ -525,8 +597,6 @@ namespace ccf::historical
     std::unordered_map<ccf::SeqNo, size_t> raw_store_sizes;
 
     CacheSize soft_store_cache_limit{std::numeric_limits<size_t>::max()};
-    CacheSize soft_store_cache_limit_raw =
-      soft_store_cache_limit / soft_to_raw_ratio;
     CacheSize estimated_store_cache_size{0};
 
     void add_request_ref(SeqNo seq, CompoundHandle handle)
@@ -814,7 +884,11 @@ namespace ccf::historical
                request.supporting_signatures.end());
           if (seqno_in_this_request)
           {
-            request.populate_receipts(seqno);
+            auto added = request.populate_receipts(seqno);
+            for (auto seq : added)
+            {
+              add_request_ref(seq, handle);
+            }
           }
         }
 
@@ -1158,7 +1232,6 @@ namespace ccf::historical
     void set_soft_cache_limit(CacheSize cache_limit)
     {
       soft_store_cache_limit = cache_limit;
-      soft_store_cache_limit_raw = soft_store_cache_limit / soft_to_raw_ratio;
     }
 
     void track_deletes_on_missing_keys(bool track)
@@ -1450,7 +1523,7 @@ namespace ccf::historical
         }
       }
 
-      lru_shrink_to_fit(soft_store_cache_limit_raw);
+      lru_shrink_to_fit(soft_store_cache_limit);
 
       {
         auto it = all_stores.begin();
@@ -1648,5 +1721,10 @@ namespace ccf::historical
     {
       return StateCacheImpl::drop_cached_states(make_compound_handle(handle));
     }
+
+    size_t get_estimated_store_cache_size() override
+    {
+      return StateCacheImpl::get_estimated_store_cache_size();
+    }
   };
 }

src/node/rpc/node_frontend.h

@@ -93,6 +93,19 @@ namespace ccf
   DECLARE_JSON_TYPE(NodeMetrics);
   DECLARE_JSON_REQUIRED_FIELDS(NodeMetrics, sessions);
 
+  struct GetHistoricalCacheInfo
+  {
+    using In = void;
+
+    struct Out
+    {
+      size_t estimated_size;
+    };
+  };
+
+  DECLARE_JSON_TYPE(GetHistoricalCacheInfo::Out);
+  DECLARE_JSON_REQUIRED_FIELDS(GetHistoricalCacheInfo::Out, estimated_size);
+
   struct JavaScriptMetrics
   {
     uint64_t bytecode_size = 0;
@@ -420,7 +433,7 @@ namespace ccf
       openapi_info.description =
         "This API provides public, uncredentialed access to service and node "
         "state.";
-      openapi_info.document_version = "5.0.3";
+      openapi_info.document_version = "5.0.4";
     }
 
     void init_handlers() override
@@ -1847,6 +1860,22 @@ namespace ccf
       ccf::node::init_recovery_decision_protocol_handlers(*this, context);
 
       ccf::node::init_file_serving_handlers(*this, context);
+
+      auto historical_cache_info = [this](
+                                     [[maybe_unused]] auto& args,
+                                     [[maybe_unused]] nlohmann::json&&) {
+        GetHistoricalCacheInfo::Out result{};
+        result.estimated_size =
+          this->context.get_historical_state().get_estimated_store_cache_size();
+        return make_success(result);
+      };
+      make_read_only_endpoint(
+        "/historical_cache",
+        HTTP_GET,
+        json_read_only_adapter(historical_cache_info),
+        no_auth_required)
+        .set_auto_schema<GetHistoricalCacheInfo>()
+        .install();
     }
   };
 

src/node/rpc/test/node_stub.h

@@ -268,6 +268,11 @@ namespace ccf
     {
       return true;
     }
+
+    size_t get_estimated_store_cache_size()
+    {
+      return 0;
+    }
   };
 
   struct StubNodeContext : public ccf::AbstractNodeContext