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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Update testnet configs, change on-disk format #1799

Closed
wants to merge 8 commits into from
Closed

[Merged by Bors] - Update testnet configs, change on-disk format #1799

wants to merge 8 commits into from

Conversation

paulhauner
Copy link
Member

@paulhauner paulhauner commented Oct 21, 2020 •
edited
Loading

Issue Addressed

Proposed Changes

  • Add DEPOSIT_CHAIN_ID and DEPOSIT_NETWORK_ID to config.yaml.
    • Pass the DEPOSIT_NETWORK_ID to the eth1::Service.
  • Remove the unused MAX_EPOCHS_PER_CROSSLINK from the altona and medalla configs (see spec commit).
  • Change from compressing the whole testnet directory, to only compressing the genesis state file. This is the only file we need to compress and not compressing the others makes them work nicely with git.
    • We can modify the boot nodes, configs, etc. without incurring an eternal binary-blob cost on our git history.
    • This change is backwards compatible (i.e., non-breaking).

Additional Info

NA

@paulhauner paulhauner added the work-in-progress PR is a work-in-progress label Oct 21, 2020
@paulhauner paulhauner mentioned this pull request Oct 21, 2020
Closed
18 tasks
@paulhauner paulhauner marked this pull request as ready for review October 21, 2020 23:01
@paulhauner paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Oct 21, 2020
@paulhauner
Copy link
Member Author

Hey @pawanjay176, would you mind giving this a look over please? I know you're familiar with it :)

pawanjay176
pawanjay176 approved these changes Oct 22, 2020
View reviewed changes
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

LGTM! just a minor nit and raising an unrelated issue.

common/eth2_testnet_config/.gitignore
@@ -1,4 +1,5 @@
testnet*/*
schlesi-*
witti-*
altona*
/altona*
Copy link
Member

Choose a reason for hiding this comment

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

Don't think we need these in gitignore. Just the built_in_testnet_configs/*/genesis.ssz should suffice no?

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 wanted to try and avoid everyone suddenly having dirty working trees when we merged it and maybe accidentally adding things with git add -A. Perhaps we can remove after a bit of time has passed.

consensus/types/src/chain_spec.rs
@@ -108,6 +108,8 @@ pub struct ChainSpec {
*/
pub eth1_follow_distance: u64,
pub seconds_per_eth1_block: u64,
pub deposit_chain_id: u64,
Copy link
Member

Choose a reason for hiding this comment

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

We are currently checking only network_id in the eth1 crate to make sure beacon node is connected to the correct eth1 chain. I think we should check chain_id as well to ensure that we aren't reading any bogus logs from an eth1 chain with a different chain id.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I made an issue here: #1815

common/eth2_config/src/lib.rs
@@ -3,6 +3,18 @@ use std::env;
use std::path::PathBuf;
use types::ChainSpec;

// A macro is used to define this constant so it can be used with `include_bytes!`.
Copy link
Member

Choose a reason for hiding this comment

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

cool trick!

@paulhauner
Copy link
Member Author

Thanks @pawanjay176!

bors r+

@paulhauner paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Oct 23, 2020
@bors
Copy link

bors bot commented Oct 23, 2020

Merge conflict.

@paulhauner
Copy link
Member Author

bors retry

bors bot pushed a commit that referenced this pull request Oct 25, 2020
@paulhauner
Update testnet configs, change on-disk format (#1799)
eba51f0 
## Issue Addressed

- Related to #1691

## Proposed Changes

- Add `DEPOSIT_CHAIN_ID` and `DEPOSIT_NETWORK_ID` to `config.yaml`.
    - Pass the `DEPOSIT_NETWORK_ID` to the `eth1::Service`.
- Remove the unused `MAX_EPOCHS_PER_CROSSLINK` from the `altona` and `medalla` configs (see [spec commit](ethereum/consensus-specs@2befe90#diff-efb845ac2ebd4aafbc23df40f47ce25699255064e99d36d0406d0a14ca7953ec)).
- Change from compressing the whole testnet directory, to only compressing the genesis state file. This is the only file we need to compress and *not* compressing the others makes them work nicely with git.
    - We can modify the boot nodes, configs, etc. without incurring an eternal binary-blob cost on our git history.
    - This change is backwards compatible (i.e., non-breaking).

## Additional Info

NA
@bors
Copy link

bors bot commented Oct 25, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title Update testnet configs, change on-disk format [Merged by Bors] - Update testnet configs, change on-disk format Oct 25, 2020
@bors bors bot closed this Oct 25, 2020
@pawanjay176 pawanjay176 mentioned this pull request Nov 2, 2020
Closed
bors bot pushed a commit that referenced this pull request Nov 2, 2020
@pawanjay176
Update to discv5 bootnodes (#1849)
863ee7c 
## Issue Addressed

We seem to have roll backed to old discv5 bootnodes with #1799 because of which fresh nodes with no cached peers cannot find any peers.

## Proposed Changes

Updates `boot_enr.yaml` to discv5.1 bootnodes.
@paulhauner paulhauner deleted the update-testnet-configs branch January 20, 2021 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pawanjay176 pawanjay176 pawanjay176 approved these changes

Assignees
No one assigned
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@paulhauner @pawanjay176