New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use headblock for prunePostBlockOperationPools, remove duplicate markInclusionBLStoExecutionChange calls #12085
Merged
Merged
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
27c8bc7
removing duplicate function
james-prysm 0e04895
Merge branch 'develop' into duplicate-changes-removal
james-prysm 8d08dce
moved markInclusion for bls to use headblock instead of processed block
james-prysm 34b9b61
Merge branch 'develop' into duplicate-changes-removal
james-prysm c20c641
Merge branch 'develop' into duplicate-changes-removal
james-prysm 63f95cc
Merge branch 'develop' into duplicate-changes-removal
james-prysm 90bf88e
updating based on internal feedback
james-prysm 5b5823e
addressing some comments
james-prysm 3235997
addressing feedback from slack
james-prysm ab353ca
Merge branch 'develop' into duplicate-changes-removal
james-prysm 362f353
fixing conflict
james-prysm 10058a3
Merge branch 'develop' into duplicate-changes-removal
james-prysm 2d2545c
Merge branch 'develop' into duplicate-changes-removal
james-prysm f838b31
making changes based on suggestions on slack
james-prysm 30cf37c
reverting a change
james-prysm 904944b
Merge branch 'develop' into duplicate-changes-removal
james-prysm 4cbb316
Merge branch 'develop' into duplicate-changes-removal
james-prysm ad05c42
making chases based on potuz's comments
james-prysm 1ac08f8
Merge branch 'develop' into duplicate-changes-removal
james-prysm ef5da67
removing one additional block copy
james-prysm 7a5818a
Merge branch 'develop' into duplicate-changes-removal
james-prysm 38ecfee
Merge branch 'develop' into duplicate-changes-removal
james-prysm 52cb064
Merge branch 'develop' into duplicate-changes-removal
james-prysm 7c7b7ef
clarifying comments
james-prysm File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package blockchain | ||
|
||
import ( | ||
"bytes" | ||
"context" | ||
|
||
"github.com/pkg/errors" | ||
|
@@ -48,17 +49,16 @@ func (s *Service) ReceiveBlock(ctx context.Context, block interfaces.ReadOnlySig | |
|
||
s.cfg.ForkChoiceStore.Lock() | ||
defer s.cfg.ForkChoiceStore.Unlock() | ||
|
||
// Apply state transition on the new block. | ||
if err := s.onBlock(ctx, blockCopy, blockRoot); err != nil { | ||
err := errors.Wrap(err, "could not process block") | ||
tracing.AnnotateError(span, err) | ||
return err | ||
} | ||
|
||
// Handle post block operations such as attestations and exits. | ||
if err := s.handlePostBlockOperations(blockCopy.Block()); err != nil { | ||
return err | ||
// Handle post block operations such as pruning exits and bls messages. | ||
if err := s.prunePostBlockOperationPools(ctx, blockRoot); err != nil { | ||
log.WithError(err).Error("Could not prune canonical objects from pool ") | ||
} | ||
|
||
// Have we been finalizing? Should we start saving hot states to db? | ||
|
@@ -157,29 +157,44 @@ func (s *Service) ReceiveAttesterSlashing(ctx context.Context, slashing *ethpb.A | |
s.InsertSlashingsToForkChoiceStore(ctx, []*ethpb.AttesterSlashing{slashing}) | ||
} | ||
|
||
func (s *Service) handlePostBlockOperations(b interfaces.ReadOnlyBeaconBlock) error { | ||
// prunePostBlockOperationPools only runs on new head otherwise should return a nil. | ||
func (s *Service) prunePostBlockOperationPools(ctx context.Context, root [32]byte) error { | ||
headRoot, err := s.HeadRoot(ctx) | ||
if err != nil { | ||
return err | ||
} | ||
// By comparing the current headroot, that has already gone through forkchoice, | ||
// we can assume that if equal the current block root is canonical. | ||
if !bytes.Equal(headRoot, root[:]) { | ||
return nil | ||
} | ||
// uses the newly saved block from forkchoiceUpdateWithExecution | ||
headBlock, err := s.HeadBlock(ctx) | ||
if err != nil { | ||
return err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just pass the block as part of the argument? We can save a block copy |
||
} | ||
// Mark block exits as seen so we don't include same ones in future blocks. | ||
for _, e := range b.Body().VoluntaryExits() { | ||
for _, e := range headBlock.Block().Body().VoluntaryExits() { | ||
s.cfg.ExitPool.MarkIncluded(e) | ||
} | ||
|
||
// Mark block BLS changes as seen so we don't include same ones in future blocks. | ||
if err := s.handleBlockBLSToExecChanges(b); err != nil { | ||
if err := s.markIncludedBlockBLSToExecChanges(headBlock.Block()); err != nil { | ||
return errors.Wrap(err, "could not process BLSToExecutionChanges") | ||
} | ||
|
||
// Mark attester slashings as seen so we don't include same ones in future blocks. | ||
for _, as := range b.Body().AttesterSlashings() { | ||
for _, as := range headBlock.Block().Body().AttesterSlashings() { | ||
s.cfg.SlashingPool.MarkIncludedAttesterSlashing(as) | ||
} | ||
return nil | ||
} | ||
|
||
func (s *Service) handleBlockBLSToExecChanges(blk interfaces.ReadOnlyBeaconBlock) error { | ||
if blk.Version() < version.Capella { | ||
func (s *Service) markIncludedBlockBLSToExecChanges(headBlock interfaces.ReadOnlyBeaconBlock) error { | ||
if headBlock.Version() < version.Capella { | ||
return nil | ||
} | ||
changes, err := blk.Body().BLSToExecutionChanges() | ||
changes, err := headBlock.Body().BLSToExecutionChanges() | ||
if err != nil { | ||
return errors.Wrap(err, "could not get BLSToExecutionChanges") | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there will be a dead lock.
HeadRoot
requires a read lockThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought what potuz mentioned here #12085 (comment) was to use this lock instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HeadRoot
does not require a read lock, the function takes one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok.
HeadRoot
usesheadLock
from Blockchain pkg notForkchoiceLock
. Confusing how there are two different locks