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

[Merged by Bors] - Update consensus code and tests to v0.12.3 #1655

Closed
wants to merge 6 commits into from

Conversation

michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Sep 23, 2020

Proposed Changes

Update test vectors for v0.12.3, and introduced configurable proportional_slashing_multiplier.

Also makes YamlConfig a bit safer by making every field access in apply_to_chain_spec explicit, and removing the #[serde(default)] attribute, which would instantiate missing fields to type defaults! Risky!

@michaelsproul michaelsproul added ready-for-review The code is ready for review A0 labels Sep 23, 2020
@michaelsproul michaelsproul added work-in-progress PR is a work-in-progress ready-for-review The code is ready for review and removed ready-for-review The code is ready for review work-in-progress PR is a work-in-progress labels Sep 24, 2020
@michaelsproul michaelsproul changed the title Spec v0.12.3 Update consensus code and tests to v0.12.3 Sep 25, 2020
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Looks great!

No changes required, I just made a comment :)

@@ -642,10 +639,8 @@ impl YamlConfig {
}

pub fn apply_to_chain_spec<T: EthSpec>(&self, chain_spec: &ChainSpec) -> Option<ChainSpec> {
Copy link
Member

Choose a reason for hiding this comment

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

I notice we don't check config_name in this function.

I think this is fine, it might be nice to name them different things but have the same values. IMO we don't really care about the name.

Happy to leave this as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, part of the problem with that would be backwards compatibility, the Altona and Medalla configs don't have the config_name param at all (I guess we could default to mainnet). But happy to leave as-is.

@paulhauner
Copy link
Member

paulhauner commented Sep 26, 2020

For the record, I've got the new v0.12.3 gossip verification conditions in a PR here: #1667.

I could change the base for that PR to this branch, if we want an atomic merge.

@michaelsproul
Copy link
Member Author

Thanks Paul!

bors r+

bors bot pushed a commit that referenced this pull request Sep 26, 2020
## Proposed Changes

Update test vectors for v0.12.3, and introduced configurable `proportional_slashing_multiplier`.

Also makes `YamlConfig` a bit safer by making every field access in `apply_to_chain_spec` explicit, and removing the `#[serde(default)]` attribute, which would instantiate missing fields to type defaults! Risky!
@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Sep 26, 2020
@bors bors bot changed the title Update consensus code and tests to v0.12.3 [Merged by Bors] - Update consensus code and tests to v0.12.3 Sep 26, 2020
@bors bors bot closed this Sep 26, 2020
@michaelsproul michaelsproul deleted the spec-v0.12.3 branch September 26, 2020 05:33
@michaelsproul michaelsproul added the consensus An issue/PR that touches consensus code, such as state_processing or block verification. label Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus An issue/PR that touches consensus code, such as state_processing or block verification. ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants