Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New bootstrap handshake format #911

Merged
merged 5 commits into from Dec 19, 2019

Conversation

@octol
Copy link
Contributor

octol commented Dec 11, 2019

No description provided.

@octol octol force-pushed the octol:new-bootstrap-handshake-format branch 2 times, most recently from f050664 to a965220 Dec 12, 2019
Cargo.toml Outdated Show resolved Hide resolved
@octol octol requested a review from nbaksalyar Dec 12, 2019
@octol octol added this to In progress in Vaults Phase 2a via automation Dec 17, 2019
@octol octol force-pushed the octol:new-bootstrap-handshake-format branch 2 times, most recently from 6b9959c to 731d760 Dec 18, 2019
@octol octol marked this pull request as ready for review Dec 18, 2019
@octol octol force-pushed the octol:new-bootstrap-handshake-format branch from 731d760 to 5bdc963 Dec 18, 2019
@octol octol requested a review from maqi Dec 18, 2019
self.handle_join_request(peer_addr, client_id);
}
Ok(HandshakeRequest::ChallengeResult(signature)) => {
// Handle challenge

This comment has been minimized.

Copy link
@maqi

maqi Dec 18, 2019

Member

comment not required? the function name says it

Some(pk) => pk,
None => {
info!(
"{}: Client on {} identifies as a node: {}",

This comment has been minimized.

Copy link
@maqi

maqi Dec 18, 2019

Member

this log better to be a warn ? also the wording can be :
{}: Client on {} identifies as a node: {}, hence disconnect from it.


if closest_known_elders.is_empty() {
warn!(
"{}: No closest known elders in any section we know of",

This comment has been minimized.

Copy link
@maqi

maqi Dec 18, 2019

Member

this sounds more like meaning this vault knows no elders ?
I'd prefer a log_or_panic here as it's very suspicious.

This comment has been minimized.

Copy link
@octol

octol Dec 19, 2019

Author Contributor

Don't think we use log_or_panic in this crate though

This comment has been minimized.

Copy link
@maqi

maqi Dec 19, 2019

Member

ok, then

self
);
} else {
self.send(peer_addr, &HandshakeResponse::Join(closest_known_elders));

This comment has been minimized.

Copy link
@maqi

maqi Dec 18, 2019

Member

just to confirm, so this means we are sending the HandshakeResponse::Join(closest_known_elders) back to the client,
and he shall try to connect to them, right?

This comment has been minimized.

Copy link
@octol

octol Dec 19, 2019

Author Contributor

That's correct

@@ -94,6 +99,26 @@ impl Node {
Ok(())
}

/// Find out if the given XorName matches our prefix.
pub fn matches_our_prefix(&self, _name: &routing::XorName) -> Result<bool, RoutingError> {

This comment has been minimized.

Copy link
@maqi

maqi Dec 18, 2019

Member

better import XorName in the above use block ?
and same to the other places


// TODO: handle payload
// Currently we are connected to a single vault. At this stage we should disconnect from it
// and connect to the new elders we are given as payload.

This comment has been minimized.

Copy link
@maqi

maqi Dec 18, 2019

Member

Shall we simulate the process even with just one vault?
I am asking this as the self.send(&request) actually only sends to the connected vaults.
which seems the test is not covering the whole procedure?

This comment has been minimized.

Copy link
@octol

octol Dec 19, 2019

Author Contributor

This is phase 2B though for multiple sections?

This comment has been minimized.

Copy link
@maqi

maqi Dec 19, 2019

Member

I mean, this flow indicates that if within the same section, then probably don't need to carry out the disconnection and connect to the new.
So, maybe worth to mention in the comment that:
Disconnect if we are not among the elders ?

This comment has been minimized.

Copy link
@octol

octol Dec 19, 2019

Author Contributor

Good point, I'll update the comment

@octol octol force-pushed the octol:new-bootstrap-handshake-format branch from 5bdc963 to 874cae9 Dec 19, 2019
Vaults Phase 2a automation moved this from In progress to Reviewer approved Dec 19, 2019
@maqi
maqi approved these changes Dec 19, 2019
@jeanphilippeD jeanphilippeD merged commit 51c5add into maidsafe:vNext Dec 19, 2019
4 checks passed
4 checks passed
Rustfmt-Clippy
Details
Test (ubuntu-latest)
Details
Test (windows-latest)
Details
Test (macOS-latest)
Details
Vaults Phase 2a automation moved this from Reviewer approved to Done Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.