Encryption of proposals in HoneyBadger #68

vkomenda · 2018-06-20T17:23:46Z

Close #41

afck · 2018-06-21T08:27:58Z

src/honey_badger.rs

                self.handle_common_subset_message(sender_id, epoch, cs_msg)
            }
+            MessageContent::DecryptionShare { proposer_id, share } => {
+                self.handle_decryption_share_message(sender_id, epoch, &proposer_id, &share)


You could pass the proposer ID and share by value to avoid the cloning.

Then clippy starts complaining on "needless" pass-by-value.

But not if you remove the .clone() in handle_decryption_share_message.

afck · 2018-06-21T08:32:18Z

src/honey_badger.rs

+                        .collect(),
+                )].into_iter()
+                    .collect()
+            });


This is another point where we'll need to think about spam protection (#43) in the future, i.e. guard against a node sending us tons of fake decryption shares for later epochs. But I don't have an idea how to do that yet.

afck · 2018-06-21T08:36:13Z

src/honey_badger.rs

+            .decrypted_selections
+            .iter()
+            .flat_map(|(proposer_id, ser_batch)| {
+                // If serialization fails, the proposer of that batch is faulty. Ignore it.


deserialization?

afck · 2018-06-21T08:42:03Z

src/honey_badger.rs

+        let mut proposer_ids: Option<BTreeSet<NodeUid>> = None;
+        if let Some(shares) = self.received_shares.get(&self.epoch) {
+            proposer_ids = Some(shares.keys().cloned().collect());
+        }


With else { return Ok(false); }, the proposer_ids wouldn't have to be an option and could be immutable.

afck · 2018-06-21T08:45:05Z

src/honey_badger.rs

+    }
+
+    /// Tries to decrypt the selection of transactions from a given proposer. Outputs `true` if and
+    /// only of decryption finished without errors.


of → if

afck · 2018-06-21T09:27:29Z

src/honey_badger.rs

+            self.try_output_batch()?;
+        }
+
+        Ok(())


We need to pk.verify_decryption_share(&share, &ciphertext) with the public key share pk of the sender as soon as we have both the ciphertext and the share. Then we drop all invalid shares, so we can be sure that any set of f + 1 share will successfully yield the proposer's input.

I created a separate issue to test my proposed implementation: #75. I suggest adding that test after we finish working on this PR.

afck · 2018-06-21T09:30:07Z

src/honey_badger.rs

+                        .collect(),
+                )].into_iter()
+                    .collect()
+            });


I think this could be simplified:

self.received_shares .entry(epoch) .or_insert_with(BTreeSet::new) .entry(proposer_id.clone()) .or_insert_with(BTreeSet::new) .insert(sender_id.clone(), share.clone());

afck · 2018-06-21T09:33:49Z

src/honey_badger.rs

+                // If serialization fails, the proposer of that batch is faulty. Ignore it.
+                bincode::deserialize::<Vec<Tx>>(&ser_batch)
+                    .ok()
+                    .map(|proposed| (proposer_id.clone(), proposed))


Something to keep in mind for #51: We could add another field (faulty_nodes?) to Batch, to report the nodes whose input didn't deserialize.

afck · 2018-06-21T09:40:55Z

src/honey_badger.rs

+            if !proposer_ids.is_empty()
+                && proposer_ids
+                    .iter()
+                    .all(|proposer_id| self.try_decrypt_proposer_selection(proposer_id))


The criteria here should probably be:

CommonSubset has already output in this epoch.

For every proposer from CommonSubset's output, we have received f + 1 valid decryption shares.

If there are at most f faulty nodes, it should be guaranteed that that eventually happens and that decryption succeeds.

Calling try_decrypt_proposer_selection every time looks like it might be expensive: wouldn't that repeatedly decrypt and overwrite the same thing? (Or should that method just return true immediately, if there's already an entry in decrypted_selections?)

afck · 2018-06-21T09:41:50Z

src/honey_badger.rs

+        let all_ciphertext_proposers: BTreeSet<_> = ciphertexts.keys().collect();
+        let all_decrypted_selection_proposers: BTreeSet<_> =
+            self.decrypted_selections.keys().collect();
+        all_ciphertext_proposers.is_subset(&all_decrypted_selection_proposers)


Can this be a proper subset or would it just be equal?

afck

Looks great!
Just one possible simplification (but maybe I'm just confused by all those nested maps).

afck · 2018-06-21T18:44:06Z

src/honey_badger.rs

@@ -60,6 +60,9 @@ pub struct HoneyBadger<Tx, NodeUid> {
    decrypted_selections: BTreeMap<NodeUid, Vec<u8>>,
    /// Ciphertexts output by Common Subset in an epoch.
    ciphertexts: BTreeMap<u64, BTreeMap<NodeUid, Ciphertext<Bls12>>>,
+    /// Those shares in `received_shares` that could not be verified on reception due to the absense


Is keeping track about those even necessary? When we receive the ciphertext, doesn't that mean that none of the existing shares have been verified yet? I.e. won't this set always either be equal to the senders whose shares we've already received (if we don't have the ciphertext yet), or empty (if we have it)?

(Also typo: absence)

This is indeed the case. Thanks for spotting this.

afck · 2018-06-22T07:52:18Z

src/honey_badger.rs

@@ -104,7 +104,10 @@ where
    }

    fn next_message(&mut self) -> Option<TargetedMessage<Self::Message, NodeUid>> {
-        self.messages.pop_front()
+        let convert = |msg: TargetedMessage<InternalMessage<NodeUid>, NodeUid>| {
+            msg.map(|int_msg| int_msg.into_external())


That probably doesn't need a closure: map(InternalMessage::into_external).

afck · 2018-06-22T08:01:37Z

src/honey_badger.rs

+                InternalMessageContent::CommonSubset(msg) => MessageContent::CommonSubset(msg),
+                InternalMessageContent::DecryptionShare { proposer_id, share } => {
+                    // FIXME: Handle errors.
+                    let share_copy = Rc::try_unwrap(share).unwrap();


I think that will usually fail: Most of the time, the local reference will live longer than the message.

I hadn't realized that Rc doesn't automatically implement the serde traits. Maybe instead of the internal-external distinction the message should just always have an Rc, but we should implement Serialize and Deserialize manually? (Or rather with some attribute #[serde(with = "a_new_rc_serde_module")].)

But maybe this is all premature optimization; I'm happy to postpone that to a separate PR.

Yes, Rc::try_unwrap() does fail here. I'll try to experiment with custom Serialize and Deserialize. There seems to be no shortcut to that and I think you are right to say it may deserve a separate PR.

…ived messages

… in messages

…s shares in messages" This reverts commit 10c442a.

vkomenda requested a review from afck June 20, 2018 17:23

afck reviewed Jun 21, 2018

View reviewed changes

vkomenda force-pushed the vk-encrypt-proposed branch from dba94e6 to 8d0180b Compare June 21, 2018 10:55

vkomenda mentioned this pull request Jun 21, 2018

Test filtering out decryption shares that fail verification #75

Closed

afck approved these changes Jun 21, 2018

View reviewed changes

afck mentioned this pull request Jun 21, 2018

README re-organization and additions #74

Merged

afck reviewed Jun 22, 2018

View reviewed changes

This was referenced Jun 22, 2018

The schedule 0, 1, get_coin(), ... #76

Merged

Reduce the memory footprint of a decryption share in HoneyBadger #77

Closed

vkomenda added 9 commits June 22, 2018 10:12

threshold encryption of proposals in HoneyBadger

c7f4994

tries to decrypt and output a batch on epoch update from already rece…

af03158

…ived messages

derived instance fixes after serde option removal

5811ead

review comments

5b0d331

added verification of decryption shares as soon as they can be verified

69b42d5

removed the unneeded pending_verification and added clone-less shares…

b7a0d64

… in messages

Revert "removed the unneeded pending_verification and added clone-les…

f2893c5

…s shares in messages" This reverts commit 10c442a.

removed the unneeded pending_verification

5778ced

removed the obsolete Bls12 type parameters

4c2e92e

vkomenda force-pushed the vk-encrypt-proposed branch from 0feb6fa to 4c2e92e Compare June 22, 2018 09:17

vkomenda merged commit e0a85a5 into master Jun 22, 2018

afck deleted the vk-encrypt-proposed branch June 26, 2018 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encryption of proposals in HoneyBadger #68

Encryption of proposals in HoneyBadger #68

vkomenda commented Jun 20, 2018

afck Jun 21, 2018

vkomenda Jun 21, 2018

afck Jun 21, 2018

afck Jun 21, 2018

afck Jun 21, 2018

afck Jun 21, 2018

afck Jun 21, 2018

afck Jun 21, 2018

vkomenda Jun 21, 2018

afck Jun 21, 2018

afck Jun 21, 2018

afck Jun 21, 2018

afck Jun 21, 2018

afck left a comment

afck Jun 21, 2018

vkomenda Jun 21, 2018

afck Jun 22, 2018

afck Jun 22, 2018

vkomenda Jun 22, 2018

vkomenda Jun 22, 2018

Encryption of proposals in HoneyBadger #68

Encryption of proposals in HoneyBadger #68

Conversation

vkomenda commented Jun 20, 2018

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

afck 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