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

Fraser999 · 2016-11-09T11:02:40Z

No description provided.

…ning node

…ew_acc

… sorted This resolves an issue where some lists of prefixes related to merging groups were not sorted, causing the merge test to fail sporadically.

maidsafe-highfive · 2016-11-09T11:02:48Z

r? @afck

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

afck · 2016-11-09T11:10:07Z

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) {


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.

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?

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.)

Right - of course!

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

afck · 2016-11-09T11:12:15Z

src/states/node.rs

-                                   &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) {


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

Well spotted.

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

fizyk20 · 2016-11-09T12:02:12Z

src/messages.rs

    /// 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()`.


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.

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

I see - makes sense.

fizyk20 · 2016-11-09T12:05:33Z

src/messages.rs

               self.content,
-               self.public_id)
+               self.signatures.len())


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

Good point - I'll add that.

fizyk20 · 2016-11-09T12:13:25Z

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.


The commend could probably use updating ;)

dhardy · 2016-11-10T14:14:28Z

src/authority.rs

+            Authority::Client { .. } => true,
+        }
+    }
+


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?

Good point - I'll change to if let.

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 ;)

This fixes an issue and adds a regression test for a bug in the 'impl_xorable_for_array' macro.

…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.

afck · 2016-11-10T16:42:35Z

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.


Typo, should be quorum. (My bad.)

afck · 2016-11-10T17:14:05Z

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.


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

Yup - thanks.

afck · 2016-11-10T17:21:53Z

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) => {


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

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

afck · 2016-11-10T17:23:41Z

src/states/node.rs

-                                   &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) {


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

afck · 2016-11-10T17:25:40Z

src/states/node.rs

+        // 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).


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.

This causes 'core_tests::more_than_group_size_nodes()' to be ignored. Also addresses further review comments.

Andreas Fackler and others added 19 commits November 5, 2016 11:32

fix/clippy: fix new clippy warning

370e682

chore/build: update dependencies

6cf9312

fix/utils: update calculate_relocated_name and re-enable and fix test

a118ccc

feat/node: accumulate messages in the first hop

78d9e70

fix/routing_table: workaround for routing connection info via the joi…

bb98db6

…ning node

fix/routing_table: use the joining node's name for excluding groups

8ec841d

style/all: address review comments

ae40d6f

Merge branch 'master' into MAID-1698

2bbb057

fix/messages: address review comments

959e9bd

Merge branch 'new_acc' of https://github.com/afck/routing into new_acc

f1013b6

Merge branch 'MAID-1698' of https://github.com/fizyk20/routing into n…

d21568a

…ew_acc

fix/messages: address review comments

20b74da

fix/routing_table: update group merge prefixes to remain consistently…

03a93c1

… sorted This resolves an issue where some lists of prefixes related to merging groups were not sorted, causing the merge test to fail sporadically.

Merge branch 'master' into use_sorted

db15401

Merge branch 'master' into use_sorted

6cbe6eb

Merge remote-tracking branch 'origin/use_sorted' into new_acc

e8f78eb

chore/ci: increase Travis CI timeouts

2f635d3

Merge branch 'use_sorted' into new_acc

2903b66

Merge branch 'master' into new_acc

a1ec83b

maidsafe-highfive assigned afck Nov 9, 2016

afck mentioned this pull request Nov 9, 2016

MAID-1675 Accumulate messages in the first hop #1200

Closed

afck reviewed Nov 9, 2016

View reviewed changes

fizyk20 reviewed Nov 9, 2016

View reviewed changes

fix/node: address further review comments

5535faa

dhardy reviewed Nov 10, 2016

View reviewed changes

Fraser Hutchison added 4 commits November 10, 2016 15:06

fix/xorable: fix bug in 'Xorable::common_prefix()' for arrays

243e0ee

This fixes an issue and adds a regression test for a bug in the 'impl_xorable_for_array' macro.

Merge branch 'master' into new_acc

449584d

style/clippy: fix clippy warning

aa9ac23

Fraser999 force-pushed the new_acc branch from 7a21169 to aa9ac23 Compare November 10, 2016 15:38

afck reviewed Nov 10, 2016

View reviewed changes

fix/core_test: ignore an intermittently-failing test for now

32d1941

This causes 'core_tests::more_than_group_size_nodes()' to be ignored. Also addresses further review comments.

afck approved these changes Nov 11, 2016

View reviewed changes

afck merged commit 7d2f10e into maidsafe:master Nov 11, 2016

Fraser999 deleted the new_acc branch November 11, 2016 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Fraser999 commented Nov 9, 2016

maidsafe-highfive commented Nov 9, 2016

afck Nov 9, 2016

Fraser999 Nov 9, 2016

afck Nov 9, 2016

Fraser999 Nov 9, 2016

afck Nov 9, 2016

Fraser999 Nov 9, 2016

afck Nov 10, 2016

fizyk20 Nov 9, 2016

Fraser999 Nov 9, 2016

fizyk20 Nov 9, 2016

fizyk20 Nov 9, 2016

Fraser999 Nov 9, 2016

fizyk20 Nov 9, 2016

Fraser999 Nov 9, 2016

dhardy Nov 10, 2016 •

edited

Fraser999 Nov 10, 2016

fizyk20 Nov 10, 2016

afck Nov 10, 2016

afck Nov 10, 2016

Fraser999 Nov 11, 2016

afck Nov 10, 2016

Fraser999 Nov 11, 2016

afck Nov 10, 2016

afck Nov 10, 2016

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

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

Conversation

Fraser999 commented Nov 9, 2016

maidsafe-highfive commented Nov 9, 2016

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

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

dhardy Nov 10, 2016 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy Nov 10, 2016 •

edited