Secure Message Delivery initial changes #1714

fizyk20 · 2019-07-16T13:12:18Z

This PR makes some initial changes related to SMD:

Add a BLS emulation layer in order to enable writing code that pretends to use BLS before BLS is merged in
Add our_history and their_keys to the shared state
Update our_history together with our_infos

Closes #1672
Closes #1673
Closes #1674
Closes #1675

jeanphilippeD

Looks good a couple of comments.
Mostly can we test the new code?

jeanphilippeD · 2019-07-16T13:20:03Z

src/chain/bls_emu.rs

+
+impl fmt::Debug for PublicKey {
+    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+        write!(formatter, "[BLS PublicKey]")


Maybe use the SectionInfo debug value here?

Right. This was pre-SectionInfo, but it makes sense now 👍

src/chain/bls_emu.rs

jeanphilippeD · 2019-07-16T13:25:26Z

src/chain/shared_state.rs

+
+#[derive(Clone, PartialEq, Eq)]
+pub struct SectionProofBlock {
+    key: PublicKey,


Would it make sense as BlsPublicKey and BlsSignature?
I'm thinking having 2 different crypto keys/signature will cause confusion if we are not explicit.

I was kind of aiming for a drop-in replacement of BLS. With these names, we can just replace use bls_emu::... with use threshold_crypto::... and it should (mostly) just work. I can use bls_emu::PublicKey as BlsPublicKey etc., though.

Yes, could also do that at the crate level in lib.rs so you only do it once and then use crate::{BlsPublicKey, ...}

src/chain/chain.rs

jeanphilippeD

Looks good to me.

jeanphilippeD

Looks good

maqi · 2019-07-18T09:08:12Z

src/chain/bls_emu.rs

+pub struct PublicKeyShare(PublicId);
+
+#[derive(Clone, PartialEq, Eq)]
+pub struct Signature {


shall the name of the struct to be Signatures ?
Also, as it only contains one field, shall it be Signatures(BTreeMap<PublicId, SignatureShare>) ?

This struct is supposed to emulate the BLS Signature type, hence it's singular - with BLS, this would be indeed just a single signature, here we just emulate the functionality using a set of signatures.
I could make it a tuple struct, though.

Quite happy with the struct with a name as it is personally.

maqi · 2019-07-18T09:11:34Z

src/chain/bls_emu.rs

+}
+
+#[derive(Clone, PartialEq, Eq)]
+pub struct PublicKey(PublicKeySet);


I got little bit confused with the first glance of this PublicKey(PublicKeySet).
Also, the name of the file is bls_emu.rs.
However the comment at #1714 (comment) mentioned BlsPublicKey as well for the shared state.
I am not sure, however shall we use Bls... here ?

Like with Signature - this is an emulated implementation of threshold_crypto::PublicKey. I want to keep the names the same as they are in threshold_crypto, so that when we merge BLS, we can just replace this module with threshold_crypto crate and keep the rest of the code mostly intact.

In this case, we would replace this in lib.rs:
pub use crate::chain::bls_emu::PublicKey as BlsPublicKey;
with this:
pub use threshold_crypto::PublicKey as BlsPublicKey;
when BLS is merged.

maqi · 2019-07-18T09:18:08Z

src/chain/bls_emu.rs

+
+    pub fn from_section_info(threshold: usize, sec_info: SectionInfo) -> Self {
+        Self {
+            threshold,


just wondering, can the threshold be deduced from the section info?
and in which case the threshold can be un-related to the section info?

In our use cases, it probably can - but I wanted to keep the code as generic as possible. This part probably won't make it to the code after BLS is merged, so maybe coupling the threshold with the section info here is not such a bad idea. What do you think @jeanphilippeD ?

I guess you can make that function non public as well.
Since it is code that is going to go away, I'd go for whatever is simpler.

It seem it may be simplifying code & test if you use your delivery_group function inside it instead of taking the threshold param? would it?

(as in you are using in in PublicKey, so why not moving it into PublicKeySet?)

Yeah, I originally wanted to calculate the threshold completely outside this module to make the functionality boundaries more clear (BLS won't be aware of our threshold requirements, we'll have to pass it the threshold we want explicitly), but then I did this thing in PublicKey. I'm not sure which is better - make the method in PublicKey also take the threshold from outside, or just move this into PublicKeySet. Might be that it's the latter, as this code won't work with real BLS, anyway (we won't be able to derive keys from SectionInfos, we'll have to get them from DKG).

Correct me if I'm wrong, but we will not pass the threshold to either signature or public key, only the DKG.

The way we'll know we reach the threshold is if the combined signature is valid against the public key.

If anything, we would use the threshold directly in the signature validation. But where it is fine too (i.e pre-computed once and cached). and I don't want us spend too much time on something that is going to be disappearing soon-ish :-)

(also I still assume we'll have to fix stuff when moving to real BLS).

Yes, I think you're right - the threshold is passed to the DKG, then it is actually stored in the resulting PublicKeySet IIRC (it has to know what the degree of the polynomial needs to be when combining the signatures).

maqi · 2019-07-18T09:19:56Z

src/chain/bls_emu.rs

+            .map(|(pk, ss)| (pk.0, *ss))
+            .collect();
+        if sigs.len() <= self.threshold {
+            None


the name of the function is combine, however here seems a threshold screening ?
if it's really required, would it be better to at least have a comment to explain?

This emulates threshold_crypto::PublicKeySet::combine_signatures, which can only succeed if there are more than threshold valid signatures. Again, the reason here is the emulation of BLS functionality. I can add a comment 👍

yeah, pls add a comment then. thx

maqi · 2019-07-18T09:25:56Z

src/chain/bls_emu.rs

+impl PublicKey {
+    pub fn from_section_info(sec_info: &SectionInfo) -> Self {
+        let threshold = delivery_group_size(sec_info.members().len()) - 1;
+        PublicKey(PublicKeySet::from_section_info(threshold, sec_info.clone()))


as mentioned above, I'd prefer the threshold to be calculated inside the PublicKeySet::from_section_info function.

maqi · 2019-07-18T09:35:43Z

src/chain/shared_state.rs

+        self.blocks.push(block);
+    }
+
+    #[allow(unused)]


if this function is only to be used during test, shall we use cfg(test) guard instead of the allow(unused)?

It's only used in tests now, but it is intended to be used in production code as well in the future - it will probably be useful during verification of chains attached to messages.

maqi · 2019-07-18T09:36:07Z

src/chain/shared_state.rs

+        let mut current_pk = &self.genesis_pk;
+        for block in &self.blocks {
+            if !block.verify_with_pk(current_pk) {
+                return false;


maybe log some info about which block fails the verification?

What kind of message would you have in mind, though? The blocks don't really contain any meaningful data that would make it easier to identify the offender - it's a public key and a signature, neither is very readable.

maybe the public key (which is a public id)?
In case of failure, we need to find out who is failing anyway, right?

The public key here is more of a set of public IDs, not a single one - and it actually contains a full section info... But it makes sense 👍

right, then maybe that shall be inside the PublicKey::verify, we shall log which one is failing or whether it's not reaching the threshold.
Maybe no need to log whehter reaching the threshold.
However failing a signature is some behaviour shall be noticed.

Would not spend too much time on that, when we debug the first issue we will find out what log we need and we can add it then (it might be neither of the options, as I think we will probably want the id of the current node).

So I'd say, add something minimal if that make sense otherwise happy to leave it as is.

I think I'd leave it as is for now, then - adding some logging to the verification with a single public ID would make the code there quite a bit uglier, which we can probably do temporarily when debugging, but I'm not sure if it's worth it to have such code by default.

fizyk20 added the wip label Jul 16, 2019

jeanphilippeD reviewed Jul 16, 2019

View reviewed changes

fizyk20 force-pushed the smd branch from 5fd9fea to 6fff86f Compare July 16, 2019 16:11

jeanphilippeD previously approved these changes Jul 17, 2019

View reviewed changes

fizyk20 dismissed jeanphilippeD’s stale review via 8cce65c July 17, 2019 15:34

fizyk20 force-pushed the smd branch from 1b56457 to 8cce65c Compare July 17, 2019 15:34

fizyk20 changed the title ~~WIP: Secure Message Delivery initial changes~~ Secure Message Delivery initial changes Jul 17, 2019

fizyk20 removed the wip label Jul 17, 2019

fizyk20 force-pushed the smd branch from 8cce65c to a92e10b Compare July 17, 2019 16:01

jeanphilippeD previously approved these changes Jul 18, 2019

View reviewed changes

maqi reviewed Jul 18, 2019

View reviewed changes

fizyk20 dismissed jeanphilippeD’s stale review via a03765a July 18, 2019 16:21

fizyk20 force-pushed the smd branch from a92e10b to a03765a Compare July 18, 2019 16:21

fizyk20 added 4 commits July 18, 2019 18:21

feat/chain: store our_history in the shared state

5948132

feat/chain: store their_keys in the shared state

83f4e3d

feat/chain: update our_history together with our_infos

925aba9

tests/chain: add some tests for the new functionality

a03765a

maqi approved these changes Jul 19, 2019

View reviewed changes

jeanphilippeD approved these changes Jul 19, 2019

View reviewed changes

jeanphilippeD merged commit 1622e11 into maidsafe:fleming Jul 19, 2019

fizyk20 deleted the smd branch July 19, 2019 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secure Message Delivery initial changes #1714

Secure Message Delivery initial changes #1714

fizyk20 commented Jul 16, 2019 •

edited

jeanphilippeD left a comment

jeanphilippeD Jul 16, 2019

fizyk20 Jul 16, 2019

jeanphilippeD Jul 16, 2019

fizyk20 Jul 16, 2019

jeanphilippeD Jul 16, 2019

jeanphilippeD left a comment

jeanphilippeD left a comment

maqi Jul 18, 2019

fizyk20 Jul 18, 2019

jeanphilippeD Jul 18, 2019

maqi Jul 18, 2019

fizyk20 Jul 18, 2019

maqi Jul 18, 2019

fizyk20 Jul 18, 2019

jeanphilippeD Jul 18, 2019

jeanphilippeD Jul 18, 2019

fizyk20 Jul 18, 2019

jeanphilippeD Jul 18, 2019

jeanphilippeD Jul 18, 2019

fizyk20 Jul 18, 2019

maqi Jul 18, 2019

fizyk20 Jul 18, 2019

maqi Jul 18, 2019

maqi Jul 18, 2019

maqi Jul 18, 2019

fizyk20 Jul 18, 2019

maqi Jul 18, 2019

fizyk20 Jul 18, 2019

maqi Jul 19, 2019

fizyk20 Jul 19, 2019

maqi Jul 19, 2019

jeanphilippeD Jul 19, 2019

fizyk20 Jul 19, 2019

Secure Message Delivery initial changes #1714

Secure Message Delivery initial changes #1714

Conversation

fizyk20 commented Jul 16, 2019 • edited

jeanphilippeD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanphilippeD left a comment

Choose a reason for hiding this comment

jeanphilippeD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fizyk20 commented Jul 16, 2019 •

edited