Skip to content

Commit

Permalink
[code health] refactor payload attrs checks in handleSubmitNewBlock (#…
Browse files Browse the repository at this point in the history 
…491)

refactor payload attrs checks to isolated function
  • Loading branch information
@michaelneuder
michaelneuder authored Aug 2, 2023
1 parent 10859e6 commit 2944f4f
Show file tree
Hide file tree
Showing 2 changed files with 189 additions and 31 deletions.
67 changes: 38 additions & 29 deletions services/api/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down
153 changes: 151 additions & 2 deletions services/api/service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 (
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 2944f4f

Please sign in to comment.