Cleanup of `stategen` package #10607

rkapka · 2022-05-02T19:59:03Z

What type of PR is this?

Cleanup

What does this PR do? Why is it needed?

Various cleanup of stategen including:

improved comments
renaming root to blockRoot in several places for better clarity

prestonvanloon · 2022-05-02T20:42:36Z

Tests fail

prestonvanloon · 2022-05-02T20:43:15Z

beacon-chain/state/stategen/getter.go

+	st := s.hotStateCache.getWithoutCopy(blockRoot)
+	return st


Suggested change

st := s.hotStateCache.getWithoutCopy(blockRoot)

return st

return s.hotStateCache.getWithoutCopy(blockRoot)

prestonvanloon · 2022-05-02T20:43:40Z

beacon-chain/state/stategen/getter.go

+// state is not copied (block batches returned from initial sync are linear).
+// It invalidates cache for parent root because pre-state will get mutated.
+//
+// WARNING: Do not use this method for anything other than initial syncing purpose or block tree is applied.


What does it mean "or block tree is applied"?

@nisdas , can you help me out with a better comment?

prestonvanloon · 2022-05-02T20:46:00Z

beacon-chain/state/stategen/migrate.go

 	// If the slot is on archived point, save the state of that slot to the DB.
-	for slot := oldFSlot; slot < fSlot; slot++ {
+slotLoop:


What is with this addition of a goto statement? This is a logic change beyond simple renaming, no?

+1, why are we using goto here ? Goto statements are avoided unless they are necessary.

The idea was to use a named loop because continue is very far away from for. But I agree this is not necessary.

prestonvanloon · 2022-05-02T20:49:51Z

beacon-chain/state/stategen/epoch_boundary_state_cache.go

+func (e *epochBoundaryState) ByRoot(blockRoot [32]byte) (state.BeaconState, error) {
+	rsi, ok, err := e.getByRoot(blockRoot)


Go style generally prefers smaller variable names. https://github.com/golang/go/wiki/CodeReviewComments#variable-names

In this case, I would recommend leaving the root as r because this method is aptly named and very short. It is clear to everyone that r is a root, although it is not clear what kind of root it is.

Perhaps there is an argument that this method is not aptly named since there is ambiguity around what kind of root it is. My suggestion is to change this to ByBlockRoot(br [32]byte) then we all know what we are dealing with.

prestonvanloon · 2022-05-02T20:50:13Z

beacon-chain/state/stategen/epoch_boundary_state_cache.go

@@ -79,15 +79,15 @@ func (e *epochBoundaryState) ByRoot(r [32]byte) (state.BeaconState, error) {
 }

 // get epoch boundary state by its block root. Returns copied state in state info object if exists. Otherwise returns nil.
-func (e *epochBoundaryState) getByRoot(r [32]byte) (*rootStateInfo, bool, error) {
+func (e *epochBoundaryState) getByRoot(blockRoot [32]byte) (*rootStateInfo, bool, error) {


Same comment here and all of these ByRoot methods

prestonvanloon · 2022-05-02T20:50:36Z

beacon-chain/state/stategen/epoch_boundary_state_cache.go

 }

 // put adds a state to the epoch boundary state cache. This method also trims the
 // least recently added state info if the cache size has reached the max cache
 // size limit.
-func (e *epochBoundaryState) put(r [32]byte, s state.BeaconState) error {
+func (e *epochBoundaryState) put(blockRoot [32]byte, s state.BeaconState) error {


This renaming is good, r was not clear at all.

prestonvanloon · 2022-05-02T20:51:20Z

beacon-chain/state/stategen/epoch_boundary_state_cache.go

@@ -152,11 +152,11 @@ func (e *epochBoundaryState) put(r [32]byte, s state.BeaconState) error {
 }

 // delete the state from the epoch boundary state cache.
-func (e *epochBoundaryState) delete(r [32]byte) error {
+func (e *epochBoundaryState) delete(blockRoot [32]byte) error {


Same here, this was a good renaming as it added context that was not provided by the method name or godoc comment.

…eral-improvements-2

* powchain and stategen * revert powchain changes * rename field to blockRootsOfSavedStates * rename params to blockRoot * review feedback * fix loop Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

* SubmitBlockSSZ grpc * SubmitBlockSSZ middleware * test fixes * use VersionedUnmarshaller * use VersionedUnmarshaller (cherry picked from commit 7388eeb) * tests * fuzz: Add fuzz tests for sparse merkle trie (#10662) * Add fuzz tests for sparse merkle trie and change HTR signature to return an error * fix capitalization of error message * Add engine timeout values (#10645) * Add timeout values * Update engine_client.go * Update engine_client.go * Update beacon-chain/powchain/engine_client.go Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com> * Update beacon-chain/powchain/engine_client.go Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com> * Update beacon-chain/powchain/engine_client.go Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com> * Update engine_client.go Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com> * Cleanup of `stategen` package (#10607) * powchain and stategen * revert powchain changes * rename field to blockRootsOfSavedStates * rename params to blockRoot * review feedback * fix loop Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> * Process atts and update head before proposing (#10653) * Process atts and updeate head * Fix ctx * New test and old tests * Update validator_test.go * Update validator_test.go * Update service.go * Rename to UpdateHead * Update receive_attestation.go * Update receive_attestation.go Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> * Add link to e2e docs in `README` (#10672) * Improve `ReceiveBlock`'s comment (#10671) Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> * Call fcu on invalid payload (#10565) * Starting * remove finalized root * Just call fcu * Review feedbacks * fix one test * Fix conflicts * Update execution_engine_test.go * Add a test for invalid recursive call * Add comprehensive recursive test * dissallow override empty hash Co-authored-by: Potuz <potuz@prysmaticlabs.com> Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> * Cache and use justified and finalized payload block hash (#10657) * Cache and use justified and finalized payload block hash * Fix tests * Use real byte * Fix conflicts Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> * do not export slotFromBlock * simplify tests * grpc * middleware * extract package-level consts * Simplify SSZ handling * fix tests * test fixes * test hack Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com> Co-authored-by: terencechain <terence@prysmaticlabs.com> Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Potuz <potuz@prysmaticlabs.com>

rkapka added 4 commits April 29, 2022 12:07

powchain and stategen

1219261

revert powchain changes

d25b265

rename field to blockRootsOfSavedStates

d5ae9d6

rename params to blockRoot

214f1fd

rkapka added Ready For Review Cleanup Code health! labels May 2, 2022

rkapka requested a review from a team as a code owner May 2, 2022 19:59

rkapka requested review from prestonvanloon, terencechain and symbolpunk May 2, 2022 19:59

rkapka added 2 commits May 2, 2022 21:59

Merge branch 'develop' into general-improvements-2

cb2033b

Merge branch '__develop' into general-improvements-2

5372f14

rkapka removed Ready For Review labels May 2, 2022

prestonvanloon reviewed May 2, 2022

View reviewed changes

rkapka added 4 commits May 3, 2022 14:14

Merge remote-tracking branch 'origin/general-improvements-2' into gen…

3b2819f

…eral-improvements-2

review feedback

7f41c82

fix loop

f361df1

Merge branch 'develop' into general-improvements-2

71dde52

rkapka added the Ready For Review label May 3, 2022

rauljordan approved these changes May 9, 2022

View reviewed changes

Merge branch 'develop' into general-improvements-2

af0040b

rauljordan added the OK to merge label May 9, 2022

Merge refs/heads/develop into general-improvements-2

3ef9996

prylabs-bulldozer bot merged commit 5d94030 into develop May 9, 2022

delete-merged-branch bot deleted the general-improvements-2 branch May 9, 2022 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup of `stategen` package #10607

Cleanup of `stategen` package #10607

rkapka commented May 2, 2022

prestonvanloon commented May 2, 2022

prestonvanloon May 2, 2022

prestonvanloon May 2, 2022

rkapka May 3, 2022

prestonvanloon May 2, 2022

nisdas May 3, 2022 •

edited

Loading

rkapka May 3, 2022

prestonvanloon May 2, 2022

prestonvanloon May 2, 2022

prestonvanloon May 2, 2022

prestonvanloon May 2, 2022

	st := s.hotStateCache.getWithoutCopy(blockRoot)
	return st
	return s.hotStateCache.getWithoutCopy(blockRoot)

		func (e *epochBoundaryState) ByRoot(blockRoot [32]byte) (state.BeaconState, error) {
		rsi, ok, err := e.getByRoot(blockRoot)

Cleanup of stategen package #10607

Cleanup of stategen package #10607

Conversation

rkapka commented May 2, 2022

prestonvanloon commented May 2, 2022

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

nisdas May 3, 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

Cleanup of `stategen` package #10607

Cleanup of `stategen` package #10607

nisdas May 3, 2022 •

edited

Loading