From d4ba04d8b795417b2a2ac6c1f18c56edd21e516a Mon Sep 17 00:00:00 2001
From: r4f43l <rafael@hindsightcrypto.com>
Date: Tue, 5 Dec 2023 10:28:56 +0100
Subject: [PATCH] Refactor cosigner handling and add logging

---
 cmd/horcrux/cmd/threshold.go     | 15 ++++++---------
 pkg/pcosigner/cosigner.go        |  6 ++++--
 pkg/pcosigner/remote_cosigner.go | 19 +++++++++++--------
 test/horcrux_test.go             |  2 +-
 4 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/cmd/horcrux/cmd/threshold.go b/cmd/horcrux/cmd/threshold.go
index 79bdd684..23a377f3 100644
--- a/cmd/horcrux/cmd/threshold.go
+++ b/cmd/horcrux/cmd/threshold.go
@@ -25,7 +25,7 @@ func NewThresholdValidator(
 
 	thresholdCfg := config.Config.ThresholdModeConfig
 	// NOTE: Shouldnt this be a list of concrete type instead of interface type?
-	remoteCosigners := make([]pcosigner.IRemoteCosigner, 0, len(thresholdCfg.Cosigners)-1)
+	remoteCosigners := make([]pcosigner.IRemoteCosigner, 0, len(thresholdCfg.Cosigners)-1) // list of remote cosigners
 	// peers := make([]pcosigner.ICosigner, 0, len(thresholdCfg.Cosigners)-1)
 
 	var p2pListen string
@@ -43,15 +43,12 @@ func NewThresholdValidator(
 
 	for _, c := range thresholdCfg.Cosigners {
 		if c.ShardID != security.GetID() {
-
-			// remoteCosigners = append(
-			//	remoteCosigners,
-			//	temp,
-			// )
 			remoteCosigners = append(remoteCosigners, pcosigner.NewRemoteCosigner(security.GetID(), c.P2PAddr))
+			logger.Info("Added remote cosigner", "id", c.ShardID, "address", c.P2PAddr)
 		} else {
-			// p2pListen = c.P2PAddr
+			p2pListen = c.P2PAddr
 			cosign = pcosigner.NewCosign(c.ShardID, c.P2PAddr)
+			logger.Info("Created a new cosigner", "id", c.ShardID, "address", c.P2PAddr)
 
 		}
 	}
@@ -92,8 +89,8 @@ func NewThresholdValidator(
 		thresholdCfg.Threshold,
 		grpcTimeout,
 		maxWaitForSameBlockAttempts,
-		localCosigner,
-		remoteCosigners, // remote Cosigners are the peers we are calling remotely
+		localCosigner,   // our "server"
+		remoteCosigners, // remote Cosigners are the peers we are requesting remotely (client to server)
 		raftStore,       // raftStore implements the ILeader interface
 	)
 
diff --git a/pkg/pcosigner/cosigner.go b/pkg/pcosigner/cosigner.go
index 3251396e..366d5941 100644
--- a/pkg/pcosigner/cosigner.go
+++ b/pkg/pcosigner/cosigner.go
@@ -34,7 +34,9 @@ func NewThresholdSignerSoft(config *RuntimeConfig, id int, chainID string) (*cip
 		pubKey,
 		threshold,
 		total)
-
+	if err != nil {
+		return nil, fmt.Errorf("cipher.NewThresholdSignerSoft error: (%s)", err)
+	}
 	return &s, nil
 }
 
@@ -55,7 +57,7 @@ func (cosign *Cosigner) GetAddress() string {
 
 // GetID returns the ID of the remote cosigner
 // Implements the cosigner interface
-func (cosign *RemoteCosigner) GetID() int {
+func (cosign *Cosigner) GetID() int {
 	return cosign.id
 }
 
diff --git a/pkg/pcosigner/remote_cosigner.go b/pkg/pcosigner/remote_cosigner.go
index 386bb0d6..911e4fdb 100644
--- a/pkg/pcosigner/remote_cosigner.go
+++ b/pkg/pcosigner/remote_cosigner.go
@@ -10,6 +10,7 @@ package pcosigner
 
 import (
 	"context"
+	"fmt"
 	"net/url"
 	"time"
 
@@ -65,31 +66,33 @@ func (cosigner *RemoteCosigner) VerifySignature(_ string, _, _ []byte) bool {
 	return false
 }
 */
-func (cosigner *RemoteCosigner) getGRPCClient() (proto.ICosignerGRPCClient, *grpc.ClientConn, error) {
+// getGRPCClient returns a gRPC client and a connection.
+func (rc *RemoteCosigner) getGRPCClient() (proto.ICosignerGRPCClient, *grpc.ClientConn, error) {
 	var grpcAddress string
-	url, err := url.Parse(cosigner.address)
+	url, err := url.Parse(rc.address)
 	if err != nil {
-		grpcAddress = cosigner.address
+		grpcAddress = rc.address
 	} else {
 		grpcAddress = url.Host
 	}
 	conn, err := grpc.Dial(grpcAddress, grpc.WithTransportCredentials(insecure.NewCredentials()))
 	if err != nil {
+		fmt.Printf("Error grpc.Dial: %v, with grpcAddress = %s\n", err, grpcAddress)
 		return nil, nil, err
 	}
 	return proto.NewICosignerGRPCClient(conn), conn, nil
 }
 
 // GetNonces implements the Cosigner interface
-// It uses the gRPC client to request the nonces from the other
-// Its the client side (Stub) of the gRPC
+// It uses the gRPC client to request the nonces from the specific peer cosigner
+// This is the  client side (Stub) of the gRPC
 // TODO: Change name to DealNonces or RequestNonces
-func (cosigner *RemoteCosigner) GetNonces(
+func (rc *RemoteCosigner) GetNonces(
 	chainID string,
 	req types.HRSTKey,
 ) (*CosignNoncesResponse, error) {
 
-	client, conn, err := cosigner.getGRPCClient()
+	client, conn, err := rc.getGRPCClient()
 	if err != nil {
 		return nil, err
 	}
@@ -106,7 +109,7 @@ func (cosigner *RemoteCosigner) GetNonces(
 	if err != nil {
 		return nil, err
 	}
-	// Returns one nonce from each cosigner
+	// Returns nonce from a cosigner
 	return &CosignNoncesResponse{
 		Nonces: CosignerNoncesFromProto(res.GetNonces()),
 	}, nil
diff --git a/test/horcrux_test.go b/test/horcrux_test.go
index 170e458c..bfd2b692 100644
--- a/test/horcrux_test.go
+++ b/test/horcrux_test.go
@@ -28,7 +28,7 @@ const (
 // Test2Of3SignerOneSentry will spin up a chain with one single-node validator and one horcrux validator
 // the horcrux validator will have three pcosigners nodes with a threshold of two, and one sentry node
 func Test2Of3SignerOneSentry(t *testing.T) {
-	testChainSingleNodeAndHorcruxThreshold(t, 2, 3, 2, 1, 1)
+	testChainSingleNodeAndHorcruxThreshold(t, 4, 3, 2, 1, 1)
 }
 
 // Test2Of3SignerTwoSentries will spin up a chain with one single-node validator and one horcrux validator