Capella beacon state #11141

rkapka · 2022-08-01T14:12:22Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Adds in Capella state, including new:

versions
types
getters
setters
hashing
spec params
SSZ generated code
proto conversions

This PR introduces the ExecutionData interface, which is a common abstraction over executionPayload, executionPayloadCapella, executionPayloadHeader and executionPayloadHeaderCapella. The reason for it is to avoid duplication in functions such as this:

func (s *Service) notifyNewPayload(ctx context.Context, postStateVersion int,
	postStateHeader interfaces.ExecutionData, blk interfaces.SignedBeaconBlock) (bool, error)

Here postStateHeader is of type ExecutionData, which means a single function can handle both possible underlying header types.

Which issues(s) does this PR fix?

Part of #11077

beacon-chain/db/kv/state.go

rkapka · 2022-08-01T14:20:01Z

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

+		fieldRoots[nativetypes.LatestExecutionPayloadHeaderCapella.RealPosition()] = executionPayloadRoot[:]
+
+		// Withdrawal queue root.
+		withdrawalQueueRoot := []byte("stub")


I left an exercise for you @potuz - complete the stub

These are hashed as normal SSZ lists of Withdrawal objects, doesn't fastssz already cover this?

discussed offline, we can reuse Nishant's vectorized merkleizer

Do we intend to fix this ? Or is this meant to be in a follow up PR. i rather not have our hashing incomplete like this if we are adding in the whole state.

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

rkapka · 2022-09-30T10:31:55Z

consensus-types/blocks/execution.go

+		BaseFeePerGas:    bytesutil.SafeCopyBytes(payload.BaseFeePerGas()),
+		BlockHash:        bytesutil.SafeCopyBytes(payload.BlockHash()),
+		TransactionsRoot: txRoot[:],
+		WithdrawalsRoot:  []byte("stub"),


Another one for @potuz

Same here this is the usual list.HTR from the withdrawal list in the execution payload.

potuz · 2022-09-30T10:36:49Z

beacon-chain/db/kv/state.go

@@ -294,6 +294,28 @@ func (s *Store) saveStatesEfficientInternal(ctx context.Context, tx *bolt.Tx, bl
 			if err := valIdxBkt.Put(rt[:], validatorKeys[i]); err != nil {
 				return err
 			}
+		case *ethpb.BeaconStateCapella:


This case switch is a good candidate for simplification using generics since all cases are the same function based on pbState.

Yes, this was my impression too. But I didn't want to refactor existing code in this PR.

There isn't even a need for generics if we can just pass in fastssz.Marshaler as an interface (beacon state satisfies this)

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

potuz · 2022-09-30T10:47:11Z

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

@@ -19,11 +21,20 @@ func (b *BeaconState) LatestExecutionPayloadHeader() (*enginev1.ExecutionPayload
 	b.lock.RLock()
 	defer b.lock.RUnlock()

-	return b.latestExecutionPayloadHeaderVal(), nil
+	if b.version == version.Bellatrix {


I think in general and considering future forks, the best approach for all these switches is the opposite order

if version >= Capella { do something } else if version >= Bellatrix { do something } else { not supported }

The first one is hit more often.

rkapka · 2022-09-30T10:49:26Z

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

+	case nativetypes.LatestExecutionPayloadHeaderCapella:
+		return b.latestExecutionPayloadHeaderCapella.HashTreeRoot()
+	case nativetypes.WithdrawalQueue:
+		return [32]byte{}, errors.New("not implemented")


and one more @potuz

Same as above, why doesn't fastssz already cover this give it's a list?

its not hard to do custom hashing for this too.

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

This reverts commit bcae282.

nisdas · 2022-10-03T10:42:10Z

beacon-chain/state/interfaces.go

@@ -85,6 +85,10 @@ type WriteOnlyBeaconState interface {
 	UpdateSlashingsAtIndex(idx, val uint64) error
 	AppendHistoricalRoots(root [32]byte) error
 	SetLatestExecutionPayloadHeader(payload interfaces.ExecutionData) error
+	SetWithdrawalQueue(val []*enginev1.Withdrawal) error


Not really related to your PR, but why is the withdrawal object in our engine namespace ? Seems like the wrong place for it

See discussion starting in https://discord.com/channels/476244492043812875/998988600530194583/1001913104512335882

nisdas · 2022-10-03T11:10:12Z

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

+		fieldRoots[nativetypes.LatestExecutionPayloadHeaderCapella.RealPosition()] = executionPayloadRoot[:]
+
+		// Withdrawal queue root.
+		withdrawalQueueRoot := []byte("stub")


Do we intend to fix this ? Or is this meant to be in a follow up PR. i rather not have our hashing incomplete like this if we are adding in the whole state.

nisdas · 2022-10-03T11:15:13Z

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

@@ -14,14 +14,20 @@ func (b *BeaconState) SetLatestExecutionPayloadHeader(val interfaces.ExecutionDa
 	b.lock.Lock()
 	defer b.lock.Unlock()

-	if b.version == version.Phase0 || b.version == version.Altair {
+	if b.version < version.Bellatrix {
 		return errNotSupported("SetLatestExecutionPayloadHeader", b.version)
 	}
 	header, ok := val.Proto().(*enginev1.ExecutionPayloadHeader)


can we use a switch statement here rather than multiple type assertions. Would make the code cleaner to read

nisdas · 2022-10-03T11:24:02Z

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

-	case version.Altair:
-		dst.sharedFieldReferences = make(map[nativetypes.FieldIndex]*stateutil.Reference, 11)
-	case version.Bellatrix:
+	case version.Altair, version.Bellatrix, version.Capella:


this isnt correct, altair, bellatrix and capella have different sizes.

nisdas · 2022-10-03T11:24:41Z

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

+	case nativetypes.LatestExecutionPayloadHeaderCapella:
+		return b.latestExecutionPayloadHeaderCapella.HashTreeRoot()
+	case nativetypes.WithdrawalQueue:
+		return [32]byte{}, errors.New("not implemented")


its not hard to do custom hashing for this too.

terencechain · 2022-10-03T20:48:48Z

beacon-chain/blockchain/execution_engine.go

@@ -183,7 +183,7 @@ func (s *Service) getPayloadHash(ctx context.Context, root []byte) ([32]byte, er
 // notifyForkchoiceUpdate signals execution engine on a new payload.
 // It returns true if the EL has returned VALID for the block
 func (s *Service) notifyNewPayload(ctx context.Context, postStateVersion int,
-	postStateHeader *enginev1.ExecutionPayloadHeader, blk interfaces.SignedBeaconBlock) (bool, error) {
+	postStateHeader interfaces.ExecutionData, blk interfaces.SignedBeaconBlock) (bool, error) {


What's the reason for changing *enginev1.ExecutionPayloadHeader to interfaces.ExecutionData. First glance, it seems unrelated to the PR title. Can you please explain it or even better cover that in the description?

I updated the description

terencechain · 2022-10-03T20:56:38Z

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

+	b.lock.RLock()
+	defer b.lock.RUnlock()


Should we move these read locks before the version check? Currently, it's not consistent with SetWithdrawalQueue and the setter method

I made everything consistent

terencechain · 2022-10-04T15:41:02Z

Let's get one more approve before merge plz

rkapka added 15 commits July 29, 2022 14:47

fork

d56db46

types

657b897

cloners

fffa376

getters

ed6c73a

remove CapellaBlind from fork

2b8a8da

hasher

6d303f6

setters

d21d437

spec params, config tests

f061dab

generate ssz

00a840b

executionPayloadHeaderCapella

a8c9125

proto state

6790b1c

BeaconStateCapella SSZ

b1ba747

saving state

0b7cfbd

configfork

437c265

BUILD files

c5deedb

rkapka requested review from nisdas, rauljordan and a team as code owners August 1, 2022 14:12

rkapka requested a review from symbolpunk August 1, 2022 14:12

Merge branch 'develop' into capella-state

8af7855

rkapka commented Aug 1, 2022

View reviewed changes

beacon-chain/db/kv/state.go Outdated Show resolved Hide resolved

rkapka added 4 commits August 1, 2022 16:16

fix RealPosition

d5e8c25

Merge branch '__develop' into capella-state

f98e90b

Merge remote-tracking branch 'origin/capella-state' into capella-state

8b4b3cf

fix hasher

d2aabcf

rkapka commented Aug 1, 2022

View reviewed changes

SetLatestExecutionPayloadHeaderCapella

e74ad33

rkapka commented Aug 1, 2022

View reviewed changes

beacon-chain/state/state-native/beacon_state_mainnet.go Show resolved Hide resolved

rkapka added 2 commits August 1, 2022 16:24

fix error message

e21ab9c

reduce complexity of saveStatesEfficientInternal

c96303e

rkapka added 2 commits September 30, 2022 12:20

fix configs

bcae282

Merge branch '__develop' into capella-state

fb6b1d1

rkapka commented Sep 30, 2022

View reviewed changes

rkapka added Ready For Review A pull request ready for code review and removed Capella labels Sep 30, 2022

potuz reviewed Sep 30, 2022

View reviewed changes

rkapka commented Sep 30, 2022

View reviewed changes

potuz reviewed Sep 30, 2022

View reviewed changes

beacon-chain/state/state-native/hasher.go Outdated Show resolved Hide resolved

beacon-chain/state/state-native/hasher.go Outdated Show resolved Hide resolved

simplify hashing

0df2f33

rkapka force-pushed the capella-state branch from 920fc31 to 0df2f33 Compare September 30, 2022 11:44

rkapka added 3 commits September 30, 2022 14:47

Revert "fix configs"

a117e0b

This reverts commit bcae282.

remove capella from config test

0b00e1e

Merge branch 'develop' into capella-state

1a43b53

nisdas reviewed Oct 3, 2022

View reviewed changes

terencechain requested changes Oct 3, 2022

View reviewed changes

unify locking

5548f0f

terencechain previously approved these changes Oct 4, 2022

View reviewed changes

terencechain and others added 3 commits October 4, 2022 05:41

Merge branch 'develop' into capella-state

6a49722

review

54e519c

hashing

ae78e88

rkapka dismissed terencechain’s stale review via ae78e88 October 5, 2022 13:05

rkapka added 2 commits October 5, 2022 15:06

Merge branch 'develop' into capella-state

2ab4908

fixes

872245c

rauljordan approved these changes Oct 5, 2022

View reviewed changes

rauljordan and others added 2 commits October 5, 2022 11:55

Merge branch 'develop' into capella-state

f437cd8

Merge branch 'develop' into capella-state

6266ce2

nisdas approved these changes Oct 12, 2022

View reviewed changes

rkapka merged commit cafe0bd into develop Oct 12, 2022

delete-merged-branch bot deleted the capella-state branch October 12, 2022 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Capella beacon state #11141

Capella beacon state #11141

rkapka commented Aug 1, 2022 •

edited

Loading

rkapka Aug 1, 2022 •

edited

Loading

potuz Sep 30, 2022

potuz Sep 30, 2022

nisdas Oct 3, 2022

rkapka Sep 30, 2022

potuz Sep 30, 2022

potuz Sep 30, 2022

rkapka Sep 30, 2022

rauljordan Sep 30, 2022

potuz Sep 30, 2022

rkapka Sep 30, 2022

potuz Sep 30, 2022

nisdas Oct 3, 2022

nisdas Oct 3, 2022

rkapka Oct 4, 2022

nisdas Oct 3, 2022

nisdas Oct 3, 2022

nisdas Oct 3, 2022

nisdas Oct 3, 2022

terencechain Oct 3, 2022

rkapka Oct 4, 2022

terencechain Oct 3, 2022

rkapka Oct 4, 2022

terencechain commented Oct 4, 2022

Capella beacon state #11141

Capella beacon state #11141

Conversation

rkapka commented Aug 1, 2022 • edited Loading

rkapka Aug 1, 2022 • 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

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

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

terencechain commented Oct 4, 2022

rkapka commented Aug 1, 2022 •

edited

Loading

rkapka Aug 1, 2022 •

edited

Loading