Skip to content
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

Remove Fulu block and state. #14905

Merged
merged 3 commits into from
Feb 14, 2025
Merged

Remove Fulu block and state. #14905

merged 3 commits into from
Feb 14, 2025

Conversation

nalepae
Copy link
Contributor

@nalepae nalepae commented Feb 10, 2025

What type of PR is this?
Other

What does this PR do? Why is it needed?
Remove as much as Fulu proto definitions. (Including Fulu block and state.)

How I realized this PR:

  1. I first removed all Fulu related proto definitions
  2. I fixed in the codebase new compile time errors (because the code was still using the removed Fulu structs.)
  3. I ran tests, and some errors appeared, because, for example, blk.Version returned electra for a Fulu block.
  4. In order to fix3. (being able to differentiate versions), I had to re-add some Fulu structs, even if they are the same than the Electra ones. Only the minimal amount of structs have been added.
  5. Additionally to unit tests and E2E tests, I ran Kurtosis devnets ensuring that we are able to sync, from Deneb to Fulu, with a finalized and unfinalized network, with both validators clients using gRPC and the beacon API.

Acknowledgements

@nalepae nalepae force-pushed the remove-fulu-block-state branch from b037b20 to 7c00886 Compare February 10, 2025 12:26
@nalepae nalepae marked this pull request as ready for review February 10, 2025 12:35
@nalepae nalepae requested a review from a team as a code owner February 10, 2025 12:35
@nalepae nalepae force-pushed the remove-fulu-block-state branch 2 times, most recently from 0b3075a to 5c8bcee Compare February 10, 2025 13:01
@nalepae nalepae marked this pull request as draft February 11, 2025 13:47
@nalepae nalepae force-pushed the remove-fulu-block-state branch 11 times, most recently from 1afc93c to f7774b4 Compare February 12, 2025 16:22
@nalepae nalepae force-pushed the remove-fulu-block-state branch from f7774b4 to a2d3db0 Compare February 12, 2025 16:23
@nalepae nalepae marked this pull request as ready for review February 12, 2025 17:32
@@ -636,10 +556,10 @@ func ProtobufBeaconStateElectra(s interface{}) (*ethpb.BeaconStateElectra, error

// ProtobufBeaconStateFulu transforms an input into beacon state Fulu in the form of protobuf.
// Error is returned if the input is not type protobuf beacon state.
func ProtobufBeaconStateFulu(s interface{}) (*ethpb.BeaconStateFulu, error) {
pbState, ok := s.(*ethpb.BeaconStateFulu)
func ProtobufBeaconStateFulu(s interface{}) (*ethpb.BeaconStateElectra, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this function needed? Can't we alias ProtobufBeaconStateElectra?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 60ad5fa.

// Execution requests.
ethereum.engine.v1.ExecutionRequests execution_requests = 13;
}

message SignedBlindedBeaconBlockFulu {
// The unsigned blinded beacon block itself.
BlindedBeaconBlockFulu message = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find the definition for some reason, but is BlindedBeaconBlockFulu needed? You did remove the full block

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The definition of BlindedBeaconBlockFulu is here.

Copy link
Contributor Author

@nalepae nalepae Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • In produceBlockV3, to produce a non blinded block, we need to compare type against *eth.GenericBeaconBlock_Fulu.
  • *eth.GenericBeaconBlock_Fulu is created here and is of type BeaconBlockContentsFulu (and not of type BeaconBlockFulu).

  • In produceBlockV3, to produce a blinded block, we need to compare type against *eth.GenericBeaconBlock_BlindedFulu.
  • *eth.GenericBeaconBlock_BlindedFulu is created here and is of type BlindedBeaconBlockFulu.

==> I need both BlindedBeaconBlockFulu and BeaconBlockContentsFulu, but I don't need BlindedBeaconBlockFulu.

Copy link
Contributor Author

@nalepae nalepae Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do all this to set version.String(version.Fulu)) on Fulu blocks.
(Even if the shape of the struct is exactly the same than Electra block.)

BlobKzgCommitments []string `json:"blob_kzg_commitments"`
ExecutionRequests *ExecutionRequests `json:"execution_requests"`
}

type BlindedBeaconBlockFulu struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? Se comment in proto/prysm/v1alpha1/beacon_block.proto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nalepae nalepae added this pull request to the merge queue Feb 14, 2025
Merged via the queue into develop with commit 215fbcb Feb 14, 2025
17 checks passed
@nalepae nalepae deleted the remove-fulu-block-state branch February 14, 2025 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants