Add Fulu fork boilerplate by nalepae · Pull Request #14771 · OffchainLabs/prysm

nalepae · 2025-01-03T09:44:12Z

What type of PR is this?
Other

What does this PR do? Why is it needed?
This PR implements the Fulu fork Boilerplate.
It does not implement any Fulu feature.
Within this PR, the Fulu fork behaves exactly as the Electra Fork.

Other notes for review
For an complete, optimal, and enjoyable experience, please read commit by commit.

The current state of the Fulu specification does not modify neither the beacon state nor the beacon block.
However, this pull request does add a new Fulu beacon state and beacon block definition, which are identical to Electra beacon state and block.

There is no benefit to copy the beacon block and state declaration if there is no change in the Fulu update.
However, I'm pretty confident, knowing the Ethereum history, that before the Fulu fork effectively happens, at least one EIP will be introduced needing at least one beacon state/block modified/added field.

I won't launch a polymarket about that, but all the previous forks both modified the Beacon block and the Beacon state ...
So I chose to do the heavy lifting work now so nobody has to do it later.

If, finally no change occurs the Beacon Block or State, we will just have to delete the proto files, re-run code generation, and quickly fix the newly xxx undefined errors in the codebase.

I have read CONTRIBUTING.md.
I have made an appropriate entry to CHANGELOG.md.
I have added a description to this PR with sufficient context for reviewers to understand this PR.

rkapka · 2025-01-07T13:45:45Z

api/server/structs/state.go

+	InactivityScores              []string                       `json:"inactivity_scores"`
+	CurrentSyncCommittee          *SyncCommittee                 `json:"current_sync_committee"`
+	NextSyncCommittee             *SyncCommittee                 `json:"next_sync_committee"`
+	LatestExecutionPayloadHeader  *ExecutionPayloadHeaderElectra `json:"latest_execution_payload_header"`


Suggested change

LatestExecutionPayloadHeader *ExecutionPayloadHeaderElectra `json:"latest_execution_payload_header"`

LatestExecutionPayloadHeader *ExecutionPayloadHeaderFulu `json:"latest_execution_payload_header"`

See #14771 (comment)

rkapka · 2025-01-07T13:49:10Z

beacon-chain/db/kv/blocks.go

@@ -852,30 +862,35 @@ func encodeBlock(blk interfaces.ReadOnlySignedBeaconBlock) ([]byte, error) {

 func keyForBlock(blk interfaces.ReadOnlySignedBeaconBlock) ([]byte, error) {


The reason why we have it backwards is because during normal operations the block/state will always be in the latest fork. Although a micro-optimization, it does make sense to do it this way. I think we should write such code for every if or switch

Fixed in a1e7efa.

rkapka · 2025-01-07T13:57:29Z

beacon-chain/p2p/types/object_mapping.go

 		},
+		bytesutil.ToBytes4(params.BeaconConfig().FuluForkVersion): func() (interfaces.ReadOnlySignedBeaconBlock, error) {
+			return blocks.NewSignedBeaconBlock(
+				&ethpb.SignedBeaconBlockFulu{Block: &ethpb.BeaconBlockFulu{Body: &ethpb.BeaconBlockBodyFulu{ExecutionPayload: &enginev1.ExecutionPayloadDeneb{}}}},


You should probably create ExecutionPayloadFulu and ExecutionPayloadHeaderFulu somewhere in the /proto/engine/v1 directory, just as Electra types are created in electra.go. And use it here and everywhere else.

Actually my target was to remove ExecutionPayloadElectra and ExecutionPayloadHeaderElectra as well and to do the exact contrary.

I thought I did it, I probably messed up something writing this PR.

Rationale: In all parts of the code, when a struct does not change between fork <n> and fork <n+1>, we just use the struct of fork <n> without aliasing it to the new fork.

Aka, we don't write things such as:

ExecutionPayloadElectra = ExecutionPayloadDeneb

So, my idea here is to remove those 2 type aliases, and use the Deneb version both in Electra and Fulu.

Right, looks like that's what we should do

Fixed in b43d1a6.

rkapka · 2025-01-07T14:16:12Z

beacon-chain/rpc/eth/beacon/handlers_test.go

 		v1alpha1Server.EXPECT().ProposeBeaconBlock(gomock.Any(), mock.MatchedBy(func(req *eth.GenericSignedBeaconBlock) bool {
-			block, ok := req.Block.(*eth.GenericSignedBeaconBlock_Electra)
-			converted, err := structs.SignedBeaconBlockContentsElectraFromConsensus(block.Electra)
+			// Convert back Fulu to Electra when there is at least one difference between Electra and Fulu blocks.


I don't understand this. Why can't we have a simple Electra test?

Because of the publishBlock function, which is called in the PublishBlock function, which itself is called in the test.

The way the publishBlock function works is it goes, in reverse, fork after fork to determine if a block if of type Fulu, Electra, Deneb etc...

To decide if a block is an Fulu block, it only looks if the block can be used as an Fulu block (by using unmarshalStrict.)

Currently, there is no difference between a Fulu block and an Electra block. So, when trying to unmarshal an Electra block, it will actually be considered as a Fulu block.

Note that this is absolutely not an issue for the publishBlock function itself: An Electra block is correctly published, and a Fulu block is correctly published as well.

Because this failed test will stop to fail once there is at least one difference between an Electra block and a Fulu block, then I think it's simpler to, at the moment, patch the test. This patch will simply need to be removed once there is at least one difference between and Electra block and a Fulu block.

rkapka · 2025-01-07T14:21:55Z

beacon-chain/rpc/prysm/v1alpha1/validator/construct_generic_block_test.go

 		require.ErrorContains(t, "block cannot be nil", err)
 	})

+	// Test for Fulu version


Nitpick: These comments don't have any value since the test case is already named after the fork

Hum indeed I over-commented it.

My idea was to have one comment by Fork (and not by test), as fixed in 83b895f.

Why?
Because if a new test appears (or a new fork), I would ensure the next developer to follow this logic, and to group tests by fork, and not by test type (block, blinded block...)

rkapka · 2025-01-07T14:24:38Z

beacon-chain/state/state-native/state_trie.go

 	return b, nil
 }

+// InitializeFromProtoUnsafeElectra directly uses the beacon state protobuf fields


Suggested change

// InitializeFromProtoUnsafeElectra directly uses the beacon state protobuf fields

// InitializeFromProtoUnsafeFulu directly uses the beacon state protobuf fields

See #14771 (comment).

beacon-chain/state/state-native/state_trie.go

rkapka · 2025-01-07T14:28:15Z

beacon-chain/state/state-native/state_trie.go

+	b.sharedFieldReferences[types.PreviousEpochParticipationBits] = stateutil.NewRef(1)
+	b.sharedFieldReferences[types.CurrentEpochParticipationBits] = stateutil.NewRef(1)
+	b.sharedFieldReferences[types.LatestExecutionPayloadHeaderDeneb] = stateutil.NewRef(1) // New in Electra.
+	b.sharedFieldReferences[types.HistoricalSummaries] = stateutil.NewRef(1)               // New in Capella.


I know it's not exactly the scope of this PR since the wrong comment had been there before, but HistoricalSummaries were added in Deneb, not in Electra.

Hum, it seems it has been introduced in Capella (as stated by the comment).
https://github.com/ethereum/consensus-specs/blob/dev/specs/capella/beacon-chain.md#beaconstate

testing/util/helpers.go

rkapka · 2025-01-07T17:36:04Z

api/server/structs/block.go

+type (
+	ExecutionPayloadFulu       = ExecutionPayloadDeneb
+	ExecutionPayloadHeaderFulu = ExecutionPayloadHeaderDeneb
+)


Given the discussion we had, looks like we can remove this (and the Electra equivalent, if there is one)

Fixed in b43d1a6.

rkapka · 2025-01-07T17:36:41Z

beacon-chain/p2p/types/object_mapping.go

 		},
+		bytesutil.ToBytes4(params.BeaconConfig().FuluForkVersion): func() (interfaces.ReadOnlySignedBeaconBlock, error) {
+			return blocks.NewSignedBeaconBlock(
+				&ethpb.SignedBeaconBlockFulu{Block: &ethpb.BeaconBlockFulu{Body: &ethpb.BeaconBlockBodyFulu{ExecutionPayload: &enginev1.ExecutionPayloadDeneb{}}}},


Right, looks like that's what we should do

Rationale: This log is the only one notifying the user a new fork happened. A new fork is always a little bit stressful for a node operator. Having at least one log indicating the client switched fork is something useful.

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

nalepae force-pushed the fulu-boilerplate branch 9 times, most recently from 8da3e00 to bcfaff8 Compare January 5, 2025 15:22

nalepae added the fulu label Jan 5, 2025

nalepae changed the title ~~Fulu boilerplate~~ Add Fulu fork boilerplate Jan 5, 2025

nalepae marked this pull request as ready for review January 6, 2025 10:08

nalepae requested a review from a team as a code owner January 6, 2025 10:08

nalepae requested review from dB2510, james-prysm and nisdas January 6, 2025 10:08

rkapka requested changes Jan 7, 2025

View reviewed changes

rkapka reviewed Jan 7, 2025

View reviewed changes

nalepae force-pushed the fulu-boilerplate branch from f40b4d9 to b43d1a6 Compare January 7, 2025 18:03

Prepare for future fork boilerplate.

20377aa

nalepae force-pushed the fulu-boilerplate branch from b43d1a6 to 2d08eca Compare January 7, 2025 18:05

nalepae and others added 9 commits January 7, 2025 19:11

Implement the Fulu fork boilerplate.

4aa945a

Upgraded state to <fork> log: Move from debug to info.

14ef71a

Rationale: This log is the only one notifying the user a new fork happened. A new fork is always a little bit stressful for a node operator. Having at least one log indicating the client switched fork is something useful.

Update testing/util/helpers.go

c3597df

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

Fix Radek's comment.

7782750

Fix Radek's comment.

f56f7ec

Update beacon-chain/state/state-native/state_trie.go

769bb91

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

Update beacon-chain/state/state-native/state_trie.go

82e644f

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

Fix Radek's comment.

77c5bfd

Fix Radek's comment.

b714ee0

nalepae added 2 commits January 7, 2025 19:11

Fix Radek's comment.

5f39a1f

Remove Electra struct type aliasing.

8a699c0

rkapka previously approved these changes Jan 7, 2025

View reviewed changes

nalepae dismissed rkapka’s stale review via 8a699c0 January 7, 2025 18:21

nalepae force-pushed the fulu-boilerplate branch from 2d08eca to 8a699c0 Compare January 7, 2025 18:21

rkapka approved these changes Jan 7, 2025

View reviewed changes

nalepae added this pull request to the merge queue Jan 7, 2025

Merged via the queue into develop with commit c48d409 Jan 7, 2025
6 checks passed

nalepae deleted the fulu-boilerplate branch January 7, 2025 20:17

rkapka mentioned this pull request Jan 16, 2025

Builder: Electra #14344

Merged

jihoonsong mentioned this pull request Mar 23, 2025

Rebase eip7805 fork onto Electra #15088

Open

	LatestExecutionPayloadHeader *ExecutionPayloadHeaderElectra `json:"latest_execution_payload_header"`
	LatestExecutionPayloadHeader *ExecutionPayloadHeaderFulu `json:"latest_execution_payload_header"`

		@@ -852,30 +862,35 @@ func encodeBlock(blk interfaces.ReadOnlySignedBeaconBlock) ([]byte, error) {

		func keyForBlock(blk interfaces.ReadOnlySignedBeaconBlock) ([]byte, error) {

	// InitializeFromProtoUnsafeElectra directly uses the beacon state protobuf fields
	// InitializeFromProtoUnsafeFulu directly uses the beacon state protobuf fields

Comments

Conversation

nalepae commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalepae Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nalepae commented Jan 3, 2025 •

edited

Loading

nalepae Jan 7, 2025 •

edited

Loading