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 to spec v1.0.0-rc.0 and BLSv4 #1765

Closed
wants to merge 23 commits into from

Conversation

michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Oct 13, 2020

Issue Addressed

Closes #1504
Closes #1505
Replaces #1703
Closes #1707

Proposed Changes

  • Update BLST and Milagro to versions compatible with BLSv4 spec
  • Update Lighthouse to spec v1.0.0-rc.0, and update EF test vectors
  • Use the v1.0.0 constants for MainnetEthSpec.
  • Rename InteropEthSpec -> V012LegacyEthSpec
    • Change all constants to suit the mainnet v0.12.3 specification (i.e., Medalla).
  • Deprecate the --spec flag for the lighthouse binary
    • This value is now obtained from the config_name field of the YamlConfig.
      • Built in testnet YAML files have been updated.
    • Ignore the --spec value, if supplied, log a warning that it will be deprecated
    • lcli still has the spec flag, that's fine because it's dev tooling.
  • Remove the E: EthSpec from YamlConfig
    • This means we need to deser the genesis BeaconState on-demand, but this is fine.
  • Swap the old "minimal", "mainnet" strings over to the new EthSpecId enum.
  • Always require a CONFIG_NAME field in YamlConfig (it used to have a default).

Additional Info

Lots of breaking changes, do not merge! We will likely need a Lighthouse v0.4.0 branch, and possibly a long-term v0.3.0 branch to keep Medalla alive.

@michaelsproul michaelsproul added spec_change A change related to the Eth2 spec work-in-progress PR is a work-in-progress do-not-merge consensus An issue/PR that touches consensus code, such as state_processing or block verification. crypto An issue/PR that touches cryptography code. A1 labels Oct 13, 2020
@michaelsproul
Copy link
Member Author

Likely need to delete the Altona, Medalla and Zinken configs from this branch, or else attempt some backwards-compatibility measures (that's why the tests are failing currently)

@paulhauner
Copy link
Member

Likely need to delete the Altona, Medalla and Zinken configs from this branch, or else attempt some backwards-compatibility measures (that's why the tests are failing currently)

Would the incompatibilities cause a consensus split or are they just regarding our configs?

@michaelsproul
Copy link
Member Author

Would the incompatibilities cause a consensus split or are they just regarding our configs?

I've thought about this some more and I think we would need to make an EthSpec instance for SpecV12, and a corresponding ChainSpec. That would get us most of the way there.

The only risk then would be that using BLSv4 on Medalla creates a chain split, because it is stricter about infinity pubkeys and signatures. As far as I understand it would be trivial for someone to create a pair of secret keys S and -S so that an aggregate signature of a message signed by S and a message signed by -S is infinite... although this is based on a very limited understanding and I'd like to summon @kirk-baird to confirm/deny.

If that is the case, I think the separate branch for Medalla compatibility makes sense, or maybe a feature flag...

@kirk-baird
Copy link
Member

Yep it would be very trivial to cause a consensus split on Medalla if we update since we previously allowed secret key = 0 => public key = infinity and signing any message would have signature = infinity. Where we now reject any public key = infinity.

As a result it would be trivial to make a deposit for the public key = infinity and old version would accept the deposit and new ones reject it.

P.s.

As far as I understand it would be trivial for someone to create a pair of secret keys S and -S so that an aggregate signature of a message signed by S and a message signed by -S is infinite

This use case is still valid. We still need to accept AggregateSignature = infinity to handle this. We only reject PublicKey = infinity.

@kirk-baird
Copy link
Member

This should also solve #1707

michaelsproul and others added 2 commits October 26, 2020 15:16
* Update blst and milagro_bls subgroup checking

Signed-off-by: Kirk Baird <[email protected]>

* cargo fmt

Signed-off-by: Kirk Baird <[email protected]>
@paulhauner
Copy link
Member

I'm comandeering this PR!

boat

With @michaelsproul's permission, of course.

@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 28, 2020
@paulhauner
Copy link
Member

All ready! I can't select you as a reviewer though @michaelsproul 😕

Copy link
Member Author

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looks good on the whole @paulhauner! Thanks for taking this over and getting it over the line.

I just have a few stylistic suggestions that should be addressed, then we can merge!

common/eth2_testnet_config/src/lib.rs Outdated Show resolved Hide resolved
common/eth2_testnet_config/src/lib.rs Outdated Show resolved Hide resolved
consensus/types/src/chain_spec.rs Outdated Show resolved Hide resolved
consensus/types/src/eth_spec.rs Outdated Show resolved Hide resolved
lighthouse/src/main.rs Outdated Show resolved Hide resolved
lighthouse/src/main.rs Outdated Show resolved Hide resolved
testing/ef_tests/tests/tests.rs Show resolved Hide resolved
@paulhauner
Copy link
Member

Thanks! All comments addressed. I've been jigging around in my .vimrc, perhaps I caused those weird string wrappings..

Could you please double check I set the proportional_slashing_multiplier correctly in the YAML configs, please? :)

@michaelsproul
Copy link
Member Author

Yep! All good! And my Medalla node is happy with them too ☺️

@michaelsproul
Copy link
Member Author

Merge at will!

@paulhauner
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 28, 2020
## Issue Addressed

Closes #1504 
Closes #1505
Replaces #1703
Closes #1707

## Proposed Changes

* Update BLST and Milagro to versions compatible with BLSv4 spec
* Update Lighthouse to spec v1.0.0-rc.0, and update EF test vectors
* Use the v1.0.0 constants for `MainnetEthSpec`.
* Rename `InteropEthSpec` -> `V012LegacyEthSpec`
    * Change all constants to suit the mainnet `v0.12.3` specification (i.e., Medalla).
* Deprecate the `--spec` flag for the `lighthouse` binary
    * This value is now obtained from the `config_name` field of the `YamlConfig`.
        * Built in testnet YAML files have been updated.
    * Ignore the `--spec` value, if supplied, log a warning that it will be deprecated
    * `lcli` still has the spec flag, that's fine because it's dev tooling.
* Remove the `E: EthSpec` from `YamlConfig`
    * This means we need to deser the genesis `BeaconState` on-demand, but this is fine.
* Swap the old "minimal", "mainnet" strings over to the new `EthSpecId` enum.
* Always require a `CONFIG_NAME` field in `YamlConfig` (it used to have a default).

## Additional Info

Lots of breaking changes, do not merge! ~~We will likely need a Lighthouse v0.4.0 branch, and possibly a long-term v0.3.0 branch to keep Medalla alive~~.

Co-authored-by: Kirk Baird <[email protected]>
Co-authored-by: Paul Hauner <[email protected]>
@paulhauner
Copy link
Member

bors r-

@bors
Copy link

bors bot commented Oct 28, 2020

Canceled.

@paulhauner
Copy link
Member

I had to merge #1836 into this to fix a merge conflict.

@paulhauner
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 28, 2020
## Issue Addressed

Closes #1504 
Closes #1505
Replaces #1703
Closes #1707

## Proposed Changes

* Update BLST and Milagro to versions compatible with BLSv4 spec
* Update Lighthouse to spec v1.0.0-rc.0, and update EF test vectors
* Use the v1.0.0 constants for `MainnetEthSpec`.
* Rename `InteropEthSpec` -> `V012LegacyEthSpec`
    * Change all constants to suit the mainnet `v0.12.3` specification (i.e., Medalla).
* Deprecate the `--spec` flag for the `lighthouse` binary
    * This value is now obtained from the `config_name` field of the `YamlConfig`.
        * Built in testnet YAML files have been updated.
    * Ignore the `--spec` value, if supplied, log a warning that it will be deprecated
    * `lcli` still has the spec flag, that's fine because it's dev tooling.
* Remove the `E: EthSpec` from `YamlConfig`
    * This means we need to deser the genesis `BeaconState` on-demand, but this is fine.
* Swap the old "minimal", "mainnet" strings over to the new `EthSpecId` enum.
* Always require a `CONFIG_NAME` field in `YamlConfig` (it used to have a default).

## Additional Info

Lots of breaking changes, do not merge! ~~We will likely need a Lighthouse v0.4.0 branch, and possibly a long-term v0.3.0 branch to keep Medalla alive~~.

Co-authored-by: Kirk Baird <[email protected]>
Co-authored-by: Paul Hauner <[email protected]>
@bors bors bot changed the title Update to spec v1.0.0-rc.0 and BLSv4 [Merged by Bors] - Update to spec v1.0.0-rc.0 and BLSv4 Oct 28, 2020
@bors bors bot closed this Oct 28, 2020
@michaelsproul michaelsproul deleted the spec-v1.0.0-rc branch December 1, 2020 01:21
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. crypto An issue/PR that touches cryptography code. do-not-merge ready-for-review The code is ready for review spec_change A change related to the Eth2 spec
Projects
None yet
3 participants