From 65e141752d7c15daf7864c1a842c20df4c65eb39 Mon Sep 17 00:00:00 2001 From: Michael Neuder Date: Tue, 1 Aug 2023 15:05:55 -0400 Subject: [PATCH 1/4] refactor payload attrs checks to isolated function --- services/api/service.go | 67 ++++++++------- services/api/service_test.go | 153 ++++++++++++++++++++++++++++++++++- 2 files changed, 189 insertions(+), 31 deletions(-) diff --git a/services/api/service.go b/services/api/service.go index 8a58c88d..1ebace74 100644 --- a/services/api/service.go +++ b/services/api/service.go @@ -1502,6 +1502,42 @@ func (api *RelayAPI) checkSubmissionFeeRecipient(w http.ResponseWriter, log *log return slotDuty.Entry.Message.GasLimit, true } +func (api *RelayAPI) checkSubmissionPayloadAttrs(w http.ResponseWriter, log *logrus.Entry, payload *common.BuilderSubmitBlockRequest) bool { + api.payloadAttributesLock.RLock() + attrs, ok := api.payloadAttributes[payload.ParentHash()] + api.payloadAttributesLock.RUnlock() + if !ok || payload.Slot() != attrs.slot { + log.Warn("payload attributes not (yet) known") + api.RespondError(w, http.StatusBadRequest, "payload attributes not (yet) known") + return false + } + + if payload.Random() != attrs.payloadAttributes.PrevRandao { + msg := fmt.Sprintf("incorrect prev_randao - got: %s, expected: %s", payload.Random(), attrs.payloadAttributes.PrevRandao) + log.Info(msg) + api.RespondError(w, http.StatusBadRequest, msg) + return false + } + + if hasReachedFork(payload.Slot(), api.capellaEpoch) { // Capella requires correct withdrawals + withdrawalsRoot, err := ComputeWithdrawalsRoot(payload.Withdrawals()) + if err != nil { + log.WithError(err).Warn("could not compute withdrawals root from payload") + api.RespondError(w, http.StatusBadRequest, "could not compute withdrawals root") + return false + } + + if withdrawalsRoot != attrs.withdrawalsRoot { + msg := fmt.Sprintf("incorrect withdrawals root - got: %s, expected: %s", withdrawalsRoot.String(), attrs.withdrawalsRoot.String()) + log.Info(msg) + api.RespondError(w, http.StatusBadRequest, msg) + return false + } + } + + return true +} + func (api *RelayAPI) handleSubmitNewBlock(w http.ResponseWriter, req *http.Request) { //nolint:gocognit,maintidx var pf common.Profile var prevTime, nextTime time.Time @@ -1685,38 +1721,11 @@ func (api *RelayAPI) handleSubmitNewBlock(w http.ResponseWriter, req *http.Reque log = log.WithField("timestampBeforeAttributesCheck", time.Now().UTC().UnixMilli()) - api.payloadAttributesLock.RLock() - attrs, ok := api.payloadAttributes[payload.ParentHash()] - api.payloadAttributesLock.RUnlock() - if !ok || payload.Slot() != attrs.slot { - log.Warn("payload attributes not (yet) known") - api.RespondError(w, http.StatusBadRequest, "payload attributes not (yet) known") - return - } - - if payload.Random() != attrs.payloadAttributes.PrevRandao { - msg := fmt.Sprintf("incorrect prev_randao - got: %s, expected: %s", payload.Random(), attrs.payloadAttributes.PrevRandao) - log.Info(msg) - api.RespondError(w, http.StatusBadRequest, msg) + cont = api.checkSubmissionPayloadAttrs(w, log, payload) + if !cont { return } - if hasReachedFork(payload.Slot(), api.capellaEpoch) { // Capella requires correct withdrawals - withdrawalsRoot, err := ComputeWithdrawalsRoot(payload.Withdrawals()) - if err != nil { - log.WithError(err).Warn("could not compute withdrawals root from payload") - api.RespondError(w, http.StatusBadRequest, "could not compute withdrawals root") - return - } - - if withdrawalsRoot != attrs.withdrawalsRoot { - msg := fmt.Sprintf("incorrect withdrawals root - got: %s, expected: %s", withdrawalsRoot.String(), attrs.withdrawalsRoot.String()) - log.Info(msg) - api.RespondError(w, http.StatusBadRequest, msg) - return - } - } - // Verify the signature log = log.WithField("timestampBeforeSignatureCheck", time.Now().UTC().UnixMilli()) signature := payload.Signature() diff --git a/services/api/service_test.go b/services/api/service_test.go index a23dccb9..882a0015 100644 --- a/services/api/service_test.go +++ b/services/api/service_test.go @@ -16,6 +16,7 @@ import ( builderCapella "github.com/attestantio/go-builder-client/api/capella" v1 "github.com/attestantio/go-builder-client/api/v1" "github.com/attestantio/go-eth2-client/spec/bellatrix" + "github.com/attestantio/go-eth2-client/spec/capella" "github.com/attestantio/go-eth2-client/spec/phase0" "github.com/ethereum/go-ethereum/common/hexutil" "github.com/flashbots/go-boost-utils/bls" @@ -30,8 +31,11 @@ import ( ) const ( - testGasLimit = uint64(30000000) - testSlot = uint64(42) + testGasLimit = uint64(30000000) + testSlot = uint64(42) + testParentHash = "0xbd3291854dc822b7ec585925cda0e18f06af28fa2886e15f52d52dd4b6f94ed6" + testWithdrawalsRoot = "0x7f6d156912a4cb1e74ee37e492ad883f7f7ac856d987b3228b517e490aa0189e" + testPrevRandao = "0x9962816e9d0a39fd4c80935338a741dc916d1545694e41eb5a505e1a3098f9e4" ) var ( @@ -580,6 +584,151 @@ func TestCheckSubmissionFeeRecipient(t *testing.T) { } } +func TestCheckSubmissionPayloadAttrs(t *testing.T) { + withdrawalsRoot, err := hexutil.Decode(testWithdrawalsRoot) + require.NoError(t, err) + prevRandao, err := hexutil.Decode(testPrevRandao) + require.NoError(t, err) + parentHash, err := hexutil.Decode(testParentHash) + require.NoError(t, err) + + cases := []struct { + description string + attrs payloadAttributesHelper + payload *common.BuilderSubmitBlockRequest + expectCont bool + }{ + { + description: "success", + attrs: payloadAttributesHelper{ + slot: testSlot, + parentHash: testParentHash, + withdrawalsRoot: phase0.Root(withdrawalsRoot), + payloadAttributes: beaconclient.PayloadAttributes{ + PrevRandao: testPrevRandao, + }, + }, + payload: &common.BuilderSubmitBlockRequest{ + Capella: &builderCapella.SubmitBlockRequest{ + ExecutionPayload: &capella.ExecutionPayload{ + PrevRandao: [32]byte(prevRandao), + Withdrawals: []*capella.Withdrawal{ + { + Index: 989694, + }, + }, + }, + Message: &v1.BidTrace{ + Slot: testSlot, + ParentHash: phase0.Hash32(parentHash), + }, + }, + }, + expectCont: true, + }, + { + description: "failure_attrs_not_known", + attrs: payloadAttributesHelper{ + slot: testSlot, + }, + payload: &common.BuilderSubmitBlockRequest{ + Capella: &builderCapella.SubmitBlockRequest{ + Message: &v1.BidTrace{ + Slot: testSlot + 1, // submission for a future slot + }, + }, + }, + expectCont: false, + }, + { + description: "failure_wrong_prev_randao", + attrs: payloadAttributesHelper{ + slot: testSlot, + payloadAttributes: beaconclient.PayloadAttributes{ + PrevRandao: testPrevRandao, + }, + }, + payload: &common.BuilderSubmitBlockRequest{ + Capella: &builderCapella.SubmitBlockRequest{ + Message: &v1.BidTrace{ + Slot: testSlot, + ParentHash: phase0.Hash32(parentHash), + }, + ExecutionPayload: &capella.ExecutionPayload{ + PrevRandao: [32]byte(parentHash), // use a different hash to cause an error + }, + }, + }, + expectCont: false, + }, + { + description: "failure_nil_withdrawals", + attrs: payloadAttributesHelper{ + slot: testSlot, + payloadAttributes: beaconclient.PayloadAttributes{ + PrevRandao: testPrevRandao, + }, + }, + payload: &common.BuilderSubmitBlockRequest{ + Capella: &builderCapella.SubmitBlockRequest{ + Message: &v1.BidTrace{ + Slot: testSlot, + ParentHash: phase0.Hash32(parentHash), + }, + ExecutionPayload: &capella.ExecutionPayload{ + PrevRandao: [32]byte(prevRandao), + Withdrawals: nil, // set to nil to cause an error + }, + }, + }, + expectCont: false, + }, + { + description: "failure_wrong_withdrawal_root", + attrs: payloadAttributesHelper{ + slot: testSlot, + parentHash: testParentHash, + withdrawalsRoot: phase0.Root(prevRandao), // use different root to cause an error + payloadAttributes: beaconclient.PayloadAttributes{ + PrevRandao: testPrevRandao, + }, + }, + payload: &common.BuilderSubmitBlockRequest{ + Capella: &builderCapella.SubmitBlockRequest{ + ExecutionPayload: &capella.ExecutionPayload{ + PrevRandao: [32]byte(prevRandao), + Withdrawals: []*capella.Withdrawal{ + { + Index: 989694, + }, + }, + }, + Message: &v1.BidTrace{ + Slot: testSlot, + ParentHash: phase0.Hash32(parentHash), + }, + }, + }, + expectCont: false, + }, + } + for _, tc := range cases { + t.Run(tc.description, func(t *testing.T) { + _, _, backend := startTestBackend(t) + backend.relay.payloadAttributesLock.RLock() + backend.relay.payloadAttributes[testParentHash] = tc.attrs + backend.relay.payloadAttributesLock.RUnlock() + backend.relay.capellaEpoch = 1 + + w := httptest.NewRecorder() + logger := logrus.New() + log := logrus.NewEntry(logger) + cont := backend.relay.checkSubmissionPayloadAttrs(w, log, tc.payload) + require.Equal(t, tc.expectCont, cont) + }) + } +} + func gzipBytes(t *testing.T, b []byte) []byte { t.Helper() var buf bytes.Buffer From 76125ba1e99ffd1ad132b27e4eaf25916161f914 Mon Sep 17 00:00:00 2001 From: Michael Neuder Date: Wed, 2 Aug 2023 10:31:51 -0400 Subject: [PATCH 2/4] refactor slot details checks to isolated function --- 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..1a651ea2 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, "not 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 From d185468b2807426c6a2fa1fcf7be6dde8035299b Mon Sep 17 00:00:00 2001 From: Michael Neuder Date: Wed, 2 Aug 2023 11:00:59 -0400 Subject: [PATCH 3/4] non-capella nit --- services/api/service.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/api/service.go b/services/api/service.go index 1a651ea2..32377192 100644 --- a/services/api/service.go +++ b/services/api/service.go @@ -1541,8 +1541,8 @@ func (api *RelayAPI) checkSubmissionPayloadAttrs(w http.ResponseWriter, log *log 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, "not capella payload") + log.Info("rejecting submission - non-capella payload for capella fork") + api.RespondError(w, http.StatusBadRequest, "non-capella payload") return false } From 566a20294f77bc9fdc079951172e19849b3159cf Mon Sep 17 00:00:00 2001 From: Michael Neuder Date: Thu, 3 Aug 2023 15:07:13 -0400 Subject: [PATCH 4/4] s/cont/ok/g --- services/api/service.go | 12 ++++++------ services/api/service_test.go | 36 ++++++++++++++++++------------------ 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/services/api/service.go b/services/api/service.go index 32377192..ff980db1 100644 --- a/services/api/service.go +++ b/services/api/service.go @@ -1671,8 +1671,8 @@ func (api *RelayAPI) handleSubmitNewBlock(w http.ResponseWriter, req *http.Reque return } - cont := api.checkSubmissionSlotDetails(w, log, headSlot, payload) - if !cont { + ok := api.checkSubmissionSlotDetails(w, log, headSlot, payload) + if !ok { return } @@ -1708,8 +1708,8 @@ func (api *RelayAPI) handleSubmitNewBlock(w http.ResponseWriter, req *http.Reque log = log.WithField("timestampAfterChecks1", time.Now().UTC().UnixMilli()) - gasLimit, cont := api.checkSubmissionFeeRecipient(w, log, payload) - if !cont { + gasLimit, ok := api.checkSubmissionFeeRecipient(w, log, payload) + if !ok { return } @@ -1730,8 +1730,8 @@ func (api *RelayAPI) handleSubmitNewBlock(w http.ResponseWriter, req *http.Reque log = log.WithField("timestampBeforeAttributesCheck", time.Now().UTC().UnixMilli()) - cont = api.checkSubmissionPayloadAttrs(w, log, payload) - if !cont { + ok = api.checkSubmissionPayloadAttrs(w, log, payload) + if !ok { return } diff --git a/services/api/service_test.go b/services/api/service_test.go index 2f3549a9..2b5ebb52 100644 --- a/services/api/service_test.go +++ b/services/api/service_test.go @@ -508,7 +508,7 @@ func TestCheckSubmissionFeeRecipient(t *testing.T) { description string slotDuty *common.BuilderGetValidatorsResponseEntry payload *common.BuilderSubmitBlockRequest - expectCont bool + expectOk bool expectGasLimit uint64 }{ { @@ -529,7 +529,7 @@ func TestCheckSubmissionFeeRecipient(t *testing.T) { }, }, }, - expectCont: true, + expectOk: true, expectGasLimit: testGasLimit, }, { @@ -542,7 +542,7 @@ func TestCheckSubmissionFeeRecipient(t *testing.T) { }, }, }, - expectCont: false, + expectOk: false, expectGasLimit: 0, }, { @@ -563,7 +563,7 @@ func TestCheckSubmissionFeeRecipient(t *testing.T) { }, }, }, - expectCont: false, + expectOk: false, expectGasLimit: 0, }, } @@ -579,7 +579,7 @@ func TestCheckSubmissionFeeRecipient(t *testing.T) { log := logrus.NewEntry(logger) gasLimit, cont := backend.relay.checkSubmissionFeeRecipient(w, log, tc.payload) require.Equal(t, tc.expectGasLimit, gasLimit) - require.Equal(t, tc.expectCont, cont) + require.Equal(t, tc.expectOk, cont) }) } } @@ -596,7 +596,7 @@ func TestCheckSubmissionPayloadAttrs(t *testing.T) { description string attrs payloadAttributesHelper payload *common.BuilderSubmitBlockRequest - expectCont bool + expectOk bool }{ { description: "success", @@ -624,7 +624,7 @@ func TestCheckSubmissionPayloadAttrs(t *testing.T) { }, }, }, - expectCont: true, + expectOk: true, }, { description: "failure_attrs_not_known", @@ -638,7 +638,7 @@ func TestCheckSubmissionPayloadAttrs(t *testing.T) { }, }, }, - expectCont: false, + expectOk: false, }, { description: "failure_wrong_prev_randao", @@ -659,7 +659,7 @@ func TestCheckSubmissionPayloadAttrs(t *testing.T) { }, }, }, - expectCont: false, + expectOk: false, }, { description: "failure_nil_withdrawals", @@ -681,7 +681,7 @@ func TestCheckSubmissionPayloadAttrs(t *testing.T) { }, }, }, - expectCont: false, + expectOk: false, }, { description: "failure_wrong_withdrawal_root", @@ -709,7 +709,7 @@ func TestCheckSubmissionPayloadAttrs(t *testing.T) { }, }, }, - expectCont: false, + expectOk: false, }, } for _, tc := range cases { @@ -724,7 +724,7 @@ func TestCheckSubmissionPayloadAttrs(t *testing.T) { logger := logrus.New() log := logrus.NewEntry(logger) cont := backend.relay.checkSubmissionPayloadAttrs(w, log, tc.payload) - require.Equal(t, tc.expectCont, cont) + require.Equal(t, tc.expectOk, cont) }) } } @@ -733,7 +733,7 @@ func TestCheckSubmissionSlotDetails(t *testing.T) { cases := []struct { description string payload *common.BuilderSubmitBlockRequest - expectCont bool + expectOk bool }{ { description: "success", @@ -747,14 +747,14 @@ func TestCheckSubmissionSlotDetails(t *testing.T) { }, }, }, - expectCont: true, + expectOk: true, }, { description: "failure_nil_capella", payload: &common.BuilderSubmitBlockRequest{ Capella: nil, // nil to cause error }, - expectCont: false, + expectOk: false, }, { description: "failure_past_slot", @@ -765,7 +765,7 @@ func TestCheckSubmissionSlotDetails(t *testing.T) { }, }, }, - expectCont: false, + expectOk: false, }, { description: "failure_wrong_timestamp", @@ -779,7 +779,7 @@ func TestCheckSubmissionSlotDetails(t *testing.T) { }, }, }, - expectCont: false, + expectOk: false, }, } for _, tc := range cases { @@ -791,7 +791,7 @@ func TestCheckSubmissionSlotDetails(t *testing.T) { logger := logrus.New() log := logrus.NewEntry(logger) cont := backend.relay.checkSubmissionSlotDetails(w, log, headSlot, tc.payload) - require.Equal(t, tc.expectCont, cont) + require.Equal(t, tc.expectOk, cont) }) } }