Retired committed hook receives whole config (microsoft#6018)

eddyashton · Feb 13, 2024 · 74326ef · 74326ef
1 parent 35befc7
commit 74326ef
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 34 deletions.
diff --git a/src/consensus/aft/raft.h b/src/consensus/aft/raft.h
@@ -296,16 +296,29 @@ namespace aft
       return state->membership_state == kv::MembershipState::Retired;
     }
 
-    void set_retired_committed(ccf::SeqNo seqno) override
-    {
-      state->retirement_phase = kv::RetirementPhase::RetiredCommitted;
-      CCF_ASSERT_FMT(
-        state->retired_committed_idx == state->commit_idx,
-        "Retired "
-        "committed index {} does not match current commit index {}",
-        state->retired_committed_idx.value_or(0),
-        state->commit_idx);
-      state->retired_committed_idx = seqno;
+    void set_retired_committed(
+      ccf::SeqNo seqno, const std::vector<kv::NodeId>& node_ids) override
+    {
+      for (auto& node_id : node_ids)
+      {
+        if (id() == node_id)
+        {
+          CCF_ASSERT(
+            state->membership_state == kv::MembershipState::Retired,
+            "Node is not retired, cannot become retired committed");
+          CCF_ASSERT(
+            state->retirement_phase == kv::RetirementPhase::Completed,
+            "Node is not retired, cannot become retired committed");
+          CCF_ASSERT_FMT(
+            state->retired_committed_idx == state->commit_idx,
+            "Retired "
+            "committed index {} does not match current commit index {}",
+            state->retired_committed_idx.value_or(0),
+            state->commit_idx);
+          state->retirement_phase = kv::RetirementPhase::RetiredCommitted;
+          state->retired_committed_idx = seqno;
+        }
+      }
     }
 
     Index last_committable_index() const

diff --git a/src/consensus/aft/test/driver.h b/src/consensus/aft/test/driver.h
@@ -198,7 +198,9 @@ class RaftDriver
       std::make_shared<aft::State>(node_id),
       nullptr);
     kv->set_set_retired_committed_hook(
-      [&raft](aft::Index idx) { raft->set_retired_committed(idx); });
+      [raft](aft::Index idx, const std::vector<kv::NodeId>& node_ids) {
+        raft->set_retired_committed(idx, node_ids);
+      });
     raft->start_ticking();
 
     if (_nodes.find(node_id) != _nodes.end())

diff --git a/src/consensus/aft/test/logging_stub.h b/src/consensus/aft/test/logging_stub.h
@@ -329,17 +329,19 @@ namespace aft
     }
   };
 
+  using RCHook = std::function<void(Index, const std::vector<kv::NodeId>&)>;
+
   class LoggingStubStore
   {
   protected:
     ccf::NodeId _id;
-    std::function<void(Index)> set_retired_committed_hook;
+    RCHook set_retired_committed_hook;
 
   public:
     LoggingStubStore(ccf::NodeId id) : _id(id) {}
 
     virtual void set_set_retired_committed_hook(
-      std::function<void(Index)> set_retired_committed_hook_)
+      RCHook set_retired_committed_hook_)
     {
       set_retired_committed_hook = set_retired_committed_hook_;
     }
@@ -480,16 +482,12 @@ namespace aft
       {
         if (version <= i)
         {
-          std::cout << "Retired committed configuration: "
-                    << configuration.dump() << std::endl;
-          if (configuration.find(_id) != configuration.end())
+          std::vector<kv::NodeId> retired_committed_node_ids;
+          for (auto& [node_id, _] : configuration.items())
           {
-            std::cout << "Node id: " << _id
-                      << " is in the retired committed configuration, calling "
-                         "set_retired_committed hook"
-                      << std::endl;
-            set_retired_committed_hook(i);
+            retired_committed_node_ids.push_back(node_id);
           }
+          set_retired_committed_hook(i, retired_committed_node_ids);
         }
         else
         {

diff --git a/src/kv/kv_types.h b/src/kv/kv_types.h
@@ -476,7 +476,9 @@ namespace kv
 
     virtual void enable_all_domains() {}
 
-    virtual void set_retired_committed(ccf::SeqNo){};
+    virtual void set_retired_committed(
+      ccf::SeqNo, const std::vector<NodeId>& node_ids)
+    {}
   };
 
   struct PendingTxInfo

diff --git a/src/node/node_state.h b/src/node/node_state.h
@@ -2148,23 +2148,16 @@ namespace ccf
         network.nodes.get_name(),
         network.nodes.wrap_commit_hook(
           [this](kv::Version hook_version, const Nodes::Write& w) {
+            std::vector<NodeId> retired_committed_nodes;
             for (const auto& [node_id, node_info] : w)
             {
-              if (node_id != self)
+              if (node_info.has_value() && node_info->retired_committed)
               {
-                // Only update our own state
-                continue;
-              }
-
-              if (node_info.has_value())
-              {
-                if (node_info->retired_committed)
-                {
-                  consensus->set_retired_committed(hook_version);
-                }
-                return;
+                retired_committed_nodes.push_back(node_id);
               }
             }
+            consensus->set_retired_committed(
+              hook_version, retired_committed_nodes);
           }));
 
       // Service-endorsed certificate is passed to history as early as _local_