process cancellable bids below floor correctly (#401)

* process cancellable bids below floor correctly

* fixes

* more cleanup

* fix logic

* simplify

.golangci.yaml

@@ -65,7 +65,7 @@ linters-settings:
     min-complexity: 85 # default: 30
 
   gocyclo:
-    min-complexity: 65 # default: 30
+    min-complexity: 70 # default: 30
 
   gomoddirectives:
     replace-allow-list:

datastore/redis.go

@@ -496,32 +496,36 @@ type SaveBidAndUpdateTopBidResponse struct {
 }
 
 func (r *RedisCache) SaveBidAndUpdateTopBid(payload *common.BuilderSubmitBlockRequest, getPayloadResponse *common.GetPayloadResponse, getHeaderResponse *common.GetHeaderResponse, reqReceivedAt time.Time, isCancellationEnabled bool, floorValue *big.Int) (state SaveBidAndUpdateTopBidResponse, err error) {
-	// 1. Load latest bids for a given slot+parent+proposer
-	keyBidValues := r.keyBlockBuilderLatestBidsValue(payload.Slot(), payload.ParentHash(), payload.ProposerPubkey())
-	bidValueMap, err := r.client.HGetAll(context.Background(), keyBidValues).Result()
+	// Load latest bids for a given slot+parent+proposer
+	builderBids, err := NewBuilderBidsFromRedis(r, payload.Slot(), payload.ParentHash(), payload.ProposerPubkey())
 	if err != nil {
 		return state, err
 	}
 
+	// Load floor value (if not passed in already)
 	if floorValue == nil {
 		floorValue, err = r.GetFloorBidValue(payload.Slot(), payload.ParentHash(), payload.ProposerPubkey())
 		if err != nil {
 			return state, err
 		}
 	}
 
-	builderBids := NewBuilderBids(bidValueMap)
-	_, state.PrevTopBidValue = builderBids.getTopBid()
-	state.TopBidValue = state.PrevTopBidValue
+	// Get the reference top bid value
+	_, state.TopBidValue = builderBids.getTopBid()
+	if floorValue.Cmp(state.TopBidValue) == 1 {
+		state.TopBidValue = floorValue
+	}
+	state.PrevTopBidValue = state.TopBidValue
 
-	// 2. Do we even need to continue / save the new payload and update the top bid?
-	// - In cancellation mode: always continue to saving latest bid
-	// - In non-cancellation mode: only save if current bid is higher value than floor value
-	if !isCancellationEnabled && payload.Value().Cmp(floorValue) < 1 {
+	// Abort now if non-cancellation bid is lower than floor value
+	isBidAboveFloor := payload.Value().Cmp(floorValue) == 1
+	if !isCancellationEnabled && !isBidAboveFloor {
 		return state, nil
 	}
 
+	//
 	// Time to save things in Redis
+	//
 	// 1. Save the execution payload
 	err = r.SaveExecutionPayload(payload.Slot(), payload.ProposerPubkey(), payload.BlockHash(), getPayloadResponse)
 	if err != nil {
@@ -534,26 +538,82 @@ func (r *RedisCache) SaveBidAndUpdateTopBid(payload *common.BuilderSubmitBlockRe
 		return state, err
 	}
 	state.WasBidSaved = true
-
-	// 3. Update this builders latest bid in local cache
 	builderBids.bidValues[payload.BuilderPubkey().String()] = payload.Value()
-	topBidBuilder := ""
-	topBidBuilder, state.TopBidValue = builderBids.getTopBid()
-	keyBidSource := r.keyLatestBidByBuilder(payload.Slot(), payload.ParentHash(), payload.ProposerPubkey(), topBidBuilder)
 
-	// 4. Only proceed to update top bid in redis if it changed in local cache
+	// If top bid value hasn't change, abort now
+	_, state.TopBidValue = builderBids.getTopBid()
 	if state.TopBidValue.Cmp(state.PrevTopBidValue) == 0 {
 		return state, nil
 	}
 
+	//
+	// Update the top bid
+	//
+	state, err = r._UpdateTopBid(state, builderBids, payload.Slot(), payload.ParentHash(), payload.ProposerPubkey(), floorValue)
+	if err != nil {
+		return state, err
+	}
+	state.IsNewTopBid = payload.Value().Cmp(state.TopBidValue) == 0
+
+	//
+	// Check if should set a new bid floor
+	//
+	if !isCancellationEnabled && isBidAboveFloor {
+		keyBidSource := r.keyLatestBidByBuilder(payload.Slot(), payload.ParentHash(), payload.ProposerPubkey(), payload.BuilderPubkey().String())
+		keyFloorBid := r.keyFloorBid(payload.Slot(), payload.ParentHash(), payload.ProposerPubkey())
+		wasCopied, copyErr := r.client.Copy(context.Background(), keyBidSource, keyFloorBid, 0, true).Result()
+		if copyErr != nil {
+			return state, copyErr
+		} else if wasCopied == 0 {
+			return state, fmt.Errorf("could not copy %s to %s", keyBidSource, keyFloorBid) //nolint:goerr113
+		}
+		err = r.client.Expire(context.Background(), keyFloorBid, expiryBidCache).Err()
+		if err != nil {
+			return state, err
+		}
+
+		keyFloorBidValue := r.keyFloorBidValue(payload.Slot(), payload.ParentHash(), payload.ProposerPubkey())
+		err = r.client.Set(context.Background(), keyFloorBidValue, payload.Value().String(), expiryBidCache).Err()
+		if err != nil {
+			return state, err
+		}
+	}
+
+	return state, nil
+}
+
+func (r *RedisCache) _UpdateTopBid(state SaveBidAndUpdateTopBidResponse, builderBids *BuilderBids, slot uint64, parentHash, proposerPubkey string, floorValue *big.Int) (resp SaveBidAndUpdateTopBidResponse, err error) {
+	if builderBids == nil {
+		builderBids, err = NewBuilderBidsFromRedis(r, slot, parentHash, proposerPubkey)
+		if err != nil {
+			return state, err
+		}
+	}
+
+	if len(builderBids.bidValues) == 0 {
+		return state, nil
+	}
+
+	// Load floor value (if not passed in already)
+	if floorValue == nil {
+		floorValue, err = r.GetFloorBidValue(slot, parentHash, proposerPubkey)
+		if err != nil {
+			return state, err
+		}
+	}
+
+	topBidBuilder := ""
+	topBidBuilder, state.TopBidValue = builderBids.getTopBid()
+	keyBidSource := r.keyLatestBidByBuilder(slot, parentHash, proposerPubkey, topBidBuilder)
+
 	// If floor value is higher than this bid, use floor bid instead
 	if floorValue.Cmp(state.TopBidValue) == 1 {
 		state.TopBidValue = floorValue
-		keyBidSource = r.keyFloorBid(payload.Slot(), payload.ParentHash(), payload.ProposerPubkey())
+		keyBidSource = r.keyFloorBid(slot, parentHash, proposerPubkey)
 	}
 
-	// 5. Copy winning bid to top bid cache
-	keyTopBid := r.keyCacheGetHeaderResponse(payload.Slot(), payload.ParentHash(), payload.ProposerPubkey())
+	// Copy winning bid to top bid cache
+	keyTopBid := r.keyCacheGetHeaderResponse(slot, parentHash, proposerPubkey)
 	wasCopied, err := r.client.Copy(context.Background(), keyBidSource, keyTopBid, 0, true).Result()
 	if err != nil {
 		return state, err
@@ -565,35 +625,15 @@ func (r *RedisCache) SaveBidAndUpdateTopBid(payload *common.BuilderSubmitBlockRe
 		return state, err
 	}
 
-	state.WasTopBidUpdated = state.PrevTopBidValue.Cmp(state.TopBidValue) != 0
-	state.IsNewTopBid = payload.Value().Cmp(state.TopBidValue) == 0
+	state.WasTopBidUpdated = state.PrevTopBidValue == nil || state.PrevTopBidValue.Cmp(state.TopBidValue) != 0
 
 	// 6. Finally, update the global top bid value
-	keyTopBidValue := r.keyTopBidValue(payload.Slot(), payload.ParentHash(), payload.ProposerPubkey())
+	keyTopBidValue := r.keyTopBidValue(slot, parentHash, proposerPubkey)
 	err = r.client.Set(context.Background(), keyTopBidValue, state.TopBidValue.String(), expiryBidCache).Err()
 	if err != nil {
 		return state, err
 	}
 
-	// 7. If non-cancelling, perhaps set a new bid floor
-	if !isCancellationEnabled && payload.Value().Cmp(floorValue) == 1 {
-		keyBidSource := r.keyLatestBidByBuilder(payload.Slot(), payload.ParentHash(), payload.ProposerPubkey(), payload.BuilderPubkey().String())
-		keyFloorBid := r.keyFloorBid(payload.Slot(), payload.ParentHash(), payload.ProposerPubkey())
-		wasCopied, copyErr := r.client.Copy(context.Background(), keyBidSource, keyFloorBid, 0, true).Result()
-		if copyErr != nil {
-			return state, copyErr
-		} else if wasCopied == 0 {
-			return state, fmt.Errorf("could not copy %s to %s", keyBidSource, keyTopBid) //nolint:goerr113
-		}
-		err = r.client.Expire(context.Background(), keyFloorBid, expiryBidCache).Err()
-		if err != nil {
-			return state, err
-		}
-
-		keyFloorBidValue := r.keyFloorBidValue(payload.Slot(), payload.ParentHash(), payload.ProposerPubkey())
-		err = r.client.Set(context.Background(), keyFloorBidValue, payload.Value().String(), expiryBidCache).Err()
-	}
-
 	return state, err
 }
 
@@ -627,6 +667,28 @@ func (r *RedisCache) GetBuilderLatestValue(slot uint64, parentHash, proposerPubk
 	return topBidValue, nil
 }
 
+// DelBuilderBid removes a builders most recent bid
+func (r *RedisCache) DelBuilderBid(slot uint64, parentHash, proposerPubkey, builderPubkey string) (err error) {
+	// delete the value
+	keyLatestValue := r.keyBlockBuilderLatestBidsValue(slot, parentHash, proposerPubkey)
+	err = r.client.HDel(context.Background(), keyLatestValue, builderPubkey).Err()
+	if err != nil && !errors.Is(err, redis.Nil) {
+		return err
+	}
+
+	// delete the time
+	keyLatestBidsTime := r.keyBlockBuilderLatestBidsTime(slot, parentHash, proposerPubkey)
+	err = r.client.HDel(context.Background(), keyLatestBidsTime, builderPubkey).Err()
+	if err != nil {
+		return err
+	}
+
+	// update bids now to compute current top bid
+	state := SaveBidAndUpdateTopBidResponse{} //nolint:exhaustruct
+	_, err = r._UpdateTopBid(state, nil, slot, parentHash, proposerPubkey, nil)
+	return err
+}
+
 // GetFloorBidValue returns the value of the highest non-cancellable bid
 func (r *RedisCache) GetFloorBidValue(slot uint64, parentHash, proposerPubkey string) (floorValue *big.Int, err error) {
 	keyFloorBidValue := r.keyFloorBidValue(slot, parentHash, proposerPubkey)

datastore/redis_test.go

@@ -243,6 +243,10 @@ func TestBuilderBids(t *testing.T) {
 		require.Equal(t, big.NewInt(expectedValue), floorValue)
 	}
 
+	// deleting a bid that doesn't exist should not error
+	err := cache.DelBuilderBid(slot, parentHash, proposerPubkey, bApubkey)
+	require.NoError(t, err)
+
 	// submit ba1=10
 	payload, getPayloadResp, getHeaderResp := common.CreateTestBlockSubmission(t, bApubkey, big.NewInt(10), &opts)
 	resp, err := cache.SaveBidAndUpdateTopBid(payload, getPayloadResp, getHeaderResp, time.Now(), false, nil)
@@ -254,15 +258,23 @@ func TestBuilderBids(t *testing.T) {
 	ensureBestBidValueEquals(10, bApubkey)
 	ensureBidFloor(10)
 
-	// submit ba2=5 (should not update)
+	// deleting ba1
+	err = cache.DelBuilderBid(slot, parentHash, proposerPubkey, bApubkey)
+	require.NoError(t, err)
+
+	// best bid and floor should still exist, because it was the floor bid
+	ensureBestBidValueEquals(10, "")
+	ensureBidFloor(10)
+
+	// submit ba2=5 (should not update, because floor is 10)
 	payload, getPayloadResp, getHeaderResp = common.CreateTestBlockSubmission(t, bApubkey, big.NewInt(5), &opts)
 	resp, err = cache.SaveBidAndUpdateTopBid(payload, getPayloadResp, getHeaderResp, time.Now(), false, nil)
 	require.NoError(t, err)
 	require.False(t, resp.WasBidSaved, resp)
 	require.False(t, resp.WasTopBidUpdated)
 	require.False(t, resp.IsNewTopBid)
 	require.Equal(t, big.NewInt(10), resp.TopBidValue)
-	ensureBestBidValueEquals(10, bApubkey)
+	ensureBestBidValueEquals(10, "")
 	ensureBidFloor(10)
 
 	// submit ba3c=5 (should not update, because floor is 10)

datastore/utils.go

@@ -1,6 +1,7 @@
 package datastore
 
 import (
+	"context"
 	"math/big"
 )
 
@@ -9,6 +10,15 @@ type BuilderBids struct {
 	bidValues map[string]*big.Int
 }
 
+func NewBuilderBidsFromRedis(r *RedisCache, slot uint64, parentHash, proposerPubkey string) (*BuilderBids, error) {
+	keyBidValues := r.keyBlockBuilderLatestBidsValue(slot, parentHash, proposerPubkey)
+	bidValueMap, err := r.client.HGetAll(context.Background(), keyBidValues).Result()
+	if err != nil {
+		return nil, err
+	}
+	return NewBuilderBids(bidValueMap), nil
+}
+
 func NewBuilderBids(bidValueMap map[string]string) *BuilderBids {
 	b := BuilderBids{
 		bidValues: make(map[string]*big.Int),

services/api/service.go

@@ -841,6 +841,11 @@ func (api *RelayAPI) RespondMsg(w http.ResponseWriter, code int, msg string) {
 func (api *RelayAPI) Respond(w http.ResponseWriter, code int, response any) {
 	w.Header().Set("Content-Type", "application/json")
 	w.WriteHeader(code)
+	if response == nil {
+		return
+	}
+
+	// write the json response
 	if err := json.NewEncoder(w).Encode(response); err != nil {
 		api.log.WithField("response", response).WithError(err).Error("Couldn't write response")
 		http.Error(w, "", http.StatusInternalServerError)
@@ -1787,19 +1792,28 @@ func (api *RelayAPI) handleSubmitNewBlock(w http.ResponseWriter, req *http.Reque
 	if err != nil {
 		log.WithError(err).Error("failed to get floor bid value from redis")
 	} else {
-		isBidAboveFloor := payload.Value().Cmp(floorBidValue) == 1
-		log = log.WithFields(logrus.Fields{
-			"floorBidValue":   floorBidValue.String(),
-			"isBidAboveFloor": isBidAboveFloor,
-		})
+		log = log.WithField("floorBidValue", floorBidValue.String())
+	}
 
-		// Without cancellations, discard bids below floor value
-		if !isCancellationEnabled && !isBidAboveFloor {
-			simResultC <- &blockSimResult{false, false, nil, nil}
-			log.Info("ignoring submission without cancellation and below floor bid value")
-			api.RespondMsg(w, http.StatusAccepted, "ignoring submission without cancellation and below floor bid value")
+	// Check if submission can be skipped (if it's 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(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
 	}
 
 	// Get the latest top bid value from Redis
@@ -1930,7 +1944,7 @@ func (api *RelayAPI) handleSubmitNewBlock(w http.ResponseWriter, req *http.Reque
 	updateBidResult, err := api.redis.SaveBidAndUpdateTopBid(payload, getPayloadResponse, getHeaderResponse, receivedAt, isCancellationEnabled, floorBidValue)
 	if err != nil {
 		log.WithError(err).Error("could not save bid and update top bids")
-		api.RespondError(w, http.StatusInternalServerError, err.Error())
+		api.RespondError(w, http.StatusInternalServerError, "failed saving and updating bid")
 		return
 	}