Switch BFT to hashed public key node ids (#2892)

* Switch BFT to hashed public keys as node ids

Co-authored-by: Julien Maffre <42961061+jumaffre@users.noreply.github.com>

src/consensus/aft/raft.h

@@ -342,6 +342,7 @@ namespace aft
         consensus_type == ConsensusType::BFT,
         "Computing primary id from view is only supported with BFT consensus");
 
+      assert(configurations.size() > 0);
       const auto& config = configurations.back();
       return get_primary_at_config(view, config.bft_offset, config.nodes);
     }
@@ -1198,6 +1199,7 @@ namespace aft
         consensus_type == ConsensusType::BFT,
         "Computing primary id from view is only supported with BFT consensus");
 
+      assert(conf.size() > 0);
       auto it = conf.begin();
       std::advance(it, (view - starting_view_change + offset) % conf.size());
       return it->first;

src/node/node_state.h

@@ -265,10 +265,7 @@ namespace ccf
       rpcsessions(rpcsessions),
       share_manager(share_manager)
     {
-      if (network.consensus_type == ConsensusType::CFT)
-      {
-        self = crypto::Sha256Hash(node_sign_kp->public_key_der()).hex_str();
-      }
+      self = crypto::Sha256Hash(node_sign_kp->public_key_der()).hex_str();
     }
 
     QuoteVerificationResult verify_quote(
@@ -359,18 +356,6 @@ namespace ccf
 
           network.ledger_secrets->init();
 
-          if (network.consensus_type == ConsensusType::BFT)
-          {
-            // BFT consensus requires a stable order of node IDs so that the
-            // primary node in a given view can be computed deterministically by
-            // all nodes in the network
-            // See https://github.com/microsoft/CCF/issues/1852
-
-            // Pad node id string to avoid memory alignment issues on
-            // node-to-node messages
-            self = NodeId(fmt::format("{:#064}", 0));
-          }
-
           setup_snapshotter();
           setup_encryptor();
           setup_consensus(ServiceStatus::OPENING);
@@ -550,13 +535,6 @@ namespace ccf
                 resp.network_info->consensus_type));
             }
 
-            if (network.consensus_type == ConsensusType::BFT)
-            {
-              // In CFT, the node id is computed at startup, as the hash of the
-              // node's public key
-              self = resp.node_id;
-            }
-
             setup_snapshotter();
             setup_encryptor();
             setup_consensus(
@@ -654,8 +632,7 @@ namespace ccf
           else if (resp.node_status == NodeStatus::PENDING)
           {
             LOG_INFO_FMT(
-              "Node {} is waiting for votes of members to be trusted",
-              resp.node_id);
+              "Node {} is waiting for votes of members to be trusted", self);
           }
 
           return true;
@@ -962,19 +939,6 @@ namespace ccf
         last_recovered_signed_idx);
 
       auto tx = network.tables->create_read_only_tx();
-      if (network.consensus_type == ConsensusType::BFT)
-      {
-        // BFT consensus requires a stable order of node IDs so that the
-        // primary node in a given view can be computed deterministically by
-        // all nodes in the network
-        // See https://github.com/microsoft/CCF/issues/1852
-
-        // Pad node id string to avoid memory alignment issues on
-        // node-to-node messages
-        auto values = tx.ro(network.values);
-        auto id = values->get(0);
-        self = NodeId(fmt::format("{:#064}", id.value()));
-      }
 
       network.ledger_secrets->init(last_recovered_signed_idx + 1);
       setup_encryptor();

src/node/rpc/node_call_types.h

@@ -94,7 +94,7 @@ namespace ccf
     struct Out
     {
       NodeStatus node_status;
-      NodeId node_id; // Only used in BFT
+      std::optional<NodeId> node_id;
 
       struct NetworkInfo
       {

src/node/rpc/node_frontend.h

@@ -141,18 +141,7 @@ namespace ccf
 
       auto pk_der = crypto::public_key_der_from_cert(node_der);
 
-      NodeId joining_node_id;
-      if (network.consensus_type == ConsensusType::CFT)
-      {
-        joining_node_id = crypto::Sha256Hash(pk_der).hex_str();
-      }
-      else
-      {
-        // Pad node id string to avoid memory alignment issues on
-        // node-to-node messages
-        joining_node_id = fmt::format(
-          "{:#064}", get_next_id(tx.rw(this->network.values), NEXT_NODE_ID));
-      }
+      NodeId joining_node_id = crypto::Sha256Hash(pk_der).hex_str();
 
       CodeDigest code_digest;
 
@@ -299,7 +288,6 @@ namespace ccf
           {
             JoinNetworkNodeToNode::Out rep;
             rep.node_status = joining_node_status;
-            rep.node_id = existing_node_info->first;
             rep.network_info = JoinNetworkNodeToNode::Out::NetworkInfo(
               context.get_node_state().is_part_of_public_network(),
               context.get_node_state().get_last_recovered_signed_idx(),
@@ -362,7 +350,6 @@ namespace ccf
         if (existing_node_info.has_value())
         {
           JoinNetworkNodeToNode::Out rep;
-          rep.node_id = existing_node_info->first;
 
           // If the node already exists, return network secrets if is already
           // trusted. Otherwise, only return its status

src/node/rpc/serialization.h

@@ -52,8 +52,9 @@ namespace ccf
   DECLARE_JSON_OPTIONAL_FIELDS(
     JoinNetworkNodeToNode::Out::NetworkInfo, service_status)
   DECLARE_JSON_TYPE_WITH_OPTIONAL_FIELDS(JoinNetworkNodeToNode::Out)
-  DECLARE_JSON_REQUIRED_FIELDS(JoinNetworkNodeToNode::Out, node_status, node_id)
-  DECLARE_JSON_OPTIONAL_FIELDS(JoinNetworkNodeToNode::Out, network_info)
+  DECLARE_JSON_REQUIRED_FIELDS(JoinNetworkNodeToNode::Out, node_status)
+  DECLARE_JSON_OPTIONAL_FIELDS(
+    JoinNetworkNodeToNode::Out, node_id, network_info)
 
   DECLARE_JSON_TYPE(CreateNetworkNodeToNode::In::GenesisInfo)
   DECLARE_JSON_REQUIRED_FIELDS(

src/node/rpc/test/node_frontend_test.cpp

@@ -159,7 +159,8 @@ TEST_CASE("Add a node to an opening service")
     CHECK(response.network_info->identity == *network.identity.get());
     CHECK(response.network_info->public_only == false);
 
-    const NodeId node_id = response.node_id;
+    auto pk_der = kp->public_key_der();
+    const NodeId node_id = crypto::Sha256Hash(pk_der).hex_str();
     auto nodes = tx.rw(network.nodes);
     auto node_info = nodes->get(node_id);
 
@@ -259,8 +260,8 @@ TEST_CASE("Add a node to an open service")
 
     CHECK(!response.network_info.has_value());
 
-    auto node_id = response.node_id;
-
+    auto pk_der = kp->public_key_der();
+    const NodeId node_id = crypto::Sha256Hash(pk_der).hex_str();
     auto nodes = tx.rw(network.nodes);
     node_info = nodes->get(node_id);
     CHECK(node_info.has_value());

tests/infra/node.py

@@ -266,15 +266,10 @@ class Node:
         self.remote.get_startup_files(self.common_dir)
         self.consensus = kwargs.get("consensus")
 
-        if self.consensus == "cft":
-            with open(os.path.join(self.common_dir, f"{self.local_node_id}.pem")) as f:
-                self.node_id = infra.crypto.compute_public_key_der_hash_hex_from_pem(
-                    f.read()
-                )
-        else:
-            # BFT consensus should deterministically compute the primary id from the
-            # consensus view, so node ids are monotonic in this case
-            self.node_id = "{:0>64}".format(self.local_node_id)
+        with open(os.path.join(self.common_dir, f"{self.local_node_id}.pem")) as f:
+            self.node_id = infra.crypto.compute_public_key_der_hash_hex_from_pem(
+                f.read()
+            )
 
         self._read_ports()
         LOG.info(f"Node {self.local_node_id} started: {self.node_id}")