MAID-1675: accumulate messages in the first hop #1213

Merged
merged 25 commits into from Nov 11, 2016

Projects

None yet

5 participants

@Fraser999
Member

No description provided.

afck and others added some commits Nov 5, 2016
@afck afck fix/clippy: fix new clippy warning 370e682
@afck afck chore/build: update dependencies 6cf9312
@Fraser999 @afck Fraser999 fix/utils: update calculate_relocated_name and re-enable and fix test a118ccc
@afck afck feat/node: accumulate messages in the first hop
78d9e70
@fizyk20 fizyk20 fix/routing_table: workaround for routing connection info via the joi…
…ning node
bb98db6
@fizyk20 fizyk20 fix/routing_table: use the joining node's name for excluding groups
8ec841d
@fizyk20 fizyk20 style/all: address review comments
ae40d6f
@Fraser999 Fraser999 Merge branch 'master' into MAID-1698
2bbb057
@afck afck fix/messages: address review comments
959e9bd
@Fraser999 Fraser999 Merge branch 'new_acc' of https://github.com/afck/routing into new_acc f1013b6
@Fraser999 Fraser999 Merge branch 'MAID-1698' of https://github.com/fizyk20/routing into n…
…ew_acc
d21568a
@Fraser999 Fraser999 fix/messages: address review comments 20b74da
@Fraser999 Fraser999 fix/routing_table: update group merge prefixes to remain consistently…
… sorted

This resolves an issue where some lists of prefixes related to merging groups were not sorted,
causing the merge test to fail sporadically.
03a93c1
@Fraser999 Fraser999 Merge branch 'master' into use_sorted
db15401
@afck afck Merge branch 'master' into use_sorted
6cbe6eb
@Fraser999 Fraser999 Merge remote-tracking branch 'origin/use_sorted' into new_acc e8f78eb
@Fraser999 Fraser999 chore/ci: increase Travis CI timeouts
2f635d3
@Fraser999 Fraser999 Merge branch 'use_sorted' into new_acc
2903b66
@Fraser999 Fraser999 Merge branch 'master' into new_acc
a1ec83b
@maidsafe-highfive

r? @afck

(maidsafe_highfive has picked a reviewer for you, use r? to override)

src/messages.rs
+ }
+
+ /// Appends the group list to the message, and removes all invalid signatures.
+ pub fn add_group_list(&mut self, group_list: GroupList) {
@afck
afck Nov 9, 2016 Member

Either add_group_list and add_signature should both check for whether the public key belongs to the group, or none of them should.
I'd vote for both.

@Fraser999
Fraser999 Nov 9, 2016 Member

Ah - I missed that add_group_list() was doing this.

The problem with checking is that we can receive the signatures and the actual SignedMessage in random order, so when we receive a signature we maybe haven't added the group list yet, or vice-versa.

I suppose we could change the behaviour to fix the first case (e.g. have ability to call add_group_list() multiple times for same group and only actually do the addition once - then we could call this when handling each incoming signature, or just pass the group list as an arg of add_signature/s()), but I don't see a simple way round the "vice-versa" scenario.

I guess I'd be inclined to try and fix this in remove_invalid_signatures() since when that's called we've definitely added the group and enough signatures. So I'd remove the check from here and instead add it to the existing check in remove_invalid_signatures(). Does that sound OK?

@afck
afck Nov 9, 2016 Member

Yes, that's fine.
(But checking it in add_signature if there is a first group list would also work. Receiving the signatures before the message itself should already be handled correctly by the signature accumulator.)

@Fraser999
Fraser999 Nov 9, 2016 Member

Right - of course!

I'll check it in add_signature() then, since it dumps unneeded signatures earlier.

- &hop_name,
- hop_msg.sent_to())
+ let HopMessage { mut content, route, sent_to, .. } = hop_msg;
+ let hop_pub_ids = if let Some(group) = self.peer_mgr.routing_table().get_group(&hop_name) {
@afck
afck Nov 9, 2016 Member

We need to add our own name in there in case it's our own group.

@Fraser999
Fraser999 Nov 9, 2016 Member

Well spotted.

@afck
afck Nov 10, 2016 Member

Thanks - although I'm the one who forgot this in the first place. 😁

/// The priority Crust should send this message with.
pub fn priority(&self) -> u8 {
self.content.priority()
}
+
+ /// Returns whether there are enough signatures from the sender. NOTE: to ensure only validated
+ /// signatures are counted, first call `remove_invalid_signatures()`.
@fizyk20
fizyk20 Nov 9, 2016 Contributor

Is there any use for counting signatures including invalid ones? Maybe it would be a good idea to just call remove_invalid_signatures in this function and not require callers to remember about it.

@Fraser999
Fraser999 Nov 9, 2016 Member

The concern is the overhead of serialising the message each time remove_invalid_signatures() is called.

@fizyk20
fizyk20 Nov 9, 2016 Contributor

I see - makes sense.

src/messages.rs
self.content,
- self.public_id)
+ self.signatures.len())
@fizyk20
fizyk20 Nov 9, 2016 Contributor

Maybe consider adding the list of names that signed the message, could be useful in debugging.

@Fraser999
Fraser999 Nov 9, 2016 Member

Good point - I'll add that.

src/states/node.rs
- new_sent_to.clone(),
- target_peer_id));
+ let bytes = if send_sig {
+ // Not our turn to send the full group message. Only send a hash.
@fizyk20
fizyk20 Nov 9, 2016 Contributor

The commend could probably use updating ;)

@Fraser999
Fraser999 Nov 9, 2016 Member

Will do.

src/authority.rs
+ Authority::Client { .. } => true,
+ }
+ }
+
@dhardy
dhardy Nov 10, 2016 edited Contributor

You can shortcut this (silly pattern, but still shorter):

if let *self == Authority::Client { .. } { true } else { false }

You could of course do the same with match, unless an explicit match against other cases is required?

@Fraser999
Fraser999 Nov 10, 2016 Member

Good point - I'll change to if let.

@fizyk20
fizyk20 Nov 10, 2016 Contributor

I personally find match more readable (and easier to modify in the future if we add some other client type), but I won't argue ;)

Fraser999 added some commits Nov 9, 2016
@Fraser999 Fraser999 fix/xorable: fix bug in 'Xorable::common_prefix()' for arrays
This fixes an issue and adds a regression test for a bug in the 'impl_xorable_for_array' macro.
243e0ee
@Fraser999 Fraser999 fix/signature_accumulator: fix a bug in 'SignatureAccumulator::add_me…
…ssage()'

This fixes a bug which prevented any message from accumulating in the 'SignatureAccumulator'.  It
also adds a couple of basic unit tests for this struct, improves the debug output of 'RoutingTable'
and implements 'Drop' for it so that all RTs will be logged in the case of a test panicking.
0d311f5
@Fraser999 Fraser999 Merge branch 'master' into new_acc 449584d
@Fraser999 Fraser999 style/clippy: fix clippy warning
aa9ac23
src/messages.rs
use std::time::Duration;
use types::MessageId;
use utils;
use xor_name::XorName;
+/// The quroum, as a percentage of the group size.
@afck
afck Nov 10, 2016 Member

Typo, should be quorum. (My bad.)

src/messages.rs
- src: Authority::ClientManager(name),
- dst: Authority::ClientManager(name),
- content: MessageContent::GetCloseGroup(MessageId::zero()),
+ // Add a signature which will not correspond to an ID in the first group list.
@afck
afck Nov 10, 2016 Member

Maybe that comment should be a few lines further down, after creating the message.

@Fraser999
Fraser999 Nov 11, 2016 Member

Yup - thanks.

src/signature_accumulator.rs
+ .zip(env.other_ids.iter())
+ .foreach(|(signature_msg, full_id)| {
+ let result = match *signature_msg {
+ DirectMessage::MessageSignature(ref hash, ref sig) => {
@afck
afck Nov 10, 2016 Member

Those refs are unnecessary, as the values are copied below anyway. (Or is this intentional, to make the copying more explicit?)

@Fraser999
Fraser999 Nov 11, 2016 Member

No - they're just an oversight - I'll remove them.

- &hop_name,
- hop_msg.sent_to())
+ let HopMessage { mut content, route, sent_to, .. } = hop_msg;
+ let hop_pub_ids = if let Some(group) = self.peer_mgr.routing_table().get_group(&hop_name) {
@afck
afck Nov 10, 2016 Member

Thanks - although I'm the one who forgot this in the first place. 😁

+ // TODO - If the hop group is our own group, we should add our own ID in some cases here.
+ // However, we can't always do this (e.g. the first node of a new network won't
+ // handle any of the second node's requests since adding its own ID will stop it
+ // from ever accumulating).
@afck
afck Nov 10, 2016 Member

But the second node should only send the request as a ManagedNode, and in that case we need to consider the message accumulated as soon as it has just one signature.

@Fraser999 Fraser999 fix/core_test: ignore an intermittently-failing test for now
This causes 'core_tests::more_than_group_size_nodes()' to be ignored.  Also addresses further
review comments.
32d1941
@afck
afck approved these changes Nov 11, 2016 View changes
@afck afck merged commit 7d2f10e into maidsafe:master Nov 11, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Fraser999 Fraser999 deleted the Fraser999:new_acc branch Nov 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment