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

[crypto] Switches LedgerInfo to nextgen_crypto #373

Merged
merged 1 commit into from Jul 30, 2019

Conversation

@huitseeker
Copy link
Contributor

commented Jul 29, 2019

This commit modifies the following behavior:

  • Switches LedgerInfo to use Ed25519Signature

Why this is better:

  • Progress of conversion to the nextgen-crypto API
  • this includes QuorumCertificate, VoteMsg, and transfers the whole of Consensus to nextgen

Why this is worse:

  • breaking change, as LedgerInfo (etc) are serialized

Test:

  • existing unit tests (type-only modifications)

Follow-up:

  • more conversion, esp. transaction.rs
@huitseeker huitseeker self-assigned this Jul 29, 2019
@huitseeker huitseeker added the breaking label Jul 29, 2019
@huitseeker huitseeker changed the title Switches LedgerInfo to nextgen_crypto [crypto] Switches LedgerInfo to nextgen_crypto Jul 29, 2019
any::<[u8; 32]>()
.prop_map(|seed| {
let mut rng: StdRng = SeedableRng::from_seed(seed);
let (private_key, public_key) = generate_keypair(&mut rng);

This comment has been minimized.

Copy link
@kchalkias

kchalkias Jul 29, 2019

Contributor

Just realized that generate_keypair is not annotated for testing. Let's take the opportunity and update this as well.

This comment has been minimized.

Copy link
@huitseeker

huitseeker Jul 29, 2019

Author Contributor

I would, but that annotation would be near-impossible to propagate through the large amount of proptest code in types without #356. In fact, propagating this restriction pretty much is #356

@huitseeker huitseeker force-pushed the huitseeker:nextgen_ledgerinfo branch from 6921b68 to fc64709 Jul 29, 2019
* This commit modifies the following behavior:
- Switches LedgerInfo to use Ed25519Signature

* Why this is better:
- Progress of conversion to the nextgen-crypto API
- this includes QuorumCertificate, VoteMsg, and transfers the whole of Consensus to nextgen

** Why this is worse:
- breaking change, as LedgerInfo (etc) are serialized

** Test:
- existing unit tests (type-only modifications)

** Follow-up:
- more conversion, esp. `transaction.rs`
@huitseeker huitseeker force-pushed the huitseeker:nextgen_ledgerinfo branch from fc64709 to 6e09e2e Jul 29, 2019
@huitseeker huitseeker added the crypto label Jul 29, 2019
@huitseeker huitseeker requested review from kchalkias and kevinlewi Jul 29, 2019
Copy link
Contributor

left a comment

LGTM

@huitseeker huitseeker merged commit dfdcbb6 into libra:master Jul 30, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: terraform Your tests passed on CircleCI!
Details
huitseeker added a commit to huitseeker/libra that referenced this pull request Jul 31, 2019
*What this changes*

- make `libra_fuzzer` call into the `"testing"` feature of `vm_runtime_types`
- mask a strategy introduced in libra#373 that could not be masked until libra#356 was introduced

*Test*

- to make sure this doesn't break, I ran `find ./ -iname Cargo.toml -execdir cargo check --all-targets \;` (but it costs CPU)
- another option is `cargo check --all-targets --bins -p libra-node -p libra_swarm -p libra_fuzzer` but it requires keeping in mind an up-to-date list of relevant binaries
huitseeker added a commit that referenced this pull request Jul 31, 2019
*What this changes*

- make `libra_fuzzer` call into the `"testing"` feature of `vm_runtime_types`
- mask a strategy introduced in #373 that could not be masked until #356 was introduced

*Test*

- to make sure this doesn't break, I ran `find ./ -iname Cargo.toml -execdir cargo check --all-targets \;` (but it costs CPU)
- another option is `cargo check --all-targets --bins -p libra-node -p libra_swarm -p libra_fuzzer` but it requires keeping in mind an up-to-date list of relevant binaries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.