Skip to content

Commit

Permalink
[code health] refactor floor bid checks to independent function (#513)
Browse files Browse the repository at this point in the history 
refactor floor bid checks into separate function
  • Loading branch information
@michaelneuder
michaelneuder authored Aug 30, 2023
1 parent 5dc019f commit dabae5e
Show file tree
Hide file tree
Showing 3 changed files with 160 additions and 45 deletions.
7 changes: 7 additions & 0 deletions datastore/redis.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
111 changes: 66 additions & 45 deletions services/api/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1596,6 +1596,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
Expand Down Expand Up @@ -1765,24 +1818,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
Expand All @@ -1806,37 +1858,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
// ---------------------------------
Expand Down
87 changes: 87 additions & 0 deletions services/api/service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit dabae5e

Please sign in to comment.