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

Remove bls_emu and use real BLS #1906

Merged
merged 4 commits into from Nov 21, 2019
Merged

Conversation

@jeanphilippeD
Copy link
Contributor

jeanphilippeD commented Nov 21, 2019

Closes #1907

Remove bls_emu: tweak quorum test as threshold in bls_emu was off by 1 in some cases (did not do the saturating_sub like Parsec and mock Parsec.

Some things still need some work (see commit comments), but it provide a reasonable baseline for further work.

This is the initial replacement, some issue remain:
 - Needs clean up
 - Security for messages is not really guarantee as any node can
   overwrite previous share by specifying invalid idx. (could do that already).
 - Combining Parsec signatures is a bit messy as it requires going
   through the elder_info to find position of all PublicIds for each
   signature.


Note:
- messages_accumulate_with_quorum was failing because bls_emu threshold
  was different from the on used in Parsec & mock Parsec. Update the
  test to match Parsec threshold.

Test:
Clippy + soak tests.
@jeanphilippeD jeanphilippeD requested review from madadam and octol Nov 21, 2019
src/chain/chain_accumulator.rs Outdated Show resolved Hide resolved
src/chain/shared_state.rs Outdated Show resolved Hide resolved
"SectionProofChain {{ genesis_key_info: {:?}, blocks: {:?}, validate: {} }}",
self.genesis_key_info,
self.blocks,
self.validate(),

This comment has been minimized.

Copy link
@madadam

madadam Nov 21, 2019

Contributor

Not sure this is a good idea. Might negatively impact performance when logging is enabled. I guess we'll see when we soak-test this.

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Nov 22, 2019

Author Contributor

Fixed with #1913

src/messages/mod.rs Outdated Show resolved Hide resolved
pk_set: BlsPublicKeySet,
proof: SectionProofChain,
) -> Result<SignedRoutingMessage> {
let mut signatures = BTreeMap::new();
let pk_share = BlsPublicKeyShare(*full_id.public_id());
let pk_share = full_id.public_key_share();
// WIP: Get more efficient.

This comment has been minimized.

Copy link
@madadam

madadam Nov 21, 2019

Contributor

Maybe we can get the fr from DKG somehow? We would have to modify parsec though.

This comment has been minimized.

Copy link
@madadam

madadam Nov 21, 2019

Contributor

Yeah, seems we do have it here: https://github.com/maidsafe/parsec/blob/master/src/key_gen/mod.rs#L404. We would just need to pass it to DkgResult.

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Nov 21, 2019

Author Contributor

SecretKeyShare::from_mut does modify the given Fr.
I'm thinking we probably want to stick with indices, though we might want to store the index or indices somewhere, but I think this is more for a follow up PR.

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Nov 22, 2019

Author Contributor

Fixed with #1913

@jeanphilippeD jeanphilippeD force-pushed the jeanphilippeD:part_3_bls branch from d2410b0 to 89e15c9 Nov 21, 2019
@jeanphilippeD jeanphilippeD changed the title WIP: Remove bls_emu and use real BLS Remove bls_emu and use real BLS Nov 21, 2019
let pk_share = BlsPublicKeyShare(*full_id.public_id());
let pk_share = full_id.public_key_share();
// WIP: Get more efficient.
let position = (0..100)

This comment has been minimized.

Copy link
@madadam

madadam Nov 21, 2019

Contributor

Wouldn't (0..elder_size) be enough? You would have to pass elder_size as argument though...

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Nov 22, 2019

Author Contributor

Fixed with #1913

@jeanphilippeD jeanphilippeD force-pushed the jeanphilippeD:part_3_bls branch from 70e10d8 to 159d12e Nov 21, 2019
@jeanphilippeD

This comment has been minimized.

Copy link
Contributor Author

jeanphilippeD commented Nov 21, 2019

Fixed clippy.

I'm looking at how to address the other comment, but the right way to do it (i.e I think keep the index with the secret key share is bigger than I would want for this PR).

@madadam is it ok to fix the not yet fixed comment in another PR so we can merge this since it has gone through soak test?

Copy link
Contributor

madadam left a comment

Assuming the position lookup in SignedRoutingMessage::new gets addressed in a followup PR then this one looks good.

@octol
octol approved these changes Nov 21, 2019
@jeanphilippeD jeanphilippeD merged commit 843cadf into maidsafe:fleming Nov 21, 2019
13 of 16 checks passed
13 of 16 checks passed
Test (ubuntu-latest) Test (ubuntu-latest)
Details
Check (ubuntu-latest)
Details
Test (windows-latest) Test (windows-latest)
Details
Check (windows-latest)
Details
Test (macOS-latest) Test (macOS-latest)
Details
Check (macOS-latest)
Details
Test (ubuntu-latest, --release --features mock)
Details
Test (ubuntu-latest, --release)
Details
Test (windows-latest, --release --features mock)
Details
Test (windows-latest, --release)
Details
Test (macOS-latest, --release --features mock)
Details
Test (macOS-latest, --release)
Details
Rustfmt
Details
Clippy
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@jeanphilippeD jeanphilippeD deleted the jeanphilippeD:part_3_bls branch Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.