From b1690efeabdfc298f24afd5a1ddf755f274378a8 Mon Sep 17 00:00:00 2001 From: "F. Eugene Aumson" Date: Mon, 29 Jul 2024 19:54:33 +0000 Subject: [PATCH 01/10] SigAggRawReq::UnsignedMessage: hex str, not bytes --- signature-aggregator/api/api.go | 17 ++++++++++++++--- tests/signature_aggregator_api.go | 3 ++- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/signature-aggregator/api/api.go b/signature-aggregator/api/api.go index d98e5f6c..d227c121 100644 --- a/signature-aggregator/api/api.go +++ b/signature-aggregator/api/api.go @@ -4,8 +4,10 @@ package api import ( + "encoding/hex" "encoding/json" "net/http" + "strings" "github.com/ava-labs/avalanchego/ids" "github.com/ava-labs/avalanchego/utils/logging" @@ -34,8 +36,8 @@ type SignatureAggregationByIDRequest struct { // Defines a request interface for signature aggregation for a raw unsigned message. // Currently a copy of the `ManualWarpMessageRequest` struct in relay_message.go type SignatureAggregationRawRequest struct { - // Required. Unsigned base64 encoded message bytes. - UnsignedMessageBytes []byte `json:"unsigned-message-bytes"` + // Required. hex-encoded message bytes, optionally prefixed with "0x". + UnsignedMessage string `json:"unsigned-message"` // Optional hex or cb58 encoded signing subnet ID. If omitted will default to the subnetID of the source BlockChain SigningSubnetID string `json:"signing-subnet-id"` // Optional. Integer from 0 to 100 representing the percentage of the quorum that is required to sign the message @@ -61,7 +63,16 @@ func signatureAggregationAPIHandler(logger logging.Logger, aggregator *aggregato http.Error(w, err.Error(), http.StatusBadRequest) return } - unsignedMessage, err := types.UnpackWarpMessage(req.UnsignedMessageBytes) + var decodedMessage []byte + decodedMessage, err = hex.DecodeString( + strings.TrimPrefix(req.UnsignedMessage, "0x"), + ) + if err != nil { + logger.Warn("Could not decode message", zap.String("msg", req.UnsignedMessage)) + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + unsignedMessage, err := types.UnpackWarpMessage(decodedMessage) if err != nil { logger.Warn("Error unpacking warp message", zap.Error(err)) http.Error(w, err.Error(), http.StatusBadRequest) diff --git a/tests/signature_aggregator_api.go b/tests/signature_aggregator_api.go index 4ef5cb2f..f3ae9d1f 100644 --- a/tests/signature_aggregator_api.go +++ b/tests/signature_aggregator_api.go @@ -6,6 +6,7 @@ package tests import ( "bytes" "context" + "encoding/hex" "encoding/json" "fmt" "io" @@ -55,7 +56,7 @@ func SignatureAggregatorAPI(network interfaces.LocalNetwork) { time.Sleep(5 * time.Second) reqBody := api.SignatureAggregationRawRequest{ - UnsignedMessageBytes: warpMessage.Bytes(), + UnsignedMessage: "0x" + hex.EncodeToString(warpMessage.Bytes()), } client := http.Client{ From c61239e09ba9f3f95c0b89b08790359523b0f289 Mon Sep 17 00:00:00 2001 From: "F. Eugene Aumson" Date: Mon, 29 Jul 2024 19:56:22 +0000 Subject: [PATCH 02/10] fix log error message before this, it was being rendered as: INFO [07-29|19:29:09.151] Starting the signature aggregator with config at :%s /home/gene/tmp/signature-aggregator-config.json61842304= LOG_ERROR="Normalized odd number of arguments by adding nil" --- tests/signature_aggregator_api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/signature_aggregator_api.go b/tests/signature_aggregator_api.go index f3ae9d1f..ae8b277d 100644 --- a/tests/signature_aggregator_api.go +++ b/tests/signature_aggregator_api.go @@ -47,7 +47,7 @@ func SignatureAggregatorAPI(network interfaces.LocalNetwork) { signatureAggregatorConfig, testUtils.DefaultSignatureAggregatorCfgFname, ) - log.Info("Starting the signature aggregator with config at :%s", signatureAggregatorConfigPath) + log.Info("Starting the signature aggregator", "configPath", signatureAggregatorConfigPath) signatureAggregatorCancel := testUtils.BuildAndRunSignatureAggregatorExecutable(ctx, signatureAggregatorConfigPath) defer signatureAggregatorCancel() From 22d638089d37d3703e35d78919e8451fec54afb0 Mon Sep 17 00:00:00 2001 From: "F. Eugene Aumson" Date: Mon, 29 Jul 2024 19:57:37 +0000 Subject: [PATCH 03/10] deduplicate log message "Starting the signature-aggregator executable" message already exists elsewhere. This differentiates this log entry from that one. --- tests/utils/utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/utils/utils.go b/tests/utils/utils.go index 57ef200f..fc2b3cbe 100644 --- a/tests/utils/utils.go +++ b/tests/utils/utils.go @@ -115,7 +115,7 @@ func BuildAndRunSignatureAggregatorExecutable(ctx context.Context, configPath st // Run signature-aggregator binary with config path var signatureAggregatorCtx context.Context signatureAggregatorCtx, signatureAggregatorCancelFunc := context.WithCancel(ctx) - log.Info("Starting the signature-aggregator executable") + log.Info("Instantiating the signature-aggregator executable command") log.Info(fmt.Sprintf("./signature-aggregator/build/signature-aggregator --config-file %s ", configPath)) signatureAggregatorCmd := exec.CommandContext( signatureAggregatorCtx, From 68b0ee2ea05868f75dc63ac4ded2d5fc79332fc3 Mon Sep 17 00:00:00 2001 From: "F. Eugene Aumson" Date: Mon, 29 Jul 2024 21:27:53 +0000 Subject: [PATCH 04/10] SigAgResponse: hex encoded, not bytes --- signature-aggregator/api/api.go | 11 ++++++----- tests/signature_aggregator_api.go | 5 ++++- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/signature-aggregator/api/api.go b/signature-aggregator/api/api.go index d227c121..4d8a8ec7 100644 --- a/signature-aggregator/api/api.go +++ b/signature-aggregator/api/api.go @@ -36,7 +36,7 @@ type SignatureAggregationByIDRequest struct { // Defines a request interface for signature aggregation for a raw unsigned message. // Currently a copy of the `ManualWarpMessageRequest` struct in relay_message.go type SignatureAggregationRawRequest struct { - // Required. hex-encoded message bytes, optionally prefixed with "0x". + // Required. hex-encoded message, optionally prefixed with "0x". UnsignedMessage string `json:"unsigned-message"` // Optional hex or cb58 encoded signing subnet ID. If omitted will default to the subnetID of the source BlockChain SigningSubnetID string `json:"signing-subnet-id"` @@ -46,8 +46,8 @@ type SignatureAggregationRawRequest struct { } type SignatureAggregationResponse struct { - // base64 encoding of the signature - SignedMessageBytes []byte `json:"signed-message-bytes"` + // hex encoding of the signature + SignedMessage string `json:"signed-message"` } func HandleSignatureAggregationRawRequest(logger logging.Logger, signatureAggregator *aggregator.SignatureAggregator) { @@ -98,14 +98,15 @@ func signatureAggregationAPIHandler(logger logging.Logger, aggregator *aggregato } signedMessage, err := aggregator.AggregateSignaturesAppRequest(unsignedMessage, signingSubnetID, quorumNum) - if err != nil { logger.Warn("Failed to aggregate signatures", zap.Error(err)) http.Error(w, "error aggregating signatures: "+err.Error(), http.StatusInternalServerError) } resp, err := json.Marshal( SignatureAggregationResponse{ - SignedMessageBytes: signedMessage.Bytes(), + SignedMessage: hex.EncodeToString( + signedMessage.Bytes(), + ), }, ) diff --git a/tests/signature_aggregator_api.go b/tests/signature_aggregator_api.go index ae8b277d..da20f13b 100644 --- a/tests/signature_aggregator_api.go +++ b/tests/signature_aggregator_api.go @@ -87,7 +87,10 @@ func SignatureAggregatorAPI(network interfaces.LocalNetwork) { err = json.Unmarshal(body, &response) Expect(err).Should(BeNil()) - signedMessage, err := avalancheWarp.ParseMessage(response.SignedMessageBytes) + decodedMessage, err := hex.DecodeString(response.SignedMessage) + Expect(err).Should(BeNil()) + + signedMessage, err := avalancheWarp.ParseMessage(decodedMessage) Expect(err).Should(BeNil()) Expect(signedMessage.ID()).Should(Equal(warpMessage.ID())) } From 05b1d593982c6ba42b8c2fdb997ea6c94ecfd9c3 Mon Sep 17 00:00:00 2001 From: "F. Eugene Aumson" Date: Tue, 30 Jul 2024 15:01:29 +0000 Subject: [PATCH 05/10] set response type to json --- signature-aggregator/api/api.go | 1 + tests/signature_aggregator_api.go | 1 + 2 files changed, 2 insertions(+) diff --git a/signature-aggregator/api/api.go b/signature-aggregator/api/api.go index 4d8a8ec7..3f5ddb10 100644 --- a/signature-aggregator/api/api.go +++ b/signature-aggregator/api/api.go @@ -115,6 +115,7 @@ func signatureAggregationAPIHandler(logger logging.Logger, aggregator *aggregato http.Error(w, "error marshalling a response: "+err.Error(), http.StatusInternalServerError) return } + w.Header().Set("Content-Type", "application/json") _, err = w.Write(resp) if err != nil { logger.Error("Error writing response", zap.Error(err)) diff --git a/tests/signature_aggregator_api.go b/tests/signature_aggregator_api.go index da20f13b..acd01fef 100644 --- a/tests/signature_aggregator_api.go +++ b/tests/signature_aggregator_api.go @@ -78,6 +78,7 @@ func SignatureAggregatorAPI(network interfaces.LocalNetwork) { res, err := client.Do(req) Expect(err).Should(BeNil()) Expect(res.Status).Should(Equal("200 OK")) + Expect(res.Header.Get("Content-Type")).Should(Equal("application/json")) defer res.Body.Close() body, err := io.ReadAll(res.Body) From 079ff030062b57acfddaf1e14c89dba7e1f8715a Mon Sep 17 00:00:00 2001 From: "F. Eugene Aumson" Date: Tue, 30 Jul 2024 14:55:27 +0000 Subject: [PATCH 06/10] errors: don't expose internals; always JSON and other error handling cleanup --- signature-aggregator/api/api.go | 64 +++++++++++++++++++++++++-------- 1 file changed, 50 insertions(+), 14 deletions(-) diff --git a/signature-aggregator/api/api.go b/signature-aggregator/api/api.go index 3f5ddb10..7f7f8807 100644 --- a/signature-aggregator/api/api.go +++ b/signature-aggregator/api/api.go @@ -54,13 +54,35 @@ func HandleSignatureAggregationRawRequest(logger logging.Logger, signatureAggreg http.Handle(RawMessageAPIPath, signatureAggregationAPIHandler(logger, signatureAggregator)) } +func writeJsonError( + logger logging.Logger, + w http.ResponseWriter, + errorMsg string, +) { + resp, err := json.Marshal(struct{ error string }{error: errorMsg}) + if err != nil { + logger.Error( + "Error marshalling JSON error response", + zap.Error(err), + ) + } + + w.Header().Set("Content-Type", "application/json") + + w.Write(resp) + if err != nil { + logger.Error("Error writing error response", zap.Error(err)) + } +} + func signatureAggregationAPIHandler(logger logging.Logger, aggregator *aggregator.SignatureAggregator) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var req SignatureAggregationRawRequest err := json.NewDecoder(r.Body).Decode(&req) if err != nil { - logger.Warn("Could not decode request body") - http.Error(w, err.Error(), http.StatusBadRequest) + msg := "Could not decode request body" + logger.Warn(msg, zap.Error(err)) + writeJsonError(logger, w, msg) return } var decodedMessage []byte @@ -68,22 +90,29 @@ func signatureAggregationAPIHandler(logger logging.Logger, aggregator *aggregato strings.TrimPrefix(req.UnsignedMessage, "0x"), ) if err != nil { - logger.Warn("Could not decode message", zap.String("msg", req.UnsignedMessage)) - http.Error(w, err.Error(), http.StatusBadRequest) + msg := "Could not decode message" + logger.Warn( + msg, + zap.String("msg", req.UnsignedMessage), + zap.Error(err), + ) + writeJsonError(logger, w, msg) return } unsignedMessage, err := types.UnpackWarpMessage(decodedMessage) if err != nil { - logger.Warn("Error unpacking warp message", zap.Error(err)) - http.Error(w, err.Error(), http.StatusBadRequest) + msg := "Error unpacking warp message" + logger.Warn(msg, zap.Error(err)) + writeJsonError(logger, w, msg) return } var quorumNum uint64 if req.QuorumNum == nil { quorumNum = defaultQuorumNum } else if *req.QuorumNum >= 0 || *req.QuorumNum > 100 { - logger.Warn("Invalid quorum number", zap.Uint64("quorum-num", *req.QuorumNum)) - http.Error(w, "invalid quorum number", http.StatusBadRequest) + msg := "Invalid quorum number" + logger.Warn(msg, zap.Uint64("quorum-num", *req.QuorumNum)) + writeJsonError(logger, w, msg) return } else { quorumNum = *req.QuorumNum @@ -92,15 +121,21 @@ func signatureAggregationAPIHandler(logger logging.Logger, aggregator *aggregato if req.SigningSubnetID != "" { *signingSubnetID, err = utils.HexOrCB58ToID(req.SigningSubnetID) if err != nil { - logger.Warn("Error parsing signing subnet ID", zap.Error(err)) - http.Error(w, "error parsing signing subnet ID: "+err.Error(), http.StatusBadRequest) + msg := "Error parsing signing subnet ID" + logger.Warn( + msg, + zap.Error(err), + zap.String("input", req.SigningSubnetID), + ) + writeJsonError(logger, w, msg) } } signedMessage, err := aggregator.AggregateSignaturesAppRequest(unsignedMessage, signingSubnetID, quorumNum) if err != nil { - logger.Warn("Failed to aggregate signatures", zap.Error(err)) - http.Error(w, "error aggregating signatures: "+err.Error(), http.StatusInternalServerError) + msg := "Failed to aggregate signatures" + logger.Warn(msg, zap.Error(err)) + writeJsonError(logger, w, msg) } resp, err := json.Marshal( SignatureAggregationResponse{ @@ -111,8 +146,9 @@ func signatureAggregationAPIHandler(logger logging.Logger, aggregator *aggregato ) if err != nil { - logger.Error("Failed to marshal response", zap.Error(err)) - http.Error(w, "error marshalling a response: "+err.Error(), http.StatusInternalServerError) + msg := "Failed to marshal response" + logger.Error(msg, zap.Error(err)) + writeJsonError(logger, w, msg) return } w.Header().Set("Content-Type", "application/json") From 90f4005050a60c885d22c829185d055a7aa4cd7a Mon Sep 17 00:00:00 2001 From: "F. Eugene Aumson" Date: Tue, 30 Jul 2024 17:55:30 +0000 Subject: [PATCH 07/10] avoid a pointer to int --- signature-aggregator/api/api.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/signature-aggregator/api/api.go b/signature-aggregator/api/api.go index 7f7f8807..ab72a511 100644 --- a/signature-aggregator/api/api.go +++ b/signature-aggregator/api/api.go @@ -42,7 +42,7 @@ type SignatureAggregationRawRequest struct { SigningSubnetID string `json:"signing-subnet-id"` // Optional. Integer from 0 to 100 representing the percentage of the quorum that is required to sign the message // defaults to 67 if omitted. - QuorumNum *uint64 `json:"quorum-num"` + QuorumNum uint64 `json:"quorum-num"` } type SignatureAggregationResponse struct { @@ -107,15 +107,15 @@ func signatureAggregationAPIHandler(logger logging.Logger, aggregator *aggregato return } var quorumNum uint64 - if req.QuorumNum == nil { + if req.QuorumNum == 0 { quorumNum = defaultQuorumNum - } else if *req.QuorumNum >= 0 || *req.QuorumNum > 100 { + } else if req.QuorumNum >= 0 || req.QuorumNum > 100 { msg := "Invalid quorum number" - logger.Warn(msg, zap.Uint64("quorum-num", *req.QuorumNum)) + logger.Warn(msg, zap.Uint64("quorum-num", req.QuorumNum)) writeJsonError(logger, w, msg) return } else { - quorumNum = *req.QuorumNum + quorumNum = req.QuorumNum } var signingSubnetID *ids.ID if req.SigningSubnetID != "" { From 97b926c5cc549b046cddaff21e92b67ec86e127e Mon Sep 17 00:00:00 2001 From: "F. Eugene Aumson" Date: Tue, 30 Jul 2024 17:56:55 +0000 Subject: [PATCH 08/10] avoid a pointer to ids.ID --- signature-aggregator/api/api.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/signature-aggregator/api/api.go b/signature-aggregator/api/api.go index ab72a511..3383a4c5 100644 --- a/signature-aggregator/api/api.go +++ b/signature-aggregator/api/api.go @@ -117,9 +117,11 @@ func signatureAggregationAPIHandler(logger logging.Logger, aggregator *aggregato } else { quorumNum = req.QuorumNum } - var signingSubnetID *ids.ID + var signingSubnetID ids.ID if req.SigningSubnetID != "" { - *signingSubnetID, err = utils.HexOrCB58ToID(req.SigningSubnetID) + signingSubnetID, err = utils.HexOrCB58ToID( + req.SigningSubnetID, + ) if err != nil { msg := "Error parsing signing subnet ID" logger.Warn( @@ -131,7 +133,11 @@ func signatureAggregationAPIHandler(logger logging.Logger, aggregator *aggregato } } - signedMessage, err := aggregator.AggregateSignaturesAppRequest(unsignedMessage, signingSubnetID, quorumNum) + signedMessage, err := aggregator.AggregateSignaturesAppRequest( + unsignedMessage, + &signingSubnetID, + quorumNum, + ) if err != nil { msg := "Failed to aggregate signatures" logger.Warn(msg, zap.Error(err)) From e90f8d2d1be5690552437fe8c2f955479abc9fa7 Mon Sep 17 00:00:00 2001 From: "F. Eugene Aumson" Date: Tue, 30 Jul 2024 19:51:48 +0000 Subject: [PATCH 09/10] avoid another pointer to ids.ID --- relayer/application_relayer.go | 2 +- signature-aggregator/aggregator/aggregator.go | 6 +++--- signature-aggregator/api/api.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/relayer/application_relayer.go b/relayer/application_relayer.go index 03faa6f8..682f4772 100644 --- a/relayer/application_relayer.go +++ b/relayer/application_relayer.go @@ -209,7 +209,7 @@ func (r *ApplicationRelayer) ProcessMessage(handler messages.MessageHandler) (co // TODO: do we actually want to pass the pointer here or adapt the interface? signedMessage, err = r.signatureAggregator.AggregateSignaturesAppRequest( unsignedMessage, - &r.signingSubnetID, + r.signingSubnetID, r.warpQuorum.QuorumNumerator, ) r.incFetchSignatureAppRequestCount() diff --git a/signature-aggregator/aggregator/aggregator.go b/signature-aggregator/aggregator/aggregator.go index 933db4bd..c1aa4ff4 100644 --- a/signature-aggregator/aggregator/aggregator.go +++ b/signature-aggregator/aggregator/aggregator.go @@ -75,7 +75,7 @@ func NewSignatureAggregator( func (s *SignatureAggregator) AggregateSignaturesAppRequest( unsignedMessage *avalancheWarp.UnsignedMessage, - inputSigningSubnet *ids.ID, + inputSigningSubnet ids.ID, quorumPercentage uint64, ) (*avalancheWarp.Message, error) { requestID := s.currentRequestID.Add(1) @@ -87,10 +87,10 @@ func (s *SignatureAggregator) AggregateSignaturesAppRequest( if err != nil { return nil, fmt.Errorf("Source message subnet not found for chainID %s", unsignedMessage.SourceChainID) } - if inputSigningSubnet == nil { + if inputSigningSubnet == ids.Empty { signingSubnet = sourceSubnet } else { - signingSubnet = *inputSigningSubnet + signingSubnet = inputSigningSubnet } connectedValidators, err := s.network.ConnectToCanonicalValidators(signingSubnet) diff --git a/signature-aggregator/api/api.go b/signature-aggregator/api/api.go index 3383a4c5..87f7d613 100644 --- a/signature-aggregator/api/api.go +++ b/signature-aggregator/api/api.go @@ -135,7 +135,7 @@ func signatureAggregationAPIHandler(logger logging.Logger, aggregator *aggregato signedMessage, err := aggregator.AggregateSignaturesAppRequest( unsignedMessage, - &signingSubnetID, + signingSubnetID, quorumNum, ) if err != nil { From da09584537decb4de155eb37c00ad64af61931a4 Mon Sep 17 00:00:00 2001 From: "F. Eugene Aumson" Date: Tue, 30 Jul 2024 21:13:46 +0000 Subject: [PATCH 10/10] streamline quorum handling addresses review comments https://github.com/ava-labs/awm-relayer/pull/393#discussion_r1697528672 and https://github.com/ava-labs/awm-relayer/pull/393#discussion_r1697545793 i went ahead and just removed the `>=0` case (which, as discussed, should really be `<=0`) since we're already checking `==0` in the previous branch, and since it can't be `<0` since it's an unsigned value. --- signature-aggregator/api/api.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/signature-aggregator/api/api.go b/signature-aggregator/api/api.go index 87f7d613..35549eb2 100644 --- a/signature-aggregator/api/api.go +++ b/signature-aggregator/api/api.go @@ -106,16 +106,14 @@ func signatureAggregationAPIHandler(logger logging.Logger, aggregator *aggregato writeJsonError(logger, w, msg) return } - var quorumNum uint64 - if req.QuorumNum == 0 { + quorumNum := req.QuorumNum + if quorumNum == 0 { quorumNum = defaultQuorumNum - } else if req.QuorumNum >= 0 || req.QuorumNum > 100 { + } else if req.QuorumNum > 100 { msg := "Invalid quorum number" logger.Warn(msg, zap.Uint64("quorum-num", req.QuorumNum)) writeJsonError(logger, w, msg) return - } else { - quorumNum = req.QuorumNum } var signingSubnetID ids.ID if req.SigningSubnetID != "" {