-
Notifications
You must be signed in to change notification settings - Fork 267
Fulu support for mev-boost #805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
9726e44
39b6701
0fc7320
d024021
89c1d4a
08e9d5c
eef602e
481f298
b8753f7
866b905
eb43547
acb775d
5e00dcc
0fcdc83
746cd0d
c8eb8ae
50ae7b4
f76a9ac
b7b6d90
45c82c3
a07eaf1
28b0582
58b4935
4aaea35
8a07d50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -15,6 +15,7 @@ import ( | |||||
|
||||||
builderApi "github.com/attestantio/go-builder-client/api" | ||||||
builderApiDeneb "github.com/attestantio/go-builder-client/api/deneb" | ||||||
builderApiFulu "github.com/attestantio/go-builder-client/api/fulu" | ||||||
eth2Api "github.com/attestantio/go-eth2-client/api" | ||||||
eth2ApiV1Bellatrix "github.com/attestantio/go-eth2-client/api/v1/bellatrix" | ||||||
eth2ApiV1Capella "github.com/attestantio/go-eth2-client/api/v1/capella" | ||||||
|
@@ -24,6 +25,7 @@ import ( | |||||
"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/flashbots/mev-boost/common" | ||||||
"github.com/flashbots/mev-boost/config" | ||||||
"github.com/flashbots/mev-boost/server/params" | ||||||
"github.com/flashbots/mev-boost/server/types" | ||||||
|
@@ -36,7 +38,6 @@ var ( | |||||
errInvalidBlockhash = errors.New("invalid blockhash") | ||||||
errInvalidKZGLength = errors.New("invalid KZG commitments length") | ||||||
errInvalidKZG = errors.New("invalid KZG commitment") | ||||||
errFailedToDecode = errors.New("failed to decode payload") | ||||||
errFailedToConvert = errors.New("failed to convert block from SSZ to JSON") | ||||||
) | ||||||
|
||||||
|
@@ -339,31 +340,70 @@ func verifyBlobsBundle(log *logrus.Entry, request *eth2Api.VersionedSignedBlinde | |||||
return err | ||||||
} | ||||||
|
||||||
// Ensure the blobs bundle field counts are correct | ||||||
if len(requestCommitments) != len(responseBlobsBundle.Blobs) || | ||||||
len(requestCommitments) != len(responseBlobsBundle.Commitments) || | ||||||
len(requestCommitments) != len(responseBlobsBundle.Proofs) { | ||||||
// Check commitments | ||||||
responseCommitments, err := responseBlobsBundle.Commitments() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you could still verify blobs here, would save a bit of duplicated code |
||||||
if err != nil { | ||||||
log.WithError(err).Error("failed to get response commitments") | ||||||
return err | ||||||
} | ||||||
if len(requestCommitments) != len(responseCommitments) { | ||||||
log.WithFields(logrus.Fields{ | ||||||
"requestBlobCommitments": len(requestCommitments), | ||||||
"responseBlobs": len(responseBlobsBundle.Blobs), | ||||||
"responseBlobCommitments": len(responseBlobsBundle.Commitments), | ||||||
"responseBlobProofs": len(responseBlobsBundle.Proofs), | ||||||
}).Error("different lengths for blobs/commitments/proofs") | ||||||
"requestBlobCommitments": len(requestCommitments), | ||||||
"responseCommitments": len(responseCommitments), | ||||||
}).Error("different lengths for commitments") | ||||||
return errInvalidKZGLength | ||||||
} | ||||||
|
||||||
// Ensure the request and response KZG commitments are the same | ||||||
for i, commitment := range requestCommitments { | ||||||
if commitment != responseBlobsBundle.Commitments[i] { | ||||||
if commitment != responseCommitments[i] { | ||||||
log.WithFields(logrus.Fields{ | ||||||
"index": i, | ||||||
"requestBlobCommitment": commitment.String(), | ||||||
"responseBlobCommitment": responseBlobsBundle.Commitments[i].String(), | ||||||
"responseBlobCommitment": responseCommitments[i].String(), | ||||||
}).Error("requestBlobCommitment does not equal responseBlobCommitment") | ||||||
return errInvalidKZG | ||||||
} | ||||||
} | ||||||
|
||||||
// Check proofs | ||||||
responseProofs, err := responseBlobsBundle.Proofs() | ||||||
if err != nil { | ||||||
log.WithError(err).Error("failed to get response proofs") | ||||||
return err | ||||||
} | ||||||
|
||||||
if request.Version >= spec.DataVersionFulu { | ||||||
Comment on lines
+428
to
+430
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a nit, I'd remove this blank line. This would be consistent with the commitments & blobs checks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you mean the blank line in like 373? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's right. |
||||||
if len(requestCommitments)*common.CellsPerExtBlob != len(responseProofs) { | ||||||
log.WithFields(logrus.Fields{ | ||||||
"requestBlobCommitments": len(requestCommitments), | ||||||
"responseProofs": len(responseProofs), | ||||||
"cellsPerExtBlob": common.CellsPerExtBlob, | ||||||
}).Error("different lengths for proofs") | ||||||
return errInvalidKZGLength | ||||||
} | ||||||
} else { | ||||||
if len(requestCommitments) != len(responseProofs) { | ||||||
log.WithFields(logrus.Fields{ | ||||||
"requestBlobCommitments": len(requestCommitments), | ||||||
"responseProofs": len(responseProofs), | ||||||
}).Error("different lengths for proofs") | ||||||
|
}).Error("different lengths for proofs") | |
}).Error("wrong lengths for proofs") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicky, but I would change all "different" to "wrong"
jtraglia marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole api is so cursed. out
should just have a type block any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol. I don't think making it any
would be a wise change. It's okay to be explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, the correct way would be an interface that all the different types implement.
Making it explicit is just super ugly.
edit: just how we implement transactions in geth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, that'd be nice! perhaps open an issue to remind us of it later and we do a follow-up PR for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to maintain this for the long term @jtraglia ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. We will remove this when attestant adds support. I don't think we have to block merging this PR on that though.