poanetwork / hbbft

An implementation of the paper "Honey Badger of BFT Protocols" in Rust. This is a modular library of consensus.
Other
357 stars 96 forks source link

External sender queue implementation #308

Closed vkomenda closed 6 years ago

vkomenda commented 6 years ago

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:

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 commented 6 years ago

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 commented 6 years ago

@c0gent, I updated the description as you asked.

c0gent commented 6 years ago

Excellent description!

afck commented 6 years ago

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 commented 6 years ago

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.

c0gent commented 6 years ago

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

c0gent commented 6 years ago

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

afck commented 6 years ago

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

vkomenda commented 6 years ago

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

vkomenda commented 6 years ago

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.