Fix issues with snapshot generation for CHAMP

.daily_canary

@@ -1,4 +1,4 @@
    ___     ___
-  (- *)   (O o)   |     Y   D  O
+  (- *)   (O o)   |     Y   ^  O
  (  V  ) <  V  >  O    +---'---'
 /--x-m- /--m-m---xXx--/--yy---

CHANGELOG.md

@@ -29,6 +29,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### Fixed
 
 - Session consistency is now provided even across elections. If session consistency would be broken, the inconsistent request will return an error and the TLS session will be terminated.
+- Fixed issue where invalid snapshots could be generated depending on the pattern of additions/removals of keys in a given key-value map (#4730).
 
 ## [4.0.0-dev0]
 

include/ccf/ds/logger.h

@@ -271,6 +271,7 @@ namespace logger
 
     static inline void default_init()
     {
+      get_loggers().clear();
       add_text_console_logger();
     }
 #endif

src/ds/champ_map.h

@@ -126,8 +126,10 @@ namespace champ
         const auto& entry = bin[i];
         if (k == entry->key)
         {
+          const auto diff = map::get_serialized_size_with_padding(entry->key) +
+            map::get_serialized_size_with_padding(entry->value);
           bin[i] = std::make_shared<Entry<K, V>>(k, v);
-          return map::get_size(k) + map::get_size(v);
+          return diff;
         }
       }
       bin.push_back(std::make_shared<Entry<K, V>>(k, v));
@@ -143,8 +145,8 @@ namespace champ
         const auto& entry = bin[i];
         if (k == entry->key)
         {
-          const auto diff =
-            map::get_size(entry->key) + map::get_size(entry->value);
+          const auto diff = map::get_serialized_size_with_padding(entry->key) +
+            map::get_serialized_size_with_padding(entry->value);
           bin.erase(bin.begin() + i);
           return diff;
         }
@@ -211,6 +213,7 @@ namespace champ
       return node_as<SubNodes<K, V, H>>(c_idx)->getp(depth + 1, hash, k);
     }
 
+    // Returns serialised size of overwritten (k,v) if k exists, 0 otherwise
     size_t put_mut(SmallIndex depth, Hash hash, const K& k, const V& v)
     {
       const auto idx = mask(hash, depth);
@@ -246,8 +249,8 @@ namespace champ
       const auto& entry0 = node_as<Entry<K, V>>(c_idx);
       if (k == entry0->key)
       {
-        auto current_size = map::get_size_with_padding(entry0->key) +
-          map::get_size_with_padding(entry0->value);
+        auto current_size = map::get_serialized_size_with_padding(entry0->key) +
+          map::get_serialized_size_with_padding(entry0->value);
         nodes[c_idx] = std::make_shared<Entry<K, V>>(k, v);
         return current_size;
       }
@@ -297,6 +300,7 @@ namespace champ
         std::make_shared<SubNodes<K, V, H>>(std::move(node)), r);
     }
 
+    // Returns serialised size of removed (k,v) if k exists, 0 otherwise
     size_t remove_mut(SmallIndex depth, Hash hash, const K& k)
     {
       const auto idx = mask(hash, depth);
@@ -311,8 +315,8 @@ namespace champ
         if (entry->key != k)
           return 0;
 
-        const auto diff = map::get_size_with_padding(entry->key) +
-          map::get_size_with_padding(entry->value);
+        const auto diff = map::get_serialized_size_with_padding(entry->key) +
+          map::get_serialized_size_with_padding(entry->value);
         nodes.erase(nodes.begin() + c_idx);
         data_map = data_map.clear(idx);
         return diff;
@@ -437,9 +441,10 @@ namespace champ
       if (r.second == 0)
         size_++;
 
-      const auto size_change =
-        (map::get_size_with_padding(key) + map::get_size_with_padding(value)) -
+      const auto size_change = (map::get_serialized_size_with_padding(key) +
+                                map::get_serialized_size_with_padding(value)) -
         r.second;
+
       return Map(std::move(r.first), size_, size_change + serialized_size);
     }
 
@@ -492,17 +497,14 @@ namespace champ
     {
       std::vector<KVTuple> ordered_state;
       ordered_state.reserve(map.size());
-      size_t size = 0;
+      size_t serialized_size = 0;
 
-      map.foreach([&](auto& key, auto& value) {
+      map.foreach([&ordered_state, &serialized_size](auto& key, auto& value) {
         K* k = &key;
         V* v = &value;
-        uint32_t ks = map::get_size(key);
-        uint32_t vs = map::get_size(value);
-        uint32_t key_size = ks + map::get_padding(ks);
-        uint32_t value_size = vs + map::get_padding(vs);
-
-        size += (key_size + value_size);
+        uint32_t key_size = map::get_serialized_size_with_padding(key);
+        uint32_t value_size = map::get_serialized_size_with_padding(value);
+        serialized_size += (key_size + value_size);
 
         ordered_state.emplace_back(k, static_cast<Hash>(H()(key)), v);
 
@@ -517,25 +519,30 @@ namespace champ
         });
 
       CCF_ASSERT_FMT(
-        size == map.get_serialized_size(),
-        "size:{}, map->size:{} ==> count:{}, vect:{}",
-        size,
+        serialized_size == map.get_serialized_size(),
+        "Serialized size:{}, map.get_serialized_size():{} (map count:{}, "
+        "ordered state count:{})",
+        serialized_size,
         map.get_serialized_size(),
         map.size(),
         ordered_state.size());
 
       for (const auto& p : ordered_state)
       {
         // Serialize the key
-        uint32_t key_size = map::serialize(*p.k, data, size);
-        map::add_padding(key_size, data, size);
+        uint32_t key_size = map::serialize(*p.k, data, serialized_size);
+        map::add_padding(key_size, data, serialized_size);
 
         // Serialize the value
-        uint32_t value_size = map::serialize(*p.v, data, size);
-        map::add_padding(value_size, data, size);
+        uint32_t value_size = map::serialize(*p.v, data, serialized_size);
+        map::add_padding(value_size, data, serialized_size);
       }
 
-      CCF_ASSERT_FMT(size == 0, "buffer not filled, remaining:{}", size);
+      CCF_ASSERT_FMT(
+        serialized_size == 0,
+        "Serialization buffer is not complete, remaining:{}/{}",
+        serialized_size,
+        map.get_serialized_size());
     }
   };
 

src/ds/map_serializers.h

@@ -38,7 +38,7 @@ namespace map
   }
 
   template <class T>
-  static size_t get_size_with_padding(const T& t)
+  static size_t get_serialized_size_with_padding(const T& t)
   {
     const uint32_t t_size = get_size(t);
     return t_size + get_padding(t_size);

src/ds/rb_map.h

@@ -38,8 +38,8 @@ namespace rb
         _rgt(rgt)
       {
         total_size = 1;
-        total_serialized_size =
-          map::get_size_with_padding(key) + map::get_size_with_padding(val);
+        total_serialized_size = map::get_serialized_size_with_padding(key) +
+          map::get_serialized_size_with_padding(val);
         if (lft)
         {
           total_size += lft->size();