Process atts and update head before proposing #10653

terencechain · 2022-05-06T15:10:17Z

This implements ethereum/consensus-specs#2878

Joined work with @potuz

This PR refactors out ProcessAttestationsAndUpdateHead with a lock to only allow one caller at a time to avoid race between:

Calling it at slot start
Calling it before the proposal

terencechain · 2022-05-06T15:10:37Z

beacon-chain/blockchain/receive_attestation.go

+	// Only one process can process attestations and update head at a time.
+	s.processAttestationsLock.Lock()
+	defer s.processAttestationsLock.Unlock()


This is the only addition from the refactor above

terencechain · 2022-05-06T15:11:14Z

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

@@ -82,6 +82,10 @@ func (vs *Server) buildPhase0BlockData(ctx context.Context, req *ethpb.BlockRequ
 		return nil, fmt.Errorf("syncing to latest head, not ready to respond")
 	}

+	if err := vs.HeadUpdater.ProcessAttestationsAndUpdateHead(ctx); err != nil {
+		log.WithError(err).Error("Could not process attestations and update head")


Opt it to log an error instead of return error. Error from this call should not prevent block proposal

…-atts-update-head

rauljordan · 2022-05-09T21:47:23Z

beacon-chain/blockchain/chain_info.go

+// HeadUpdater defines a common interface for methods in blockchain service
+// which allow to update the head info
+type HeadUpdater interface {
+	ProcessAttestationsAndUpdateHead(context.Context) error


Is there no way to separate this? Can head updater have two functions?

rauljordan · 2022-05-09T21:48:16Z

beacon-chain/blockchain/receive_attestation.go

 			}
 		}
 	}()
 }

+// ProcessAttestationsAndUpdateHead processes fork choice attestations from the pool and updates the head.
+func (s *Service) ProcessAttestationsAndUpdateHead(ctx context.Context) error {


We're combining multiple concerns here. It feels a lot cleaner to have the caller do update head after forkchoice attestations are processed

rauljordan · 2022-05-09T22:09:21Z

beacon-chain/blockchain/receive_attestation.go

 			}
 		}
 	}()
 }

+// UpdateHead processes fork choice attestations from the pool and updates the head.


We should say that it updates the canonical head of the chain based on information from fork-choice attestations and votes. It requires no external inputs.

* 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>

* 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 <[email protected]> * Update beacon-chain/powchain/engine_client.go Co-authored-by: Preston Van Loon <[email protected]> * Update beacon-chain/powchain/engine_client.go Co-authored-by: Preston Van Loon <[email protected]> * Update engine_client.go Co-authored-by: Preston Van Loon <[email protected]> * 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> Co-authored-by: terencechain <[email protected]> Co-authored-by: Raul Jordan <[email protected]> Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Potuz <[email protected]>

Dayday10

?

Dayday10 · 2022-05-30T10:50:21Z

Do not merge this account

Process atts and updeate head

4c9b1ff

terencechain commented May 6, 2022

View reviewed changes

terencechain marked this pull request as ready for review May 6, 2022 15:16

terencechain requested a review from a team as a code owner May 6, 2022 15:16

terencechain requested review from kasey, rkapka and symbolpunk May 6, 2022 15:16

terencechain marked this pull request as draft May 6, 2022 15:16

terencechain assigned potuz and terencechain May 6, 2022

terencechain added the Spec label May 6, 2022

terencechain added 2 commits May 6, 2022 09:11

Fix ctx

3a101dc

New test and old tests

d76a726

terencechain marked this pull request as ready for review May 6, 2022 17:50

terencechain added 5 commits May 6, 2022 11:05

Merge branch 'develop' into process-atts-update-head

1dc94a5

Update validator_test.go

2729c29

Update validator_test.go

a180724

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

cb07337

…-atts-update-head

Update service.go

5240935

terencechain added Ready For Review A pull request ready for code review OK to merge labels May 9, 2022

terencechain and others added 2 commits May 9, 2022 09:53

Merge branch 'develop' into process-atts-update-head

12a7b4e

Merge refs/heads/develop into process-atts-update-head

e7b2003

rauljordan reviewed May 9, 2022

View reviewed changes

Rename to UpdateHead

21456f5

rauljordan reviewed May 9, 2022

View reviewed changes

terencechain added 2 commits May 9, 2022 15:11

Update receive_attestation.go

c1d315e

Update receive_attestation.go

3d4821a

rauljordan approved these changes May 9, 2022

View reviewed changes

Merge refs/heads/develop into process-atts-update-head

f33f7b4

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

delete-merged-branch bot deleted the process-atts-update-head branch May 9, 2022 23:12

Dayday10 suggested changes May 30, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process atts and update head before proposing #10653

Process atts and update head before proposing #10653

terencechain commented May 6, 2022

terencechain May 6, 2022

terencechain May 6, 2022

rauljordan May 9, 2022

rauljordan May 9, 2022

rauljordan May 9, 2022

Dayday10 left a comment

Dayday10 commented May 30, 2022

Process atts and update head before proposing #10653

Process atts and update head before proposing #10653

Conversation

terencechain commented May 6, 2022

terencechain May 6, 2022

Choose a reason for hiding this comment

terencechain May 6, 2022

Choose a reason for hiding this comment

rauljordan May 9, 2022

Choose a reason for hiding this comment

rauljordan May 9, 2022

Choose a reason for hiding this comment

rauljordan May 9, 2022

Choose a reason for hiding this comment

Dayday10 left a comment

Choose a reason for hiding this comment

Dayday10 commented May 30, 2022