KS-199: Update node signer type

.changeset/two-papayas-begin.md

@@ -0,0 +1,5 @@
+---
+"chainlink": patch
+---
+
+#internal update node signer type

contracts/.changeset/sour-parents-explode.md

@@ -0,0 +1,5 @@
+---
+"@chainlink/contracts": patch
+---
+
+update node signer type

contracts/src/v0.8/keystone/CapabilityRegistry.sol

@@ -26,7 +26,7 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
     /// @notice The id of the node operator that manages this node
     uint32 nodeOperatorId;
     /// @notice The signer address for application-layer message verification.
-    address signer;
+    bytes32 signer;
     /// @notice This is an Ed25519 public key that is used to identify a node.
     /// This key is guaranteed to be unique in the CapabilityRegistry. It is
     /// used to identify a node in the the P2P network.
@@ -42,7 +42,13 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
     /// @notice The number of times the node's capability has been updated
     uint32 configCount;
     /// @notice The signer address for application-layer message verification.
-    address signer;
+    /// @dev This key is guaranteed to be unique in the CapabilityRegistry
+    /// as a signer address can only belong to one node.
+    /// @dev This should be the ABI encoded version of the node's address.
+    /// I.e 0x0000address.  The Capability Registry does not store it as an address so that
+    /// non EVM chains with addresses greater than 20 bytes can be supported
+    /// in the future.
+    bytes32 signer;
     /// @notice This is an Ed25519 public key that is used to identify a node.
     /// This key is guaranteed to be unique in the CapabilityRegistry. It is
     /// used to identify a node in the the P2P network.
@@ -158,7 +164,8 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
   /// @notice This event is emitted when a new node is added
   /// @param p2pId The P2P ID of the node
   /// @param nodeOperatorId The ID of the node operator that manages this node
-  event NodeAdded(bytes32 p2pId, uint256 nodeOperatorId);
+  /// @param signer The encoded node's signer address
+  event NodeAdded(bytes32 p2pId, uint256 nodeOperatorId, bytes32 signer);
 
   /// @notice This event is emitted when a node is removed
   /// @param p2pId The P2P ID of the node that was removed
@@ -168,15 +175,16 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
   /// @param p2pId The P2P ID of the node
   /// @param nodeOperatorId The ID of the node operator that manages this node
   /// @param signer The node's signer address
-  event NodeUpdated(bytes32 p2pId, uint256 nodeOperatorId, address signer);
+  event NodeUpdated(bytes32 p2pId, uint256 nodeOperatorId, bytes32 signer);
 
   /// @notice This event is emitted when a new DON is created
   /// @param donId The ID of the newly created DON
   /// @param isPublic True if the newly created DON is public
   event DONAdded(uint256 donId, bool isPublic);
 
   /// @notice This error is thrown when trying to set the node's
-  /// signer address to zero
+  /// signer address to zero or if the signer address has already
+  /// been used by another node
   error InvalidNodeSigner();
 
   /// @notice This error is thrown when trying add a capability that already
@@ -251,6 +259,9 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
   /// Deprecated capabilities are skipped by the `getCapabilities` function.
   EnumerableSet.Bytes32Set private s_deprecatedHashedCapabilityIds;
 
+  /// @notice Encoded node signer addresses
+  EnumerableSet.Bytes32Set private s_encodedNodeSignerAddresses;
+
   /// @notice Mapping of node operators
   mapping(uint256 nodeOperatorId => NodeOperator nodeOperator) private s_nodeOperators;
 
@@ -338,10 +349,11 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
       NodeOperator memory nodeOperator = s_nodeOperators[node.nodeOperatorId];
       if (!isOwner && msg.sender != nodeOperator.admin) revert AccessForbidden();
 
-      bool nodeExists = s_nodes[node.p2pId].signer != address(0);
+      bool nodeExists = bytes32(s_nodes[node.p2pId].signer) != bytes32("");
       if (nodeExists || bytes32(node.p2pId) == bytes32("")) revert InvalidNodeP2PId(node.p2pId);
 
-      if (node.signer == address(0)) revert InvalidNodeSigner();
+      if (bytes32(node.signer) == bytes32("") || s_encodedNodeSignerAddresses.contains(node.signer))
+        revert InvalidNodeSigner();
 
       bytes32[] memory capabilityIds = node.hashedCapabilityIds;
       if (capabilityIds.length == 0) revert InvalidNodeCapabilities(capabilityIds);
@@ -357,7 +369,8 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
       s_nodes[node.p2pId].nodeOperatorId = node.nodeOperatorId;
       s_nodes[node.p2pId].p2pId = node.p2pId;
       s_nodes[node.p2pId].signer = node.signer;
-      emit NodeAdded(node.p2pId, node.nodeOperatorId);
+      s_encodedNodeSignerAddresses.add(node.signer);
+      emit NodeAdded(node.p2pId, node.nodeOperatorId, node.signer);
     }
   }
 
@@ -369,12 +382,13 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
     for (uint256 i; i < removedNodeP2PIds.length; ++i) {
       bytes32 p2pId = removedNodeP2PIds[i];
 
-      bool nodeExists = s_nodes[p2pId].signer != address(0);
+      bool nodeExists = bytes32(s_nodes[p2pId].signer) != bytes32("");
       if (!nodeExists) revert InvalidNodeP2PId(p2pId);
 
       NodeOperator memory nodeOperator = s_nodeOperators[s_nodes[p2pId].nodeOperatorId];
 
       if (!isOwner && msg.sender != nodeOperator.admin) revert AccessForbidden();
+      s_encodedNodeSignerAddresses.remove(s_nodes[p2pId].signer);
       delete s_nodes[p2pId];
       emit NodeRemoved(p2pId);
     }
@@ -392,10 +406,13 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
       NodeOperator memory nodeOperator = s_nodeOperators[node.nodeOperatorId];
       if (!isOwner && msg.sender != nodeOperator.admin) revert AccessForbidden();
 
-      bool nodeExists = s_nodes[node.p2pId].signer != address(0);
+      bool nodeExists = bytes32(s_nodes[node.p2pId].signer) != bytes32("");
       if (!nodeExists) revert InvalidNodeP2PId(node.p2pId);
 
-      if (node.signer == address(0)) revert InvalidNodeSigner();
+      if (
+        bytes32(node.signer) == bytes32("") ||
+        (s_nodes[node.p2pId].signer != node.signer && s_encodedNodeSignerAddresses.contains(node.signer))
+      ) revert InvalidNodeSigner();
 
       bytes32[] memory supportedCapabilityIds = node.hashedCapabilityIds;
       if (supportedCapabilityIds.length == 0) revert InvalidNodeCapabilities(supportedCapabilityIds);
@@ -410,7 +427,14 @@ contract CapabilityRegistry is OwnerIsCreator, TypeAndVersionInterface {
 
       s_nodes[node.p2pId].nodeOperatorId = node.nodeOperatorId;
       s_nodes[node.p2pId].p2pId = node.p2pId;
-      s_nodes[node.p2pId].signer = node.signer;
+
+      bytes32 previousSigner = s_nodes[node.p2pId].signer;
+
+      if (s_nodes[node.p2pId].signer != node.signer) {
+        s_encodedNodeSignerAddresses.remove(previousSigner);
+        s_nodes[node.p2pId].signer = node.signer;
+        s_encodedNodeSignerAddresses.add(node.signer);
+      }
       emit NodeUpdated(node.p2pId, node.nodeOperatorId, node.signer);
     }
   }

contracts/src/v0.8/keystone/test/CapabilityRegistry_AddNodesTest.t.sol

@@ -5,10 +5,10 @@ import {BaseTest} from "./BaseTest.t.sol";
 import {CapabilityRegistry} from "../CapabilityRegistry.sol";
 
 contract CapabilityRegistry_AddNodesTest is BaseTest {
-  event NodeAdded(bytes32 p2pId, uint256 nodeOperatorId);
+  event NodeAdded(bytes32 p2pId, uint256 nodeOperatorId, bytes32 signer);
 
   uint32 private constant TEST_NODE_OPERATOR_ONE_ID = 0;
-  uint256 private constant TEST_NODE_OPERATOR_TWO_ID = 1;
+  uint32 private constant TEST_NODE_OPERATOR_TWO_ID = 1;
 
   function setUp() public override {
     BaseTest.setUp();
@@ -46,14 +46,43 @@ contract CapabilityRegistry_AddNodesTest is BaseTest {
     nodes[0] = CapabilityRegistry.NodeParams({
       nodeOperatorId: TEST_NODE_OPERATOR_ONE_ID,
       p2pId: P2P_ID,
-      signer: address(0),
+      signer: bytes32(""),
       hashedCapabilityIds: hashedCapabilityIds
     });
 
     vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.InvalidNodeSigner.selector));
     s_capabilityRegistry.addNodes(nodes);
   }
 
+  function test_RevertWhen_SignerAddressNotUnique() public {
+    changePrank(NODE_OPERATOR_ONE_ADMIN);
+    CapabilityRegistry.NodeParams[] memory nodes = new CapabilityRegistry.NodeParams[](1);
+
+    bytes32[] memory hashedCapabilityIds = new bytes32[](1);
+    hashedCapabilityIds[0] = s_basicHashedCapabilityId;
+
+    nodes[0] = CapabilityRegistry.NodeParams({
+      nodeOperatorId: TEST_NODE_OPERATOR_ONE_ID,
+      p2pId: P2P_ID,
+      signer: NODE_OPERATOR_ONE_SIGNER_ADDRESS,
+      hashedCapabilityIds: hashedCapabilityIds
+    });
+
+    s_capabilityRegistry.addNodes(nodes);
+
+    changePrank(NODE_OPERATOR_TWO_ADMIN);
+
+    // Try adding another node with the same signer address
+    nodes[0] = CapabilityRegistry.NodeParams({
+      nodeOperatorId: TEST_NODE_OPERATOR_TWO_ID,
+      p2pId: P2P_ID_TWO,
+      signer: NODE_OPERATOR_ONE_SIGNER_ADDRESS,
+      hashedCapabilityIds: hashedCapabilityIds
+    });
+    vm.expectRevert(abi.encodeWithSelector(CapabilityRegistry.InvalidNodeSigner.selector));
+    s_capabilityRegistry.addNodes(nodes);
+  }
+
   function test_RevertWhen_AddingDuplicateP2PId() public {
     changePrank(NODE_OPERATOR_ONE_ADMIN);
     CapabilityRegistry.NodeParams[] memory nodes = new CapabilityRegistry.NodeParams[](1);
@@ -143,7 +172,7 @@ contract CapabilityRegistry_AddNodesTest is BaseTest {
     });
 
     vm.expectEmit(address(s_capabilityRegistry));
-    emit NodeAdded(P2P_ID, TEST_NODE_OPERATOR_ONE_ID);
+    emit NodeAdded(P2P_ID, TEST_NODE_OPERATOR_ONE_ID, NODE_OPERATOR_ONE_SIGNER_ADDRESS);
     s_capabilityRegistry.addNodes(nodes);
 
     (CapabilityRegistry.NodeParams memory node, uint32 configCount) = s_capabilityRegistry.getNode(P2P_ID);
@@ -171,7 +200,7 @@ contract CapabilityRegistry_AddNodesTest is BaseTest {
     });
 
     vm.expectEmit(address(s_capabilityRegistry));
-    emit NodeAdded(P2P_ID, TEST_NODE_OPERATOR_ONE_ID);
+    emit NodeAdded(P2P_ID, TEST_NODE_OPERATOR_ONE_ID, NODE_OPERATOR_ONE_SIGNER_ADDRESS);
     s_capabilityRegistry.addNodes(nodes);
 
     (CapabilityRegistry.NodeParams memory node, uint32 configCount) = s_capabilityRegistry.getNode(P2P_ID);

contracts/src/v0.8/keystone/test/CapabilityRegistry_RemoveNodesTest.t.sol

@@ -75,11 +75,42 @@ contract CapabilityRegistry_RemoveNodesTest is BaseTest {
     (CapabilityRegistry.NodeParams memory node, uint32 configCount) = s_capabilityRegistry.getNode(P2P_ID);
     assertEq(node.nodeOperatorId, 0);
     assertEq(node.p2pId, bytes32(""));
-    assertEq(node.signer, address(0));
+    assertEq(node.signer, bytes32(""));
     assertEq(node.hashedCapabilityIds.length, 0);
     assertEq(configCount, 0);
   }
 
+  function test_CanAddNodeWithSameSignerAddressAfterRemoving() public {
+    changePrank(NODE_OPERATOR_ONE_ADMIN);
+
+    bytes32[] memory nodes = new bytes32[](1);
+    nodes[0] = P2P_ID;
+
+    s_capabilityRegistry.removeNodes(nodes);
+
+    CapabilityRegistry.NodeParams[] memory nodeParams = new CapabilityRegistry.NodeParams[](1);
+    bytes32[] memory hashedCapabilityIds = new bytes32[](2);
+    hashedCapabilityIds[0] = s_basicHashedCapabilityId;
+    hashedCapabilityIds[1] = s_capabilityWithConfigurationContractId;
+
+    nodeParams[0] = CapabilityRegistry.NodeParams({
+      nodeOperatorId: TEST_NODE_OPERATOR_ONE_ID,
+      p2pId: P2P_ID,
+      signer: NODE_OPERATOR_ONE_SIGNER_ADDRESS,
+      hashedCapabilityIds: hashedCapabilityIds
+    });
+
+    s_capabilityRegistry.addNodes(nodeParams);
+
+    (CapabilityRegistry.NodeParams memory node, uint32 configCount) = s_capabilityRegistry.getNode(P2P_ID);
+    assertEq(node.nodeOperatorId, TEST_NODE_OPERATOR_ONE_ID);
+    assertEq(node.p2pId, P2P_ID);
+    assertEq(node.hashedCapabilityIds.length, 2);
+    assertEq(node.hashedCapabilityIds[0], s_basicHashedCapabilityId);
+    assertEq(node.hashedCapabilityIds[1], s_capabilityWithConfigurationContractId);
+    assertEq(configCount, 1);
+  }
+
   function test_OwnerCanRemoveNodes() public {
     changePrank(ADMIN);
 
@@ -93,7 +124,7 @@ contract CapabilityRegistry_RemoveNodesTest is BaseTest {
     (CapabilityRegistry.NodeParams memory node, uint32 configCount) = s_capabilityRegistry.getNode(P2P_ID);
     assertEq(node.nodeOperatorId, 0);
     assertEq(node.p2pId, bytes32(""));
-    assertEq(node.signer, address(0));
+    assertEq(node.signer, bytes32(""));
     assertEq(node.hashedCapabilityIds.length, 0);
     assertEq(configCount, 0);
   }