MAID-1839 feature/routing: resource_proof #1261

maqi · 2016-12-06T09:24:17Z

No description provided.

maidsafe-highfive · 2016-12-06T09:24:27Z

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

dhardy · 2016-12-09T16:49:40Z

src/peer_manager.rs

+        (&mut self,
+         expected_name: &XorName,
+         our_public_id: &PublicId)
+         -> Result<(bool, (Prefix<XorName>, Vec<PublicId>)), RoutingTableError> {


Better to use a named struct instead of a tuple of a tuple? Or at least update the function's documentation to explain what gets returned.

updated function documentation to explain what gets returned.

dhardy · 2016-12-09T16:54:34Z

src/peer_manager.rs

+
+        let (prefix, names) = self.routing_table.expect_join_our_group(expected_name)?;
+
+        if names.len() > 8 {


Don't hard-code 8 here. I think you want routing_table.min_group_size().

dhardy · 2016-12-09T16:56:49Z

src/peer_manager.rs

@@ -258,6 +269,8 @@ pub struct PeerManager {
    proxy_peer_id: Option<PeerId>,
    routing_table: RoutingTable<XorName>,
    our_public_id: PublicId,
+    node_candidate: Option<(XorName, Instant, Vec<u8>)>,


Please document these fields (within the struct with comments)

dhardy · 2016-12-09T17:02:20Z

src/peer_manager.rs

+        if let Some((name, now, _)) = self.node_candidate {
+            if name == *expected_name ||
+               now.elapsed() < Duration::from_secs(RESOURCE_PROOF_APPROVE_TIMEOUT_SECS) {
+                return Err(RoutingTableError::AlreadyExists);


I wonder if AlreadyExists is the right error (especially considering there are two different causes), but it will do for now (better in my opinion to either return or log a more specific message).

dhardy · 2016-12-09T17:03:27Z

src/peer_manager.rs

+        let (prefix, names) = self.routing_table.expect_join_our_group(expected_name)?;
+
+        if names.len() > 8 {
+            self.node_candidate = None;


Useless line since it's changed again just below?

dhardy · 2016-12-09T17:04:31Z

src/peer_manager.rs

+            let seed = rng.gen_iter().take(10).collect();
+            self.node_candidate = Some((*expected_name, Instant::now(), seed));
+        } else {
+            return Ok((false, (prefix, vec![])));


What are we returning? Would love a comment, unless you switch to a struct with named method describing what this is.

updated function documentation to explain what gets returned.

dhardy · 2016-12-12T09:52:51Z

src/peer_manager.rs

+        None
+    }
+
+    pub fn handle_node_approval_vote(&mut self,


Also document the return value please

dhardy

I didn't review the complete algorithm. Code doesn't look bad other than a couple of names and missing doc. Could you also update the message flow documentation (messages.rs:438 onwards)?

dhardy · 2016-12-12T10:17:24Z

src/peer_manager.rs

+        (false, None)
+    }
+
+    pub fn handle_node_approval(&mut self) -> Vec<(PublicId, PeerId)> {


Could you think of a better name (collect_peer_ids?), and add a line of documentation please?

dhardy · 2016-12-12T10:25:14Z

src/peer_manager.rs

+        result
+    }
+
+    pub fn is_node_candidate(&mut self,


I would assume a method named is_... would just check whether some property is true, but this also updates the peer state. Could you at least add documentation of this, if not also find a better name?

dhardy · 2016-12-12T10:26:20Z

src/peer_manager.rs

+        Ok(None)
+    }
+
+    pub fn is_peer_candidate(&mut self, pub_id: &PublicId, peer_id: &PeerId) -> bool {


Same as above

dhardy · 2016-12-12T10:31:21Z

src/peer_manager.rs

+            Some(PeerState::AwaitingNodeIdentify(true)) => true,
+            Some(PeerState::Routing(_tunnel)) => {
+                error!("PeerCandidate {:?} already in state Routing.", peer_id);
+                return true;


If the function returns here the peer gets removed from the peer_map without replacement. I don't think this was intended?

dhardy · 2016-12-12T10:31:35Z

src/peer_manager.rs

+                    Some(PeerState::AwaitingNodeIdentify(true)) => true,
+                    Some(PeerState::Routing(_tunnel)) => {
+                        error!("NodeCandidate {:?} already in state Routing.", peer_id);
+                        return Err(RoutingTableError::AlreadyExists);


If the function returns here the peer gets removed from the peer_map without replacement. I don't think this was intended?

dhardy · 2016-12-12T10:43:14Z

src/routing_table/mod.rs

    /// Returns the collection of groups to which a peer joining our group should connect.
    ///
    /// This will generally be all the groups in our routing table.  However, if the addition of the
    /// new node will cause our group to split, only the groups which will remain neighbours after
    /// the split are returned.  Returns `Err(Error::PeerNameUnsuitable)` if `name` is not within
    /// our group, or `Err(Error::AlreadyExists)` if `name` is already in our table.
-    pub fn expect_add_to_our_group(&self, name: &T) -> Result<Groups<T>, Error> {
+    pub fn get_groups(&self, name: &T) -> Result<Groups<T>, Error> {


Could we call this get_sections_to_join? The problem is that it calculates what to do if the join would cause a split... I'd love to remove this logic later but for now lets just acknowledge it in the name?

dhardy · 2016-12-12T10:43:44Z

src/peer_manager.rs

+    }
+
+    /// Wraps the routing table function of the same name and maps `XorName`s to `PublicId`s.
+    pub fn get_groups(&self,


If RoutingTable::get_groups is renamed this one might need to be too

dhardy · 2016-12-12T10:52:05Z

src/states/node.rs

+            if tunnel {
+                /// if connection is in tunnel, vote NO directly, don't carry out profiling
+                /// limitation: joining node ONLY carries out QUORAM valid connection/evaluations
+                info!("{:?} Sending CandidateApproval false to group rejeting {:?}.",


/rejeting/rejecting/

dhardy · 2016-12-12T12:07:42Z

src/messages.rs

+/// answer that request (resolving a hashing challenge) with `ResourceProofResponse`. Member of Y
+/// will sends out `CandidateApproval` to vote for the approval in group. Once approved, member of Y
+/// add A into its routing_table and send `NodeApproval` to A. On receiving first `NodeApproval`, A
+/// adds members of Y into its routing_table.


When nodes Z of section Y receive NodeIdentify from A, they respond with a ResourceProof request. Node A needs to answer these requests (resolving a hashing challenge) with ResourceProofResponse. Members of Y will send out CandidateApproval messages to vote for the approval in their section. Once the vote succeeds, the members of Y add A to their routing table and send NodeApproval to A. When A receives the first NodeApproval message, it adds the members of Y to its routing table.

Don't mind the corrections; your English is pretty good, but I still think this is a little easier to understand ;). Copy the source version to get the back-ticks.

…ter header

fizyk20 · 2016-12-13T16:16:50Z

src/states/node.rs

+                                              groups: Vec<(Prefix<XorName>, Vec<PublicId>)>,
+                                              dst: Authority<XorName>) {
+        if !self.peer_mgr.routing_table().is_empty() {
+            warn!("{:?} Received duplicate GetNodeName response.", self);


Is GetNodeName right here?

a node may receive CandidateApproval before connection established PeerSate::Candidate needs to be considered in peer_manager::connection_info_received

resource_proof; resolve clippy failures

…h NodeApproval

…en adding prefixes

afck · 2017-01-17T08:47:57Z

src/states/node.rs

+
+        self.get_approval_timer_token = None;
+
+        self.peer_mgr.add_prefixes(sections.keys().into_iter().map(|&prefix| prefix).collect_vec());


You can use cloned() instead of map(…).

Also, since this modifies the RT's Prefixes, we should add the following:

trace!("{:?} Node approved. Prefixes: {:?}", self, self.peer_mgr.routing_table().prefixes());

afck · 2017-01-17T08:48:15Z

src/states/node.rs

+        if validity {
+            let sections = self.peer_mgr.pub_ids_by_section();
+            info!("{:?} Sending NodeApproval to {:?}.", self, candidate_name);
+            let _ = self.send_routing_message(RoutingMessage {


Let's not ignore the result here.

…e of it after plit

# Conflicts: # src/peer_manager.rs # src/routing_table/mod.rs # src/states/node.rs

Merge master

…s out GetNodeNameResponse till receives AcceptAsCandidate

refactor/all: adds 'CandidateIdentify' message and handling

…NodeApproval

…ng candidate approval

use accumulation timeout to calculate timeout for sending candidate approval

…nto routing table when send out NodeApproval

refactor/all: update resource proof functions

fix/peer_manager: fix incorrect filter closures

afck · 2017-01-20T16:38:06Z

Included in #1319

feature/routing: resource_proof POC

96ecc31

maidsafe-highfive assigned afck Dec 6, 2016

maqi added 3 commits December 7, 2016 16:08

chore/resource_proff: working with current network tests

8dcecb8

Merge branch 'master' of github.com:maidsafe/routing into resource_proof

0d3b83c

fix/resource_proof: use DirectMessage

2afbc54

maqi force-pushed the resource_proof branch from c9cc4a6 to 2afbc54 Compare December 8, 2016 12:06

maqi added 2 commits December 8, 2016 14:33

fix/resource_proof: resolving hanging tests

66da308

fix/resource_proof: routing_table consistency

274b334

maqi force-pushed the resource_proof branch from 2f63b88 to 274b334 Compare December 9, 2016 05:47

maqi added 4 commits December 9, 2016 07:50

Merge branch 'master' of github.com:maidsafe/routing into resource_proof

dfb2164

chore/rustfmt

bbd787f

Merge branch 'master' of github.com:maidsafe/routing into resource_proof

d685f61

Merge branch 'master' of github.com:maidsafe/routing into resource_proof

68bffdf

dhardy reviewed Dec 12, 2016

View reviewed changes

src/peer_manager.rs

None

}

pub fn handle_node_approval_vote(&mut self,

Copy link

Contributor

dhardy Dec 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also document the return value please

Merge branch 'master' of github.com:maidsafe/routing into resource_proof

c5aff33

dhardy suggested changes Dec 12, 2016

View reviewed changes

chore/resource_proof: addessing review comments

a8e7099

dhardy reviewed Dec 12, 2016

View reviewed changes

maqi added 2 commits December 13, 2016 14:46

fix/resource_proof: resolving failing tests due to merging latest mas…

a599a3e

…ter header

Merge branch 'master' of github.com:maidsafe/routing into resource_proof

5980981

fizyk20 reviewed Dec 13, 2016

View reviewed changes

fix/resource_proof:

c150cdd

a node may receive CandidateApproval before connection established PeerSate::Candidate needs to be considered in peer_manager::connection_info_received

maqi force-pushed the resource_proof branch from 7255b01 to c150cdd Compare December 14, 2016 16:59

maqi added 2 commits December 15, 2016 06:54

Merge branch 'master' of github.com:maidsafe/routing into resource_proof

8a06fd0

Merge branch 'master' of github.com:maidsafe/routing into resource_proof

a94fd6a

maqi force-pushed the resource_proof branch 2 times, most recently from 05d0a20 to 58ae4af Compare December 15, 2016 14:08

fix/resource_proof: allowing only one joining node even not for

b208cd2

resource_proof; resolve clippy failures

maqi added 4 commits January 16, 2017 22:21

fix/node: add neighbouring member as expect_peer when received throug…

6dc6ac8

…h NodeApproval

chore/resource_proof: using BTreeSet instead of Vec

8239140

fix/routing_table: move members of our_section into proper section wh…

2eadbc9

…en adding prefixes

fix/node: restart node when timed out in waiting for approval

b8001d8

maqi force-pushed the resource_proof branch from 13021a5 to b8001d8 Compare January 17, 2017 02:36

afck reviewed Jan 17, 2017

View reviewed changes

maqi and others added 20 commits January 17, 2017 18:16

chore/resource_proof: address review comments

61d23d7

Merge branch 'master' of github.com:maidsafe/routing into resource_proof

f30e8f2

fix/peer_manager: clearup un-approved candidate if no longer in charg…

b59917a

…e of it after plit

fix/node: missing log in handle_node_approval when adding prefixes

a7731b6

chore/peer_manager: change candidate field to candidates

29fab3c

Merge remote-tracking branch 'upstream/master' into maqi-resource_proof

f9b8c68

# Conflicts: # src/peer_manager.rs # src/routing_table/mod.rs # src/states/node.rs

Merge pull request #5 from Fraser999/resource_proof

53bdab3

Merge master

chore/resource_proof: rename ExpectCloseNode to ExpectCandidate; send…

bad82c8

…s out GetNodeNameResponse till receives AcceptAsCandidate

refactor/all: adds 'CandidateIdentify' message and handling

f82f5ff

Merge remote-tracking branch 'qi/resource_proof' into resource_proof

9ab7ab3

Merge pull request #6 from Fraser999/resource_proof

d3d952c

refactor/all: adds 'CandidateIdentify' message and handling

chore/resource_proof: sending RT in CandidateApproval and use it for …

9a38673

…NodeApproval

refactor/all: use accumulation timeout to calculate timeout for sendi…

1f374fe

…ng candidate approval

Merge remote-tracking branch 'qi/resource_proof' into resource_proof

18518e1

Merge pull request #7 from Fraser999/resource_proof

2e73b58

use accumulation timeout to calculate timeout for sending candidate approval

chore/resource_proof: remove ApprovalConfirmation and add candidate i…

3951cc0

…nto routing table when send out NodeApproval

refactor/all: update resource proof functions

06a83b6

Merge pull request #8 from Fraser999/resource_proof

9f89947

refactor/all: update resource proof functions

fix/peer_manager: fix incorrect filter closures

da24d27

Merge pull request #9 from Fraser999/resource_proof

91d7072

fix/peer_manager: fix incorrect filter closures

maqi mentioned this pull request Jan 20, 2017

Resource proof + RT recovery #1319

Merged

afck closed this Jan 20, 2017

maqi deleted the resource_proof branch March 11, 2017 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAID-1839 feature/routing: resource_proof #1261

MAID-1839 feature/routing: resource_proof #1261

maqi commented Dec 6, 2016

maidsafe-highfive commented Dec 6, 2016

dhardy Dec 9, 2016

maqi Dec 12, 2016

dhardy Dec 9, 2016

dhardy Dec 9, 2016

dhardy Dec 9, 2016

dhardy Dec 9, 2016

dhardy Dec 9, 2016

maqi Dec 12, 2016

dhardy Dec 12, 2016

dhardy left a comment

dhardy Dec 12, 2016

dhardy Dec 12, 2016

dhardy Dec 12, 2016

dhardy Dec 12, 2016

dhardy Dec 12, 2016

dhardy Dec 12, 2016

dhardy Dec 12, 2016

dhardy Dec 12, 2016

dhardy Dec 12, 2016 •

edited

Loading

fizyk20 Dec 13, 2016

afck Jan 17, 2017

Fraser999 Jan 17, 2017

afck Jan 17, 2017

afck commented Jan 20, 2017


		let (prefix, names) = self.routing_table.expect_join_our_group(expected_name)?;

		if names.len() > 8 {


		self.get_approval_timer_token = None;

		self.peer_mgr.add_prefixes(sections.keys().into_iter().map(\|&prefix\| prefix).collect_vec());

MAID-1839 feature/routing: resource_proof #1261

MAID-1839 feature/routing: resource_proof #1261

Conversation

maqi commented Dec 6, 2016

maidsafe-highfive commented Dec 6, 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

dhardy 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy Dec 12, 2016 • edited Loading

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 Jan 20, 2017

dhardy Dec 12, 2016 •

edited

Loading