[code health] refactor floor bid checks to independent function

datastore/redis.go

@@ -727,6 +727,13 @@ func (r *RedisCache) GetFloorBidValue(ctx context.Context, tx redis.Pipeliner, s
 	return floorValue, nil
 }
 
+// SetFloorBidValue is used only for testing.
+func (r *RedisCache) SetFloorBidValue(slot uint64, parentHash, proposerPubkey, value string) error {
+	keyFloorBidValue := r.keyFloorBidValue(slot, parentHash, proposerPubkey)
+	err := r.client.Set(context.Background(), keyFloorBidValue, value, 0).Err()
+	return err
+}
+
 func (r *RedisCache) NewPipeline() redis.Pipeliner { //nolint:ireturn,nolintlint
 	return r.client.Pipeline()
 }

services/api/service.go

@@ -1595,6 +1595,59 @@ func (api *RelayAPI) checkBuilderEntry(w http.ResponseWriter, log *logrus.Entry,
 	return builderEntry, true
 }
 
+type bidFloorOpts struct {
+	w                    http.ResponseWriter
+	tx                   redis.Pipeliner
+	log                  *logrus.Entry
+	cancellationsEnabled bool
+	simResultC           chan *blockSimResult
+	payload              *common.BuilderSubmitBlockRequest
+}
+
+func (api *RelayAPI) checkFloorBidValue(opts bidFloorOpts) (*big.Int, *logrus.Entry, bool) {
+	// Reject new submissions once the payload for this slot was delivered - TODO: store in memory as well
+	slotLastPayloadDelivered, err := api.redis.GetLastSlotDelivered(context.Background(), opts.tx)
+	if err != nil && !errors.Is(err, redis.Nil) {
+		opts.log.WithError(err).Error("failed to get delivered payload slot from redis")
+	} else if opts.payload.Slot() <= slotLastPayloadDelivered {
+		opts.log.Info("rejecting submission because payload for this slot was already delivered")
+		api.RespondError(opts.w, http.StatusBadRequest, "payload for this slot was already delivered")
+		return nil, nil, false
+	}
+
+	// Grab floor bid value
+	floorBidValue, err := api.redis.GetFloorBidValue(context.Background(), opts.tx, opts.payload.Slot(), opts.payload.ParentHash(), opts.payload.ProposerPubkey())
+	if err != nil {
+		opts.log.WithError(err).Error("failed to get floor bid value from redis")
+	} else {
+		opts.log = opts.log.WithField("floorBidValue", floorBidValue.String())
+	}
+
+	// --------------------------------------------
+	// Skip submission if below the floor bid value
+	// --------------------------------------------
+	isBidBelowFloor := floorBidValue != nil && opts.payload.Value().Cmp(floorBidValue) == -1
+	isBidAtOrBelowFloor := floorBidValue != nil && opts.payload.Value().Cmp(floorBidValue) < 1
+	if opts.cancellationsEnabled && isBidBelowFloor { // with cancellations: if below floor -> delete previous bid
+		opts.simResultC <- &blockSimResult{false, false, nil, nil}
+		opts.log.Info("submission below floor bid value, with cancellation")
+		err := api.redis.DelBuilderBid(context.Background(), opts.tx, opts.payload.Slot(), opts.payload.ParentHash(), opts.payload.ProposerPubkey(), opts.payload.BuilderPubkey().String())
+		if err != nil {
+			opts.log.WithError(err).Error("failed processing cancellable bid below floor")
+			api.RespondError(opts.w, http.StatusInternalServerError, "failed processing cancellable bid below floor")
+			return nil, nil, false
+		}
+		api.Respond(opts.w, http.StatusAccepted, "accepted bid below floor, skipped validation")
+		return nil, nil, false
+	} else if !opts.cancellationsEnabled && isBidAtOrBelowFloor { // without cancellations: if at or below floor -> ignore
+		opts.simResultC <- &blockSimResult{false, false, nil, nil}
+		opts.log.Info("submission at or below floor bid value, without cancellation")
+		api.RespondMsg(opts.w, http.StatusAccepted, "accepted bid below floor, skipped validation")
+		return nil, nil, false
+	}
+	return floorBidValue, opts.log, true
+}
+
 func (api *RelayAPI) handleSubmitNewBlock(w http.ResponseWriter, req *http.Request) {
 	var pf common.Profile
 	var prevTime, nextTime time.Time
@@ -1764,24 +1817,23 @@ func (api *RelayAPI) handleSubmitNewBlock(w http.ResponseWriter, req *http.Reque
 	// Create the redis pipeline tx
 	tx := api.redis.NewTxPipeline()
 
-	// Reject new submissions once the payload for this slot was delivered - TODO: store in memory as well
-	slotLastPayloadDelivered, err := api.redis.GetLastSlotDelivered(context.Background(), tx)
-	if err != nil && !errors.Is(err, redis.Nil) {
-		log.WithError(err).Error("failed to get delivered payload slot from redis")
-	} else if payload.Slot() <= slotLastPayloadDelivered {
-		log.Info("rejecting submission because payload for this slot was already delivered")
-		api.RespondError(w, http.StatusBadRequest, "payload for this slot was already delivered")
-		return
-	}
-
-	// -------------------------------------------------------------------
-	// SUBMISSION SIGNATURE IS VALIDATED AND BID IS GENERALLY LOOKING GOOD
-	// -------------------------------------------------------------------
-
 	// channel to send simulation result to the deferred function
 	simResultC := make(chan *blockSimResult, 1)
 	var eligibleAt time.Time // will be set once the bid is ready
 
+	bfOpts := bidFloorOpts{
+		w:                    w,
+		tx:                   tx,
+		log:                  log,
+		cancellationsEnabled: isCancellationEnabled,
+		simResultC:           simResultC,
+		payload:              payload,
+	}
+	floorBidValue, log, ok := api.checkFloorBidValue(bfOpts)
+	if !ok {
+		return
+	}
+
 	// Deferred saving of the builder submission to database (whenever this function ends)
 	defer func() {
 		savePayloadToDatabase := !api.ffDisablePayloadDBStorage
@@ -1805,37 +1857,6 @@ func (api *RelayAPI) handleSubmitNewBlock(w http.ResponseWriter, req *http.Reque
 		}
 	}()
 
-	// Grab floor bid value
-	floorBidValue, err := api.redis.GetFloorBidValue(context.Background(), tx, payload.Slot(), payload.ParentHash(), payload.ProposerPubkey())
-	if err != nil {
-		log.WithError(err).Error("failed to get floor bid value from redis")
-	} else {
-		log = log.WithField("floorBidValue", floorBidValue.String())
-	}
-
-	// --------------------------------------------
-	// Skip submission if below the floor bid value
-	// --------------------------------------------
-	isBidBelowFloor := floorBidValue != nil && payload.Value().Cmp(floorBidValue) == -1
-	isBidAtOrBelowFloor := floorBidValue != nil && payload.Value().Cmp(floorBidValue) < 1
-	if isCancellationEnabled && isBidBelowFloor { // with cancellations: if below floor -> delete previous bid
-		simResultC <- &blockSimResult{false, false, nil, nil}
-		log.Info("submission below floor bid value, with cancellation")
-		err := api.redis.DelBuilderBid(context.Background(), tx, payload.Slot(), payload.ParentHash(), payload.ProposerPubkey(), payload.BuilderPubkey().String())
-		if err != nil {
-			log.WithError(err).Error("failed processing cancellable bid below floor")
-			api.RespondError(w, http.StatusInternalServerError, "failed processing cancellable bid below floor")
-			return
-		}
-		api.Respond(w, http.StatusAccepted, "accepted bid below floor, skipped validation")
-		return
-	} else if !isCancellationEnabled && isBidAtOrBelowFloor { // without cancellations: if at or below floor -> ignore
-		simResultC <- &blockSimResult{false, false, nil, nil}
-		log.Info("submission below floor bid value, without cancellation")
-		api.RespondMsg(w, http.StatusAccepted, "accepted bid below floor, skipped validation")
-		return
-	}
-
 	// ---------------------------------
 	// THE BID WILL BE SIMULATED SHORTLY
 	// ---------------------------------

services/api/service_test.go

@@ -860,6 +860,93 @@ func TestCheckBuilderEntry(t *testing.T) {
 	}
 }
 
+func TestCheckFloorBidValue(t *testing.T) {
+	cases := []struct {
+		description          string
+		payload              *common.BuilderSubmitBlockRequest
+		cancellationsEnabled bool
+		floorValue           string
+		expectOk             bool
+	}{
+		{
+			description: "success",
+			payload: &common.BuilderSubmitBlockRequest{
+				Capella: &builderCapella.SubmitBlockRequest{
+					Message: &v1.BidTrace{
+						Slot:  testSlot,
+						Value: uint256.NewInt(1),
+					},
+				},
+			},
+			expectOk: true,
+		},
+		{
+			description: "failure_slot_already_delivered",
+			payload: &common.BuilderSubmitBlockRequest{
+				Capella: &builderCapella.SubmitBlockRequest{
+					Message: &v1.BidTrace{
+						Slot: 0,
+					},
+				},
+			},
+			expectOk: false,
+		},
+		{
+			description: "failure_cancellations_below_floor",
+			payload: &common.BuilderSubmitBlockRequest{
+				Capella: &builderCapella.SubmitBlockRequest{
+					Message: &v1.BidTrace{
+						Slot:  testSlot,
+						Value: uint256.NewInt(1),
+					},
+				},
+			},
+			expectOk:             false,
+			cancellationsEnabled: true,
+			floorValue:           "2",
+		},
+		{
+			description: "failure_no_cancellations_at_floor",
+			payload: &common.BuilderSubmitBlockRequest{
+				Capella: &builderCapella.SubmitBlockRequest{
+					Message: &v1.BidTrace{
+						Slot:  testSlot,
+						Value: uint256.NewInt(0),
+					},
+				},
+			},
+			expectOk: false,
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.description, func(t *testing.T) {
+			_, _, backend := startTestBackend(t)
+			err := backend.redis.SetFloorBidValue(tc.payload.Slot(), tc.payload.ParentHash(), tc.payload.ProposerPubkey(), tc.floorValue)
+			require.Nil(t, err)
+
+			w := httptest.NewRecorder()
+			logger := logrus.New()
+			log := logrus.NewEntry(logger)
+			tx := backend.redis.NewTxPipeline()
+			simResultC := make(chan *blockSimResult, 1)
+			bfOpts := bidFloorOpts{
+				w:                    w,
+				tx:                   tx,
+				log:                  log,
+				cancellationsEnabled: tc.cancellationsEnabled,
+				simResultC:           simResultC,
+				payload:              tc.payload,
+			}
+			floor, log, ok := backend.relay.checkFloorBidValue(bfOpts)
+			require.Equal(t, tc.expectOk, ok)
+			if ok {
+				require.NotNil(t, floor)
+				require.NotNil(t, log)
+			}
+		})
+	}
+}
+
 func gzipBytes(t *testing.T, b []byte) []byte {
 	t.Helper()
 	var buf bytes.Buffer