fix error checking over rpc (#223)

* temp fix for error checking over rpc * improve logs, lint * use grpc status check instead of checking horcrux errs
strangelove-ventures · Nov 17, 2023 · ad63753 · ad63753
1 parent de54dfd
commit ad63753
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 40 deletions.
diff --git a/signer/sign_state.go b/signer/sign_state.go
@@ -348,11 +348,7 @@ func newStepRegressionError(height, round int64, regressed, last int8) *StepRegr
 	}
 }
 
-type EmptySignBytesError struct{}
-
-func (e *EmptySignBytesError) Error() string {
-	return "no SignBytes found"
-}
+var ErrEmptySignBytes = errors.New("no SignBytes found")
 
 // CheckHRS checks the given height, round, step (HRS) against that of the
 // SignState. It returns an error if the arguments constitute a regression,
@@ -380,7 +376,7 @@ func (signState *SignState) CheckHRS(hrst HRSTKey) (bool, error) {
 					}
 					return true, nil
 				}
-				return false, new(EmptySignBytesError)
+				return false, ErrEmptySignBytes
 			}
 		}
 	}

diff --git a/signer/threshold_validator.go b/signer/threshold_validator.go
@@ -14,6 +14,8 @@ import (
 	"github.com/google/uuid"
 	"github.com/strangelove-ventures/horcrux/signer/proto"
 	"golang.org/x/sync/errgroup"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 var _ PrivValidator = &ThresholdValidator{}
@@ -610,6 +612,13 @@ func (pv *ThresholdValidator) proxyIfNecessary(
 func (pv *ThresholdValidator) Sign(ctx context.Context, chainID string, block Block) ([]byte, time.Time, error) {
 	height, round, step, stamp, signBytes := block.Height, block.Round, block.Step, block.Timestamp, block.SignBytes
 
+	log := pv.logger.With(
+		"chain_id", chainID,
+		"height", height,
+		"round", round,
+		"type", signType(step),
+	)
+
 	if err := pv.LoadSignStateIfNecessary(chainID); err != nil {
 		return nil, stamp, err
 	}
@@ -622,13 +631,8 @@ func (pv *ThresholdValidator) Sign(ctx context.Context, chainID string, block Bl
 	}
 
 	totalRaftLeader.Inc()
-	pv.logger.Debug(
-		"I am the leader. Managing the sign process for this block",
-		"chain_id", chainID,
-		"height", height,
-		"round", round,
-		"step", step,
-	)
+
+	log.Debug("I am the leader. Managing the sign process for this block")
 
 	timeStartSignBlock := time.Now()
 
@@ -645,7 +649,7 @@ func (pv *ThresholdValidator) Sign(ctx context.Context, chainID string, block Bl
 		return nil, stamp, fmt.Errorf("error saving last sign state initiated: %w", err)
 	}
 	if existingSignature != nil {
-		pv.logger.Debug("Returning existing signature", "signature", fmt.Sprintf("%x", existingSignature))
+		log.Debug("Returning existing signature", "signature", fmt.Sprintf("%x", existingSignature))
 		return existingSignature, existingTimestamp, nil
 	}
 
@@ -722,33 +726,17 @@ func (pv *ThresholdValidator) Sign(ctx context.Context, chainID string, block Bl
 					SignBytes: signBytes,
 				})
 				if err != nil {
-					pv.logger.Error(
+					log.Error(
 						"Cosigner failed to set nonces and sign",
-						"id", cosigner.GetID(),
+						"cosigner", cosigner.GetID(),
 						"err", err.Error(),
 					)
 
 					if cosigner.GetID() == pv.myCosigner.GetID() {
 						return err
 					}
 
-					hre := new(HeightRegressionError)
-					rre := new(RoundRegressionError)
-					sre := new(StepRegressionError)
-					ese := new(EmptySignBytesError)
-					ase := new(AlreadySignedVoteError)
-					dbe := new(DiffBlockIDsError)
-					cde := new(ConflictingDataError)
-					ue := new(UnmarshalError)
-
-					if !errors.As(err, &hre) &&
-						!errors.As(err, &rre) &&
-						!errors.As(err, &sre) &&
-						!errors.As(err, &ese) &&
-						!errors.As(err, &ue) &&
-						!errors.As(err, &ase) &&
-						!errors.As(err, &dbe) &&
-						!errors.As(err, &cde) {
+					if c := status.Code(err); c == codes.DeadlineExceeded || c == codes.NotFound || c == codes.Unavailable {
 						pv.cosignerHealth.MarkUnhealthy(cosigner)
 						pv.nonceCache.ClearNonces(cosigner)
 					}
@@ -851,20 +839,16 @@ func (pv *ThresholdValidator) Sign(ctx context.Context, chainID string, block Bl
 	if err != nil {
 		// this is not required for double sign protection, so we don't need to return an error here.
 		// this is only an additional mechanism that will catch double signs earlier in the sign process.
-		pv.logger.Error("Error emitting LSS", err.Error())
+		log.Error("Error emitting LSS", err.Error())
 	}
 
 	timeSignBlock := time.Since(timeStartSignBlock)
 	timeSignBlockSec := timeSignBlock.Seconds()
 	timedSignBlockLag.Observe(timeSignBlockSec)
 
-	pv.logger.Info(
+	log.Info(
 		"Signed",
-		"chain_id", chainID,
-		"height", height,
-		"round", round,
-		"type", signType(step),
-		"duration_ms", timeSignBlock.Round(time.Millisecond),
+		"duration_ms", float64(timeSignBlock.Microseconds())/1000,
 	)
 
 	return signature, stamp, nil