External sender queue implementation #308

vkomenda · 2018-10-29T16:48:06Z

Fixes #226

This PR implements a reference spam control mechanism - the sender queue - by requiring all peers implement post-processing of outgoing messages as follows:

Peer epochs are monitored.
An outgoing message is sent to a peer if and when the sender reckons the peer is at an epoch that matches the epoch of the message. If the peer is already ahead, the message is discarded by the sender instead of being sent.
Target::All messages may be split into Target::Node messages for each peer if peers happen to be at different epochs, some of which match the epoch of the message while others don't.

Incoming message queues have been removed. Message recipients discard obsolete messages and return a Fault for messages with epochs too far ahead.

The sender queue is agnostic to the differences between validators and observers. All it cares about is what the peer IDs are and what is the ID of the node it is running on. I believe this is approximately in line with Option 2 of @afck's comment.

There are three implementations of a sender queue, one for each of HoneyBadger, DynamicHoneyBadger and QueueingHoneyBadger, although the last one uses the DynamicHoneyBadger implementation, so those two are essentially the same.

Comments welcome: I'm not sure whether Epoched::Epoch is fine to stay Copy rather than Clone. Maybe I should have implemented an optional storage for the computed epoch and make Epoched::Epoch to be Clone instead to avoid possible recomputation of the epoch of the same message multiple times.

c0gent · 2018-10-29T17:00:26Z

Vlad, could you provide a brief (but complete) outline of the changes you've made just for a frame of reference? I haven't been following the this topic closely so it would be helpful for me but it would also make it easier for us in the future if we ever look back at this. I usually like to put a description or list of changes in the commit message but here in the PR is fine too.

vkomenda · 2018-10-29T21:30:57Z

@c0gent, I updated the description as you asked.

c0gent · 2018-10-29T23:01:58Z

Excellent description!

afck

This looks great! I think that's the right approach to the sender queue.
(Only halfway through the review so far. Will continue in the afternoon.)

I'm still a bit concerned about getting rid of the general "observer", and instead coupling that concept to the peer turning up inside an InProgress in a batch. I wonder whether there's something to be gained from keeping that concept more general, and allowing the user to just tell the sender queue if an observer has connected.
But maybe both can be combined anyway, and it's certainly something for a separate PR, if at all.

afck · 2018-10-30T10:26:13Z

examples/simulation.rs

-        let dyn_hb = DynamicHoneyBadger::builder().build(netinfo);
-        QueueingHoneyBadger::builder(dyn_hb)
+        let dhb = DynamicHoneyBadger::builder().build(netinfo.clone());
+        let netinfo = Arc::new(netinfo);


Is this Arc needed? It seems like we don't even have to clone netinfo: We could collect our_id and all_ids before building dhb.

src/dynamic_honey_badger/batch.rs

src/dynamic_honey_badger/builder.rs

src/dynamic_honey_badger/mod.rs

src/dynamic_honey_badger/sender_queue.rs

src/sender_queue/mod.rs

src/traits.rs

afck · 2018-10-30T13:58:40Z

src/traits.rs

+                        .iter()
+                        .filter(|(_, them)| isnt_earlier_epoch(them))
+                        .map(|(id, _)| id)
+                        .collect();


We could avoid creating these collections by just pushing the messages right away. Especially if remote_epochs contained all remote nodes, it would be a lot simpler:

for (id, them) in remote_epochs { if is_accepting_epoch(&message, them) { passed_msgs.push(Target::Node(id.clone()).message(message.clone())); } else if is_later_epoch(&message, them) { deferred_msgs.push((id.clone(), message.clone())); } }

(Although I might be confused by all the negations. 😵)

I still think this ⬆️ would make the code simpler and avoid allocating the two sets.

Good point about double negations. Let's keep them away if possible.

There should only be one extra negation in your else if clause.

src/sender_queue/mod.rs

afck · 2018-10-30T14:02:19Z

tests/dynamic_honey_badger.rs

-                    && !node.instance().has_input()
-                    && node.instance().netinfo().is_validator()
+                    && !node.instance().algo().has_input()
+                    && node.instance().algo().netinfo().is_validator()


Maybe we should add these as convenience methods to SenderQueue (has_input only for SenderQueue<DHB> and SenderQueue<HB>, of course).

afck · 2018-10-31T10:34:27Z

I just realized that we probably need to include max_future_epochs in the JoinPlan now (or, alternatively, include more information about which epochs we accept in the EpochStarted message). Otherwise a node with max_future_epochs == 3 will send messages to one with max_future_epochs == 2 that it won't accept yet.

With this and #288, it would make sense to add a mechanism that on network startup and for joining nodes ensures that the nodes all agree on those parameters (but probably in a separate PR).

vkomenda · 2018-10-31T11:08:26Z

I reckon including max_future_epochs in every EpochStarted message would be a bit chatty. But including it in the JoinPlan is doable. I can do it in this PR if you prefer.

afck · 2018-11-01T15:49:29Z

examples/simulation.rs

-            .expect("instantiate QueueingHoneyBadger")
+            .expect("instantiate QueueingHoneyBadger");
+        let our_id = *netinfo.our_id();
+        let peer_ids = netinfo.all_ids().filter(|&&them| them != our_id).cloned();


If we collect these before creating dhb, we don't need to clone the netinfo at all.

src/dynamic_honey_badger/sender_queueable.rs

src/sender_queue/mod.rs

src/dynamic_honey_badger/mod.rs

c0gent · 2018-11-01T16:22:18Z

Also, how is the era/epoch terminology defined now?

Unnecessary

c0gent · 2018-11-01T16:30:17Z

Hydrabadger works fine with these changes fyi. Batch::seqnum definitely needs to be renamed.

c0gent

More documentation of era, epoch terminology needed.

src/dynamic_honey_badger/batch.rs

afck · 2018-11-01T16:26:45Z

src/honey_badger/batch.rs

+
+    /// Returns the **next** `HoneyBadger` epoch after the sequential epoch of the batch.
+    fn epoch(&self) -> u64 {
+        self.epoch + 1


I find that very counterintuitive. Should there maybe be a next_epoch method in SenderQueueableOutput instead?

afck · 2018-11-01T16:28:58Z

src/sender_queue/mod.rs

+    fn is_obsolete(&self, them: <Self as Epoched>::Epoch) -> bool;
+}
+
+pub trait SenderQueueableOutput<N, M>: Epoched


If we add next_epoch, we can probably remove the Epoched trait bound here.

afck · 2018-11-01T16:38:25Z

src/traits.rs

+                    Target::Node(id) => peer_epochs
+                        .get(&id)
+                        .map_or(false, |&them| message.is_accepted(them, max_future_epochs)),
+                });


I still feel it would be easier to just iterate through all the messages in the big loop below, and populate passed_msgs there, too. That would avoid allocating and then consuming failed_msgs.
(Can also be refactored in another PR, of course.)

afck · 2018-11-01T16:40:50Z

src/traits.rs

+                        .iter()
+                        .filter(|(_, them)| isnt_earlier_epoch(them))
+                        .map(|(id, _)| id)
+                        .collect();


I still think this ⬆️ would make the code simpler and avoid allocating the two sets.

afck · 2018-11-01T16:43:29Z

I won't be able to continue the review before Monday, but if @mbr and @c0gent approve, feel free to merge.

vkomenda · 2018-11-01T18:21:34Z

If that is needed, I would rather add max_future_epochs to JoinPlan in a separate PR not to overcrowd this one.

vkomenda · 2018-11-02T10:31:07Z

Rebased on top of master.

The last commit adds an intermediate type between non-linear epochs Epoched::Epoch and linear epochs of DHB, that is u64. This intermediate type is Epoched::LinEpoch of "linearizable epochs". It can be converted to u64 but not vice versa. Also peer_epochs now use this type, which allows to avoid unreachable match cases.

src/traits.rs

…t on eras

vkomenda requested review from mbr, c0gent and afck October 29, 2018 16:48

afck reviewed Oct 30, 2018

View reviewed changes

vkomenda force-pushed the vk-extern-sender-queue branch 10 times, most recently from e021c40 to 8247d33 Compare November 1, 2018 14:33

afck reviewed Nov 1, 2018

View reviewed changes

c0gent previously requested changes Nov 1, 2018

View reviewed changes

src/dynamic_honey_badger/mod.rs Show resolved Hide resolved

c0gent suggested changes Nov 1, 2018

View reviewed changes

src/dynamic_honey_badger/batch.rs Outdated Show resolved Hide resolved

afck reviewed Nov 1, 2018

View reviewed changes

vkomenda force-pushed the vk-extern-sender-queue branch from b1346be to 11c185b Compare November 1, 2018 18:41

vkomenda force-pushed the vk-extern-sender-queue branch from 59af49b to 4563df3 Compare November 2, 2018 10:09

afck approved these changes Nov 5, 2018

View reviewed changes

src/traits.rs Outdated Show resolved Hide resolved

vkomenda added 4 commits November 5, 2018 11:11

sender queue implementation

ee46dd4

work on review comments

a8586ef

refactored an error! by adding linearizable epochs and wrote a commen…

3deb5f1

…t on eras

simplified Step::defer_messages

52cbde4

vkomenda force-pushed the vk-extern-sender-queue branch from 4563df3 to 52cbde4 Compare November 5, 2018 13:38

Updated documentation and removed a debug print

f02bbbb

afck approved these changes Nov 5, 2018

View reviewed changes

c0gent approved these changes Nov 5, 2018

View reviewed changes

vkomenda merged commit 62cd29e into master Nov 5, 2018

vkomenda deleted the vk-extern-sender-queue branch November 5, 2018 19:05

afck mentioned this pull request Nov 6, 2018

Limit message caches and future epochs? #43

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External sender queue implementation #308

External sender queue implementation #308

vkomenda commented Oct 29, 2018 •

edited

Loading

c0gent commented Oct 29, 2018

vkomenda commented Oct 29, 2018

c0gent commented Oct 29, 2018

afck left a comment

afck Oct 30, 2018

afck Oct 30, 2018

afck Nov 1, 2018

vkomenda Nov 1, 2018

vkomenda Nov 1, 2018

afck Oct 30, 2018

afck commented Oct 31, 2018

vkomenda commented Oct 31, 2018

afck Nov 1, 2018

c0gent commented Nov 1, 2018

c0gent commented Nov 1, 2018

c0gent left a comment

afck Nov 1, 2018

afck Nov 1, 2018

afck Nov 1, 2018

afck Nov 1, 2018

afck commented Nov 1, 2018

vkomenda commented Nov 1, 2018

vkomenda commented Nov 2, 2018

External sender queue implementation #308

External sender queue implementation #308

Conversation

vkomenda commented Oct 29, 2018 • edited Loading

c0gent commented Oct 29, 2018

vkomenda commented Oct 29, 2018

c0gent commented Oct 29, 2018

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

afck commented Oct 31, 2018

vkomenda commented Oct 31, 2018

Choose a reason for hiding this comment

c0gent commented Nov 1, 2018

c0gent commented Nov 1, 2018

c0gent 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

afck commented Nov 1, 2018

vkomenda commented Nov 1, 2018

vkomenda commented Nov 2, 2018

vkomenda commented Oct 29, 2018 •

edited

Loading