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.

Sign up for GitHub

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

Jump to bottom

PNet take 2 #3697

Merged
merged 3 commits into from
Mar 2, 2017
Merged

PNet take 2 #3697

merged 3 commits into from
Mar 2, 2017

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Feb 16, 2017

No description provided.

@Kubuxu Kubuxu added the status/in-progress In progress label Feb 16, 2017
@Kubuxu Kubuxu force-pushed the feat/pnet2 branch 2 times, most recently from a70886b to 74305d4 Compare February 16, 2017 17:06
whyrusleeping
whyrusleeping requested changes Feb 16, 2017
View reviewed changes
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. I'd like to see it squashed into two commits, the first just updating deps, and the second making all the code changes, then i'll review again.

core/commands/swarm.go Outdated
@@ -14,7 +14,7 @@ import (
"github.com/ipfs/go-ipfs/repo/fsrepo"
iaddr "github.com/ipfs/go-ipfs/thirdparty/ipfsaddr"
pstore "gx/ipfs/QmQMQ2RUjnaEEX8ybmrhuFFGhAwPjyL1Eo6ZoJGD7aAccM/go-libp2p-peerstore"
swarm "gx/ipfs/QmY8hduizbuACvYmL4aZQbpFeKhEQJ1Nom2jY6kv6rL8Gf/go-libp2p-swarm"
Copy link
Member

Choose a reason for hiding this comment

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

lets keep all the dep updates in the same commit

@Kubuxu Kubuxu force-pushed the feat/pnet2 branch 2 times, most recently from 2077a9d to b217609 Compare February 17, 2017 12:45
@Kubuxu
Copy link
Member Author

Kubuxu commented Feb 17, 2017

Done.

whyrusleeping
whyrusleeping requested changes Feb 21, 2017
View reviewed changes
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

LGTM, Just one weird merge conflict thing with the dependency updating commit. Gotta make sure every commit at least builds

core/core.go Outdated
@@ -174,7 +176,25 @@ func (n *IpfsNode) startOnlineServices(ctx context.Context, routingOption Routin

tpt := makeSmuxTransport(mplex)

<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

Merge conflict here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry that I have missed it.

core/core.go
case <-t.C:
if ph := n.PeerHost; ph != nil {
if len(ph.Network().Peers()) == 0 {
log.Warning("We are in private network and have no peers.")
Copy link
Member

Choose a reason for hiding this comment

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

Rather unfortunate that these warnings won't show up by default... I wonder if we should look at turning on warning messages by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it might be good idea, there doesn't seem that much of a spam on warning level.

Copy link
Member Author

Choose a reason for hiding this comment

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

I take it back, there is a lot of spam on global warning channel.

Copy link
Member

Choose a reason for hiding this comment

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

Hrm... definitely out of scope for this PR, but we should look at reducing spam there and think about setting the default level to be warning

@Kubuxu Kubuxu force-pushed the feat/pnet2 branch 2 times, most recently from df2ae38 to 4acf869 Compare February 21, 2017 13:41
@Kubuxu
Copy link
Member Author

Kubuxu commented Feb 21, 2017

Fixed the merge conflict and rebased.

@Kubuxu
Copy link
Member Author

Kubuxu commented Feb 21, 2017

Coverage is a bit low, as the lack of peer warning isn't tested. There is no clean way to test it and as it is just a log I don't think it matters.

@Kubuxu Kubuxu added this to the Ipfs 0.4.7 milestone Feb 21, 2017
whyrusleeping
whyrusleeping reviewed Feb 22, 2017
View reviewed changes
cmd/ipfs/daemon.go Outdated
@@ -347,6 +348,11 @@ func daemonFunc(req cmds.Request, res cmds.Response) {
}
node.SetLocal(false)

if node.PNetFingerpint != nil {
fmt.Println("Swarm is limited to private network of peers with the swarm key")
fmt.Printf("Swarm key fingerprint: %s\n", hex.EncodeToString(node.PNetFingerpint))
Copy link
Member

Choose a reason for hiding this comment

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

you can just use %x

@Kubuxu
deps: update dependencies for PNet
4eed546 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
pnet: integrate PNet
faca451 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
pnet: add warning when we are in PNet and have no peers
4ce6358 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
whyrusleeping
whyrusleeping reviewed Feb 22, 2017
View reviewed changes
core/core.go

var DefaultHostOption HostOption = constructPeerHost

// isolates the complex initialization steps
func constructPeerHost(ctx context.Context, id peer.ID, ps pstore.Peerstore, bwr metrics.Reporter, fs []*net.IPNet, tpt smux.Transport) (p2phost.Host, error) {
func constructPeerHost(ctx context.Context, id peer.ID, ps pstore.Peerstore, bwr metrics.Reporter, fs []*net.IPNet, tpt smux.Transport, protec ipnet.Protector) (p2phost.Host, error) {
Copy link
Member

Choose a reason for hiding this comment

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

At some point (not now) I want to make this more simply composed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we should probably change it to be struct instead of function args.

@whyrusleeping
Copy link
Member

LGTM, lets ship this in 0.4.7

@Kubuxu Kubuxu added RFM and removed status/in-progress In progress labels Feb 22, 2017
@Kubuxu Kubuxu mentioned this pull request Feb 28, 2017
Closed
@whyrusleeping whyrusleeping merged commit dd9584b into master Mar 2, 2017
@whyrusleeping whyrusleeping deleted the feat/pnet2 branch March 2, 2017 00:43
@salsa-dev salsa-dev mentioned this pull request Jul 20, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whyrusleeping whyrusleeping whyrusleeping approved these changes

Assignees
No one assigned
Labels
RFM
Projects
None yet
Milestone
Ipfs 0.4.7
Development

Successfully merging this pull request may close these issues.
2 participants
@Kubuxu @whyrusleeping