Allow arbitrary validator set changes in DHB. #339

afck · 2018-11-14T11:39:30Z

This replaces NodeChange with a full list of IDs and public keys, instead of just a single to-be-added or to-be-removed node, to allow completely replacing the set of validators by any arbitrary new set in a single key generation step.

Closes #330.

This replaces `NodeChange` with a full list of IDs and public keys, instead of just a single to-be-added or to-be-removed node, to allow completely replacing the set of validators by any arbitrary new set in a single key generation step.

afck · 2018-11-14T12:04:30Z

I prepared the hydrabadger update here: https://github.com/poanetwork/hydrabadger/tree/afck-change

vkomenda

A landmark change! A few remarks.

vkomenda · 2018-11-14T13:41:24Z

src/dynamic_honey_badger/change.rs

-    // Add or Remove a node from the set of validators
-    NodeChange(NodeChange<N>),
+pub enum Change<N: Ord> {
+    /// Add or remove node from the set of validators. Contains the full new set of validators.


Let's not shy away from the scale of the change! I suggest changing the comment as follows:
"Change the set of validators to the one in the provided map. There are no restrictions on the new set of validators. In particular, it can be disjoint with the current set of validators."

vkomenda · 2018-11-14T14:08:55Z

src/sender_queue/mod.rs

-    /// all nodes.
-    fn added_node(&self) -> Option<N>;
+    /// Returns the new set of validator that this batch is starting key generation for, including
+    /// the existing ones. These should be added to the set of all nodes.


I think it is a bit misleading to say that the new nodes are added to "all nodes". Do you rather mean that all old peer_epochs should remain monitored and new entries should be created for each new node?

BTW, we don't have an upper limit on the size of peer_epochs regarding (probably very sophisticated) spam control.

Good point… that's a more general issue that I'm still unclear about:
Do we want to have the sender queue automatically manage the set of observer nodes or should the user also have some control over that (to e.g. add an observer that isn't about to join as a validator)?

In the first case, we'd probably also want to remove peers on ChangeState::Complete, in addition to adding them on ChangeState::InProgress, so that the set of peers is always exactly the union of the current and the next set of validators.

In the second case, I'm not sure: Either we'd remove this feature completely and require the user to add and remove observers, or we'd maintain an additional list of observers in SenderQueue (those would be the ones that DHB will never know about; they only observe, without ever becoming a validator).

For now, maybe I should actually implement the first point right away? It probably wouldn't be much work and I think it's mainly the intended behavior of the current code?

Our sender queue is a reference implementation which, I think, should be as simple as possible for that reason. I tend to choose the second option. In principle, the user can fully control the peers and possibly whether or not new peers join as observers only. Removing old peers however should be totally fine in our implementation. The user can change that in their implementation.

I just realized that removing peers is actually more complicated than I thought:
If we remove a peer in epoch 42, we'll still need to deliver their messages from epochs up to 41. I think that'll require some changes to update_epoch? I'm not entirely sure. Maybe we should do that as a separate PR after all?

Of course, no rush with that. Might be doable with a list of "pending actions", e.g.,

"suspend node id after epoch 41" followed by

"remove the suspended node id as soon as all it's messages are sent".

Maybe this can be done without composing actions but I think there are still temporal dependencies that may require removing in steps.

Yes, I think that would work. I created #341 for it.

vkomenda · 2018-11-14T14:23:18Z

src/sender_queue/honey_badger.rs

@@ -10,8 +10,8 @@ where
    C: Contribution,
    N: NodeIdT + Rand,
 {
-    fn added_node(&self) -> Option<N> {
-        None
+    fn new_nodes(&self) -> Vec<N> {


Maybe added_nodes or - better matching - added_peers?

afck · 2018-11-14T15:57:52Z

Looks like I broke test_dynamic_honey_badger_first_delivery_silent… investigating… 😕

afck · 2018-11-14T16:47:12Z

OK, I found the problem. It's just the "first delivery" thing again, and the fact that I effectively increased the required threshold for key generation (which is the right thing to do, I think): In the test, nodes 0 to 5 just keep producing batches and the others never get to send a message.
I'll try to fix the test tomorrow.

Make sure every node eventually gets to handle its messages.

afck · 2018-11-15T09:36:44Z

Please take another look; the "first" delivery tests now choose a random node in every 10th step.

vkomenda · 2018-11-15T11:56:31Z

tests/network/mod.rs

+            .filter(|(_, node)| !node.queue.is_empty())
+            .map(|(id, _)| id.clone());
+        let rand_node = match *self {
+            MessageScheduler::First => rand::thread_rng().gen_weighted_bool(10),


What if the user implements first non-randomized delivery? Wouldn't DHB fail then?

If the adversary can do that, Honey Badger fails anyway.
The assumption is that messages from a correct node to a correct node always eventually arrive. The previous "first delivery" implementation didn't satisfy that.

My point is that previously First used to work. Is there a fix of the algorithm without changing the test?

Yes: What broke it was that in is_ready I'm now requiring N - f ~~signatures~~ complete Parts, which is what we should always expect to be able to reach. I could revert that, but I think it's safer this way. And I think it just exposed what was wrong with the test in the first place.

The reason why I changed it in this PR is that before, we had the criterion: f + 1 complete Parts and the candidate's Part must be complete.

Now that the candidates could potentially be all of the new nodes, that criterion would actually be too strong. So I changed it to the strongest criterion that we can always expect to be satisfied, under the assumptions the algorithms are making anyway.

In the real world the "first delivery" scenario is unrealistic, and corresponds to a network that would completely block the connection between C and D as long as there are messages sent from A to B. The requirement has not changed:

The adversary is allowed to control the network as long as every message from a correct node to a correct node eventually arrives.

This requirement was not satisfied by the "first" schedule. That's why the test was wrong, and we were just "lucky" that it passed so far.

I agree in general. Now you essentially have two randomized delivery strategies. I think we still need a deterministic one, round-robin for example. I'm not sure what priority that would have given that we move all tests to net.

I agree, in the long run it would be good to have several different "extreme" delivery strategies (while still satisfying the above requirement). We could easily make this one deterministic, too, by making it do round-robin instead of randomizing in every 10th round.
But I didn't want to spend too much time on implementing delivery strategies for the old test network.

I'm getting more convinced about this change but still think that the new validator set feature is not explored enough to be deemed safe and moreover the old test failed on it. This is too big for a single-person review. @c0gent, @mbr, please check if you are happy with @afck's "real world" argument.

Buyer beware: adversaries would stretch the real world boundaries.

Note that, as a mathematician, I don't care for "real world arguments" myself! 😎
However, in this case, it's also technically correct! The "first" delivery strategy simply didn't meet the theoretical requirement.

c0gent

This and the Hydrabadger changes look good to me. I'll address the whole node removal/reconnection aspect soon as well.

Allow arbitrary validator set changes in DHB.

cc877cb

This replaces `NodeChange` with a full list of IDs and public keys, instead of just a single to-be-added or to-be-removed node, to allow completely replacing the set of validators by any arbitrary new set in a single key generation step.

afck requested review from mbr, c0gent and vkomenda November 14, 2018 11:39

vkomenda reviewed Nov 14, 2018

View reviewed changes

Address review comments: added_nodes, comments.

4f53d08

vkomenda approved these changes Nov 14, 2018

View reviewed changes

Fix MessageScheduler::First.

d9fce46

Make sure every node eventually gets to handle its messages.

vkomenda reviewed Nov 15, 2018

View reviewed changes

afck mentioned this pull request Nov 15, 2018

An optional observer node in the net framework #340

Closed

c0gent approved these changes Nov 15, 2018

View reviewed changes

afck merged commit e89688b into master Nov 18, 2018

afck deleted the afck-change branch November 18, 2018 09:17

afck mentioned this pull request Nov 18, 2018

Migrate to new hbbft API: arbitrary validator set changes. poanetwork/hydrabadger#19

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow arbitrary validator set changes in DHB. #339

Allow arbitrary validator set changes in DHB. #339

afck commented Nov 14, 2018 •

edited

Loading

afck commented Nov 14, 2018

vkomenda left a comment

vkomenda Nov 14, 2018

vkomenda Nov 14, 2018

afck Nov 14, 2018

vkomenda Nov 14, 2018

afck Nov 14, 2018

vkomenda Nov 14, 2018 •

edited

Loading

afck Nov 14, 2018

vkomenda Nov 14, 2018

afck commented Nov 14, 2018 •

edited

Loading

afck commented Nov 14, 2018

afck commented Nov 15, 2018

vkomenda Nov 15, 2018

afck Nov 15, 2018

vkomenda Nov 15, 2018

afck Nov 15, 2018 •

edited

Loading

afck Nov 15, 2018

afck Nov 15, 2018

vkomenda Nov 15, 2018

afck Nov 15, 2018

vkomenda Nov 15, 2018

afck Nov 15, 2018

c0gent left a comment

Allow arbitrary validator set changes in DHB. #339

Allow arbitrary validator set changes in DHB. #339

Conversation

afck commented Nov 14, 2018 • edited Loading

afck commented Nov 14, 2018

vkomenda 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

vkomenda Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afck commented Nov 14, 2018 • edited Loading

afck commented Nov 14, 2018

afck commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afck Nov 15, 2018 • edited Loading

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

c0gent left a comment

Choose a reason for hiding this comment

afck commented Nov 14, 2018 •

edited

Loading

vkomenda Nov 14, 2018 •

edited

Loading

afck commented Nov 14, 2018 •

edited

Loading

afck Nov 15, 2018 •

edited

Loading