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 to spec v1.0.0 #7469

Merged
merged 27 commits into from
Nov 13, 2020
Merged

Align to spec v1.0.0 #7469

merged 27 commits into from
Nov 13, 2020

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented Oct 8, 2020

What goes in here?
Items that are not backwards compatible with current master and medalla testnet

Notable components that should be there

  • Consensus params changes
  • BLSv4
  • Disv5

Change list:

  • Mainnet rewards and penalty params
  • New beacon state length for eth1 data voting
  • New mainnet and minimal spec tests
  • Toledo config
  • Pyrmont config

@terencechain terencechain changed the title Align to spec v1.0.0 release candidate [DO NOT MERGE] Align to spec v1.0.0 release candidate Oct 8, 2020
@stale
Copy link

stale bot commented Oct 20, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale There hasn't been any activity here in some time... label Oct 20, 2020
@rkapka rkapka removed the Stale There hasn't been any activity here in some time... label Oct 22, 2020
prestonvanloon added a commit that referenced this pull request Oct 26, 2020
@stale
Copy link

stale bot commented Oct 31, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale There hasn't been any activity here in some time... label Oct 31, 2020
@terencechain terencechain removed the Stale There hasn't been any activity here in some time... label Nov 4, 2020
@codecov
Copy link

codecov bot commented Nov 6, 2020

Codecov Report

Merging #7469 (a70d61f) into master (5f92395) will increase coverage by 0.04%.
The diff coverage is 25.80%.

@@            Coverage Diff             @@
##           master    #7469      +/-   ##
==========================================
+ Coverage   62.03%   62.07%   +0.04%     
==========================================
  Files         430      431       +1     
  Lines       30475    30451      -24     
==========================================
- Hits        18905    18903       -2     
+ Misses       8628     8603      -25     
- Partials     2942     2945       +3     

@terencechain terencechain changed the title [DO NOT MERGE] Align to spec v1.0.0 release candidate Align to spec v1.0.0 Nov 12, 2020
@@ -16,19 +16,6 @@ import (

var runAmount = 25

func TestExecuteStateTransition_FullBlock(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm removing this test. This is actually more housekeeping work than good. Whenever there's a change to a beacon object, this test requires a manual update

@terencechain terencechain added Ready For Review A pull request ready for code review and removed In Progress labels Nov 12, 2020
@terencechain terencechain marked this pull request as ready for review November 12, 2020 21:47
rauljordan
rauljordan previously approved these changes Nov 12, 2020
SlotsPerHistoricalRoot: 8192,
MinValidatorWithdrawabilityDelay: 256,
ShardCommitteePeriod: 256,
MinEpochsToInactivityPenalty: 4,
Eth1FollowDistance: 1024,
Eth1FollowDistance: 2048,
SafeSlotsToUpdateJustified: 8,
SecondsPerETH1Block: 13,
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't match spec. https://github.com/ethereum/eth2.0-specs/blob/579da6d2dc734b269dbf67aa1004b54bb9449784/configs/mainnet/phase0.yaml#L48.

In reality, eth1 block times are 13 seconds on average and Prysm's eth1 connection sends too many requests this value is wrong. Thanks to @nisdas for investigating. Discord discussion link

Copy link
Member

Choose a reason for hiding this comment

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

I've asked @nisdas to file an issue so we can outline our spec config deviation and potentially have the spec correct this value.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's also worth noting that current master has been at 13s, This v1.0.0 branch did not make this particular change.
With that said, this was the only field that did not align with latest config

https://github.com/prysmaticlabs/prysm/blob/master/shared/params/mainnet_config.go#L85

Copy link
Member

Choose a reason for hiding this comment

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

SlotsPerHistoricalRoot: 8192,
MinValidatorWithdrawabilityDelay: 256,
ShardCommitteePeriod: 256,
MinEpochsToInactivityPenalty: 4,
Eth1FollowDistance: 1024,
Eth1FollowDistance: 2048,
SafeSlotsToUpdateJustified: 8,
SecondsPerETH1Block: 13,
Copy link
Member

Choose a reason for hiding this comment

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

I've asked @nisdas to file an issue so we can outline our spec config deviation and potentially have the spec correct this value.

@prylabs-bulldozer prylabs-bulldozer bot merged commit 5fdb916 into master Nov 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the v1.0.0-rc.0 branch November 13, 2020 01:00
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

4 participants