Add historical summaries to state and processing #11842

terencechain · 2023-01-03T23:59:39Z

This PR implements ethereum/consensus-specs#2649, which adds historical summaries to the beacon state and updates them for epoch processing. This is part of the Capella update

⚠️ Note: We need to update the new state for beacon-api. But we can do this in subsequent PR

terencechain · 2023-01-09T16:15:58Z

beacon-chain/core/epoch/epoch_processing.go

-func ProcessHistoricalRootsUpdate(state state.BeaconState) (state.BeaconState, error) {
+// ProcessHistoricalDataUpdate processes the updates to historical data during epoch processing.
+// For Capella state, per spec, historical summaries is updated instead of historical roots.
+func ProcessHistoricalDataUpdate(state state.BeaconState) (state.BeaconState, error) {


I decided to embed two different processing logic for historical data so that callers don't have to worry about what fork uses what.
Another approach is to implement two different functions ProcessHistoricalRootsUpdate and ProcessHistoricalSummariesUpdate with some duplicated conditions and let the callers decide which to call

+1 to encapsulating that here, the way you're doing it

terencechain · 2023-01-09T16:18:10Z

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

+
+// HistoricalSummaries of the beacon state.
+func (b *BeaconState) HistoricalSummaries() []*ethpb.HistoricalSummary {
+	if b.historicalSummaries == nil {


I skipped the versioning checks here and HistoricalRoots() because the list would just be emptied and error out upon return. I don't think it's a big deal, and I didn't feel like changing the return argument to include an error. Happy to change it otherwise

Don't most of the other getters return an error when a property is accessed on a version of the state which does not have that property?

rkapka

Very nice and clean!

beacon-chain/core/epoch/epoch_processing.go

beacon-chain/core/epoch/epoch_processing_test.go

rkapka · 2023-01-11T14:23:59Z

beacon-chain/core/epoch/epoch_processing_test.go

+				require.NoError(t, err)
+				return st
+			},
+			verifier: func(st state.BeaconState) {


Verify historical summaries were not updates?

rkapka · 2023-01-11T14:24:14Z

beacon-chain/core/epoch/epoch_processing_test.go

+				require.NoError(t, err)
+				return st
+			},
+			verifier: func(st state.BeaconState) {


Verify historical roots were not updated?

rkapka · 2023-01-11T14:25:30Z

beacon-chain/state/interfaces.go

@@ -85,6 +86,7 @@ type WriteOnlyBeaconState interface {
 	SetSlashings(val []uint64) error
 	UpdateSlashingsAtIndex(idx, val uint64) error
 	AppendHistoricalRoots(root [32]byte) error
+	AppendHistoricalSummariesUpdate(*ethpb.HistoricalSummary) error


This Update part sounds weird. How about AppendHistoricalSummaries?

rkapka · 2023-01-11T14:31:24Z

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

 func (b *BeaconState) AppendHistoricalRoots(root [32]byte) error {
 	b.lock.Lock()
 	defer b.lock.Unlock()

+	if b.version > version.Bellatrix {
+		return fmt.Errorf("AppendHistoricalRoots is not supported for version %d", b.version)


Please use the errNotSupported function

rkapka · 2023-01-11T14:31:57Z

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

@@ -124,6 +131,29 @@ func (b *BeaconState) AppendHistoricalRoots(root [32]byte) error {
 	return nil
 }

+// AppendHistoricalSummariesUpdate AppendHistoricalSummary for the beacon state. Appends the new value


Something is not correct

rkapka · 2023-01-11T14:32:13Z

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

+	defer b.lock.Unlock()
+
+	if b.version < version.Capella {
+		return fmt.Errorf("AppendHistoricalSummariesUpdate is not supported for version %d", b.version)


Please use the errNotSupported function

proto/prysm/v1alpha1/beacon_state.proto

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

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

… into historical-summaries

…cal-summaries

rkapka · 2023-01-11T17:01:20Z

tools/exploredb/main.go

+		roots, err := st.HistoricalRoots()
+		if err != nil {
+			log.WithError(err).Error("could not get historical roots")
+			return


return or continue logging?

cont is probably better here

kasey · 2023-01-11T21:29:04Z

tools/exploredb/main.go

 		}
-		size, count = sizeAndCountOfByteList(roots)
-		log.Infof("historical_roots              : size = %s, count = %d", humanize.Bytes(size), count)
+


nitpick: not a fan of this whitespace formatting. I've been leaning more towards using WithField to make structured logs easier.

It's a little unrelated to this PR

kasey · 2023-01-11T21:29:47Z

beacon-chain/core/epoch/epoch_processing_test.go

-				require.NoError(t, err)
-				require.Equal(t, 0, len(summaries))
+				_, err = st.HistoricalSummaries()
+				require.ErrorContains(t, "HistoricalSummaries is not supported for phase0", err)


can we use require.ErrorIs instead of string matching the error text?

Error is not exported. I can export the error though. Was trying to minimize the changes since the diff is getting big

nisdas · 2023-01-12T00:24:54Z

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

@@ -67,15 +68,19 @@ func (b *BeaconState) forkVal() *ethpb.Fork {
 }

 // HistoricalRoots based on epochs stored in the beacon state.
-func (b *BeaconState) HistoricalRoots() [][]byte {
+func (b *BeaconState) HistoricalRoots() ([][]byte, error) {
+	if b.version > version.Bellatrix {


This is not accurate, the historical roots field still exists in the state. Just that it is now 'frozen'

nisdas · 2023-01-12T00:33:33Z

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

@@ -530,6 +533,7 @@ func (b *BeaconState) Copy() state.BeaconState {
 		nextSyncCommittee:                   b.nextSyncCommitteeVal(),
 		latestExecutionPayloadHeader:        b.latestExecutionPayloadHeaderVal(),
 		latestExecutionPayloadHeaderCapella: b.latestExecutionPayloadHeaderCapellaVal(),
+		historicalSummaries:                 b.historicalSummariesVal(),


if we are sharing fields for historical summaries with other state instances, then this is incorrect. This performs a whole copy of the field, when we should just be sharing the reference like how it is done for historical roots:

historicalRoots: b.historicalRoots,

nisdas · 2023-01-12T00:35:56Z

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

 	}

 	return fieldRoots, nil
 }
+
+func historicalSummaryRoot(summaries []*ethpb.HistoricalSummary) ([32]byte, error) {


This should be in the stateutil package, since there is where we keep all the state hashing functions at

Add historical summaries to state and processing

9cb48be

terencechain self-assigned this Jan 3, 2023

terencechain added 3 commits January 6, 2023 11:56

Process historical roots test

144218c

Passing spec tests

055e225

Fix shas and tests

91b15c1

terencechain added the Ready For Review A pull request ready for code review label Jan 9, 2023

terencechain marked this pull request as ready for review January 9, 2023 16:13

terencechain requested a review from a team as a code owner January 9, 2023 16:13

terencechain requested review from potuz and nisdas January 9, 2023 16:13

Merge branch 'develop' into historical-summaries

5898b39

terencechain requested a review from james-prysm January 9, 2023 16:13

terencechain commented Jan 9, 2023

View reviewed changes

terencechain added 5 commits January 9, 2023 11:40

Merge branch 'develop' into historical-summaries

d875647

Fix mainnet spectest sha

3f6f4b6

Fix tests

8b53c65

Merge branch 'develop' into historical-summaries

79ba8dd

Merge branch 'develop' into historical-summaries

60d106c

rkapka reviewed Jan 11, 2023

View reviewed changes

terencechain and others added 6 commits January 11, 2023 08:10

Update beacon-chain/core/epoch/epoch_processing.go

ffd165e

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

Update beacon-chain/core/epoch/epoch_processing_test.go

7886997

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

Update proto/prysm/v1alpha1/beacon_state.proto

261229e

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

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

549993f

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

Radek's feedback

f9a040a

Merge branch 'develop' into historical-summaries

77b00ab

rkapka previously approved these changes Jan 11, 2023

View reviewed changes

terencechain added 2 commits January 11, 2023 08:52

Getters error

782d500

Merge branch 'historical-summaries' of github.com:prysmaticlabs/prysm…

c0e35d3

… into historical-summaries

terencechain dismissed rkapka’s stale review via c0e35d3 January 11, 2023 16:53

Merge branch 'develop' of github.com:prysmaticlabs/prysm into histori…

957e4ec

…cal-summaries

rkapka reviewed Jan 11, 2023

View reviewed changes

terencechain added 2 commits January 11, 2023 09:13

Dont return

0cc7843

Fix else

c4eee98

rkapka previously approved these changes Jan 11, 2023

View reviewed changes

terencechain added the OK to merge label Jan 11, 2023

Fix tests

9fe0774

terencechain dismissed rkapka’s stale review via 9fe0774 January 11, 2023 18:40

Fix test

93c8ccb

kasey reviewed Jan 11, 2023

View reviewed changes

Rm white space

93bc55d

kasey approved these changes Jan 11, 2023

View reviewed changes

prylabs-bulldozer bot merged commit 7fa3ebf into develop Jan 11, 2023

delete-merged-branch bot deleted the historical-summaries branch January 11, 2023 22:20

terencechain mentioned this pull request Jan 11, 2023

Add historical summaries to beacon api state #11863

Merged

nisdas reviewed Jan 12, 2023

View reviewed changes

terencechain mentioned this pull request Jan 12, 2023

Address historical summaries feedback #11864

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add historical summaries to state and processing #11842

Add historical summaries to state and processing #11842

terencechain commented Jan 3, 2023 •

edited

Loading

terencechain Jan 9, 2023

kasey Jan 11, 2023 •

edited

Loading

terencechain Jan 9, 2023 •

edited

Loading

kasey Jan 11, 2023

rkapka left a comment

rkapka Jan 11, 2023

rkapka Jan 11, 2023

rkapka Jan 11, 2023

rkapka Jan 11, 2023

rkapka Jan 11, 2023

rkapka Jan 11, 2023

rkapka Jan 11, 2023

terencechain Jan 11, 2023

kasey Jan 11, 2023

terencechain Jan 11, 2023

kasey Jan 11, 2023

terencechain Jan 11, 2023

nisdas Jan 12, 2023

nisdas Jan 12, 2023

nisdas Jan 12, 2023

Add historical summaries to state and processing #11842

Add historical summaries to state and processing #11842

Conversation

terencechain commented Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

kasey Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

terencechain Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka 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

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 Jan 3, 2023 •

edited

Loading

kasey Jan 11, 2023 •

edited

Loading

terencechain Jan 9, 2023 •

edited

Loading