Merge pull request #70 from jagerman/set-erase-fix

Fix bug in set erasing causing config dirtying

Author: jagerman

include/session/config/base.hpp

@@ -438,8 +438,13 @@ class ConfigBase : public ConfigSig {
         /// Ouputs:
         /// - `bool` -- True if the value was inserted, false otherwise
         bool insert_if_missing(config::scalar&& value) {
+            // If the config isn't marked dirty then we can do a first pass to see if the value is
+            // already there: if it is, we want to short-circuit marking the config dirty because
+            // this insertion is a no-op.  If the config is *already* dirty then there's no point in
+            // doing this possibly extra check, because even if we don't change anything the config
+            // is *already* dirty.
             if (!_conf.is_dirty())
-                if (auto current = get_clean<config::set>(); current && current->count(value))
+                if (auto current = get_clean<config::set>(); current && current->contains(value))
                     return false;
 
             get_dirty<config::set>().insert(std::move(value));
@@ -456,8 +461,10 @@ class ConfigBase : public ConfigSig {
         /// Outputs:
         /// - `bool` -- True if an element was erased, false otherwise
         bool set_erase_impl(const config::scalar& value) {
+            // See comment in `insert_if_missing`, above.  This is the same short-circuiting logic
+            // (but with a negated condition because this is erasing instead of inserting).
             if (!_conf.is_dirty())
-                if (auto current = get_clean<config::set>(); current && !current->count(value))
+                if (auto current = get_clean<config::set>(); !(current && current->contains(value)))
                     return false;
 
             config::dict* data = &_conf.dirty().data();

src/config/internal.cpp

@@ -149,17 +149,17 @@ std::optional<std::string> maybe_string(const session::config::dict& d, const ch
 
 uint64_t bitset_from_set_of_int64_or_0(const session::config::set& s) {
     uint64_t result = 0;
-    const size_t bits_available = sizeof(result) * 8;
-    for (auto it : s) {
-        auto* val = std::get_if<int64_t>(&it);
+    constexpr size_t bits_available = sizeof(result) * 8;
+    for (auto& v : s) {
+        auto* val = std::get_if<int64_t>(&v);
         if (val && (*val >= 0 && *val < bits_available))
             result |= (1ULL << *val);
     }
     return result;
 }
 
 void set_int64_set_from_bitset(ConfigBase::DictFieldProxy&& field, uint64_t bitset) {
-    const size_t bits_available = sizeof(bitset) * 8;
+    constexpr size_t bits_available = sizeof(bitset) * 8;
     for (size_t index = 0; index < bits_available; index++) {
         uint64_t bit = bitset & (1ULL << index);
         if (bit)

tests/test_config_contacts.cpp

@@ -1124,6 +1124,8 @@ TEST_CASE("Contacts Pro Storage", "[config][contacts][pro]") {
 
     session::config::Contacts contacts{std::span<const unsigned char>{seed}, std::nullopt};
 
+    REQUIRE(contacts.is_clean());
+
     // Set the bitsets onto the profile
     {
         auto c = contacts.get_or_construct(
@@ -1133,6 +1135,8 @@ TEST_CASE("Contacts Pro Storage", "[config][contacts][pro]") {
         c.profile_bitset.set(SESSION_PROTOCOL_PRO_PROFILE_FEATURES_PRO_BADGE);
         contacts.set(c);
 
+        CHECK(contacts.is_dirty());
+
         c = contacts.get_or_construct(
                 "050000000000000000000000000000000000000000000000000000000000000000"sv);
         CHECK(c.profile_bitset.is_set(SESSION_PROTOCOL_PRO_PROFILE_FEATURES_PRO_BADGE));
@@ -1171,4 +1175,29 @@ TEST_CASE("Contacts Pro Storage", "[config][contacts][pro]") {
                 "050000000000000000000000000000000000000000000000000000000000000001"sv);
         CHECK(!c.profile_bitset.is_set(SESSION_PROTOCOL_PRO_PROFILE_FEATURES_PRO_BADGE));
     }
+
+    CHECK(contacts.needs_push());
+    CHECK(contacts.needs_dump());
+
+    auto [seqno, to_push, obs] = contacts.push();
+    // Simulated push:
+    contacts.confirm_pushed(seqno, {"fakehash1"});
+    // Simulated dump:
+    contacts.dump();
+
+    REQUIRE_FALSE(contacts.needs_push());
+    REQUIRE_FALSE(contacts.needs_dump());
+
+    {
+        auto c = contacts.get_or_construct(
+                "050000000000000000000000000000000000000000000000000000000000000000"sv);
+        CHECK(c.profile_bitset.is_set(SESSION_PROTOCOL_PRO_PROFILE_FEATURES_PRO_BADGE));
+        CHECK(c.profile_bitset.is_set(SESSION_PROTOCOL_PRO_PROFILE_FEATURES_ANIMATED_AVATAR));
+        // This previously exposed a bug in set_erase_impl where the contact was being dirtied even
+        // when nothing was actually being removed.
+        contacts.set(c);
+    }
+
+    CHECK_FALSE(contacts.needs_push());
+    CHECK_FALSE(contacts.needs_dump());
 }