Fulu support for mev-boost #805
Conversation
There was a problem hiding this comment.
Thanks @bharath-123! Looks great. Just a few very small nits.
common/common.go
Outdated
| FieldElementsPerBlob = 4096 | ||
| // BlobExpansionFactor is the factor by which we extend a blob for PeerDas. | ||
| BlobExpansionFactor = 2 | ||
| // FieldElementsPerCell is the number of field elements in a cell |
There was a problem hiding this comment.
| // FieldElementsPerCell is the number of field elements in a cell | |
| // FieldElementsPerCell is the number of field elements in a cell. |
| } | ||
|
|
||
| if request.Version >= spec.DataVersionFulu { |
There was a problem hiding this comment.
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.
do you mean the blank line in like 373?
common/common.go
Outdated
| @@ -10,6 +10,16 @@ import ( | |||
|
|
|||
| const ( | |||
| SlotTimeSecMainnet = 12 | |||
| // FieldElementsPerBlob is the number of field elements needed to represent a blob. | |||
| FieldElementsPerBlob = 4096 | |||
| // BlobExpansionFactor is the factor by which we extend a blob for PeerDas. | |||
There was a problem hiding this comment.
| // BlobExpansionFactor is the factor by which we extend a blob for PeerDas. | |
| // BlobExpansionFactor is the factor by which we extend a blob for PeerDAS. |
|
@metachris is on vacation for the next few weeks. I will merge this after we get approvals from at least 2 of the following: |
| @@ -60,3 +60,5 @@ require ( | |||
| gopkg.in/yaml.v2 v2.4.0 // indirect | |||
| gopkg.in/yaml.v3 v3.0.1 // indirect | |||
| ) | |||
|
|
|||
| replace github.com/attestantio/go-builder-client => github.com/jtraglia/go-builder-client v0.4.6-0.20250410195459-42a3ff7f1546 | |||
There was a problem hiding this comment.
Do you want to maintain this for the long term @jtraglia ?
There was a problem hiding this comment.
No. We will remove this when attestant adds support. I don't think we have to block merging this PR on that though.
server/get_payload.go
Outdated
| log.WithFields(logrus.Fields{ | ||
| "requestBlobCommitments": len(requestCommitments), | ||
| "responseProofs": len(responseProofs), | ||
| }).Error("different lengths for proofs") |
There was a problem hiding this comment.
| }).Error("different lengths for proofs") | |
| }).Error("wrong lengths for proofs") |
There was a problem hiding this comment.
Nitpicky, but I would change all "different" to "wrong"
| len(requestCommitments) != len(responseBlobsBundle.Commitments) || | ||
| len(requestCommitments) != len(responseBlobsBundle.Proofs) { | ||
| // Check commitments | ||
| responseCommitments, err := responseBlobsBundle.Commitments() |
There was a problem hiding this comment.
you could still verify blobs here, would save a bit of duplicated code
| block := new(eth2ApiV1Deneb.SignedBlindedBeaconBlock) | ||
| if err = json.Unmarshal(in, block); err != nil { | ||
| return err | ||
| } | ||
| out.Version = spec.DataVersionDeneb | ||
| out.Deneb = block |
There was a problem hiding this comment.
This whole api is so cursed. out should just have a type block any
There was a problem hiding this comment.
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.
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
| numBlobs := len(kzgCommitments) | ||
| commitments := make([]deneb.KZGCommitment, numBlobs) | ||
| copy(commitments, kzgCommitments) | ||
| // For testing, proofs and blobs are not populated |
There was a problem hiding this comment.
it would be a good idea to have some default values
server/service_test.go
Outdated
| commitments := make([]deneb.KZGCommitment, numBlobs) | ||
| copy(commitments, kzgCommitments) | ||
| // For testing, proofs and blobs are not populated | ||
| proofs := make([]deneb.KZGProof, numBlobs) |
There was a problem hiding this comment.
Shouldn't this be numBlobs* CellsPerExtBlob
📝 Summary
This PR contains the changes required for the upcoming Fulu hard fork.
These changes have been tested locally against rustic-builder using kurtosis
⛱ Motivation and Context
We need to still merge the fulu changes in
github.com/jtraglia/go-builder-clientto main before merging this.📚 References
✅ I have run these commands
make lintmake test-racego mod tidy