Electra: upgrade #13933

james-prysm · 2024-04-29T16:13:42Z

What type of PR is this?
Feature

What does this PR do? Why is it needed?

NewGenesisBlockForState returns Electra state
UpgradeToElectra upgrades Deneb state to Electra
CanUpgradeToElectra returns true if it's Electra epoch
ProcessSlots and ReplayProcessSlots can upgrade to Electra
Add spec test for upgrade

Which issues(s) does this PR fix?

part of PSM-371

Other notes for review

dependency

Electra: BeaconState implementation #13919

…e.go

james-prysm · 2024-05-06T21:57:24Z

beacon-chain/core/electra/upgrade.go

+	}
+
+	// Sorting preActivation based on a custom criteria
+	sort.Slice(preActivation, func(i, j int) bool {


This part of the code needs more unit testing

beacon-chain/core/electra/upgrade_test.go

james-prysm · 2024-05-06T21:57:57Z

config/params/mainnet_config.go

@@ -290,6 +290,7 @@ var mainnetBeaconConfig = &BeaconChainConfig{
 	MaxPendingPartialsPerWithdrawalsSweep: 8,
 	FullExitRequestAmount:                 0,
 	MaxWithdrawalRequestsPerPayload:       16,
+	UnsetDepositReceiptsStartIndex:        math.MaxUint64,


double check if constant should go here?

beacon-chain/core/electra/upgrade.go

james-prysm · 2024-05-07T22:15:24Z

testing/util/electra_state.go

+		DepositReceiptsStartIndex:     params.BeaconConfig().UnsetDepositReceiptsStartIndex,
+		ExitBalanceToConsume:          helpers.ActivationExitChurnLimit(math.Gwei(tab)),
+		EarliestConsolidationEpoch:    helpers.ActivationExitEpoch(slots.ToEpoch(preState.Slot())),
+		ConsolidationBalanceToConsume: helpers.ConsolidationChurnLimit(math.Gwei(tab)),


wondering how important to set value of earliest exit here

…dependencies in other PRs

terencechain

I only reviewed UpgradeToElectra. Happy to review the helpers tomorrow, just let me know

terencechain · 2024-05-08T01:51:01Z

beacon-chain/core/electra/upgrade.go

+	}
+
+	// Find the earliest exit epoch
+	exitEpochs := make([]primitives.Epoch, 0)


You dont need a list here. You can save on memory. Just track a single variable of primitives.Epoch, first set it to time.CurrentEpoch(beaconState), then override it if there's a higher one. Finally, increment it by one

terencechain · 2024-05-08T01:53:44Z

beacon-chain/core/electra/upgrade.go

+	// Sorting preActivationIndices based on a custom criteria
+	sort.Slice(preActivationIndices, func(i, j int) bool {
+		// Comparing based on ActivationEligibilityEpoch and then by index if the epochs are the same
+		if s.Validators[preActivationIndices[i]].ActivationEligibilityEpoch == s.Validators[preActivationIndices[j]].ActivationEligibilityEpoch {
+			return preActivationIndices[i] < preActivationIndices[j]
+		}
+		return s.Validators[preActivationIndices[i]].ActivationEligibilityEpoch < s.Validators[preActivationIndices[j]].ActivationEligibilityEpoch
+	})


Why do we need to sort here? I know spec says it but why? We should avoid wasteful compute if we can

validators might have different ActivationEligibilityEpoch and should be sorted based on that value, there is also a tie breaker situation. i don't have a good solution to replace this right now.

rkapka · 2024-05-08T01:53:40Z

beacon-chain/core/electra/upgrade.go

+		PendingBalanceDeposits:        nil,
+		PendingPartialWithdrawals:     nil,
+		PendingConsolidations:         nil,


In the spec these are [] which I understand as empty slices. Having this nil might cause a panic the first time we access the field

rkapka · 2024-05-08T02:07:41Z

beacon-chain/core/electra/upgrade.go

+	// Find the earliest exit epoch
+	exitEpochs := make([]primitives.Epoch, 0)
+	// [New in Electra:EIP7251]
+	// add validators that are not yet active to pending balance deposits


This comment feels out of place

rkapka · 2024-05-08T02:09:26Z

beacon-chain/core/electra/upgrade.go

+
+	// Find the earliest exit epoch
+	exitEpochs := make([]primitives.Epoch, 0)
+	// [New in Electra:EIP7251]


nitpick: I think this comment should come before exitEpochs declaration

beacon-chain/core/electra/upgrade.go

rkapka · 2024-05-08T02:27:58Z

beacon-chain/state/interfaces.go

@@ -115,7 +116,7 @@ type ReadOnlyValidator interface {
 	WithdrawableEpoch() primitives.Epoch
 	ExitEpoch() primitives.Epoch
 	PublicKey() [fieldparams.BLSPubkeyLength]byte
-	WithdrawalCredentials() []byte
+	GetWithdrawalCredentials() []byte


Why the change?

It needs to follow the interface

the WithWithdrawalCredentials interface? I think that interface's function can also be called WithdrawalCredentials and it shouldn't cause an issue

the protobuf doesn't have WithdrawalCredentials as a function, only GetWithdrawalCredentials

beacon-chain/state/stategen/replay.go

rkapka · 2024-05-08T02:33:28Z

consensus-types/interfaces/validator.go

+type WithWithdrawalCredentials interface {
+	GetWithdrawalCredentials() []byte
+}


What's the purpose of this?

it's used in the helper function so that readOnly validator and protovalidator both can be passed in.

testing/util/electra_state.go

rkapka · 2024-05-08T02:36:10Z

testing/util/electra_state.go

+	return state_native.InitializeFromProtoElectra(st)
+}
+
+func buildGenesisBeaconStateElectra(genesisTime uint64, preState state.BeaconState, eth1Data *ethpb.Eth1Data) (state.BeaconState, error) {


Similarly to my comment in the upgrade function, I would set PendingBalanceDeposits, PendingPartialWithdrawals and PendingConsolidations to an empty array

will try that

Co-authored-by: Radosław Kapka <[email protected]>

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

* wip fork logic upgrade * fixing replay and fork.go * improving process function and adding tests for transition * updating unit tests and temporarily removing some fields on state_trie.go * updating state * wip adding upgrade to electra code * adding some comments * adding spec tests * fixing values used in state transition logic * updating upgrade test * gofmt * avoid dup word linting * fixing spec tests for fork * gaz * fixing tests * improving unit test with new getters * fixing bazel for minimal fork test * adding bazel file * Update beacon-chain/core/electra/upgrade.go Co-authored-by: Preston Van Loon <[email protected]> * addressing some comments and adding more tests * addressing more feedback * one more feedback * changing value to interface after talking to preston * adding missed review feedback * fixing linting * noticed I was using the wrong function in the state upgrade * fixing and ignoring some deepsource issues * moving core electra validator functions to helper to remove circular dependencies in other PRs * missed deepsource complaint * Update upgrade.go Co-authored-by: Radosław Kapka <[email protected]> * Update testing/util/electra_state.go Co-authored-by: Radosław Kapka <[email protected]> * Update testing/util/electra_state.go Co-authored-by: Radosław Kapka <[email protected]> * addressing feedback * removing deepsoure ignore comments --------- Co-authored-by: Preston Van Loon <[email protected]> Co-authored-by: Radosław Kapka <[email protected]>

james-prysm added 2 commits April 29, 2024 11:12

wip fork logic upgrade

3b58756

fixing replay and fork.go

e02475a

james-prysm changed the title ~~electra - fork logic~~ Electra - fork logic Apr 29, 2024

james-prysm changed the title ~~Electra - fork logic~~ Electra: fork logic Apr 29, 2024

improving process function and adding tests for transition

7d0c2bc

james-prysm changed the title ~~Electra: fork logic~~ Electra: upgrade Apr 29, 2024

james-prysm added the Blocked Blocked by research or external factors label Apr 29, 2024

james-prysm and others added 8 commits April 29, 2024 12:00

updating unit tests and temporarily removing some fields on state_tri…

2f7f899

…e.go

updating state

c594df8

wip adding upgrade to electra code

45ee712

adding some comments

0a2dae0

Merge branch 'develop' into electra-fork-logic

5287f56

Merge branch 'develop' into electra-fork-logic

b5560be

adding spec tests

1f7bb36

Merge branch 'develop' into electra-fork-logic

49cacc4

prestonvanloon removed the Blocked Blocked by research or external factors label May 6, 2024

james-prysm added 2 commits May 6, 2024 15:24

fixing values used in state transition logic

38bca88

updating upgrade test

de12137

james-prysm commented May 6, 2024

View reviewed changes

beacon-chain/core/electra/upgrade_test.go Outdated Show resolved Hide resolved

james-prysm commented May 6, 2024

View reviewed changes

james-prysm and others added 8 commits May 6, 2024 17:02

gofmt

4dd8861

avoid dup word linting

eae67f7

fixing spec tests for fork

dd36ec6

gaz

6080275

Merge branch 'develop' into electra-fork-logic

8ea9034

fixing tests

abbdd1a

improving unit test with new getters

0b34cf9

fixing bazel for minimal fork test

5cfe8d2

james-prysm marked this pull request as ready for review May 7, 2024 14:14

one more feedback

266c7ec

james-prysm commented May 7, 2024

View reviewed changes

beacon-chain/core/electra/upgrade.go Outdated Show resolved Hide resolved

james-prysm added 3 commits May 7, 2024 15:48

changing value to interface after talking to preston

969ae73

adding missed review feedback

174c6f6

fixing linting

ecc6e6a

james-prysm commented May 7, 2024

View reviewed changes

beacon-chain/core/electra/upgrade.go Outdated Show resolved Hide resolved

james-prysm and others added 2 commits May 7, 2024 16:47

noticed I was using the wrong function in the state upgrade

7f69c2d

Merge branch 'develop' into electra-fork-logic

62c0cee

james-prysm commented May 7, 2024

View reviewed changes

james-prysm and others added 4 commits May 7, 2024 18:19

fixing and ignoring some deepsource issues

2b57557

Merge branch 'develop' into electra-fork-logic

a523f30

moving core electra validator functions to helper to remove circular …

37dbcbd

…dependencies in other PRs

missed deepsource complaint

7ddae4a

terencechain reviewed May 8, 2024

View reviewed changes

rkapka reviewed May 8, 2024

View reviewed changes

james-prysm and others added 4 commits May 7, 2024 21:37

Update upgrade.go

1c9a6ae

Co-authored-by: Radosław Kapka <[email protected]>

Update testing/util/electra_state.go

cf89f83

Co-authored-by: Radosław Kapka <[email protected]>

Update testing/util/electra_state.go

44993f6

Co-authored-by: Radosław Kapka <[email protected]>

addressing feedback

429b474

prestonvanloon reviewed May 8, 2024

View reviewed changes

beacon-chain/rpc/eth/config/handlers_test.go Outdated Show resolved Hide resolved

removing deepsoure ignore comments

a915fbe

rkapka approved these changes May 8, 2024

View reviewed changes

prestonvanloon approved these changes May 8, 2024

View reviewed changes

james-prysm enabled auto-merge May 8, 2024 15:15

james-prysm added this pull request to the merge queue May 8, 2024

Merged via the queue into develop with commit 2fa3694 May 8, 2024
16 of 17 checks passed

james-prysm deleted the electra-fork-logic branch May 8, 2024 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Electra: upgrade #13933

Electra: upgrade #13933

james-prysm commented Apr 29, 2024 •

edited

Loading

james-prysm May 6, 2024

james-prysm May 6, 2024

prestonvanloon May 7, 2024

james-prysm May 7, 2024

terencechain left a comment

terencechain May 8, 2024

terencechain May 8, 2024

james-prysm May 8, 2024

rkapka May 8, 2024

rkapka May 8, 2024

rkapka May 8, 2024 •

edited

Loading

rkapka May 8, 2024

james-prysm May 8, 2024

rkapka May 8, 2024

james-prysm May 8, 2024

rkapka May 8, 2024

james-prysm May 8, 2024

rkapka May 8, 2024

james-prysm May 8, 2024

Electra: upgrade #13933

Electra: upgrade #13933

Conversation

james-prysm commented Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-prysm commented Apr 29, 2024 •

edited

Loading

rkapka May 8, 2024 •

edited

Loading