An optional observer node in the net framework

[vkomenda]

An observer node in VirtualNet serves the purpose of porting old tests, cf. #322.

Closes #337.

[afck]

Looks good to me, but let's wait for @mbr's approval.

[mbr]

I am not happy with this yet, for several reasons:

1. We are still missing a good definition of observer, here's the latest from @afck

   A node without a threshold key share, that doesn't send messages as part of the algorithms (except DHB while joining) and only receives Target::All messages.

   One huge issue here naturally is "except DHB", since it is very counter-intuitive to have observer nodes send anything at all. We may need a better definition, or just reduce it to something like

   A node without a threshold key share, that does not count toward the network size N or faulty node count f.

   It may be advantageous to separate the concept of observers from joining nodes in DHB entirely though.

2. Making observers an overlay by tracking them, but otherwise treating them as normal nodes (they themselves do not know they are observers with the implementation in this PR) might cause issues down the road. I believe it's much better to add an is_observer to Node, update the relevant iterators and message dispatching logic.

3. With that said, why only have one observer? We could have multiple, sure it should not make a difference for most algorithms, but testing that it actually does not do so is very much in the scope of things to test for?

   Additionally, if we are to implement a new, more generic DHB that allows swapping out a validator set completely instead of just adding or dropping one node, this functionality might be worthwhile anyway. While we do not need to make affordances for these hypotheticals right now, I think the multiple-observers implementation is more natural and not more complicated than a single one anyway!

4. It might be a good idea to allow enabling selective, automatic filtering of messages from observers (by panic'ing if they ever send some). In this model, we are shifting the burden of making sure observers do not interfere to the outer layer, which is already in charge of making sure messages coming in are authenticated.

   This functionality must come with an off-switch for DHB. Alternative, we could approach this from the other way around and ensure algorithms handle these issues by introducing adversaries that inject message from observers.

5. Documentation must be updated, especially regarding network sizes. We are not counting observer nodes in the network sizes usually, as they are not relevant and should be ignored from a security perspective (and therefore should also not be able to affect the network, weirdness of DHB aside).

6. The interface then probably needs a separate construction function for observer nodes and a way to specify how many should be constructed.

These are just some points to take into account. All-in-all, I think this is a bit of a bigger issue, which is why I put it off.

[vkomenda]

@mbr, responding to your

they themselves do not know they are observers with the implementation in this PR

That's not exactly right. Observers know that they are not validators because their IDs are not in the public key set.

[mbr]

That's not exactly right. Observers know that they are not validators because their IDs are not in the public key set.

Hmm, true. A Node itself doesn't know it, but VirtualNet knows and DistAlgorithm knows, but there is no is_observer method as far as I can tell. I still think it might be better fit to put this info into Node though, even if not for this particular reason.

[afck]

I agree that we need to properly document the observer concept (if we keep it).

Regarding the "weirdness" of DHB: I don't think it's that weird. It's just that we're in a situation with an old and a new set of validators, where the new set of validators needs to perform DKG before it can fully take over. Since we do DKG "on-chain", using the HB that is still run by the old set of validators, the two sets temporarily overlap: During DKG, the new nodes need to use the old nodes' network, kind of as "clients", because they need to use an existing consensus process for their DKG. They send their DKG messages as their input to the old network, and they observe it so they can see the output.

Anyway, maybe the definition should just be that then: doesn't have a key share but receives Target::All messages.

these hypotheticals

Uh… these hypotheticals will be merged soon: #339. 😬

[vkomenda]

@mbr, to sum up your comments, would this list of changes be sufficient?

• Allow for multiple observers (while still keeping them in nodes as at present).

• Add a getter for the subset of nodes which are observers (in the same way as the getters fo correct and faulty nodes; that would cover your is_observer requirement).

• Add a separate constructor in Node for an observer node.

• Update the test documentation with the definition of observer.