Skip to content

Commit

Permalink
Support multiple BLS implementations (#1335)
Browse files Browse the repository at this point in the history
## Issue Addressed

NA

## Proposed Changes

- Refactor the `bls` crate to support multiple BLS "backends" (e.g., milagro, blst, etc).
- Removes some duplicate, unused code in `common/rest_types/src/validator.rs`.
- Removes the old "upgrade legacy keypairs" functionality (these were unencrypted keys that haven't been supported for a few testnets, no one should be using them anymore).

## Additional Info

Most of the files changed are just inconsequential changes to function names.

## TODO

- [x] Optimization levels
- [x] Infinity point: supranational/blst#11
- [x] Ensure milagro *and* blst are tested via CI
- [x] What to do with unsafe code?
- [x] Test infinity point in signature sets
  • Loading branch information
paulhauner committed Jul 25, 2020
1 parent 21bcc88 commit b73c497
Show file tree
Hide file tree
Showing 117 changed files with 2,988 additions and 2,442 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/test-suite.yml
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ jobs:
- uses: actions/checkout@v1
- name: Get latest version of stable Rust
run: rustup update stable
- name: Run eth2.0-spec-tests with and without fake_crypto
- name: Run eth2.0-spec-tests with blst, milagro and fake_crypto
run: make test-ef
dockerfile-ubuntu:
name: dockerfile-ubuntu
Expand Down
30 changes: 26 additions & 4 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ check-benches:
run-ef-tests:
cargo test --release --manifest-path=$(EF_TESTS)/Cargo.toml --features "ef_tests"
cargo test --release --manifest-path=$(EF_TESTS)/Cargo.toml --features "ef_tests,fake_crypto"
cargo test --release --manifest-path=$(EF_TESTS)/Cargo.toml --features "ef_tests,milagro"

# Runs only the tests/state_transition_vectors tests.
run-state-transition-tests:
Expand Down
2 changes: 1 addition & 1 deletion account_manager/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ clap_utils = { path = "../common/clap_utils" }
eth2_wallet = { path = "../crypto/eth2_wallet" }
eth2_wallet_manager = { path = "../common/eth2_wallet_manager" }
rand = "0.7.2"
validator_dir = { path = "../common/validator_dir", features = ["unencrypted_keys"] }
validator_dir = { path = "../common/validator_dir" }
tokio = { version = "0.2.21", features = ["full"] }
eth2_keystore = { path = "../crypto/eth2_keystore" }
account_utils = { path = "../common/account_utils" }
3 changes: 0 additions & 3 deletions account_manager/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
mod common;
pub mod upgrade_legacy_keypairs;
pub mod validator;
pub mod wallet;

Expand All @@ -19,15 +18,13 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
.about("Utilities for generating and managing Ethereum 2.0 accounts.")
.subcommand(wallet::cli_app())
.subcommand(validator::cli_app())
.subcommand(upgrade_legacy_keypairs::cli_app())
}

/// Run the account manager, returning an error if the operation did not succeed.
pub fn run<T: EthSpec>(matches: &ArgMatches<'_>, env: Environment<T>) -> Result<(), String> {
match matches.subcommand() {
(wallet::CMD, Some(matches)) => wallet::cli_run(matches)?,
(validator::CMD, Some(matches)) => validator::cli_run(matches, env)?,
(upgrade_legacy_keypairs::CMD, Some(matches)) => upgrade_legacy_keypairs::cli_run(matches)?,
(unknown, _) => {
return Err(format!(
"{} is not a valid {} command. See --help.",
Expand Down
149 changes: 0 additions & 149 deletions account_manager/src/upgrade_legacy_keypairs.rs

This file was deleted.

3 changes: 3 additions & 0 deletions beacon_node/beacon_chain/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ default = ["participation_metrics"]
write_ssz_files = [] # Writes debugging .ssz files to /tmp during block processing.
participation_metrics = [] # Exposes validator participation metrics to Prometheus.

[dev-dependencies]
int_to_bytes = { path = "../../consensus/int_to_bytes" }

[dependencies]
eth2_config = { path = "../../common/eth2_config" }
merkle_proof = { path = "../../consensus/merkle_proof" }
Expand Down
4 changes: 2 additions & 2 deletions beacon_node/beacon_chain/src/attestation_verification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ pub fn verify_attestation_signature<T: BeaconChainTypes>(
let _signature_verification_timer =
metrics::start_timer(&metrics::ATTESTATION_PROCESSING_SIGNATURE_TIMES);

if signature_set.is_valid() {
if signature_set.verify() {
Ok(())
} else {
Err(Error::InvalidSignature)
Expand Down Expand Up @@ -589,7 +589,7 @@ pub fn verify_signed_aggregate_signatures<T: BeaconChainTypes>(
.map_err(BeaconChainError::SignatureSetError)?,
];

Ok(verify_signature_sets(signature_sets))
Ok(verify_signature_sets(signature_sets.iter()))
}

/// Assists in readability.
Expand Down
4 changes: 2 additions & 2 deletions beacon_node/beacon_chain/src/beacon_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -862,7 +862,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
root: target_root,
},
},
signature: AggregateSignature::empty_signature(),
signature: AggregateSignature::empty(),
})
}

Expand Down Expand Up @@ -1654,7 +1654,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
},
},
// The block is not signed here, that is the task of a validator client.
signature: Signature::empty_signature(),
signature: Signature::empty(),
};

per_block_processing(
Expand Down
2 changes: 1 addition & 1 deletion beacon_node/beacon_chain/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -666,7 +666,7 @@ fn genesis_block<T: EthSpec>(
message: BeaconBlock::empty(&spec),
// Empty signature, which should NEVER be read. This isn't to-spec, but makes the genesis
// block consistent with every other block.
signature: Signature::empty_signature(),
signature: Signature::empty(),
};
genesis_block.message.state_root = genesis_state
.update_tree_hash_cache()
Expand Down
3 changes: 1 addition & 2 deletions beacon_node/beacon_chain/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use crate::observed_attesters::Error as ObservedAttestersError;
use crate::observed_block_producers::Error as ObservedBlockProducersError;
use operation_pool::OpPoolError;
use safe_arith::ArithError;
use ssz::DecodeError;
use ssz_types::Error as SszTypesError;
use state_processing::{
block_signature_verifier::Error as BlockSignatureVerifierError,
Expand Down Expand Up @@ -69,7 +68,7 @@ pub enum BeaconChainError {
AttestationCacheLockTimeout,
ValidatorPubkeyCacheLockTimeout,
IncorrectStateForAttestation(RelativeEpochError),
InvalidValidatorPubkeyBytes(DecodeError),
InvalidValidatorPubkeyBytes(bls::Error),
ValidatorPubkeyCacheIncomplete(usize),
SignatureSetError(SignatureSetError),
BlockSignatureVerifierError(state_processing::block_signature_verifier::Error),
Expand Down
6 changes: 4 additions & 2 deletions beacon_node/beacon_chain/src/snapshot_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ mod test {
use super::*;
use types::{
test_utils::{generate_deterministic_keypair, TestingBeaconStateBuilder},
BeaconBlock, Epoch, MainnetEthSpec, Signature, SignedBeaconBlock, Slot,
BeaconBlock, Epoch, MainnetEthSpec, SignedBeaconBlock, Slot,
};

const CACHE_SIZE: usize = 4;
Expand All @@ -115,7 +115,9 @@ mod test {
beacon_state_root: Hash256::from_low_u64_be(i),
beacon_block: SignedBeaconBlock {
message: BeaconBlock::empty(&spec),
signature: Signature::new(&[42], &generate_deterministic_keypair(0).sk),
signature: generate_deterministic_keypair(0)
.sk
.sign(Hash256::from_low_u64_be(42)),
},
beacon_block_root: Hash256::from_low_u64_be(i),
}
Expand Down
Loading

0 comments on commit b73c497

Please sign in to comment.