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

Remove remaining instances of proto.clone() #4806

Merged

Conversation

garyschulteog
Copy link
Contributor

@garyschulteog garyschulteog commented Feb 9, 2020

TODO(you): choose "part of" or "resolves" and the associated github issue #.

Resolves #4757


Description

  • use existing Copy* methods from state/getters.go
  • add Copy* methods to state/getters.go to remove proto.Clone() from initial-sync
  • resolve test breakage fallout
    • most breakage seems due to the different resulting object between proto.Clone and the Copy* methods; the latter yields deeper copies of sparse objects which serialize differently

Write why you are making the changes in this pull request

Write a summary of the changes you are making

Link anything that would be helpful or relevant to the reviewers

@claassistantio
Copy link

claassistantio commented Feb 9, 2020

CLA assistant check
All committers have signed the CLA.

@terencechain terencechain changed the title Remove remaining instances of proto.clone() [WIP] Remove remaining instances of proto.clone() Feb 9, 2020
@terencechain
Copy link
Member

Thanks for opening this up!

@garyschulte garyschulte force-pushed the prysm-4757-no-proto-clone branch 3 times, most recently from 70c55e0 to 57fe25d Compare February 10, 2020 06:44
@codecov
Copy link

codecov bot commented Feb 10, 2020

Codecov Report

Merging #4806 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4806   +/-   ##
=======================================
  Coverage   26.45%   26.45%           
=======================================
  Files         187      187           
  Lines       13348    13348           
=======================================
  Hits         3531     3531           
  Misses       9215     9215           
  Partials      602      602           

@garyschulteog garyschulteog changed the title [WIP] Remove remaining instances of proto.clone() Remove remaining instances of proto.clone() Feb 10, 2020
@garyschulteog garyschulteog marked this pull request as ready for review February 10, 2020 07:09
@garyschulte garyschulte force-pushed the prysm-4757-no-proto-clone branch 2 times, most recently from 9a00a07 to aaf059e Compare February 10, 2020 08:41
@garyschulteog garyschulteog changed the title Remove remaining instances of proto.clone() [WIP]Remove remaining instances of proto.clone() Feb 10, 2020
@garyschulteog garyschulteog changed the title [WIP]Remove remaining instances of proto.clone() Remove remaining instances of proto.clone() Feb 11, 2020
@garyschulteog garyschulteog changed the title Remove remaining instances of proto.clone() [WIP]Remove remaining instances of proto.clone() Feb 11, 2020
@prestonvanloon
Copy link
Member

Thanks for doing this @garyschulte. Can I recommend that we merge this PR incrementally? If you have some changes that are ready to go, lets put this PR for review and continue progress in a follow up PR.

@garyschulte garyschulte force-pushed the prysm-4757-no-proto-clone branch 4 times, most recently from 974e369 to 8e98a18 Compare February 12, 2020 08:26
@garyschulteog
Copy link
Contributor Author

garyschulteog commented Feb 12, 2020

Thanks for doing this @garyschulte. Can I recommend that we merge this PR incrementally? If you have some changes that are ready to go, lets put this PR for review and continue progress in a follow up PR.

I removed the use of CopySignedBeaconBlock in ReceiveBlockNoVerify for now. reflect.DeepEquals reports that objects cloned via CopySignedBeaconBlock(block) are identical to those cloned with proto.Clone(block).(*ethpb.SignedBeaconBlock). However, during testing, intial sync always panics at slot 860 during testing if using the copy method rather than proto.Clone there.

I am have been running this branch for a few hours on the test net on a synced node, and am have nearly completely synced from 0 on another machine with this branch. To the best of my knowledge it is safe, but I honestly don't understand the root cause for the panics when using that clone method during sync.

Edit: resolved! so pleased :)

@garyschulteog garyschulteog changed the title [WIP]Remove remaining instances of proto.clone() Remove remaining instances of proto.clone() Feb 12, 2020
beacon-chain/state/getters.go Outdated Show resolved Hide resolved
beacon-chain/state/getters.go Outdated Show resolved Hide resolved
beacon-chain/state/getters.go Outdated Show resolved Hide resolved
@0xKiwi
Copy link
Contributor

0xKiwi commented Feb 12, 2020

Fixed the suggestions for ya @garyschulte 😄

beacon-chain/state/getters.go Outdated Show resolved Hide resolved
beacon-chain/state/getters.go Outdated Show resolved Hide resolved
beacon-chain/state/getters.go Outdated Show resolved Hide resolved
beacon-chain/state/getters.go Outdated Show resolved Hide resolved
rauljordan
rauljordan previously approved these changes Feb 13, 2020
@0xKiwi 0xKiwi added Ready For Review A pull request ready for code review and removed In Progress labels Feb 13, 2020
0xKiwi
0xKiwi previously approved these changes Feb 13, 2020
prestonvanloon
prestonvanloon previously approved these changes Feb 13, 2020
nisdas
nisdas previously approved these changes Feb 13, 2020
@prylabs-bulldozer prylabs-bulldozer bot merged commit 27ec40f into prysmaticlabs:master Feb 14, 2020
cryptomental pushed a commit to cryptomental/prysm that referenced this pull request Feb 24, 2020
* prysm-4757 remove proto.Clone() in favor of existing getters.Copy* methods
* prysm-4757 added a bunch of copy methods, and broke some tests
* squash commits
 fix tests and getter implementations
 remove usage of CopySignedBeaconBlock from ReceiveBlockNoVerify
* correctly copy Deposit proof and remove proto.clone() again
* Merge branch 'master' into prysm-4757-no-proto-clone
* Merge branch 'master' into prysm-4757-no-proto-clone
* Fix for comments, inline possible function calls
* Merge branch 'master' of https://github.com/prysmaticlabs/Prysm into prysm-4757-no-proto-clone
* Merge branch 'master' into prysm-4757-no-proto-clone
* updated with feedback from review
* Merge branch 'master' into prysm-4757-no-proto-clone
* Merge branch 'master' into prysm-4757-no-proto-clone
* Merge branch 'master' into prysm-4757-no-proto-clone
cryptomental pushed a commit to cryptomental/prysm that referenced this pull request Feb 28, 2020
* prysm-4757 remove proto.Clone() in favor of existing getters.Copy* methods
* prysm-4757 added a bunch of copy methods, and broke some tests
* squash commits
 fix tests and getter implementations
 remove usage of CopySignedBeaconBlock from ReceiveBlockNoVerify
* correctly copy Deposit proof and remove proto.clone() again
* Merge branch 'master' into prysm-4757-no-proto-clone
* Merge branch 'master' into prysm-4757-no-proto-clone
* Fix for comments, inline possible function calls
* Merge branch 'master' of https://github.com/prysmaticlabs/Prysm into prysm-4757-no-proto-clone
* Merge branch 'master' into prysm-4757-no-proto-clone
* updated with feedback from review
* Merge branch 'master' into prysm-4757-no-proto-clone
* Merge branch 'master' into prysm-4757-no-proto-clone
* Merge branch 'master' into prysm-4757-no-proto-clone
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.

Eliminate All Uses of Proto.Clone From Prysm
8 participants