From bcc805c7d3a4a8121e2655b3a7c5855252f73f35 Mon Sep 17 00:00:00 2001
From: mike neuder <michael.neuder@gmail.com>
Date: Wed, 2 Aug 2023 11:06:36 -0400
Subject: [PATCH] [code health] refactor slot details checks in
 handleSubmitNewBlock (#494)

---
 services/api/service.go      | 47 +++++++++++++++----------
 services/api/service_test.go | 67 ++++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+), 19 deletions(-)

diff --git a/services/api/service.go b/services/api/service.go
index 1ebace74..32377192 100644
--- a/services/api/service.go
+++ b/services/api/service.go
@@ -1538,7 +1538,32 @@ func (api *RelayAPI) checkSubmissionPayloadAttrs(w http.ResponseWriter, log *log
 	return true
 }
 
-func (api *RelayAPI) handleSubmitNewBlock(w http.ResponseWriter, req *http.Request) { //nolint:gocognit,maintidx
+func (api *RelayAPI) checkSubmissionSlotDetails(w http.ResponseWriter, log *logrus.Entry, headSlot uint64, payload *common.BuilderSubmitBlockRequest) bool {
+	// TODO: add deneb support.
+	if payload.Capella == nil {
+		log.Info("rejecting submission - non-capella payload for capella fork")
+		api.RespondError(w, http.StatusBadRequest, "non-capella payload")
+		return false
+	}
+
+	if payload.Slot() <= headSlot {
+		log.Info("submitNewBlock failed: submission for past slot")
+		api.RespondError(w, http.StatusBadRequest, "submission for past slot")
+		return false
+	}
+
+	// Timestamp check
+	expectedTimestamp := api.genesisInfo.Data.GenesisTime + (payload.Slot() * common.SecondsPerSlot)
+	if payload.Timestamp() != expectedTimestamp {
+		log.Warnf("incorrect timestamp. got %d, expected %d", payload.Timestamp(), expectedTimestamp)
+		api.RespondError(w, http.StatusBadRequest, fmt.Sprintf("incorrect timestamp. got %d, expected %d", payload.Timestamp(), expectedTimestamp))
+		return false
+	}
+
+	return true
+}
+
+func (api *RelayAPI) handleSubmitNewBlock(w http.ResponseWriter, req *http.Request) {
 	var pf common.Profile
 	var prevTime, nextTime time.Time
 
@@ -1646,16 +1671,8 @@ func (api *RelayAPI) handleSubmitNewBlock(w http.ResponseWriter, req *http.Reque
 		return
 	}
 
-	// TODO: add deneb support.
-	if payload.Capella == nil {
-		log.Info("rejecting submission - non capella payload for capella fork")
-		api.RespondError(w, http.StatusBadRequest, "not capella payload")
-		return
-	}
-
-	if payload.Slot() <= headSlot {
-		log.Info("submitNewBlock failed: submission for past slot")
-		api.RespondError(w, http.StatusBadRequest, "submission for past slot")
+	cont := api.checkSubmissionSlotDetails(w, log, headSlot, payload)
+	if !cont {
 		return
 	}
 
@@ -1674,14 +1691,6 @@ func (api *RelayAPI) handleSubmitNewBlock(w http.ResponseWriter, req *http.Reque
 	}
 	log = log.WithField("builderIsHighPrio", builderEntry.status.IsHighPrio)
 
-	// Timestamp check
-	expectedTimestamp := api.genesisInfo.Data.GenesisTime + (payload.Slot() * common.SecondsPerSlot)
-	if payload.Timestamp() != expectedTimestamp {
-		log.Warnf("incorrect timestamp. got %d, expected %d", payload.Timestamp(), expectedTimestamp)
-		api.RespondError(w, http.StatusBadRequest, fmt.Sprintf("incorrect timestamp. got %d, expected %d", payload.Timestamp(), expectedTimestamp))
-		return
-	}
-
 	if builderEntry.status.IsBlacklisted {
 		log.Info("builder is blacklisted")
 		time.Sleep(200 * time.Millisecond)
diff --git a/services/api/service_test.go b/services/api/service_test.go
index 882a0015..2f3549a9 100644
--- a/services/api/service_test.go
+++ b/services/api/service_test.go
@@ -729,6 +729,73 @@ func TestCheckSubmissionPayloadAttrs(t *testing.T) {
 	}
 }
 
+func TestCheckSubmissionSlotDetails(t *testing.T) {
+	cases := []struct {
+		description string
+		payload     *common.BuilderSubmitBlockRequest
+		expectCont  bool
+	}{
+		{
+			description: "success",
+			payload: &common.BuilderSubmitBlockRequest{
+				Capella: &builderCapella.SubmitBlockRequest{
+					ExecutionPayload: &capella.ExecutionPayload{
+						Timestamp: testSlot * common.SecondsPerSlot,
+					},
+					Message: &v1.BidTrace{
+						Slot: testSlot,
+					},
+				},
+			},
+			expectCont: true,
+		},
+		{
+			description: "failure_nil_capella",
+			payload: &common.BuilderSubmitBlockRequest{
+				Capella: nil, // nil to cause error
+			},
+			expectCont: false,
+		},
+		{
+			description: "failure_past_slot",
+			payload: &common.BuilderSubmitBlockRequest{
+				Capella: &builderCapella.SubmitBlockRequest{
+					Message: &v1.BidTrace{
+						Slot: testSlot - 1, // use old slot to cause error
+					},
+				},
+			},
+			expectCont: false,
+		},
+		{
+			description: "failure_wrong_timestamp",
+			payload: &common.BuilderSubmitBlockRequest{
+				Capella: &builderCapella.SubmitBlockRequest{
+					ExecutionPayload: &capella.ExecutionPayload{
+						Timestamp: testSlot*common.SecondsPerSlot - 1, // use wrong timestamp to cause error
+					},
+					Message: &v1.BidTrace{
+						Slot: testSlot,
+					},
+				},
+			},
+			expectCont: false,
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.description, func(t *testing.T) {
+			_, _, backend := startTestBackend(t)
+
+			headSlot := testSlot - 1
+			w := httptest.NewRecorder()
+			logger := logrus.New()
+			log := logrus.NewEntry(logger)
+			cont := backend.relay.checkSubmissionSlotDetails(w, log, headSlot, tc.payload)
+			require.Equal(t, tc.expectCont, cont)
+		})
+	}
+}
+
 func gzipBytes(t *testing.T, b []byte) []byte {
 	t.Helper()
 	var buf bytes.Buffer