Skip to content
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

Refactor process operation function arguments #8767

Merged
merged 11 commits into from
Apr 15, 2021

Conversation

terencechain
Copy link
Member

Motivated by #8638

Refactor these arguments to take the actual object instead of versioned block. They can be reused for Altair.

  • ProcessBlockHeaderNoVerify: *ethpb.BeaconBlock-> slot types.Slot, proposerIndex types.ValidatorIndex, parentRoot, bodyRoot []byte
  • ProcessEth1DataInBlock: *ethpb.SignedBeaconBlock -> *ethpb.Eth1Data
  • ProcessRandaoNoVerify: *ethpb.BeaconBlockBody -> []byte
  • ProcessProposerSlashings: *ethpb.SignedBeaconBlock -> []*ethpb.ProposerSlashing
  • ProcessAttesterSlashings: *ethpb.SignedBeaconBlock ->[]*ethpb.AttesterSlashing
  • ProcessVoluntaryExits: *ethpb.SignedBeaconBlock -> []*ethpb.SignedVoluntaryExit

This is the first part. Expect many parts.

@terencechain terencechain self-assigned this Apr 15, 2021
@terencechain terencechain requested a review from a team as a code owner April 15, 2021 00:13
@terencechain terencechain added the Ready For Review A pull request ready for code review label Apr 15, 2021
@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

Merging #8767 (ef38c43) into develop (405e2a1) will decrease coverage by 0.00%.
The diff coverage is 91.22%.

@@             Coverage Diff             @@
##           develop    #8767      +/-   ##
===========================================
- Coverage    60.54%   60.53%   -0.01%     
===========================================
  Files          519      519              
  Lines        36262    36271       +9     
===========================================
+ Hits         21954    21957       +3     
  Misses       11144    11144              
- Partials      3164     3170       +6     

rkapka
rkapka previously approved these changes Apr 15, 2021
Copy link
Contributor

@farazdagi farazdagi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great solution, to both allow us to re-use code, and without getting into wrappers hell!

Just one comment.

@@ -32,25 +32,33 @@ var processDepositsFunc = func(ctx context.Context, s iface.BeaconState, blk *et
return b.ProcessDeposits(ctx, s, blk.Block.Body.Deposits)
}
var processProposerSlashingFunc = func(ctx context.Context, s iface.BeaconState, blk *ethpb.SignedBeaconBlock) (iface.BeaconState, error) {
return b.ProcessProposerSlashings(ctx, s, blk, v.SlashValidator)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, within processor function blk and blk.Body was checked for nilness. If there are code paths that depended on that check, we might end up with panics (this method is used for ProcessBlock() when going through processing pipeline, no extra checks there either).

NB: The processDepositsFunc() (above) doesn't check for block/body nilness, and we seem to be ok. So, if you absolutely sure that check was redundant, and we can have a pipeline items w/o it, please, feel free to dismiss the review. Otherwise, checks should be added to processAttesterSlashingFunc, processEth1DataFunc(), and processExitFunc()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better safe than sorry. I added VerifyNilBeaconBlock to ProcessBlock. Thanks!

@prylabs-bulldozer prylabs-bulldozer bot merged commit 169cd78 into develop Apr 15, 2021
@delete-merged-branch delete-merged-branch bot deleted the update-arguments branch April 15, 2021 13:58
@terencechain terencechain mentioned this pull request Apr 19, 2021
62 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants