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

Align code base to v0.11 #5127

Merged
merged 423 commits into from
Apr 14, 2020
Merged

Align code base to v0.11 #5127

merged 423 commits into from
Apr 14, 2020

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented Mar 17, 2020

Builds on #4804 and incorporates all changes.

Resolves #5119

prylabs-bulldozer bot and others added 30 commits March 14, 2020 15:34
* Refactoring of initial sync (#5096)

* implements blocks queue

* refactors updateCounter method

* fixes deadlock on stop w/o start

* refactors updateSchedulerState

* more tests on schduler

* parseFetchResponse tests

* wraps up tests for blocks queue

* eod commit

* fixes data race in round robin

* revamps fetcher

* fixes race conditions + livelocks + deadlocks

* less verbose output

* fixes data race, by isolating critical sections

* minor refactoring: resolves blocking calls

* implements init-sync queue

* udpate fetch/send buffers in blocks fetcher

* blockState enum-like type alias

* refactors common code into releaseTicket()

* better gc

* linter

* minor fix to round robin

* moves original round robin into its own package

* adds enableInitSyncQueue flag

* fixes issue with init-sync service selection

* Update beacon-chain/sync/initial-sync/round_robin.go

Co-Authored-By: terence tsao <terence@prysmaticlabs.com>

* initsyncv1 -> initsyncold

* adds span

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
Co-authored-by: terence tsao <terence@prysmaticlabs.com>

* Handle rewards overflow

* Revert "Refactoring of initial sync (#5096)"

This reverts commit 3ec2a0f.

Co-authored-by: Victor Farazdagi <simple.square@gmail.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
beacon-chain/core/blocks/block_operations_test.go Outdated Show resolved Hide resolved
beacon-chain/core/blocks/eth1_data_test.go Outdated Show resolved Hide resolved
beacon-chain/core/helpers/signing_root.go Outdated Show resolved Hide resolved
deniedText := "Historical states will not be generated. Please remove usage --new-state-mgmt"
actionText := "--disable-new-state-mgmt was used. To proceed without the flag, the db will need " +
"to generate and save historical states. This process may take a while, - do you want to proceed? (Y/N)"
deniedText := "Historical states will not be generated. Please continue use --disable-new-state-mgmt"
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this supposed to say @terencechain , "please continue use --disable" on the denied text?

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh? The PR says Please continue use --disable-new-state-mgmt

Copy link
Contributor

Choose a reason for hiding this comment

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

The english just seems weird, its on the denied text for a confirmation of the usage of the flag. "Please continue use --disable" when you deny the flag? Maybe its supposed to be "Please continue using --disable"?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure we can update it

@@ -149,8 +150,15 @@ func createDepositData(privKey *bls.SecretKey, pubKey *bls.PublicKey) (*ethpb.De
if err != nil {
return nil, err
}
domain := bls.ComputeDomain(params.BeaconConfig().DomainDeposit)
di.Signature = privKey.Sign(sr[:], domain).Marshal()
domain, err := helpers.ComputeDomain(params.BeaconConfig().DomainDeposit, nil, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be changed?

Copy link
Member

Choose a reason for hiding this comment

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

why ?

"github.com/prysmaticlabs/prysm/shared/keystore"
"github.com/prysmaticlabs/prysm/shared/params"
)

func TestDepositInput_GeneratesPb(t *testing.T) {
t.Skip("To be resolved until 5119 gets in")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to stay skipped?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, please unskip it

Copy link
Contributor

Choose a reason for hiding this comment

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

Kk, on it

shared/testutil/helpers.go Outdated Show resolved Hide resolved

go_library(
name = "go_default_library",
srcs = ["slasher.pb.go"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, must've missed them in #5308 , on it.

@@ -0,0 +1,2042 @@
// Code generated by protoc-gen-gogo. DO NOT EDIT.
// source: proto/beacon/rpc/v1/slasher.proto
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer needed, we can remove. On it.

terencechain and others added 11 commits April 13, 2020 18:07
Co-Authored-By: Ivan Martinez <ivanthegreatdev@gmail.com>
Co-Authored-By: Ivan Martinez <ivanthegreatdev@gmail.com>
Co-Authored-By: Ivan Martinez <ivanthegreatdev@gmail.com>
Co-Authored-By: Ivan Martinez <ivanthegreatdev@gmail.com>
* address all comments

* set faucet

* nishant feedback

* Update beacon-chain/p2p/service.go

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
* Revert "Updates for remote keymanager (#5260)"

This reverts commit bbcd895.

* Revert "Remove keystore keymanager from validator (#5236)"

This reverts commit 4600877.

* Revert "Update eth2 wallet keymanager (#4984)"

This reverts commit 7f7ef43.

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
* remove duplicated BLS, add golang.org/x/mod

* Update BLS and restrict visibility

* fix build
* Unskip benchutil tests

* Remove protos and gaz

* Fixes
* check

* fix test

* fix size

* fix test

* more fixes

* fix test again
prestonvanloon
prestonvanloon previously approved these changes Apr 14, 2020
* Proper err handling for tests

* Lint

* Fixed rest of the tests

* Gaz

* Fixed old master tests
Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

LGTM

@prylabs-bulldozer prylabs-bulldozer bot merged commit cb045dd into master Apr 14, 2020
@delete-merged-branch delete-merged-branch bot deleted the v0.11 branch April 14, 2020 20:27
Copy link

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

A copy paste error caused two critical bugs, and tests didn't catch it sadly. Refactoring rewards in spec to be less tangled together, then tested with good encapsulation, would be good. Too many reward bugs so far :(

Also, two more possible bugs:

  • Attester eligibility should be considered in rewards and penalties processing. Currently the whole registry is used.
  • Order of operations wrong, the rewards overflow spec bug is still there in Prysm. And the test that was supposed to catch it didn't

And maxAtteserReward typo.

@@ -93,14 +95,18 @@ func attestationDelta(state *stateTrie.BeaconState, bp *Balance, v *Validator) (

// Process target reward / penalty
if v.IsPrevEpochTargetAttester && !v.IsSlashed {
r += br * bp.PrevEpochTargetAttesters / bp.CurrentEpoch
inc := params.BeaconConfig().EffectiveBalanceIncrement
rewardNumerator := br * bp.PrevEpochAttesters / inc

Choose a reason for hiding this comment

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

This is a critical bug 🪲 🐛

} else {
p += br
}

// Process head reward / penalty
if v.IsPrevEpochHeadAttester && !v.IsSlashed {
r += br * bp.PrevEpochHeadAttesters / bp.CurrentEpoch
inc := params.BeaconConfig().EffectiveBalanceIncrement
rewardNumerator := br * bp.PrevEpochAttesters / inc

Choose a reason for hiding this comment

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

This is a critical bug as well 🪲 🐛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align to spec v0.11
9 participants